Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 1 addition & 3 deletions internal/agent/tools/bash.go
Original file line number Diff line number Diff line change
Expand Up @@ -292,9 +292,7 @@ func (t *BashTool) executeBashWithStreaming(ctx context.Context, cmd *exec.Cmd,
detached = true
detachedMux.Unlock()

time.Sleep(20 * time.Millisecond)

shellID, err := t.backgroundShellService.DetachToBackground(ctx, cmd, result.Command, outputBuffer)
shellID, err := t.backgroundShellService.DetachToBackground(ctx, cmd, result.Command, outputBuffer, done)
if err != nil {
logger.Debug("bash: DetachToBackground failed", "error", err)
result.ExitCode = -1
Expand Down
8 changes: 6 additions & 2 deletions internal/domain/shell.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ type BackgroundShell struct {
OutputBuffer OutputRingBuffer
CancelFunc context.CancelFunc
ReadOffset int64

ReadersDone <-chan struct{}
}

// OutputRingBuffer defines the interface for the circular output buffer.
Expand Down Expand Up @@ -110,8 +112,10 @@ func NewShellInfo(shell *BackgroundShell) *ShellInfo {

// BackgroundShellService defines the interface for managing background shells
type BackgroundShellService interface {
// DetachToBackground moves a running command to background
DetachToBackground(ctx context.Context, cmd *exec.Cmd, command string, outputBuffer OutputRingBuffer) (string, error)
// DetachToBackground moves a running command to background. readersDone, when
// non-nil, is closed once the caller's pipe readers reach EOF; the supervisor
// waits on it before reaping so trailing output is not truncated.
DetachToBackground(ctx context.Context, cmd *exec.Cmd, command string, outputBuffer OutputRingBuffer, readersDone <-chan struct{}) (string, error)

// GetShellOutput retrieves output from a shell
GetShellOutput(shellID string, fromOffset int64) (string, int64, ShellState, error)
Expand Down
2 changes: 2 additions & 0 deletions internal/services/background_shell_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ func (s *BackgroundShellService) DetachToBackground(
cmd *exec.Cmd,
command string,
outputBuffer domain.OutputRingBuffer,
readersDone <-chan struct{},
) (string, error) {
if !s.config.Tools.Bash.BackgroundShells.Enabled {
return "", fmt.Errorf("background shells are disabled in configuration")
Expand All @@ -66,6 +67,7 @@ func (s *BackgroundShellService) DetachToBackground(
State: domain.ShellStateRunning,
OutputBuffer: outputBuffer,
ReadOffset: 0,
ReadersDone: readersDone,
}

if err := s.shellTracker.Add(shell); err != nil {
Expand Down
9 changes: 8 additions & 1 deletion internal/services/directexec/bash.go
Original file line number Diff line number Diff line change
Expand Up @@ -367,18 +367,25 @@ func (s *Service) executeBashCommandInBackground(commandText, command string) te
}
}()

done := make(chan struct{})
go func() {
wg.Wait()
close(done)
}()

shellID, err := s.backgroundShellService.DetachToBackground(
ctx,
cmd,
command,
outputBuffer,
done,
)

if err != nil {
return
}

wg.Wait()
<-done

assistantEntry := domain.ConversationEntry{
Message: sdk.Message{
Expand Down
11 changes: 10 additions & 1 deletion internal/services/jobs/shell_job.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,17 @@ func (j *shellJob) Meta() domain.JobMeta {
}

// Run waits for the shell process to exit and records the outcome on the shell.
// It first waits for the pipe readers to drain (ReadersDone) so Cmd.Wait doesn't
// close the pipes mid-read and lose output; the wait honours ctx so a kill or
// shutdown can't wedge when a grandchild is holding the pipe open.
func (j *shellJob) Run(ctx context.Context, _ func(domain.JobSignal)) domain.ToolExecutionResult {
if j.shell.ReadersDone != nil {
select {
case <-j.shell.ReadersDone:
case <-ctx.Done():
}
}

waitErr := j.shell.Cmd.Wait()

now := time.Now()
Expand All @@ -49,7 +59,6 @@ func (j *shellJob) Run(ctx context.Context, _ func(domain.JobSignal)) domain.Too

switch {
case ctx.Err() != nil:
// Terminated by Wind(WindStop) / supervisor shutdown rather than finishing.
code := -1
j.shell.ExitCode = &code
j.shell.State = domain.ShellStateCancelled
Expand Down
65 changes: 65 additions & 0 deletions internal/services/jobs/shell_job_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,71 @@ func waitTerminal(t *testing.T, sup *Supervisor, id string) domain.JobStatus {
return ""
}

// isTerminal reports whether the job is currently in a terminal state without
// blocking, for asserting that a job has NOT yet finished.
func isTerminal(sup *Supervisor, id string) bool {
for _, j := range sup.Snapshot() {
if j.Meta.ID == id {
return j.Status.IsTerminal()
}
}
return false
}

// TestShellJob_WaitsForReadersDoneBeforeReaping asserts Run does not call
// Cmd.Wait until the pipe readers have drained (ReadersDone closed), even after
// the process has already exited — otherwise Wait closes the pipes mid-drain and
// truncates trailing output.
func TestShellJob_WaitsForReadersDoneBeforeReaping(t *testing.T) {
tracker := utils.NewShellTracker(10)
sup := NewSupervisor(&domainmocks.FakeMessageQueue{}, &domainmocks.FakeConversationRepository{}, nil)
defer sup.Stop()

// `true` exits immediately; the job must still park on ReadersDone.
shell := startShell(t, "s-drain", "true")
readersDone := make(chan struct{})
shell.ReadersDone = readersDone
_ = tracker.Add(shell)
sup.Submit(NewShellJob(shell, tracker))

time.Sleep(50 * time.Millisecond)
if isTerminal(sup, "s-drain") {
t.Fatal("shell reaped before the pipe readers signalled done")
}

close(readersDone)
if got := waitTerminal(t, sup, "s-drain"); got != domain.JobCompleted {
t.Fatalf("status = %s, want completed", got)
}
if shell.State != domain.ShellStateCompleted {
t.Fatalf("shell state = %s, want completed", shell.State)
}
}

// TestShellJob_WindStopUnblocksReadersWait guards the cancellable-wait escape:
// when ReadersDone never closes (e.g. a grandchild holds the pipe open),
// Wind(WindStop) must still reap the job via ctx cancellation rather than
// wedging Run — which would also hang Supervisor.Stop().
func TestShellJob_WindStopUnblocksReadersWait(t *testing.T) {
tracker := utils.NewShellTracker(10)
sup := NewSupervisor(&domainmocks.FakeMessageQueue{}, &domainmocks.FakeConversationRepository{}, nil)
defer sup.Stop()

shell := startShell(t, "s-stuck", "sleep", "60")
shell.ReadersDone = make(chan struct{}) // never closed
_ = tracker.Add(shell)
sup.Submit(NewShellJob(shell, tracker))

time.Sleep(20 * time.Millisecond)
if err := sup.Wind("s-stuck", domain.WindStop); err != nil {
t.Fatalf("Wind: %v", err)
}
waitTerminal(t, sup, "s-stuck")
if shell.State != domain.ShellStateCancelled {
t.Fatalf("shell state = %s, want cancelled", shell.State)
}
}

func TestShellJob_CompletesAndNotifies(t *testing.T) {
tracker := utils.NewShellTracker(10)
queue := &domainmocks.FakeMessageQueue{}
Expand Down
18 changes: 10 additions & 8 deletions tests/mocks/domain/fake_background_shell_service.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.