fix(reborn): remove per-record lock convoys via shared cas_update helper - #5234
Conversation
Extract the proven mutex-free CAS pattern from ironclaw_turns (PR #5142) into one shared helper in ironclaw_filesystem so the remaining stores can drop their per-record tokio::sync::Mutex convoys. cas_update runs a bounded read-modify-write loop: read versioned snapshot, run the caller's idempotent apply closure, CAS-put with the read version, and on VersionMismatch re-read and retry with jittered exponential backoff (2ms..50ms), capped at 32 retries and wrapped in a 15s timeout. A fail-closed capability gate rejects backends that cannot CAS rather than silently blind-overwriting. The helper is generic over the record type, the outcome, and the caller's error; it never leaks store-specific types. No store migrated yet — that follows in the next commits. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…te helper ironclaw_resources had NO CAS-retry loop — its per-record tokio::sync::Mutex (FILESYSTEM_RECORD_LOCKS) held across the backend get/put awaits was the ONLY serializer. Under burst, one writer stalled inside its critical section parked every other same-scope writer (the convoy that contributed to the 2026-06-24 runtime wedge). Naively deleting that mutex without a retry loop loses updates: a racing writer's single-attempt CAS detects the version mismatch and errors out (cross-process CAS contention) instead of retrying. Route update_snapshot through ironclaw_filesystem::cas_update (bounded CAS retry + jittered backoff + 15s timeout + fail-closed capability gate) and delete the per-record mutex, FilesystemRecordLock, the local PutError, and the fail-open put_with_cas (CasExpectation::Any blind-overwrite fallback). The helper fails closed instead; verified safe — these store aliases only ever resolve to CAS-capable db/in-memory backends in production. The public update API widens from FnOnce to FnMut because the helper re-runs the closure against a freshly read snapshot on every CAS retry; leaf closures that previously moved captured values now clone per invocation. Add PartialEq to BudgetGateSnapshot (helper needs S: Clone + PartialEq). Red->green regression: cas_snapshot::tests::concurrent_increments_have_no_lost_updates proves RED on lock-removed-no-retry (lost updates / spurious contention) and GREEN through the helper (every concurrent increment lands, no convoy). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ant per-record mutex run_state held BOTH a store-local CAS-retry loop AND a per-record tokio::sync::Mutex (FILESYSTEM_RECORD_LOCKS) across the backend get/put awaits — belt-and-suspenders where the mutex was a redundant in-process serializer over a backend that already does versioned CAS. Under burst the mutex convoyed same-scope writers behind one stalled writer (the 2026-06-24 wedge pattern). Route apply_update, update_status, start, and save_pending through ironclaw_filesystem::cas_update (one shared CAS implementation: bounded retry + jittered backoff + 15s timeout + fail-closed capability gate). Delete the store-local FILESYSTEM_CAS_RETRIES loop, the per-record lock + accessor + its two unit tests, the local PutError, and the fail-open put_with_cas (CasExpectation::Any blind-overwrite fallback). The scope-ownership check and the approval Pending guard move inside the re-runnable apply closure; the ApprovalStatus guard returns Err from apply. discard_pending keeps its read-then-delete, only the lock is dropped. The run_state contract tests previously ran against LocalFilesystem (byte-only, no versioned CAS) where the OLD fail-open fallback masked the missing CAS. Production never routes run_state to LocalFilesystem — only to CAS-capable db/in-memory backends — so the tests now use InMemoryBackend, matching the production capability shape (CAS) instead of a non-production blind-overwrite path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…p per-record mutex ensure_thread held the only per-record tokio::sync::Mutex (FILESYSTEM_RECORD_LOCKS) in this crate, across the backend get/put awaits, to serialize the check-then-create against Unsupported(WriteFile)-fallback backends. Route it through ironclaw_filesystem::cas_update: the apply closure returns the existing record unchanged (no-op, no write) when the thread already exists with a matching scope, or builds the fresh StoredThreadRecord when absent. A concurrent create-if-absent loser hits VersionMismatch, the helper re-reads and re-runs apply which now sees the winner and reconciles scope — exactly the old single-reconcile semantics, now via the shared CAS-retry loop without any lock. Delete the lock infra (Weak map + accessor); add PartialEq to StoredThreadRecord (helper needs S: Clone + PartialEq). The three other record/txn loops were already lock-free and are left as-is. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cord mutex (Arc leak fix) The secrets filesystem store kept FILESYSTEM_RECORD_LOCKS as a HashMap of *strong* Arc<Mutex<()>> (not Weak), one entry per (scope,lease)/session path ever seen, NEVER pruned — an unbounded memory leak. The per-record mutex was also held across the backend get/put awaits, convoying same-key writers behind one stalled writer (the 2026-06-24 wedge pattern). Route consume/revoke/consume_session_use through ironclaw_filesystem::cas_update (one shared CAS implementation), mapping the local CasDecision onto CasApply: Commit/BestEffortCommit -> changed snapshot; Settle(Ok) -> unchanged snapshot (helper skips the write via PartialEq no-op); Settle(Err)/not-found -> apply error. Crypto decrypt + use-count increment run inside the re-runnable apply closure (pure / recomputed from the freshly read record each retry). validate_session is a pure read — its lock is simply deleted. Delete the entire lock map (leak gone), cas_mutate/CasDecision/CAS_RETRY_ATTEMPTS, and the fail-open put_with_version_fallback (helper fails closed). Add PartialEq to StoredLease and StoredSession. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PR #5142 removed the per-record mutex from ironclaw_turns but left a local copy of the CAS read-modify-write loop (apply_with_retry, put_with_cas, PutError, cas_retry_backoff + constants). Re-home it onto ironclaw_filesystem::cas_update so there is ONE CAS implementation across the codebase. Behavior is preserved: the 18 call sites and their closure shape (FnMut(InMemoryTurnStateStore) -> Fut) are unchanged; the exact timeout/exhaustion error strings are preserved. A BridgeError<T> sentinel carries the absent-record + default-snapshot no-op through the helper's apply-error channel (the helper's own no-op check only fires for Some(existing)==new; the turns store additionally must skip creating a file for an empty default store, which the old new==old check covered because read returned default on absent). The 500ms SNAPSHOT_READ_CACHE_TTL read cache is kept as a separate layer: cleared around the CAS call and repopulated on the next read (the helper does its own fresh get and does not surface the new RecordVersion; every read_snapshot caller already discards the version, so caching None is safe). Delete the now-unused local loop, put_with_cas, PutError, cas_retry_backoff, and the duplicate CAS constants. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cord mutex Add the invariant to crates/ironclaw_filesystem/CLAUDE.md (#2 "CAS is the floor") and a pointer in .claude/rules/database.md: every filesystem read-modify-write must go through the one shared ironclaw_filesystem::cas_update helper; never wrap it in a per-record tokio::sync::Mutex held across the backend .await (redundant serializer + convoy/wedge risk + leak). Also commit Cargo.lock (async-trait dev-dep added to ironclaw_resources for the convoy regression test). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
scope: docs
![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 migrates multiple persistence stores (ironclaw_resources, ironclaw_run_state, ironclaw_threads, ironclaw_secrets, and ironclaw_turns) to use a shared, lock-free compare-and-swap (CAS) helper (cas_update). This eliminates redundant per-record mutexes held across .await boundaries, preventing runtime-wedging convoys under high contention. Feedback on the changes highlights an opportunity to improve the jitter calculation in cas_retry_backoff using RandomState to avoid zero-jitter scenarios in low clock resolution environments (such as VMs or Windows). Additionally, it is recommended to explicitly add Send bounds to the closure generic constraints in cas_update and cas_update_loop to catch any non-Send regressions at the helper's definition site.
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 cas_retry_backoff(attempt: usize) { | ||
| let shift = attempt.min(8) as u32; | ||
| let multiplier = 1_u32.checked_shl(shift).unwrap_or(u32::MAX); | ||
| let base_delay = FILESYSTEM_CAS_BACKOFF_BASE | ||
| .saturating_mul(multiplier) | ||
| .min(FILESYSTEM_CAS_BACKOFF_MAX); | ||
| let jitter = SystemTime::now() | ||
| .duration_since(UNIX_EPOCH) | ||
| .map(|elapsed| { | ||
| let jitter_ceiling = base_delay.as_millis().max(1); | ||
| Duration::from_millis((elapsed.as_nanos() % jitter_ceiling) as u64) | ||
| }) | ||
| .unwrap_or_default(); | ||
| tokio::time::sleep(base_delay.saturating_add(jitter)).await; | ||
| } |
There was a problem hiding this comment.
Using SystemTime::now() for jitter calculation can lead to zero jitter on systems with low clock resolution (e.g., 1ms resolution in VMs, containers, or Windows). Since base_delay.as_millis() is at most 50, and 1,000,000 (1ms in nanoseconds) is divisible by all common backoff ceilings (2, 4, 8, 16, 32, 50), the modulo operation elapsed.as_nanos() % jitter_ceiling will always evaluate to exactly 0. This completely defeats the purpose of jitter under high contention.
Instead, use std::collections::hash_map::RandomState to obtain a high-quality, thread-safe, and un-correlated pseudo-random value seeded from the OS without adding external dependencies.
| async fn cas_retry_backoff(attempt: usize) { | |
| let shift = attempt.min(8) as u32; | |
| let multiplier = 1_u32.checked_shl(shift).unwrap_or(u32::MAX); | |
| let base_delay = FILESYSTEM_CAS_BACKOFF_BASE | |
| .saturating_mul(multiplier) | |
| .min(FILESYSTEM_CAS_BACKOFF_MAX); | |
| let jitter = SystemTime::now() | |
| .duration_since(UNIX_EPOCH) | |
| .map(|elapsed| { | |
| let jitter_ceiling = base_delay.as_millis().max(1); | |
| Duration::from_millis((elapsed.as_nanos() % jitter_ceiling) as u64) | |
| }) | |
| .unwrap_or_default(); | |
| tokio::time::sleep(base_delay.saturating_add(jitter)).await; | |
| } | |
| async fn cas_retry_backoff(attempt: usize) { | |
| let shift = attempt.min(8) as u32; | |
| let multiplier = 1_u32.checked_shl(shift).unwrap_or(u32::MAX); | |
| let base_delay = FILESYSTEM_CAS_BACKOFF_BASE | |
| .saturating_mul(multiplier) | |
| .min(FILESYSTEM_CAS_BACKOFF_MAX); | |
| let jitter = { | |
| use std::collections::hash_map::RandomState; | |
| use std::hash::{BuildHasher, Hash, Hasher}; | |
| let mut hasher = RandomState::new().build_hasher(); | |
| attempt.hash(&mut hasher); | |
| let hash = hasher.finish(); | |
| let jitter_ceiling = base_delay.as_millis().max(1) as u64; | |
| Duration::from_millis(hash % jitter_ceiling) | |
| }; | |
| tokio::time::sleep(base_delay.saturating_add(jitter)).await; | |
| } |
There was a problem hiding this comment.
Fixed in 0eaf69d — jitter now derives from a RandomState-seeded hash (clock-independent), so it no longer collapses to 0 on coarse-resolution clocks. See cas_retry_backoff in crates/ironclaw_filesystem/src/cas.rs.
| pub async fn cas_update<F, S, T, E, D, N, A, Fut>( | ||
| filesystem: &ScopedFilesystem<F>, | ||
| scope: &ResourceScope, | ||
| path: &ScopedPath, | ||
| decode: D, | ||
| encode: N, | ||
| mut apply: A, | ||
| ) -> Result<T, CasUpdateError<E>> | ||
| where | ||
| F: RootFilesystem + ?Sized, | ||
| S: PartialEq + Clone, | ||
| D: Fn(&[u8]) -> Result<S, E>, | ||
| N: Fn(&S) -> Result<Entry, E>, | ||
| A: FnMut(Option<S>) -> Fut, | ||
| Fut: Future<Output = Result<CasApply<S, T>, E>>, |
There was a problem hiding this comment.
According to the repository's general rules, async helper functions that return futures intended to be Send should explicitly add Send bounds to their closure generic constraints. This ensures that any non-Send regressions are caught at the helper's definition site rather than at the call site.
pub async fn cas_update<F, S, T, E, D, N, A, Fut>(
filesystem: &ScopedFilesystem<F>,
scope: &ResourceScope,
path: &ScopedPath,
decode: D,
encode: N,
mut apply: A,
) -> Result<T, CasUpdateError<E>>
where
F: RootFilesystem + ?Sized,
S: PartialEq + Clone + Send,
T: Send,
E: Send,
D: Fn(&[u8]) -> Result<S, E> + Send,
N: Fn(&S) -> Result<Entry, E> + Send,
A: FnMut(Option<S>) -> Fut + Send,
Fut: Future<Output = Result<CasApply<S, T>, E>> + Send,References
- In Rust, explicitly add
Sendbounds to closure generic constraints in async helper functions that return futures intended to beSend. This ensures that non-Sendregressions are caught at the helper's definition site rather than at the call site, even if the future is already bound bySendat the call site.
There was a problem hiding this comment.
Considered but declined. Adding Send to the helper generics forces propagating Send across consumer-crate generics (run_state mutate: M, turns persistence T) — threading a bound through multiple layers for marginal "catch at the def site" benefit, which .claude/rules/architecture.md flags as bound-creep. The futures are already Send-constrained where awaited/spawned.
| async fn cas_update_loop<F, S, T, E, D, N, A, Fut>( | ||
| filesystem: &ScopedFilesystem<F>, | ||
| scope: &ResourceScope, | ||
| path: &ScopedPath, | ||
| decode: &D, | ||
| encode: &N, | ||
| apply: &mut A, | ||
| ) -> Result<T, CasUpdateError<E>> | ||
| where | ||
| F: RootFilesystem + ?Sized, | ||
| S: PartialEq + Clone, | ||
| D: Fn(&[u8]) -> Result<S, E>, | ||
| N: Fn(&S) -> Result<Entry, E>, | ||
| A: FnMut(Option<S>) -> Fut, | ||
| Fut: Future<Output = Result<CasApply<S, T>, E>>, |
There was a problem hiding this comment.
According to the repository's general rules, async helper functions that return futures intended to be Send should explicitly add Send bounds to their closure generic constraints. This ensures that any non-Send regressions are caught at the helper's definition site rather than at the call site.
async fn cas_update_loop<F, S, T, E, D, N, A, Fut>(
filesystem: &ScopedFilesystem<F>,
scope: &ResourceScope,
path: &ScopedPath,
decode: &D,
encode: &N,
apply: &mut A,
) -> Result<T, CasUpdateError<E>>
where
F: RootFilesystem + ?Sized,
S: PartialEq + Clone + Send,
T: Send,
E: Send,
D: Fn(&[u8]) -> Result<S, E> + Send,
N: Fn(&S) -> Result<Entry, E> + Send,
A: FnMut(Option<S>) -> Fut + Send,
Fut: Future<Output = Result<CasApply<S, T>, E>> + Send,References
- In Rust, explicitly add
Sendbounds to closure generic constraints in async helper functions that return futures intended to beSend. This ensures that non-Sendregressions are caught at the helper's definition site rather than at the call site, even if the future is already bound bySendat the call site.
There was a problem hiding this comment.
Same decision as the cas_update bound above — declined to avoid propagating Send across consumer-crate generics for marginal benefit.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFilesystem-backed read-modify-write paths now use ChangesShared filesystem CAS migration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
ironclaw/crates/ironclaw_run_state/src/lib.rs
Line 932 in 3223540
Guard discard deletes with CAS
When discard_pending races in the same process with approve or deny for the same request, this unconditional delete can now run after the resolver's CAS-protected status update and remove the terminal approval record. The removed per-record lock used to serialize that get/check/delete sequence with status updates; without either that lock or a CAS/tombstone transition here, a user approval can be lost and later look like UnknownApprovalRequest rather than Approved.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_run_state/src/lib.rs (1)
922-932: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake
discard_pendingatomic before dropping the lock.This path reads
Pending, then deletes later with no CAS expectation. A concurrentapprove()/deny()can win the CAS update between Line 922 and Line 932, then this delete removes a non-pending approval record. Use a CAS-aware delete/transactional transition, or model discard as a CAS status update instead of read-then-delete. As per path instructions: “Fail loud” and filesystem read-modify-write paths must use the CAS invariant rather than split multi-step state changes.🤖 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_run_state/src/lib.rs` around lines 922 - 932, The discard_pending flow in RunState::discard_pending is currently split into a read/validate step and a later filesystem delete, which can race with approve()/deny() and remove a record after its status has already changed. Update this path to be atomic by using a CAS-aware transition or transactional delete that enforces the same status invariant at the point of mutation, rather than relying on a prior Pending check; keep the failure path loud if the expected state no longer matches.Sources: Coding guidelines, Path instructions
🤖 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_filesystem/src/cas.rs`:
- Line 193: The new Clippy allow on cas.rs needs the required arch-exempt
rationale comment immediately above #[allow(clippy::too_many_arguments)]. Update
the same site near the cas:: function signature that triggered the lint by
adding the mandatory // arch-exempt: too_many_args, <reason naming the missing
aggregation>, plan `#NNNN` annotation, and make sure the reason justifies why this
many parameters is unavoidable until the missing aggregation is introduced.
- Around line 295-306: The CAS preflight is treating
BackendCapabilities::default() as “unknown,” which lets a backend with no
advertised capabilities bypass the fail-closed gate. Update capabilities_known()
in cas.rs to only treat an explicit unknown state as unknown and not overload
BackendCapabilities::empty()/default(), then make the cas_update path rely on
that check so unsupported backends still return CasUpdateError::CasUnsupported
instead of falling back to op-time behavior.
- Around line 262-265: The no-op shortcut in cas_update_loop currently only
returns early when current is Some(existing) and matches the snapshot, while
absent records still fall through to encoding and writing; update the logic to
either treat None plus an unchanged/default snapshot as a true no-op or adjust
the CasApply/cas_update contract and surrounding comments to state the shortcut
only applies to existing records. Use the existing cas_update_loop and CasApply
symbols to keep the behavior and docs aligned, and remove any need for
downstream NoOp suppression if you choose to enforce it here.
In `@crates/ironclaw_filesystem/src/cas/tests.rs`:
- Around line 184-399: Add a regression test in cas/tests.rs for the timeout
path in cas_update, since the current tests cover retries, CAS rejection, and
apply errors but never exercise CasUpdateError::Timeout. Introduce a tiny wedged
backend or stub that blocks one of the CAS operations (get, put, or apply), and
use paused Tokio time so the outer timeout in cas_update deterministically
fires. Reference cas_update and CasUpdateError::Timeout in the new test, and
assert that the timeout branch wins over the hung backend behavior.
In `@crates/ironclaw_resources/src/cas_snapshot.rs`:
- Line 212: The CasUpdateError::Backend handling in cas_snapshot.rs currently
forwards the backend error string directly via E::storage_from(inner), which can
leak virtual paths and raw backend details into StorageError. Update this
mapping to return a stable sanitized public message from the error conversion
path, and preserve the detailed backend error only in internal diagnostics or
source context associated with E::storage_from.
In `@crates/ironclaw_run_state/src/lib.rs`:
- Line 1109: The `CasUpdateError::Backend` branch in `RunStateError` currently
converts the filesystem failure to a string, which drops the structured
`FilesystemError` context. Update this match arm in `lib.rs` to preserve the
typed error by using the existing `?`-style conversion path or direct typed
`From`/`Into` conversion used elsewhere in this file, so the `FilesystemError`
variant and its path/operation details continue to propagate through
`RunStateError::Filesystem`.
In `@crates/ironclaw_secrets/src/filesystem_store.rs`:
- Around line 479-483: The no-op CAS comment in filesystem_store.rs is
inaccurate: the `already_marked` branch in the lease update path returns
`Err(SecretStoreError::LeaseExpired { lease_id })` and goes through
`CasUpdateError::Apply`, not the unchanged record/`PartialEq` skip path. Update
the comment to describe the actual intent, or change the branch in the relevant
lease/CAS helper to return `CasApply::new(lease, Err(...))` if that is the
desired behavior; use the `already_marked`, `SecretLeaseStatus::Expired`, and
`CasUpdateError::Apply` symbols to locate and align the code with the documented
contract.
In `@crates/ironclaw_turns/src/filesystem_store.rs`:
- Around line 378-385: The CAS error mapper in map_cas_error should not panic on
BridgeError::NoOp, since this is production error-handling code. Replace the
unreachable! fallback in map_cas_error with a typed unavailable TurnError that
preserves context about the unexpected NoOp leak, while keeping the
BridgeError::Real(inner) path unchanged and consistent with CasUpdateError
handling.
In `@docs/plans/2026-06-25-cas-migration.md`:
- Around line 137-140: The quality gate in the migration plan is outdated and
should match the repo’s required backend validation for persistence changes.
Update the “Quality gate” section to include the feature-isolation `cargo check`
matrix for the default/postgres build, `--no-default-features --features
libsql`, and `--all-features`, and strengthen the clippy step to the full `cargo
clippy --all --benches --tests --examples --all-features -- -D warnings`. Keep
the existing formatting/tests coverage, but ensure the plan explicitly reflects
the required checks for the legacy persistence migration.
---
Outside diff comments:
In `@crates/ironclaw_run_state/src/lib.rs`:
- Around line 922-932: The discard_pending flow in RunState::discard_pending is
currently split into a read/validate step and a later filesystem delete, which
can race with approve()/deny() and remove a record after its status has already
changed. Update this path to be atomic by using a CAS-aware transition or
transactional delete that enforces the same status invariant at the point of
mutation, rather than relying on a prior Pending check; keep the failure path
loud if the expected state no longer matches.
🪄 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: 1c594037-4929-451d-97ad-73e096ed325d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (17)
.claude/rules/database.mdcrates/ironclaw_filesystem/CLAUDE.mdcrates/ironclaw_filesystem/src/cas.rscrates/ironclaw_filesystem/src/cas/tests.rscrates/ironclaw_filesystem/src/lib.rscrates/ironclaw_filesystem/src/scoped.rscrates/ironclaw_resources/Cargo.tomlcrates/ironclaw_resources/src/cas_snapshot.rscrates/ironclaw_resources/src/filesystem_store.rscrates/ironclaw_resources/src/lib.rscrates/ironclaw_run_state/src/lib.rscrates/ironclaw_run_state/tests/approval_resolution_contract.rscrates/ironclaw_run_state/tests/run_state_contract.rscrates/ironclaw_secrets/src/filesystem_store.rscrates/ironclaw_threads/src/filesystem_service.rscrates/ironclaw_turns/src/filesystem_store.rsdocs/plans/2026-06-25-cas-migration.md
|
🚅 Deployed to the ironclaw-pr-5234 environment in ironclaw-ci-preview
|
Fold in PR review (henrypark133) + a durable-write hot-path audit. Adds §12 framing the deployed lease-expiry cascade as three independent layers: - Layer A: in-process lock convoy → #5234 (open); write-behind composes on the post-#5234 CAS path. - Layer B: pool starvation — DEFAULT_POSTGRES_POOL_MAX_SIZE=2 shared across all Postgres FS I/O. Notes the existing 30s checkout guard (closes the infinite hang) but flags pool-too-small + checkout(30s)>apply(15s); cheap mitigations (raise pool, reserve a critical connection, align checkout<apply) prior to and complementary with write-behind. - Layer C: the synchronous hot-path write map (events / governor / thread-append / lease / memory) with the write-behind-vs-batch-coalesce distinction. Events are the top batch-coalesce target (highest churn, O(1) INSERT no CAS); must stay DURABLE (source of truth), per-step flush to preserve live SSE. Memory stays synchronous (FTS-only, no embedding write; agent-initiated, low churn). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolve PR #5234 review comments across the shared cas_update helper and its consumers: - cas: replace SystemTime-based jitter (collapses to 0 on coarse clocks) with a RandomState-seeded value; drop the now-unnecessary too_many_arguments allow (helper has 6 args, below clippy threshold). - cas: add explicit CasApply::no_op so absent-record no-ops are signalled directly; make the contract doc true. Deletes the BridgeError::NoOp "success-through-an-error-channel" hack in ironclaw_turns (E is now TurnError directly) and simplifies map_cas_error. - run_state: fix a TOCTOU race in FilesystemApprovalRequestStore:: discard_pending (Codex P1). The non-atomic get->check->delete could remove a record an approve()/deny() CAS had just transitioned. Route discard through cas_update as a version-checked tombstone transition (new ApprovalStatus::Discarded); get/records_for_scope filter it so it still reads as gone. Adds a no-clobber regression test. - turns/secrets: correct inaccurate CAS no-op comment; replace an unreachable!() panic in map_cas_error with a typed Unavailable error. - add a deterministic CasUpdateError::Timeout regression test (paused Tokio time + hanging backend). - docs: restore the feature-isolation cargo check matrix and full clippy gate in the migration plan. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_product_workflow/src/approval_interaction/service.rs (1)
158-164: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClassify discarded approvals as stale before the persistent branch.
For
persistent == true,ApprovalStatus::Discardedhits Line 163 and returnsAlwaysAllowUnsupported, so the new stale arm at Line 199 is unreachable for that path.Proposed fix
- if matches!(status, ApprovalStatus::Denied | ApprovalStatus::Expired) { + if matches!( + status, + ApprovalStatus::Denied | ApprovalStatus::Expired | ApprovalStatus::Discarded + ) {As per coding guidelines, WebUI-facing facade errors must expose the stable sanitized taxonomy for blocked approval/auth/resource and stale/conflict states.
Also applies to: 199-199
🤖 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_product_workflow/src/approval_interaction/service.rs` around lines 158 - 164, The approval gating in approval_interaction::service::ApprovalInteractionService should classify ApprovalStatus::Discarded as stale before the persistent-only unsupported branch. Update the early status checks around the approval_rejected flow so the stale rejection path is taken for Discarded regardless of persistent, and ensure the later stale arm remains reachable with the sanitized StaleGate taxonomy instead of falling through to AlwaysAllowUnsupported.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.
Inline comments:
In `@crates/ironclaw_filesystem/src/cas.rs`:
- Around line 102-114: The no-op early returns in cas_update can return a
snapshot that is no longer current, which then gets cached by
filesystem_store::apply_from_snapshot and similar callers. Update cas_update so
the equality and explicit no_op branches either revalidate the record/version
before returning or avoid exposing a cacheable “final” snapshot from those
paths; use the cas_update, CasApply::new, and CasApply::no_op branches as the
main touchpoints. Also adjust the surrounding contract/comments so they only
promise what is actually enforced after concurrent writes.
In `@crates/ironclaw_run_state/tests/run_state_contract.rs`:
- Around line 491-510: The regression test in
filesystem_discard_does_not_clobber_resolved_approval is still sequential and
does not force the TOCTOU interleaving it describes. Update this test to use a
store/filesystem harness that can pause discard_pending after it reads a Pending
record, then let approve run on the same request before discard_pending resumes
its write/CAS path. Use FilesystemApprovalRequestStore and
discard_pending/approve to verify the resolved approval is preserved and the
terminal record is not clobbered.
In `@docs/plans/2026-06-25-cas-migration.md`:
- Around line 141-145: The quality gate currently uses cargo fmt --all, which
reformats files instead of validating CI-style formatting; update the checklist
entry in the plan to use the repo’s check-mode formatting command instead. Make
this change in the validation list alongside the other cargo check/clippy steps
so the migration plan matches the “Before You Open a PR” requirement and remains
authoritative.
---
Outside diff comments:
In `@crates/ironclaw_product_workflow/src/approval_interaction/service.rs`:
- Around line 158-164: The approval gating in
approval_interaction::service::ApprovalInteractionService should classify
ApprovalStatus::Discarded as stale before the persistent-only unsupported
branch. Update the early status checks around the approval_rejected flow so the
stale rejection path is taken for Discarded regardless of persistent, and ensure
the later stale arm remains reachable with the sanitized StaleGate taxonomy
instead of falling through to AlwaysAllowUnsupported.
🪄 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: e8a5fca8-3068-48d7-92df-53d101912fa5
📒 Files selected for processing (10)
crates/ironclaw_capabilities/src/helpers.rscrates/ironclaw_filesystem/Cargo.tomlcrates/ironclaw_filesystem/src/cas.rscrates/ironclaw_filesystem/src/cas/tests.rscrates/ironclaw_product_workflow/src/approval_interaction/service.rscrates/ironclaw_run_state/src/lib.rscrates/ironclaw_run_state/tests/run_state_contract.rscrates/ironclaw_secrets/src/filesystem_store.rscrates/ironclaw_turns/src/filesystem_store.rsdocs/plans/2026-06-25-cas-migration.md
Reconcile the CAS-migration work with main, notably PR #5232 (durable runner-lease sidecar) which independently rewrote ironclaw_turns/filesystem_store.rs — splitting it into io/profile_resolver/ projection/runner_lease submodules and adding a per-run lease sidecar. Resolution of crates/ironclaw_turns/src/filesystem_store.rs: - Keep main's module split + runner-lease sidecar feature. - Keep this PR's routing of the turn-state snapshot RMW through the shared ironclaw_filesystem::cas_update helper (no inline CAS loop, no BridgeError; E is TurnError directly; CasApply::no_op for the inert-snapshot no-op). - The cas_update apply callback applies the runner-lease overlay per retry and uses the OVERLAID snapshot as the no-op baseline, matching main's pre-merge `new_snapshot == old_snapshot` semantics so an active lease overlay does not force a spurious write each apply. Covered by a new regression test (filesystem_turn_state_store_no_op_under_active_lease_overlay_does_not_rewrite_snapshot). - service.rs: add ApprovalStatus::Discarded to the early stale-gate guard for consistency with the exhaustive match arms. Known scoped exception, tracked in #5274: the runner-lease sidecar still uses a local put_with_cas + cas_retry_backoff loop rather than cas_update. Documented in crates/ironclaw_filesystem/CLAUDE.md; the main snapshot RMW does go through cas_update. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed the Codex P1 (and CodeRabbit's matching outside-diff note) on Fixed in 0eaf69d: the non-atomic Regression test added: |
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 (2)
crates/ironclaw_product_workflow/src/approval_interaction/service.rs (2)
312-332: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse
debug!for these fallback diagnostics.These new
warn!calls are internal diagnostics on a best-effort path. In this repo,warn!/info!from runtime flows are reserved out because they corrupt the interactive REPL/TUI display. As per path instructions,REPL/TUI logging: info!/warn! corrupt the terminal UI — internal diagnostics use debug!.Suggested diff
- tracing::warn!( + tracing::debug!( error = %error, capability_id = %policy.key.capability_id, action = ?policy.key.action, "persistent approval policy write failed after approval resolution" ); @@ - tracing::warn!( + tracing::debug!( error = %error, capability_id = %override_key.capability_id, "tool permission override clear failed after persistent approval" );🤖 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_product_workflow/src/approval_interaction/service.rs` around lines 312 - 332, These fallback diagnostics in approval_interaction::service, specifically the tracing calls inside the persistent_policies.allow and tool_permission_overrides.clear error paths, should use debug! instead of warn! because runtime warn/info logging can disrupt the REPL/TUI. Update both logging sites to debug! while keeping the same error and capability context so the internal best-effort failure reporting remains available without corrupting terminal UI.Source: Path instructions
306-333: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRoute persistent-approval side effects through the approval layer.
This service now calls
PersistentApprovalPolicyStore::allow()andToolPermissionOverrideStore::clear()directly fromironclaw_product_workflow. That bypasses the approval boundary and leaves the durable allow + override-clear sequence owned by product code instead of the approval layer that already owns resolution semantics. Move this behindApprovalResolutionPort(or a dedicated approval-layer port) and invoke that here. As per path instructions,Approve/deny decisions must go through ApprovalResolutionPort and TurnCoordinator; product/WebUI code must not directly execute tools or mutate approval stores ad hoc.🤖 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_product_workflow/src/approval_interaction/service.rs` around lines 306 - 333, Route the durable approval side effects out of approval_interaction::service::persist_allow_policy and behind the approval boundary instead of calling PersistentApprovalPolicyStore::allow() and ToolPermissionOverrideStore::clear() directly. Introduce or use an ApprovalResolutionPort-owned method that performs the allow-then-clear sequence, and have persist_allow_policy delegate to that port so the approval layer remains the single owner of resolution semantics and store mutation.Source: Path instructions
🤖 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_product_workflow/src/approval_interaction/service.rs`:
- Around line 312-332: These fallback diagnostics in
approval_interaction::service, specifically the tracing calls inside the
persistent_policies.allow and tool_permission_overrides.clear error paths,
should use debug! instead of warn! because runtime warn/info logging can disrupt
the REPL/TUI. Update both logging sites to debug! while keeping the same error
and capability context so the internal best-effort failure reporting remains
available without corrupting terminal UI.
- Around line 306-333: Route the durable approval side effects out of
approval_interaction::service::persist_allow_policy and behind the approval
boundary instead of calling PersistentApprovalPolicyStore::allow() and
ToolPermissionOverrideStore::clear() directly. Introduce or use an
ApprovalResolutionPort-owned method that performs the allow-then-clear sequence,
and have persist_allow_policy delegate to that port so the approval layer
remains the single owner of resolution semantics and store mutation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a098d8a1-20cb-419f-920f-fe4f83865273
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (5)
crates/ironclaw_filesystem/CLAUDE.mdcrates/ironclaw_product_workflow/src/approval_interaction/service.rscrates/ironclaw_turns/src/filesystem_store.rscrates/ironclaw_turns/src/filesystem_store/tests.rscrates/ironclaw_turns/tests/filesystem_turn_state_contract.rs
💤 Files with no reviewable changes (3)
- crates/ironclaw_turns/src/filesystem_store/tests.rs
- crates/ironclaw_turns/tests/filesystem_turn_state_contract.rs
- crates/ironclaw_turns/src/filesystem_store.rs
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Replace per-record filesystem mutexes with a shared CAS update helper across Reborn stores to eliminate lock convoys and related leaks.
Stats: 6 findings kept (from 8 raw, 6 after filtering/dedup) across 3 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 1. Filtered: 2 low-signal local-pattern doc notes.
All surviving findings are Medium severity, so I am leaving this as a comment review rather than requesting changes.
Security
- Medium Discarded approvals now keep the full request payload on disk (
crates/ironclaw_run_state/src/lib.rs:947-956, confidence 88) — anchor: crates/ironclaw_run_state/src/lib.rs:947
discard_pendingnow rewrites the filesystem record asDiscardedinstead of deleting it, but it keeps the full originalApprovalRequestbody in the tombstone. That means a cancelled approval's action, reason, requester, and fingerprint remain recoverable from disk/backups even though public APIs hide discarded records. This is a data-minimization regression on an approval control-plane record.
Fix: Store only the minimal tombstone data needed to reserve the request ID, or keep the discarded-ID reservation in a compact side index instead of retaining the full approval payload.
Tests
- Medium Missing test for op-time unsupported-write fail-closed path (
crates/ironclaw_filesystem/src/cas.rs:337-340, confidence 95) — anchor: crates/ironclaw_filesystem/src/cas.rs:337
The helper covers the pre-flight non-CAS rejection, but the second fail-closed path is untested: an unknown/default-capability backend can read successfully and then returnFilesystemError::Unsupported { operation: WriteFile }fromput. That branch maps toCasUpdateError::CasUnsupportedand should be protected because it is the composite-router fallback path.
Fix: Addtests::cas::unsupported_write_file_maps_to_cas_unsupportedusing a default-capabilities backend whose read succeeds and whose CAS write returnsUnsupported(WriteFile).
Performance / Concurrency
- Medium CAS helper clones the full snapshot on every retry (
crates/ironclaw_filesystem/src/cas.rs:299-307, confidence 86) — anchor: crates/ironclaw_filesystem/src/cas.rs:306
cas_updateclones the decoded snapshot before everyapply, and several migrated call sites clone again when buildingCasApply::new(...). For large snapshots such as turn state, each write now copies the whole record at least twice, and contention multiplies that cost across the retry loop. That is avoidable churn on the hot persistence path this PR is trying to improve.
Fix: Passapplya borrowed snapshot or split the helper API so the helper retains one owned copy for equality/write checks while call sites clone only when they truly need ownership.
Conventions
- Medium Filesystem discard now blocks ID reuse, but the in-memory store does not (
crates/ironclaw_run_state/src/lib.rs:947-956, confidence 82) — anchor: crates/ironclaw_run_state/tests/run_state_contract.rs:459 (no diff position — body only)
The filesystem store now writes aDiscardedtombstone, so a latersave_pendingwith the same request ID is rejected even thoughgetandrecords_for_scopehide the record. The in-memory store still removes the record outright, so tests and early wiring can accept a reused approval ID that production rejects. That diverges the store contract introduced by this PR.
Fix: MakeInMemoryApprovalRequestStore::discard_pendingpreserve a discarded tombstone, or separately track discarded IDs sosave_pendingrejects reused IDs in both backends.
Maintainability
- Medium Turn-state apply has become a 1k-line orchestration knot (
crates/ironclaw_turns/src/filesystem_store.rs:381-533, confidence 95) — anchor: crates/ironclaw_turns/src/filesystem_store.rs:381
This refactor pushesFilesystemTurnStateStorefrom 961 to 1051 lines, and the newapplyflow now mixes cache invalidation, runner-lease overlay, transient store construction, FnMut adaptation, CAS no-op detection, timeout policy, and cache update/rollback in one method. That is a lot of state to hold in one control flow, especially in the turn-state persistence path.
Fix: Split the CAS-specific orchestration into smaller helpers, or move runner-lease overlay/cache bookkeeping behind a focused helper soapplycoordinates the transition at a higher level. - Medium The apply timeout is now defined in two places (
crates/ironclaw_turns/src/filesystem_store.rs:77-77, confidence 86) — anchor: crates/ironclaw_turns/src/filesystem_store.rs:77
The turn store keeps its own 15-secondFILESYSTEM_APPLY_TIMEOUT, while the shared CAS helper now owns the same 15-second loop timeout. The outer timeout comment says this may be shorter for tests, but the default value is still duplicated, so a future timeout-policy change can drift the turn store away from the helper contract.
Fix: Use the sharedironclaw_filesystem::FILESYSTEM_APPLY_TIMEOUTas the turn-store default, or remove the outer default when the helper deadline is the only intended policy.
| status: record.status, | ||
| }); | ||
| } | ||
| // Write a Discarded tombstone so the file still exists (preventing |
There was a problem hiding this comment.
Medium — Discarded approvals now keep the full request payload on disk.
discard_pending now rewrites the filesystem record as Discarded instead of deleting it, but it keeps the full original ApprovalRequest body in the tombstone. That means a cancelled approval's action, reason, requester, and fingerprint remain recoverable from disk/backups even though public APIs hide discarded records. This is a data-minimization regression on an approval control-plane record.
Fix: Store only the minimal tombstone data needed to reserve the request ID, or keep the discarded-ID reservation in a compact side index instead of retaining the full approval payload.
There was a problem hiding this comment.
Deferred (consistency, not a discard-specific bug). ApprovalRecord.request is non-optional and Approved/Denied records already retain the identical full payload on disk — the Discarded tombstone is consistent with every other terminal state. Singling out discard for minimization would be the inconsistency. If approval-payload retention matters it's a cross-cutting policy (pruning/redaction of all terminal approval records) plus a schema change (optional/redacted request), which is out of scope for this PR. Tracking as a retention follow-up.
| cas_retry_backoff(attempt).await; | ||
| } | ||
| // 5b. Backend cannot CAS-write — fail closed (no blind overwrite). | ||
| Err(FilesystemError::Unsupported { |
There was a problem hiding this comment.
Medium — Missing test for op-time unsupported-write fail-closed path.
The helper covers the pre-flight non-CAS rejection, but the second fail-closed path is untested: an unknown/default-capability backend can read successfully and then return FilesystemError::Unsupported { operation: WriteFile } from put. That branch maps to CasUpdateError::CasUnsupported and should be protected because it is the composite-router fallback path.
Fix: Add tests::cas::unsupported_write_file_maps_to_cas_unsupported using a default-capabilities backend whose read succeeds and whose CAS write returns Unsupported(WriteFile).
There was a problem hiding this comment.
Fixed in 578e971 — added unsupported_write_file_maps_to_cas_unsupported: a default/unknown-capability backend whose get succeeds and whose put returns Unsupported{WriteFile}, asserting cas_update maps it to CasUpdateError::CasUnsupported. This exercises the op-time fallback path, distinct from the already-tested pre-flight rejection.
| snapshot, | ||
| outcome, | ||
| write, | ||
| } = apply(current.clone()) |
There was a problem hiding this comment.
Medium — CAS helper clones the full snapshot on every retry.
cas_update clones the decoded snapshot before every apply, and several migrated call sites clone again when building CasApply::new(...). For large snapshots such as turn state, each write now copies the whole record at least twice, and contention multiplies that cost across the retry loop. That is avoidable churn on the hot persistence path this PR is trying to improve.
Fix: Pass apply a borrowed snapshot or split the helper API so the helper retains one owned copy for equality/write checks while call sites clone only when they truly need ownership.
There was a problem hiding this comment.
Deferred (low-value perf). Confirmed two in-memory clones per attempt: cas_update clones current (needed — retained for the equality check after apply consumes its copy) and the turns call site clones new_snapshot (threaded into outcome to populate the read cache). Both are struct copies, not IO, and the loop only re-clones on VersionMismatch (rare). Eliminating them requires an API change (return the final snapshot from cas_update, or hash-based equality) for marginal gain — premature without a profile showing turn-state writes are hot. Will revisit with a benchmark.
| // there is no lock contention in practice. | ||
| let apply = Arc::new(Mutex::new(apply)); | ||
|
|
||
| let cas_future = cas_update( |
There was a problem hiding this comment.
Medium — Turn-state apply has become a 1k-line orchestration knot.
This refactor pushes FilesystemTurnStateStore from 961 to 1051 lines, and the new apply flow now mixes cache invalidation, runner-lease overlay, transient store construction, FnMut adaptation, CAS no-op detection, timeout policy, and cache update/rollback in one method. That is a lot of state to hold in one control flow, especially in the turn-state persistence path.
Fix: Split the CAS-specific orchestration into smaller helpers, or move runner-lease overlay/cache bookkeeping behind a focused helper so apply coordinates the transition at a higher level.
There was a problem hiding this comment.
Agreed it's doing a lot, deferred to #5274. The clean move is extracting the per-retry transform (overlay + store build + no-op detection) into a named helper so apply reads as orchestration. Follow-up #5274 (migrate the runner-lease sidecar onto cas_update) already rewrites this exact apply/CAS path, so the decomposition lands there — once — rather than re-churning a just-merged file twice. Added to #5274's scope. (Note: most of the 961→1051 growth is main's own module split landing via the merge; net the file is smaller than this branch's pre-merge 1455.)
| /// unrelated callers behind one wedged operation. | ||
| pub const FILESYSTEM_CAS_RETRIES: usize = 32; | ||
| /// Deadline for the entire read-modify-write loop (including all retries). | ||
| pub const FILESYSTEM_APPLY_TIMEOUT: Duration = Duration::from_secs(15); |
There was a problem hiding this comment.
Medium — The apply timeout is now defined in two places.
The turn store keeps its own 15-second FILESYSTEM_APPLY_TIMEOUT, while the shared CAS helper now owns the same 15-second loop timeout. The outer timeout comment says this may be shorter for tests, but the default value is still duplicated, so a future timeout-policy change can drift the turn store away from the helper contract.
Fix: Use the shared ironclaw_filesystem::FILESYSTEM_APPLY_TIMEOUT as the turn-store default, or remove the outer default when the helper deadline is the only intended policy.
There was a problem hiding this comment.
Fixed in 578e971 — the turn store now uses the shared ironclaw_filesystem::FILESYSTEM_APPLY_TIMEOUT instead of a duplicated local constant; the per-store override via with_apply_timeout (used in tests) is unchanged.
Resolve import-union conflict in ironclaw_threads/filesystem_service.rs vs #5455 (row-native sequence primitive): keep this branch's cas_update imports (CasApply/CasUpdateError/RecordKind/cas_update for the ensure_thread migration) plus main's SeqNo (row-native sequence). Rest auto-merged: main's row-native reserve_sequence coexists with this branch's cas_update ensure_thread. Refresh the put_with_cas exception note (comment + filesystem CLAUDE.md): #5455 made reserve_sequence row-native, so the local CAS loop's callers are now write_new_message / reserve_sequence_via_thread_record (legacy fallback) / apply_message_update / append_capability_display_preview / create_summary_artifact + message indices. Tracked by #5469. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Remove Reborn per-record filesystem mutex convoys by migrating stores to a shared bounded CAS update helper.
Stats: 6 findings kept (from 8 raw final-head reviewer candidates, 6 after dedupe/filter) across 6 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 1.
tests
-
Medium Discarded approval gates lack interaction coverage (
crates/ironclaw_product_workflow/src/approval_interaction/service.rs:183-189, confidence 100) — anchor: crates/ironclaw_product_workflow/src/approval_interaction/service.rs:183
This diff adds ApprovalStatus::Discarded to the approve/deny stale-gate handling, but approval_interaction_contract has no Discarded fixture coverage. A regression could route a discarded tombstone through approval, persistent allow, or denial side effects instead of returning StaleGate.
Fix: Add approval_interaction_contract coverage such as discarded_gate_returns_stale_without_resolution for ApproveOnce, AlwaysAllow, and Deny on ApprovalStatus::Discarded. -
Medium Resource CAS retry exhaustion is untested through callers (
crates/ironclaw_resources/src/cas_snapshot.rs:263-267, confidence 75) — anchor: crates/ironclaw_resources/src/cas_snapshot.rs:265
CasSnapshotStore now maps CasUpdateError::RetriesExhausted into storage errors, and resources gained a retry loop it did not previously have. The helper test covers persistent VersionMismatch, and resource tests cover concurrency and CasUnsupported, but no resource caller test drives persistent VersionMismatch through FilesystemResourceGovernorStore or FilesystemBudgetGateStore to pin the caller-level error mapping.
Fix: Add a resource_governor_contract test such as filesystem_resource_governor_store_exhausts_cas_retries that uses a CAS-capable backend returning persistent VersionMismatch and asserts the ResourceError::Storage path. -
Medium Durable restart test no longer uses durable run-state storage (
crates/ironclaw_host_runtime/tests/reborn_durable_restart_integration.rs:51-55, confidence 75) — anchor: crates/ironclaw_host_runtime/tests/reborn_durable_restart_integration.rs:54
The approval restart test now shares one InMemoryBackend across service-graph rebuilds. That still covers service reconstruction, but it no longer proves the CAS-migrated run-state and approval-request records survive a real durable backend reopen like production libSQL/Postgres storage.
Fix: Add or retarget a feature-gated integration test that runs this approval resume flow over a CAS-capable durable backend reopen, for example libSQL, instead of only sharing an in-memory backend Arc.
conventions
- Low Resource store docs overstate writer overlap (
crates/ironclaw_resources/src/filesystem_store.rs:15-20, confidence 75) — anchor: AGENTS.md:84
The module docs say same-scope writers overlap instead of convoying, but this store still routes updates through CasSnapshotStore's single-consumer AsyncStorageWorker when handles are cloned. The lower-level CasSnapshotStore docs correctly say same-process writers sharing one handle still serialize, so this top-level guarantee is too broad.
Fix: Soften the docs to say backend CAS is lock-free and removes the per-record mutex, while the current sync worker bridge still serializes cloned-handle resource updates until the async-store follow-up.
local-patterns
-
Low CAS test comments point at stale source line numbers (
crates/ironclaw_filesystem/src/cas/tests.rs:578-578, confidence 100) — anchor: crates/ironclaw_filesystem/src/cas/tests.rs:578
The new test comment says the Unsupported-to-CasUnsupported arm is at cas.rs ~lines 337-340, but those lines now cover the VersionMismatch backoff branch. Other nearby regression comments use the same brittle approximate anchors, which sends future readers to the wrong branch while debugging the helper.
Fix: Drop the numeric cas.rs line references and describe the exercised branch by symbol or match arm instead. -
Low submit_turn comment still references the removed async lock (
crates/ironclaw_turns/src/filesystem_store.rs:612-614, confidence 100) (no diff position — body only) — anchor: crates/ironclaw_turns/src/filesystem_store.rs:612
This PR routes turn writes through cas_update, but submit_turn still says profile resolution runs early so the resolver future is not held across the per-path async lock. That stale mental model makes the current retry behavior harder to reason about.
Fix: Rewrite the comment around the current reason: resolve the profile once before the retryable CAS closure so retries reuse a pre-resolved resolver and do not repeat provider/profile resolution.
…mment accuracy Five reviewer comments (henrypark133, 2026-06-30 22:32): Tests (pin existing behavior; mutation-verified): - product_workflow: discarded_gate_returns_stale_without_resolution + expired_gate_returns_stale_without_resolution — ApprovalStatus::Discarded/ Expired gates return StaleGate for ApproveOnce/AlwaysAllow/Deny with zero resolution side effects (approval/denial/resume counts all 0). Neither status was fixtured before. - resources: filesystem_resource_governor_store_surfaces_storage_error_on_ persistent_version_mismatch — drives FilesystemResourceGovernorStore through a PersistentVersionMismatchBackend (ported from secrets' AlwaysRacingBackend) so cas_update exhausts all 32 retries, pinning CasUpdateError::RetriesExhausted -> ResourceError::Storage at the caller boundary. - host_runtime: approval_resume_survives_durable_libsql_reopen_and_consumes_ lease_once (#[cfg(feature = "libsql")]) — reopens a real on-disk LibSqlRoot Filesystem across each service-graph rebuild (vs the prior shared-InMemory backend), proving CAS-migrated run-state/approval records survive a durable reopen. DurableServices/mount helper generified over F: RootFilesystem; the in-memory test is unchanged. Docs/comments: - filesystem cas/tests.rs: replace all 8 brittle/stale numeric `cas.rs ~line N` anchors in regression comments with match-arm/symbol descriptions (3 were stale, pointing at the VersionMismatch backoff arm after line drift). - resources filesystem_store.rs module doc: scope the lock-free "overlap" claim to cross-process CAS contention; note same-process cloned-handle writers still serialize on AsyncStorageWorker until #5470 makes the store async. No production logic changed. Full --all-features clippy clean (pre-existing criterion bench deprecations only). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Remove Reborn per-record persistence lock convoys by migrating remaining stores to a shared mutex-free CAS update helper.
Stats: 0 findings (from 0 raw, 0 after dedup) across 0 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
No new issues survived review against head 1b0e97a30d14dd5f44eda97364365ad603a71151. Existing live review threads were treated as prior discussion and not duplicated; the current-head pass found the previously flagged fail-closed, no-op, retry, docs, and caller-level test gaps either fixed, explicitly deferred with rationale, or already covered by existing review comments.
…InMemory store parity (#5661) * fix(run_state): InMemoryApprovalRequestStore tombstones discard, not deletes (#5467) discard_pending removed the record outright, so a later save_pending for the same request id succeeded silently — diverging from FilesystemApprovalRequestStore's already-shipped (#5234) tombstone semantics. Mutate to ApprovalStatus::Discarded in place instead, filter Discarded out of get/records_for_scope, matching the filesystem store exactly. InMemoryApprovalRequestStore is LocalOnly per production_wiring.rs::classify_component_type, reachable only via a cargo build/run of ironclaw_reborn_cli with neither --features libsql nor --features postgres (Dockerfile.reborn always passes both, and tests/integration/ always compiles ironclaw_reborn_composition with libsql per root Cargo.toml, so it wires FilesystemApprovalRequestStore regardless of StorageMode). This fixes a genuine cross-backend divergence for the unfeatured local-dev path and crate-tier tests; it does not change any tests/integration/ or Docker-deployed behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(capabilities): pin discard-reuse invariant through the real rollback caller capability_host_does_not_orphan_approval_when_run_block_fails already drives host.rs's real save_pending-then-block_approval rollback path (the only production caller of discard_pending). Extend it to also prove the reuse-blocking invariant: after rollback, a fresh save_pending for the discarded request's id must fail closed with ApprovalRequestAlreadyExists, not silently succeed. Adds a RecordingApprovalStore spy (records the id at save_pending time, since run_state.fail() clears RunRecord.approval_request_id before the test can read it back). Confirmed RED pre-fix (temporarily reverted the prior commit's InMemoryApprovalRequestStore::discard_pending diff, restoring the old records.remove(&key) behavior): save_pending after the rollback returned Ok(..) instead of Err(ApprovalRequestAlreadyExists). Restored the fix and re-verified GREEN before committing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(integration): discard-then-resubmit coverage over the real approval store No int-tier test drove discard_pending before this (grep -rl discard tests/integration/ only matched a doubles-file doc comment). The only production caller (host.rs's rollback-on-partial-failure path) fires on a crash-window race a scripted submit_turn can't naturally trigger without new fault-injection harness surface, so this drives save_pending -> discard_pending -> save_pending directly against the group's real wired FilesystemApprovalRequestStore (reached via HostRuntimeCapabilityHarness::approval_requests_store(), mirroring how local_dev_approval_test_parts() exposes the same store elsewhere) rather than a hand-built store. Expected to pass today: documents already-correct filesystem-parity behavior shipped in #5234, does not pin a currently-broken invariant. Wired into approvals_group_e2e only (StorageMode::InMemory) -- the approval-request store is always on-disk regardless of StorageMode, same rationale as approval_request_persists_after_reopen's C-DURABLE note, so no StorageMode::LibSql variant is needed. Mutation-verified both assertions independently: - Commented out the discard_pending call (no-op-discard stub): the get()-must-be-None assertion failed with "discarded record must not be readable via get()". - Temporarily made FilesystemApprovalRequestStore::discard_pending delete the file after its CAS tombstone write (simulating the old in-memory-style delete bug): the reuse-block assertion failed with "expected save_pending to fail closed on a discarded request id". Reverted both mutations before committing; git diff against crates/ironclaw_run_state/src/lib.rs is clean (byte-identical to the prior commit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(run_state): trim discard-tombstone comments to lean form Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(capabilities): trim discard-reuse spy/test comments to lean form Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(integration): trim discard-then-resubmit comments to lean form Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(integration): real parallel CAS-contention scenario (#5466) scenario_concurrent_dual_gate_resume.rs's own module doc deferred this: "truly parallel read-modify-write turns against the shared CAS turn-state store is a separate, prod-relevant concern orthogonal to what this scenario proves." This is that follow-up: two threads tokio::join!ed through submit -> gate-resolve -> terminal-wait against the group's ONE shared FilesystemTurnStateStore<InMemoryBackend> (the same concrete CAS-over-RootFilesystem mechanism prod uses regardless of StorageMode). #5466 measured ~10% of single-attempt real-parallel exchanges landing on a sanitized exit_application_failed catch-all instead of Completed. Bounded retry (3 attempts, fresh run_ids each) plus a category allow-list keeps steady-state CI green without silently swallowing a different, unreported failure mode under the same category. Every tolerated retry emits a grep-able stdout line so a drift in the real flake rate is visible in CI logs, not just invisible until the retry bound itself starts failing. wait_for_terminal (builder.rs) extracts a shared private poll loop (poll_until_terminal_condition) also used by wait_for_status, so the 30s/10ms literals have one home instead of two copies to drift. StorageMode::LibSql is deliberately excluded: #5466 reports the libsql variant SIGABRTs the whole test process (consistent with SQLITE_MISUSE unwinding across an extern "C" boundary), which would crash every other test in this [[test]] binary. Verification: - Mutation-verified the cross-bleed assertion: swapped thread B's file-absence check to read thread A's path (which legitimately gets written) and confirmed the test fails with "workspace file parallel_a_1.txt exists ... but should not"; reverted. - Ran the scenario 20x locally (10x default, 10x --test-threads=1): all 20 passed cleanly with zero occurrences of the tolerated-flake log line. The local InMemoryBackend + fast filesystem does not reproduce #5466's race in this environment, so the retry path is unverified empirically here; its correctness rests on code review + the cross-bleed mutation-verify above, not an observed tolerated-flake run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(integration): restore byte-identical wait_for_status timeout text The poll_until_terminal_condition extraction (#5466 lane) lost wait_for_status's original "timed out waiting for {expected:?}" timeout wording, generalizing it to "timed out waiting for terminal condition" for both callers. No test parsed the exact text, but the lane's own review bar required byte-identical wait_for_status semantics across the extraction. Parameterize the shared loop with a timeout_context so wait_for_status keeps its original message and wait_for_terminal gets its own descriptive one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(integration): review fixes — doc exception + poll helper rename Document discard_then_resubmit as a store-direct exception in the group_approvals module doc, and rename poll_until_terminal_condition to poll_run_state_until since wait_for_status also uses it for non-terminal BlockedApproval/BlockedAuth waits. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: relocate discard-tombstone coverage to store contract tier Drop the store-direct discard_then_resubmit scenario from group_approvals: it carried a second execution model in a suite whose value is turn-driven E2E purity, and its assertions were already pinned at the store contract tier plus the real CapabilityHost rollback caller. Extend the run_state tombstone contract tests with the missing resolution path: approve()/deny() on a discarded id must reject with ApprovalNotPending(Discarded) on both stores (mutation-verified RED). Narrow the group module doc's gate-path guarantee accordingly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Problem
Each Reborn persistence store wrapped its filesystem read-modify-write in a per-record
tokio::sync::Mutex(FILESYSTEM_RECORD_LOCKS) held across.await— a redundant in-process serializer over backends that already do versioned CAS. Under burst, one writer stalled inside its critical section blocks every other writer for that scope (the convoy that contributed to the runtime wedge). PR #5142 removed exactly this fromironclaw_turns; this PR finishes the job for the remaining stores via one shared helper.Change
Shared helper (
ironclaw_filesystem::cas_update): one mutex-free, bounded read-modify-write loop — read versioned snapshot → idempotentapplyclosure → CAS put → onVersionMismatchre-read and retry (32×, jittered 2–50ms backoff, 15s timeout), with a fail-closed capability gate. Generic over record/outcome/error; leaks no store types.Migrated all 5 stores onto it and deleted every per-record mutex:
ironclaw_turns— re-homed its local fix(turns): prevent turn-state write convoy #5142 copy onto the shared helper (one owner).ironclaw_run_state,ironclaw_threads— dropped the mutex; route through the helper.ironclaw_resources— gains a retry loop it never had (the lock was its only serializer).ironclaw_secrets— deletes theArc-keyed lock map → fixes an unbounded memory leak (it storedArc, never pruned).Post-condition:
grep FILESYSTEM_RECORD_LOCKS / record_lock.lock().await / filesystem_secret_lockacross the 5 store crates is empty.Guardrail:
.claude/rules/database.mdrecords the invariant — filesystem read-modify-write must go throughcas_update; never wrap it in a per-record mutex held across.await.Tri-backend parity (no diverging experience by host)
The helper is
RootFilesystem-level (backend-agnostic). Verified across all threeironclaw_filesystembackends:--features postgres): CAS contract tests pass —postgres_native_put_cas_absent_rejects_existing_path,postgres_native_put_cas_any_increments_existing_version,postgres_transaction_rollback_discards_prior_put_after_later_cas_conflict.--features libsql): CAS contract tests pass. (One unrelated append/dir contract test is flaky under parallel suite execution — passes 3/3 in isolation;db.rsis untouched by this PR, so it is pre-existing test-infra flakiness, not a regression.)Notes
🤖 Generated with Claude Code