fix: a failed session summary no longer fails the run - #305
Merged
Conversation
Adding .gitkeep for PR creation (default mode). This file will be removed when the task is complete. Issue: #304
A local provider that answers the streaming turn but refuses the non-streaming summarization request with HTTP 400 makes the failure deterministic: a 4xx is not retried, so the rejection lands while the turn is still streaming. Against 0.26.1 the CLI exits 1 with an `UnhandledRejection` on stderr and emits no `result` event at all. The unit-level case covers the same contract without a provider: summarize() must resolve even when its own work throws, because neither call site awaits it. Issue: #304
Session summarization is on by default, runs concurrently with the turn and is awaited by neither of its two call sites, so a provider error during it had no rejection handler anywhere. It surfaced as `unhandledRejection`, which exits the process with status 1 and aborts the still-streaming turn: the run reported the failure of its title as its own, and emitted no `result` event. `SessionSummary.summarize` is now guarded so it cannot reject, and logs the failure as a warning instead. The title `generateText` call carries the same `.catch` the body-summary call at the bottom of the file already had, so a future caller cannot reintroduce the crash, and both call sites mark the fire-and-forget with `void` plus a handler. Fixes #304
konard
changed the title
konard
marked this pull request as ready for review
September 9, 2026 08:51
The e2e case of tests/session-summary-failure.ts is the first top-level test that spawns src/index.js, and it is the only one failing on windows-latest: the child exits 0 in ~60ms right after the first config 'loading' log without ever reaching the provider. This temporary always-passing probe prints what --version and a plain cooperative-provider turn do on each platform so the Windows CI log can answer whether the CLI turn works there at all. Refs #304
Round 1 answered the first question: on windows-latest the CLI turn never reaches the provider even with a cooperative fake provider — the child exits 0 about 60ms after the first config 'loading' log with zero requests, i.e. a startup promise never settles, the event loop drains and Bun exits. So the stall is pre-existing and independent of the summary path. This round marks each startup await (global config file read, the TOML dynamic import, Auth.all, Config.global, Config.get, ModelsDev.get, Provider.state) to find which one never settles. Refs #304
Round 2 ruled out the startup path itself: driving Config.get, ModelsDev.get and Provider.state directly under 'bun --eval' settles on windows-latest in under half a second. The stall therefore needs the CLI entry point, whose one extra ingredient over that script is stdin. This round runs the same cooperative-provider turn three ways — closed Uint8Array stdin (what the failing test does), a shell pipe, and -p which bypasses stdin entirely. Refs #304
Round 3 ruled out stdin: closed-Uint8Array stdin, a shell pipe and -p (which bypasses stdin entirely) all stall the same way on windows-latest, always inside the global config loadFile sequence. This round imports the CLI entry point under a ref'd keep-alive timer — if the turn then completes, the CLI is losing the event loop rather than deadlocking. Refs #304
Round 4 showed the Windows stall is not a deadlock: under a ref'd keep-alive timer the same entry point finishes the turn, so the event loop simply drains while startup is pending. src/index.js ends in a floating 'main();', which leaves the whole run in an unawaited promise. This round runs a patched copy that awaits it, next to an unpatched control. Refs #304
The windows-latest checkout has CRLF line endings, so the round 5 patch regex matched nothing there and the probe threw before it could run. Refs #304
js/src/index.js ended in a floating 'main();': everything the CLI does lives in that promise, and nothing awaited it. On Linux and macOS the pending async filesystem I/O of startup keeps Bun's event loop alive, so the run survives; on Windows it does not — the loop drains, the process exits 0 about 60ms after the first config 'loading' log, and the CLI reports success without ever contacting the provider. Measured on the windows-latest runner against a local fake provider, patched copy versus unpatched control, side by side: PROBE_I_PATCHED exit=0 requests=["stream","non-stream"] hasResult=true PROBE_I_CONTROL exit=0 requests=[] hasResult=false This is what lets the end-to-end case of tests/session-summary-failure.ts run on all three matrix legs instead of only two. Closes #306
CI requires exactly one changeset per PR (scripts/validate-changeset.mjs); the Windows entry-point fix is folded into the existing note.
Contributor
Author
Working session summaryThe pipeline is still running (Windows/macOS/Linux unit tests plus lint). Current state of run 34334544440 on commit
What I did since the last CI failure:
I'll report the per-job conclusions once the run completes, and mark the PR ready if everything is green. This summary was automatically extracted from the AI working session output. |
Contributor
Author
🤖 Solution Draft LogThis log file contains the complete execution trace of the AI solution draft process. 💰 Cost: $12.483433📊 Context and tokens usage:Claude Opus 5: (3 sub-sessions)
Total: (5.1K new + 237.0K cache writes + 14.8M cache reads) input tokens, 106.9K output tokens, $12.483433 cost 🤖 Models used:
📎 Log file uploaded as Gist (5134KB)Now working session is ended, feel free to review and add any feedback on the solution draft. |
Contributor
Author
🎉 Auto-mergedThis pull request has been automatically merged by hive-mind.
Auto-merged by hive-mind with --auto-merge flag |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was wrong
Session summarization is a side quest whose whole product is a session title, a
body and a diff stat. It is on by default, it runs concurrently with the turn,
and neither of its two call sites awaits it:
js/src/session/prompt.ts:836— fire-and-forget on the first stepjs/src/session/processor.ts:511— fire-and-forget after a patch partNeither had a
.catch(), andSessionSummary.summarizeguarded modelresolution (
Provider.getModel(...).catch(() => null)) but not the titlegenerateTextcall. A provider that is reachable enough to resolve a model buterrors on the actual request therefore produced a rejection with no handler
anywhere, which
js/src/index.js:126turns intooutputError(...)+process.exit(1)— killing a turn that was still streaming. The failing runemits no
"type":"result"event at all: the run reported the failure of itstitle as its own.
How to reproduce
A local provider that answers the streaming request (the turn) normally and
refuses the non-streaming request (the summary) with HTTP 400. Both details
are load-bearing: a 4xx is not retried, so the rejection lands promptly, and
holding the stream open keeps the turn in flight when it does — which is the
race the bug loses (and why it presents as flakiness in the wild).
Before this PR:
After:
The fix
js/src/session/summary.ts—summarizecannot reject. Its body iswrapped and the failure is logged as a warning. This is what both callers
already assumed, and it defends once instead of twice.
generateTextgets the same.catchthe body-summary pass atthe bottom of the file already had. A missing title no longer skips the
rest of the summary, and a future third caller cannot reintroduce the crash.
voidand attach ahandler — defence in depth, and it documents the intent for the reader and
for
no-floating-promises.The failure is reported, not swallowed silently: it appears on the event stream
as
{"type":"log","level":"warn","service":"session.summary", ...}.Also fixed: the same defect class one level up (#306)
The new end-to-end test passed on Linux and macOS and failed on
windows-latestwithexit=0and zero requests reaching the fake provider.That was not the test:
js/src/index.jsended in a baremain();, so theentire run lived in a promise nobody awaited. On Linux and macOS the pending
filesystem I/O of startup happens to keep Bun's event loop alive to the end; on
Windows it does not — the loop drained mid-startup and the process exited 0
after printing its startup logs, without ever contacting the provider or
emitting a result.
Measured on the
windows-latestrunner with a patched/control pair of probesbuilt from the same entry point in the same job:
await main();is the fix, and it is what lets this PR's end-to-end regressiontest run on all three matrix legs instead of only two. Full evidence — five
rounds of CI probes ruling out stdin, a deadlock and the fake provider — is in
#306.
Tests
js/tests/session-summary-failure.ts(runs in the CI unit job — hermetic,loopback only, no API keys):
SessionSummary.summarizeresolves instead of rejecting when its own workthrows, and the child process exits 0 with no
unhandledRejection.resultevent withstatus: "success", noUnhandledRejectionon stderr, and asession.summarywarning proving the summary really was attempted and reallydid fail.
Both tests fail on the pre-fix tree (
0 pass, 2 fail) and pass after it.The full suite is green:
768 tests across 67 files, plusbun run check(eslint, prettier, file-size).
Out of scope
The issue's second hardening item — whether summarization should ever leave the
session's provider — is the routing half tracked in #303 / #217. This PR fixes
only the exit-status half: whatever provider the summary uses, its failure is
no longer the run's failure.
Fixes #304
Fixes #306