Skip to content

fix(peer): release the request body when the signal is already aborted - #127

Merged
dinwwwh merged 1 commit into
mainfrom
claude/peer-client-abort-leak-39b5ce
Sep 29, 2026
Merged

dinwwwh merged 1 commit into
mainfrom
claude/peer-client-abort-leak-39b5ce

Conversation

@dinwwwh

@dinwwwh dinwwwh commented Sep 29, 2026

Copy link
Copy Markdown
Member

When ClientPeer.request() was called with a signal that was already aborted, the request rejected but its body was never cleaned up. A ReadableStream body was never cancelled, and an async iterator body never ran its return(), so any resources behind it stayed open. That body is now released with the abort reason, the same way as when the request ends before sending for any other reason.

Fixes

  • An already-aborted signal now cancels a ReadableStream request body with the abort reason, and returns an async iterator body.
  • The request still rejects with the signal's reason, nothing is sent, and no request state or abort listener is left behind.
  • A body that fails to clean up is still ignored, since the request has already settled.

Performance

  • No change to the success path. An aborted request is still rejected before its body is encoded.

Testing

  • Two new client tests, one for each body type, both fail against main.
  • vitest run packages/peer (254 passed), eslint, and tsc -b are clean.

ClientPeer.request() checked the signal before transmitRequest ran, so
an already-aborted signal rejected the request without cancelling a
ReadableStream body or returning an async iterator body. The check now
runs at the start of transmitRequest, so this case goes through the
same cleanup as every other exit before the body is sent.
@pkg-pr-new

pkg-pr-new Bot commented Sep 29, 2026

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

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

@standard-server/core

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

@standard-server/fastify

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

@standard-server/fetch

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

@standard-server/node

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

@standard-server/peer

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

@standard-server/shared

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

commit: 7666369

@codecov

codecov Bot commented Sep 29, 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 Sep 29, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 26 untouched benchmarks
⏩ 108 skipped benchmarks1


Comparing claude/peer-client-abort-leak-39b5ce (7666369) with main (28db1dd)

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

  • packages/peer/src/client.ts — move the abort check into the transmit path. request() no longer calls throwIfAborted(signal) synchronously; the check moves to the first statement of transmitRequest's try (before encodeAtomicStandardBody). A pre-aborted signal now flows through the existing catch → closeById rejection and the pre-existing finally → cancelStandardBody, releasing a ReadableStream body or returning an async-iterator body. This reuses the same cleanup path already exercised by abort-during-encode/send, peer close, and send failure.
  • packages/peer/src/client.test.ts — two regression cases. One per body type under signal / abort, asserting the body is released with the abort reason, nothing is sent, and no request state remains.

I traced the cleanup and race behavior: for a pre-aborted signal the abort listener never fires (addEventListener on an already-aborted signal is a no-op), closeById removes it and the state, and only transmitRequest's finally cancels the body, so there is no double cleanup. The check still sits before body encoding, so the stated "no change to the success path" holds.

I also confirmed the tests are discriminating: with base packages/peer/src/client.ts (28db1dd) restored, both new cases fail with expected "vi.fn()" to be called once, but got 0 times. Full peer client suite (55 tests), tsc -b, and eslint on both files are clean.

Pullfrog  | View workflow run | Using DeepSeek Flash (default — pick a model for stronger reviews) | 𝕏

@dinwwwh
dinwwwh merged commit e2cded4 into main Sep 29, 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.

1 participant