Fix checkpointless pre-model recovery - #6841
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
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 (7)
💤 Files with no reviewable changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds structured capability observations to recovery callbacks, preserves typed and sanitized prompt-stage diagnostics, and introduces bounded checkpointless runner-failure redrive with durable identity, lease, cancellation, checkpoint, and backend coverage. ChangesRecovery flow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ 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 |
🔎 Review · PR #6841
Execution result is invalid The structured result could not be verified. Automatic · PR opened · attempt 1 of 3 · failed after 3m 33s Failure details
|
|
🚅 Deployed to the ironclaw-pr-6841 environment in ironclaw-ci-preview
|
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
🔎 Review · PR #6841
Submitted review →Reviewed the complete trusted base-to-head comparison. No concrete correctness, security, concurrency, persistence, architecture, or test-coverage defects were found. The checkpointless re-drive is limited to explicit pre-model failure categories and remains lease-validated, cancellation-first, checkpoint-gated, same-run, and durably bounded by claim count. Automatic · PR opened · attempt 1 of 3 · completed in 1m 29s Run details
|
There was a problem hiding this comment.
🔍 Review complete · PR #6841
✅ No actionable findings
Reviewed the complete trusted base-to-head comparison. No concrete correctness, security, concurrency, persistence, architecture, or test-coverage defects were found. The checkpointless re-drive is limited to explicit pre-model failure categories and remains lease-validated, cancellation-first, checkpoint-gated, same-run, and durably bounded by claim count.
Validation and technical details
- Verified trusted comparison refs: de34247..9bdb802.
- Inspected all 15 changed files and surrounding scheduler, executor, lease-retirement, row-store commit, checkpoint, queue, lifecycle-event, snapshot-reopen, and recovery-limit code.
- Enumerated all RecordRunnerFailureRequest constructors and verified explicit terminal/re-drive disposition at production and test call sites.
- Verified added tests cover structured model observation, category allowlisting, same-run identity, bounded exhaustion with preserved failure cause, cancellation precedence, checkpoint prevention, and durable reopen.
git diff --check refs/ironloop/base..refs/ironloop/headpassed.- Scoped cargo tests could not be executed in this review environment because
cargois unavailable (/bin/bash: cargo: command not found). - Base:
main - Head:
codex/ws6-pre-model-recoveryat9bdb802 - Run:
a4e867f5-afd1-4234-adec-a264828f59ec
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_turns/src/turn_state_row_store/row_store/traits.rs`:
- Around line 755-764: The runner-leaving bookkeeping in record_runner_failure
currently derives retired_status from request.recovery, which incorrectly forces
RedriveIfCheckpointless to Queued. Update the record_runner_failure transition
flow to derive the status passed to apply_run_state_transition from the
TurnRunState returned by runner_failure_transition, preserving resolved Failed
or Cancelled outcomes. Cover checkpointed, exhausted, and cancel-requested
RedriveIfCheckpointless cases across row-store and partition paths.
🪄 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: 230fa7aa-95ab-469c-a506-b6f4050cb46f
📒 Files selected for processing (15)
crates/ironclaw_agent_loop/src/executor/capabilities.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/src/executor/tests/support.rscrates/ironclaw_agent_loop/src/strategies/recovery.rscrates/ironclaw_runner/src/loop_exit_applier/tests/support.rscrates/ironclaw_runner/src/turn_run_executor.rscrates/ironclaw_runner/src/turn_scheduler.rscrates/ironclaw_runner/src/turn_scheduler/tests.rscrates/ironclaw_runner/tests/turn_scheduler_contract.rscrates/ironclaw_turns/src/runner.rscrates/ironclaw_turns/src/turn_state_row_store/row_store/traits.rscrates/ironclaw_turns/src/turn_state_row_store/turn_state_engine/mod.rscrates/ironclaw_turns/src/turn_state_row_store/turn_state_engine/transitions.rscrates/ironclaw_turns/tests/row_store_crash_consistency.rscrates/ironclaw_turns/tests/turn_coordinator_contract.rs
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/turn_state_row_store/turn_state_engine/transitions.rs`:
- Around line 965-979: Update the TurnStatus::CancelRequested transition to call
the existing release_terminal_lease helper instead of manually performing
clear_runner_lease, cursor advancement, active-lock release, and queued-run
removal. Preserve the existing cancellation status, failure reset, event, and
terminal-marking behavior while reusing the helper’s shared terminal-transition
tail.
In `@crates/ironclaw_turns/tests/runner_failure_backend_parity.rs`:
- Around line 48-71: Update build_postgres_scoped so only the initial
environment-variable lookup can return None when PostgreSQL is not configured.
After a URL is present, replace the .ok()? error suppression for URL parsing,
pool construction, and root.run_migrations() with explicit fail-loud handling
such as expect messages, while preserving the existing optional skip behavior
for missing configuration.
🪄 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: e3950054-0f11-4f93-bcc8-5f0ceac789ab
📒 Files selected for processing (7)
crates/ironclaw_runner/src/turn_scheduler.rscrates/ironclaw_runner/tests/turn_scheduler_contract.rscrates/ironclaw_turns/src/turn_state_row_store/turn_state_engine/transitions.rscrates/ironclaw_turns/tests/row_store_crash_consistency.rscrates/ironclaw_turns/tests/runner_failure_backend_parity.rscrates/ironclaw_turns/tests/turn_coordinator_contract.rsdocs/reborn/contracts/turn-runner.md
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_agent_loop/src/executor/tests.rs`:
- Around line 2129-2162: Extend the PromptStage tests around
prompt_stage_preserves_policy_denied_kind_from_prompt_bundle to cover
prompt_host_error’s remaining branches: verify a prompt-bundle
AgentLoopHostErrorKind::Cancelled becomes AgentLoopExecutorError::Cancelled, and
inject a full host error whose safe_summary fallback is rejected to verify the
sanitized error behavior. Update MockHost only as needed to supply that full
AgentLoopHostError, while preserving the existing policy-denied case.
In `@crates/ironclaw_runner/src/planned_driver.rs`:
- Around line 348-356: The permanent failure mapping around
permanent_host_stage_failure_category must be scoped to the intended
prompt-stage cases instead of treating every non-Model stage as terminal. Update
the stage/kind handling in the surrounding error path to preserve capability,
checkpoint, transcript, and input failure kinds and durable state, or define and
test an explicit stage/kind matrix covering them.
🪄 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: 79ec6ce8-2f3f-4e08-8512-853671405fd7
📒 Files selected for processing (5)
crates/ironclaw_agent_loop/src/executor/prompt.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/src/executor/tests/support.rscrates/ironclaw_reborn_composition/src/runtime/tests/core.rscrates/ironclaw_runner/src/planned_driver.rs
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.82% — 316906 / 369259 lines Per-crate breakdown (61 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)
|
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/turn_state_row_store/turn_state_engine/transitions.rs (1)
891-947: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCheckpointless redrive discards the triggering failure — no audit trail until retries exhaust.
runner_failure_transition'sRedriveIfCheckpointlessbranch (937-943) callsrequeue_checkpointless_runner_failure(record), but that function (905-916) never takes thefailure: SanitizedFailurethe caller received — it's simply dropped.requeue_claimed_record(891-903) always pushesTurnEventKind::RunnerHeartbeatwithNone, Nonefor category/detail (line 902). So a runner-reported transient crash that triggers a redrive is durably indistinguishable from a normal heartbeat — operators get zero signal about why a run is being retried, until (if ever) retries exhaust intoFailed. This directly undercuts "preserve observable failure kinds ... audit error_kind" — the failure detail exists (SanitizedFailureis right there in the caller) and is being thrown away rather than recorded.
fail_claimed_record/terminal_transitionalready threadfailure.into_category()/failure.detail()intopush_event's 3rd/4th params — reuse that pattern here instead of inventing a new event kind.🐛 Proposed fix — carry failure category/detail through the requeue event
- fn requeue_claimed_record(&mut self, record: &mut RunRecord, now: DateTime<Utc>) { + fn requeue_claimed_record( + &mut self, + record: &mut RunRecord, + now: DateTime<Utc>, + redrive_cause: Option<&SanitizedFailure>, + ) { let transition = record.status.set(TurnStatus::Queued); self.apply_status_transition(transition, record); record.failure = None; clear_runner_lease(record); record.event_cursor = self.next_cursor(); self.update_active_lock(record, now); self.queued_runs.push_back(record.run_id); - // Running → Queued uses the same lifecycle classification for graceful - // relinquish, checkpointless failure re-drive, and expired-lease - // recovery so the durable log and publishing wrapper stay aligned. - self.push_event(record, TurnEventKind::RunnerHeartbeat, None, None); + // Running → Queued keeps the same event KIND for graceful relinquish, + // checkpointless failure re-drive, and expired-lease recovery so the + // publishing wrapper stays aligned, but a failure-driven redrive still + // records its cause via category/detail. + let (category, detail) = redrive_cause + .map(|failure| (Some(failure.category().to_string()), failure.detail().map(str::to_string))) + .unwrap_or((None, None)); + self.push_event(record, TurnEventKind::RunnerHeartbeat, category, detail); } fn requeue_checkpointless_runner_failure( &mut self, mut record: RunRecord, + failure: SanitizedFailure, ) -> AppliedLoopTransition { - self.requeue_claimed_record(&mut record, Utc::now()); + self.requeue_claimed_record(&mut record, Utc::now(), Some(&failure)); ... }Other two call sites (
recover_expired_leases,relinquish_transition) passNoneand keep today's behavior unchanged.🤖 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/turn_state_row_store/turn_state_engine/transitions.rs` around lines 891 - 947, Preserve the triggering SanitizedFailure when checkpointless redrive occurs: update requeue_checkpointless_runner_failure and requeue_claimed_record to accept and forward the failure category and detail, and have the requeue event use failure.into_category() and failure.detail() instead of None values. Update only the runner_failure_transition call site for this failure-aware path; keep recover_expired_leases and relinquish_transition behavior unchanged.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_turns/src/turn_state_row_store/turn_state_engine/transitions.rs`:
- Around line 891-947: Preserve the triggering SanitizedFailure when
checkpointless redrive occurs: update requeue_checkpointless_runner_failure and
requeue_claimed_record to accept and forward the failure category and detail,
and have the requeue event use failure.into_category() and failure.detail()
instead of None values. Update only the runner_failure_transition call site for
this failure-aware path; keep recover_expired_leases and relinquish_transition
behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5a44e920-b42b-4efd-9fb8-de2a18243ada
📒 Files selected for processing (7)
crates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_agent_loop/src/executor/tests/support.rscrates/ironclaw_runner/src/planned_driver.rscrates/ironclaw_turns/src/turn_state_row_store/row_store/commit.rscrates/ironclaw_turns/src/turn_state_row_store/row_store/traits.rscrates/ironclaw_turns/src/turn_state_row_store/turn_state_engine/transitions.rscrates/ironclaw_turns/tests/runner_failure_backend_parity.rs
…ecovery # Conflicts: # crates/ironclaw_agent_loop/src/strategies/recovery.rs
…ecovery # Conflicts: # crates/ironclaw_agent_loop/src/executor/capabilities.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 (2)
crates/ironclaw_runner/src/planned_driver.rs (2)
346-354: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCentralize the diagnostic-scrubbing path.
This branch duplicates the fallback,
scrub_model_visible_detail, andFailedconstruction already used above. Extract one helper so model- and prompt-stage failures cannot diverge on redaction behavior.As per coding guidelines, extract helpers when logic is reused.
🤖 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_runner/src/planned_driver.rs` around lines 346 - 354, Extract a shared helper for constructing scrubbed AgentLoopDriverError::Failed values, including the detail fallback to safe_summary and ironclaw_loop_host::scrub_model_visible_detail. Update this prompt-stage failure branch and the existing model-failure path above to use the helper, preserving their respective category and diagnostic inputs.Source: Coding guidelines
373-379: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winComplete regression coverage for the new terminal mappings.
The added tests cover Prompt/
PolicyDeniedand non-Prompt rejection, but do not coverRecoverySequenceExhaustedor the positive Prompt mappings forInvalidInvocation,Invalid, andScopeMismatch. Add caller-level assertions for these contracts.As per coding guidelines, new production-wired behavior requires caller-level regression tests.
Also applies to: 759-815
🤖 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_runner/src/planned_driver.rs` around lines 373 - 379, Add caller-level regression tests for the terminal mappings in the planned driver flow: assert RecoverySequenceExhausted produces Failed with reason_kind "driver_bug", and assert Prompt requests map InvalidInvocation, Invalid, and ScopeMismatch to their expected positive outcomes. Extend the existing tests around the caller handling these AgentLoopExecutorError variants, preserving the current Prompt/PolicyDenied and non-Prompt rejection coverage.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_runner/src/planned_driver.rs`:
- Around line 346-354: Extract a shared helper for constructing scrubbed
AgentLoopDriverError::Failed values, including the detail fallback to
safe_summary and ironclaw_loop_host::scrub_model_visible_detail. Update this
prompt-stage failure branch and the existing model-failure path above to use the
helper, preserving their respective category and diagnostic inputs.
- Around line 373-379: Add caller-level regression tests for the terminal
mappings in the planned driver flow: assert RecoverySequenceExhausted produces
Failed with reason_kind "driver_bug", and assert Prompt requests map
InvalidInvocation, Invalid, and ScopeMismatch to their expected positive
outcomes. Extend the existing tests around the caller handling these
AgentLoopExecutorError variants, preserving the current Prompt/PolicyDenied and
non-Prompt rejection coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 65fc8d9a-3c3b-4a4e-b606-b18521f0e241
📒 Files selected for processing (4)
crates/ironclaw_agent_loop/src/executor/capabilities.rscrates/ironclaw_agent_loop/src/executor/checkpoint.rscrates/ironclaw_agent_loop/src/executor/tests.rscrates/ironclaw_runner/src/planned_driver.rs
* fix checkpointless pre-model recovery * fix(runner): preserve active redrive lease identity * fix(runner): keep permanent prompt failures terminal * test(runner): expect preserved prompt summary * fix(runner): address coderabbit recovery review (nearai#6841) * chore(ci): ratchet removed runner panic baseline
Summary
BeforeModelcheckpoint.Change Type
Linked Issue
Related #6284
Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warnings— Not run repo-wide;cargo clippy -p ironclaw_runner -p ironclaw_turns --all-targets -- -D warningspassed after the review fixes, and the original touched-crate clippy pass includedironclaw_agent_loop.cargo build— Not run; targeted crate tests and all-target clippy compiled every changed Rust path.ironclaw_turnscrate suites, including lifecycle, crash/reopen, and libSQL parity coverage.cargo test --features integrationif database-backed or integration behavior changed — Not applicable:ironclaw_turnshas nointegrationfeature and the integration harness seam did not change. Real libSQL coverage runs in the crate suite; the PostgreSQL parity test is opt-in when a test database URL is configured.review-prorpr-shepherd --fixwas run before requesting review — Eight-lenscode-review-multicompleted with full packet coverage; all six deduplicated findings were fixed.Test Strategy
User behavior: A first-iteration transient input, prompt/context, or capability-surface construction failure is retried automatically as the same run. Persistent failure stops at the bounded attempt limit with the original safe cause. Cancellation and any recorded loop checkpoint prevent scratch re-drive. Shutdown relinquishes the currently active same-run attempt rather than losing its lease identity.
Risk areas:
Tests added or updated:
CanonicalAgentLoopExecutor; pre-model category allowlist; same-run scheduler recovery; bounded exhaustion; cancellation precedence; checkpoint side-effect guard; exact lease identity during same-run redrive shutdown.What the tests prove:
accepted_message_refwithout creating a duplicate run.Commands run:
cargo test -p ironclaw_agent_loop invalid_provider_tool_failure_appends_structured_model_observationcargo test -p ironclaw_agent_loop --lib strategies::recovery::testscargo test -p ironclaw_runner --test turn_scheduler_contract— 34 passedcargo test -p ironclaw_turns— all unit, contract, crash-consistency, and libSQL parity tests passedcargo test -p ironclaw_runner --test turn_scheduler_contract shutdown_relinquishes_the_active_lease_after_same_run_redrive -- --exact— also repeated 50 times after the fixcargo clippy -p ironclaw_agent_loop -p ironclaw_turns -p ironclaw_runner --all-targets -- -D warnings— original changecargo clippy -p ironclaw_runner -p ironclaw_turns --all-targets -- -D warnings— review fixescargo fmt --all -- --checkgit diff --checkSecurity Impact
None. The change does not expand authority, permissions, network access, secret handling, or sandbox policy. Recovery receives the existing sanitized model-visible observation rather than raw host/provider detail.
Reborn Trust-Boundary Checklist
RunnerFailureRecoveryis selected by the trusted scheduler; the store remains authoritative for cancellation, checkpoint, lease, and bound validation.rg -n "RecordRunnerFailureRequest|RunnerFailureRecovery|RedriveIfCheckpointless" cratesand all constructors/production forwarders were reviewed.serde(default)fields fail closed or have migration tests.RunnerFailureRecoverydefaults toTerminal; the request is not persisted as durable state.claim_count < max_crash_recovery_reclaimsbound.Transient,Permanent,Misconfigured,PolicyDeniedor equivalent). Only the three stable pre-modelhost_stage_unavailable_*categories opt in; exhaustion preserves the original sanitized category/detail.Database Impact
No migration or schema change. Durable row-store transition semantics change for eligible checkpointless runner failures; libSQL parity passed locally and matching PostgreSQL parity is included for configured CI/developer environments.
Blast Radius
Touches capability recovery strategy inputs, runner executor-failure settlement, scheduler active-lease tracking, and turn-state row-store transitions. The principal risks are accidental scratch execution after work has begun, duplicate queueing, lost identity, stale lifecycle projection, or unbounded retries; cancellation/checkpoint/identity/bound/event/reopen/backend tests cover those seams.
Rollback Plan
Revert commits
1074646aaand0f5ad2f32. Existing behavior will return: graceful pre-model executor failures terminalize immediately, while manual retry and expired-lease checkpointless recovery remain available.Review Follow-Through
Eight isolated reviewers covered both complete diff packets. The review found one high-confidence same-run lease race plus test, contract, backend-parity, and duplication gaps; all were fixed. Reviewer judgment remains useful on the deliberately narrow pre-model category allowlist and reuse of
max_crash_recovery_reclaimsas the shared automatic-redrive bound. No epic boxes were edited; the epic's literal “no retry path” premise is stale after #6295 and #6376, while this narrower graceful-failure gap remained live.Review track: C (runtime/persistence recovery behavior)