Skip to content

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

Merged
juliusmarminge merged 2 commits into
pingdotgg:mainfrom
ScottN-PV:fix/20014-completion-keeps-run-completed
Oct 20, 2026
Merged

juliusmarminge merged 2 commits into
pingdotgg:mainfrom
ScottN-PV:fix/20014-completion-keeps-run-completed

Conversation

@ScottN-PV

@ScottN-PV ScottN-PV commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

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.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Oct 15, 2026
@coderabbitai

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

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 d8ce455 and c4dafc7.

📒 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 bbce6c8 and d8ce455.

📒 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 name Status Explanation
Title check Passed The title clearly and concisely describes the primary fix: preserving finished parent runs during delegated completion finalization.
Linked Issues check Passed For #20014, delegated-task finalization enables updateDelegatedCompletionOnly. EventSink.writeWithEffects reloads the current parent run for run.updated and replaces only delegatedCompletion. …
Out of Scope Changes check Passed 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 eve…
Description check Passed 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 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.

@ScottN-PV
ScottN-PV force-pushed the fix/20014-completion-keeps-run-completed branch from bbce6c8 to d8ce455 Compare October 19, 2026 14:16
@juliusmarminge juliusmarminge self-assigned this Oct 19, 2026
@juliusmarminge

Copy link
Copy Markdown
Member

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.

# Conflicts:
#	apps/server/src/orchestration-v2/EventSink.ts
#	apps/server/src/orchestration-v2/Orchestrator.ts
@juliusmarminge
juliusmarminge merged commit e27de4e into pingdotgg:main Oct 20, 2026
27 checks passed
Pipyakas pushed a commit to Pipyakas/t3code that referenced this pull request Oct 21, 2026
…gdotgg#20025)

Co-authored-by: Julius Marminge <julius0216@outlook.com>
(cherry picked from commit e27de4e)
aorwall added a commit to aorwall/t3code that referenced this pull request Oct 21, 2026
Merges `pingdotgg/t3code` `main` at `e64766c56a` (367 commits from
`143ab1606d`) into the fork. It branches from `main`; no other merge PR
is open.

## Numbers

- Landed: 2630 files from `git diff --stat HEAD^1 HEAD`, against 2629 in
the upstream range. The gap:
- Kept deleted: `pair.ts`, `pair.test.ts` and
`useDiscoveredLocalServers.test.ts`.
- Kept as fork files: `branding.test.ts` and
`addBrowserSurface.test.ts`.
- Extra on the merge side: four ported fork browser tests,
`fork/ThreadParentLine.tsx` and `inventory.json`.
- Fork delta: 812 files, down from 817.

## Conflicts (40, each by its preflight verdict)

- **Upstream's test prune (pingdotgg#22557):** upstream's deletions win,
fork-only tests stay, and fork adaptations of deleted upstream tests are
dropped.
- Upstream's deletion accepted for the `ThreadDetailsPrRows`,
`PreviewEmptyState`, `previewEmptyStateLogic`,
`projectScriptEditor.permissions` and `terminalSessionAvailability`
tests.
- Four fork browser tests are ported to the new
`require-disposable-render` pattern: ThreadForkDialog,
MoatlessArtifactChip, PreviewView.frame and MobileSidebarDrawer.
- **`threadRoutes.ts`, `ThreadRouteView.tsx`, `ChatView.tsx`:** take
upstream's `unlistedThreadDetail` (pingdotgg#19724), plus three fork changes:
  - An unlisted thread is "missing" once its subscription has failed.
- A closed thread is not redirected by `archivedAt`, because it is
absent from a Moatless listing and still opens by URL.
  - `onOpenRelatedThread` shows its toast only for a deleted shell.

The `unlisted-thread-adoption` inventory entry now owns
`threadRoutes.test.ts` and `ChatView.tsx`.
- **`AGENTS.md` (`decide`):** kept the fork bullet and appended
upstream's test rules and browser-test section.
- **Upstream's change with the fork delta re-stated:** `BranchToolbar`
(`showWorkspaceControls`), `GitActionsControl`, `ChatOverlayHosts`,
`FilePreviewPanel`, `projectFilesQueryState`, `usePreviewSession` (on
upstream's `inactiveAtom`) and `useThreadActions`.
- **`getSidebarForkParentThreadId`:** upstream removed it, so it moves
into `fork/ThreadParentLine.tsx`.
- **New inventory entries:** the `branding-test` path policy and the
`preview-session-use-atom-value` duplicate-add exception.
- **Sweep:** 19 owned-concern hits, all upstream-owned and taken as
upstream's. No new workflow.

## Feature classification

### Usable as-is
- Mermaid rendering through pele (pingdotgg#21472) and project icon previews
(pingdotgg#20372).
- Dialog and composer polish, PR panel fixes and centred tables.
- The Waiting thread state (pingdotgg#21500), which shows once the backend sends
`blockedOnThreadWait`.
- The even-pace tick on Limits (pingdotgg#22408).

### Unsupported in Moatless / needs implementation
- `device.easAccount` and EAS Simulator device hosts (pingdotgg#21841–pingdotgg#21846).
Added to the `UnsupportedMethodError` union, and the Integrations page
that holds them is already flag-gated.
- `projects.listIgnoredFiles` and the `ignoredFileListing` capability,
which "Add project to another machine" uses (pingdotgg#22180). Added to the
union. The dialog falls back to tracked files only while the capability
is absent.
- `pullRequests.changeBreakdown` (pingdotgg#22189, pingdotgg#22255). Its union already
carries `UnsupportedMethodError`, and the tooltip drops the split when
the call fails.
- T3 Connect reachability (pingdotgg#22172).
- Scheduled-task webhook filters and dedupe (pingdotgg#22194, pingdotgg#22234).
- `lastEventLoopStall` (pingdotgg#22213) and the `previewClose` scope (pingdotgg#22156).
- `compactIdleThreads` and `autoCompactThreshold` (pingdotgg#21682).
- MCP diff tools (pingdotgg#15464), the install id (pingdotgg#21506) and the writing-style
setting (pingdotgg#22203).

### Backend behavior to consider reproducing in Moatless
- **Settlement and waits:** `blockedOnThreadWait`, orphaned follow-ups
and delegated waits (pingdotgg#21499, pingdotgg#22581, pingdotgg#20037, pingdotgg#20025).
- **Stop and wake:** `latestRunStopRequested` stops a stopped
usage-limit thread from auto-resuming (pingdotgg#21114). A wake run settles when
the provider never opens the wake turn (pingdotgg#19196). Snooze survives a run's
completion (pingdotgg#22202, pingdotgg#17118).
- **Runs and shells:** run failures are persisted (pingdotgg#19646), and shell
updates coalesce for a slow client (pingdotgg#14859).
- **VCS:** bare repositories (pingdotgg#17340, pingdotgg#16688, pingdotgg#18152, pingdotgg#20368), stack
layers (pingdotgg#20415, pingdotgg#20388), VCS safety fixes (pingdotgg#15036, pingdotgg#19802) and PR
provider robustness (pingdotgg#18780).
- **Other:** update and restart safety (pingdotgg#22253), terminal and checkpoint
performance (pingdotgg#21242, pingdotgg#21291), resource metrics (pingdotgg#22122), provider
polling (pingdotgg#18851) and tool-token expiry (pingdotgg#18552, pingdotgg#19491).

`docs/fork/gaps.md` is updated under *Methods the backend does not
dispatch* and *Runtime fixes upstream made to its own server*.

## Verification

Full `verify.mjs` run: every check except `test` passes (duplicate-adds,
tripwires, resolution-check, unsupported-methods with ADD 2 / DROP 0 /
KEEP 2, lockfile, fmt, lint, typecheck and build).

Test failures and what was done about each:
- **Machine failures seen in earlier merges:** `shared` `AgentScope`,
`t3` `OrchestratorReplayFixtures` and `provider-opencode`
`OpenCodeServerLedger`. The `GitVcsDriver` and
`RepositoryIdentityResolver` partial-clone tests fail because of the
sandbox's `insteadOf` rewrite.
- **New and environmental:** `t3` `ProviderInstanceRegistry` reads the
sandbox's own `ANTHROPIC_*` and `CLAUDE_CONFIG_DIR`. With those unset,
all 9 of its tests pass.
- **`web`:**
- The fork's `AppRoot.test.tsx` broke on upstream's new `useEffect`
(pingdotgg#20886). It now stubs `useEffect` and passes.
- Four browser files failed to import in the parallel run and pass
alone.
- **Playwright:** 1.60 wants chromium build 1223, which this sandbox
lacks. Browser tests ran against 1243 through a symlinked
`PLAYWRIGHT_BROWSERS_PATH`.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---
Moatless task:
https://moatless.soaplabstest.com/tasks/c3df6407-7ad0-483c-a46c-e8b603326150
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Delegated-task completion overwrites a just-completed parent run back to waiting, deadlocking the thread behind its own completion notice

2 participants