Sign up
View stargazersSign up

fix(server): delegated completions preserve finished parent runs#20025

merged2
main
GitHub fork: fix/20014-completion-keeps-run-completed
opened22d agobySScottN-PV
3 files changed
View on GitHubTwo-way sync enabledPR last imported 17d ago

A delegated child could finish while its parent was capturing a checkpoint. Finalization then wrote an older parent snapshot, reverting the completed run to waiting and clearing its checkpoint. Completion notices and later messages could remain queued.

Finalization now reloads the parent inside the event sink transaction and replaces only delegatedCompletion, preserving the current lifecycle in both the event log and projection. This covers delegated-task finalization; recovery of already corrupted runs and other cohort writers remain outside this change.

Closes #20014. Original implementation and regression by @ScottN-PV in #20025. The maintainer takeover retained their implementation and single regression after independent test and implementation passes found no smaller correct solution.

Verification at d8ce455c5f:

  • Regression module with production fix reverted: 20 passed, 1 failed (completed expected, waiting received). With the fix restored: 21 passed. The test forces checkpoint completion through the real event sink between finalization read and write, checks persisted event and projection fields, then dispatches the completion message and verifies it is not queued.
  • Delegated completion and checkpoint capture suites together: 25 tests passed across two existing files.
  • Server typecheck passed. Formatting and scoped lint passed; two existing Orchestrator warnings remain outside this diff.
  • Independent full-diff review round 1: GPT-6-Sol and Claude Opus 5.5 both found no blockers. Suggestions to broaden the repair to other existing cohort writers were kept outside this focused fix.

Headless Linux verification uses real orchestration and in-memory SQLite with deterministic timing. It injects the checkpoint completion event rather than running Git capture. No running-app or live-provider reproduction is claimed; macOS, Windows, iOS, Android, and Electron desktop were not exercised. This server-only change has no visual UI diff, so screenshots do not apply. The PR remains unmerged.

Models: GPT-6-Astra (tests), GPT-6.1-Sol (implementation), GPT-6-Sol and Claude Opus 5.5 (review). Harnesses: Codex and Claude Code through T3 Code.

Checks

21 successful · 10 skipped
All checks passed
c4dafc7
CcoderabbitaiBot22d ago
Synced from GitHub

Review in Change Stack →

Warning

Review limit reached

Only developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing.

Next included review available in 24 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 13afbeb6-cd22-48c5-8296-3de0468ca0b7
📥 Commits

Reviewing files that changed from the base of the PR and between d8ce455c5fdfcdb7b3dcb2d6a990a7e61b01a674 and c4dafc71445dc7d43e5a24795fe8971efebd8253.

📒 Files selected for processing: 3
  • apps/server/src/orchestration-v2/DelegatedCompletionDelivery.test.ts
  • apps/server/src/orchestration-v2/EventSink.ts
  • apps/server/src/orchestration-v2/Orchestrator.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 85c58f6e-9d3b-4fd1-a85b-18046787ee1f
📥 Commits

Reviewing files that changed from the base of the PR and between bbce6c85decf13b1e462f4951a2e882d9c6a0765 and d8ce455c5fdfcdb7b3dcb2d6a990a7e61b01a674.

📒 Files selected for processing: 3
  • apps/server/src/orchestration-v2/DelegatedCompletionDelivery.test.ts
  • apps/server/src/orchestration-v2/EventSink.ts
  • apps/server/src/orchestration-v2/Orchestrator.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

📝 Walkthrough

Delegated-task finalization now writes completion events with an option to preserve current stored run fields while updating delegated completion data. A regression test checks that checkpoint capture and delegated delivery retain the parent run’s completed state.

Changes: Delegated Completion

Layer / File(s)Summary
Rebuild updated run events
apps/server/src/orchestration-v2/EventSink.ts		The optional updateDelegatedCompletionOnly flag rebuilds run.updated events from the stored run row and replaces its delegatedCompletion. The selected events then pass through the existing cancellation guard and normalization.
Apply the write mode to finalization
apps/server/src/orchestration-v2/Orchestrator.ts, apps/server/src/orchestration-v2/DelegatedCompletionDelivery.test.ts		Delegated-task finalization enables the flag when writing completion events. The regression test checks that recovery retains the parent run’s completed status, completion time, and checkpoint while recording child delivery.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to d8ce4

The delegated-completion write preserves a checkpoint committed before finalization. No outstanding issue identified here prevents merging after normal checks.

Security architecture risk: 🔵 Low · up to d8ce4

The change narrowly protects finished runs from being reverted by delayed completion updates. No new access path or privilege expansion was identified. The protection applies to the targeted finalization path, not every completion writer or previously affected run.

Retained concerns No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated behavioral change is scoped to the parent-run update in an app-owned delegated-task completion batch. It does not establish new attacker-selected thread or run authority: the caller retains its lineage and ownership checks, and the new option is passed as a server-side constant.

Trust Boundaries and Controls

  • observed — Completion offering reloads the parent and current cohort after the write. It refuses missing parents, non-open cohorts, empty deliveries, archived or deleted threads, and already materialized deliveries except eligible undelivered mailbox steering. It resolves the destination provider thread from that refreshed state.

Resilience and Maintainability Implications

  • observed — Existing recovery and terminal-update paths take the parent-thread dispatch lock for finalization. Duplicate result transfers stop repeated finalization. Delivery settlement checks message and generation ownership, returns cancelled delivery tasks to pending, and recovery can re-offer persisted deliveries after interruption between commit and offering.
  • observed — The protection deliberately excludes delegatedCompletion itself: that value still comes from the pre-write plan. A missing current run passes the original event through. These are limitations of the targeted repair, not established new vulnerabilities; the change also leaves other cohort writers unmodified.
Pre-merge checks: Passed 4
Check nameStatusExplanation
Title checkPassedThe title clearly and concisely describes the primary fix: preserving finished parent runs during delegated completion finalization.
Linked Issues checkPassedFor #20014, delegated-task finalization enables updateDelegatedCompletionOnly. EventSink.writeWithEffects reloads the current parent run for run.updated and replaces only delegatedCompletion. …
Out of Scope Changes checkPassedThe changes stay within #20014. EventSink.ts adds the scoped write option, Orchestrator.ts enables it for delegated completion finalization, and the test covers the reported race. No unrelated eve…
Description checkPassedPASS: The description clearly explains the race, the fix, scope limitations, linked issue, verification steps, observed results, environment limitations, and agent details. It does not use the templat…
Full details: Linked Issues check

Explanation

For #20014, delegated-task finalization enables updateDelegatedCompletionOnly. EventSink.writeWithEffects reloads the current parent run for run.updated and replaces only delegatedCompletion. This preserves concurrent status, completedAt, and checkpointId in the event and projection. The regression test commits capture between finalizer read and write and verifies the completion delivery is not queued behind the parent.

Full details: Out of Scope Changes check

Explanation

The changes stay within #20014. EventSink.ts adds the scoped write option, Orchestrator.ts enables it for delegated completion finalization, and the test covers the reported race. No unrelated event type, schema, projection guard, cohort writer, or recovery behavior is changed.

Full details: Description check

Explanation

PASS: The description clearly explains the race, the fix, scope limitations, linked issue, verification steps, observed results, environment limitations, and agent details. It does not use the template headings exactly, and the maintainer approval comment is not linked directly in the description, but the required information is otherwise substantially present.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Added 1 commit

  • fix(server): delegated completions preserve finished parent runsd8ce455
JJulius Marminge18d ago

Taking over this one as maintainer for batch scheduler-tasks-03. I will retain the contributor commits, verify the checkpoint race with focused tests, and push any needed fixes here. This batch will not merge the PR.

Added 1 commit

  • Merge remote-tracking branch 'origin/main' into pr-20025c4dafc7