Repository navigation
fix(remote-transport): apply backpressure instead of aborting at the queued-bytes cap - #21979
Conversation
…queued-bytes cap RemoteSocketStream.sendData threw when bufferedAmount plus the chunk exceeded the queued-bytes cap, so a briefly congested socket aborted the whole stream. Return sentBytes 0 and canSendMore false instead so the base stream keeps the chunk and waits for a drain, poll bufferedAmount until it falls to half the cap, then dispatch drain. Abort if the socket stays congested for longer than the idle timeout, and clear the timer when the stream closes. A socket that is not open still throws. Refs CherryHQ#21959 Signed-off-by: drakeo338 <[email protected]>
![cherry-pr-review[bot]](https://avatars.githubusercontent.com/in/5310206?s=60&v=4){kind=small}
There was a problem hiding this comment.
One release-contract issue requires changes: add a patch Changeset for @cherrystudio/remote-transport so the published fix receives a version bump. No runtime defect was confirmed in the backpressure implementation.
At the frozen head, package, main, renderer, platform, general and render tests passed. basic-checks failed at Changesets and planned-check verification; type and lint checks passed. Review used source and the exact pinned libp2p implementation. Scoped tests and a congestion experiment were reported by an independent reviewer; the Lead did not rerun runtime tests. Physical mobile-device behavior remains unverified.
Cherry Review · Blocked
1 blocking
The following 1 finding could not be placed as an inline comment and is delivered in the summary instead. Each entry begins with its location.
?Must fix: Please add a patch Changeset for@cherrystudio/remote-transport. This PR changes the published transport's congestion behavior but includes no release entry. The package is not ignored by Changesets, its README specifies Changesets publication, and the frozen head CI's “Check changesets” step fails. Without the entry, this fix does not queue a package version bump for published consumers.inv_1423eb75972b38e1#f1
…re fix Signed-off-by: drakeo338 <[email protected]>
|
Added a patch changeset for |
There was a problem hiding this comment.
No confirmed technical defects at ab38181f8cbd34afce18f8239b457fcd1103a11b. The pinned libp2p queue contract preserves unsent bytes and resumes on drain; congestion polling and cleanup match the intended behavior. The patch Changeset resolves the earlier release-entry finding.
Independent verification passed 18 transport tests and 19 gateway consumer tests. A controlled loopback experiment preserved eight 1 MiB encrypted records across repeated congestion/recovery cycles; the frozen base aborted on the same trigger. The Lead inspected this evidence and independently checked the pinned queue implementation, without rerunning runtime tests.
At the 2026-10-19 08:54:30 UTC head snapshot, basic checks and all recorded test jobs succeeded. Legacy status had zero contexts; synthetic-merge CI was excluded for mismatched parents. Physical mobile devices and real slow networks remain unverified. No downstream PR is required for this assessment.
Cherry Review · Passed
no findings
A newer review conclusion is available, so this REQUEST_CHANGES review was dismissed
|
Verified with real sockets: the transport now waits for the queue to drain instead of killing the session. I could not reproduce this through the app Remote Access session, so this is not an approval. What the change does: when the outgoing socket already has ~4 MiB queued, sending used to throw and the session died. Now it reports that it cannot send more, waits for the queue to drain, and only aborts if the queue stays full past the idle timeout. What I ran: two real WebSocket endpoints on localhost. The sending side used the changed code; the receiving side connected and then stopped reading, so the socket queue filled up for real (no fake queue counters). I pushed 64 KiB frames until the queue reached the cap, then let the receiver read again.
Not covered:
No conflicts. Merge is blocked on review, not on this branch. If the branch moves, the numbers above are for ab38181 and would need a re-run. |
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @cherrystudio/[email protected] ### Patch Changes - [#21979](#21979) [`332717f`](332717f) Thanks [@drakeo338](https://github.com/drakeo338)! - Apply backpressure when a remote socket's queued bytes reach the cap instead of aborting the stream; the stream waits for the buffer to drain and aborts only if it stays congested past the idle timeout. Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @cherrystudio/[email protected] ### Patch Changes - [#22031](#22031) [`0041d52`](0041d52) Thanks [@cherry-coding](https://github.com/apps/cherry-coding)! - DOCX preview now honours the OOXML default lineRule (auto) when w:spacing omits it. ## @cherrystudio/[email protected] ### Patch Changes - [#21979](#21979) [`332717f`](332717f) Thanks [@drakeo338](https://github.com/drakeo338)! - Apply backpressure when a remote socket's queued bytes reach the cap instead of aborting the stream; the stream waits for the buffer to drain and aborts only if it stays congested past the idle timeout. Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…queued-bytes cap (CherryHQ#21979) (cherry-lite) adapted uploadTransferLimits → local remoteLimits export (cherry picked from commit 332717f)
What this PR does
Before this PR:
sendDatathrew past the 4 MiBbufferedAmountcap, so a slow mobile client aborted the session, and reconnecting hit the same abort.After this PR: it reports
canSendMore: false, waits for the buffer to halve, then signals drain. It aborts only after the idle timeout.Refs #21959 (congestion abort only).
Why we need it and why it was done in this way
The following tradeoffs were made: polling
bufferedAmount, since the WebSocket has no drain event.The following alternatives were considered: raising the cap, which only delays the abort.
Agent Note / Spec
Agent Note: N/A — local bug fix.
Spec PR: N/A
Acceptance criteria: N/A
Verification
packages/remote-transport/tests/socket.test.ts4 passed here; the new tests fail (2 of 4) on the base commit.Breaking changes
None.
Special notes for your reviewer
AI-assisted (Claude Code), reviewed and tested by me.
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
main/gh-pr-review,gh pr diff, or GitHub UI) before requesting review from othersRelease note