Give the upgrade socket an error listener before it can be reset - #40
Merged
Conversation
The http server drops its own socket error handler before emitting 'upgrade', so handleUpgrade receives a bare socket — and then awaited resolveTarget, which walks locks.run into wakeSandbox and can sit there for seconds on a cold wake, before attaching any listener of its own. A TCP reset inside that window is a listener-less 'error' event, which throws; nothing in the server installs an uncaughtException handler, so the daemon exits and every in-flight exec, restore and archive dies with it. handleRequest never had the problem: its ServerResponse keeps the server's own cleanup. Only a reset does this — a graceful FIN does not — and an HTTP reverse proxy absorbs a client's reset rather than passing it upstream, so with the daemon bound to loopback this is a local robustness defect, not a remotely reachable one. The realistic trigger is a local client aborting during a wake. The listener moves to the first statement, before any await. Its 'error' twin further down goes away rather than being kept in parallel: the new listener destroys the socket, and a destroyed socket always reaches 'close', which already runs the same settle() and upstream.destroy(). Verifying the fix against a real daemon turned up the same bug a second time, in the fake: its echo upstream self-pipes the upgrade socket with no error listener, so tearing the connection down after a client reset re-emits 'error' on a pipe destination with zero listeners. The real executor's upstream is a separate process and cannot take the daemon down that way, which made the stand-in deadlier than the thing it stands in for. Both tests are red without their own fix: one resets while the wake is held in the key slot, the other after the handshake, staggered, because a single well-timed close does not reproduce it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Closes #39.
handleUpgradereceived a bare socket from the http server (which drops its own error handler before emitting'upgrade') and then awaitedresolveTarget— seconds, on a cold wake — before attaching any listener. A TCP reset in that window is a listener-less'error', which throws; the server installs nouncaughtExceptionhandler, so the daemon exits and every in-flight exec/restore/archive dies with it.The listener moves to the first statement, before any await. The
'error'twin further down is deleted rather than kept alongside it: the new listener destroys the socket, and a destroyed socket always reaches'close', which already runs the samesettle()+upstream.destroy(). Net one listener, not two.Scope, stated plainly because it first looked worse than it is: only a reset triggers it (a graceful FIN does not), and an HTTP reverse proxy absorbs a client's reset rather than passing it upstream — measured with nginx. With the daemon bound to loopback this is a local robustness defect, not a remotely reachable one, and it hits none of SECURITY.md's three criteria. The realistic trigger is a local client aborting during a wake.
Verifying against a real daemon turned up the same bug a second time, in
FakeExecutor: its echo upstream self-pipes the upgrade socket with no error listener, so the pipe re-emits'error'on a destination with zero listeners when the connection is torn down. The real executor's upstream is a separate process and cannot do this, which made the fake deadlier than what it stands in for.Checklist
pnpm lintcleanpnpm typecheckclean (8 projects)pnpm testgreen — server 559 (was 557; +2), console 25, sdk 31, cli 59, e2e 86 pass / 11 skipDORMICE_SANDBOX_DOMAINset) survives 120 staggered upgrade-then-reset connections, still serves the proxy afterwards, zero crash traces; normal upgrade still completes 101 + echo. Temp dirs and sandbox cleaned up after.packages/serveris private and never published