feat(turns): opt-in async write-behind durability mode for the turn-state row store (#6263 Step 3) - #6298
Conversation
…tate row store (#6263 Step 3) Adds TurnStateDurabilityPolicy { WriteThrough (default), WriteBehind } to FilesystemTurnStateRowStore. WriteThrough is byte-for-byte today's behavior (every mutation awaits the durable ack) — default deployments get zero change and no crash-loss window. This PR adds the capability + proves it + measures it; wiring the volatile profile onto WriteBehind (retiring the raw direct authority) is a separate follow-on. WriteBehind: a mutation whose durable delta is NOT recoverability-critical returns Ok immediately after enqueue (background flush); a critical transition (is_recoverability_critical = is_blocked() || is_terminal(), now a production predicate in status.rs the crash suite references) awaits the ack. Because the journal is a strictly sequential single-writer, awaiting a critical op's ack implies the whole prior async tail is durable — critical ops are natural barriers, so a run can only ever lose trailing non-critical churn, never a gate-park or terminal a human/model waits on. Write-behind robustness (write-through gets these for free): - Backpressure: bounded pending-un-acked delta window (max_pending_write_behind_deltas, default 128); at the cap a non-critical op awaits the oldest ack. Bounds memory + the crash-loss window. - Append-failure halt: the flusher latches degraded + closes on append failure in write-behind (write-through still continues), so no durable gap; mutations then fail fast and reads reload from the rolled-back durable point. Pre-append cross-store-CAS reservations are disabled in write-behind (incompatible: a critical op's reservation can't find rows the async ops never wrote synchronously). - Cache safety: a rejected mutation rebuilds the engine from the cached (unflushed-inclusive) snapshot instead of reload-from-durable, so acked non-critical ops the caller was told succeeded aren't silently dropped. - Journal abort-on-drop so a 'crashed' store can't flush its queued tail post-crash and race a store reopened over the same backend. Crash suite parameterized over both modes (19 tests, 0 ignored): WriteThrough keeps the strict acked-durable projection diff; WriteBehind keeps assert_recoverability_critical_survives strict (gate-park + terminal never lost) and replaces the diff with a legal-prefix check PLUS an anti-cheat convergence test — fork the durable bytes before K acked non-critical ops, recover, verify prefix, then re-apply the K lost ops and assert convergence to the model (loss = redoable work, not corruption). Barrier, append-failure-halt/degrade/recover, and backpressure tests added. Measured (store-isolated per-transition, InMemoryBackend): WriteBehind non-critical writes ~45-190us (vs WriteThrough ~2ms c=1 / ~20ms c=32 durable-ack floor — 10-400x), approaching the direct authority; critical transitions stay synchronously durable in both. Follow-on: graceful drain-before-shutdown when the profile is wired onto WriteBehind (abort-on-drop currently discards the un-flushed tail at drop). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
github-actions
Bot
added
size: L
📝 WalkthroughSummary by CodeRabbit
WalkthroughFilesystem turn-state storage now supports configurable write-through or write-behind durability. Critical transitions remain durable barriers, non-critical acknowledgements use bounded deferral, journal failures enter degraded mode, runtime shutdown drains pending work, and tests cover both policies. ChangesTurn-state durability
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TurnMutation
participant FilesystemTurnStateRowStore
participant DeltaJournal
participant DurableStorage
participant RebornRuntime
TurnMutation->>FilesystemTurnStateRowStore: submit delta
FilesystemTurnStateRowStore->>DeltaJournal: enqueue delta
DeltaJournal->>DurableStorage: append batch
DurableStorage-->>DeltaJournal: durable acknowledgement
FilesystemTurnStateRowStore-->>TurnMutation: barrier or deferred acknowledgement
RebornRuntime->>FilesystemTurnStateRowStore: drain pending acknowledgements
FilesystemTurnStateRowStore-->>RebornRuntime: drain result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an asynchronous WriteBehind durability policy for the FilesystemTurnStateRowStore as an alternative to the default WriteThrough policy. In WriteBehind mode, non-critical transitions return immediately after enqueueing, while recoverability-critical transitions (such as gate-parks and terminal states) act as synchronous durability barriers. The changes also introduce backpressure bounds, append-failure halting, and comprehensive crash-consistency tests. The review feedback highlights a critical performance issue where a mutex lock is held across an expensive I/O operation (DeltaJournal::await_ack) in register_write_behind_commit, which should be optimized by releasing the lock before awaiting. Additionally, it is recommended to use exhaustive enum matching instead of the matches! macro in with_durability_policy and is_write_behind to ensure compile-time safety for future enum variants.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| async fn register_write_behind_commit<T>( | ||
| &self, | ||
| pending: PendingRowCommit<T>, | ||
| ) -> Result<T, TurnError> { | ||
| let PendingRowCommit { | ||
| value, | ||
| ack, | ||
| active_lock_reservations, | ||
| run_row_reservations, | ||
| critical: _, | ||
| } = pending; | ||
| debug_assert!( | ||
| active_lock_reservations.is_empty() && run_row_reservations.is_empty(), | ||
| "write-behind non-critical commits must not pre-append row reservations", | ||
| ); | ||
| let Some(ack) = ack else { | ||
| return Ok(value); | ||
| }; | ||
| let cap = self.limits.max_pending_write_behind_deltas.max(1); | ||
| let mut window = self.pending_write_behind.lock().await; | ||
| while window.len() >= cap { | ||
| let Some(oldest) = window.pop_front() else { | ||
| break; | ||
| }; | ||
| if let Err(error) = DeltaJournal::await_ack(Some(oldest)).await { | ||
| // The oldest pending append failed; the flusher has halted and | ||
| // latched the store degraded. Surface the retryable error and | ||
| // roll the hot cache back to the last consistent durable point. | ||
| drop(window); | ||
| self.clear_snapshot_cache().await; | ||
| return Err(error); | ||
| } | ||
| } | ||
| window.push_back(ack); | ||
| Ok(value) | ||
| } |
There was a problem hiding this comment.
Holding the pending_write_behind lock across the expensive I/O operation DeltaJournal::await_ack inside register_write_behind_commit can serialize concurrent threads and severely degrade throughput under write-behind load.
Following the general rule, we should check the queue length under a brief lock, release the lock to perform the expensive await_ack outside of it, and then re-acquire the lock to push the new ack.
async fn register_write_behind_commit<T>(
&self,
pending: PendingRowCommit<T>,
) -> Result<T, TurnError> {
let PendingRowCommit {
value,
ack,
active_lock_reservations,
run_row_reservations,
critical: _,
} = pending;
debug_assert!(
active_lock_reservations.is_empty() && run_row_reservations.is_empty(),
"write-behind non-critical commits must not pre-append row reservations",
);
let Some(ack) = ack else {
return Ok(value);
};
let cap = self.limits.max_pending_write_behind_deltas.max(1);
loop {
let mut oldest = None;
{
let mut window = self.pending_write_behind.lock().await;
if window.len() < cap {
window.push_back(ack);
break;
}
oldest = window.pop_front();
}
if let Some(oldest_ack) = oldest {
if let Err(error) = DeltaJournal::await_ack(Some(oldest_ack)).await {
// The oldest pending append failed; the flusher has halted and
// latched the store degraded. Surface the retryable error and
// roll the hot cache back to the last consistent durable point.
self.clear_snapshot_cache().await;
return Err(error);
}
}
}
Ok(value)
}References
- Avoid holding a mutex lock across expensive operations, such as I/O or resource construction, inside cache lookups. Instead, check the cache under a brief lock, perform the expensive build outside the lock, and then re-acquire the lock to insert and deduplicate. This prevents serializing concurrent threads and distorting latency measurements in throughput-sensitive code.
| pub fn with_durability_policy(mut self, durability_policy: TurnStateDurabilityPolicy) -> Self { | ||
| self.durability_policy = durability_policy; | ||
| self.delta_journal.set_write_behind(matches!( | ||
| durability_policy, | ||
| TurnStateDurabilityPolicy::WriteBehind | ||
| )); | ||
| self | ||
| } |
There was a problem hiding this comment.
Exhaustive enum matching without wildcards (or the matches! macro) is preferred when classifying enum variants for critical behaviors, ensuring that any future variants trigger a compile-time error rather than silently falling through to a default value.
pub fn with_durability_policy(mut self, durability_policy: TurnStateDurabilityPolicy) -> Self {
self.durability_policy = durability_policy;
let write_behind = match durability_policy {
TurnStateDurabilityPolicy::WriteThrough => false,
TurnStateDurabilityPolicy::WriteBehind => true,
};
self.delta_journal.set_write_behind(write_behind);
self
}References
- Prefer exhaustive enum matching without wildcards (or the matches! macro) when classifying enum variants for critical behaviors, ensuring that any future variants trigger a compile-time error rather than silently falling through to a default value.
| fn is_write_behind(&self) -> bool { | ||
| matches!( | ||
| self.durability_policy, | ||
| TurnStateDurabilityPolicy::WriteBehind | ||
| ) | ||
| } |
There was a problem hiding this comment.
Exhaustive enum matching without wildcards (or the matches! macro) is preferred when classifying enum variants for critical behaviors, ensuring that any future variants trigger a compile-time error rather than silently falling through to a default value.
fn is_write_behind(&self) -> bool {
match self.durability_policy {
TurnStateDurabilityPolicy::WriteThrough => false,
TurnStateDurabilityPolicy::WriteBehind => true,
}
}References
- Prefer exhaustive enum matching without wildcards (or the matches! macro) when classifying enum variants for critical behaviors, ensuring that any future variants trigger a compile-time error rather than silently falling through to a default value.
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 3 | 0 | 3 | c6b39ba3f0d7 |
Head: c6b39ba3f0d7fe91fb3ac23d5ce8f67bec5e327a
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Found three write-behind correctness issues: successful non-critical writes are not locally readable yet, running cancellations can be lost across a crash, and the backpressure cap does not bound concurrent enqueues.
Findings
Blocking: 3 / Notes: 0
Blocking findings
1. ❌ [HIGH] Running cancellation requests can be lost after reporting success
Location: crates/ironclaw_turns/src/status.rs:76
request_cancel transitions a running run to CancelRequested, which this predicate classifies as non-critical. Write-behind therefore returns success before the cancel state and its idempotency record are durable. A crash in that window recovers the prior Running/Queued state and may execute work that the caller successfully cancelled. Treat CancelRequested as a durability barrier (or override this operation) and add a crash test for cancelling a running run.
2. ❌ [MEDIUM] Write-behind violates read-your-writes for run-state reads
Location: crates/ironclaw_turns/src/filesystem_store/row_store.rs:392
A non-critical submit/claim now returns here before its journal append. However, TurnStateStore::get_run_state still reads and materializes only durable rows, so an immediate same-store read can return ScopeNotFound (or the previous status) until the flusher runs. Route local reads through the hot snapshot while healthy, or explicitly await the relevant ack before exposing these writes; add an immediate submit→get-state test.
3. ❌ [MEDIUM] The backpressure cap is applied after unbounded enqueue
Location: crates/ironclaw_turns/src/filesystem_store/row_store.rs:424
The capacity check occurs after apply has already enqueued the delta into DeltaJournal's unbounded channel. With a stalled flusher and cap=1, one caller waits here, but concurrent callers can each acquire snapshot_state, enqueue another delta, then block on this mutex. The queued deltas can therefore grow without bound and are all lost on a crash, contrary to the advertised memory/crash-window bound. Reserve or await capacity before enqueueing, and test concurrent writers with a stalled append.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
| /// and the crash-consistency suite references THIS function (not a copy) as the | ||
| /// single boundary write-behind flips. | ||
| pub fn is_recoverability_critical(status: TurnStatus) -> bool { | ||
| status.is_blocked() || status.is_terminal() |
There was a problem hiding this comment.
CancelRequested is non-critical here, so a successful cancellation of a running run can be dropped before the write-behind append. After restart the older Running/Queued state can execute despite the acknowledged cancel. Make this transition a durability barrier and add a crash test.
| // No durable delta (no-op / empty commit): nothing to flush. | ||
| return Ok(pending.value); | ||
| } | ||
| if self.write_behind_async(pending.critical) { |
There was a problem hiding this comment.
This returns a non-critical submit/claim before the append, but get_run_state still reads only durable rows. An immediate same-store get can therefore report ScopeNotFound or stale status until the flusher runs. Preserve local read-your-writes or test/document an intentional consistency boundary.
| return Ok(value); | ||
| }; | ||
| let cap = self.limits.max_pending_write_behind_deltas.max(1); | ||
| let mut window = self.pending_write_behind.lock().await; |
There was a problem hiding this comment.
This cap is checked only after the delta was put on the unbounded journal channel. While one caller waits for an old ack, concurrent callers can still enqueue their deltas before blocking on this mutex, so memory and the actual queued tail are not bounded. Gate capacity before enqueueing.
|
🚅 Deployed to the ironclaw-pr-6298 environment in ironclaw-ci-preview
|
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 86.21% — 319799 / 370944 lines Per-crate breakdown (65 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (3 entry/entries excluded from the accounting above)
|
All three are write-behind-only; WriteThrough is unaffected. The crash suite (now 22 tests) passes in both modes; each fix ships a regression test verified to fail before the fix. f1 [HIGH] — CancelRequested must be a durability barrier. `request_cancel` reports success once the transition commits, so a write-behind crash that reverts a Running run's acked CancelRequested to Running re-executes work the caller was told was cancelled (and drops its idempotency record). Add `CancelRequested` to `is_recoverability_critical` (the single boundary the row store and the crash-suite oracle both key on), so the cancel awaits its durable ack. Test: `write_behind_cancel_of_running_run_survives_crash` (without the fix the whole un-flushed tail is lost → ScopeNotFound). f2 [MED] — read-your-writes. `get_run_state` read only durable rows, but a non-critical write-behind submit returns Ok after updating the hot snapshot and before its durable append, so an immediate same-store read returned ScopeNotFound/stale. Serve `get_run_state` from the hot snapshot under healthy write-behind (the single-writer authority); write-through and the degraded path still read durable. Renames `read_run_state_for_cancellation` → `read_run_state_from_hot_cache` (it was never cancellation-specific). Test: `write_behind_get_run_state_reflects_unflushed_submit`. f3 [MED] — backpressure was applied AFTER an unbounded enqueue: concurrent callers each enqueued into the journal channel, then serialized on the pending window, so a stalled flusher let the channel grow without bound. Reserve the pending-window slot and track the ack BEFORE/around the enqueue, both inside `apply` under the `snapshot_state` lock that serializes enqueue, so the channel can never exceed the cap. `register_write_behind_commit` is removed; `commit_pending` short-circuits on the now-`None` ack. Test: `write_behind_concurrent_writers_under_cap_stay_consistent` exercises the concurrent reserve→enqueue→track path (the strict peak-depth bound is structural — the journal channel length is not externally observable). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed all three IronLoop blocking findings ( f1 [HIGH] — f2 [MED] — read-your-writes. f3 [MED] — backpressure before enqueue. The pending-window slot is now reserved, and the ack tracked, before/around the journal enqueue — both inside One honesty note on f3: the strict peak-depth bound is enforced structurally (reserve-before-enqueue under the serializing lock) rather than asserted by the test — the journal channel length isn't externally observable without a test-only introspection hook, which I didn't add. Happy to add one if you'd prefer a depth assertion. Ready for merge once CI greens. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_turns/src/filesystem_store/row_store/traits.rs (1)
373-411: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
put_loop_checkpoint's ack bypasses the write-behind deferral path — triggers thecommit_pendingdebug_assert and defeats the documented lazy-flush design.Unlike
apply/apply_with_targeted_delta, this path never callsreserve_write_behind_slot()before enqueue nor routes the resultingackthroughtrack_write_behind_ack_if_async(). It passes the rawSome(ack)straight intoPendingRowCommit { critical: false, .. }and callscommit_pending.Under
WriteBehind,commit_pendingseesack.is_some()andcritical == false, sowrite_behind_async(false)istrue, and the guard fires:debug_assert!( !self.write_behind_async(pending.critical), "non-critical write-behind commit must be tracked in apply, not awaited", );This is
debug_assert!(false, ...)— panics in debug/test builds. In release builds it silently falls through to a synchronous await, contradicting the same function's own comment: "under write-behind it lazy-flushes." Comments/guarantees must match the code — as per coding guidelines, "Comments and documentation that promise guarantees must match the code and tests."🐛 Proposed fix: route through the same reserve/track pattern as apply()
let ack = { let mut guard = self.snapshot_state.lock().await; + if self.write_behind_async(false) + && let Err(error) = self.reserve_write_behind_slot().await + { + *guard = None; + return Err(error); + } let ack = self .enqueue_delta(row_store_durable_delta(delta.clone())) .map_err(|error| match error { RowPersistError::Turn(error) => error, })?; if let Some(state) = guard.as_mut() && let Err(error) = state.apply_delta(delta, state.journal_seq) { *guard = None; return Err(error); } - ack + self.track_write_behind_ack_if_async(false, ack).await };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_turns/src/filesystem_store/row_store/traits.rs` around lines 373 - 411, Update put_loop_checkpoint to follow the apply/apply_with_targeted_delta write-behind flow: reserve a write-behind slot before enqueueing the delta, then pass the resulting acknowledgment through track_write_behind_ack_if_async before constructing PendingRowCommit. Preserve critical: false so non-critical checkpoints remain lazily flushed without triggering commit_pending’s assertion.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/ironclaw_turns/src/filesystem_store/row_store/traits.rs`:
- Around line 373-411: Update put_loop_checkpoint to follow the
apply/apply_with_targeted_delta write-behind flow: reserve a write-behind slot
before enqueueing the delta, then pass the resulting acknowledgment through
track_write_behind_ack_if_async before constructing PendingRowCommit. Preserve
critical: false so non-critical checkpoints remain lazily flushed without
triggering commit_pending’s assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ba4c596b-6a04-47d2-8bfd-f141547191a3
📒 Files selected for processing (4)
crates/ironclaw_turns/src/filesystem_store/row_store.rscrates/ironclaw_turns/src/filesystem_store/row_store/traits.rscrates/ironclaw_turns/src/status.rscrates/ironclaw_turns/tests/row_store_crash_consistency.rs
|
@ironloopai review |
|
✅ Ready for merge. CI is fully green (59 pass, 0 fail) on the fix head All three IronLoop blocking findings are fixed (
Crash suite now 22 tests / 0 ignored, green in both WriteThrough and WriteBehind; |
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 1 | 0 | 1 | 7d5be70415e7 |
Head: 7d5be70415e72b7490af8783ec585afa5ac697bf
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Write-behind checkpoints panic in debug builds and do not take the intended async path in release builds.
Findings
Blocking: 1 / Notes: 0
Blocking findings
1. ❌ [MEDIUM] Write-behind checkpoints trip a debug assertion
Location: crates/ironclaw_turns/src/filesystem_store/row_store.rs:408-410
put_loop_checkpoint supplies critical: false with a live ack directly to commit_pending, without reserving/tracking that ack through track_write_behind_ack_if_async. Under WriteBehind, this assertion fires on every checkpoint in debug/test builds; in release it instead waits synchronously, contradicting the intended path. The runner checkpoint adapter calls this method for normal checkpoints. Either keep checkpoints synchronous by marking them critical, or implement the same reserve/enqueue/track flow and read semantics as other async commits; add a WriteBehind checkpoint test.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
| // A durable ack is present ⇒ write-through, or a critical write-behind | ||
| // barrier: await it. A non-critical write-behind commit never reaches | ||
| // here — `track_write_behind_ack_if_async` returned `ack: None` above. | ||
| debug_assert!( |
There was a problem hiding this comment.
put_loop_checkpoint reaches this with critical: false and ack: Some(_), but it never calls track_write_behind_ack_if_async. That panics in debug builds under WriteBehind (and blocks synchronously in release). Please either keep checkpoint writes synchronous/critical or route them through the reserved+tracked async path, with a WriteBehind checkpoint regression test.
…path (#6298 IronLoop f4) The f3 backpressure fix added a `commit_pending` debug assertion that a non-critical write-behind commit is never awaited there — it must be reserved+tracked in `apply`. But `put_loop_checkpoint` enqueues under `snapshot_state` and hands a live ack straight to `commit_pending` with `critical: false`, bypassing that flow. Under WriteBehind it therefore tripped the assertion in debug/test builds, and in release waited synchronously — contradicting the intended lazy flush for a non-critical checkpoint (and leaving the same unbounded-channel gap f3 closed elsewhere). Give the checkpoint enqueue the same reserve→enqueue→track flow every other async commit uses: reserve the pending-window slot before enqueue (under `snapshot_state`), then `track_write_behind_ack_if_async` returns `None` so `commit_pending` lazy-flushes. WriteThrough awaits the ack unchanged. Regression: `write_behind_put_loop_checkpoint_takes_async_path` drives the real `put_loop_checkpoint` under WriteBehind — verified it panics on the assertion before this fix. Crash suite now 23 tests / 0 ignored, green in both modes; clippy clean (all-features + default). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
size: XL
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_turns/src/filesystem_store/row_store/traits.rs (1)
217-224: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRestore the durable-row fallback for write-through and degraded modes.
The AI summary claims
get_run_state_for_cancellationconditionally reads from the hot cache, but the implementation unconditionally reads from it. InWriteThroughmode or when the journal degrades (clearing the cache), this will incorrectly fail withScopeNotFoundinstead of falling back to durable rows, which breaks cancellations. Match the conditional logic used inget_run_state.🐛 Proposed fix
async fn get_run_state_for_cancellation( &self, request: GetRunStateRequest, ) -> Result<TurnRunState, TurnError> { - self.read_run_state_from_hot_cache(&request) - .await? - .ok_or(TurnError::ScopeNotFound) + let state = if self.is_write_behind_healthy() { + self.read_run_state_from_hot_cache(&request).await? + } else { + self.read_run_state_from_durable_rows(&request).await? + }; + state.ok_or(TurnError::ScopeNotFound) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_turns/src/filesystem_store/row_store/traits.rs` around lines 217 - 224, Update get_run_state_for_cancellation to match the conditional cache-and-durable-row lookup logic used by get_run_state, including fallback to durable rows in WriteThrough mode and when the journal has degraded or cleared the hot cache; preserve TurnError::ScopeNotFound only when neither source contains the run state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_turns/src/filesystem_store/row_store/traits.rs`:
- Around line 393-398: In the write-behind reservation failure branch, remove
the cache invalidation assignment to `guard` and propagate the error from
`reserve_write_behind_slot().await` directly. Keep the existing backpressure
check and `Err(error)` return behavior unchanged.
---
Outside diff comments:
In `@crates/ironclaw_turns/src/filesystem_store/row_store/traits.rs`:
- Around line 217-224: Update get_run_state_for_cancellation to match the
conditional cache-and-durable-row lookup logic used by get_run_state, including
fallback to durable rows in WriteThrough mode and when the journal has degraded
or cleared the hot cache; preserve TurnError::ScopeNotFound only when neither
source contains the run state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fb6948e0-5686-4b24-86f1-6eefc50bf136
📒 Files selected for processing (2)
crates/ironclaw_turns/src/filesystem_store/row_store/traits.rscrates/ironclaw_turns/tests/row_store_crash_consistency.rs
| if self.write_behind_async(CHECKPOINT_CRITICAL) | ||
| && let Err(error) = self.reserve_write_behind_slot().await | ||
| { | ||
| *guard = None; | ||
| return Err(error); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Remove cache invalidation on backpressure reservation failure.
Under write-behind, reservation failure acts as a backpressure mechanism. Destroying the hot cache (*guard = None) before any mutation has occurred forces subsequent operations to rebuild from durable rows. This creates a severe negative performance feedback loop precisely when the system is under heavy load. Propagate the error without invalidating the cache.
⚡ Proposed fix
if self.write_behind_async(CHECKPOINT_CRITICAL)
&& let Err(error) = self.reserve_write_behind_slot().await
{
- *guard = None;
return Err(error);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if self.write_behind_async(CHECKPOINT_CRITICAL) | |
| && let Err(error) = self.reserve_write_behind_slot().await | |
| { | |
| *guard = None; | |
| return Err(error); | |
| } | |
| if self.write_behind_async(CHECKPOINT_CRITICAL) | |
| && let Err(error) = self.reserve_write_behind_slot().await | |
| { | |
| return Err(error); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_turns/src/filesystem_store/row_store/traits.rs` around lines
393 - 398, In the write-behind reservation failure branch, remove the cache
invalidation assignment to `guard` and propagate the error from
`reserve_write_behind_slot().await` directly. Keep the existing backpressure
check and `Err(error)` return behavior unchanged.
|
Addressed the re-review's new finding ( f4 [MED] — Fix: give the checkpoint enqueue the same reserve→enqueue→track flow every other async commit uses (reserve the slot before enqueue under Regression: @ironloopai review |
Import reordering only — the f4 regression-test imports were added out of sorted order and tripped the Formatting / Code Style lane. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ry paths (#6263 Step 3) The runtime does submit_turn -> get_run_state, but get_run_state (and read_turn_events_after / get_loop_checkpoint) read materialized durable rows. Under WriteBehind a submit's delta materializes asynchronously, so the immediate read saw stale/missing rows and returned ScopeNotFound — every runtime turn would fail. (WriteThrough was unaffected: durable == cache there.) The crash suite never caught it because it only ever read AFTER recovery, never live after an async write. The naive fix (serve reads from the hot cache) is WRONG and loses data: the cache is a bounded window, not a superset of durable state — terminal runs/events are evicted (max_terminal_records / max_events) while durable rows retain them, and cross-writer freshness needs the durable read. Four WriteThrough contract tests pin exactly this. Fix: a read-side barrier. flush_pending_write_behind_for_read() drains the enqueued-but-un-acked non-critical pending window (awaiting those durable appends) at the top of the three durable-read helpers, so durable rows catch up to the just-acked writes BEFORE the read — read-your-writes consistent, while the durable read keeps its exact eviction / cross-writer-freshness / scope / cursor / retention semantics. No-op under WriteThrough (window always empty), so that path is byte-for-byte unchanged. get_run_state_for_cancellation and get_run_record already read the cache and were WB-safe; traits.rs untouched. Coverage gap closed: live read-your-writes tests in BOTH modes (submit -> immediate get_run_state/get_run_record/read_turn_events_after, put -> get_loop_checkpoint) plus a property test asserting live get_run_state tracks the model after every op. Reverting only the source reproduces model=Queued vs store=ScopeNotFound under WriteBehind. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_turns/src/filesystem_store/row_store.rs`:
- Around line 458-502: The read barrier in flush_pending_write_behind_for_read
must be shared by concurrent readers rather than allowing each reader to drain
pending_write_behind independently. Add synchronization so one reader performs
the drain and await, while concurrent readers wait for that same barrier result
before proceeding to durable reads; preserve the existing error handling that
clears snapshot cache and returns the retryable error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 796e2b2f-f643-4083-b842-6edad4e23b13
📒 Files selected for processing (2)
crates/ironclaw_turns/src/filesystem_store/row_store.rscrates/ironclaw_turns/tests/row_store_crash_consistency.rs
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
1 similar comment
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
#6298 f5/f6) f5 [HIGH] — `BeforeSideEffect` checkpoints must be synchronous. f4 made ALL `put_loop_checkpoint` writes async under write-behind, but a `BeforeSideEffect` checkpoint is written immediately before a capability's external side effect, and expired-lease recovery treats the ABSENCE of a durable checkpoint as proof no side effect ran (and requeues). If it lazy-flushed, a crash after the capability ran but before the flush would replay a non-idempotent side effect. Key the checkpoint's criticality on its kind: `BeforeSideEffect` → critical (synchronous, durable before the capability executes); other kinds stay async (a gate-park's checkpoint is flushed by the block transition's barrier; a lost BeforeModel is redoable). Test: `write_behind_before_side_effect_checkpoint_survives_crash`. f6 [MED] — evicted-but-durable terminals hidden. f2 routed healthy-write-behind `get_run_state` through the bounded hot snapshot, which evicts OLD TERMINAL runs while their durable rows persist and must stay queryable (the eviction contract). Once `max_terminal_records` is exceeded the hot-cache read returned `ScopeNotFound` for those durable terminals. On a hot-cache miss, flush the pending write-behind tail (read-your-writes) then fall back to the durable-row lookup. Test: `write_behind_get_run_state_finds_evicted_terminal_via_durable_fallback`. Both verified failing before the fix. Crash suite now 27 tests / 0 ignored, green in both modes; clippy clean (all-features + default); fmt clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the two new findings ( f5 [HIGH] — f6 [MED] — evicted-but-durable terminals hidden. f2 routed healthy-write-behind Both verified failing before the fix. Crash suite 28/0-ignored green in both modes; clippy clean (all-features + default); fmt clean. @ironloopai review |
There was a problem hiding this comment.
⏭️ IronLoop Review Declined: reviewer
Review at a glance
| Disposition | Head |
|---|---|
| ⏭️ Review declined | d218e328e45c |
Head: d218e328e45c669262403557ab9514127963fe1d
Reason: The generated/vendor payload dominates the diff and prevents an auditable full review of all changed content.
Next: Remove or split the generated frontend node_modules payload into a separately auditable change, then rerun review on the focused source diff.
Run details
Status: Current
Trustworthy review produced: no
Summary
Skipped: the supplied base-to-head comparison is vendor/generated-heavy (4,079 files and 1,385,483 additions; 4,058 paths are frontend node_modules), so a reliable complete review is not feasible within scope.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
1 similar comment
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 2566-2575: In RebornRuntime::shutdown, change the turn_state_flush
drain failure log from warn! to debug!, preserving the existing %error field and
message. Keep the diagnostic internal and consistent with the nearby
wait_for_terminal_or_gate logging rule for REPL/TUI-reachable runtime paths.
In `@crates/ironclaw_reborn_composition/tests/runtime.rs`:
- Around line 303-312: Update the assertion message and shutdown comment near
the turn completion check to refer to the store’s WriteThrough policy, not
WriteBehind. Describe shutdown as using the WriteThrough default and its no-op
drain, while preserving the existing assertion and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| #[cfg(feature = "inmemory-turn-state")] | ||
| if let Some(turn_state) = &self.turn_state_flush { | ||
| turn_state.flush().await; | ||
| if let Some(turn_state) = &self.turn_state_flush | ||
| && let Err(error) = turn_state.drain().await | ||
| { | ||
| tracing::warn!( | ||
| %error, | ||
| "turn-state WriteBehind drain failed during graceful shutdown; the un-acked \ | ||
| non-critical tail may not be durable on restart" | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
warn! on the shutdown-drain path contradicts the REPL/TUI logging rule.
RebornRuntime::shutdown is REPL/TUI-reachable, and this same impl block already chose debug! over warn! for exactly this reason (wait_for_terminal_or_gate, ~Line 2745: "debug! not warn! per the logging rule — this runtime is REPL/TUI-reachable"). A degraded-drain diagnostic is internal, not intentionally-rendered user status, so prefer debug! here for consistency.
As per path instructions: "REPL/TUI logging: info!/warn! corrupt the terminal UI — internal diagnostics use debug!".
Proposed change
- tracing::warn!(
+ tracing::debug!(
%error,
"turn-state WriteBehind drain failed during graceful shutdown; the un-acked \
non-critical tail may not be durable on restart"
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[cfg(feature = "inmemory-turn-state")] | |
| if let Some(turn_state) = &self.turn_state_flush { | |
| turn_state.flush().await; | |
| if let Some(turn_state) = &self.turn_state_flush | |
| && let Err(error) = turn_state.drain().await | |
| { | |
| tracing::warn!( | |
| %error, | |
| "turn-state WriteBehind drain failed during graceful shutdown; the un-acked \ | |
| non-critical tail may not be durable on restart" | |
| ); | |
| } | |
| #[cfg(feature = "inmemory-turn-state")] | |
| if let Some(turn_state) = &self.turn_state_flush | |
| && let Err(error) = turn_state.drain().await | |
| { | |
| tracing::debug!( | |
| %error, | |
| "turn-state WriteBehind drain failed during graceful shutdown; the un-acked \ | |
| non-critical tail may not be durable on restart" | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_composition/src/runtime.rs` around lines 2566 - 2575,
In RebornRuntime::shutdown, change the turn_state_flush drain failure log from
warn! to debug!, preserving the existing %error field and message. Keep the
diagnostic internal and consistent with the nearby wait_for_terminal_or_gate
logging rule for REPL/TUI-reachable runtime paths.
Source: Path instructions
| assert_eq!( | ||
| reply.status, | ||
| TurnStatus::Completed, | ||
| "turn must complete over the WriteBehind store, got {:?} ({:?})", | ||
| reply.status, | ||
| reply.failure_category | ||
| ); | ||
|
|
||
| // Graceful shutdown drains the WriteBehind tail through | ||
| // `FilesystemTurnStateStoreKind::drain`; a broken drain wiring surfaces here. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Assertion message and comment say "WriteBehind" but this store ships WriteThrough.
The test's own doc comment states the profile ships the row store at the WriteThrough default and the drain is a no-op here, yet the assert message ("turn must complete over the WriteBehind store") and the shutdown comment ("drains the WriteBehind tail") claim otherwise. Align the wording with the actual policy so a future reader isn't misled about what is exercised.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_composition/tests/runtime.rs` around lines 303 - 312,
Update the assertion message and shutdown comment near the turn completion check
to refer to the store’s WriteThrough policy, not WriteBehind. Describe shutdown
as using the WriteThrough default and its no-op drain, while preserving the
existing assertion and behavior.
…6298/#6305 IronLoop) `crates/ironclaw_webui_v2/frontend/node_modules` was committed with no source, manifest, lockfile, or Cargo.toml — the directory holds *only* vendored deps and the crate is not even a workspace member. The 4,058-file / ~1.38M-line payload dominated the diff and caused IronLoop to decline the review as unauditable. Remove all 4,058 tracked node_modules files (working tree untouched) and add a `.gitignore` rule so it can't be re-added. No source or build change: nothing references or builds this vendored tree. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Removed the vendored payload that made IronLoop decline the review (
For reference, all six write-behind findings from earlier heads are fixed with failing-first regression tests (crash suite 28/0-ignored, both modes): f1 CancelRequested-critical, f2 read-your-writes, f3 backpressure-before-enqueue, f4 put_loop_checkpoint async path, f5 BeforeSideEffect-checkpoint-critical, f6 evicted-terminal durable fallback. @ironloopai review |
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 1 | 0 | 1 | 9c91db450b2a |
Head: 9c91db450b2a6275e03eacfa29d0ff5e15f4c660
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Write-behind backpressure can drop tracking for an already-successful mutation when the flusher stalls, defeating the pending-window bound and graceful drain guarantee.
Findings
Blocking: 1 / Notes: 0
Blocking findings
1. ❌ [HIGH] Preserve pending acknowledgements when backpressure times out
Location: crates/ironclaw_turns/src/filesystem_store/row_store.rs:457-460
pop_front() removes the oldest successful write's ack before awaiting it, but this call runs inside apply's 15-second outer timeout. If the flusher is stalled, that timeout cancels this future and drops the ack permanently. Later writes see an empty window and can enqueue behind the stalled append, so the channel/loss window is again unbounded; drain() can also return Ok while those acknowledged writes are still in flight, and dropping the store then loses them. Keep the ack tracked until it resolves (including cancellation/timeout paths), and add a stalled-flusher regression that verifies drain does not falsely succeed.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
| let cap = self.limits.max_pending_write_behind_deltas.max(1); | ||
| let mut window = self.pending_write_behind.lock().await; | ||
| while window.len() >= cap { | ||
| let Some(oldest) = window.pop_front() else { |
There was a problem hiding this comment.
pop_front() removes the ack before an await enclosed by apply's outer timeout. If the flusher is stalled, cancellation drops this ack permanently; later writes bypass the cap and drain() can falsely report success before an already-acknowledged write is durable. Preserve/reinsert the ack until it resolves, including timeout paths.
Vendored node_modules trees kept getting accidentally committed (the webui_v2 frontend payload; likely more as frontends land). Replace the single-path rule with a bare `node_modules/` (no leading slash), which git matches at ANY depth — so every current and future frontend/tooling node_modules is ignored, not just the one path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…6298 IronLoop f7) `reserve_write_behind_slot` (backpressure) and `flush_pending_write_behind_for_read` (read barrier) removed the oldest ack from the pending window BEFORE awaiting it — `pop_front()` and `drain(..).collect()` respectively. Both run under a caller that can cancel them (apply's outer timeout; a dropped read future). A cancellation then dropped the removed ack permanently, so: (1) later writes saw an empty window and enqueued behind a stalled append — the channel/loss window unbounded again; and (2) a subsequent flush found nothing and falsely reported success while the acknowledged write was still un-appended (lost on the next store drop). Await each ack IN PLACE instead — peek the front, await it by `&mut` (new `DeltaJournal::await_ack_ref`; a `oneshot::Receiver` is `Future + Unpin`, so awaiting `&mut` it doesn't consume it), and remove it only once it resolves. A cancelled await now leaves the ack tracked. The flush holds the window lock across its awaits (bounded by the pending cap; the journal flusher doesn't take that lock, so no deadlock), which also keeps its set to the writes present when the flush began. Regression: `write_behind_cancelled_flush_preserves_pending_ack` — under a stalled flusher (new FaultBackend append gate), a timed-out first flush must leave the ack so a second flush still blocks rather than falsely succeeding. Verified failing before the fix. Crash suite 29/0-ignored green in both modes; clippy clean (all-features + default); fmt clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the HIGH finding ( f7 [HIGH] — preserve pending acks across cancellation. Fix: await each ack in place — peek the front, await it by Regression: That's f1–f7 now, each with a failing-first regression test. @ironloopai review |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_turns/src/filesystem_store/row_store.rs (1)
271-280: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winApply the cancellation-safe await pattern to
drain().The
#6298 IronLoop f7cancellation-safety fix was applied toreserve_write_behind_slotandflush_pending_write_behind_for_read, butdrain()still pops the ack before awaiting it. If a caller times out while awaitingdrain(), the pending ack is dropped and the write-behind guarantee is broken. Use the samefront_mut()andawait_ack_refpattern here.🛠️ Proposed fix
pub async fn drain(&self) -> Result<(), TurnError> { let mut window = self.pending_write_behind.lock().await; - while let Some(ack) = window.pop_front() { - if let Err(error) = DeltaJournal::await_ack(Some(ack)).await { + while let Some(front) = window.front_mut() { + let result = DeltaJournal::await_ack_ref(front).await; + window.pop_front(); + if let Err(error) = result { drop(window); self.clear_snapshot_cache().await; return Err(error); } } Ok(()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_turns/src/filesystem_store/row_store.rs` around lines 271 - 280, Update drain() to await the queue’s front acknowledgment without removing it first, using front_mut() with DeltaJournal::await_ack_ref. Pop the acknowledgment only after the await succeeds, preserving the existing snapshot-cache clearing and error propagation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/ironclaw_turns/src/filesystem_store/row_store.rs`:
- Around line 271-280: Update drain() to await the queue’s front acknowledgment
without removing it first, using front_mut() with DeltaJournal::await_ack_ref.
Pop the acknowledgment only after the await succeeds, preserving the existing
snapshot-cache clearing and error propagation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3ec56e0d-a398-4d6e-a7c1-51d6b65f4527
📒 Files selected for processing (3)
crates/ironclaw_turns/src/filesystem_store/row_store.rscrates/ironclaw_turns/src/filesystem_store/row_store/journal.rscrates/ironclaw_turns/tests/row_store_crash_consistency.rs
There was a problem hiding this comment.
✅ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ✅ Approved | 0 | 2 | 2 | d3810e0b1fdd |
Head: d3810e0b1fddd5d673ed9004e7322c27366fd92b
Next: No reviewer action needed.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
No blocking correctness, durability, or concurrency defect was identified in the exact PR diff. Two non-blocking corrections are needed in the committed benchmark report, which is stale and contradicts its accompanying data/current implementation.
Findings
Blocking: 0 / Notes: 2
Non-blocking notes (2)
1. 💬 [LOW] Correct the benchmark's claim that all failures are admission shedding
Location: tools/ironclaw_stress/results/2026-07-20-turn-state-write-behind-libsql/README.md:59-64
The committed JSONL contradicts this conclusion. Write-behind has non-admission failures at c32/c64/c100 (turn_claim_miss, turn_conflict, turn_scope_not_found, and turn_unavailable), so the report should not characterize every failed operation as turn_thread_busy or rule out store failures without investigating those outcomes.
2. 💬 [LOW] Remove the stale WriteBehind read-after-write blocker description
Location: tools/ironclaw_stress/results/2026-07-20-turn-state-write-behind-libsql/README.md:74-84
This is no longer true in this PR: get_run_state now serves the healthy write-behind hot snapshot and only falls back to durable rows after flushing pending writes. Update this report and its production-wiring TODO so they do not state that every WriteBehind turn fails with ScopeNotFound.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
| Queued, claim = Running) no longer block on the libSQL fsync/ack; only | ||
| gate-park + terminal barriers do. The store-tier speedup this flip was meant | ||
| to capture is real on the durable backend, not just `InMemoryBackend`. | ||
| 2. **No livelock, either policy.** Zero `CAS-retries-exhausted`; the row store |
There was a problem hiding this comment.
The committed JSONL contradicts this conclusion: c32/c64/c100 include turn_claim_miss, turn_conflict, turn_scope_not_found, and (at c100) turn_unavailable, not only turn_thread_busy. Please investigate or qualify these as store/runtime failures rather than reporting all failed operations as expected admission shedding.
| store at `WriteThrough`, not `WriteBehind`**, because WriteBehind has a | ||
| runtime-breaking read-after-write defect at the store tier: | ||
|
|
||
| - `FilesystemTurnStateRowStore::get_run_state` (and the other durable-read query |
There was a problem hiding this comment.
This blocker description is stale in this PR. get_run_state now serves the healthy write-behind hot snapshot and falls back after flushing pending writes, so it no longer necessarily returns ScopeNotFound after submit. Please update this report and the related TODO.
|
✅ Ready for merge. IronLoop Approved (0 blocking) on the current head All seven IronLoop findings across this PR's life are fixed, each with a regression test verified to fail before the fix (crash suite 19 → 29, green in both WriteThrough and WriteBehind):
Plus the accidental webui_v2 node_modules payload removed (−1.38M lines) so the diff is auditable, and The 2 remaining notes are non-blocking LOW items on the |
This branch was successfully deployed
1 participant
Step 3 of the turn-state consolidation (#6263). Adds an opt-in async write-behind mode to
FilesystemTurnStateRowStore, gated by the crash-consistency chaos suite (#6295) running in both modes. Capability only — no production wiring changes here; flipping the volatile profile onto it (retiring the raw direct authority) is a separate follow-on.What
TurnStateDurabilityPolicy { WriteThrough (default), WriteBehind }.Okimmediately after enqueue (background flush); a critical transition awaits the ack. The boundaryis_recoverability_critical = is_blocked() || is_terminal()is now a production predicate the suite references.Critical ops are free durability barriers. The journal is a strictly sequential single-writer, so awaiting a critical op's ack implies the entire prior async tail is durable. A run can therefore only ever lose trailing non-critical churn (Queued/Running) — never a gate-park or terminal a human or the model is waiting on.
Write-behind robustness (write-through gets these free)
max_pending_write_behind_deltas, default 128); at the cap a non-critical op awaits the oldest ack. Bounds memory + the crash-loss window.Acceptance gate — the crash suite, both modes (19 tests, 0 ignored)
assert_recoverability_critical_survivesstays strict (gate-park + terminal never lost, gate ref + failure detail survive); the full diff becomes a legal-prefix check plus the anti-cheat convergence test — fork the durable bytes before K acked non-critical ops, recover, verify the recovered state is a consistent legal prefix, then re-apply the K lost ops and assert it converges to the model exactly. That proves the loss is redoable work, not corruption, and stops the invariant being weakened to pass a buggy impl. Barrier, append-failure-halt/degrade/recover, and backpressure tests added.Measured (store-isolated per-transition, InMemoryBackend)
WriteBehind's non-critical writes approach memory speed (~45–190µs, vs write-through's
2ms→20ms durable-ack floor — 10–400×) while critical transitions stay synchronously durable. The heaviercompleteunder load is the expected barrier cost (it flushes the accumulated async tail), amortized because non-critical ops dominate.Tests
ironclaw_turns612 passed; crash suite 19 both modes, 0 ignored;ironclaw_runner --features filesystem-goal-storegreen; clippy-D warningsclean; fmt + pre-commit-safety clean. WriteThrough and all pre-existing #6284 crash-recovery tests untouched.Follow-on: graceful drain-before-shutdown when the profile is wired onto WriteBehind (abort-on-drop currently discards the un-flushed tail at drop).
🤖 Generated with Claude Code