fix: recover from unclean shutdown using PID-based lockfile#1031
Open
shehanrao12-cpu wants to merge 3 commits into
Open
fix: recover from unclean shutdown using PID-based lockfile#1031shehanrao12-cpu wants to merge 3 commits into
shehanrao12-cpu wants to merge 3 commits into
Conversation
When dolos is killed with SIGKILL (kill -9), the Unix socket file remains on disk, preventing restart with 'Address already in use'. This fix implements the suggested lockfile approach: - Write a .pid file alongside the socket with the current process ID - On startup, if a stale socket exists, check if the PID in the lockfile is still running using kill(pid, 0) - If the process is dead, safely remove the stale socket and proceed - If the process is alive, return a clear error message - Clean up the lockfile on graceful shutdown Fixes txpipe#293
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe Unix server now uses a PID lockfile to decide when an existing socket can be removed, records the current PID on startup, and removes the lockfile on shutdown. The root crate also adds ChangesUnix socket PID lockfile recovery
Sequence Diagram(s)sequenceDiagram
participant DriverRun as Driver::run
participant PidLock as pid lockfile
participant PidCheck as is_process_running(pid)
participant Kill as libc::kill(pid, 0)
participant SocketFile as existing socket file
DriverRun->>PidLock: read stored PID
PidLock-->>DriverRun: PID or read/parse failure
DriverRun->>PidCheck: verify PID liveness
PidCheck->>Kill: probe process with signal 0
Kill-->>PidCheck: alive or not alive
PidCheck-->>DriverRun: stale or running
alt stale or invalid
DriverRun->>SocketFile: remove stale socket
DriverRun->>PidLock: write current PID
else alive
Note over DriverRun: return socket-in-use error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/serve/o7s_unix/mod.rs (1)
117-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the already-read PID instead of re-reading the lockfile.
The in-use error branch reads
lock_patha second time (Line 132), which can race with the parse done at Lines 118-120 and may produce an empty PID if the second read fails. Capture the PID once and reuse it. The redundant.trim().to_string()insideformat!is also an unnecessary allocation thatcargo clippywill likely flag.As per coding guidelines: "Run `cargo clippy --workspace --all-targets --all-features` and resolve all clippy warnings before committing changes".♻️ Suggested consolidation
- let stale = match std::fs::read_to_string(&lock_path) { - Ok(pid_str) => { - let pid: u32 = pid_str.trim().parse().unwrap_or(0); - pid == 0 || !is_process_running(pid) - } - Err(_) => true, // no lockfile = stale, safe to remove - }; + let stored_pid: u32 = std::fs::read_to_string(&lock_path) + .ok() + .and_then(|s| s.trim().parse().ok()) + .unwrap_or(0); + let stale = stored_pid == 0 || !is_process_running(stored_pid); if stale { debug!("preventive removal of stale socket file"); let _ = std::fs::remove_file(&lock_path); std::fs::remove_file(&cfg.service.listen_path) .map_err(|e| ServeError::Internal(e.into()))?; } else { return Err(ServeError::Internal( - format!("socket {} is in use by PID {}", cfg.service.listen_path.display(), - std::fs::read_to_string(&lock_path).unwrap_or_default().trim().to_string()).into(), + format!( + "socket {} is in use by PID {}", + cfg.service.listen_path.display(), + stored_pid + ) + .into(), )); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/serve/o7s_unix/mod.rs` around lines 117 - 134, The in-use error path in serve/o7s_unix::mod::stale-socket cleanup re-reads lock_path after already parsing the PID earlier, which can race and also adds an unnecessary allocation. Reuse the already parsed pid value from the existing stale-check logic when building the ServeError::Internal message, and avoid calling read_to_string/trim/to_string again in that branch.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/serve/o7s_unix/mod.rs`:
- Around line 9-23: The process-liveness check in is_process_running currently
treats any failed libc::kill(pid, 0) call as “not running,” which misclassifies
EPERM as dead; update this helper to inspect errno after the kill call so EPERM
is treated as still running and only ESRCH is treated as missing, keeping the
existing behavior for the non-unix branch unchanged and preserving the callers
that rely on is_process_running.
---
Nitpick comments:
In `@src/serve/o7s_unix/mod.rs`:
- Around line 117-134: The in-use error path in
serve/o7s_unix::mod::stale-socket cleanup re-reads lock_path after already
parsing the PID earlier, which can race and also adds an unnecessary allocation.
Reuse the already parsed pid value from the existing stale-check logic when
building the ServeError::Internal message, and avoid calling
read_to_string/trim/to_string again in that branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7eac270-062f-4d6a-a2f4-16bf40d45fb9
📒 Files selected for processing (2)
Cargo.tomlsrc/serve/o7s_unix/mod.rs
shehanrao12-cpu
force-pushed
the
fix/unix-socket-recovery-after-kill9
branch
3 times, most recently
from
fdb2f06 to
5fb7d8f
Compare
shehanrao12-cpu
force-pushed
the
fix/unix-socket-recovery-after-kill9
branch
from
5fb7d8f to
d69498a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/serve/o7s_unix/mod.rs (1)
16-16: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
EPERMclassified as “running.”This still treats every failed
kill(pid, 0)as dead. Ifkillfails withEPERM, the process exists but is owned by another user, so the socket must not be removed. This matches the earlier review finding, so tagging as duplicate.Proposed fix
- unsafe { libc::kill(pid as libc::pid_t, 0) == 0 } + let ret = unsafe { libc::kill(pid as libc::pid_t, 0) }; + ret == 0 + || matches!( + std::io::Error::last_os_error().raw_os_error(), + Some(libc::EPERM) + )On Unix, what errno does kill(pid, 0) set when the target process exists but the caller lacks permission, and does libc 0.2 expose EPERM?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/serve/o7s_unix/mod.rs` at line 16, `is_running` in `o7s_unix::mod` currently treats any nonzero `kill(pid, 0)` result as dead; update it to check the errno from `libc::kill` and classify `EPERM` as still running while only treating `ESRCH` as not running. Use the existing `is_running` helper and keep the socket-removal flow unchanged except for this permission-aware status handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/serve/o7s_unix/mod.rs`:
- Around line 116-138: Make PID lock ownership atomic in the socket startup
path: the current logic in the o7s_unix module checks the socket and then writes
the lockfile separately, which lets two instances race and overwrite each
other’s PID. Update the startup flow around the listen_path/lock_path handling
so the lock is acquired before binding, validate the lockfile even when the
socket file is missing, and keep ownership tied to the current process’s PID.
Also ensure the shutdown cleanup path only removes the lockfile if it still
contains this process’s PID, using the same lockfile handling logic in the
related cleanup code.
- Around line 117-123: The stale-socket check in the lockfile handling path is
too permissive: in the logic around read_to_string and pid parsing, unreadable
or malformed PID data is currently treated as stale. Update the stale
determination to only treat a missing lockfile as stale, and make other read
failures or parse errors return a clear error instead of proceeding. Use the
existing stale/is_process_running flow and the lock_path handling in
src/serve/o7s_unix/mod.rs to locate the fix.
---
Duplicate comments:
In `@src/serve/o7s_unix/mod.rs`:
- Line 16: `is_running` in `o7s_unix::mod` currently treats any nonzero
`kill(pid, 0)` result as dead; update it to check the errno from `libc::kill`
and classify `EPERM` as still running while only treating `ESRCH` as not
running. Use the existing `is_running` helper and keep the socket-removal flow
unchanged except for this permission-aware status handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ebbb7c3f-878f-4c0c-8b17-2f2ac6227b64
📒 Files selected for processing (2)
Cargo.tomlsrc/serve/o7s_unix/mod.rs
Comment on lines
116
to
+138
| if std::fs::metadata(&cfg.service.listen_path).is_ok() { | ||
| debug!("preventive removal of socket file"); | ||
| std::fs::remove_file(&cfg.service.listen_path) | ||
| .map_err(|e| ServeError::Internal(e.into()))?; | ||
| let stale = match std::fs::read_to_string(&lock_path) { | ||
| Ok(pid_str) => { | ||
| let pid: u32 = pid_str.trim().parse().unwrap_or(0); | ||
| pid == 0 || !is_process_running(pid) | ||
| } | ||
| Err(_) => true, // no lockfile = stale, safe to remove | ||
| }; | ||
| if stale { | ||
| debug!("preventive removal of stale socket file"); | ||
| let _ = std::fs::remove_file(&lock_path); | ||
| std::fs::remove_file(&cfg.service.listen_path) | ||
| .map_err(|e| ServeError::Internal(e.into()))?; | ||
| } else { | ||
| return Err(ServeError::Internal( | ||
| format!("socket {} is in use by PID {}", cfg.service.listen_path.display(), | ||
| std::fs::read_to_string(&lock_path).unwrap_or_default().trim()).into(), | ||
| )); | ||
| } | ||
| } | ||
| // write our PID to the lockfile | ||
| std::fs::write(&lock_path, std::process::id().to_string()) | ||
| .map_err(|e| ServeError::Internal(e.into()))?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make PID lock ownership atomic.
Two instances starting together can both pass Line 116 before either binds, then Line 137 can be overwritten by the losing process. A later restart can see a live socket paired with a dead PID and remove the active socket. Acquire the PID lock atomically, check it even when the socket is absent, and only remove the lockfile on shutdown if it still contains this process’s PID.
Also applies to: 158-159
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/serve/o7s_unix/mod.rs` around lines 116 - 138, Make PID lock ownership
atomic in the socket startup path: the current logic in the o7s_unix module
checks the socket and then writes the lockfile separately, which lets two
instances race and overwrite each other’s PID. Update the startup flow around
the listen_path/lock_path handling so the lock is acquired before binding,
validate the lockfile even when the socket file is missing, and keep ownership
tied to the current process’s PID. Also ensure the shutdown cleanup path only
removes the lockfile if it still contains this process’s PID, using the same
lockfile handling logic in the related cleanup code.
Comment on lines
+117
to
+123
| let stale = match std::fs::read_to_string(&lock_path) { | ||
| Ok(pid_str) => { | ||
| let pid: u32 = pid_str.trim().parse().unwrap_or(0); | ||
| pid == 0 || !is_process_running(pid) | ||
| } | ||
| Err(_) => true, // no lockfile = stale, safe to remove | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail closed on unreadable or malformed PID files.
Line 122 treats all read errors as stale, and Line 119 turns parse failures into PID 0; both paths can remove a socket without proving the owner is dead. Only NotFound should follow the stale/no-lockfile path; other read or parse failures should return a clear error.
Proposed hardening
- let stale = match std::fs::read_to_string(&lock_path) {
- Ok(pid_str) => {
- let pid: u32 = pid_str.trim().parse().unwrap_or(0);
- pid == 0 || !is_process_running(pid)
- }
- Err(_) => true, // no lockfile = stale, safe to remove
- };
+ let stale = match std::fs::read_to_string(&lock_path) {
+ Ok(pid_str) => {
+ let pid: u32 = pid_str.trim().parse().map_err(|e| {
+ ServeError::Internal(
+ format!("invalid PID lockfile {}: {e}", lock_path.display()).into(),
+ )
+ })?;
+
+ if pid == 0 {
+ return Err(ServeError::Internal(
+ format!("invalid PID lockfile {}: PID cannot be 0", lock_path.display())
+ .into(),
+ ));
+ }
+
+ !is_process_running(pid)
+ }
+ Err(error) if error.kind() == std::io::ErrorKind::NotFound => true,
+ Err(error) => return Err(ServeError::Internal(error.into())),
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let stale = match std::fs::read_to_string(&lock_path) { | |
| Ok(pid_str) => { | |
| let pid: u32 = pid_str.trim().parse().unwrap_or(0); | |
| pid == 0 || !is_process_running(pid) | |
| } | |
| Err(_) => true, // no lockfile = stale, safe to remove | |
| }; | |
| let stale = match std::fs::read_to_string(&lock_path) { | |
| Ok(pid_str) => { | |
| let pid: u32 = pid_str.trim().parse().map_err(|e| { | |
| ServeError::Internal( | |
| format!("invalid PID lockfile {}: {e}", lock_path.display()).into(), | |
| ) | |
| })?; | |
| if pid == 0 { | |
| return Err(ServeError::Internal( | |
| format!("invalid PID lockfile {}: PID cannot be 0", lock_path.display()) | |
| .into(), | |
| )); | |
| } | |
| !is_process_running(pid) | |
| } | |
| Err(error) if error.kind() == std::io::ErrorKind::NotFound => true, | |
| Err(error) => return Err(ServeError::Internal(error.into())), | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/serve/o7s_unix/mod.rs` around lines 117 - 123, The stale-socket check in
the lockfile handling path is too permissive: in the logic around read_to_string
and pid parsing, unreadable or malformed PID data is currently treated as stale.
Update the stale determination to only treat a missing lockfile as stale, and
make other read failures or parse errors return a clear error instead of
proceeding. Use the existing stale/is_process_running flow and the lock_path
handling in src/serve/o7s_unix/mod.rs to locate the fix.
Reformats the multi-line format!() call in the PID-lockfile conflict path to satisfy 'cargo fmt --check'.
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
1 participant
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.
Fixes #293
When dolos is killed with
kill -9, the Unix socket file remains on disk, preventing restart withAddress already in use (os error 48).This implements the lockfile approach suggested in the issue:
.pidfile alongside the socket containing the current process IDkill(pid, 0)(signal 0 = no-op, just checks existence)socket X is in use by PID YThe existing code already tried to remove stale sockets, but it had no way to know if another dolos instance was legitimately using the socket. The PID lockfile solves this.
Changes:
src/serve/o7s_unix/mod.rs: Addedis_process_running()helper, PID lockfile logic on startup, lockfile cleanup on shutdownCargo.toml: Addedlibc = "0.2"dependency (already transitively included vianix)Summary by CodeRabbit