peer-link: closing a displaced server unlinks the winner's socket

[nedtwigg]

closeServer in vscode-ext/src/peer-link.ts documents "Unlink only when the path still names our socket", but unlink only gates the explicit rm. closing.close() unlinks the path on its own: libuv's uv__pipe_close unlinks pipe_fname unconditionally. Reproduced on Node 26.9: A binds, B unlinks and rebinds, A.close() removes B's socket.

So the reclaim stand-down (closeServer(false)) removes the winner's socket too, not only disposal. Trace (from the #753 review):

1. A and B both clear the corpse; B displaces A. Both are inside stillOurs, and A's 250 ms expires first.
2. A mismatches and stands down: A.close() unlinks the path, which now names B's socket.
3. If B's stat landed before that unlink, B confirms and serves an unreachable socket. A's next round tryBinds the free path uncontested and confirms immediately. That leaves two confirmed brokers, which brokerConfirmed exists to prevent (docs/specs/vscode.md → "A bind is not a role until it is believed").

A likely fix direction is an atomic create-only publish: bind a unique path, then link() it onto the fixed one, which fails EEXIST instead of displacing. The unique path is then the one libuv unlinks on close.

Surfaced in #753, which narrows a different race and doesn't address this.

[dormouse-bot]

Confirmed by source, and the unlink parameter turns out to buy nothing at all — neither branch of closeServer is a "still ours" check.

Server.prototype.close calls this._handle.close() on the Pipe handle unconditionally, and uv__pipe_close unlinks pipe_fname guarded only by it being non-NULL. pipe_fname is the path this handle bound, so the unlink is by name, not by inode — libuv removes whatever currently holds the name. Its own comment there ("Doing it the other way around introduces a race where our process unlinks a socket with the same name that's just been created by another thread or process") is about ordering the unlink before the close; it does not address a name already replaced before close was called, which is exactly this case. I could not run a live reproduction — this sandbox blocks AF_UNIX outright (listen fails EAFNOSUPPORT on every filesystem, so vscode-ext/test/peer-link.test.ts cannot run here at all) — so the chain above is read from Node's and libuv's source rather than observed. Your Node 26.9 run is the observation. No fix PR from me: with no way to exercise the arbitration locally, a platform-forked rewrite of the publish path is not a change I can defend, and it would collide with #753 on both this file and its test file. Analysis only, below.

Two things that follow beyond the trace in the report:

The unlink: true path has the same defect, on a longer fuse. closeServer runs closing.close() before it consults unlink, and uv_close dispatches uv__pipe_close synchronously, so the path is gone by the time line 1302 reads the flag; the rm on line 1304 finds it already absent in every case where the server was listening, which is every case a caller reaches. unlink: false therefore does not skip an unlink, and unlink: true does not add one — the flag is inert on unix, and the doc comment's rule ("Unlink only when the path still names our socket") is implemented in neither branch. The bind-then-disposed branches at L1163 and L1190 inherit it: a window displaced during its own tryBind and then disposed removes the winner's socket too. That window is narrow (the disposed read is synchronous after the bind) where the stand-down's is a full RECLAIM_VERIFY_MS, but it is the same defect and a fix that only guards the stand-down would leave it.

A create-only publish would make stillOurs dead code, which puts it in tension with #753. If the fixed path is only ever produced by link() from a unique bound path, nothing can displace a publisher — the loser gets EEXIST and stands down before it ever believes anything — so there is no post-bind displacement left for stillOurs to detect on unix, and on Windows it already short-circuits to unstattable. #753 anchors that check to the window's own bind and is correct against today's code, but the two changes are alternatives on the same race rather than layers: landing the link() publish would delete the function #753 improves. Worth deciding the order before either lands.

Two loose ends in the link() direction

Orphaned unique paths. A broker that dies without running its disposables leaves both names of the same inode behind. A reclaimer clearing the fixed name cannot derive the unique one from it, so the unique socket file survives every reclaim. peerDirPath() is under tmpdir(), so OS tmp reaping bounds the accumulation eventually, but the reclaim path would need its own rule — resolving the unique name before the rm, or sweeping the peer directory for sockets that refuse connection.

The reclaim dance still has a job. link() removes the displacement race but not the corpse problem: a stale fixed name still blocks every publish with EEXIST until somebody unlinks it, so the two-refusal check and the jitter at attempt stay. What changes is that the rm-then-publish sequence no longer needs to be atomic — several windows can clear the same corpse and race to link(), and exactly one wins.

Both are verified against the code, not against a running socket.

docs/specs/vscode.md -> "A corpse is cleared, then the bind is re-checked" states the re-check rule but says nothing about what the stand-down may unlink; whichever fix lands, that line is where the invariant belongs.