Skip to content

fix(process-daemon): prune dead runtimes and sockets sooner - #60301

Closed
jlongster wants to merge 1 commit into
devfrom
daemon-stale-sweep
Closed

jlongster wants to merge 1 commit into
devfrom
daemon-stale-sweep

Conversation

@jlongster

Copy link
Copy Markdown
Contributor

Issue for this PR

Follow-up to a drive test of the persistent PTY daemon. No separate issue.

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

What we found. After an opencode-pty daemon is SIGKILLed, its runtime dir (service.json + service.lock) and socket stay behind. The next daemon didn't remove them, for two separate reasons:

  • Runtime dirs: the 10-minute age guard alone explains what the test saw. The test checked right away, and the sweep skips any dir younger than 10 minutes even when its lock is free and its registered pid is dead. The sweep only runs when a daemon starts, so a dead dir stayed until a daemon started at least 10 minutes later. The guard is still needed in one case: a daemon that has created its dir and lock file but not locked it yet looks exactly like an abandoned dir.
  • Sockets: the sweep only finds a socket through a sibling runtime dir. A socket is orphaned for good if its runtime dir is gone (temp runtime roots, removed test dirs, older layouts) or lives under another runtime root (a different XDG_STATE_HOME or --runtime-dir). That explains the weeks-old sockets in /tmp/opencode-pty-0.

There was also a related race in Runtime::bind. It counted any lock it acquired, even on a lock file that had already been unlinked. A stopping daemon (Cleanup::run) and the sweep both unlink the lock file while holding it and then release it. A daemon that opened the file just before that could then lock the orphaned inode, and a later daemon could create and lock a new service.lock, leaving two owners. This affects dirs older than 10 minutes today, because reusing an existing dir does not refresh its mtime.

Changes

  1. bind (Unix) now locks through lock_runtime. After try_lock_exclusive, it checks that the locked fd's dev/inode still matches the file at service.lock. If not, or if the dir disappeared, it retries on the new file (up to 3 times). The sweep makes the same check after taking a lock.
  2. On Unix, the sweep removes a sibling dir at any age when its lock is free and verified and its registration parses with a dead pid. Every other case keeps the 10-minute guard: no registration, a live or reused pid, or no lock file.
  3. On Unix, after the dir sweep, the daemon also sweeps its own socket root. It removes *.sock entries only if all of these hold: the entry is a socket (found with lstat, so symlinks are skipped), it is owned by our uid, it is older than 10 minutes, it is not our own socket, and a non-blocking connect returns ECONNREFUSED. It checks the inode again just before unlinking.
  4. Windows behaviour is unchanged: the age guard stays, and is_current_lock returns true because held lock handles deny delete sharing. The README runtime-dir paragraph is updated.

Why the races are safe. A daemon writes service.json only after it holds a lock it has verified is on the linked service.lock. The sweep deletes only while it holds a verified lock on that same file, and it unlinks the lock file last. So when the sweep sees a free, verified lock with a dead registered pid, no live daemon owns that dir. If a binder loses the race, either its try_lock fails while the sweep holds the lock, or it locks the unlinked inode and the check makes it retry. A failed bind already makes core retry in a fresh dir. If the binder creates a new lock file and registration after the sweep has unlinked the old one, the sweep's remove_dir fails with ENOTEMPTY and the binder's files stay.

For sockets, the 10-minute guard covers the window between bind(2) and listen(2), when a socket also refuses connections. The inode check covers a daemon reusing the same path. One window is left: between that last lstat and unlink, a daemon would have to restart in that exact runtime dir. Two other edge cases are accepted: a live daemon gets one unauthenticated connection that closes at once (it logs to stderr, which core ignores), and on macOS a listener with a full backlog also returns ECONNREFUSED, so a wedged daemon with 128 pending connections and a socket older than 10 minutes could lose its socket.

The sweep still runs only at daemon start. That seems fine: each OpenCode server start spawns one daemon with a fresh UUID dir, and leftover dirs are small.

This is an internal daemon change. It ships with the next process-daemon release.

How did you verify your code works?

New unit tests:

  • sweep_removes_young_runtime_with_free_lock_and_dead_pid: removes a young dir with a free lock and a dead pid. Keeps a dir with a live pid, a dir whose lock is held, and a dir with a lock but no registration.
  • sweep_without_lock_file_checks_registered_pid: still keeps a young dir with no lock file.
  • lock_is_current_only_while_linked_at_its_path: fails while the lock is held, fails after the file is unlinked or replaced, and lets a successor claim the replacement.
  • socket_sweep_removes_only_old_dead_sockets: removes a dead socket. Keeps our own socket, a listening socket, a non-.sock socket, and a regular file.
  • socket_sweep_keeps_young_foreign_and_symlinked_sockets: keeps a young socket, one owned by another uid, and a symlink plus its target.

Commands run:

  • cargo fmt --check
  • cargo clippy --locked --all-targets --all-features -- -D warnings
  • cargo test --locked --lib --test service --test runtime --test ownership --test rows --test ghostty-effects (macOS)
  • Cross clippy for x86_64-pc-windows-msvc and x86_64-unknown-linux-gnu

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

@opencodejr opencodejr Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks sound from reading the code, but I couldn't verify it: the review sandbox has no Rust toolchain, so I couldn't build the daemon, reproduce the leftover dirs and sockets after a SIGKILL, or run the new tests. I relied on your macOS test run and cross-clippy.

What I checked in the code:

  • Runtime dir sweep: removal still needs a free lock that is verified to be on the file at service.lock, plus a dead registered pid. Registration is written only after the binder holds a verified lock, and the lock file is unlinked last. A binder that locks the orphaned inode retries, and one that recreates the dir afterwards stops remove_dir with ENOTEMPTY. I found no case where a live daemon loses its dir.
  • Socket sweep: it only looks in the dedicated opencode-pty-<uid> root and skips symlinks, other uids, young sockets and our own socket, then checks the inode again before unlinking. A probe connection to a live daemon gets its own handler thread in server.rs, so it can't stall the accept loop. The probe file descriptor can't leak into PTY children either, because portable-pty closes stray descriptors in children.
  • Tests: they are small and deterministic. They pass a future now instead of sleeping and use real sockets in a private temp root.

Public API: unchanged. This only changes internal daemon behaviour; the runtime file layout and the OPENCODE_PTY_SOCKET_DIR handling stay as they are.

No blocking concerns.

@jlongster

Copy link
Copy Markdown
Contributor Author

Closing: the existing 10-minute sweep covers abandoned runtime dirs well enough, and the remaining cases aren't worth the extra cleanup logic.

@jlongster jlongster closed this Oct 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant