fix(backgroundjobs): resolve status race condition and subprocess pipe hang

[Piyush0049]

Summary

This pull request resolves a critical status race condition in background jobs and eliminates process hangs caused by inherited I/O pipes from background subprocesses. It also ensures cross-platform test reliability on Windows and silences platform-specific linter false-positives.

Root Cause & Changes

1. Atomic Status Transitions (CompareAndSwap)

• Issue: Previously, monitorJob used blind status writes (job.status.Store(...)). If StopBackgroundJob or ToolSet.Stop terminated a job at the exact moment a process was exiting naturally, monitorJob could overwrite statusStopped with statusCompleted or statusFailed. This caused erroneous recall steering messages to be sent back to the AI model for manually aborted jobs.
• Fix: Replaced blind store assignments in monitorJob with atomic Compare-And-Swap (CAS) transitions (job.status.CompareAndSwap(statusRunning, newStatus)). Once a job transitions out of statusRunning, subsequent background monitoring completions cannot overwrite the terminated state.

2. Piped Subprocess Cleanup (cmd.WaitDelay)

• Issue: When background commands spawned child or grandchild processes that inherited open stdout/stderr file descriptors (such as backgrounded scripts or daemons), Go's cmd.Wait() would block indefinitely waiting for an EOF on the pipes even after the primary process terminated.
• Fix: Configured cmd.WaitDelay = 1 * time.Second on all executed background jobs to automatically force-close orphaned I/O pipes if child processes hold them open after the primary process exits.

3. Cross-Platform Compatibility & Linting

• Test Reliability: Updated TestResolveWorkDir in backgroundjobs_test.go to use filepath.FromSlash and conditional OS root prefixes (C:\... on Windows) to prevent false-positive path resolution failures on Windows.
• Linting Harmony: Updated exec_windows.go to use exec.CommandContext and wrapped secondary fallback errors with %w. Added ,nolintlint to cross-platform silencing directives (//nolint:gosec in snapshot.go and //nolint:bodyclose in transport_test.go) so that running golangci-lint on Windows does not flag them as unused.

[aheritier]

Hi @Piyush0049

Please provide more details about the issue you are trying to solve. Is it something which happens often? What are the symptoms?

Right now the PR doesn't provide enough context to understand if you are fixing a bug or if these are just code reviews by an LLM which could easily hallucinate bugs.

Thanks

[Piyush0049]

Hi @aheritier,

• Official Go Docs: pkg.go.dev/os/exec#Cmd.WaitDelay

"If WaitDelay is zero (the default), Wait waits indefinitely for I/O to complete."

While reproducing this, I noticed two separate issues:

1. cmd.Wait() can hang

If a child/background process keeps the inherited stdout/stderr pipes open, cmd.Wait() can block indefinitely because WaitDelay is 0 by default.

Setting:

cmd.WaitDelay = time.Second

prevents Wait() from hanging forever in this scenario.

2. Status race condition

There also seems to be a race between StopBackgroundJob() and monitorJob().

If the user stops a job while monitorJob() is finishing, the final Store(statusFailed) can overwrite statusStopped.

Using:

job.status.CompareAndSwap(statusRunning, newStatus)

ensures monitorJob() only updates the status if the job is still running, so a manually stopped job remains stopped.

[docker-agent]

Assessment: 🟢 APPROVE

The changes are well-structured and correctly address the stated goals:

• CAS in monitorJob: The mutex acquisition and CompareAndSwap ordering is correct. The mutex is held at the point of the early unlock on CAS failure — no double-unlock or missed unlock. The CAS prevents monitorJob from overwriting a statusStopped state set by StopBackgroundJob.
• cmd.WaitDelay: Appropriately handles orphaned child processes holding open pipes. ErrWaitDelay (non-ExitError) is correctly routed to statusFailed with exit code -1.
• Multiple %w in fmt.Errorf: Supported since Go 1.20; this project targets Go 1.26.5 — no issue.
• exec.CommandContext: The gosec G204 nolint removal is safe because G204 is globally excluded in .golangci.yml.
• nolintlint additions: Correctly suppress the nolintlint false-positives on Windows builds where the guarded linters don't run.

[Sayt-0]

Thanks for the detailed follow-up. The CAS fix is sound, but the WaitDelay change introduces a behavioral regression for this specific toolset, and the PR bundles several unrelated changes. Details inline; summary below.

Change	Assessment
CAS in monitorJob	Correct, real (narrow) race, matches existing CAS usage in Stop
cmd.WaitDelay = 1s	Regression: ErrWaitDelay mapped to failed/-1, and pipe force-close kills backgrounded servers (SIGPIPE)
TestResolveWorkDir rewrite	Reverts a deliberate portability commit (8f227f7); the current test passes on the blocking windows-tests CI job
selfupdate / snapshot / transport lint changes	Unrelated to the stated fix, CI lints on Linux only; belongs in a separate chore PR

Blocking points:

1. exec.ErrWaitDelay is not an *exec.ExitError, so a job whose command exited 0 while a grandchild holds the pipes gets statusFailed with exit code -1, and (with recall enabled) a "failed" steering message. This is the exact symptom the PR aims to fix. Reproduced locally: err=ErrWaitDelay, isExitError=false, ProcessState.ExitCode()=0.
2. Force-closing the pipes SIGPIPE-kills a grandchild that keeps writing (reproduced locally: writer dead ~1s after shell exit, output capture truncated). run_background_job is documented for servers and watchers, so mycommand & or daemonizing commands would see the server killed and the job marked failed within a second. The shell tool comparison does not transfer: a shell call must return promptly, monitorJob is a goroutine and wait_background_job already has its own timeout.
3. No regression tests for either claimed bug. The equivalent shell-tool fix (35bb084) shipped with two dedicated tests.

Suggested scope to make this mergeable:

• Keep the CAS change (and drop the now-redundant statusStopped early return).
• Either drop WaitDelay for this toolset, or handle errors.Is(err, exec.ErrWaitDelay) explicitly (derive the status from cmd.ProcessState.ExitCode()), document the SIGPIPE / lost-output trade-off, and add regression tests.
• Revert the TestResolveWorkDir changes, or attach a CI log showing the current test failing on Windows.
• Move the selfupdate/snapshot/transport lint changes to a separate PR.

[Sayt-0]

The shell tool's twin constant (waitDelayAfterShellExit, shell.go) carries a detailed comment explaining the pipe/copy-goroutine mechanics and why the value is safe there. If a delay is kept here, a similar comment is needed, including why 1s is appropriate for long-running jobs, since the trade-offs differ (see comment on the assignment below).

[Sayt-0]

This assignment changes behavior for a documented use case of this toolset.

Scenario: run_background_job with cmd: "myserver &" (or any command that daemonizes). The direct shell child exits 0 immediately, the grandchild inherits the stdout/stderr pipes.

Behavior	Before	After
Job status	running (grandchild is the job)	failed, exit code -1, after ~1s
Output capture	continues streaming	stops after ~1s
Grandchild	keeps running	SIGPIPE on next write, typically killed
Recall message	none until real completion	"finished with status failed" for a healthy server

Reproduced locally with a minimal exec.Cmd harness: Wait() returns exec.ErrWaitDelay with ProcessState.ExitCode() == 0, and a grandchild writing to the pipe dies within ~1s of the forced close.

The shell tool needs WaitDelay because the tool call must return promptly. Here monitorJob runs in its own goroutine and wait_background_job already has a timeout, so a blocked Wait() does not block any caller. If the goal is to avoid a permanently "running" job after the direct child exits, that needs a design that neither kills the grandchild nor reports a false failure.

[Sayt-0]

The CAS itself is correct and consistent with StopBackgroundJob and ToolSet.Stop; it closes the window between the statusStopped load and the store (Stop does its CAS without holding outputMu). Two follow-ups:

1. The if job.status.Load() == statusStopped early return above is now redundant: the CAS already fails for any non-running status. It can be dropped to keep a single exit path.

2. With WaitDelay set, ErrWaitDelay is not an *exec.ExitError, so the else branch classifies a successful command (exit 0, pipes held open) as statusFailed with exit code -1. If WaitDelay stays, this needs an explicit branch, e.g.:

   if err != nil && errors.Is(err, exec.ErrWaitDelay) {
       job.exitCode = cmd.ProcessState.ExitCode()
       if job.exitCode == 0 {
           newStatus = statusCompleted
       } else {
           newStatus = statusFailed
       }
   }

   plus a regression test covering it (see 35bb084 for the shell-tool equivalent tests).

[Sayt-0]

This reverts commit 8f227f7 ("test: use portable filesystem expectations"), which deliberately replaced hard-coded /configured/project paths with t.TempDir() for portability. resolveWorkDir never touches the filesystem, and t.TempDir() returns valid absolute paths on Windows, so the current expectations (filepath.Join, filepath.Dir) hold there too. The windows-tests CI job is blocking and green on main, which would not be the case if this test failed on Windows.

Unless a CI log shows an actual Windows failure, this hunk should be dropped.

[Sayt-0]

exec.CommandContext with context.Background() is behaviorally identical to exec.Command (the context never cancels), so this only silences noctx on local Windows lint runs. The repo convention for intentionally context-free commands is //nolint:noctx with a reason (see backgroundjobs.go and shell.go). The change also drops the informative // path is our own freshly installed binary comment.

Suggestion: keep exec.Command and, if the Windows lint noise matters, add //nolint:noctx // re-exec outlives any request context in a separate lint PR.

[Sayt-0]

Fine on its own (multiple %w is supported since Go 1.20, and it makes cpErr/rbErr matchable via errors.Is/As), but unrelated to the backgroundjobs fix. Better suited to the separate lint/chore PR.

[Sayt-0]

CI runs golangci-lint on ubuntu only, so these ,nolintlint suffixes address local Windows lint runs that CI cannot verify (applies to all three occurrences here and to transport_test.go). The repo already has an established pattern for platform/context-specific nolintlint false positives: a path exclusion in .golangci.yml (see the pkg/worktree/namesgenerator/ rule). Either approach is defensible, but this belongs in a dedicated lint PR with the reproduction (golangci-lint version + GOOS) in the description.

[Sayt-0]

Same remark as snapshot.go (applies to shell/cmd_windows.go too): Windows-only files are never linted in CI, so these annotations are unverifiable here. Also inconsistent with the other hunks: no ,nolintlint suffix, so if gosec does not emit these findings on some setups, nolintlint would flag the directive as unused. Should move to the dedicated lint PR with the exact rule IDs (G103/G115) confirmed.

[Piyush0049]

@Sayt-0 I have updated this PR too.