Repository navigation
sdk/js: a join that hangs says where, and stops what it started - #739
Conversation
… stage The SDK probe's JS programs hung silently for 120s in-cluster, and nothing said which await they were in (google#732). Each step of a join already has a timeout of its own, but a join takes them in turn over every router it is offered, after up to four control plane requests of 30s each, so the whole of it had no bound. JoinOptions .signal, documented as bounding the whole join, reached only the router dials. It now bounds the control plane sync, and the dial, auth handshake and relay reservation with every router; without one the join gets JOIN_TIMEOUT_MS (60s). A join that stops names each router it tried and the step it was in, or the control plane it was waiting on. A directory request no router answered names every router and what each said, not only the last error. call.js, a2a-call.js and a2a-agent.js print each stage to stderr as they reach it (enrolling, enrolled, joined via, provider, dialing), so a run that hangs shows the last one in the probe's own output. stdout is unchanged; the docs that embed the examples are regenerated. Refs google#732
There was a problem hiding this comment.
Code Review
This pull request improves error reporting and timeout handling during the join and discovery processes in the JavaScript SDK. It ensures that when a join or directory request fails or is aborted, the error message lists all tried routers and the specific step (such as dial, auth handshake, or relay reservation) where the failure or hang occurred. It also introduces a default 60-second join timeout when no abort signal is provided. The review feedback suggests two improvements: directly throwing the abort reason (signal.reason) when a directory request is aborted to align with standard practices, and checking the abort signal at the start of the router dial loop to prevent unnecessary network connections once the join process has been cancelled.
| if (signal?.aborted === true) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
When the signal is aborted, the function should reject with the abort reason (signal.reason) rather than breaking the loop and throwing a custom DirectoryError. Propagating the abort reason directly is standard practice in JavaScript/TypeScript and allows callers to correctly distinguish between a user-initiated cancellation/timeout and a routing/network failure.
| if (signal?.aborted === true) { | |
| break; | |
| } | |
| if (signal?.aborted === true) { | |
| throw signal.reason; | |
| } |
There was a problem hiding this comment.
I'd like to leave this one as it is, if that's OK. The break on an aborted signal was already there before this PR; all I changed here is the message, which now lists every router asked and what each said. That's what #732 asks for, and throwing signal.reason instead would lose those router names.
It would also change what request() rejects with on a timeout, and with it discover(), findPeer() and announce(). DirectoryError is exported from the package, and the Python SDK raises the same DirectoryError("no held router answered: ..."), so right now callers in both languages get one error type. If the maintainers would rather have the raw abort reason, I'm happy to do it, but I think it'd be better as its own change that covers both SDKs.
| for (const cand of picks) { | ||
| tried.add(cand.peerId); |
There was a problem hiding this comment.
To avoid unnecessary network dials and CPU cycles when the join process has already been aborted, check if the signal is aborted at the start of the for loop before attempting to dial the next candidate router.
| for (const cand of picks) { | |
| tried.add(cand.peerId); | |
| for (const cand of picks) { | |
| if (signal?.aborted === true) { | |
| return failures; | |
| } | |
| tried.add(cand.peerId); |
There was a problem hiding this comment.
Good catch, fixed in e39dfb2. It was actually a bit worse than extra dials. If the signal aborted while one router was being admitted, the loop moved on to the next router and dialed it with a signal that had already aborted. That dial failed straight away, so the router got shunned and showed up in the join's error as a failed dial, even though it was never really tried.
The signal is now checked before each router as well as after a failure. Both checks go through a small aborted() helper. Otherwise TypeScript narrows the first check and complains that the one in the catch can never be true, even though the signal can abort during the awaits in between.
I added a test for it: after a join, the control plane lists a second router, and a top-up with an already-aborted signal comes back with no failures and shuns nothing. The same top-up without the signal then picks up that router.
topUpRouters checked its signal only after a router failed. A signal that aborted while one router was being admitted sent the next one to a dial that failed at once, so the router was shunned and listed in the join's error without having been tried. The signal is checked before each router as well.
aojea
left a comment
There was a problem hiding this comment.
Please revert the console.error changes in sdk/js/examples/ and the generated documentation in sdk/js/README.md and site/content/docs/guides/native-sdks.md. Those example files are embedded directly into the user documentation, so adding probe debug logs makes the guides noisy and inconsistent with the Python examples. Also, logging the raw controlPlaneUrl before validation or formatting mesh.controlPlane.url into errors can leak credentials or query parameters if present in the URL. Once join() has a 60s timeout and throws an error with the router and step name, Node prints the unhandled error to stderr and exits before the 120s probe timeout kills it, so the probe output already shows where it failed.
For cancellation, please pass AbortSignal directly to ControlPlaneClient and authenticateWithPeer with AbortSignal.any([signal, AbortSignal.timeout(...)]) (like discovery.ts does) instead of using untilAborted and joinDeadline. untilAborted does not cancel the underlying promise, so an aborted syncControlPlane() keeps running in the background after join() fails. That background run can still send a new /info request, call refresh(), and write to credential.json in stateDir while keeping the 30s request timer alive on the event loop.
Also, ControlPlaneClient uses a 30s timeout per request and syncControlPlane() runs /keys and /info one after another. If the control plane is unreachable when a member joins with a stored credential, syncControlPlane() takes the full 60s JOIN_TIMEOUT_MS and fails join() before dialing any router. The pre-join sync is best-effort and should use a shorter timeout so that join() still falls back to the stored credential unless the caller aborted options.signal.
Finally, in joinUnder, checking signal.aborted || session.routers.length === 0 fails the whole join if the first router admits the member and the second router times out at 60s. If session.routers.length > 0, join() should succeed unless the caller aborted options.signal. And in topUpRouters, the catch block calls this.#shunRouter(cand.peerId, ...) before checking aborted(), which shuns the in-flight router for 5 minutes when the signal aborts.
From review on google#739. The examples go back to what they were: they are embedded in the user guides, and printing the control plane URL could leak credentials or query parameters in it. A join that stops already says where in the error Node prints, so the probe's output shows it without them. The error no longer names the control plane URL either. untilAborted only stopped waiting: a control plane sync the join gave up on went on in the background, could still call /info and refresh(), and write credential.json. The signal now goes into the requests themselves. ControlPlaneClient.info() and keys() and authenticateWithPeer take one and combine it with their own timeout, as discovery does, and syncControlPlane stops before its next request and before saving once it aborts. A /refresh already sent is left to finish, since the control plane spends the old biscuit when it answers. The sync before a join is best effort, so it gets JOIN_SYNC_TIMEOUT_MS (5s) rather than the whole join: an unreachable control plane leaves the join to go on with the stored credential. A join that holds a router succeeds even if its own timeout fires while a second router is still being tried; only the caller's signal turns that into a failure. A router cut off by the signal is no longer shunned. Refs google#732
|
@aojea thanks for the review. Everything is in 349983a:
I added tests for the sync fallback, for the partial join, and for "nothing is sent after the abort". I also checked that each one fails when its fix is reverted. I've updated the PR description to match. |
|
Thanks |
Part of #732, the "instrumentation to add regardless of the cause" piece. This doesn't fix the in-cluster hang itself, but the next time it happens, the output should show where it got stuck.
Why
Every step of a JS join already has its own timeout: the libp2p dial (10s), the auth handshake (10s), the relay reservation (5s), a directory request (10s), and each control plane request (30s). But
joinMeshruns them one after another.syncControlPlane()can make up to four control plane requests before a single router is dialed, and thentopUpRouterstries the candidate routers one by one. So there was no limit on the join as a whole.JoinOptions.signalsays it "Bounds the whole join", but it only reached the router dials.What changed
joinMeshruns under the caller'ssignal, or underAbortSignal.timeout(JOIN_TIMEOUT_MS)(60s, exported) if there isn't one. The signal is passed into the work itself, so the work actually stops:ControlPlaneClient.info()andkeys()andauthenticateWithPeertake an optional signal and combine it with their own timeout viaAbortSignal.any, the same waydiscovery.tsdoes.syncControlPlane()sends no further request and writes nothing to the state directory. A/refreshthat has already been sent is left to finish: the control plane spends the old biscuit when it answers, so cutting it off could lose the new one.JOIN_SYNC_TIMEOUT_MS(5s) instead of the whole join. If the control plane is unreachable, the join goes on with the stored credential, unless the caller abortedoptions.signal.topUpRouterschecks the signal before each router and before shunning, so it never shuns a router it hasn't dialed or one it was in the middle of trying.<router>: auth handshake: ...), or saysjoin stopped while syncing with the control plane. Router failures that aren't timeouts include the step too (dial <addrs>,auth handshake,relay reservation). The error doesn't include the control plane URL, which could carry credentials or query parameters.request()indiscovery.tsreports each router it asked and what it said, not just the last error.The examples and the docs that embed them are unchanged. When a join fails, Node prints the error to stderr and exits well before the probe's 120s timeout, so the probe's output already shows where it stopped.
Behaviour changes
Testing
session.test.ts:auth handshake;/keysand/infonever answer, with a caller signal: the join ends at the signal, the error doesn't contain the URL, and nothing goes to the control plane after the aborted/keys(no/info, no refresh);syncControlPlane. The matching test fails each time.discovery.test.ts: two routers that both fail, and the error names both along with what each said.npm test: all 139 pass. The join tests passed 15 runs in a row pinned to 2 CPUs (taskset -c 0-1).npm run build,npm run examples,npm run typecheckandgo run ./hack/gen-sdk-docs -check sdk site/content/docsall pass.