Repository navigation
fix: publish Unix socket endpoints by link so a runtime close never removes a replacement - #109
Conversation
…emoves a replacement From Bun 1.3.14, closing a node:net Unix-socket server unlinks its bound path by name, whatever that path holds by then, and Bun also replaces an existing file at the bound path on listen. The MCP socket backend, the native HTTPS owner and the project HTTPS owner challenge bound their public endpoint directly, so a close could remove a foreign replacement their identity-checked cleanup was written to preserve. listenPublishedUnixSocket binds a fresh owner-only staging name in the same private directory (never longer than the endpoint's own name), then publishes it with link(2), which refuses an existing endpoint instead of replacing it, verifies the published inode and removes the staging name. A runtime close-time unlink then finds nothing, and the endpoint is removed only by each caller's identity-checked cleanup. The endpoint path, mode and recorded identity are unchanged. Controls cover normal and forced close, replacement, an existing endpoint, concurrent owners, restart and failed startup on Bun 1.3.9 and 1.4.2.
Bun 1.4, like Node, stores the string "undefined" when a process.env key is assigned undefined. Tests that unset variables that way, or restored a captured value that was never set, left "undefined" behind, and it leaked into later files in the same bun test process. Literal unsets now use Reflect.deleteProperty, and restores of captured values go through a shared restoreEnv helper that deletes a missing value.
|
Focused review of c86576b found one startup ownership gap in
Please retain the returned identity immediately, use it for exact failure cleanup, and compare every subsequent path observation against that original identity before claiming readiness. The helper already establishes private mode before publication; preserve a replacement rather than changing its permissions or adopting it. Add regression controls for a failure immediately after publication and a replacement before the first ownership observation. Assert listener closure, exact owned-path cleanup, preservation of the replacement, and successful subsequent startup. Existing release-time replacement tests do not exercise these startup windows. All eight CI checks pass, but this gap should be fixed before merging the compatibility prerequisite. The generic helper also starts its cleanup |
A review of #109 found two startup ownership gaps. The native HTTPS owner discarded the identity listenPublishedUnixSocket returned and only recorded one after lstat, chmod and lstat of the path. A failure after publication then skipped listener and endpoint cleanup, and a replacement in that window could be chmodded or recorded as the endpoint. The owner now keeps the published identity at once, compares its first observation against it, and never changes the endpoint by path. Failure cleanup always closes the listener, which can no longer unlink anything but the retired staging name, and then removes the endpoint only while it is this listener's inode; a replacement is kept. The helper awaited listen() outside its cleanup, so a listen that bound the staging name and then failed could leak that socket and listener. Listening is now inside the cleanup, and a failed start removes only a staging socket this user created. The helper also sets the endpoint mode (0600) on the staging inode before publication, so the MCP backend, the HTTPS owner and the owner challenge no longer chmod the public path. Controls: bind-then-fail, mode-preparation failure, an endpoint at the AF_UNIX path limit (and one byte over: refused cleanly on Bun 1.3.9, bound in full on 1.4.2, never truncated), and an owner fault or replacement between publication and first observation, on Bun 1.3.9 and 1.4.2.
Summary
Compatibility prerequisites for moving past Bun 1.3.9 (hack-dance/hack#108, HACK-1211). Both changes are correct on 1.3.9 as well, so this PR is safe to merge on its own; #108 stacks on it.
1. Unix-socket endpoints can no longer be removed by a runtime close (product)
From Bun 1.3.14, closing a
node:netUnix-socket server unlinks its bound path by name, whatever that path holds by then. Bun also replaces an existing file at the bound path onlisten. A standalone probe keeps a replacement file on 1.3.9 and loses it on 1.3.14 and 1.4.2.Three servers bound their public endpoint directly, so a close could remove a foreign replacement that their identity-checked cleanup exists to preserve:
src/mcp/socket-backend.ts:mcp.sock. Its testidle cleanup preserves replacement endpoint and claim identitiesfailed on 1.4.2.src/backends/native-https-owner-server.ts:control.sock. Its pre-close path check narrowed the window but did not close it, andretire()closed without a fresh check.src/backends/native-project-https.ts: the owner challengeowner.sock.stopOwnerChallengeclosed first and checked identity afterward.Fix: the new
listenPublishedUnixSocket(src/lib/unix-socket-publish.ts), the smallest supported change:link(2). This refuses an existing endpoint (EEXIST, surfaced asUnixSocketEndpointExists) instead of replacing it.A runtime close-time unlink then finds nothing, and each endpoint is removed only by its caller's existing identity-checked cleanup. On any failure after listening, the helper closes the server, removes the staging name, and removes the endpoint only while it still holds this socket.
Public endpoint contract: unchanged. The same path, a socket inode, mode
0600, and the same recorded device/inode (MCP receipt, ownerendpoint.json); clients connect exactly as before.Residual: a hidden staging name exists in the private directory only between
listenand publication. A crash inside that window could leave one such file behind. It is never bound again and blocks nothing.Per-caller changes are minimal:
chmod.chmodfailure) removes its own endpoint by identity instead of leaving it behind.2. Tests delete unset environment variables (test-only)
Bun 1.4, like Node, stores the string
"undefined"when aprocess.envkey is assignedundefined. Tests that unset that way, or restored a captured value that was never set, left"undefined"behind, and it leaked into later files in the samebun testprocess. That caused most of #108's 51 failures.Reflect.deleteProperty.tests/helpers/env.tsrestoreEnv, which deletes a missing value.config-paths.test.ts's local helper is fixed the same way.undefinedtoprocess.env.Verification
tests/unix-socket-publish.test.ts, 6 controls, passing on Bun 1.3.9 and 1.4.2:rename, and binding the endpoint directly (the previous design) each make controls fail. The previous design loses the replacement on 1.4.2, and replaces an existing endpoint atlisteneven on 1.3.9.mcp-socket-backend12/12,native-https-owner12/12,native-project-https24/24.bun testprocess (mirroring CI's shared environment): 410/410 on 1.4.2 and 410/410 on 1.3.9.bun run checkpass uncached; the 47 changed test files passultracite check; privacy ok.Refs HACK-1211.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.