Long poll TSS sessions - #4243
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Long-poll listeners can leak or miss updates, storage failures can become unhandled rejections, and CLI retries can loop without backoff.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds broker-driven long polling to TSS retrieval endpoints to reduce polling latency and API traffic.
Changes:
- Adds TSS session notifications, long-poll waiting, and heartbeat responses.
- Extends broker typing/testing and updates TSS integration tests.
- Adjusts CLI retry handling and fixes ancillary documentation/error naming.
File summaries
| File | Description |
|---|---|
packages/bitcore-wallet-service/test/messagebroker.test.ts |
Adds broker lifecycle tests. |
packages/bitcore-wallet-service/test/integration/server.test.ts |
Uses configured TSS version. |
packages/bitcore-wallet-service/src/lib/tss.ts |
Implements event-driven TSS waits. |
packages/bitcore-wallet-service/src/lib/storage.ts |
Refines type-only imports. |
packages/bitcore-wallet-service/src/lib/server.ts |
Exposes broker and updates notifications. |
packages/bitcore-wallet-service/src/lib/routes/tss.ts |
Adds long-poll HTTP responses. |
packages/bitcore-wallet-service/src/lib/routes/helpers/error.ts |
Ends streamed responses on errors. |
packages/bitcore-wallet-service/src/lib/model/notification.ts |
Supports string notification IDs. |
packages/bitcore-wallet-service/src/lib/messagebroker.ts |
Types messages and adds unsubscribe. |
packages/bitcore-wallet-service/README.md |
Updates installation guidance. |
packages/bitcore-wallet-client/test/tss.test.ts |
Shortens waits in TSS tests. |
packages/bitcore-wallet-client/src/lib/payproV2.ts |
Corrects an error identifier. |
packages/bitcore-wallet-client/src/lib/errors/spec.ts |
Renames the transaction error. |
packages/bitcore-cli/src/tss.ts |
Retries transient signing failures. |
Review details
Suppressed comments (3)
packages/bitcore-wallet-service/src/lib/tss.ts:558
- This duplicated signing path has the same listener leak as the helper above: timeout or an incomplete update settles the request without unsubscribing
handler. Each poll then leaves another broker listener and future signing messages trigger redundant storage fetches. Remove the handler infinallyon every exit.
session = await Promise.race([
new Promise<TssSigGenModel>(r => events.once('session', r)),
new Promise<TssSigGenModel>(r => timer = setTimeout(() => r(session), maxWaitTime))
]);
clearTimeout(timer);
packages/bitcore-wallet-service/src/lib/tss.ts:545
- A rejection from this async storage fetch is not handled by
EventEmitter; it becomes an unhandled promise rejection and never rejects the pending request. Propagate the failure through the long-poll promise and clean up the broker listener instead.
const _session = await storage.fetchTssSigSession({ id: session.id });
packages/bitcore-wallet-service/src/lib/tss.ts:552
- The signing endpoint has the same lost-wakeup window: the route passes a previously fetched incomplete session, and a completing message can be stored and published before this listener is attached. With no subsequent event, the request waits until timeout and returns stale data. Register the listener and then immediately re-fetch/check the session before beginning the timed wait.
messageBroker.onMessage(handler);
- Files reviewed: 14/14 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…g class; unsubscribe to msg broker on timeout
There was a problem hiding this comment.
🟡 Changes recommended
The retry loop can restart a successfully created signing session, and the long-poll implementation has stale-state and resource-retention issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
packages/bitcore-cli/src/tss.ts:79
- After one connection failure sets
isTransientErrortotrue, a later successfulstart()does not reset it, so thedo/whileimmediately starts the same session again. That retry can hitTSS_ROUND_ALREADY_DONEand incorrectly report an interrupted session; exit the loop once the session has been stored.
packages/bitcore-wallet-client/src/lib/errors/spec.ts:150 - Renaming this generated public error removes
Errors.NO_TRASACTIONand changes the error's exported name for existing consumers. Keep a deprecated compatibility alias while introducingNO_TRANSACTION, then remove it only in a documented breaking release.
- Files reviewed: 14/14 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Broker lifecycle, compressed heartbeats, unbounded retries, and public API compatibility need correction.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
packages/bitcore-wallet-service/src/lib/routes/tss.ts:137
- The app installs
compression()globally (routes/setup.ts:13), so this one-byte write can remain buffered in the compression stream instead of reaching the client. That defeats the heartbeat for gzip-capable clients and can let idle timeouts terminate the long poll. Flush the compression stream after each heartbeat (or disable compression for this response).
interval = setInterval(() => res.write('\n'), 1000);
packages/bitcore-cli/src/tss.ts:93
- A connection refusal can make
start()reject immediately, and this branch retries forever with no delay. While BWS is unavailable the CLI will repeatedly rebuild the signing state and issue requests in a tight loop, consuming CPU and hammering the endpoint. Add a bounded/exponential backoff (and preferably a retry or cancellation bound) before the next iteration.
} else if (err instanceof Errors.CONNECTION_ERROR) {
isTransientError = true;
connErrs = connResilience(connErrs, err);
- Files reviewed: 17/17 changed files
- Comments generated: 3
- Review effort level: Balanced
| // Flush ensures the heartbeat is sent immediately to keep the connection alive. | ||
| res.writeHead(200, { 'Content-Type': 'application/json' }); | ||
| interval = setInterval(() => { res.write('\n'); res.flush(); }, 1000); | ||
| req.on('close', () => clearInterval(interval)); |
There was a problem hiding this comment.
This is to clean up the heartbeat when the client disconnects?
For Node 22, req's close is emitted when the request completes. res.close is emitted when the response is completed or its underlying connection was terminated prematurely.
Consider res.once('close',...).
Same thing on '/v1/tss/sign/:id/:round'.
https://nodejs.org/docs/latest-v22.x/api/http.html#event-close_3
https://nodejs.org/docs/latest-v22.x/api/http.html#event-close_2
Description
Add a long poll for the TSS session get endpoints to reduce both latency and the number of API calls
Changelog
Testing Notes
Checklist