perf(turns): long-lived authority + remove redundant global commit_gate from the turn-state row store (#6263 Step 1) - #6281
Conversation
…mit_gate from the row store (#6263 Step 1) FilesystemTurnStateRowStore's mutation path carried a process-wide `commit_gate` async mutex in addition to the `snapshot_state` mutex it already held across the read-seq -> apply -> enqueue window. Because production wires a single shared row-store instance, that redundant global lock serialized every user's turn-state transition. This removes `commit_gate` entirely: `snapshot_state` is the single serialization point (held for microseconds — CPU apply + delta build + mpsc send, never across the durable ack), and journal-sequence ordering is preserved because enqueue order already equals reservation-seq order under that lock. Also stops the per-op full engine rebuild on the non-hot `apply()` path (retry/submit_child/tree/recover/fail/relinquish/record_model_route): the long-lived cached InMemoryTurnStateStore authority is now reused in place for the None/Run lease overlays instead of reconstructed from a full-snapshot clone each call. Only the All-overlay (expired-lease recovery, off the hot path) still rebuilds. The hot chat-turn ops (submit/claim/complete/block/cancel/resume) already used the targeted -delta path and are unchanged. Durability is UNCHANGED — write-through preserved: every mutation still awaits the durable journal ack before returning, and load/materialize/ legacy-blob-migration/orphan-lock/runner-lease-overlay semantics are byte-for-byte identical. This is a pure latency/CPU + contention refactor, not the async-write-behind change (that is #6263 Step 3, gated on the crash-consistency property suite). Measured (chat-turn contention sweep, filesystem-row, 32-core box): store-isolated per-transition p50 down ~20-27% at c32-c100; goodput at c100 277 -> 625 ops; turn_conflict errors roughly halved (c100 99 -> 43). The c8 p50 (~7ms) is unchanged — it is the synchronous durable-ack floor, which only Step 3 addresses. 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: M
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesFilesystem store synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 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 |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
⏭️ IronLoop Review Declined: reviewer
Review at a glance
| Disposition | Head |
|---|---|
| ⏭️ Review declined | d588a37549d8 |
Head: d588a37549d8d60d081e57aade24354997a71041
Reason: The comparison changes 158 files across 19+ crates/subsystems (3,020 additions and 8,891 deletions), including CLI, auth/secrets, host runtime, WebUI, Slack WASM, fixtures, and scripts. This materially exceeds and conflicts with the PR description’s focused row-store change.
Next: Split or rebase the PR so base..head contains only the intended turn-state row-store refactor, with unrelated cross-subsystem changes removed or submitted separately; then request a new scoped review.
Run details
Status: Current
Trustworthy review produced: no
Summary
Skipped: the supplied base-to-head comparison is a mega, mixed-scope diff that cannot be reliably reviewed as the stated turn-state refactor within this review budget.
![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 refactors FilesystemTurnStateRowStore by removing the commit_gate mutex and instead serializing enqueues on the shared snapshot_state lock. It also introduces a helper method acquire_overlaid_store to deduplicate the overlay-store acquisition logic across the apply and apply_with_targeted_delta paths. Feedback was provided regarding the need to invalidate the snapshot_state cache if apply_delta fails during a loop checkpoint operation, preventing the cache from being left in an inconsistent state.
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.
| if let Some(state) = guard.as_mut() { | ||
| state.apply_delta(delta, state.journal_seq)?; | ||
| } |
There was a problem hiding this comment.
If state.apply_delta fails here, the in-memory snapshot_state cache (guard) is left in a partially-mutated, inconsistent state. To ensure correctness and follow defensive programming practices, we should invalidate the cache by setting *guard = None on failure, matching the pattern used in apply and apply_with_targeted_delta.
if let Some(state) = guard.as_mut() {
if let Err(error) = state.apply_delta(delta, state.journal_seq) {
*guard = None;
return Err(error);
}
}There was a problem hiding this comment.
Confirmed and fixed — the new put_loop_checkpoint path now sets *guard = None before propagating a failed apply_delta, matching the invariant apply and apply_with_targeted_delta enforce at every mutation-error site (8+ *guard = None points). So a later read rebuilds the cache from durable state instead of serving a partially-mutated snapshot. The durable enqueue has already succeeded at that point, so recovery replays it — the in-memory cache is the only thing to invalidate.
…oop_checkpoint apply If the in-memory apply_delta fails, the cached snapshot can be left partially mutated; the new put_loop_checkpoint path propagated the error via ? without dropping the cache, unlike apply / apply_with_targeted_delta (which *guard = None on every mutation error). Now matches that invariant — a later read rebuilds from durable state rather than serving a corrupt cache. The durable enqueue already succeeded, so recovery replays it; the cache is the only thing to invalidate. The failure arm is an internal-invariant apply error not reachable from the public store API without fault injection the store doesn't expose, so it follows the tested sibling pattern rather than a new drivable test. [skip-regression-check] Reported-by: gemini-code-assist (PR #6281 review) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🚅 Deployed to the ironclaw-pr-6281 environment in ironclaw-ci-preview
|
![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)
364-382: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winStale (non-advanced) sequence number passed to
apply_delta.Line 379 calls
state.apply_delta(delta, state.journal_seq)— the current, already-consumed sequence, not the next one. Every other call site that applies a delta to the cached state advances first:apply()inrow_store.rsuseslet reservation_seq = current_journal_seq.next();beforeRowSnapshotState::new(new_snapshot, store, reservation_seq), andapply_with_targeted_delta()computesstate.journal_seq.next()before callingstate.apply_delta(delta, reservation_seq). Reusing the stale seq here risks stamping the loop-checkpoint row with a seq that collides with — or fails to reflect — the seq the delta journal actually assigned, and leavesstate.journal_sequnder-counting relative to the durable head thatenqueue_deltajust advanced, whichrefresh_snapshot_cache_after_stale_mutation_error'sstate.journal_seq < head_seqcheck relies on.🐛 Proposed fix
if let Some(state) = guard.as_mut() { - state.apply_delta(delta, state.journal_seq)?; + let next_seq = state.journal_seq.next(); + state.apply_delta(delta, next_seq)?; }Per repo guidance, this bug fix should ship with a regression test through the caller (e.g. asserting
journal_seq/row seq advances by exactly one across aput_loop_checkpointcall, or that a subsequent mutation's reservation seq doesn't collide) rather than only a helper-level check. Want me to draft that test?#!/bin/bash # Confirm apply_delta's seq contract and whether journal_seq is mutated internally. rg -n -B2 -A15 'fn apply_delta' crates/ironclaw_turns🤖 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 364 - 382, Advance the sequence before applying the loop-checkpoint delta in the put_loop_checkpoint flow: update the block around enqueue_delta and RowSnapshotState::apply_delta to compute the next journal sequence and pass that reservation instead of state.journal_seq. Add a regression test through the put_loop_checkpoint caller verifying the journal/row sequence advances exactly once and subsequent mutations do not reuse the sequence.
🤖 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 364-382: Advance the sequence before applying the loop-checkpoint
delta in the put_loop_checkpoint flow: update the block around enqueue_delta and
RowSnapshotState::apply_delta to compute the next journal sequence and pass that
reservation instead of state.journal_seq. Add a regression test through the
put_loop_checkpoint caller verifying the journal/row sequence advances exactly
once and subsequent mutations do not reuse the sequence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 94555689-be91-4f95-b354-6016ba12173f
📒 Files selected for processing (2)
crates/ironclaw_turns/src/filesystem_store/row_store.rscrates/ironclaw_turns/src/filesystem_store/row_store/traits.rs
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.59% — 313150 / 365872 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)
|
|
✅ Ready for merge — turn-state row-store perf refactor (#6263 Step 1: long-lived authority + removed the redundant global Review + thermo-nuclear pass complete:
CI green 59/59 (9 skipped). Ready to merge. |
This branch was successfully deployed
1 participant
Step 1 of the turn-state consolidation in #6263. A pure latency/CPU + contention refactor of
FilesystemTurnStateRowStore; durability and crash-recovery semantics are unchanged (write-through preserved).What
commit_gateasync mutex. It was held in addition to thesnapshot_statemutex the mutation path already held across its read-seq → apply → enqueue window. Production wires a single shared row-store instance, so that redundant global lock serialized every user's turn-state transition.snapshot_stateis now the single serialization point (held microseconds — CPU apply + delta build + mpsc send, never across the durable ack); journal-sequence ordering is preserved because enqueue order already equals reservation-seq order under it.apply()path (retry / submit_child / tree / recover / fail / relinquish / record_model_route): the long-lived cachedInMemoryTurnStateStoreauthority is reused in place for the None/Run lease overlays instead of reconstructed from a full-snapshot clone per call. Only the All-overlay (expired-lease recovery, off the hot path) still rebuilds. The hot chat-turn ops (submit/claim/complete/block/cancel/resume) already used the targeted-delta path and are untouched.Durability preserved
Every mutation still
enqueue_delta→ awaits the durable journal ack before returning.load_snapshot_from_rows,migrate_legacy_blob_if_needed,materialize_delta_log,remove_orphan_active_locks, backend seq assignment, and the entire runner-lease overlay/heartbeat surface are unmodified. This is not the async-write-behind change — that's #6263 Step 3, gated on the crash-consistency property suite.Measured (chat-turn contention sweep,
filesystem-row, 32-core box; before = byte-identical main)Store-isolated per-transition p50 down ~20-27% at c32-c100; goodput at c100 more than doubled; conflict errors roughly halved. The c8 p50 (~7 ms) is unchanged — it's the synchronous durable-ack floor, which only Step 3 (async write-behind) addresses.
Tests
cargo test -p ironclaw_turns: 599 passed / 0 failed (full 47-test row-store contract suite incl. concurrency/crash/reservation cases), independently re-verified.ironclaw_runner(filesystem-goal-store) andironclaw_reborn_compositionturn-state tests green. clippy-D warningsclean; pre-commit-safety exit 0.🤖 Generated with Claude Code