fix(backend): cancel demo-run polling when client disconnects (#531) - #532
fix(backend): cancel demo-run polling when client disconnects (#531)#532RIOS-JORGE wants to merge 5 commits into
Conversation
Move the disconnect listener to res.on('close') guarded by
!res.writableEnded so it stays active for the whole handler, and
pass the AbortSignal into waitForActivityTxHash() so the poll loop
breaks early when the client disconnects mid-request.
req.on('close') fires when the request body is fully consumed, not
on disconnect (verified on Node 22/24), so keeping it registered
would abort every normal request.
…ection
Adds code comments documenting the empirical finding: req.on('close')
fires when the request body is fully consumed (not on client disconnect)
in Node >=22, so keeping it registered would abort every normal request.
res.on('close') + !writableEnded distinguishes real disconnects from
normal completion. Verified on Node 22.23.1 (CI) and 24.15.0.
…lling New standalone file (backend/test/demo-disconnect.test.js) that covers issue Stellar-Ecosystem#531's real scenario: client disconnect DURING the polling phase. The existing suite cannot see this bug because waitForActivityTxHash is mocked to resolve instantly, so the handler completes before the 'close' event fires. This suite runs the real poll with a real HTTP socket: - unit: abort signal in waitForActivityTxHash (already-aborted, mid-wait) - integration: normal request completes the full poll; disconnect mid-request cancels the poll early (no full 8s budget waste) Lives separately so the maintainer can adopt or discard it freely.
|
Warning Review limit reached
Next review available in: 47 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change propagates response-close cancellation from ChangesDemo request cancellation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant DemoRoute
participant waitForActivityTxHash
Client->>DemoRoute: submit demo request
DemoRoute->>waitForActivityTxHash: poll with AbortSignal
Client-->>DemoRoute: close response
DemoRoute->>DemoRoute: abort unfinished request
DemoRoute->>waitForActivityTxHash: observe aborted signal
waitForActivityTxHash-->>DemoRoute: return empty string
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/lib/waitForActivityTxHash.js`:
- Line 10: Update the polling loop in waitForActivityTxHash to make each
in-flight sleep abortable, racing the delay used by sleep against signal before
continuing. Ensure an already-aborted signal and an abort occurring during the
delay both wake the loop immediately, preserve the existing empty-string return
behavior, and avoid leaving abort listeners or timers behind after either race
completes.
In `@backend/src/routes/demo.js`:
- Around line 118-146: Update the completion flow around waitForActivityTxHash
and the final res.json call to handle an aborted request before writing the
response. Keep recordActivity unconditional, but if abortController.signal is
aborted after polling, avoid the normal response write and return the existing
cancellation response behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6fac0491-6245-4f6b-9766-6a263d499e63
📒 Files selected for processing (3)
backend/src/lib/waitForActivityTxHash.jsbackend/src/routes/demo.jsbackend/test/demo-disconnect.test.js
The loop's sleep now honors the AbortSignal (timers cleaned up), breaks on abort, and demo-run no longer responds once the client has gone.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/lib/waitForActivityTxHash.test.js`:
- Around line 100-117: Update the test around waitForActivityTxHash to use fake
timers, assert exactly one pending timer before controller.abort(), and assert
no pending timers after resultPromise resolves. Keep the existing result and
getFeed assertions while making the test verify prompt abort handling without
relying on real-time delays.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a919b10a-8d90-4cf0-824f-09052004f449
📒 Files selected for processing (3)
backend/src/lib/waitForActivityTxHash.jsbackend/src/lib/waitForActivityTxHash.test.jsbackend/src/routes/demo.js
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/routes/demo.js
Use fake timers to assert one timer is active before the abort and zero timers after it resolves, so a regression to a non-abortable sleep fails instead of passing by waiting longer.
|
"Hi, I’ve submitted the PR and it was approved by Coderabbit. Whenever you have a chance, please review it and, if everything looks good, proceed with the merge. Thanks!" |
1 participant
Closes #531
Summary
res.on('close')and!res.writableEndedwithout aborting normal requests on Node.js >=22.AbortSignalthroughwaitForActivityTxHash()so polling stops when the client disconnects.Changes
backend/src/routes/demo.jsbackend/src/lib/waitForActivityTxHash.jsbackend/test/demo-disconnect.test.jsVerification
backend/package.jsononmain; that unrelated file is intentionally not included in this PR.Notes
The implementation deliberately does not use a long-lived
req.on('close')listener: on Node.js >=22, that event can fire when the request body is consumed during a normal request. Usingres.on('close')with!res.writableEndeddistinguishes an incomplete client connection from normal response completion.Summary by CodeRabbit
Bug Fixes
Tests