Move runner lease heartbeats to memory store - #5452
Conversation
📝 WalkthroughWalkthroughReplaces the filesystem-backed ChangesRunner lease: filesystem sidecar → in-memory store
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Code Review
This pull request refactors the FilesystemTurnStateStore to use an in-memory store (RunnerLeaseMemory) for high-churn runner lease heartbeats instead of persisting them as filesystem-backed JSON sidecars. This change eliminates the need for filesystem CAS retries and durable writes during heartbeats, while still overlaying active leases onto the durable snapshot. Corresponding contract tests have been updated to reflect the memory-backed behavior and verify that no durable sidecar records are materialized. There are no review comments, and I have no additional feedback to provide.
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.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_turns/src/filesystem_store/runner_lease.rs`:
- Around line 33-38: `RunnerLeaseStore` is using a blocking `std::sync::Mutex`
for shared lease state on the async heartbeat/overlay/cancel path, which can
stall Tokio workers and bypass `apply_timeout`. Update `RunnerLeaseMemory` and
the `RunnerLeaseStore` access paths to use an async-compatible lock (per the
repo’s shared async state pattern, e.g. `Arc<T>` with `RwLock`/async lock) so
callers like the heartbeat and cancel methods await lock acquisition instead of
blocking. Make sure every place that currently calls `lock()` on the leases map
is switched to the async lock API and keeps the timeout behavior intact.
- Around line 276-279: The lazy seeding path in runner_lease should be made
insert-if-absent under a single map lock: the current check in the read path
followed by the unconditional seed_from_snapshot_inner call can race with
concurrent cancellation/retirement and overwrite a newer lease state. Update the
seeding flow around read, seed_from_snapshot_inner, and the map guard logic so
the existence check and insertion happen atomically while holding the same lock,
and only seed when the entry is still absent.
🪄 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: 2ca2761b-1f03-4445-b9a1-38ebab98c9d7
📒 Files selected for processing (3)
crates/ironclaw_turns/src/filesystem_store.rscrates/ironclaw_turns/src/filesystem_store/runner_lease.rscrates/ironclaw_turns/tests/filesystem_turn_state_contract.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_turns/src/filesystem_store/runner_lease.rs (1)
252-267: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not let claim seeding downgrade an existing lease.
Line 266 routes through
upsert, and Line 334 unconditionally inserts. A late seed from the durable claim snapshot can overwrite a newer in-memoryCancelRequested/terminal lease for the same run back toRunning, reopening heartbeats and breaking two-phase cancellation.Proposed fix
async fn seed_from_snapshot_inner( &self, snapshot: &TurnPersistenceSnapshot, run_id: TurnRunId, ) -> Result<(), TurnError> { - let Some(run) = snapshot.runs.iter().find(|record| record.run_id == run_id) else { - return Err(TurnError::ScopeNotFound); - }; - let Some(record) = runner_lease_from_run(run) else { - return Err(TurnError::InvalidTransition { - from: run.status, - to: TurnStatus::Running, - }); - }; - self.upsert(record).await + let record = runner_lease_from_snapshot(snapshot, run_id)?; + let mut leases = self.leases.write().await; + match leases.get(&run_id) { + Some(existing) + if existing.runner_id == record.runner_id + && existing.lease_token == record.lease_token => + { + Ok(()) + } + Some(_) => Err(TurnError::LeaseMismatch), + None => { + leases.insert(record.run_id, record); + Ok(()) + } + } }As per coding guidelines, “running cancellation is two-phase” and active locks must be transitioned/released exactly once.
Also applies to: 333-335
🤖 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/runner_lease.rs` around lines 252 - 267, Prevent snapshot seeding from overwriting a newer lease state: in seed_from_snapshot_inner and the upsert path it calls, only create or refresh the runner lease when no lease exists or when the existing lease is compatible with the snapshot state, and never downgrade an existing CancelRequested or terminal lease back to Running. Update the RunnerLease/runner_lease_from_run flow so late durable claim snapshots are ignored or merged without reopening heartbeats, and ensure any transition/release logic remains single-shot and monotonic.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/ironclaw_turns/src/filesystem_store/runner_lease.rs`:
- Around line 252-267: Prevent snapshot seeding from overwriting a newer lease
state: in seed_from_snapshot_inner and the upsert path it calls, only create or
refresh the runner lease when no lease exists or when the existing lease is
compatible with the snapshot state, and never downgrade an existing
CancelRequested or terminal lease back to Running. Update the
RunnerLease/runner_lease_from_run flow so late durable claim snapshots are
ignored or merged without reopening heartbeats, and ensure any
transition/release logic remains single-shot and monotonic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 64747c2c-b4f8-46c7-a637-a7f088f72db5
📒 Files selected for processing (2)
crates/ironclaw_turns/src/filesystem_store.rscrates/ironclaw_turns/src/filesystem_store/runner_lease.rs
|
🚅 Deployed to the ironclaw-pr-5452 environment in ironclaw-ci-preview
|
Adopt the runner-lease heartbeat-in-memory implementation from the merged #5452, dropping this branch's overlapping reimplementation (carried over from #5453): take main's `crates/ironclaw_turns/src/filesystem_store.rs`, `runner_lease.rs`, and `filesystem_turn_state_contract.rs` verbatim. This PR's unique work — the `reserve_sequence` filesystem primitive and the thread append-path storage — is independent of the runner-lease code and is preserved. Verified ironclaw_turns/threads/filesystem/host_runtime/ first_party_extensions compile against merged main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PQzj2447pq2BUfgY3BUSJb
Reconcile lock-convoy removal (PR #5234) with main: - resources (#5447): keep opt-in unlimited fast-path skip + lock-free inspect() reads; route durable writes through cas_update. update bound FnOnce->FnMut so cas_update can retry; fast-path closures clone instead of move. read_snapshot/decode_snapshot kept; dead mutex update_snapshot/ put_with_cas/PutError removed from cas_snapshot.rs. - turns (#5452): lease heartbeats stay memory-backed; durable state.json RMW goes through cas_update. Fixed latent break where apply() closure still built deleted RunnerLeaseSidecar (now RunnerLeaseStore over the in-memory map). Removed now-dead CAS-retry island from filesystem_store/ io.rs (put_with_cas/cas_retry_backoff/PutError + consts) — both callers gone (durable path -> cas_update, fs lease sidecar deleted by #5452). - test support (session_thread.rs): generify RebornThreadHarness for both InMemoryBackend (default tier) and CompositeRootFilesystem tiers. Invariants preserved: zero lock().await-across-await in durable RMW; secrets Arc-keyed lock-map stays deleted; all durable RMW via cas_update. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…privately in the row store; fix the divergences it surfaced (#6263 Step 5c) The former public InMemoryTurnStateStore was the row store's execution engine dressed as a second store. Move it into filesystem_store as a private TurnStateEngine (crate-visible only), rename InMemoryTurnStateStoreLimits -> TurnStateStoreLimits, and migrate every consumer (42 files) to the one public turn-state store, FilesystemTurnStateRowStore<InMemoryBackend>, via a single shared `in_memory_turn_state_store()` test-double helper. Delete the public type. FROZEN_DEBT_INMEMORY_STORES is now EMPTY — the §4.3 store-consolidation axis reaches zero. Running the engine's contract tests against the row store for the first time surfaced that the row store was not a faithful wrapper of its own engine. Fixed the genuine divergences (each with regression coverage): - put_loop_checkpoint only patched the cached snapshot, not the cached engine, so a later fail_run found no checkpoint and made failed / lease-expired runs non-retryable (caught by the 8 retry-contract tests a naive dedup would have deleted); - retry_turn clobbered the linked checkpoint and dropped the persisted idempotency record on ThreadBusy; - submit/cancel admission-rejection idempotency records were discarded on Err instead of committed, so a duplicate re-evaluated instead of replaying; - terminal-outcome lease prep synthesized InvalidTransition before the engine could surface LeaseMismatch; - a reentrant observability read deadlocked against an in-flight submit. Coordinator/runner contract tests that encoded the instant in-memory engine's behavior were corrected to the row store's (more durable / realistic) contract, not weakened: heartbeat does not advance the cursor (#5452); durably-retained lifecycle events are served, not rebased; lease-recovery clocks use max() not min(); resume asserts the Resumed event by presence, not last position; manual-drive tests stop the auto-started scheduler so it cannot race their claims; terminal checks wait_for_status instead of reading immediately. Deleted two dead pub(crate) engine query methods (blocked_approval_runs_for_actor / approval_run_for_actor_and_gate). ironclaw_turns, ironclaw_runner (--features filesystem-goal-store, 3x), composition (default + inmemory-turn-state), the ratchet, and the reborn_group_approvals / reborn_minimal_dispatch_parity integration targets are green; clippy -D warnings, fmt, and pre-commit-safety clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…privately in the row store (#6263 Step 5c) (#6340) * refactor(turns): eliminate InMemoryTurnStateStore — embed the engine privately in the row store; fix the divergences it surfaced (#6263 Step 5c) The former public InMemoryTurnStateStore was the row store's execution engine dressed as a second store. Move it into filesystem_store as a private TurnStateEngine (crate-visible only), rename InMemoryTurnStateStoreLimits -> TurnStateStoreLimits, and migrate every consumer (42 files) to the one public turn-state store, FilesystemTurnStateRowStore<InMemoryBackend>, via a single shared `in_memory_turn_state_store()` test-double helper. Delete the public type. FROZEN_DEBT_INMEMORY_STORES is now EMPTY — the §4.3 store-consolidation axis reaches zero. Running the engine's contract tests against the row store for the first time surfaced that the row store was not a faithful wrapper of its own engine. Fixed the genuine divergences (each with regression coverage): - put_loop_checkpoint only patched the cached snapshot, not the cached engine, so a later fail_run found no checkpoint and made failed / lease-expired runs non-retryable (caught by the 8 retry-contract tests a naive dedup would have deleted); - retry_turn clobbered the linked checkpoint and dropped the persisted idempotency record on ThreadBusy; - submit/cancel admission-rejection idempotency records were discarded on Err instead of committed, so a duplicate re-evaluated instead of replaying; - terminal-outcome lease prep synthesized InvalidTransition before the engine could surface LeaseMismatch; - a reentrant observability read deadlocked against an in-flight submit. Coordinator/runner contract tests that encoded the instant in-memory engine's behavior were corrected to the row store's (more durable / realistic) contract, not weakened: heartbeat does not advance the cursor (#5452); durably-retained lifecycle events are served, not rebased; lease-recovery clocks use max() not min(); resume asserts the Resumed event by presence, not last position; manual-drive tests stop the auto-started scheduler so it cannot race their claims; terminal checks wait_for_status instead of reading immediately. Deleted two dead pub(crate) engine query methods (blocked_approval_runs_for_actor / approval_run_for_actor_and_gate). ironclaw_turns, ironclaw_runner (--features filesystem-goal-store, 3x), composition (default + inmemory-turn-state), the ratchet, and the reborn_group_approvals / reborn_minimal_dispatch_parity integration targets are green; clippy -D warnings, fmt, and pre-commit-safety clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(stress): migrate turn-state harness off deleted InMemoryTurnStateStore The #6263 Step 5c collapse deletes InMemoryTurnStateStore (folded into FilesystemTurnStateRowStore<InMemoryBackend>), which left tools/ironclaw_stress unbuildable under --all-features and broke the workspace Clippy/Code-Style lanes. The two stress backends built on the deleted store — `Memory` and `MemoryPersistOnBlock` — existed specifically to benchmark the *old* InMemoryTurnStateStore authority against the *new* RowMemory row-store path. This PR completes that migration, so their measurement subject no longer exists: `Memory` is now identical to `RowMemory`, and `MemoryPersistOnBlock`'s volatile-hot-path + durable-block-sink hybrid has no equivalent in the row store's uniform delta-journal durability model. Remove both; `FilesystemRow` (durable, the shipped config) + `RowMemory` cover the post-migration space. - drop the `Memory`/`MemoryPersistOnBlock` TurnStateBackend variants, the now-unused `persists_on_block()`, the `memory_turn_store` field + its `InMemoryTurnStateStore` construction, and the block-persistence sink wiring - rename `InMemoryTurnStateStoreLimits` -> `TurnStateStoreLimits` at the two stress call sites (matches the crate-side rename) - cargo fmt --all: clears pre-existing whitespace drift across the branch that was failing the Formatting lane (fmt-only, no behavior change) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Summary
Tradeoff
Live heartbeat extensions are now process-local. That matches the current single-process runner path and removes a hot durable write source, but a future multi-process runner model will need either a shared lease owner/store or a different coordination boundary before relying on cross-process heartbeats.
Deployment note
This intentionally does not migrate old
/turns/runner-leases/*heartbeat sidecars. In-flight runs across a restart may be recovered or failed from the older/turns/state.jsonlease timestamp. That is acceptable for the current internal single-instance deployment; fresh runs after deploy use the memory-backed lease path.Tests