Skip to content

fix: recover from unclean shutdown using PID-based lockfile #1031

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shehanrao12-cpu wants to merge 3 commits into
base: main
Choose a base branch
from
+48 −3
Open

fix: recover from unclean shutdown using PID-based lockfile #1031

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8222e60
fix: recover from unclean shutdown using PID-based lockfile
Jun 25, 2026
d69498a
fix: remove redundant to_string in format args (clippy)
Jun 27, 2026
8511d17
fix: apply rustfmt to socket-busy error message
Jun 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ base64.workspace = true
bech32.workspace = true
bincode.workspace = true
chrono.workspace = true
libc = "0.2"
futures-core.workspace = true
futures-util.workspace = true
hex.workspace = true
Expand Down
44 changes: 41 additions & 3 deletions src/serve/o7s_unix/mod.rs
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,22 @@ use tracing::{debug, info, instrument, warn};

use crate::prelude::*;

/// Check if a process with the given PID is still running
fn is_process_running(pid: u32) -> bool {
// Send signal 0 to check if a process exists without affecting it.
// Returns true if the process is running, false if it doesn't exist
// or we lack permission (which means something else owns the pid).
#[cfg(unix)]
{
unsafe { libc::kill(pid as libc::pid_t, 0) == 0 }
}
#[cfg(not(unix))]
{
let _ = pid;
true
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

mod chainsync;
mod statequery;
mod utils;
Expand Down Expand Up @@ -95,11 +111,31 @@ impl<D: Domain, C: CancelToken> dolos_core::Driver<D, C> for Driver {
#[instrument(skip_all)]
async fn run(cfg: Self::Config, domain: D, cancel: C) -> Result<(), ServeError> {
// preventive removal of socket file in case of unclean shutdown
// check if a stale PID lockfile exists and the process is dead before removing
let lock_path = cfg.service.listen_path.with_extension("pid");
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
};
Comment on lines +117 to +123

@coderabbitai coderabbitai Bot Jun 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 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.

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()))?;
Comment on lines 116 to +144

@coderabbitai coderabbitai Bot Jun 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 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.


let mut tasks = TaskTracker::new();

Expand All @@ -119,6 +155,8 @@ impl<D: Domain, C: CancelToken> dolos_core::Driver<D, C> for Driver {
return Err(ServeError::Internal(error.into()));
}
}
// clean up PID lockfile
let _ = std::fs::remove_file(cfg.service.listen_path.with_extension("pid"));

// notify the tracker that we're done receiving new tasks. Without this explicit
// close, the wait will block forever.
Expand Down