Skip to content

fix: Agent on_complete hooks can hang ask forever and leak subprocesses#828

Closed
sam-saffron-jarvis wants to merge 1 commit into
SamSaffron:mainfrom
sam-saffron-jarvis:feat/codereview-eead00d3
Closed

fix: Agent on_complete hooks can hang ask forever and leak subprocesses#828
sam-saffron-jarvis wants to merge 1 commit into
SamSaffron:mainfrom
sam-saffron-jarvis:feat/codereview-eead00d3

Conversation

@sam-saffron-jarvis

Copy link
Copy Markdown
Contributor

What changed

  • wrapped runOnCompleteCapture in a bounded context.WithTimeout so ask no longer waits forever on a stuck agent.on_complete hook
  • switched on_complete execution to exec.CommandContext plus tools.PrepareCommand, which applies the same process-group cancellation/cleanup used by other command execution paths
  • set WaitDelay for on_complete so ask also returns when the parent shell exits but a backgrounded child keeps stdout/stderr open
  • added focused tests covering both a hung hook timeout and the background-child/stdout-pipe hang case

Why this is high-value

agent.on_complete runs after the model has already successfully finished. Before this change, a slow or misbehaving hook could still leave term-llm ask hung indefinitely at the very end, which is especially painful for scripts and automation. The old path also had no process-group cleanup, so broken hooks could leave stray subprocesses behind.

This fix makes that tail-end hook fail open: the main ask result still completes, while timeout/failure continues to surface as the existing non-fatal warning.

Validation

  • gofmt -w cmd/ask.go cmd/ask_test.go
  • go build ./...
  • go test ./cmd -run 'TestRunOnCompleteCapture_'
  • go test ./...
  • git diff --stat
  • git diff --check

@SamSaffron

Copy link
Copy Markdown
Owner

Implemented directly in main working tree with the same fixes and regression coverage.

@SamSaffron SamSaffron closed this Jun 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants