Skip to content

A dead or erroring Web Worker leaves the client permanently wedged #315

Description

@WiktorStarczewski

Found while reviewing #313, which routes batch submission through the worker and so made this path matter more. Split out because the fix is not a drive-by: it changes shared client lifecycle for every consumer, and a partial version is worse than none.

The bug

WebClient registers only a "message" listener on its worker (crates/web-client/js/index.js, in the constructor). There is no "error" or "messageerror" handler anywhere in the file. So:

  • Worker script fails to load or parse. The spec fires error at the Worker and nothing else. this.ready never settles, so createClient() awaits forever. Given the SDK's worker does await import("./Cargo-*.js"), a bundler misconfiguration lands exactly here — and the symptom is a hang with no error, which is about the worst possible diagnostic.
  • Uncaught error in the worker. error fires; nothing settles the in-flight request. Because _serializeWasmCall chains on the pending promise, that one never-settling request blocks every later call behind it. One dead call takes the whole client with it.

Measured by driving the shipped js/index.js against a fake worker: the in-flight call never settles, and a subsequent waitForIdle() and an unrelated newAccount() hang forever too.

Why the obvious fix is not enough

Rejecting the entries in pendingRequests on error releases the current cohort and the chain slot — and then the very next call re-wedges the client. ready has already resolved, so rejecting it is a no-op; callMethodWithWorker sails past await this.ready, inserts a fresh id into the just-cleared map, and posts to the corpse. Measured: the second call hangs, and so does anything queued behind it.

A correct fix needs a terminal state, not just a flush:

  • latch a workerFailure on the instance and have callMethodWithWorker reject immediately when it is set, so later calls fail fast with a clear error instead of hanging. assertNotTerminated in client.js is the existing precedent for this shape.
  • decide what to do about late responses. An uncaught runtime error does not terminate a worker — per the HTML Standard the UA reports it and fires error, but the worker's event loop continues. So the request may still complete. Today a late success is silently dropped, and a late error response is not even logged (pendingRequests.has(requestId) is false, and the init-failure branch requires !requestId). Failing all callers on a non-fatal error can also convert a healthy batch into a reported failure.
  • event.message is absent for a load failure (a plain Event) and can be "", so ?? alone gives "worker failed — ". The two cases want different messages anyway.
  • consider preventDefault(): the worker error event is cancelable, and not cancelling it means the UA re-reports the exception on the parent global on top of whatever we log.
  • weigh the unhandled-rejection change. Rejecting promises nobody awaits turns a silent hang into an unhandledrejection, which is an improvement in most apps but new noise on a page with a strict reporter.
  • messageerror has limited browser support and this SDK deliberately routes Safari/WKWebView to a classic worker, so that half would not fire there.

Note on scope

Not caused by #313, and not batching-specific — it is the worker lifecycle for all ~40 forwarded methods. Worth fixing with a test: aliasing the ../Cargo.toml import to a stub makes js/index.js importable under vitest with a fake Worker, which is how the behaviour above was measured. That would also close the standing gap that js/index.js has no unit tests at all.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    Priority

    None yet

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions