Skip to content

fix(peer): release client request state once a streamed response finishes - #157

Merged
dinwwwh merged 5 commits into
mainfrom
claude/sleepy-fermat-d65qs9
Oct 12, 2026
Merged

dinwwwh merged 5 commits into
mainfrom
claude/sleepy-fermat-d65qs9

Conversation

@dinwwwh

@dinwwwh dinwwwh commented Oct 12, 2026

Copy link
Copy Markdown
Member

Summary

This PR fixes a resource leak where streaming response bodies (event-stream and octet-stream) were not properly released when the stream completed, even if the body was never resolved by the client. This caused request state to persist indefinitely and event listeners to accumulate.

Key Changes

  • Client peer streaming message handling: Modified ClientPeer to automatically close requests when receiving final stream messages:

    • For event-stream: close on 'close' or 'error' events
    • For octet-stream: close when the close flag is set to true
  • Test coverage: Added comprehensive tests to verify:

    • Requests are released when streaming bodies complete without being resolved
    • Event listeners are properly cleaned up on stream completion
    • No cancel messages are sent when iterating over already-finished streams
    • Integration tests confirming both client and server peers release resources correctly
    • Proper cleanup of abort signal listeners even with 100 concurrent requests

Implementation Details

The fix ensures that the server's stream completion message (which indicates the server has already released its resources) triggers an immediate cleanup on the client side via closeById(id). This prevents the accumulation of request state and event listeners that would otherwise only be cleaned up when the body is eventually resolved or garbage collected.

https://claude.ai/code/session_01AEr5Gq656PLdpCLEAw2zht

…shes

ClientPeer kept a streamed response's request entry, its abort listener,
and every buffered stream message until the caller consumed or cancelled
the body. A caller that only read the status leaked all of it, even after
the server had finished the stream and released its own state.

Release the request as soon as the terminal stream message arrives (an
event-stream `close`/`error` event, or an octet-stream `close` chunk).
The queue is closed, not aborted, so a later resolveBody() still yields
every buffered item, and cancelling that body sends no cancel for a
stream the server already finished.

A stream that never finishes still needs the caller to cancel the body
or abort the signal; document that in the README and on
ClientPeer.request.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AEr5Gq656PLdpCLEAw2zht
- drop the late-abort and exact-listener checks from the new unit tests;
  the existing abort-listener tests already cover them
- cancel a single endless stream in the integration test instead of ten
- shorten the ClientPeer.request JSDoc so it no longer repeats the README,
  and stop claiming the server always stops streaming on cancel (a
  hibernated stream is no longer tracked by ServerPeer)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AEr5Gq656PLdpCLEAw2zht
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AEr5Gq656PLdpCLEAw2zht
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AEr5Gq656PLdpCLEAw2zht
@dinwwwh dinwwwh changed the title Release streaming request resources when stream completes fix(peer): release client request state once a streamed response finishes Oct 12, 2026
@pkg-pr-new

pkg-pr-new Bot commented Oct 12, 2026

Copy link
Copy Markdown
@standard-server/aws-lambda

npm i https://pkg.pr.new/@standard-server/aws-lambda@157

@standard-server/core

npm i https://pkg.pr.new/@standard-server/core@157

@standard-server/fastify

npm i https://pkg.pr.new/@standard-server/fastify@157

@standard-server/fetch

npm i https://pkg.pr.new/@standard-server/fetch@157

@standard-server/node

npm i https://pkg.pr.new/@standard-server/node@157

@standard-server/peer

npm i https://pkg.pr.new/@standard-server/peer@157

@standard-server/shared

npm i https://pkg.pr.new/@standard-server/shared@157

commit: 81661f4

@codecov

codecov Bot commented Oct 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@codspeed

codspeed Bot commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

⚠️ 4 benchmarks spent significant time in system calls

System calls cannot be consistently instrumented, so they are not included in the measure, which understates the real cost. Please switch to the Walltime instrument to accurately measure system calls.

Measurement and system calls

✅ 26 untouched benchmarks
⏩ 108 skipped benchmarks1


Comparing claude/sleepy-fermat-d65qs9 (81661f4) with main (6c1cc3b)

Open in CodSpeed

Footnotes

  1. 108 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

This run reviewed the full PR (all 5 commits) against main.

  • Eager release on terminal stream message (packages/peer/src/client.ts) — ClientPeer.message() now calls closeById(id) when an event-stream message is 'close'/'error' or an octet-stream message has close: true, instead of waiting for the consumer to resolve/consume the body.
  • Unit coverage (packages/peer/src/client.test.ts) — release without resolving the body for event-stream close and error, no cancel sent when return()/reader.cancel() runs on an already-finished stream, and abort-listener removal asserted via a spy.
  • Integration coverage (packages/peer/tests/peer.test.ts) — 100 concurrent streamed responses left unresolved drain both peers' requests to 0 with no residual abort listeners, plus an endless stream cancelled without reading finalizes the server generator.

The ordering is sound: the terminal message is pushed before closeById, so Queue.close keeps it buffered and pullable (only pending pulls are rejected), and closeById is idempotent against the later AsyncIteratorClass/toOctetStream cleanup. Peer suite (273 tests) passes locally.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@dinwwwh
dinwwwh merged commit 835b30f into main Oct 12, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants