Repository navigation
fix(core): report every subagent result to its parent when it goes idle - #60360
kitlangton wants to merge 1 commit into
Conversation
![opencodejr[bot]](https://avatars.githubusercontent.com/in/5497873?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR fixes what it targets: on the base branch the PR's continuation, empty-step, ordering and restart tests fail, and with the PR they all pass. Core and server pass tsc. Public API: unchanged. The change only adds internal KV keys (subagent.parent/<id>) and metadata.source: "subagent" on subagent prompts.
Concerns:
- Steering a background child now drops its result. Background completions are no longer delivered by the job. Delivery now depends on the idle scan, and the scan skips any turn that contains a user message without
source: "subagent". A user typing into a running background child from the TUI therefore suppresses the parent's notification for that turn. Ondevthat turn's result was still delivered. See the inline comment. - #50751 is not fixed by this. The PR cites it as the origin, but its reproduction continues the child through a plugin's
session.prompt(prompt-session). That is a user prompt without the subagent source, which this PR deliberately skips. The case actually fixed is a child woken by a synthetic message (background shell or plugin). - Unbounded markers on every boot. Markers are only removed when a session is deleted, so they build up for every subagent child ever created. Each startup syncs all of them, and each deletion scans all of them (inline).
| } | ||
| } | ||
| for (const message of page) { | ||
| if (message.type === "user" && message.metadata?.source !== "subagent") prompted = true |
There was a problem hiding this comment.
Any user message without source: "subagent" in a busy period suppresses that turn's delivery. That includes a message a user types into a child that is still running its parent-requested work. For background children the job no longer delivers completions (notify only calls completeBackground), so the parent never gets that result. On dev it did, as noted on #50751: messages sent mid-run land in the answer the parent receives. Could the skip only apply when the turn was started by a direct prompt, e.g. the first user/synthetic message after the previous idle, rather than any user message inside it?
| }), | ||
| ) | ||
|
|
||
| const recover: Interface["recover"] = (suspended, prepare) => |
There was a problem hiding this comment.
Markers are only cleared when a session is deleted or the parent/child goes missing, so every subagent child ever spawned keeps one. recover then runs a full sync per marker on every startup: a KV read, two session lookups and a message page. forget also scans every marker on each session deletion. That grows without bound for heavy subagent users. Could markers be dropped, or skipped on recovery, once a child is idle and fully reported, or be indexed by parent?
There was a problem hiding this comment.
Both earlier points are fixed:
- A turn is now classified by the message that started it, so steering a running child still delivers its result.
- Startup only syncs markers that are dirty or orphaned, and session deletion uses a per-parent child index instead of scanning every marker.
I re-ran the #48826 reproduction in the TUI (simulated model). On dev the parent only ever gets the early reply. With this PR it receives a second <subagent> notification carrying the post-shell final answer. The three nearest test files pass (62/62) and core passes tsc.
Public API: unchanged. The only additions are internal KV keys (subagent.parent/… and subagent.child/…) and metadata.source: "subagent" on subagent prompts.
One optional note inline on the raw SQL in dirtyMarkers.
| ) | ||
|
|
||
| // Startup only visits markers that are orphaned or behind the child's newest projected idle row. | ||
| const dirtyMarkers = Effect.gen(function* () { |
There was a problem hiding this comment.
Not blocking: this goes around KV.Service and queries KVTable, SessionTable and SessionMessageTable directly, using json_extract on the KV value. That couples SubagentJob to KV's JSON-text storage and to the session schema, so a change to either would silently break startup recovery rather than fail to compile. A small query on SessionStore, such as the latest idle IDs for a set of sessions, together with kv.scan would keep it within the existing services.
There was a problem hiding this comment.
The remaining point is addressed. SubagentJob no longer queries the KV and session tables directly. Startup now scans a subagent.dirty/<child> flag, which is written in the same transaction that projects the idle or revert event. The earlier steering and startup-cost fixes are unchanged.
I re-ran the #48826 reproduction in the TUI: the parent still receives the follow-up <subagent> result. The three nearest test files pass (62/62) and core passes tsc.
Public API: unchanged. The only additions are internal KV keys (subagent.parent/…, subagent.child/…, subagent.dirty/…).
No new concerns.
efefd6a to
f2c1a6e
Compare
There was a problem hiding this comment.
All three earlier points are addressed:
- Turns are still classified by their starter message, so steering a running child keeps reporting.
- Markers are now one
jobrow per child, withreported/dirtycolumns. Startup only syncsdirtyrows, and session deletion removes rows in the projector. - The KV keys and the raw
json_extractqueries are gone.
I re-ran the #48826 reproduction in the TUI with the simulated model. On dev the parent never gets the post-shell result; with this PR it does. The six target test files pass (114/114), migration.ts --check shows no drift, and core passes tsc.
Public API: unchanged. The new job_subagent migration adds job.pending, job.reported and job.dirty, and makes job_child_session_idx unique after deleting duplicate per-child rows. As the description notes, rolling back requires dropping that index.
One new risk, inline: the catch-up loop in syncUnlocked can spin forever if a revert removes reported while it runs.
packages/core/src/session/subagent-job.ts:171: This re-scan has no exit ifreportedno longer exists.store.messagesreturns[]when the cursor ID is missing (if (input.cursor && !anchor) return []). A revert committed while this sync is running deletes that idle message and moves the row'sreportedto an older idle in the projector. The localreportedhere is still the deleted ID, solatestIdle !== reportedstays true. Every pass then gets an empty page and loops again, querying the database without end while holding the child's lock, andupdateMarker(..., false)never runs. Could this re-read the row'sreported(or bail out on an empty page) instead of looping on local state?
Subagents now report every real result to their parent whenever they go idle, including turns that finish after the initial run or across a server restart.
A subagent is a child session launched to do a task; if a background command or plugin woke it for another turn after its first reply, the parent never received the follow-up result. Turns started by a user prompting an idle child directly, empty retries, and deliberate cancellations behave as before, while steering a running child still delivers its answer.
🔍 Origin: reported in #48826; replaces #58825 with one saved-marker path on the SQLite
jobtable (#60491).❌ Before
sequenceDiagram participant P as Parent session participant J as Background job participant C as Child session J->>C: run first turn C-->>J: first turn settles (or stops empty) J->>P: deliver once, then stop watching Note over C: Background shell or plugin wakes child C-->>C: ❌ finishes real result, parent never hears✅ After
sequenceDiagram participant P as Parent session participant J as SQLite job row participant C as Child session Note over J,C: One job row per child tracks reported idle + dirty flag C->>J: turn 1 idle (text or error) J->>P: deliver <subagent ...> and advance reported Note over C: Later turn (or after server restart) C->>J: turn 2 idle (text or error) J->>P: ✅ deliver <subagent ...> in order and advance reported📊 Proof
<subagent>for each real turn in orderjobrow survives restart; catches up in order and reconciles legacy notifications without duplicatesThe continuation, empty-step, ordering, revert, and restart tests in
tool-subagent.test.tsandsession-execution.test.tsfail ondevwithout the source changes. 114 targeted tests pass across 6 test files.↩️ Undo: revert isn't enough if a database has already run migration
20261030204714_job_subagent, which addspending,reported, anddirtycolumns and upgradesjob_child_session_idxto a unique index onjob(child_session_id). Earlier code ignores the nullable/defaulted columns, while multiple retained backgroundjobrows for the same child session would require dropping the unique index.👀 Read:
packages/core/src/session/subagent-job.ts(syncUnlocked, legacynotification_idreconciliation indeliverToParent, and the foreground hold inblock) alongside theJobTableprojections inpackages/core/src/session/projector.ts. The rest is routine.Supersedes #57132 for delivering follow-up turns after background work · builds on #60491
Checks run
packages/core:bun script/migration.ts --check(0 schema drift)packages/core:bun test test/job.test.ts test/session-execution.test.ts test/tool-subagent.test.ts test/config/command-subagent.test.ts test/session-projector.test.ts test/session-revert.test.ts(114 pass, 0 fail)packages/core:bun run typecheckandbun run typecheck:tsc(0 errors)packages/server:bun run typecheckandbun run typecheck:tsc(0 errors)bun run check(lint+ workspacetypecheck, 35/35 packages, 0 errors)Added by the opencode bot (AI-generated).
Fixes #48826
Related: #50751, #36349, #45480