b296da2b

fix: muxd run refuses a live socket instead of stealing it

a73x 2026-08-08 14:08

Commit message

fix: muxd run refuses a live socket instead of stealing it

docs/decisions.md

+  not a footnote; `wan.sh`'s netem stanza adds delay on egress only, so
+  `delay 75ms` is ~75ms of added round trip and not ~150ms (the script
+  says so; the plan's parenthetical said otherwise).
+- **`muxd run` refuses a live socket rather than stealing it (post-M6
+  field fix).** The incident: three daemons were started against one
+  path, each unlinking it and binding fresh. None of them died — the
+  older two kept running with their sessions and shells intact but
+  permanently unreachable, and the user's two terminals were attached to
+  two *different* sessions, which made every cross-client behaviour
+  (latest-wins, shared output) look broken. Init now probes the path
+  first: something answers → `error.DaemonAlreadyRunning` and no unlink;
+  nothing there → bind; a socket file nobody answers on → a dead daemon's
+  leftover, unlink and rebind. The rule is **unlink only what answers
+  ECONNREFUSED *and* stats as a socket** — necessary because Linux
+  answers ECONNREFUSED for a *regular file* at the path exactly as it
+  does for a dead socket, so the connect result alone would have made
+  `muxd run --sock notes.txt` delete notes.txt. The probe is skipped
+  under systemd socket activation, where the path is systemd's to manage,
+  and it runs before the pty is spawned so a refusal costs no fork. This
+  is the complementary half of deinit's existing unlink-only-if-still-ours
+  check: one guards the path on the way in, the other on the way out.
+- **Banked cleanup (post-M6, from the socket-claim review):** the systemd
+  `LISTEN_FDS` path has no automated test proving the probe is skipped
+  (verified by hand only). `claimSockPath` still stack-traces on
+  AccessDenied / NameTooLong / an absent parent directory — cosmetic, the
+  same one-line treatment `DaemonAlreadyRunning` gets would do. The
+  stat→unlink window is a TOCTOU race, accepted as non-adversarial: a
+  socket path a hostile process can swap under us is already a directory
+  we do not control, and closing it properly means a tmux-style lockfile
+  beside the socket, not a cleverer stat. That `fstatat` FileNotFound arm
+  is consequently unreachable from any test — it exists for that window
+  alone.
+- **Owed before this is more than a prototype, in order:** TLS or QUIC
+  for any deployment that is not tunnelled through SSH — `--via` borrows
+  ssh's authentication and encryption entirely, and has none of its own;

src/main.zig

Old		New
@@ -69,12 +69,31 @@ pub fn main() !u8 {
69	try alloc.dupeZ(u8, std.posix.getenv("SHELL") orelse "/bin/sh");	69	try alloc.dupeZ(u8, std.posix.getenv("SHELL") orelse "/bin/sh");
70	defer alloc.free(shell_z);	70	defer alloc.free(shell_z);
71		71
72	var srv = try Server.init(alloc, .{	72	var srv = Server.init(alloc, .{
73	.sock_path = sock_path,	73	.sock_path = sock_path,
74	.shell = shell_z,	74	.shell = shell_z,
75	.cols = cols,	75	.cols = cols,
76	.rows = rows,	76	.rows = rows,
77	});	77	}) catch \|err\| switch (err) {
		78	// All of these mean "that path is not ours to take", and all
		79	// are ordinary operator mistakes rather than daemon bugs: say
		80	// so in one line and exit, no stack trace.
		81	//
		82	// AddressInUse is the same situation found one syscall later:
		83	// two daemons starting at once can both see an empty path and
		84	// both try to bind it. The loser has simply lost a dead heat,
		85	// and telling it "a daemon is already running" is exactly
		86	// right — by the time it reads the message, one is.
		87	error.DaemonAlreadyRunning, error.AddressInUse => {
		88	std.debug.print("muxd: a daemon is already running on {s}\n", .{sock_path});
		89	return 1;
		90	},
		91	error.SockPathNotASocket => {
		92	std.debug.print("muxd: {s} exists and is not a socket\n", .{sock_path});
		93	return 1;
		94	},
		95	else => return err,
		96	};
78	defer srv.deinit();	97	defer srv.deinit();
79	@import("server").installSignalHandlers();	98	@import("server").installSignalHandlers();
80	return try srv.run();	99	return try srv.run();

src/server.zig

@@ @@ -251,6 +251,16 @@ pub const Server = struct { @@
+    };
+    pub fn init(alloc: std.mem.Allocator, opts: Options) !Server {
+        // systemd socket activation: LISTEN_FDS=1 hands us the listener as
+        // fd 3, and the path is systemd's to manage — we neither probe nor
+        // unlink it. Read once, up front, so the claim below can be the
+        // first thing that happens on the self-bind path.
+        const systemd_fd = listenFdFromSystemd();
+        // Before the shell is spawned, so refusing costs nobody a fork and
+        // leaves no process to reap.
+        if (systemd_fd == null) try claimSockPath(opts.sock_path);
+        const eng = try Engine.init(alloc, .{ .cols = opts.cols, .rows = opts.rows });
+        errdefer eng.deinit();
@@ @@ -263,8 +273,7 @@ pub const Server = struct { @@
+        var epoch: u64 = 0;
+        while (epoch == 0) epoch = std.crypto.random.int(u64);
+        if (systemd_fd) |fd| {
+            return .{
+                .alloc = alloc,
+                .eng = eng,
@@ @@ -279,7 +288,6 @@ pub const Server = struct { @@
+            };
+        const addr = try std.net.Address.initUnix(opts.sock_path);
+        return .{
+            .alloc = alloc,
@@ @@ -292,6 +300,51 @@ pub const Server = struct { @@
+        };
+    /// Make the socket path ours to bind, or refuse it. Field incident this
+    /// exists for: three daemons were started against one path, each
+    /// unlinking it and binding fresh. Every one of them kept running with
+    /// its sessions intact, but only the newest was reachable — the older
+    /// two were stranded, invisible, holding shells nobody could get back
+    /// to, and two terminals "in the same session" were really in two
+    /// different ones.
+    ///
+    /// So: unlink only what answers ECONNREFUSED *and* is a socket.
+    ///   - something answers  → a live daemon owns this path. Refuse.
+    ///   - nothing there      → bind, nothing to clean up.
+    ///   - a dead socket file → a daemon that died without deinit's
+    ///                          unlink running. Ours to clear.
+    ///   - anything else      → propagate. A path we cannot positively
+    ///                          identify as a dead daemon's leftover is not
+    ///                          something we may delete.
+    fn claimSockPath(path: []const u8) !void {
+        if (std.net.connectUnixSocket(path)) |probe| {
+            probe.close();
+            return error.DaemonAlreadyRunning;
+        } else |err| switch (err) {
+            error.FileNotFound => return, // free path; bind straight away
+            // Nobody is listening — but this is NOT yet proof of a stale
+            // socket: Linux answers ECONNREFUSED for a regular file at the
+            // path exactly as it does for a dead socket, so connect alone
+            // cannot tell a dead daemon from `muxd run --sock notes.txt`.
+            // The stat below is what separates them.
+            error.ConnectionRefused => {},
+            else => |e| return e,
+        }
+        const st = std.posix.fstatat(std.posix.AT.FDCWD, path, 0) catch |err| switch (err) {
+            error.FileNotFound => return, // vanished under us; path is free
+            else => |e| return e,
+        };
+        if (!std.posix.S.ISSOCK(st.mode)) return error.SockPathNotASocket;
+        std.fs.cwd().deleteFile(path) catch |err| switch (err) {
+            // Someone else cleared it first. The path is free either way,
+            // which is the only thing this function was after.
+            error.FileNotFound => {},
+            else => |e| return e,
+        };
+    }
+    fn listenFdFromSystemd() ?std.posix.fd_t {
+        const pid_s = std.posix.getenv("LISTEN_PID") orelse return null;
+        const nfds_s = std.posix.getenv("LISTEN_FDS") orelse return null;
+/// Test helper: assert `Server.init` refuses `path` with `want`.
+///
+/// The teardown on the failing branch is not tidiness. A Server that got
+/// built when it should not have owns a live shell on a pty, and letting
+/// the test discard it leaves that shell holding the test runner's stdout:
+/// the build then never sees EOF and hangs for hours instead of printing a
+/// failure. A regression in the refusal must cost one red test, not a wedged
+/// CI worker — which is exactly how the daemon-stealing bug hid in the first
+/// place.
+fn expectInitRefused(alloc: std.mem.Allocator, path: []const u8, want: anyerror) !void {
+    if (Server.init(alloc, .{ .sock_path = path, .shell = "/bin/sh" })) |built| {
+        var stolen = built;
+        stolen.deinit();
+        return error.InitShouldHaveRefused;
+    } else |err| {
+        try std.testing.expectEqual(want, err);
+    }
+}
+test "Server: a second daemon refuses a live socket instead of stealing it" {
+    const alloc = std.testing.allocator;
+    var tmp = std.testing.tmpDir(.{});
+    defer tmp.cleanup();
+    var path_buf: [256]u8 = undefined;
+    const dir_path = try tmp.dir.realpath(".", &path_buf);
+    const sock_path = try std.fmt.allocPrint(alloc, "{s}/live.sock", .{dir_path});
+    defer alloc.free(sock_path);
+    var srv = try Server.init(alloc, .{ .sock_path = sock_path, .shell = "/bin/sh" });
+    defer srv.deinit();
+    var stop = std.atomic.Value(bool).init(false);
+    const th = try std.Thread.spawn(.{}, serverThread, .{ &srv, &stop });
+    defer th.join();
+    defer stop.store(true, .release);
+    // The incident: this used to unlink the path and bind over it, leaving
+    // the first daemon running and unreachable.
+    try expectInitRefused(alloc, sock_path, error.DaemonAlreadyRunning);
+    // The loser touched nothing: the socket file is still there...
+    const st = try std.posix.fstatat(std.posix.AT.FDCWD, sock_path, 0);
+    try std.testing.expect(std.posix.S.ISSOCK(st.mode));
+    // ...and it still reaches the daemon that was already there. The epoch
+    // is what makes that precise: it names one daemon *instance*, so
+    // matching it rules out having been handed a replacement.
+    const c = try std.net.connectUnixSocket(sock_path);
+    defer c.close();
+    try proto.writeFrame(c.handle, .attach, &proto.encodeAttach(80, 24, 0, 0));
+    const first = try firstStateFrame(alloc, c.handle, 10_000);
+    try std.testing.expect(first != null);
+    try std.testing.expectEqual(proto.MsgType.snapshot, first.?.type);
+    try std.testing.expectEqual(srv.epoch, first.?.epoch);
+}
+test "Server: a dead daemon's leftover socket file is cleared and rebound" {
+    const alloc = std.testing.allocator;
+    var tmp = std.testing.tmpDir(.{});
+    defer tmp.cleanup();
+    var path_buf: [256]u8 = undefined;
+    const dir_path = try tmp.dir.realpath(".", &path_buf);
+    const sock_path = try std.fmt.allocPrint(alloc, "{s}/stale.sock", .{dir_path});
+    defer alloc.free(sock_path);
+    // What a daemon killed with SIGKILL leaves behind: a bound socket file
+    // whose listener is gone, since deinit's unlink never ran.
+    {
+        const addr = try std.net.Address.initUnix(sock_path);
+        var dead = try addr.listen(.{});
+        dead.deinit(); // closes the fd; the file stays on disk
+    }
+    const before = try std.posix.fstatat(std.posix.AT.FDCWD, sock_path, 0);
+    try std.testing.expect(std.posix.S.ISSOCK(before.mode));
+    // Nobody answers there, so the path is ours: recovery, not refusal.
+    var srv = try Server.init(alloc, .{ .sock_path = sock_path, .shell = "/bin/sh" });
+    defer srv.deinit();
+    var stop = std.atomic.Value(bool).init(false);
+    const th = try std.Thread.spawn(.{}, serverThread, .{ &srv, &stop });
+    defer th.join();
+    defer stop.store(true, .release);
+    // And the rebind is real, not just a file that reappeared: it serves.
+    const c = try std.net.connectUnixSocket(sock_path);
+    defer c.close();
+    try proto.writeFrame(c.handle, .attach, &proto.encodeAttach(80, 24, 0, 0));
+    const first = try firstStateFrame(alloc, c.handle, 10_000);
+    try std.testing.expect(first != null);
+    try std.testing.expectEqual(srv.epoch, first.?.epoch);
+}
+test "Server: a path that cannot be bound fails as AddressInUse" {
+    const alloc = std.testing.allocator;
+    var tmp = std.testing.tmpDir(.{});
+    defer tmp.cleanup();
+    var path_buf: [256]u8 = undefined;
+    const dir_path = try tmp.dir.realpath(".", &path_buf);
+    const sock_path = try std.fmt.allocPrint(alloc, "{s}/dangling.sock", .{dir_path});
+    defer alloc.free(sock_path);
+    // A dangling symlink reaches bind() the way a lost start-up race does,
+    // but deterministically: connect through it gets ENOENT (so the probe
+    // reads the path as free, exactly as the loser of a race does), while
+    // bind() gets EADDRINUSE off the directory entry the symlink itself
+    // occupies. Racing two real daemons would test the same prong by
+    // coin-flip; this pins it every run.
+    try tmp.dir.symLink("no-such-target", "dangling.sock", .{});
+    try expectInitRefused(alloc, sock_path, error.AddressInUse);
+    // And the path is left alone: we could not identify it as a dead
+    // daemon's socket, so it was never ours to delete.
+    const st = try std.posix.fstatat(
+        std.posix.AT.FDCWD,
+        sock_path,
+        std.posix.AT.SYMLINK_NOFOLLOW,
+    );
+    try std.testing.expect(std.posix.S.ISLNK(st.mode));
+}
+test "Server: a non-socket at the path is refused, not deleted" {
+    const alloc = std.testing.allocator;
+    var tmp = std.testing.tmpDir(.{});
+    defer tmp.cleanup();
+    var path_buf: [256]u8 = undefined;
+    const dir_path = try tmp.dir.realpath(".", &path_buf);
+    const file_path = try std.fmt.allocPrint(alloc, "{s}/notes.txt", .{dir_path});
+    defer alloc.free(file_path);
+    const contents = "muxd must not eat this";
+    try tmp.dir.writeFile(.{ .sub_path = "notes.txt", .data = contents });
+    // Connecting to a regular file fails with ECONNREFUSED — the very same
+    // errno a dead socket gives — so identifying stale sockets by the
+    // connect result alone would delete this file. It is the stat that
+    // saves it, and this test is what pins that.
+    try expectInitRefused(alloc, file_path, error.SockPathNotASocket);
+    const after = try tmp.dir.readFileAlloc(alloc, "notes.txt", 1024);
+    defer alloc.free(after);
+    try std.testing.expectEqualStrings(contents, after);
+}
+/// Test helper: the first snapshot-or-delta frame to arrive, reduced to what
+/// the resync tests assert on. `epoch` is 0 for a delta — only snapshots
+/// carry the session epoch.