refactor(turns): simplify filesystem_store — retire blob store, dedup transitions, decompose the giant files - #6382
Merged
ilblackdragon merged 11 commits intoJul 21, 2026
Conversation
…teStoreKind enum
The blob `FilesystemTurnStateStore` and the two-arm `FilesystemTurnStateStoreKind`
dispatch enum were a production-dead parallel implementation: `Kind::blob` had
zero constructors anywhere, and the blob store's only non-dead entry point
(`with_filesystem_turn_state_store`) was test-only. Production composition,
the latency harness, and the stress tool all selected the row layout, and the
architecture ratchet already asserts `Row`. The blob path cost a full 5-trait
implementation plus ~420 lines of `match self { Blob => .., Row => .. }`
delegation, so every store-trait change was a 4x edit.
`FilesystemTurnStateRowStore` is now the one production turn-state store.
- Delete the blob store + `Kind` enum; keep the module wiring and
`FilesystemTurnStateBlockPersistence` (retained, and re-documented, purely as
the legacy `/turns/state.json` migration source the row store imports from).
- Remove 3 now-dead blob-only `RunnerLeaseStore` methods.
- Migrate every production consumer to the concrete row store: reborn
composition (alias/field/factory/local-dev/ratchet), `turn_run_snapshot`
(drop 2 dead impls), host_runtime builder, latency harness, stress tool
(drop its blob-only `Filesystem` backend variant).
- Migrate the test surface: mechanical rename across the crate/integration
suites; in `filesystem_turn_state_contract.rs` (the blob store's original
suite) delete blob-internal mechanics tests that have row-store equivalents,
rewrite the rest to assert through the row store's public API, and reseed the
legacy-blob migration fixtures via `FilesystemTurnStateBlockPersistence`.
Behavior-preserving. ironclaw_turns, host_runtime, runner, reborn_composition,
and ironclaw_architecture suites pass; affected reborn integration targets
compile.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…::queued Two duplication smells in the turn-state engine's submit path: 1. `submit_turn` / `submit_child_turn` manually re-ran `submit_idempotency_in_flight.remove(&key)` + `notify_waiters()` before every early return — but `SubmitInFlightGuard::drop` already does exactly that on every scope exit (and the manual calls double-fired the notify). Delete all 29 redundant remove/notify pairs; the RAII guard is now the single owner of in-flight release. Behavior is unchanged: the cached idempotency result is still written under the lock before return, and the guard notifies waiters after the lock drops. 2. A fresh `Queued` RunRecord was built from a ~28-field struct literal at three submit sites (`submit_turn`, `submit_child_turn`, `retry_turn_once`), 12 fields identical every time — so a new lease/gate/checkpoint field risked being defaulted inconsistently across sites. Add `RunRecord::queued( QueuedRunFields)` that fills the fresh-run defaults once; each caller sets only the few fields that differ (model route, checkpoint, lineage, product context) on the returned record. Net: submit_turn 214->178, submit_child_turn 373->321 lines, and the fresh-run default set lives in one place. Behavior-preserving; ironclaw_turns suite (15 binaries) and clippy --all-targets --all-features are green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…store
Leftovers from collapsing `TurnStateDurabilityPolicy` to the single write-behind
mode:
- `write_behind_async(&self, critical) -> bool { !critical }` — a `&self`
method that ignored `self`. Inline `!critical` at its call sites and delete
it.
- `PendingRowCommit.critical` — read only by a `debug_assert` in
`commit_pending`; the real await decision is `ack.is_none()`, and
`track_write_behind_ack_if_async` already nulls the ack on the non-critical
path. Drop the field (and the now-redundant assert); the struct is `{ value,
ack }`.
- `RowPersistError` — a single-variant enum wrapping exactly `TurnError`, with
`into_turn()`, an identity `From<TurnError>`, and ~10 `map_err`/match
conversion sites. Since it never diverged from `TurnError`, replace it with
`TurnError` throughout and delete the type and its conversions.
Pure dead-code / identity-wrapper removal; behavior-preserving. ironclaw_turns
suite (15 binaries) and clippy --all-targets --all-features are green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The identical "clear runner lease → stamp event cursor → release active-thread lock → remove queued slot" tail was copied verbatim across five terminal transitions (`cancel_completion_transition`, `terminal_transition`, `complete_claimed_record`, `cancel_claimed_record`, `fail_claimed_record`), and the bare three-field lease clear appeared again in `block_claimed_record` and `relinquish_transition`. A safety/lifecycle change to that tail had to land in all of them, one always at risk of lagging. Extract `Inner::release_terminal_lease` (the full terminal tail) and a free `clear_runner_lease` helper (the three lease fields), and route every site through them. Pure behavior-preserving extraction of identical statements — the divergent status-check / failure / checkpoint / event logic stays in each method. ironclaw_turns suite (15 binaries) + clippy green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… apply paths `apply` (whole-snapshot) and `apply_with_targeted_delta` computed the runner-lease overlay inputs (`overlay_baseline` / `overlay_run`) with the same eight lines. Extract `overlay_inputs(state, overlay)` and call it from both. Also tidy the `enqueue_delta` / `await_delta_ack` bodies left with dangling whitespace by the RowPersistError removal. Deliberately NOT merging the two apply functions into one strategy-parameterized commit loop: their cores differ exactly in the crash-consistency-critical parts — whole-snapshot diff + `RowSnapshotState` rebuild vs. a `build_delta` closure + incremental `apply_delta`, with different journal-seq reservation handling (the #6263 desync the inline comments guard against). Collapsing those into conditional branches would concentrate the WAL sequence invariants and make them harder to verify, not easier — the shared cost is scaffolding, not the risky core. Kept as two individually-readable commit strategies. Behavior-preserving; ironclaw_turns suite + clippy green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…Collection enum The row store identified its nine persisted collections with `&'static str` constants threaded as `collection: &'static str` through the mid-level read/write dispatch — a fixed dispatch set that `.claude/rules/types.md` requires be an enum. Introduce a private `RowCollection` enum with `as_str()` returning the exact historical path segment for each variant, and thread `RowCollection` through the selector fns (`read_row_collection`, `read_row_by_key`, `put_row`, `delete_row`, `write_materialized_row`, the `materialize_delta` arms). The low-level path primitives in `io.rs` keep taking `&str` — a filesystem path segment is a legitimate string boundary — with callers converting via `collection.as_str()` at exactly that boundary. Strictly behavior-preserving: `as_str()` returns byte-identical strings, so the on-disk layout and legacy-blob migration are unchanged; a new unit test pins each variant's segment. ironclaw_turns suite + clippy --all-targets --all-features green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`turn_state_engine.rs` had grown to ~4,310 lines (well past the 1,500/3,000
architecture thresholds; it carried an `arch-exempt: large_file`). Split it into
`turn_state_engine/{mod,limits,run_record,idempotency,snapshot,admission,
spawn_tree,transitions}.rs`:
- mod.rs (1,397) — TurnStateEngine/Inner/RunRecord/key struct defs, RunStatusCell,
small shared free fns, impl TurnStateEngine, and the
TurnEventProjectionSource/LoopCheckpointStore/TurnStateStore trait impls.
- transitions.rs (1,451) — the Inner run-transition family + impl
TurnRunTransitionPort + lease recovery + AppliedLoopTransition.
- spawn_tree.rs (515) — impl TurnSpawnTreeStateStore.
- idempotency.rs (340) — the idempotency ledger methods + record builders.
- snapshot.rs (309) — from_persistence_snapshot / persistence_snapshot.
- run_record.rs (116), limits.rs (150), admission.rs (73).
Move-only: bodies are unchanged; struct definitions with private fields stay in
mod.rs so descendant submodules keep field access, and only the handful of
cross-module `Inner`/`RunRecord` methods and free fns were widened from
module-private to `pub(super)` (still crate-internal — the engine is
`pub(crate)`). Public paths (`turn_state_engine::TurnStateEngine`,
`::TurnStateStoreLimits`, `::DEFAULT_RUNNER_LEASE_TTL_SECONDS`) are unchanged; no
file now exceeds 1,500 lines, so the `arch-exempt: large_file` header is dropped.
Also runs `cargo fmt -p ironclaw_turns`, cleaning formatting drift left by the
earlier import/perl edits in this branch (row_store*.rs and the migrated test
files). ironclaw_turns suite (15 binaries) + clippy --all-targets --all-features
green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…mmit submodules `row_store.rs` (the row-store module root) was ~1,597 lines, over the 1,500 threshold and carrying two `arch-exempt: large_file` markers. Split the big `impl FilesystemTurnStateRowStore` into three cohesive child modules: - row_store.rs (527) — struct/enum defs (incl. RowCollection), constructors + builders, observability accessors, snapshot-cache read helpers, runner-lease glue, span helpers, mod/use decls. - row_store/commit.rs (490) — the two apply engines (`apply`, `apply_with_targeted_delta`), overlay acquisition, and the run-state transition wrappers. - row_store/load.rs (411) — durable rehydration, legacy-blob migration, and the materialized-row / durable readers. - row_store/write_behind.rs (247) — the write-behind window: reserve / enqueue / track / commit_pending / await / drain / degradation guards. Move-only: struct fields stay in row_store.rs so submodules keep field access; only cross-module methods were widened from private to `pub(super)` (still crate-internal). No public API change; every file is now under 900 lines and both `arch-exempt: large_file` markers are dropped. ironclaw_turns suite (15 binaries) + clippy --all-targets --all-features + fmt --check green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
🔎 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. |
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6382
July 21, 2026 00:29
Destroyed
github-actions
Bot
added
size: XL
|
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 (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pull request consolidates filesystem turn-state persistence around ChangesTurn-state engine semantics
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
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 |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
⏭️ IronLoop Review Declined: reviewer
Review at a glance
| Disposition | Head |
|---|---|
| ⏭️ Review declined | 6273ccc2e110 |
Head: 6273ccc2e110a828c11c8ef99e48496bb1566b7a
Reason: The supplied base 034c177 and head 6273ccc have no common merge base, so the requested PR diff includes unrelated repository history and cannot be attributed reliably to this pull request.
Next: Re-run review from a checkout containing a shared merge base for the supplied base and head, or provide the correct PR base SHA. Then review the exact merge-base-to-head diff rather than the unrelated tree comparison.
Run details
Status: Current
Trustworthy review produced: no
Summary
Skipped: the supplied base-to-head comparison cannot be reviewed reliably because the references have no common ancestor in this checkout. The exact tree diff is a 758-file, 411,870-line mega diff spanning unrelated subsystems, while the reachable refactor commit chain covers only 45 files and is not the requested base comparison.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request completely removes the legacy blob-based FilesystemTurnStateStore and the FilesystemTurnStateStoreKind enum, establishing FilesystemTurnStateRowStore as the sole production turn-state store. As part of this refactoring, the implementation of FilesystemTurnStateRowStore has been decomposed into dedicated submodules (commit, load, and write_behind) to improve maintainability. All dependent crates, integration tests, and stress-testing tools have been updated to use the row-based store directly. Since there are no review comments provided, I have no additional feedback to offer.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/factory.rs`:
- Around line 2687-2696: The turn-state setup around FilesystemTurnStateRowStore
must classify its InMemoryBackend-backed instance as LocalOnly in
classify_component_type so the fail-closed wiring check applies. Correct the
nearby wiring note to state that writes are generally write-behind, only
gate-park, terminal, and brand-new-run transitions are synchronous, and shutdown
must still call drain() for pending writes.
In `@crates/ironclaw_turns/src/filesystem_store/turn_state_engine/admission.rs`:
- Around line 15-17: Preserve the source error from
limit_provider.limit_for(bucket) before converting it to
AdmissionRejectionReason::Unavailable. Update the admission flow around
limit_for to log the bound provider error, or use an existing cause-preserving
AdmissionRejection constructor, instead of discarding it with map_err(|_| ...).
In `@crates/ironclaw_turns/src/filesystem_store/turn_state_engine/transitions.rs`:
- Around line 1413-1419: Update relinquish_run to inspect the result of
relinquish_transition and invoke persist_terminal_cleanup for terminal Cancelled
outcomes, matching complete_run, cancel_run, fail_run, and
record_runner_failure. Preserve non-terminal results and errors, and add a
regression test covering CancelRequested -> Cancelled when block persistence is
enabled.
🪄 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: 12e01cf6-2bee-4aac-87aa-0065c2c88830
📒 Files selected for processing (45)
crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rscrates/ironclaw_host_runtime/src/services.rscrates/ironclaw_host_runtime/src/services/builder.rscrates/ironclaw_host_runtime/tests/host_runtime_services_contract.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/runtime/test_support.rscrates/ironclaw_reborn_composition/src/test_support/automation.rscrates/ironclaw_reborn_composition/src/turn_run_snapshot.rscrates/ironclaw_reborn_composition/tests/runtime.rscrates/ironclaw_runner/tests/loop_driver_host.rscrates/ironclaw_turns/src/filesystem_store.rscrates/ironclaw_turns/src/filesystem_store/row_store.rscrates/ironclaw_turns/src/filesystem_store/row_store/commit.rscrates/ironclaw_turns/src/filesystem_store/row_store/delta.rscrates/ironclaw_turns/src/filesystem_store/row_store/journal.rscrates/ironclaw_turns/src/filesystem_store/row_store/load.rscrates/ironclaw_turns/src/filesystem_store/row_store/traits.rscrates/ironclaw_turns/src/filesystem_store/row_store/write_behind.rscrates/ironclaw_turns/src/filesystem_store/runner_lease.rscrates/ironclaw_turns/src/filesystem_store/tests.rscrates/ironclaw_turns/src/filesystem_store/turn_state_engine.rscrates/ironclaw_turns/src/filesystem_store/turn_state_engine/admission.rscrates/ironclaw_turns/src/filesystem_store/turn_state_engine/idempotency.rscrates/ironclaw_turns/src/filesystem_store/turn_state_engine/limits.rscrates/ironclaw_turns/src/filesystem_store/turn_state_engine/mod.rscrates/ironclaw_turns/src/filesystem_store/turn_state_engine/run_record.rscrates/ironclaw_turns/src/filesystem_store/turn_state_engine/snapshot.rscrates/ironclaw_turns/src/filesystem_store/turn_state_engine/spawn_tree.rscrates/ironclaw_turns/src/filesystem_store/turn_state_engine/transitions.rscrates/ironclaw_turns/src/lib.rscrates/ironclaw_turns/src/test_support.rscrates/ironclaw_turns/tests/filesystem_turn_state_contract.rscrates/ironclaw_turns/tests/loop_checkpoint_store_contract.rscrates/ironclaw_turns/tests/retry_failed_turn_store_contract.rsharness/latency/runner/src/main.rstests/integration/auth/reopen_resume_through_gate.rstests/integration/group_approvals/scenario_concurrent_dual_gate_resume_parallel.rstests/integration/group_multiuser/scenario_turn_state_isolation_across_actors.rstests/integration/support/builder.rstests/integration/support/group.rstests/integration/support/harness/mod.rstests/support/reborn_parity_qa/binary_e2e.rstools/ironclaw_stress/src/main.rstools/ironclaw_stress/src/tests.rstools/ironclaw_stress/src/user_turn.rs
💤 Files with no reviewable changes (2)
- crates/ironclaw_turns/src/filesystem_store/tests.rs
- crates/ironclaw_turns/src/filesystem_store/runner_lease.rs
Comment on lines
+2687
to
2696
| // Turn state runs the production `FilesystemTurnStateRowStore` over a | ||
| // dedicated volatile `InMemoryBackend` (§4.3) — no bespoke private | ||
| // turn-state engine standalone authority. Matches the sibling | ||
| // run-state/approval stores in this build (volatile, `LocalOnly`); the row | ||
| // store persists every transition synchronously, so this build needs no | ||
| // shutdown drain. | ||
| let turn_state = Arc::new( | ||
| FilesystemTurnStateStoreKind::row(crate::wrap_scoped(Arc::new(InMemoryBackend::new()))) | ||
| FilesystemTurnStateRowStore::new(crate::wrap_scoped(Arc::new(InMemoryBackend::new()))) | ||
| .with_limits(turn_state_store_limits), | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify if FilesystemTurnStateRowStore<InMemoryBackend> is properly classified as LocalOnly.
# Test: Search for FilesystemTurnState in production_wiring.rs classification function
rg "FilesystemTurnState" crates/ironclaw_host_runtime/src/services/production_wiring.rs -A 2 -B 2Repository: nearai/ironclaw
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== factory.rs slice ==\n'
sed -n '2668,2710p' crates/ironclaw_reborn_composition/src/factory.rs
printf '\n== search FilesystemTurnStateRowStore definitions/usages ==\n'
rg -n "FilesystemTurnStateRowStore|classify_component_type|ProductionCandidate|LocalOnly" crates -g '*.rs'
printf '\n== production_wiring.rs outline ==\n'
ast-grep outline crates/ironclaw_host_runtime/src/services/production_wiring.rs --view expanded || trueRepository: nearai/ironclaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== production_wiring classify slice ==\n'
sed -n '280,360p' crates/ironclaw_host_runtime/src/services/production_wiring.rs
printf '\n== row_store write-behind docs slice ==\n'
sed -n '1,220p' crates/ironclaw_turns/src/filesystem_store/row_store/write_behind.rs
printf '\n== row_store commit/traits slice around drain ==\n'
sed -n '1,180p' crates/ironclaw_turns/src/filesystem_store/row_store/traits.rsRepository: nearai/ironclaw
Length of output: 23080
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== production_wiring classify slice ==\n'
sed -n '280,360p' crates/ironclaw_host_runtime/src/services/production_wiring.rs
printf '\n== host_runtime_services_contract turn-state assertions ==\n'
sed -n '700,730p' crates/ironclaw_host_runtime/tests/host_runtime_services_contract.rs
printf '\n== row_store write-behind docs slice ==\n'
sed -n '1,220p' crates/ironclaw_turns/src/filesystem_store/row_store/write_behind.rsRepository: nearai/ironclaw
Length of output: 16135
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== builder turn-state wiring slice ==\n'
sed -n '460,530p' crates/ironclaw_host_runtime/src/services/builder.rs
printf '\n== production_services turn-state issue handling slice ==\n'
sed -n '330,390p' crates/ironclaw_host_runtime/src/services/production_services.rs
printf '\n== tests around turn-state classification ==\n'
sed -n '700,730p' crates/ironclaw_host_runtime/tests/host_runtime_services_contract.rsRepository: nearai/ironclaw
Length of output: 6419
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== turn-state wiring references ==\n'
rg -n "TurnState|FilesystemTurnStateRowStore<InMemoryBackend>|LocalOnlyImplementation" \
crates/ironclaw_host_runtime/src/services/builder.rs \
crates/ironclaw_host_runtime/src/services/production_services.rs \
crates/ironclaw_host_runtime/tests/host_runtime_services_contract.rsRepository: nearai/ironclaw
Length of output: 7005
Fix the turn-state wiring note and readiness mapping. FilesystemTurnStateRowStore<InMemoryBackend> is missing from classify_component_type, so it falls through to ProductionCandidate instead of LocalOnly and bypasses the fail-closed wiring check. The comment also overstates durability: the row store has a write-behind path, and only gate-park/terminal/brand-new-run transitions are synchronous — drain() still matters for the tail.
🤖 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/factory.rs` around lines 2687 - 2696,
The turn-state setup around FilesystemTurnStateRowStore must classify its
InMemoryBackend-backed instance as LocalOnly in classify_component_type so the
fail-closed wiring check applies. Correct the nearby wiring note to state that
writes are generally write-behind, only gate-park, terminal, and brand-new-run
transitions are synchronous, and shutdown must still call drain() for pending
writes.
Comment on lines
+15
to
+17
| let limit = limit_provider | ||
| .limit_for(bucket) | ||
| .map_err(|_| AdmissionRejection::new(AdmissionRejectionReason::Unavailable))?; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
map_err(|_| …) drops the limit-provider cause. A limit_for failure collapses to Unavailable with no bound source, so an admission-provider outage is undiagnosable from logs. Log the source before mapping (or use a cause-preserving constructor).
As per coding guidelines: "Do not use .map_err(|_| OtherError) when it discards the original cause … log the bound source before mapping; comments cannot exempt dropped causes."
🤖 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/turn_state_engine/admission.rs`
around lines 15 - 17, Preserve the source error from
limit_provider.limit_for(bucket) before converting it to
AdmissionRejectionReason::Unavailable. Update the admission flow around
limit_for to log the bound provider error, or use an existing cause-preserving
AdmissionRejection constructor, instead of discarding it with map_err(|_| ...).
Source: Coding guidelines
Comment on lines
+1413
to
+1419
| async fn relinquish_run( | ||
| &self, | ||
| request: RelinquishRunRequest, | ||
| ) -> Result<TurnRunState, TurnError> { | ||
| let mut inner = self.lock_inner()?; | ||
| inner.relinquish_transition(request.run_id, request.runner_id, request.lease_token) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and nearby symbols.
ast-grep outline crates/ironclaw_turns/src/filesystem_store/turn_state_engine/transitions.rs --view expanded | sed -n '1,240p'
echo
echo "=== symbol search ==="
rg -n "persist_terminal_cleanup|relinquish_transition|relinquish_run|cancel_run|record_runner_failure|is_gate_persisted|clear_gate_persisted|gate_persisted" crates/ironclaw_turns/src/filesystem_store/turn_state_engine/transitions.rsRepository: nearai/ironclaw
Length of output: 4473
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '880,980p' crates/ironclaw_turns/src/filesystem_store/turn_state_engine/transitions.rs
echo
sed -n '1320,1465p' crates/ironclaw_turns/src/filesystem_store/turn_state_engine/transitions.rsRepository: nearai/ironclaw
Length of output: 8847
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the cleanup helper and any direct callers of relinquish_run.
sed -n '1465,1565p' crates/ironclaw_turns/src/filesystem_store/turn_state_engine/transitions.rs
echo
echo "=== relinquish_run callers ==="
rg -n "relinquish_run\(" crates/
echo
echo "=== persist_terminal_cleanup references ==="
rg -n "persist_terminal_cleanup\(" crates/ironclaw_turns/src/filesystem_store/turn_state_engine/transitions.rsRepository: nearai/ironclaw
Length of output: 2731
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "fn persist_terminal_cleanup|fn is_gate_persisted|fn mark_gate_persisted|fn clear_gate_persisted|block_persistence|persist_blocked_state|struct .*TurnStateEngine" crates/ironclaw_turns/src/filesystem_store/turn_state_engine/transitions.rs
echo
sed -n '1460,1545p' crates/ironclaw_turns/src/filesystem_store/turn_state_engine/transitions.rsRepository: nearai/ironclaw
Length of output: 390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the terminal-cleanup helper and inspect the direct caller that mentions it.
rg -n "persist_terminal_cleanup|gate_persisted|block_persistence" crates/ironclaw_turns/src crates/ironclaw_turns/tests crates/ironclaw_loop_host/src crates/ironclaw_runner/src | sed -n '1,220p'
echo
echo "=== lifecycle relinquish path ==="
sed -n '730,780p' crates/ironclaw_turns/src/lifecycle.rs
echo
echo "=== runner relinquish path ==="
sed -n '200,240p' crates/ironclaw_turns/src/runner.rsRepository: nearai/ironclaw
Length of output: 9080
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '700,735p' crates/ironclaw_turns/src/filesystem_store/turn_state_engine/mod.rsRepository: nearai/ironclaw
Length of output: 1792
relinquish_run needs terminal cleanup for gate-persisted runs
relinquish_transition() can return terminal Cancelled from CancelRequested, but this wrapper is the only terminal path that skips persist_terminal_cleanup(). That leaves a gate-persisted snapshot stale across restart; complete_run, cancel_run, fail_run, and record_runner_failure already converge this same state. Call the cleanup helper here too and add a regression test for CancelRequested -> Cancelled with block persistence.
🤖 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/turn_state_engine/transitions.rs`
around lines 1413 - 1419, Update relinquish_run to inspect the result of
relinquish_transition and invoke persist_terminal_cleanup for terminal Cancelled
outcomes, matching complete_run, cancel_run, fail_run, and
record_runner_failure. Preserve non-terminal results and errors, and add a
regression test covering CancelRequested -> Cancelled when block persistence is
enabled.
Run `cargo fmt --all` to fix formatting drift in the consumer files touched by the blob-store retirement (host_runtime services, integration support builder, reborn_parity_qa binary_e2e, stress user_turn) — the earlier commits fmt'd only the ironclaw_turns crate, tripping the workspace `Formatting` CI gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🚅 Deployed to the ironclaw-pr-6382 environment in ironclaw-ci-preview
|
…tFilesystem `BlockingTurnStatePutFilesystem` (the parity/integration harness turn-state backend wrapper) only forwarded the blob surface (put/get/list/query/stat/ delete) — the whole API the retired blob store used. The row store, now wired into these harnesses, drives a delta journal, whose `append`/`tail`/`head_seq`/ `reserve_sequence` (and `begin`/`append_batch`/`tail_bounded`/`delete_if_version`) default to `Unsupported` on `RootFilesystem`. Wrapping `InMemoryBackend` (which implements them) hid that support, so the first turn submit returned `Unavailable` — reproduced by `reborn_adapter_installation_scope_isolation_parity` (WorkflowRejected Unavailable/503) and the integration `HarnessTurnStorageBackend` paths. Forward all eight journal/txn methods to the inner backend, keeping the wrapper a transparent pass-through except its one blob-`put` blocking hook. Regression proof: the parity suite goes 16/1 -> 17/0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6382
July 21, 2026 00:56
Destroyed
…omment Two follow-ups after the row store replaced the blob store in the harnesses: - `assert_turn_event_recorded` checked the sink exactly once, but turn lifecycle events publish best-effort AFTER the status transition the caller waited on (`wait_for_status` reads the store's hot-cache status, set synchronously by the transition; the sink publish runs just after it returns). The row store makes status observable sooner than the blob store did, widening that pre-existing race so `group_thread_does_not_see_a_sibling_threads_turn_event` intermittently saw `[Submitted, RunnerClaimed]` without the trailing `Completed`. Poll the sink briefly (<=2s) instead of checking once. Production event ordering (status-then-best-effort-event) is unchanged; only the test's eagerness is fixed. Verified 5/5 green. - The local-dev factory comment claimed the row store "persists every transition synchronously, so this build needs no shutdown drain." That overstates durability — only gate-park/terminal/new-run transitions flush synchronously; the rest are write-behind. Corrected: no drain is needed here because the `InMemoryBackend` is volatile (a restart discards it), not because writes are synchronous. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6382
July 21, 2026 01:33
Destroyed
Member
Author
|
Triage of the CodeRabbit findings (this is a behavior-preserving refactor; each verified against Fixed (regressions this PR introduced) — pushed in
Pre-existing, not introduced here — deferred to a focused follow-up (kept out of this move-only/refactor PR per
|
Contributor
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 86.4% — 321085 / 371615 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)
|
6 tasks
7 tasks done
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
A thermo-nuclear maintainability pass over
crates/ironclaw_turns/src/filesystem_storeand its callers. The module carried a production-dead parallel store implementation, several duplication-divergence families in the crash-safety-critical transition/commit code,#6263durability-collapse residue, a stringly-typed dispatch set, and two files well past the file-size thresholds.Every commit is behavior-preserving and independently green (
cargo build,cargo clippy -p ironclaw_turns --all-targets --all-featureszero-warning,cargo test -p ironclaw_turns, plus thehost_runtime/runner/reborn_composition/architecturesuites and the reborn integration targets). Net −2,483 lines (the insertions are mostly moved code from the two module splits).Commits (each self-contained, reviewable in order)
Retire the blob store +
FilesystemTurnStateStoreKindenum.Kind::blobhad zero constructors anywhere and the blob store's only non-dead entry point was test-only; production, the latency harness, and the stress tool all selected the row layout, and the architecture ratchet already assertsRow. The blob path cost a full 5-trait implementation plus ~420 lines ofmatch self { Blob => .., Row => .. }dispatch (a 4× edit on every store-trait change).FilesystemTurnStateRowStoreis now the one production store;FilesystemTurnStateBlockPersistenceis retained purely as the legacy-blob migration source. Migrated every consumer + the contract suites.Make
SubmitInFlightGuardload-bearing; addRunRecord::queued.submit_turn/submit_child_turnmanually re-ranin_flight.remove + notify_waitersbefore every early return — redundant with the guard'sDrop(and double-firing the notify). Deleted all 29 pairs. Replaced the 3× ~28-field fresh-runRunRecordliteral with one constructor so a new lease/gate field gets its default in one place.Delete
#6263durability-collapse residue.write_behind_async(a&selfmethod returning!critical),PendingRowCommit.critical(read only by a debug-assert), and the single-variantRowPersistErrorwrapper.Extract the shared terminal-transition tail.
release_terminal_lease+clear_runner_leasereplace the identical "clear lease → cursor → release lock → remove queued" block copied across 5 terminal transitions + 2 more sites.Share overlay-input capture across the two apply paths. Deduped the overlay setup. Deliberately did not merge the two commit cores — their journal-seq / crash-consistency handling genuinely differs (whole-snapshot diff +
RowSnapshotStaterebuild vs.build_delta+ incrementalapply_delta); collapsing them would concentrate the WAL sequence invariants into conditional branches, harder to verify.RowCollectionenum. Replaced the 9 stringly-typed collection-id&'static strconsts (per.claude/rules/types.md);as_str()returns byte-identical on-disk segments (migration-compatible), pinned by a test.Decompose
turn_state_engine.rs4,310 →mod.rs(1,397) + 7 submodules (transitions,spawn_tree,idempotency,snapshot,run_record,limits,admission); no file over 1,500. Move-only.Decompose
row_store.rs1,597 → 527 +commit/load/write_behindsubmodules. Move-only.Deliberate non-changes (flagged for reviewer override)
applycommit engines are kept separate (see commit 5) — a merge would obscure the crash-safety journal-seq invariants the inline#6263comments guard.Option-wrapped lease predicates and the lease store's non-Optionones are kept as two type-appropriate implementations — a shared helper would be a boolean-flag/view-trait smell (.claude/rules/types.md/ thin-abstraction guidance).Testing
ironclaw_turns(15 test binaries) + the four dependent-crate suites are green; reborn integration targets compile. The one unrelated failure in local runs (llm_admin::...::detect_env_llm_is_none_with_no_llm_env_vars_set) is a pre-existing environment-sensitivity (the dev shell exports LLM env vars) and fails onmaintoo.🤖 Generated with Claude Code