fix(cli): flush the session ingest tail on shutdown - #12545
Conversation
| const share = await options.getShare(sessionId).catch(() => undefined) | ||
| if (!share) return | ||
| const freshShare = await options.getShare(sessionId).catch(() => undefined) | ||
| if (freshShare) shares.set(sessionId, freshShare) |
There was a problem hiding this comment.
WARNING: shares map grows unbounded for the lifetime of the process
shares.set(sessionId, freshShare) runs on every successful getShare resolution and there is no corresponding delete anywhere in this file. In a long-running kilo serve daemon (or a TUI worker that lives across many sessions), every session that has ever synced a share accumulates a permanent entry here (and cached similarly never expires, though that's a single value so it's less of a concern). Since this cache exists purely as a shutdown-drain fallback, consider bounding it — e.g. drop the entry for a session once its session_close batch is successfully flushed, or cap shares to a small LRU — so it doesn't grow proportionally to total sessions served over the process lifetime.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| shutting = true | ||
| const deadline = now() + bound | ||
|
|
||
| for (const sessionId of Array.from(queue.keys())) { |
There was a problem hiding this comment.
SUGGESTION: Redundant initial flush loop before while
This for loop that kicks off flush() for every queued session is functionally identical to the for loop at the top of the while body below (lines 397-399). Since the while condition itself checks queue.size > 0, dropping this initial loop and letting the while loop's own first iteration do the same work would remove the duplication without changing behavior.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No New Issues Found | Recommendation: Merge OverviewThis increment (vs. previously reviewed commit The previously flagged unbounded Files Reviewed (1 file)
Previous Review Summaries (7 snapshots, latest commit 0c3cc0f)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 0c3cc0f)Status: No New Issues Found | Recommendation: Merge OverviewThis increment (vs. previously reviewed commit The previously flagged unbounded Files Reviewed (1 file)
Previous review (commit ea6b7e5)Status: No New Issues Found | Recommendation: Merge OverviewThis increment (vs. previously reviewed commit The previously flagged unbounded Files Reviewed (4 files)
Previous review (commit b2540c4)Status: No New Issues Found | Recommendation: Merge OverviewThis increment (vs. previously reviewed commit The previously flagged unbounded Files Reviewed (1 file)
Previous review (commit bc34014)Status: 1 Issue Found | Recommendation: Merge Overview
Issue Details (click to expand)SUGGESTION
This round's diff (vs. previously reviewed commit The previously flagged unbounded Fix these issues in Kilo Cloud Files Reviewed (2 files)
Previous review (commit 0865051)Status: 1 Issue Found | Recommendation: Merge Overview
Issue Details (click to expand)SUGGESTION
This round's diff (vs. previously reviewed commit The previously flagged unbounded Fix these issues in Kilo Cloud Files Reviewed (2 files)
Previous review (commit 621ebc8)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
This round's diff (vs. previously reviewed commit The two previously flagged issues (unbounded Fix these issues in Kilo Cloud Files Reviewed (3 files)
Previous review (commit 6074e8e)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
The core Note: this is a user-facing bug fix (lost session tail on exit) with no Fix these issues in Kilo Cloud Files Reviewed (11 files)
Reviewed by claude-sonnet-5 · Input: 26 · Output: 5.5K · Cached: 677K Review guidance: REVIEW.md from base branch |
|
(bot) Standin review — round 1 (reviewer-bot substitute per the workflow override; head reviewed: Findings (4), with orchestrator triage:
Residual testing risks noted by the reviewer (no action): on-device E2E per ending runs as the next workflow stage; K4's interactive Ctrl-C claim rests on the raw-mode argument (K4 is droppable by design if the E2E shows a regression); the A fresh standin review will run on the repaired head; the loop repeats until a round reports no actionable findings on the exact latest head. |
|
(bot) Standin review — round 2: A fresh standin reviewer re-reviewed the complete diff on the repaired head, including the round-1 repair commit (bound-expiry test, Residual testing risks carried by the reviewer (no action): on-device E2E per ending is the next workflow stage; the The standin loop is converged on the exact latest head. On-device E2E results ( |
|
(bot) On-device E2E (plan §6.7/§6.8): ACCEPT — 11/11. Verified by a fresh Binary exercised: Pass rates vs baseline (baseline loss measured pre-fix):
Per attempt: 3 turns ending in a unique §6.8: the terminal batch is one POST ( Residuals (unchanged from the PR body): |
|
(bot) Standin review — rounds 3–5 triage record (fresh reviewer each round, each on the exact then-latest head) Round 3 (head
Round 4 (head Round 5 (head
A fresh round-6 review is running on head |
|
(bot) Standin review — convergence record (rounds 6–8) and review of record for head Round 6 (head
Round 7 (head Round 8 (head Orchestrator acceptance for the §6.5 per-directory clause (final, recorded per the reviewers' own offered resolution): plan §6.5's second half ("a per-directory instance dispose or reload does not drain the global queue") is an absence-of-hook property. The drain is reachable only through the process-level With that resolution, round 8 has no valid findings on the exact latest head — the standin loop is converged. Summary of the loop: 8 fresh Remaining evidence: CI on |
* fix(cli): drain session ingest queue on shutdown and flush terminal batches promptly * fix(cli): drain the session ingest queue on process shutdown * fix(cli): pin drain bound expiry, add changeset, conform to naming rule * fix(cli): keep kilo-sessions out of the CLI startup import graph * fix(cli): never let the ingest drain task reject the shutdown sequence * test(cli): pin drain-before-dispose ordering on the KiloCli shutdown path * fix(cli): make the guarded ingest drain non-rejecting and correct the lazy-import rationale * test(cli): cover the retryable-status drain path under shutdown * test(cli): decouple cli-shutdown drain assertions from declaration order
Problem
While a Kilo CLI session is live, the mobile session-detail screen streams assistant messages in over the relay. Opening the same session later read-only, the transcript is often missing the last message(s) the user watched arrive.
Root cause (CLI-side): the durable transcript depends on a client-side coalescing queue (
IngestQueue) that flushes at most once per ~1 s per session and was never flushed on any exit path — anything still queued when the process ends is lost permanently, even though the cloud already received (and discarded after broadcast) every one of those events over the live relay. Confirmed reproduction rates on the unmodified baseline: clean/exitimmediate 3/3, SIGINT 3/3, SIGHUP 1/1 lost the tail; delayed/exit(≥10 s) 0/2.Fix
IngestQueue. A newdrain()flushes every pending session concurrently: bounded at 3 s (the TUI parent terminates the worker unconditionally 5 s after asking it to shut down), joins in-flight flushes rather than just the queue map (an empty map is not quiescence), suppresses re-enqueue globally once shutdown begins (a joined flush that fails retryably cannot re-queue the tail behind the drain's back), and falls back to the last-known client/share without re-validating auth so the tail is actually POSTed at teardown instead of silently dropped ongetClient()'s HTTPauthValidcheck.K2), always beforedisposeAllInstances()so the GlobalBus handler and remote wiring stay live during the drain:shutdownRPC — covers the three reproduced endings (/exit, SIGHUP, SIGTERM), via a purecreateWorkerShutdown({ drain, dispose, stopServer })seam;KiloShutdownregistry for non-TUI commands (kilo run, …) reaching the top-levelfinally;kilo servesignal handler, so the daemon child drains when the daemon itself is stopped.K3). A batch containingsession_closeno longer inherits the open ~1 s debounce window:sync()bases its deadline onmax(now(), retry.until)andschedule()lets terminal batches move a flush earlier. Streamingpart/messagecoalescing is unchanged. This narrows the loss window for endings that cannot be intercepted (kill -9, crash, power loss) from ~1 s to a single POST round-trip — it does not close it.K4). An externalkill -INTnow takes the same graceful path as SIGHUP/SIGTERM. Interactive Ctrl-C is a raw-mode keypress, not a signal, so interactive behaviour is unchanged.Tests
23 unit tests in
packages/opencode/test/kilocode/sessions/: drain flushes all pending sessions (queue pinned emptied on success, not re-enqueued on failure), drain POSTs via the cached client/share when fresh resolution fails at teardown, drain joins a mid-POST flush and suppresses its re-enqueue on retryable failure, terminal scheduling (close-into-open-window pulls the flush to ~0, non-terminal stays atnow()+1000, retry backoff still respected), worker-seam ordering (drain before dispose), once-guard. The 12 pre-existingingest-queuetests are unmodified and green.bun run typecheckandoxlint(0 errors) pass.Deliberate two-PR deviation
The confirmed root cause is in the CLI, so the fix ships from this repo. The companion cloud PR Kilo-Org/cloud#4786 carries only the E2E harness change that makes on-device verification of this CLI-side fix possible at all. The batch instruction said one PR from the cloud worktree; the deviation is deliberate and stated in both PR bodies.
Residuals (documented non-goals)
kill -9, crashes and mid-turn kills keep losing an unflushed tail; the terminal-flush change narrows but cannot close that window.kilo servedrain is unit-wired with an inspected call site but no daemon-stop E2E (no reproduction exists for that path).Verification
/exit, SIGHUP, SIGINT; delayed-/exitcontrol) — ACCEPT 11/11, posted as a(bot)comment with the running CLI's version stamp