Skip to content

fix(runtime): preserve completed backends through bounded finalization - #1313

Open
aviggiano wants to merge 5 commits into
unstablefrom
codex/1300-finalization-grace
Open

aviggiano wants to merge 5 commits into
unstablefrom
codex/1300-finalization-grace

Conversation

@aviggiano

@aviggiano aviggiano commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Fixes #1300.

  • Add run.finalization_grace_ms (default 300,000 ms; 0 disables; maximum 3,600,000 ms). Historical sealed configs without the field retain the original behavior.
  • Keep the original execution deadline for unfinished/failed backends. Campaign tasks receive an outer execution-plus-grace ceiling, while the runtime admits finalization only after a checkpointed backend exits successfully before the execution deadline. Heartbeats and provider retries never renew either deadline.
  • Add a task-scoped ultrafuzz backend run <checkpoint> -- <command> primitive. It preserves bounded logs, canonical/workspace backend evidence, the campaign plan, and a validated workspace patch outside retry-cleared artifact roots. An unchanged retry restores completed work and gets a bounded finalization-only window instead of rerunning the backend. Reuse is bound to source/prompt/task identity and authenticated dependency markers.
  • Expose phase, execution/finalization deadlines, checkpoint availability, and last observed activity in ultrafuzz node and timeout diagnostics. Preserve backend exit timestamps when artifacts are finalized later; existing artifact and timeout-evidence gates remain mandatory.

This is deliberately separate from provider-backoff accounting in #1305. It does not increase the configured fuzzer duration, admit failed/nonzero backend exits as success, or turn checkpoint files into verified producer outputs.

Validation

  • Full CLI dependency-closure build; runtime and CLI test TypeScript compilation.
  • Strict changed-line ESLint, normal repository ESLint, full Prettier check, and git diff --check.
  • Config suite: 93 passing; prompt suite: 62 passing.
  • Synthetic-backend regressions: near-deadline success followed by finalization, fixed finalization timeout despite heartbeats, bounded finalization-only retry, stale-binding/failure rejection, caller cancellation, disabled-grace compatibility, workspace-mirror restoration, harness-patch restoration, and bounded raw logs.
  • Generated-workflow verifier suite, checkpoint command argv test, CLI contract tests, campaign timing/compiler tests, node human/JSON status regression, and updated-base withheld-report regressions.

Draft for independent review, particularly the phase boundary, checkpoint/retry binding, process cancellation, and interaction with existing artifact authority checks.

RetriggerView in GreptileConfidence Score: 5/5

The PR appears safe to merge; both previous findings are addressed and no new actionable issues were found.

Summary

Adds bounded finalization time and task-scoped checkpoints so completed backends can survive interrupted artifact writing.

  • Successful campaign backends get bounded time to save their results.
  • Campaign finalization time can be set with a bounded grace value.
  • Node status shows the backend phase, deadlines, and checkpoint state.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Backend execution] --> B{Successful exit before deadline?}
  B -->|No| C[Original execution deadline]
  B -->|Yes| D[Enter fixed finalization window]
  D --> E[Drain logs for at most 250 ms]
  E --> F{Logs sealed and not canceled?}
  F -->|No| G[No reusable checkpoint]
  F -->|Yes| H[Capture and authenticate evidence]
  H --> I[Finalize and validate artifacts]
  I -->|Interrupted| J[Unchanged retry restores evidence]
  J --> K[Finalization-only window]
Loading

Reviews (2) · Last reviewed commit: "fix(runtime): reject unrenderable backen..." · Reviewed by Greptile

@aviggiano aviggiano added bug Something isn't working P0 Breaks the default path (Linux VPS, default config) and the failure does not point to the fix labels Oct 15, 2026

@aviggiano aviggiano left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Verdict: REQUEST_CHANGES

Independent review of f2b6dbc7241af18f690761667166f0abcad4914f against current origin/unstable (81aec3bdff8fa943118d9979a2a68eaa4c678466), using a fresh detached worktree and synthetic fixtures. The happy-path tests pass, but the following independently reproduced cases need changes before this is ready.

  1. [P1] Completion admission trusts a task-writable receipt. refreshCompletion, lines 210–225 grants finalization from completed.json using the token and binding disclosed in active.json; the entire directory is also supplied to the agent through addDir. A synthetic generation read that policy, wrote a schema-valid completion with evidenceReady: false, and returned successfully after 350 ms despite a 200 ms execution limit, without launching any backend. The outer ceiling remains bounded, but successful backend exit is no longer a required admission condition. Keep admission authority in the runtime/supervisor and authenticate the runner's completion independently of documents the task can create or rewrite.

  2. [P1] Reuse does not authenticate the evidence it restores. prepareFinalization, lines 118–137 accepts only a matching binding and evidenceReady; the receipt contains no manifest/digests for the canonical/workspace evidence or raw logs. After a real successful launch, I modified the snapshotted backends/result.json; the next generation silently restored the modified bytes and was instructed not to rerun. Deleting that file instead also yielded checkpoint_available: true, missing evidence, and a refused backend rerun (backend already completed). Bind the complete evidence inventory and bytes to an authenticated completion record, and validate that record before selecting finalization-only recovery. The existing patch-body hash check is useful, but does not cover these evidence trees or establish the completion receipt's authority.

  3. [P1] Checkpoint copies follow destination links outside the owned root. snapshotBackendEvidence, lines 498–504 uses existsSync before copyFileSync. A dangling destination symlink makes existsSync false, so copying follows it and creates the external target. An existing hardlink passes isFile() and its external target is overwritten. Both reproduced by having the synthetic backend pre-create <checkpoint>/<token>/canonical/backends/result.json as the respective link before exiting successfully. These are runtime-owned writes, independent of the documented unrestricted-agent threat model. Use anchored, no-follow/single-link-safe reads and publication operations, including identity checks across the copy, rather than check-then-follow path operations.

  4. [P2] A hard-killed wrapper leaves its backend outside deadline ownership. runCheckpointedBackend, lines 345–367 starts a detached process group while keeping its only deadline timer and signal forwarding in the wrapper. Killing the wrapper with SIGKILL left the synthetic backend alive after the execution deadline and after the finalization monitor timed out; no durable process identity is available to recovery for cleanup. SIGTERM forwarding passed a separate probe, so this is specifically the crash/forced-kill path. Retain backend ownership through a surviving supervisor or an equivalent parent-death/recovery cleanup mechanism, and cover this alongside ordinary cancellation.

Validation: CLI dependency-closure build and runtime/CLI test compilation passed; strict changed-line ESLint against the base above, normal ESLint, changed implementation formatting checks, and git diff --check passed. Backend/generated-workflow-verifier/backend-CLI tests: 136 passing. Config: 93 passing. Prompts: 62 passing. Focused campaign timing, four withheld-report fanout cases, and human/JSON node status passed. Ten independent adversarial/guard probes exercised the reproductions above, stale bindings, disabled grace, patch digest rejection, raw retention, and signal handling. These probes assert the observed problematic behavior where identified; passing them is reproduction evidence, not approval of that behavior. No claim here is that raw checkpoints bypass the separate canonical artifact publication gates.

The separation from #1305 is appropriate: provider activity alone must neither admit finalization nor renew these deadlines; this review does not request provider-backoff accounting in this PR. At review time CI run 38732780343 has passing build/static-analysis, package/policy, and kill/resume E2E lanes, with runtime, CLI, and typecheck lanes still running and no failures reported. The PR remains a draft.

@aviggiano

Copy link
Copy Markdown
Collaborator Author

Addressed the four blockers from the independent review in be82e4b. Keeping this PR draft and requesting independent re-review.

  • The CLI is now a launch-only IPC client. The surviving runtime owns the child process group, observes its actual exit, and alone admits finalization. Task-authored or rewritten policy/completion documents cannot grant grace or extend the runtime's deadline.
  • Completion receipts use a process-private HMAC key and bind the complete SHA-256 evidence inventory, including raw logs and workspace patches. Retry validates the exact file set and bytes before restoring anything; altered, absent, extra, linked, or unauthenticated evidence fails closed and updates diagnostics to checkpoint_available: false.
  • Reads and writes use held directory descriptors, no-follow opens, single-link/identity checks, and atomic staged publication. Destination symlinks/hardlinks cannot redirect writes into their external targets; adversarial ancestor swaps and source replacement are covered too.
  • Wrapper disconnect, SIGKILL, SIGTERM, execution timeout, and operator cancellation cancel/reap the runtime-owned backend group, including a backend ignoring SIGTERM. Failed launches do not leak a successful receipt.

Validation passed: full CLI dependency-closure build; runtime/CLI test compilation; 162 focused backend/generated-verifier/withheld-state/CLI tests (21 new adversarial regressions); five timing/compiler tests; human/JSON node status; the startRun/provenance/submission integration test; 93 config and 62 prompt tests; strict changed-line ESLint, normal ESLint, full formatting, and diff checks. The old CI shard-4 failure exposed two stale generated-template assertions; those now assert the conditional wrapper shape and pass locally.

Explicit scope for re-review: descriptor-anchored checkpoint execution requires Linux /proc/self/fd (grace zero retains ordinary execution elsewhere). Authenticated automatic recovery covers task retries within the same runtime process. A full runtime-process restart deliberately cannot authenticate the old receipt key and fails closed while retaining raw evidence; this is not cross-process recovery authority. Provider backoff (#1305), artifact publication gates, and ready/merge state remain unchanged.

@aviggiano aviggiano left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Verdict: REQUEST_CHANGES

Re-reviewed be82e4bffb39702df92876722c77dc94bec836d3 from a fresh detached worktree against origin/unstable at 81aec3bd. The four substantive findings from my previous review are resolved within the documented scope. My original attack shapes now fail safely: forged completion and fake launch responses cannot admit grace; modified/missing/extra evidence, changed logs, receipt edits, and changed workspace patches fail before restoration; dangling symlink/hardlink destinations preserve their external targets; wrapper SIGKILL/SIGTERM and operator cancellation reap a backend that ignores SIGTERM.

One remaining merge blocker: [P2] Remove the unused exported EvidenceEntry interface at backend-checkpoint-files.ts:227. The required Build gates job fails at Find dead code and dependencies with Unused exported types (1): EvidenceEntry. pnpm knip reproduces the same failure locally. Keep the interface internal (or remove the unused exported surface) and rerun the gate. There are no remaining code-review objections to the four fixes beyond this gate failure.

Validation at this head: full CLI dependency-closure build and runtime/CLI test compilation passed; 157 backend/adversarial/generated-verifier/backend-CLI tests, 16 independent adversarial/guard probes, the campaign timing test, the startRun/provenance/submission integration test, and human/JSON node status all passed. Strict changed-line lint against the base above, normal lint, changed-file formatting, and diff checks passed.

The scope limits are real and documented: checkpoint execution requires Linux /proc/self/fd, and other hosts must set grace to zero to retain ordinary execution. Automatic authenticated reuse covers retries in the same runtime process; an independent-process probe confirmed that a full restart rejects the old receipt before restoring evidence. This is not automatic recovery across controller restarts. Canonical artifact verification and secret publication gates remain separate from raw checkpoint retention, and this change still does not implement provider-backoff accounting from #1305.

Eligibility: not ready/merge eligible at this head because Build gates is red, and the remaining CI lanes are still pending. The PR remains draft; I did not change its ready state.

@aviggiano

Copy link
Copy Markdown
Collaborator Author

Removed the unnecessary EvidenceEntry export in c6eb828 (one-line source change). Knip, full CLI dependency-closure build, runtime/CLI test compilation, 162 focused supporting tests, strict changed-line ESLint, formatting, and diff checks pass. Keeping the PR draft for final independent re-review.

@aviggiano aviggiano left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Verdict: APPROVE

Re-reviewed c6eb828fab139b4781be2128322de2ec2c517148 against the previously reviewed be82e4bffb39702df92876722c77dc94bec836d3. The entire delta is removal of export from the unused EvidenceEntry interface; there are no runtime behavior changes. The base remains origin/unstable at 81aec3bdff8fa943118d9979a2a68eaa4c678466.

Proportional validation at this head passed: pnpm knip, runtime TypeScript checking, strict changed-line lint against that base, changed-file formatting, and diff checks. The previous review's full dependency-closure build, 160 focused/integration/status tests, and 16 independent adversarial/guard probes remain applicable to the unchanged runtime implementation. All four substantive findings remain resolved within the documented Linux and same-runtime-process recovery scope; this does not claim authenticated automatic recovery across a full runtime restart or provider-backoff accounting from #1305.

The fresh exact-head CI run completed successfully, including Build/Knip gates, all four runtime integration shards, CLI tests, runtime/Bun contracts, controller kill/resume E2E, package/dependency/policy checks, benchmark/typecheck, and the aggregate release gate. All external checks are also green. The previous unused-export blocker is closed; I have no remaining blocking review findings at this head.

Eligibility: this PR can be marked ready from this review and current-checks standpoint. It remains draft; I did not change its ready state. This is an explicit APPROVE verdict in a COMMENT review because the same account cannot submit a formal approving review.

@aviggiano
aviggiano marked this pull request as ready for review October 15, 2026 13:44
@aviggiano
aviggiano requested a review from a team as a code owner October 15, 2026 13:44
Comment thread packages/runtime/src/backend-supervisor.ts Outdated
Comment thread packages/runtime/src/lifecycle-inspection.ts Outdated

@aviggiano aviggiano left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

CI follow-up at c6eb828fab139b4781be2128322de2ec2c517148 (runtime integration shard 4/4).

The red job 116457441280 was interrupted by a runner shutdown. At 2026-10-15T13:50:06.451Z its log says The runner has received a shutdown signal; the resulting test summary is 19 passed, 0 failed, 1 cancelled, followed by SIGTERM and The operation was canceled. The pending-promise message appears after that interruption; the job has no failed assertion. Its failure annotation also records cancellation.

Both this run and the earlier green shard 4/4 job checked out the identical merge commit 058af1a43243d870b8213db65fadaa7ac478286a (this head into 81aec3bdff8fa943118d9979a2a68eaa4c678466) and used Node 24.21.0. The earlier shard completed with 81 passed, 0 failed, 0 cancelled, 343 skipped. There is no intervening source/base change to reconcile.

Local verification on the unchanged head: runtime dependency-closure build and runtime test TypeScript compilation passed. Under Node 24.21.0, the next selected test at the interruption point, status keeps reporting runner health when run-state synchronization fails, passed 1/1 (83.8 seconds overall). I did not rerun the entire 27-minute shard locally.

This CI result establishes no actionable source defect, so it does not warrant a code patch or REQUEST_CHANGES verdict. Rerunning the interrupted lane and its aggregate release gate is the appropriate CI recovery. The current latest release gate remains red until that succeeds. This follow-up is limited to the CI interruption; it does not adjudicate the separate new review comments.

@aviggiano aviggiano left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Verdict: REQUEST_CHANGES

Follow-up at c6eb828fab139b4781be2128322de2ec2c517148. I independently reproduced both newer review observations. These actionable defects supersede my earlier technical approval at this head; they are separate from the runner interruption described in my preceding CI comment.

  1. [P1] Record backend completion on process exit, before waiting for inherited log streams. In backend-supervisor.ts:189–197, the supervisor waits for close, then records completedAt, and only then cleans descendants. A synthetic backend spawned a same-process-group worker inheriting stdout/stderr, recorded its exit marker, and exited 0 immediately. With a 1,000 ms execution limit and a 4,000 ms worker, the leader's marker preceded the deadline by 967 ms, but the task still returned PROCESS_TIMEOUT in execution at about 1,021 ms, with no completion receipt or checkpoint. With a 1,200 ms worker and sufficient budget, the backend succeeds but its recorded completion timestamp is 1,220 ms later than the leader marker. The corresponding control using ignored worker stdio succeeds and checkpoints with only a 7 ms timestamp difference. Capture code/time on exit, clean owned descendants, and bound stream draining. Ensure an on-time successful exit can admit grace even near the original deadline; preserve cancellation and log-error handling.

  2. [P2] Keep node inspection available when optional checkpoint status is invalid. getWorkflowNode, lines 613–619 puts the optional status read in the authoritative node-envelope failure path; watchWorkflowNode, lines 654–658 similarly aborts its stream. Using the existing synthetic CLI fixture, both node inspection with --tools --json and --watch initially returned valid attempt/tool data. Writing either malformed JSON ({) or a schema-invalid object ({}) to only the node's checkpoint status.json made inspection exit 1 with WORKFLOW_NODE_INVALID and watch exit 1 with WORKFLOW_NODE_WATCH_FAILED; both discarded all node data. Removing only that file restored byte-equivalent node data. Read optional diagnostics separately, return an unavailable-status warning, and preserve valid node details/watch snapshots while keeping malformed authoritative node envelopes rejected.

Validation: fresh runtime dependency-closure build and runtime test TypeScript compilation passed. Four standalone probes ran under Node 24.21.0 and reproduced the three backend cases and both malformed-status variants (112 seconds overall). Those passing probe assertions confirm the described defects. No tracked files or author-branch commits were changed.

@aviggiano aviggiano left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Verdict: REQUEST_CHANGES

Fresh independent review of b6759db9fa3fc21f462f1054bc40a141e94198d4 against 81aec3bdff8fa943118d9979a2a68eaa4c678466. The inherited-pipe fix now records leader exit promptly, admits bounded grace before draining, and withholds reusable checkpoints on cancellation, expired grace, or log-write/close failures. The original malformed-JSON/schema-invalid optional-status cases now preserve node evidence with warnings.

One remaining [P2] Validate status timestamps before rendering them as dates. The status timestamp schema accepts nonnegative safe integers outside JavaScript's supported Date range. I wrote an otherwise valid synthetic status.json with execution_deadline_ms: 9000000000000000. ultrafuzz node <run-id> <node-id> --tools then returned exit 1, empty stdout, and Invalid time value on stderr, despite valid underlying node/attempt/tool evidence. The exception comes from the new toISOString() rendering. The ordinary JSON command also eagerly constructs this human text through commandFromRuntime, and human watch uses the same renderer; the optional-status catch currently cannot contain a rendering failure.

Bound the optional status timestamps to the representable Date range, or otherwise make their rendering nonthrowing. Invalid optional timestamps should produce the bounded BACKEND_FINALIZATION_STATUS_UNAVAILABLE warning while preserving the authoritative node data. Cover execution deadline, finalization deadline, and last activity in human, JSON, and watch output; keep malformed authoritative node envelopes fail-closed.

Validation at this head: fresh CLI dependency-closure build and runtime/CLI test compilation passed; 162 backend/adversarial/generated-verifier/backend-CLI tests, 16 earlier independent adversarial probes, 7 runtime node-inspection tests, 2 compiler/integration checks, 93 config tests, and 62 prompt tests passed. Seven additional independent guards passed, including wrapper SIGKILL during post-exit draining, an expired short grace window, a log-close failure, and linked/nonregular optional status. The timestamp reproduction above is separate from those passing guards. Strict changed-line lint, changed-file formatting, documentation checks, and git diff --check passed. No author-branch files or commits were changed.

Current exact-head CI run has passing Build gates and external static analysis; release-validation lanes are still pending. The PR remains draft. This is a technical REQUEST_CHANGES verdict submitted as a COMMENT review.

@aviggiano aviggiano left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Verdict: APPROVE (technical review)

Re-reviewed exact head 2d8585c1ba57fdde0f7ba8766eb3b5c27faf5bed in a fresh detached checkout against base 81aec3bdff8fa943118d9979a2a68eaa4c678466. The complete delta from b6759db correctly bounds all four optional status timestamps to JavaScript's inclusive Date range while keeping grace as a duration. This resolves the P2 blocker in my preceding review.

I independently replayed 9000000000000000 in each rendered date field: execution deadline, finalization deadline, and last activity. All 12 field/mode combinations—human, JSON, human watch, and JSON watch—now retain the valid node/attempt/tool data and report BACKEND_FINALIZATION_STATUS_UNAVAILABLE. The checked-in boundary regression also accepts zero and the exact Date maximum, rejects values above it for all four timestamp fields, and preserves nullable finalization deadlines. Authoritative node-envelope validation remains unchanged.

Fresh validation at this head passed: CLI dependency-closure build, runtime/CLI test compilation, all 36 backend/finalization/adversarial/pipe tests, the expanded CLI status regression, the independent 12-case CLI probe, strict changed-line lint across the full PR, changed-file formatting, and diff checks. The broader full-PR review and passing probes recorded in the preceding review remain applicable to the unchanged supervisor, authenticated receipt/manifest, descriptor-anchored publication, cancellation, and artifact-authority code. The documented Linux checkpoint-execution and same-runtime-process authenticated-reuse limits remain unchanged. I have no remaining blocking code-review findings at this head.

The fresh exact-head CI run is still running. External static analysis and Socket checks are green; Build gates and release-validation lanes have not all completed. This technical approval does not establish ready/merge eligibility until the required checks pass. The PR remains draft; I did not change its ready state. This is an explicit APPROVE verdict in a COMMENT review because the author account cannot submit a formal approving review.

@aviggiano
aviggiano marked this pull request as ready for review October 15, 2026 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working P0 Breaks the default path (Linux VPS, default config) and the failure does not point to the fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Preserve completed backend work with a configurable finalization grace window

1 participant