fix(cli): wait for background continuations before headless exit - #13623
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| }) | ||
|
|
||
| function backgroundResult() { | ||
| delivery.retained = true // kilocode_change |
There was a problem hiding this comment.
[WARNING]: Promotion can retain the drain hold without installing a releaser
backgroundResult() sets delivery.retained = true before notify() has forked the ensuring(delivery.release) fiber. BackgroundJob.promote resolves waitForPromotion before running onPromote, and onPromote is Effect.ignore. If this path returns at the promotion check and ctx.metadata / notify then fails, execute's release sees retained and skips release(), so drain.wait never completes and headless kilo run hangs.
Consider setting retained only after notify() has actually forked, and invoking notify() on the promotion path only if it has not already run.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in a3f4b46. The background-result formatter no longer transfers ownership. The Task execution installs notification cleanup before returning a promoted result; the promotion hook now only updates metadata. A child scope also guarantees release if the notification is interrupted before its worker starts. Added a deterministic regression with a blocked, then failing metadata hook that verifies exactly one delivery and no early drain.
| for (let current: Entry | undefined = value; current; current = current.parent) { | ||
| current.count += delta | ||
| if (current.count !== 0) continue | ||
| for (const waiter of current.waiters) Deferred.doneUnsafe(waiter, Effect.void) |
There was a problem hiding this comment.
[WARNING]: Instance disposal can complete drain waiters as success
update finishes waiters with Effect.void whenever count hits 0. disposeInstance runs InstanceState invalidations concurrently (Promise.allSettled). If SessionRunState cancels runners first, drain.track releases while closed is still false, waiters succeed, and drainSession can publish session.drained / HTTP true for cancelled work. After re-init, a later wait also sees count === 0 and succeeds.
The stated invariant is that disposal is not a successful drain. Consider completing in-flight waiters with interrupt (and failing post-dispose wait) before other caches cancel work, or treating closed as failure even when count is already 0.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in a3f4b46. A synchronous pre-disposal phase interrupts drain waiters before any asynchronous cache cancellation runs. Drain state is now owned by its scoped service and tracks instance identity rather than allowing an invalidated cache to recreate successful empty state. Tests release the last hold during a gated disposer, verify interruption during and after disposal, verify a fresh instance can start, and verify another directory remains unaffected. The architecture check passes without a new allowlist exemption.
| [process.stdout, process.stderr].map( | ||
| (stream) => | ||
| new Promise<void>((resolve, reject) => { | ||
| stream.write("", (error) => (error ? reject(error) : resolve())) |
There was a problem hiding this comment.
[WARNING]: Output flush turns EPIPE into a failed run
flush() rejects on any stdout/stderr write error. run.ts awaits this in finally after a successful drain, outside the catch that sets exitCode. kilo run … | head or | jq (EPIPE / write-after-close) can fail a completed run.
Ignore EPIPE / ERR_STREAM_DESTROYED, or resolve rather than reject on write errors.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in a3f4b46. The final flush tolerates EPIPE and ERR_STREAM_DESTROYED, for both callback errors and synchronous write failures. Other I/O errors still reject. Tests cover both closed-stream cases, preservation of EIO failures, and waiting for both output callbacks.
| } | ||
| const request = new Request(`${config.baseUrl.replace(/\/$/, "")}/doc`, { headers, signal }) | ||
| const response = await (config.fetch ?? globalThis.fetch)(request) | ||
| if (!response.ok || !response.headers.get("content-type")?.toLowerCase().includes("application/json")) { |
There was a problem hiding this comment.
[WARNING]: Capability check reports every non-OK /doc as an old server
A 401/403 on --attach, a 5xx, or a non-JSON error body all throw "Server does not support session draining; upgrade or restart the server". Auth and transport failures are misdiagnosed as a missing drain API.
Reserve that message for missing/HTML/unrelated OpenAPI; surface status/auth errors separately.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Fixed in a3f4b46. HTTP 401/403 now report authentication/authorization rejection, and other non-404 failure statuses preserve their HTTP code instead of recommending an upgrade. Invalid capability JSON has a separate diagnostic without exposing the response body. Missing, HTML, and unrelated OpenAPI responses retain the unsupported-server diagnostic. Added coverage for 401, 403, 429, 500, 503, and malformed JSON.
| Effect.gen(function* () { | ||
| const state = yield* InstanceState.make(() => | ||
| Effect.gen(function* () { | ||
| const entries = new Map<SessionID, Entry>() |
There was a problem hiding this comment.
[SUGGESTION]: Drain entries are never pruned until instance dispose
entry() always inserts, and update never deletes when count returns to 0. A long-lived kilo serve / VS Code backend retains one Entry per session that was held, linked, or drained.
Consider dropping idle nodes with no waiters and no children so the map cannot grow without bound.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Improved in a3f4b46: idle wait queries no longer create entries, and unlinked idle entries are pruned when no holds, children, or waiters remain. Waiters pin their entry until they finish, with a controlled-scheduler regression covering release/reacquire before an awakened waiter resumes. Linked ancestry is deliberately retained until instance disposal so resuming an idle child still contributes to its original parent; that behavior now has a regression test too. This is not a claim of a strict bound on historical linked topology.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (26 files)
Previous Review Summaries (3 snapshots, latest commit ac54cca)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit ac54cca)Status: No Issues Found | Recommendation: Merge Files Reviewed (10 files)
Previous review (commit 383d9e9)Status: No Issues Found | Recommendation: Merge Files Reviewed (12 files)
Previous review (commit 7c845aa)Status: 5 Issues Found | Recommendation: Address before merge Overview
Fix these issues in Kilo Cloud Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (27 files)
Reviewed by grok-4.6 · Input: 191.3K · Output: 37.4K · Cached: 2.2M Review guidance: REVIEW.md from base branch |
|
Review/CI follow-up pushed in a3f4b46 and 383d9e9. Replied to all five review comments.
Local validation: 77 targeted tests pass, the full HTTP exerciser passes, root typechecks pass, lint reports no errors, and annotation/architecture/promise-facade/duplication checks pass without new exemptions. Fresh CI is running; no claim yet that Windows is green. |
Route drain requests and CLI output through the session owner directory. Simplify notification holds and completion waiters while preserving disposal safety. Cover local, attached, and daemon cross-directory runs.
Keep reservations through prompt intake, runner completion, and shell cleanup. Report cancellation after the parent turn has ended without emitting another turn close. Keep disposal bookkeeping in Kilo-owned code and consolidate regression fixtures without dropping the required coverage.
Preserve scoped cancellation and drain ownership through Task cleanup. Register promoted-job delivery independently of the parent caller while preventing duplicate notifications. Keep current-only parent interruption distinct from tree cancellation and verify the combined behavior with the real CLI regression suite.
2 participants
Problem
Headless
kilo runcan return exit code 0 while a background subagent or the parent's continuation still has unfinished work. This affects autonomous--autoruns, but the completion bug is not specific to that permission mode.There are two observed orderings:
idle, and the CLI exits before the queued continuation runs.finish_reason: tool-callsis normal for an intermediate model step; it is not itself an error. It becomes suspicious here because it is the last recorded step and required work is missing. The second ordering can even end withfinish_reason: stopwhile the child remains active, so checking or rewriting the finish reason is not a fix.Why this happens in the source
The old headless event consumer exits on the first parent idle:
session/prompt.tsdeliberately yields when a queued follow-up supersedes the current turn.effect/runner.tspublishes idle before the current execution/queue handoff has fully settled.cli/effect-cmd.tsdisposes the local instance when the command handler returns.Interactive clients stay alive across idle periods, which is why this can be easy to miss when testing the same model interactively.
Concrete evidence
Isolated before/after matrix
We used the real Linux CLI and a local scripted OpenAI-compatible provider. The task must obtain a child result, have the parent consume it, write a final marker, and emit the final answer. The verifier does not treat exit code 0 as success.
The three execution paths were:
kilo run --auto --format json, without Harbor or the redactor.PASS means the parent processed the child's result, wrote the required final output, and emitted its final answer. FAIL means that output was missing. These labels do not refer to the process exit code.
Each execution path runs three test cases:
Before: 3 passed, 6 failed. After: all 9 passed. The six failing cases still returned CLI exit code 0—that false appearance of completion is the bug. Harbor's verifier assigned reward 0 when the required output was missing and reward 1 when the probe passed.
All nine valid patched cases passed. A fresh unpatched 7.5.6 bare-CLI control still reproduced the incomplete successful exit. This establishes that Harbor and redaction are not required to cause the failure.
Discarded setup attempts are not counted in that matrix: an initial Harbor fixture had an ARM/AMD64 sidecar mismatch, and an early patched wrapper fixture selected the old executable through
/usr/local/bin. Both were corrected and binary selection was checked before recording the valid comparisons.Real-model confirmation, not just a scripted provider
A short real-model diagnostic made the child generate a random nonce. The parent was instructed not to read the nonce file; it had to obtain the value from the completed-task result, then write and report the matching value.
Before the fix:
Patched build:
PARENT_FINAL=<matching child nonce>, and finished withstop.task(background=true)→ readiness-onlybash→write(parent-final.txt)→ final answer. The parent did not read the child nonce file.The compiled probe build was
0.0.0-headless-drain-6137d1ea8d25, based onbbf6a278d7. This PR was subsequently rebased onto current main, preserving newer Task model-selection behavior; focused checks were rerun after rebasing.These are completion probes, not benchmark scores. No full benchmark was rerun with the fix, and this PR does not claim that every previous benchmark failure had this cause.
Fix and architecture
Keep
SessionStatus.idlemeaning “the current turn is idle.” Add a small backend session-drain contract for headless completion, instead of changing interactive idle semantics or introducing a new run/cancel protocol.Backend accounting
src/kilocode/session/drain.tsuses the existing Effect/instance-state conventions:The accounting helper does not execute prompts or create another scheduler. It adds no database tables, durable run registry, new status enum, or per-invocation ownership framework. The core background-job completion contract remains unchanged.
Task result handoff
The Task notifier now awaits parent injection instead of detaching and ignoring it. Delivery errors are surfaced through the existing session error path.
A targeted regression also demonstrated an intermediate nested-child response being returned as final. Task now waits for the child's own drain, then uses a bounded latest-assistant lookup only when that assistant is newer than the initial response. This keeps a child's initial “waiting” response from being reported as its completed result.
Wait endpoint and output fence
POST /kilocode/session/:sessionID/drainaccepts a bounded correlation token and waits for session drain.session.drainedevent through the awaited event bridge before returning the HTTP response.This avoids replacing one race with another where HTTP completion arrives before the consumer has processed buffered final output.
Headless compatibility and cleanup
background_processservers/watchers do not block it.Reviewer guide
packages/opencode/src/kilocode/session/drain.tspackages/opencode/src/kilocode/cli/run-drain.ts,packages/opencode/src/cli/cmd/run.tspackages/opencode/src/session/prompt.ts,packages/opencode/src/session/run-state.tspackages/opencode/src/tool/task.tspackages/opencode/src/kilocode/server/httpapi/{groups,handlers}/kilocode.ts,packages/schema/src/kilocode/session-drain.tspackages/opencode/test/kilocode/headless-session-drain.test.tssession-drain.test.ts,run-drain.test.ts,run-auto.test.ts,test/tool/task.test.tsThe remaining changes are dependency-layer wiring, generated SDK/OpenAPI files, test-layer wiring, and a CLI/SDK patch changeset.
Validation
7c845aa0ec: 110 focused tests passed across six files, including both headless orderings, explicit attach, drain/SDK behavior, Task lifecycle, nested results, and the newly merged Task model-selection cases. Full workspace typechecking passed (30 tasks); API regeneration left the tree unchanged; annotation, promise-facade, and the new duplication guard passed.From
packages/opencode, the focused post-rebase command was:env -u KILO_TEST_CLI_PATH bun test \ ./test/kilocode/headless-session-drain.test.ts \ ./test/kilocode/session-drain.test.ts \ ./test/kilocode/run-drain.test.ts \ ./test/tool/task.test.ts \ ./test/kilocode/task-nesting.test.ts \ ./test/kilocode/tool-task-model.test.ts \ --max-concurrency 3From the repository root:
Privacy and evidence limits
This description contains only sanitized timelines, aggregate outcomes, test commands, public source references and implementation details. It includes no API keys, auth headers, tenant IDs, raw CLI databases, or private artifact archives. Authentication-bearing temporary CLI state was removed, and retained probe output plus the proposed source changes were scanned for known credential values.
The diagnostic cost collector only measured streamed parent-step cost, so no total-cost claim is made here. Separate adapter provisioning/HOME issues and gateway payload-size failures are outside this fix.