Skip to content

fix: recover markerless stale rendezvous sockets during activation - #1894

Open
mvanhorn wants to merge 2 commits into
DeusData:mainfrom
mvanhorn:fix/1760-stale-rendezvous-recovery
Open

fix: recover markerless stale rendezvous sockets during activation#1894
mvanhorn wants to merge 2 commits into
DeusData:mainfrom
mvanhorn:fix/1760-stale-rendezvous-recovery

Conversation

@mvanhorn

Copy link
Copy Markdown
Contributor

What does this PR do?

Extend the existing POSIX stale-generation cleanup in src/daemon/ipc.c to recognize the markerless hard-linked .sock/.anc pair only while it retains the matching startup lock and a temporary exclusive lifetime reservation. Require both paths to be secure sockets for the same device/inode with the exact two-link shape, revalidate those identities immediately before inode-matched unlinking, and continue to refuse unanchored, mismatched, malformed, replaced, or actively reserved endpoints. Make the activation-only generation probe invoke this serialized cleanup before classifying a remaining endpoint, so install and update use the recovery path already shared by daemon startup without weakening the conservative ECONNREFUSED behavior for live or saturated listeners.

Hard-killing a POSIX daemon or MCP process can leave the owner-private rendezvous namespace with a hard-linked stable socket and anchor but without the pending or identity record that normally authorizes cleanup. The kernel releases the process-owned startup, lifetime, and cohort locks, yet activation's generation probe treats the refused socket as active and permanently blocks install and update. Deleting /tmp/cbm-daemon-<uid> works around the problem, confirming that durable transport artifacts—not a live CBM session—cause the refusal. The current branch already documents CBM_RUNTIME_DIR, distinguishes coordination failures from busy sessions, and returns a nonzero activation status, so this change is limited to safe automatic recovery of the reported crash shape.

Fixes #1760

Checklist

  • Every commit is signed off (git commit -s) — required, CI rejects
    Not run: no test command resolved in this workspace, so nothing was executed to pass.
    unsigned commits (DCO, see CONTRIBUTING.md)
  • Tests pass locally (make -f Makefile.cbm test)
    Not run: no test command resolved in this workspace, so nothing was executed to pass.
  • Lint passes (make -f Makefile.cbm lint-ci)
    Not run: no test command resolved in this workspace, so nothing was executed to pass.
  • New behavior is covered by a test (reproduce-first for bug fixes)

@mvanhorn
mvanhorn requested a review from DeusData as a code owner August 29, 2026 01:32
@github-actions

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

@DeusData

Copy link
Copy Markdown
Owner

Thanks for this — and apologies that the only thing you have heard so far was the automated acknowledgement.

Status: the one thing blocking it is DCO. Every other required check is green, which is a better starting position than most PRs arrive in.

It is a single commit (498f69b4), so it should be one round trip: amend it with a sign-off (git commit --amend -s --no-edit) and force-push with lease.

The substance still needs a proper read against #1760 before anything is promised — that is queued, not skipped. But the sign-off is worth doing now so the review is not waiting on a formality when it comes up.

Fixes DeusData#1760

Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
@mvanhorn
mvanhorn force-pushed the fix/1760-stale-rendezvous-recovery branch from 498f69b to 8e70590 Compare August 31, 2026 12:49
@DeusData DeusData added bug Something isn't working stability/performance Server crashes, OOM, hangs, high CPU/memory security Security vulnerabilities, hardening priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Sep 1, 2026
@DeusData

DeusData commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Review done, and the hard part holds up: I had two independent passes on the cleanup path, one of them adversarial. The only unlink primitive reached is posix_socket_path_unlink_inode_if_matchesfstatat(AT_SYMLINK_NOFOLLOW), socket type, owner == euid, mode 0600, exact link count, then dev+ino compared against the identity captured just before, then unlinkat(dir_fd, name, 0) — inside a runtime directory opened by the component-wise O_DIRECTORY|O_NOFOLLOW walk, with bare 16-hex basenames. The markerless branch runs only under the startup lock and only after taking the lifetime reservation itself, and .anc plus the lifetime lock were born in the same commit, so a same-inode nlink-2 pair with no lifetime holder really is a dead owner and a legacy bare socket is preserved. Attempts to unlink a live daemon's socket, another user's socket, a symlink target, or to race the identity capture all hit an explicit check. On a tree merged with current main the nine daemon/IPC/CLI suites pass (542/0), and with main's ipc.c/ipc.h under your tests exactly the three new ones go red — they bind. Your sign-off is in place (the earlier ask was about the older commit), lint and the no-skips check are clean, and I've refreshed the branch from main so the MSan leg (which died building its Docker image, not on your code) gets a real run.

One line before it merges — a test-isolation defect, not a logic one: your new cli test resets the activation runtime parent with cbm_cli_set_activation_runtime_parent_for_test(NULL) (tests/test_cli.c, the new test around line 1424). Main's suite contract expects g_cli_suite_runtime_parent to be restored there (see the sibling at :1310 and the contract at :61–65); with NULL, every later cli test runs the now-mutating activation probe against the developer's real /tmp/cbm-daemon-<uid> — a host-dependent verdict that can stop a live daemon. Restore g_cli_suite_runtime_parent instead of NULL, keep the sign-off, and I merge on green.

For the record on #1760: this fix covers both the markerless shape and the marker-present residue for every probe caller, so it supersedes #1790 (whose own test passes on your tree unchanged); I'll close that one with credit once yours lands. Thanks for the careful work — the nlink/identity proof is the kind of thing that makes a cleanup path trustworthy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. security Security vulnerabilities, hardening stability/performance Server crashes, OOM, hangs, high CPU/memory

Projects

None yet

2 participants