Follow-up from #126 part B (#160).
fg.rs (lines ~107-112) has the textually identical ordering bug that was
just fixed in bg.rs: manager.resume_job(job_id).await (clearing the job's
Stopped flag, marking it JobStatus::Running) runs before
killpg(pgid, SIGCONT). If the signal fails (e.g. the process already died),
the job is left looking Running with no live process.
// fg.rs, current
if let Err(e) = term.give_terminal_to(pgid) {
return ExecResult::failure(1, format!("fg: failed to give terminal: {}", e));
}
// Mark as resumed and send SIGCONT
manager.resume_job(job_id).await;
if let Err(e) = nix::sys::signal::killpg(pgid, nix::sys::signal::Signal::SIGCONT) {
let _ = term.reclaim_terminal();
return ExecResult::failure(1, format!("fg: failed to continue job: {}", e));
}
Deliberately left out of #160's scope (which was bg.rs only, per #126's own
wording) because fg has more surface to get right than a mechanical
reorder:
term.give_terminal_to(pgid) also runs before killpg — handing the
terminal to a process group that's about to fail to resume (or is already
dead) is arguably its own instance of the same bug. A correct fix likely
needs to confirm SIGCONT before both resume_job() and the terminal
handoff, with reclaim_terminal() called on every failure path (it already
is on the SIGCONT failure branch, but the ordering needs to be rethought
end to end).
- Unlike
bg, fg cannot be unit-tested without a real controlling terminal
(TerminalState::init() calls real tcsetpgrp/setpgid and has no
test-friendly constructor) — a regression test for this will need to be
PTY-based, extending crates/kaish-repl/tests/pty_job_control.rs.
Flagged during kaibo review of #160 (cast: deepseek), which agreed the split
was reasonable given the added complexity here.
Follow-up from #126 part B (#160).
fg.rs(lines ~107-112) has the textually identical ordering bug that wasjust fixed in
bg.rs:manager.resume_job(job_id).await(clearing the job'sStopped flag, marking it
JobStatus::Running) runs beforekillpg(pgid, SIGCONT). If the signal fails (e.g. the process already died),the job is left looking Running with no live process.
Deliberately left out of #160's scope (which was
bg.rsonly, per #126's ownwording) because
fghas more surface to get right than a mechanicalreorder:
term.give_terminal_to(pgid)also runs beforekillpg— handing theterminal to a process group that's about to fail to resume (or is already
dead) is arguably its own instance of the same bug. A correct fix likely
needs to confirm
SIGCONTbefore bothresume_job()and the terminalhandoff, with
reclaim_terminal()called on every failure path (it alreadyis on the
SIGCONTfailure branch, but the ordering needs to be rethoughtend to end).
bg,fgcannot be unit-tested without a real controlling terminal(
TerminalState::init()calls realtcsetpgrp/setpgidand has notest-friendly constructor) — a regression test for this will need to be
PTY-based, extending
crates/kaish-repl/tests/pty_job_control.rs.Flagged during kaibo review of #160 (cast: deepseek), which agreed the split
was reasonable given the added complexity here.