test(server,resources): pin streaming delivery + stream-error contracts - #2070
Open
kriszyp wants to merge 10 commits into
Open
test(server,resources): pin streaming delivery + stream-error contracts#2070kriszyp wants to merge 10 commits into
kriszyp wants to merge 10 commits into
Conversation
Promotes two exploratory-QA specs (QA-883 / QA-890) into permanent anchors for the subscription dispatch and streaming-error paths. Test-only. No product code changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Reviewed; no blockers found. |
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive integration tests and fixtures for subscription delivery completeness under load (QA-883) and stream error contracts across SSE, NDJSON, and iterable REST surfaces (QA-890). The review feedback focuses on enhancing the robustness of these new tests. Key recommendations include normalizing line endings in the SSE stream parser to prevent hangs on CRLF, adding safety limits to the event-settling loops to avoid infinite hangs, wrapping test execution in try...finally blocks to prevent MQTT connection leaks on failures, and using defensive error handling in the socket error logger to guard against throwing getters or revoked proxies.
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
kriszyp
marked this pull request as ready for review
August 5, 2026 00:47
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.
Promotes two exploratory-QA specs into permanent regression anchors for the streaming paths. Test-only — no product code changes.
integrationTests/resources/subscription-delivery-completeness.test.ts(QA-883 / P-629)subscribe(), SSE and MQTT all deliver every transition (dropped === 0) atthreads=1andthreads=4; under burst, coalescing exists and the terminal value is always correct and monotonic-final.integrationTests/server/stream-error-contract.test.ts(QA-890 / P-630)fetch()cannot express.Why the raw-socket capture
fetch()collapses "the server never wrote a status line" and "the server returned 200 with an empty body" into the same observable. Both happen today on different server glues for the same resource, so the spec drives a manualnet.Socketand parses the chunked framing itself.What the pre-first-yield arms assert (and deliberately do not)
Today the three surfaces diverge on a generator that throws before its first
yield— Node emits zero bytes, uWS emits a clean 200, and iterable-REST returns 200 with an in-band{"error": …}element. That divergence is an open question (QA finding F-275 / F-277), and pinning the observed byte shape would freeze a bug into CI. So those arms assert only the invariant that holds regardless of how the contract is settled: the server terminates the request itself and never leaves the client hanging until its own timeout. The full observed shape is printed as a result table on stdout, so a change is visible in CI logs without turning the suite red.The control and mid-stream arms assert normal behaviour directly.
Coverage
surface=in-process|sse|mqtt | write-rate=slow|burst | threads=1|4surface=sse|ndjson|iterable-rest | server=node|uws | throw-point=pre-yield|mid-streamNothing in the tracked suite currently pins subscription delivery completeness (existing subscription tests cover revocation and protocol behaviour) or any stream-error contract, and nothing anywhere asserts on raw socket bytes.
Verification
Run in this branch's worktree on
main@6036bd8b2:stream-error-contract— 10/10 on the Node leg, 10/10 on the uWS leg (HARPER_UWS_HTTP=1)subscription-delivery-completeness— 5/5 (threads=1 and threads=4 suites)prettier --writeto confirm formatting was behaviour-neutral;prettier --checkandoxlint --deny-warningsboth clean, run from inside the repo tree.Both specs are
requires-isolation(distinct server config per leg / in-process subscriber at module load), so each stays its own file rather than joining a shared-instance suite.🤖 Generated with Claude Code