fix(kimi-code): flush wire journals before print-mode exit - #3531
Conversation
A print-mode turn's tail records (step.end / turn.ended / prompt.completed) are dispatched fire-and-forget and reach the journal only through the wire service's async persist queue. The print cleanup path never awaited that queue: with telemetry disabled (KIMI_DISABLE_TELEMETRY=1) cleanup returns in microseconds, and process.exit on the error path cut off the pending append-log flush, dropping the failed turn's closing records. Flush every session agent's dispatcher (which awaits the wire persist queue and the append-log store) before disposing the app, bounded by the shutdown timeout and best-effort so a persist failure never masks the turn outcome.
🦋 Changeset detectedLatest commit: ab7c823 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3511b89a48
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| await Promise.all( | ||
| [...handles].map((handle) => handle.accessor.get(IEventDispatcher).flush()), | ||
| ); |
There was a problem hiding this comment.
When multiple agents are present and any dispatcher's flush() rejects before another agent finishes flushing, Promise.all rejects immediately; the caller swallows that rejection and proceeds to app.dispose(), so the remaining flushes are no longer awaited and the subsequent process exit can still truncate otherwise healthy journals. Settle every per-agent flush independently (for example with Promise.allSettled) before returning the best-effort cleanup.
Useful? React with 👍 / 👎.
commit: |
Promise.all rejects as soon as one agent's flush fails; the best-effort caller then proceeds to app.dispose() and process.exit while the remaining agents' flushes are still in flight, re-exposing their journals to the truncation this cleanup is meant to prevent. Await every flush with Promise.allSettled instead (matches the drain helper's convention), and cover it with a two-agent regression test.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 463967f226
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // that returns fast (e.g. telemetry disabled) lets process.exit cut off | ||
| // that queue before the records land on disk. Best-effort: a persist | ||
| // failure was already reported where the append failed. | ||
| await raceWithTimeout(flushWires(), CLI_SHUTDOWN_TIMEOUT_MS).catch(() => {}); |
There was a problem hiding this comment.
When the wire flush uses its 3-second allowance and both telemetry pipelines are also slow, cleanup can now take up to 9 seconds (flushWires, telemetryService.shutdown, and shutdownTelemetry each receive CLI_SHUTDOWN_TIMEOUT_MS) while the outer cleanup still stops waiting after 8 seconds. On a failed run or signal, the caller then invokes process.exit immediately, truncating the final telemetry shutdown and potentially preventing app.dispose() from running. Use a shared deadline, run independent shutdowns concurrently, or increase the outer bound to cover all sequential phases.
Useful? React with 👍 / 👎.
The wire flush, v2 telemetry shutdown, and v1 telemetry shutdown each hold a 3s allowance; run sequentially they can take 9s, past the 8s outer cleanup bound — the caller's process.exit would then cut off the tail (app.dispose included). The phases are independent, so await them concurrently (worst case one allowance) while keeping the v2 shutdown failure propagation and the best-effort semantics of the other two.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84fe69b924
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // process.exit cut off that queue before the records land on disk. | ||
| // Best-effort: a persist failure was already reported where the | ||
| // append failed. | ||
| raceWithTimeout(flushWires(), CLI_SHUTDOWN_TIMEOUT_MS).catch(() => {}), |
There was a problem hiding this comment.
When SIGINT, SIGTERM, or SIGHUP arrives during an active turn, installPromptTerminationCleanup invokes this cleanup without first cancelling or awaiting that turn. This flush only drains records queued at that instant, so the still-running turn can append its cancellation and closing records afterward; app.dispose() starts asynchronous teardown without awaiting it, and the signal handler then calls process.exit, recreating the journal truncation this change is intended to prevent. Cancel and settle active agents before the flush, or await teardown and perform the final flush afterward.
Useful? React with 👍 / 👎.
A termination signal (SIGINT/SIGTERM/SIGHUP) can arrive mid-turn: the cleanup ran the wire flush immediately, while the still-running turn only produced its cancellation and closing records from dispose()'s fire-and-forget teardown — after the flush, and after the signal handler's process.exit. Cancel every session agent's queued and active turns and await loop idleness before flushing (best-effort, bounded by the shutdown timeout); idle loops make this a no-op on the normal exit paths. Torn-down agent scopes are skipped defensively, mirroring the flush's per-agent settlement.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8fb8c8c87
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for (const turnId of loop.status().pendingTurnIds) loop.cancel(turnId); | ||
| loop.cancel(); | ||
| } | ||
| await Promise.allSettled(loops.map((loop) => loop.settled())); |
There was a problem hiding this comment.
When a termination signal arrives during an active prompt, IAgentLoopService.settled() can resolve from AgentLoopService.releaseActiveTurn() before the turn-result callback in AgentPromptService.settle() dispatches prompt.completed; cleanup can therefore start and finish the wire flush before that final record is queued, after which the signal handler immediately calls process.exit. Fresh evidence beyond the earlier comment is this ordering between releaseActiveTurn(), job.result.resolve, and AgentPromptService.settle(), so quiescing must also await prompt completion (and drain pending prompts) before flushing.
Useful? React with 👍 / 👎.
| if (suppressions.length > 0) await Promise.allSettled(suppressions); | ||
| if (activeCount === 0 || batch.length === 0) break; | ||
| await Promise.all(batch); | ||
| await Promise.allSettled(batch); | ||
| } | ||
| if (allWaiters.length > 0) await Promise.all(allWaiters); | ||
| if (allWaiters.length > 0) await Promise.allSettled(allWaiters); |
There was a problem hiding this comment.
When print background mode is drain and either suppressTerminalNotification() or wait() rejects because task state cannot be persisted, these allSettled calls discard the error and let the CLI report success even though task state or notification suppression was not saved. This behavior was unintentionally changed alongside the per-agent wire-flush handling; only independent wire flushes should be best-effort, while background draining should retain its previous rejection propagation.
Useful? React with 👍 / 👎.
…iesce Two follow-ups from review: - The loop settles (releaseActiveTurn) before the prompt-settle chain dispatches prompt.completed, so awaiting loop idleness alone let the wire flush race the final record. Quiesce now also drains each agent's prompt queue and awaits the tracked prompt completions (registered at enqueue time) before the flush runs. - Restore drainBackgroundTasks to Promise.all: a previous edit unintentionally switched it to allSettled alongside the per-agent wire flush, silently discarding persistence failures from suppressTerminalNotification()/wait(). Only the independent wire flushes are meant to be best-effort.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e566535306
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| quiesceAgents = async () => { | ||
| await quiesceSessionAgents(resolved.session, resolved.agent); | ||
| await Promise.allSettled(promptCompletions); |
There was a problem hiding this comment.
When termination occurs while a subagent or background-generated prompt is active, quiesceSessionAgents() cancels its loop, but promptCompletions contains only handles passed through the CLI's runNativeTurn. Since IAgentLoopService.settled() can resolve from releaseActiveTurn() before that agent's AgentPromptService.settle() dispatches prompt.completed, this wait does not close the ordering gap for other agents; their final record can therefore be queued after the wire flush and lost on process.exit. Fresh evidence beyond the prior comment is that the final code registers only the main run's completion rather than completions from every drained agent.
Useful? React with 👍 / 👎.
Tracking only the main run's prompt completion left subagent and background prompts racing the wire flush the same way: the loop reports idle before that agent's prompt-settle chain dispatches prompt.completed. Replace the per-handle completion tracking with a uniform wait — after draining and cancelling, poll every agent's prompt queue snapshot until none reports an active or pending prompt. settle() clears the active prompt and dispatches the record in one synchronous block, so an empty snapshot proves the record is already queued for the flush.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61e34b089f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const snapshot = service.list(); | ||
| return snapshot.active !== undefined || snapshot.pending.length > 0; |
There was a problem hiding this comment.
startNext() is launching them
When a termination signal arrives after AgentPromptService.startNext() removes a prompt from pending but before it assigns active—for example while materializeDaemonRefs() or the pre-submit hook is awaiting—drain() cannot cancel that prompt and this snapshot reports an empty queue. The loop can also still be idle, so cleanup flushes the journal and calls process.exit while startNext() remains able to launch the turn and dispatch records afterward. Include the service's launching phase in quiescence or await/cancel the in-flight prompt launch before treating an empty snapshot as settled.
Useful? React with 👍 / 👎.
… quiesce A termination signal can also arrive while startNext() is mid-launch: the prompt has left pending and is not active yet, so drain() cannot cancel it and an active/pending snapshot reads empty. Quiesce would then flush and exit while the launch still dispatches records afterward. Expose the service's launching phase on PromptQueueSnapshot and make the print cleanup's quiesce a repeat-until-idle loop: every pass drains, cancels, and awaits the loops, then re-checks launching/active/pending, so a prompt surfacing from the launch window is cancelled on the next pass instead of escaping the flush.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a755e738b8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return false; | ||
| } | ||
| }); | ||
| if (!busy) return; |
There was a problem hiding this comment.
When print background mode is exit, or a cron becomes due during shutdown, task and cron services remain live after this empty snapshot. AgentTaskService.notifyAgentTask() and cron deliverFire() enqueue directly into IAgentLoopService (taskService.ts:1113 and cronService.ts:185), so a completion or tick after loop.settled() and this check can start another turn without making PromptQueueSnapshot busy. The concurrent wire flush may already have finished while telemetry shutdown keeps the process alive, after which the signal handler exits and loses the newly emitted records. Stop these producers before quiescing, or prevent new loop enqueues and recheck loop quiescence before flushing.
Useful? React with 👍 / 👎.
An empty prompt queue is only a point-in-time observation: background task completions and cron fires enqueue straight into the loop, bypassing the prompt queue, so a late producer could still start a turn — and new records — after the journals were flushed. Quiesce now leaves a quiescence guard held on every loop once queues read empty; the caller holds the release across the wire flush and app disposal, so late submissions queue behind the guard and are rejected by disposal instead of racing process.exit. Loops that refuse the guard (still busy) send the quiesce into another drain/cancel/settle pass.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0614a7abb9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for (const agent of agentManager.list()) { | ||
| const handle = agentManager.handleOf(agent.agentId); | ||
| if (handle !== undefined) handles.add(handle); |
There was a problem hiding this comment.
When a non-main agent has entered AgentLifecycleService.remove() during shutdown, both list() and handleOf() exclude it because its managed.closing flag is already set, so this collector omits that agent from both quiescence and flushSessionWires(). Its removal releases the append-log lease with a fire-and-forget retirement flush, allowing the signal handler's subsequent process.exit() to truncate that subagent's closing records. Fresh evidence beyond the earlier all-agent comments is the explicit closing-agent filter in both lifecycle lookup methods; only the main agent is preserved separately here.
Useful? React with 👍 / 👎.
AgentLifecycleService.remove() quiesced the agent but never flushed its wire journal: the records reached the append log only through the fire-and-forget retirement flush, so a process exiting right after a subagent's removal (e.g. print mode on a termination signal) could truncate the closing records. Flush the agent's event dispatcher after the quiesce and before disposal; a persist failure is reported without blocking the removal.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dda1e054eb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| compaction.abortController.abort(reason); | ||
| } | ||
| await Promise.all([loop.settled(), compactionSettled, prompt.drain(reason)]); | ||
| await handle.accessor.get(IEventDispatcher).flush().catch(onUnexpectedError); |
There was a problem hiding this comment.
When remove() cancels an agent with an active prompt, loop.settled() can resolve in releaseActiveTurn() before the turn-result callback invokes AgentPromptService.settle() and dispatches prompt.aborted/prompt.completed; prompt.drain() also returns without awaiting that completion. Fresh evidence in the final revision is that this new flush can therefore snapshot the persistence queue before the prompt's final event is appended, after which scope disposal may truncate it. Await the active prompt completion or verify that the prompt queue is fully idle before flushing.
Useful? React with 👍 / 👎.
…ing a removed agent The remove() flush could still snapshot the persistence queue before the cancelled prompt's final record was appended: loop.settled() resolves in releaseActiveTurn() before the prompt-settle chain dispatches prompt.aborted/prompt.completed, and prompt.drain() does not await that settlement. Wait (bounded) for the prompt queue to report no launching, active, or pending prompts before flushing; an unreadable snapshot counts as idle so a wedged service cannot stall the removal.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c739e50c0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const snapshot = prompt.list(); | ||
| idle = | ||
| !snapshot.launching && snapshot.active === undefined && snapshot.pending.length === 0; | ||
| } catch { | ||
| idle = true; | ||
| } | ||
| if (idle || Date.now() >= promptIdleDeadline) break; |
There was a problem hiding this comment.
When remove() runs while startNext() is awaiting daemon materialization or a pre-submit hook, the initial prompt.drain() cannot see the prompt because it has already left pending but is not yet active, and the loop has no turn to cancel. This polling loop only observes that prompt afterward; it never drains or cancels it again, so once it becomes active it can run until the three-second deadline, after which the journal is flushed before scope disposal generates its cancellation records. Repeat the drain/cancel operation while waiting, or freeze admissions and cancel the newly launched prompt before flushing.
Useful? React with 👍 / 👎.
…moval A prompt mid-launch (startNext awaiting daemon materialization or a pre-submit hook) is invisible to drain() and has no turn to cancel yet; the passive idle wait would then let it run to the deadline and flush before its closing records existed. Repeat drain/cancel/settle on every pass until the queue reads idle, and once idle hold a quiescence guard across the flush and disposal so a late producer cannot start new work in between.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4a76a4433
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Quiescence guards stay held across the flush and disposal so a | ||
| // late background-task completion or cron fire cannot start a new | ||
| // turn (and new records) after the journals were drained. |
There was a problem hiding this comment.
In print background exit mode, or when a signal arrives while a detached task is active, the quiescence guard only blocks loop admission: AgentTaskService.recordTaskTerminated() dispatches TaskTerminated directly, and app.dispose() can itself trigger that path by cancelling the task after this flush has completed. The signal handler may then call process.exit before the new dispatch is persisted, so task records can still be lost. Fresh evidence beyond the earlier producer comment is the direct dispatcher call at taskService.ts:1085-1088, which bypasses the guarded loop entirely; stop/settle task producers before flushing or perform a final flush after their disposal.
Useful? React with 👍 / 👎.
Task termination bypasses both the prompt queue and the loop quiescence guard: AgentTaskService dispatches TaskTerminated straight to the wire, and disposal would force-stop still-running tasks after the journals were already flushed. Stop every session agent's tasks up front in the quiesce phase (mirroring AgentLifecycleService.remove()), so each task's termination record is dispatched before the flush instead of racing process.exit. keepAliveOnExit tasks stay exempt, matching remove().
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36eb475935
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| await Promise.allSettled( | ||
| handles.flatMap((handle) => { | ||
| try { | ||
| return [handle.accessor.get(IAgentTaskService).stopAllOnExit('Session closed')]; |
There was a problem hiding this comment.
When print mode exits with [task].keepAliveOnExit=true, or when suppressing a detached task's terminal notification fails, stopAllOnExit() is not a shutdown barrier: taskService.ts:800-808 either returns immediately or rejects before calling stopAll(). Because Promise.allSettled discards that result, cleanup proceeds to freeze the loops and flush while the task remains active; its direct TaskTerminated dispatch can then occur after the flush and be lost on process exit. Fresh evidence beyond the earlier producer comment is that the newly added shutdown path specifically delegates to this conditional, failure-short-circuiting helper; use an unconditional best-effort stop/settlement barrier here before flushing.
Useful? React with 👍 / 👎.
…AllOnExit A detached task's terminal-notification suppression failing (e.g. the persist write rejects) used to reject the whole stopAllOnExit before stopAll() ran, so every task stayed active — and callers settling the rejection (print-mode cleanup, agent removal) proceeded as if the tasks were stopped, losing their termination records at exit. Settle each suppression independently, log the failure, and always stop the tasks. keepAliveOnExit tasks remain exempt by design.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
2 participants
Related Issue
No tracking issue — regression reported against e2e fixtures after #3498.
Problem
Print mode (
kimi -p) never waited for the wire journal's async persist queue on exit. A turn's tail records (step.end/turn.ended/prompt.completed) are dispatched fire-and-forget and reach the journal only through the wire service's async persist queue (a microtask-scheduled append-log flush plus a threadpool fs write). Before #3498, print mode always attached the telemetry appender, and shutting it down (a network flush) kept the event loop busy long enough for that queue to drain. #3498 made print mode honorKIMI_DISABLE_TELEMETRY=1; with no appender attached, cleanup returns in microseconds and the error path'sprocess.exit(1)cuts off the pending flush — the failed turn's closing records never land on disk, so a resumed session / transcript is missing the turn's error closing. Reproduced on the same build:KIMI_DISABLE_TELEMETRY=1→ 0/6 runs persiststep.end(finishReason=error); telemetry re-enabled → 6/6.What changed
The print cleanup path now explicitly flushes every session agent's event dispatcher (which awaits the wire persist queue and the append-log store) before disposing the app:
closingagent's already-dispatched records land;CLI_SHUTDOWN_TIMEOUT_MSvia the existingraceWithTimeout, and best-effort (errors swallowed) so a persist failure never masks the turn's own outcome — it was already reported where the append failed;Tests: three new cases in
v2-run-print.test.ts— flush happens beforeapp.dispose()on success, still happens when the turn fails, and a flush failure does not mask the turn error.Checklist
/approve).gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.