Repository navigation
Conversation
There was a problem hiding this comment.
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 stopsremove_dirwith 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 inserver.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
nowinstead 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.
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue for this PR
Follow-up to a drive test of the persistent PTY daemon. No separate issue.
Type of change
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:XDG_STATE_HOMEor--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 newservice.lock, leaving two owners. This affects dirs older than 10 minutes today, because reusing an existing dir does not refresh its mtime.Changes
bind(Unix) now locks throughlock_runtime. Aftertry_lock_exclusive, it checks that the locked fd's dev/inode still matches the file atservice.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.*.sockentries only if all of these hold: the entry is a socket (found withlstat, 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-blockingconnectreturnsECONNREFUSED. It checks the inode again just before unlinking.is_current_lockreturnstruebecause held lock handles deny delete sharing. The README runtime-dir paragraph is updated.Why the races are safe. A daemon writes
service.jsononly after it holds a lock it has verified is on the linkedservice.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 itstry_lockfails 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'sremove_dirfails with ENOTEMPTY and the binder's files stay.For sockets, the 10-minute guard covers the window between
bind(2)andlisten(2), when a socket also refuses connections. The inode check covers a daemon reusing the same path. One window is left: between that lastlstatandunlink, 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 returnsECONNREFUSED, 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-.socksocket, 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 --checkcargo clippy --locked --all-targets --all-features -- -D warningscargo test --locked --lib --test service --test runtime --test ownership --test rows --test ghostty-effects(macOS)x86_64-pc-windows-msvcandx86_64-unknown-linux-gnuChecklist