Skip to content

fix: correctness hardening — force-kill ownership check, sync cwd validation, lock/parse robustness #21

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

Merged
hefgi merged 1 commit into from
Jun 11, 2026
Merged

fix: correctness hardening — force-kill ownership check, sync cwd validation, lock/parse robustness #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
61 changes: 61 additions & 0 deletions src/env.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,67 @@ mod tests {
assert_eq!(env["ECLUSE_POSTGRES_PORT"], "11533");
assert_eq!(env["PGPORT"], "11533"); // extra_port: 11532 + 1
}

// ── parse_env_file ────────────────────────────────────────────────────────

#[test]
fn parse_env_file_skips_comments_and_blanks() {
let dir = tempfile::TempDir::new().unwrap();
let path = dir.path().join(".env.ecluse");
std::fs::write(&path, "# comment=ignored\n\nPORT=3001\n # indented\n").unwrap();
let pairs = parse_env_file(&path);
assert_eq!(pairs, vec![("PORT".to_string(), "3001".to_string())]);
}

#[test]
fn parse_env_file_splits_on_first_equals() {
let dir = tempfile::TempDir::new().unwrap();
let path = dir.path().join(".env.ecluse");
std::fs::write(&path, "DATABASE_URL=postgres://x:5433/db?a=b\n").unwrap();
let pairs = parse_env_file(&path);
assert_eq!(pairs[0].1, "postgres://x:5433/db?a=b");
}

#[test]
fn parse_env_file_missing_file_is_empty() {
assert!(parse_env_file(Path::new("/nonexistent/.env.ecluse")).is_empty());
}

#[test]
fn parse_env_file_preserves_file_order() {
let dir = tempfile::TempDir::new().unwrap();
let path = dir.path().join(".env.ecluse");
std::fs::write(&path, "B=2\nA=1\n").unwrap();
let keys: Vec<String> = parse_env_file(&path).into_iter().map(|(k, _)| k).collect();
assert_eq!(keys, vec!["B", "A"]);
}
}

/// Parse a .env-style file into key/value pairs, in file order.
/// Blank lines and `#` comments are skipped; each line splits on the first
/// `=`; keys are trimmed, values taken as-is. Missing file → empty.
///
/// The single parser for `.env.ecluse` — `shell`, `env`, `up --json`, and
/// process spawning must all read the file the same way.
pub fn parse_env_file(path: &Path) -> Vec<(String, String)> {
let Ok(content) = std::fs::read_to_string(path) else {
return vec![];
};
content
.lines()
.filter_map(|line| {
let line = line.trim();
if line.is_empty() || line.starts_with('#') {
return None;
}
let (k, v) = line.split_once('=')?;
let k = k.trim();
if k.is_empty() {
return None;
}
Some((k.to_string(), v.to_string()))
})
.collect()
}

pub fn write_env_file(worktree: &Path, env: &HashMap<String, String>) -> Result<()> {
Expand Down
149 changes: 83 additions & 66 deletions src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,7 +792,7 @@ fn cmd_up_resume(
if args.force {
// Kill all non-skipped services.
log.step("--force: killing services on allocated ports...");
force_kill_session_services(&existing, &config, &explicit_skip, &log);
force_kill_session_services(&existing, &config, &root, &explicit_skip, &log);
} else {
// Auto-detect already-running services and add them to skip set.
log.step("Checking service health...");
Expand Down Expand Up @@ -914,37 +914,20 @@ fn cmd_up_resume(
}

/// Kill all non-skipped services for a session.
/// Native: kill by PID files, then by port (lsof). Docker: docker stop by container name.
/// Native: kill by port (lsof) for PIDs this session owns, then by PID files.
/// Docker: docker stop by container name.
fn force_kill_session_services(
session: &state::Session,
config: &config::Config,
root: &std::path::Path,
skip: &std::collections::HashSet<String>,
log: &log::StepLogger,
) {
// Kill native via existing PID files.
if let Some(pm) = &session.process_manager {
let result = session.spawn_result();
let pid_files_to_kill: Vec<_> = result
.pid_files
.iter()
.filter(|pf| {
let svc_name = pf.file_stem().and_then(|s| s.to_str()).unwrap_or("");
!skip.contains(svc_name)
})
.cloned()
.collect();
let filtered_result = process::SpawnResult {
tmux_session: result.tmux_session.clone(),
pid_files: pid_files_to_kill,
log_dir: result.log_dir.clone(),
};
process::kill_services(pm, &filtered_result);
}

// Kill by port for residual native processes only.
// Docker services are stopped via docker stop — never kill their host port
// by PID, as the listening process may be the container runtime itself
// (e.g. OrbStack) rather than the container.
// Kill by port first, while the session's pid files still exist — they
// are how ownership is established. Docker services are stopped via
// docker stop — never kill their host port by PID, as the listening
// process may be the container runtime itself (e.g. OrbStack) rather
// than the container.
let docker_svc_names: std::collections::HashSet<&str> = config
.services
.iter()
Expand All @@ -959,26 +942,56 @@ fn force_kill_session_services(
if docker_svc_names.contains(svc_name.as_str()) {
continue;
}
// lsof -ti TCP:<port> returns PIDs; kill -9 each
let output = std::process::Command::new("lsof")
.args(["-ti", &format!("TCP:{}", port), "-sTCP:LISTEN"])
.output();
if let Ok(out) = output {
let stdout = String::from_utf8_lossy(&out.stdout);
for pid_str in stdout.split_whitespace() {
if let Ok(pid) = pid_str.trim().parse::<u32>() {
log.detail(&format!(
"killed process {} on port {} ({})",
pid, port, svc_name
let Ok(pid) = pid_str.trim().parse::<u32>() else {
continue;
};
// The recorded port may be stale — another session's
// auto-bumped service or an unrelated app could hold it by
// now. Only kill PIDs that resolve to THIS session.
let owned = whose_pid::resolve(root, std::slice::from_ref(session), pid)
.is_some_and(|o| o.slug == session.slug);
if !owned {
log.warn(&format!(
"port {} is held by PID {} which is not owned by session '{}'; skipping — kill it manually if intended",
port, pid, session.slug
));
let _ = std::process::Command::new("kill")
.args(["-9", pid_str.trim()])
.status();
continue;
}
log.detail(&format!(
"killing process {} on port {} ({})",
pid, port, svc_name
));
terminate_with_grace(pid);
}
}
}

// Kill native via existing PID files.
if let Some(pm) = &session.process_manager {
let result = session.spawn_result();
let pid_files_to_kill: Vec<_> = result
.pid_files
.iter()
.filter(|pf| {
let svc_name = pf.file_stem().and_then(|s| s.to_str()).unwrap_or("");
!skip.contains(svc_name)
})
.cloned()
.collect();
let filtered_result = process::SpawnResult {
tmux_session: result.tmux_session.clone(),
pid_files: pid_files_to_kill,
log_dir: result.log_dir.clone(),
};
process::kill_services(pm, &filtered_result);
}

// Stop docker containers for non-skipped docker services.
let docker_svcs: Vec<_> = config
.services
Expand All @@ -994,6 +1007,23 @@ fn force_kill_session_services(
}
}

/// SIGTERM, escalating to SIGKILL after a short grace period if still alive.
fn terminate_with_grace(pid: u32) {
let _ = std::process::Command::new("kill")
.args(["-TERM", &pid.to_string()])
.status();
let deadline = std::time::Instant::now() + std::time::Duration::from_secs(2);
while process::pid_alive(pid) {
if std::time::Instant::now() >= deadline {
let _ = std::process::Command::new("kill")
.args(["-9", &pid.to_string()])
.status();
return;
}
std::thread::sleep(std::time::Duration::from_millis(50));
}
}

fn print_up_summary(session: &state::Session, _config: &config::Config, log: &log::StepLogger) {
println!();
log.success(&format!(
Expand Down Expand Up @@ -1029,12 +1059,8 @@ fn print_up_summary(session: &state::Session, _config: &config::Config, log: &lo
fn print_up_json(session: &state::Session, _root: &std::path::Path) -> Result<()> {
let env_file = std::path::Path::new(&session.worktree_path).join(".env.ecluse");
let mut env_vars: serde_json::Map<String, serde_json::Value> = serde_json::Map::new();
if env_file.exists() {
for line in std::fs::read_to_string(&env_file)?.lines() {
if let Some((k, v)) = line.split_once('=') {
env_vars.insert(k.to_string(), serde_json::Value::String(v.to_string()));
}
}
for (k, v) in env::parse_env_file(&env_file) {
env_vars.insert(k, serde_json::Value::String(v));
}
let out = serde_json::json!({
"slug": session.slug,
Expand Down Expand Up @@ -1250,7 +1276,11 @@ fn cmd_ls(args: cli::LsArgs) -> Result<()> {
ports,
tmux: s.tmux_session.clone().unwrap_or_default(),
branch: s.branch.clone(),
started: s.started_at[..16].replace('T', " "),
started: s
.started_at
.get(..16)
.unwrap_or(&s.started_at)
.replace('T', " "),
}
})
.collect();
Expand Down Expand Up @@ -1301,23 +1331,7 @@ fn cmd_shell(args: cli::ShellArgs) -> Result<()> {

let worktree = std::path::Path::new(&session.worktree_path);
let env_file = worktree.join(".env.ecluse");

let env_vars: Vec<(String, String)> = if env_file.exists() {
std::fs::read_to_string(&env_file)
.context("failed to read .env.ecluse")?
.lines()
.filter_map(|line| {
let line = line.trim();
if line.is_empty() || line.starts_with('#') {
return None;
}
let (k, v) = line.split_once('=')?;
Some((k.to_string(), v.to_string()))
})
.collect()
} else {
vec![]
};
let env_vars: Vec<(String, String)> = env::parse_env_file(&env_file);

if let Some(tmux_session) = &session.tmux_session {
println!(
Expand Down Expand Up @@ -1430,12 +1444,8 @@ fn cmd_env(args: cli::EnvArgs) -> Result<()> {
let env_file = std::path::Path::new(&session.worktree_path).join(".env.ecluse");

let mut env_vars: serde_json::Map<String, serde_json::Value> = serde_json::Map::new();
if env_file.exists() {
for line in std::fs::read_to_string(&env_file)?.lines() {
if let Some((k, v)) = line.split_once('=') {
env_vars.insert(k.to_string(), serde_json::Value::String(v.to_string()));
}
}
for (k, v) in env::parse_env_file(&env_file) {
env_vars.insert(k, serde_json::Value::String(v));
}

let out = serde_json::json!({
Expand Down Expand Up @@ -1480,10 +1490,17 @@ fn cmd_sync(args: cli::SyncArgs) -> Result<()> {
let path = if canonical.exists() {
canonical
} else {
// Fall back to the cwd only when it really is a linked git
// worktree of this repo — never on path-string coincidences.
let cwd =
std::env::current_dir().context("could not determine current directory")?;
if cwd.starts_with(&root) || cwd.to_str().is_some_and(|c| c.contains(s.as_str())) {
cwd
let belongs = worktree::is_inside_git_worktree(&cwd)
&& worktree::WorktreeManager::main_worktree_root(&cwd)
.ok()
.and_then(|r| std::fs::canonicalize(r).ok())
== std::fs::canonicalize(&root).ok();
if belongs {
worktree::git_worktree_root(&cwd)?
} else {
return Err(error::EcluseError::WorktreeNotFound { slug: s.clone() }.into());
}
Expand Down
21 changes: 4 additions & 17 deletions src/process.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,24 +176,11 @@ fn tmux_session_exists(session: &str) -> bool {
.unwrap_or(false)
}

/// Parse a .env-style file into key=value pairs. Lines starting with `#`
/// and blank lines are skipped. Values are not unquoted — taken as-is.
/// Parse a .env-style file into key=value pairs.
/// Delegates to the shared parser so spawning, `shell`, `env`, and
/// `up --json` all read env files identically.
fn parse_env_file(path: &Path) -> std::collections::HashMap<String, String> {
let Ok(content) = std::fs::read_to_string(path) else {
return std::collections::HashMap::new();
};
content
.lines()
.filter(|l| !l.trim_start().starts_with('#') && l.contains('='))
.filter_map(|l| {
let (k, v) = l.split_once('=')?;
let k = k.trim().to_string();
if k.is_empty() {
return None;
}
Some((k, v.to_string()))
})
.collect()
crate::env::parse_env_file(path).into_iter().collect()
}

/// Merge .env → .env.local → .env.ecluse from `worktree` into `base`,
Expand Down
50 changes: 27 additions & 23 deletions src/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,32 +160,18 @@ impl StateGuard {
/// Use for commands that only read state: status, ls, env, shell.
pub fn acquire_shared(root: &Path) -> Result<Self> {
let ecluse_dir = root.join(".ecluse");
let lock_path = ecluse_dir.join("state.lock");

// If the lock file doesn't exist yet there's no state either — return empty.
if !lock_path.exists() {
return Ok(StateGuard {
state: State::default(),
state_path: ecluse_dir.join("state.json"),
_lock_file: OpenOptions::new()
.create(true)
.truncate(false)
.write(true)
.open(ecluse_dir.join("state.lock"))
.unwrap_or_else(|_| {
std::fs::create_dir_all(&ecluse_dir).ok();
OpenOptions::new()
.create(true)
.truncate(false)
.write(true)
.open(&lock_path)
.expect("failed to create lock file")
}),
});
}
std::fs::create_dir_all(&ecluse_dir)
.with_context(|| format!("failed to create {}", ecluse_dir.display()))?;

// Create the lock file if missing. state.json may still exist (e.g.
// the lock file was removed by hand) — never report an empty state
// just because the lock file is gone.
let lock_path = ecluse_dir.join("state.lock");
let lock_file = OpenOptions::new()
.read(true)
.write(true)
.create(true)
.truncate(false)
.open(&lock_path)
.with_context(|| format!("failed to open lock file {}", lock_path.display()))?;

Expand Down Expand Up @@ -530,4 +516,22 @@ mod tests {
let json = serde_json::to_string(&s).unwrap();
assert!(!json.contains("services_subset"), "got: {json}");
}

// The lock file can go missing while state.json survives (manual cleanup,
// partial flush). Shared acquisition must recreate the lock and read the
// real state, not silently report "no sessions".
#[test]
fn acquire_shared_reads_state_when_lock_file_missing() {
let dir = TempDir::new().unwrap();
{
let mut guard = StateGuard::acquire(dir.path()).unwrap();
guard.state.add_session(make_session("survivor", 1));
guard.commit().unwrap();
}
std::fs::remove_file(dir.path().join(".ecluse/state.lock")).unwrap();

let guard = StateGuard::acquire_shared(dir.path()).unwrap();
assert_eq!(guard.state.sessions.len(), 1);
assert_eq!(guard.state.sessions[0].slug, "survivor");
}
}
Loading
Loading