Skip to content

fix(runtime): land the terminal fact past a corruption latch, type the sealed-run refusal - #2511

Merged
Astro-Han merged 9 commits into
apache:mainfrom
Benjamin-eecs:fix/2313-terminal-past-latch
Aug 13, 2026
Merged

fix(runtime): land the terminal fact past a corruption latch, type the sealed-run refusal#2511
Astro-Han merged 9 commits into
apache:mainfrom
Benjamin-eecs:fix/2313-terminal-past-latch

Conversation

@Benjamin-eecs

@Benjamin-eecs Benjamin-eecs commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Both issues are one question from two directions: which append refusals deserve to read as "the store is sick". This PR implements the middle position #2313 proposes, and keeps everything else fail-closed.

  • A corruption latch no longer costs the run its terminal fact. The health scan refuses tool-bearing appends only, so the terminal event is a write the damaged ledger would have taken; before this change the latch alone kept it out and the run parked at running forever. commitTerminalRun now treats a ToolLedgerCorruptionError latch as history rather than truth: it probes the store with a read, commits when the store answers, and re-latches silently when it does not.
  • A sealed-run refusal is typed and stops reading as store failure. assertRunNotSealed now throws RunSealedError, and the append queue treats it as the expected boundary it is: no store latch, no traceWriteError on a run whose history is exactly as durable as it should be. A caller that asked for the rejection still receives it.

Fixes #2313
Fixes #2311

Review focus

The probe is deliberately narrow: only a ToolLedgerCorruptionError latch is probed. A transient write failure keeps today's behaviour, because leaving those runs non-terminal is what hands them to startup recovery, and three existing tests pin that contract. The first cut probed every latch and those three tests caught it.

The corrupt-ledger test #2240 added was written to force this decision; its assertions now state the new contract: tool facts refused, stream writes closed under the latch, and finalization still lands the terminal fact.

Verification

  • The rewritten corrupt-ledger test fails on the parent commit (verified by running the suite against the parent's agent-run.ts: 2 failures) and passes here; two new tests pin the sealed-refusal bookkeeping and the preserved silent skip for a store that cannot answer the probe.
  • packages/runtime, packages/storage, packages/core, and packages/runtime-host suites pass, including the three startup-recovery tests that pin the transient-failure contract the probe must not disturb.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@Benjamin-eecs
Benjamin-eecs marked this pull request as ready for review August 8, 2026 13:03
Copilot AI lite review requested due to automatic review settings August 8, 2026 13:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for carefully separating expected append refusals from actual store failure. The overall direction is sound: RunSealedError belongs at the store boundary, and a corrupt tool ledger should not prevent a run from recording that it ended. I am approving this PR.

Two non-blocking P2 edge cases are worth tightening:

  1. The SQLite path currently validates tool-ledger semantics before checking whether the run is already sealed. A late tool-bearing event can therefore still surface as ToolLedgerRejectionError or ToolLedgerCorruptionError, leaving traceWriteError or relatching the store instead of taking the new RunSealedError path. Checking the seal first for a new event, while preserving exact-id retry semantics, would make the refusal taxonomy consistent for every post-terminal append.

  2. On the corruption-recovery path, terminalClaim.write may still be absent, but after_terminal_event_committed is invoked before the fresh terminal durability barrier. A crash at that named boundary can therefore leave a durable continuation start without the terminal fact, contrary to the existing crash-recovery contract. The smaller approach would be to keep the global latch closed, use ensureTerminalRuntimeEventDurable itself as the scoped write probe, and fire the continuation boundary only after that barrier succeeds.

Related P3 test cleanup would make these contracts easier to trust: the unreadable-probe test currently creates a generic-error latch and never enters the corruption-only probe, while the sealed fake rejects only once and then permits an append that real SQLite would permanently refuse. A persistent sealed fake plus a real SQLite instanceof RunSealedError assertion would model production more directly.

None of these suggestions blocks this approval. The error-type split and terminal-fact recovery are valuable improvements, and the remaining cases can be tightened as follow-up work.

@Benjamin-eecs

Copy link
Copy Markdown
Contributor Author

Both P2s and the P3 cleanup are in (ed388e3 and the commit before it):

  • P2-1: importRuntimeEventSync now checks the seal before tool-ledger semantics for new events, so a late tool-bearing straggler refuses as RunSealedError like every other post-terminal append. Continuation-authority refusals keep their precedence over the seal, and exact-id retries keep their dedup answer. A storage test pins all three orderings against real SQLite, with instanceof assertions.
  • P2-2: reworked to the smaller approach you described. The latch never lifts; ensureTerminalRuntimeEventDurable inside commitOrCreateTerminalRunFact is the scoped probe, and on the recovery path after_terminal_event_committed is deferred into a new afterTerminalDurable hook that fires between the durability barrier and the header commit. A unit test pins the barrier, boundary, header order. Paths where the claimed event's write actually committed keep the boundary exactly where it was.
  • P3: the sealed fake now refuses per run and permanently (a second run on the same store proves the store stays healthy), and the silent-skip test drives the corruption latch with a refused terminal barrier instead of a generic-error latch it could never enter.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the follow-ups — the sealed-run ordering and scoped terminal durability probe now look correct, and the updated tests cover the missing cases. Looks good to me!

# Conflicts:
#	packages/runtime/src/agent-run.ts
#	packages/storage/src/__tests__/sqlite-runtime-store.test.ts
# Conflicts:
#	packages/core/src/index.ts
#	packages/runtime/src/__tests__/session-manager-terminal-ledger.test.ts
#	packages/runtime/src/agent-run.ts
#	packages/storage/src/__tests__/sqlite-runtime-store.test.ts
#	packages/storage/src/sqlite-runtime-store.ts

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex automated review

I reviewed exact head e15b992e0848d753243237545d9cae3660fa42ca and rechecked the earlier review ledger against the current code. I found no new specific, reproducible P0–P3 production issue.

The current head correctly preserves seal-before-tool ordering, exact-ID retry semantics, the scoped terminal durability barrier, continuation precedence, corruption recovery, and the real SQLite error taxonomy. The six changed files form one cohesive terminal-ledger intent; I do not recommend splitting it or deleting its focused tests.

The required typecheck job is still red because its format:check step reports extra blank lines in two unrelated existing tests (mid-turn-capacity-backend.test.ts and model-factory-thinking.test.ts). The files changed by this PR format cleanly, but the required check must be fixed or rerun green before merge. Other executed tests are green; e2e/storybook are skipped.

Disclosure: This is an automated review performed by Codex using delegated adversarial review passes and a final evidence check. It has not been independently verified by Astro-Han or another human reviewer, does not constitute human approval, and does not represent the final judgment of a human reviewer.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex automated review

I re-reviewed exact head bf9887597149e3c3ee470c276e19030fcd68f0c4 after the upstream-main merge.

Disclosure: This is an automated review performed by Codex using delegated adversarial review passes and a final evidence check. It has not been independently verified by Astro-Han or another human reviewer, does not constitute human approval, and does not represent the final judgment of a human reviewer.

The merge commit leaves all six PR-owned files byte-for-byte identical to the previously reviewed head; the net PR diff remains the same focused terminal-ledger slice. I rechecked terminal-claim serialization, seal-before-ledger ordering, exact-ID deduplication, continuation precedence, and the scoped durability barrier, and found no concrete P0–P3 production issue.

The earlier unrelated format blocker is now cleared. All executed checks on this exact head are green (typecheck, workspace/runtime-host tests, test, Windows baseline/recovery, and change detection); E2E and Storybook are skipped by the current test plan. The branch is clean and mergeable. No split is warranted for this six-file, single-intent change.

@Astro-Han

Copy link
Copy Markdown
Contributor

LGTM.

@Astro-Han
Astro-Han merged commit ab0fbe7 into apache:main Aug 13, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants