Skip to content

Comments

fix(codexexec): ensure context cancellation reliably terminates spawned codex process#25

Merged
activadee merged 2 commits intomainfrom
codex/ensure-context.context-cancellation-reliably-terminates-the-spawned-codex-43
Nov 6, 2025
Merged

fix(codexexec): ensure context cancellation reliably terminates spawned codex process#25
activadee merged 2 commits intomainfrom
codex/ensure-context.context-cancellation-reliably-terminates-the-spawned-codex-43

Conversation

@activadee
Copy link
Owner

@activadee activadee commented Nov 6, 2025

Summary

  • Propagate context cancellation from codexexec.Runner.Run to callers.
  • Prefer returning ctx.Err on cancellation over wrapped read/wait errors.
  • Ensure the spawned codex process terminates reliably when the context is canceled.

Details

  • Map scanner/read and cmd.Wait() cancellation cases to context.Canceled/context.DeadlineExceeded as appropriate.
  • When ctx is canceled, return ctx.Err() even if read/wait surfaced a different error due to process teardown.
  • Keeps stderr buffering and exit-code reporting behavior for genuine failures.

Tests

  • Add thread_cancel_test.go: integration test that cancels a streaming run and asserts:
    • Wait() returns context.Canceled
    • The spawned process exits
  • Add internal/codexexec/testdata/fakecodex: a tiny fake codex binary that writes its PID and blocks until receiving SIGINT/SIGTERM.
  • Skip on Windows since the test relies on Unix signals.

Notes

activadee added 2 commits November 6, 2025 20:57
- Build a tiny fake codex binary that writes its PID and waits for a signal
- Verify RunStreamed cancellation returns context.Canceled
- Assert the spawned process terminates on cancellation
- Prefer ctx.Err when canceled over read/wait errors
- Map scanner/Wait cancellations to context.Canceled/DeadlineExceeded
- Ensure consistent termination semantics for spawned codex process
@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2025

Codex Review Summary

Recommendation: ✅ Ready to merge – no blocking issues detected.

Summary:
Reviewed the updated cancellation handling in internal/codexexec/runner.go along with the new fake Codex helper and integration test; the context error propagation now aligns with the caller’s expectations and the test convincingly verifies the child process is terminated. I don’t see any correctness, stability, or maintainability issues in this PR.

Inline findings:

  • No inline findings.

Generated at 2025-11-06T20:03:37.247Z.

@activadee activadee merged commit 75b9a3e into main Nov 6, 2025
2 checks passed
@activadee activadee deleted the codex/ensure-context.context-cancellation-reliably-terminates-the-spawned-codex-43 branch November 6, 2025 20:04
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.

Correctness: reliable context cancellation tears down CLI process

1 participant