fix(bb.js): report a dead bb process as retryable, and replace it on request - #25548
Conversation
…request A pooled bb verifier whose process dies is handed out again on every later borrow, and the pool cannot tell, because the socket backend marks itself permanently unusable and every later call fails with a bare "Socket not connected". An owner has nothing to act on, so the node reports a dead helper process as an invalid transaction proof for the rest of its life. Two changes, matching what the AVM simulator pool already relies on. A failure caused by the environment now carries `retry: true`: the bb process died, its connection broke, or it could not be started. A missing or non-executable binary stays non-retryable, since retrying cannot fix it. The property is the whole contract, feature-detected rather than imported, which is how the ipc-runtime transports already express the same thing, so a caller written against one works unchanged against the other. The socket backend can also replace a dead process, on request. The bb process and its connection become one unit that is swapped as a whole, and the next call starts a replacement; calls that were already in flight still fail, retryably. Concurrent callers that find the connection down share one replacement, so a single death costs a single process, and a replacement that arrives after destroy() is not left running. It is off by default: a replacement has no Chonk accumulation and no batch-verifier session, so an owner holding that state keeps failing loudly rather than continuing silently against a fresh process. A pool of verifiers, where every call stands alone, is what it is for. Also fixes BarretenbergSync.initSingleton() caching a failed initialization for the life of the process, where the asynchronous singleton already cleared it. Verified against a real bb as well as the fake used by the tests: with the option off a killed bb gives a retryable error and keeps doing so; with it on the next call transparently returns the same hash from a fresh process. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NuUzj3qpkpJor6GMpWB4T4
fcarreiro
left a comment
fcarreiro
left a comment
There was a problem hiding this comment.
(Written by Claude on behalf of Facundo)
I read and reasoned through this; I didn't run it. The retry contract plus opt-in respawn is the right shape: the pool can stay as it is, which is simpler than an isAlive check. The three inline comments are worth addressing before merge: a failed replacement is non-retryable, a bb is left running after a connection break, and destroy() waits on a replacement. There are also two nits.
Other notes:
- The
BarretenbergSync.initSingleton()fix has no test. - Each killed bb leaves its
/tmp/bb-*.sockbehind, so withrespawnthese pile up. The replaced incarnation's path could be unlinked once its process is gone. - The description says this supersedes #25546, but #25546 also replaced a dead singleton. The node's
BarretenbergSyncsingleton, used for foundation crypto, runs on the shared-memory backend, whichrespawndoes not cover, so a dead one stays dead. That's outside A-2222, but worth stating in the description or leaving for a follow-up. - Node side:
bb-prover/src/prover/server/bb_prover.tsretries only onProvingError.retry, so aBackendUnavailableErrorfrom a proving call is not retried.
| } | ||
|
|
||
| try { | ||
| const socket = await this.waitForSocketAndConnect(socketPath, proc); |
There was a problem hiding this comment.
(Written by Claude on behalf of Facundo)
A replacement that fails to start rejects as non-retryable. Only the spawn error above is classified. The failures from waitForSocketAndConnect are plain Errors: "bb process exited before socket connection was established", the 60 s timeout, and "Failed to connect to bb socket". With respawn on, they reach the caller of call() through replace().
Scenario: bb is OOM-killed, and its replacement dies under the same memory pressure before it connects. The call rejects without retry, so a caller that classifies on retry reports an invalid proof again. The description says a bb that "could not be started" is retryable.
Suggest wrapping these as BackendUnavailableError, keeping ENOENT/EACCES plain. A respawn test with a fake bb that exits on its second start would cover it.
| } | ||
| this.death = new BackendUnavailableError(reason); | ||
| this.proc = null; | ||
| discard(incarnation); |
There was a problem hiding this comment.
(Written by Claude on behalf of Facundo)
This drops the process without killing it, and this.proc = null means destroy() can't kill it later either. After a socket error or end, bb can still be running: SocketServer::disconnect_client closes the client, and the server loop in ipc_server.hpp keeps serving. That bb then lives until the node exits.
On next, destroy() still killed this.process in that case. With respawn, each such event leaves an extra bb running alongside its replacement.
Suggest incarnation.proc.kill('SIGKILL') here. It is a no-op for a child that has already exited.
| this.destroyed = true; | ||
| this.failAllPending(new Error('Backend connection closed')); | ||
| // Don't leave a replacement that is still starting behind: it would outlive its owner. | ||
| await this.starting?.catch(() => {}); |
There was a problem hiding this comment.
(Written by Claude on behalf of Facundo)
This makes destroy() wait for a replacement that is still starting, for up to STARTUP_TIMEOUT_MS (60 s). That holds up node shutdown and the RPC verifier's stop(). As far as I can tell, replace() already kills a replacement that finishes after destroy() (the this.destroyed branch), so the await only adds delay.
Suggest dropping it. The test "does not leave a replacement running when destroyed while it starts" would then need to wait for the second pid to appear before checking that it's gone.
| throw new Error('Backend connection closed'); | ||
| } | ||
| if (!this.opts.respawn) { | ||
| throw this.death ?? new BackendUnavailableError('Socket not connected'); |
There was a problem hiding this comment.
(Written by Claude on behalf of Facundo)
Nit: without respawn, every later call rethrows the same death instance, so all callers share one stack and anything one of them attaches to it. new BackendUnavailableError(this.death.message, { cause: this.death }) avoids that.
| return Promise.reject(new Error('Socket not connected')); | ||
| } | ||
| async call(inputBuffer: Uint8Array): Promise<Uint8Array> { | ||
| await this.ensureConnected(); |
There was a problem hiding this comment.
(Written by Claude on behalf of Facundo)
Nit: if destroy() runs between ensureConnected() resolving and this continuation, this.socket!.ref() throws a TypeError instead of "Backend connection closed". Another caller awaiting the same starting can do that. Re-checking this.socket here, or having ensureConnected() return the socket, closes the gap.
…mpt destroy A replacement that failed to come up rejected as a plain Error, so the very case the option exists for — bb killed under memory pressure, its replacement dying under the same pressure — reached the caller without the flag and was reported as an invalid proof again. Everything the connect path raises is environmental and now says so; only a binary that cannot be executed stays non-retryable. Losing the connection does not mean the process is gone. bb's server keeps serving after a client disconnects, so a socket error or end left a bb running, and nulling the handle meant destroy() could not kill it later either. That was a regression: before this change destroy() still killed it. The process is now killed where the incarnation is retired, and its socket path removed, which a killed bb never gets to do itself. destroy() no longer waits for a replacement that is still starting. It could have blocked shutdown for as long as a bb can take to come up, which has no useful upper bound, and bought nothing: a replacement that lands afterwards already kills itself. Two smaller things from the same review. Each caller now gets its own error rather than sharing one instance and whatever another caller attached to it. And call() uses the socket ensureConnected() returned, so a destroy() landing in between gives the closed-backend error rather than a TypeError. The singleton fix now has a test, which fails without it. The respawn option's documentation says what it is safe for: an owner whose every call stands alone, never one that establishes a session, whose replacement would quietly run against a process that has forgotten everything. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NuUzj3qpkpJor6GMpWB4T4
The synchronous receive loop waits forever, and there is no deadline to tune, since a slow bb and a dead one look identical from there. So a bb dying mid-call wedges its caller rather than failing it, and because the call blocks the JS thread the child's exit is never even observed: it stays an unreaped zombie, which still answers kill(pid, 0), and nothing recovers. Not addressed, on purpose. Detecting it needs a liveness signal that survives the zombie window, which means plumbing an inherited descriptor through this client and bb, and the process on the other end of a synchronous client runs one thread doing hashes and signatures. It is the smallest thing on the machine and the least likely to be killed, so the fix costs more than the case is worth. Written down where it happens, and cross-referenced from the backend options, so the next audit finds the reasoning rather than the symptom. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NuUzj3qpkpJor6GMpWB4T4
Rolling integration PR for changes labelled `port-to-v6` on `next`. Each merged source PR is cherry-picked onto `cb/port-to-v6` as its own commit, keeping `(#N)` in the title. ## Ported - [x] [#25548](#25548) fix(bb.js): report a dead bb process as retryable, and replace it on request (clean pick) ## Notes - All eight files #25548 touches end up byte-identical to `next` at d5b4b8a, so the pick applied against the same pre-image on `v6`. - Not built or tested locally; bb.js build and `native_socket.test.ts` / `singleton.test.ts` run on this PR's CI. --- *Created by [claudebox](https://claudebox.work/v2/sessions/134efca9a11ee14f/jobs/4) · group: `slackbot` · [Slack thread](https://aztecprotocol.slack.com/archives/C0AGN2WT3CP/p1790609320989739?thread_ts=1790609320.989739&cid=C0AGN2WT3CP)* Co-authored-by: Charlie <5764343+charlielye@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Addresses https://github.com/AztecProtocol/barretenberg-claude/issues/4427 from the bb.js side. The aztec-node side is aztec-node#354.
A pooled bb verifier whose process dies is handed back to the pool and handed out again on every later borrow. The pool cannot tell, because the socket backend marks itself permanently unusable and every later call fails with a bare
Socket not connected. The node therefore reports a dead helper process as an invalid transaction proof, persistently, and books it into the metric that means users are submitting bad proofs.This gives an owner the two things it needs, in the shape the AVM simulator pool already relies on.
A failed call says whether retrying can help
An environmental failure now carries
retry: true: the bb process died, its connection broke, or it could not be started. Only a binary that cannot be executed stays non-retryable, because retrying cannot fix it.The bare property is the whole contract, feature-detected rather than imported:
That is deliberately the same convention
ipc-runtime's transports use, so a caller written against one works unchanged against the other. It is also what lets a verifier distinguish a bad proof from a dead helper, which is the misattribution half of the issue.The socket backend can replace a dead process, on request
respawnis a newBackendOptionsflag, off by default. With it on, the bb process and its connection are one unit swapped as a whole: the next call starts a replacement, while calls already in flight still fail retryably. Concurrent callers that find the connection down share one replacement, so a single death costs a single process, and a replacement that arrives afterdestroy()is not left running.It is opt-in because a replacement has none of the state a command sequence establishes: no SRS loaded over the connection, no Chonk accumulation, no batch-verifier session with its registered keys. An owner that runs any such sequence must leave it off, so a death fails loudly instead of the next call quietly running against a process that has forgotten everything. The verifier pool qualifies because every verification carries its proof and key in the call.
With this, a pool needs no liveness check and no maintenance loop: returning an instance unconditionally becomes correct, exactly as it already is for the AVM pool.
Also
BarretenbergSync.initSingleton()cached a failed initialization for the life of the process, so one bad spawn was permanent. It now clears the failure, as the asynchronous singleton already did.Testing
native_socket.test.ts, against the fake bb the existing tests use: a killed bb fails the call retryably and keeps failing without the option; with it on the next call is served by a replacement process, a different pid; four concurrent calls that find the connection down start exactly one replacement;destroy()during a replacement leaves no process running; a replacement that cannot start fails retryably too; a connection that breaks while the process keeps running leaves no bb behind.singleton.test.tscovers the initialization fix and fails without it.Checked against a real bb as well: with the option off a killed bb gives a retryable error and keeps doing so, and with it on the next call transparently returns the same hash from a fresh process.
What this does not cover
BarretenbergSyncruns on the shared-memory backend, which has no respawn and no way to report a death mid-call: the NAPI receive loop retries without a deadline, and because the call blocks the event loop the process exit is never even observed, so the caller wedges rather than fails. That path is untouched here and is deliberately left alone: the synchronous bb runs one thread doing hashes and signatures, so it is the least likely process on the machine to be killed, and the fix would mean threading a liveness check into the C++ client for a case that may never happen. #25546 covers the idle-death half of it by replacing a dead singleton, so the two PRs cover different backends rather than one superseding the other.Note on direction
bb.js's hand-written backends are replaced by
ipc-runtime's in the codegen migration (#25362). Nothing above is lost in that move:ipc-runtime's spawned backend already carries the sameretrycontract and the same opt-in respawn, so the callers written against this keep working and the implementation here is deleted. That is why this is expressed as the retry contract rather than as a liveness query, which would have to become part of the generated client's interface.