Pin the SSE finite-generator completion contract with an integration regression anchor - #2427
Merged
Merged
Conversation
…regression anchor Promotes the QA-537 exploratory spec into the integration suite as integrationTests/server/sse-finite-generator.test.ts. It pins the shapes #1628 / PR #1632 turned on -- where a generator's terminal `done` step falls relative to its yielded values -- across N=0, N=1, N=5 and N=3000, plus a mid-stream-throw contrast arm, a liveness canary and an hdb.log uncaughtException sweep. The SSE suites in integrationTests/server/ had each grown their own copy of the AbortController-bounded SSE consumer, the /Probe/ reader and the hdb.log helpers. Rather than add a fourth, they move to integrationTests/utils/sseStream.ts and sse-throw-midstream.test.ts now imports them; the raw-socket capture used by stream-error-contract.test.ts is a deliberately different technique and stays put. Test-only: no product code changes. Refs #1628 Co-Authored-By: Claude Opus <noreply@anthropic.com>
…ughtException checks - awaitFixtureReady() now owns both suites' readiness poll and reads hdb.log with readFileSync, so an unreadable log fails setup instead of silently making every uncaughtException delta zero. - Every completion arm gets its own delta check, and both suites' final sweeps compare against the baseline taken in before() rather than a test-local slice, so a case whose own check was outrun by the log flush is still caught. - consumeSse() decides `aborted` from its own timer rather than from which event won the settle race: an abort destroys the response, so a hung stream also emits 'close', and attributing the settle to that event reported a genuine hang as a bounded termination. - waitForProbe() passes its remaining deadline down to each probe request. - Trimmed the narrating comments off the fixture, schema and helper module. Co-Authored-By: Claude Opus <noreply@anthropic.com>
The completion arms compared counts and endpoints, so a stream that reordered or duplicated interior frames -- or, for ThrowGen, delivered no pre-error prefix at all -- still passed. Each arm now compares the full `n` sequence; ThrowGen keeps a 1-2 upper/lower bound because the abrupt destroy can drop the last chunk before it drains. Co-Authored-By: Claude Opus <noreply@anthropic.com>
- awaitFixtureReady() polls for hdb.log instead of reading it once: the HTTP port can answer before the log writer has created the file, which would have turned the readability backstop itself into a deterministic ENOENT in before(). - Both final sweeps read the log strictly, so a log that disappears mid-run fails loudly rather than counting zero against a zero baseline. - consumeSse() only attributes a settle to its own timeout when that timer actually fired; any other AbortError now surfaces as an error with its cause instead of an empty result. - waitForProbe() treats a throwing predicate as "not satisfied yet", so a probe error payload fails on the caller's assertion rather than as a TypeError inside the poll. - The 2xx assertion reports the transport error alongside the status. Co-Authored-By: Claude Opus <noreply@anthropic.com>
…tion - consumeSse() flushes the StringDecoder on every settle path, not just a clean 'end': an abrupt close would otherwise discard a multi-byte character the decoder was still holding. - Both liveness canaries assert the probe payload actually carries lifecycle counters, so a Harper error payload fails readably instead of as a TypeError on a missing field. - Condensed the file header and the fixture's per-route comments. Co-Authored-By: Claude Opus <noreply@anthropic.com>
The per-case cleanup checks dereferenced the polled snapshot directly, so a Harper error payload surfaced as a TypeError on a missing field rather than as the assertion that was actually failing. Co-Authored-By: Claude Opus <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new integration test suite (sse-finite-generator.test.ts) and its associated fixtures to verify that finite async generators streamed to completion over Server-Sent Events (SSE) close cleanly without hanging. Additionally, it refactors sse-throw-midstream.test.ts to extract and share common SSE streaming test helpers into a new utility file integrationTests/utils/sseStream.ts. The review feedback suggests adding defensive null/undefined checks to the awaitFixtureReady helper function in the new utility file to make it self-safeguarding.
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
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.
Adds
integrationTests/server/sse-finite-generator.test.ts, an integration anchor for the contract #1628 / PR #1632 established: a finite async generator streamed to completion overAccept: text/event-streammust close cleanly. The arms vary where the generator's terminal{ done: true }step falls relative to its yielded values — the axis the bug lived on — at N=0 (the terminal step is the first step produced), N=1, N=5 and N=3000, plus a rejecting contrast arm, a liveness canary and anhdb.loguncaughtExceptionsweep. Every completion arm goes throughassertCompletes, which requires a cleanend, the exact event sequence and no new uncaught throw.The three SSE suites in
integrationTests/server/had each grown their own copy of the AbortController-bounded SSE consumer, the/Probe/reader and the log helpers. Rather than add a fourth, they move tointegrationTests/utils/sseStream.tsandsse-throw-midstream.test.tsimports them — with one behaviour fix along the way:consumeSsenow decidesabortedfrom its own timer rather than from whichever event won the settle race, because an abort destroys the response and so a genuine hang also emitsclose; the raw-socket capture instream-error-contract.test.tsis a deliberately different technique (it inspects chunk framing) and stays where it is. Test-only — no product code changes.Neither neighbouring suite covers these completion shapes:
sse-throw-midstream.test.ts(#1789) anchors a generator that throws, and itsCleanGencontrol is the only completion case;stream-error-contract.test.tspins the stream-error contract across SSE, NDJSON and iterable-REST.For the human reviewer
awaitFixtureReadyfails setup whenhdb.logis unreadable, rather than reading it safely. EveryuncaughtExceptionassertion in both suites is a delta over a count, so a log this suite cannot read makes all of them pass vacuously — zero before, zero after. The helper therefore reads withreadFileSyncand failsbefore()if the file never appears. The alternative — thereadLogSafefallback used for the mid-suite reads — trades a loud setup failure for a silently toothless suite, which is the worse direction for a regression anchor. A reviewer round flagged the reverse risk (a config that never creates the log would hang 30s and then fail a suite that is actually fine); the evidence against it is thatsse-throw-midstream.test.tshas read that path unconditionally since #2094 landed, under the samelogging: { level: 'error' }fixture config. Reversible in one line if that assumption ever breaks.ThrowGenasserts a 1–2 event prefix, not exactly 2. The fixture yieldsn=0, n=1and then throws, and Fix SSE hang + uncaughtException when a generator throws mid-stream #1789'spipeline()teardown destroys the response rather than ending it, which can drop the last chunk before it drains. The lower bound of 1 still catches the regression that matters (no pre-error events delivered at all) and the sequence itself is compared exactly. Tightening to exactly 2 would pin a flush race.Left alone:
sse-throw-midstream.test.ts's1..3prefix range. Three review rounds flagged it —ThrowMidyields exactly 3 events, so a regression delivering only the first still passes. It is a deliberate pre-existing tolerance in that anchor with its own comment saying so, and tightening someone else's anchor is a call for its owner, not a side effect of this promotion. Cheap to change if you want it.Declined: SSE-spec multi-line
data:folding inparseEvents. The shared parser treats eachdata:line as its own event. That is wrong per spec for a multi-line payload, and right for every fixture in the tree —stream-error-contract.test.ts's owndecodedRecordssplits the same way. Adding the folding now is speculative generality; worth doing the day a fixture yields a multi-line payload.Declined: comment volume. Reviewers asked repeatedly to cut the file header, the
// ── N: ──section banners and the fixture's per-class notes. The header stays becauseintegrationTests/README.mdrequires one naming the issue; the banners and the surviving notes stay because both adjacent SSE anchors are written that way, and a lone divergent file reads worse than the nit costs. It was trimmed twice — the duplicated bug recap in the fixture and the restating route comments are gone.win32skip. Both suites keep it, matching every sibling inintegrationTests/server/; these anchors do not run on the Windows shards. Noted rather than changed — bringing SSE onto the Windows gate is its own piece of work.Declined (bot): defensive null guards on
awaitFixtureReady'sharperargument. Gemini asked forharper == nulland "neitherlogDirnordataRootDir" guards. The helper has two callers, both passingctx.harperon the line aftercreateApiClient(ctx.harper)would already have thrown on a null context, so the first guard is unreachable; the second would only improve the message on thejoincall if the framework renamed both fields, which is a loud failure either way. Six lines of unreachable defence in a test helper is the same bloat three review rounds asked me to cut. Say the word and it goes in.Verification
Route: new integration test, executed against the built
distatorigin/main(fb762a3).npm run build, thennpm run test:integration -- "integrationTests/server/sse-finite-generator.test.ts" "integrationTests/server/sse-throw-midstream.test.ts"— 10/10 pass, both suites, under the default concurrent isolation.npm run test:integration -- "integrationTests/server/sse-*.test.ts" "integrationTests/server/stream-error-contract.test.ts" "integrationTests/server/qa702-sse-event-data.test.ts"— 39/39 pass across all four SSE/streaming suites.npm run test:integration -- "integrationTests/server/*.test.ts"— 204 tests, 198 pass, 0 fail, 6 cancelled. The 6 areollama-backend.test.ts, which is pre-existing and unrelated: it reproduces identically on an unmodified checkout, failing at import withERR_IMPORT_ATTRIBUTE_MISSINGonjson/systemSchema.jsonwhenever a local Ollama happens to be reachable.integrationTests/**.npm run format:writeandnpm run lint:requiredclean.Fails-on-base is inherited rather than re-measured here: the QA-537 candidate this promotes was measured in both directions when it was written — 6/6 green with the fix, and against
4d9fe57e9(the commit before #1632 landed) the FiniteGen, EmptyGen, SingleGen and LargeGen arms each hang to the ~15s timeout while the throwing arm'suncaughtExceptioncount flips 0 → 1.Refs #1628
Complexity: easy
Review-Coverage: authored=claude; ran=codex,gemini; declined=cursor-grok,cursor-composer,domain; rounds=5 @ df25a30
Human-Review-Need: 3 @ df25a30