Skip to content

fix: guarantee bounded authentication and skill shutdown - #144

Closed
GautamSharma99 wants to merge 1 commit into
openai:mainfrom
GautamSharma99:fix/bounded-child-shutdown
Closed

fix: guarantee bounded authentication and skill shutdown#144
GautamSharma99 wants to merge 1 commit into
openai:mainfrom
GautamSharma99:fix/bounded-child-shutdown

Conversation

@GautamSharma99

Copy link
Copy Markdown
Contributor

Summary

Fixes #131.

Make authentication and skill subprocess cancellation settle within a bounded deadline, even when the immediate child ignores the cooperative termination signal or a descendant keeps inherited output pipes open.

Previously, CodexLoginHandle.cancel() sent SIGTERM once and CodexSecurity.close() waited on the login handle without a deadline. The CLI skill path used the same one-signal behavior. A non-cooperative child or inherited pipe could therefore leave SDK or CLI cleanup pending indefinitely.

Changes

Bounded login cancellation

Interactive login cancellation now uses a two-stage shutdown:

  1. mark the login as canceled and send SIGTERM;
  2. allow a one-second cooperative grace period;
  3. if the child is still running, escalate to SIGKILL;
  4. destroy the remaining stdin, stdout, and stderr streams;
  5. force the login completion to settle if process events or inherited pipes still do not do so.

The forced completion remains an unsuccessful canceled login, so the authentication success callback cannot run after cancellation.

Repeated calls to cancel() reuse the existing deadline instead of creating additional timers.

Cross-platform inherited-pipe fallback

The existing post-exit pipe fallback was Windows-only. It now applies on every platform.

After the immediate login child exits, normal pipe draining still receives a one-second grace period. If a descendant continues holding stdout or stderr open after that deadline, the SDK destroys the streams and settles the login using the immediate child's exit status.

This preserves ordinary diagnostic draining while preventing descendants from keeping login.wait() and CodexSecurity.close() alive forever.

Timer and listener cleanup

Login completion and spawn failure now clear both the post-exit fallback and forced-termination timer. Process pipes are released on every terminal path, and late error, exit, or close events cannot settle the same login twice.

Bounded CLI signal forwarding

runCodexSkillCommand now applies the same one-second termination deadline when forwarding SIGINT or SIGTERM:

  • the original signal is forwarded first;
  • a child that remains alive is escalated with SIGKILL;
  • captured stdout and stderr are destroyed after the grace period;
  • the JSONL capture wait and process-status wait are explicitly released;
  • conventional exit status is preserved as 130 for SIGINT and 143 for SIGTERM;
  • signal listeners and forced-termination timers are removed on every exit path.

Explicitly releasing the capture wait is necessary because destroying a stream alone does not reliably settle every parser or inherited-pipe configuration.

Impact

SDK consumers can now rely on CodexSecurity.close() completing after it cancels an interactive login. CLI skill commands also return after cancellation instead of waiting indefinitely for an uncooperative child or inherited output pipe.

The cooperative path is unchanged: children still receive the original signal and have time to clean up normally before forced termination occurs.

Tests

Added and updated regression coverage for:

  • a login child that installs a SIGTERM handler and refuses to exit;
  • direct CodexLoginHandle.cancel() completion within a bounded deadline;
  • CodexSecurity.close() completion when its active login ignores SIGTERM;
  • preservation of an unsuccessful login result and suppression of the success callback;
  • post-exit inherited login-pipe cleanup through the cross-platform fallback;
  • a real CLI wrapper receiving SIGTERM while its skill child ignores the signal;
  • a descendant retaining the skill child's captured stdout and stderr;
  • forced CLI completion with exit code 143;
  • cleanup of all fixture subprocesses after the process-level test.

Verification

  • pnpm run types
  • pnpm run format
  • pnpm run build
  • focused authentication, SDK close, and CLI skill tests
  • PATH="/opt/homebrew/bin:$PATH" pnpm run test

Full test result:

  • 472 passed
  • 6 expected platform/integration skips
  • 0 failed

@mldangelo-oai

Copy link
Copy Markdown
Collaborator

Thanks for working through bounded shutdown for authentication and skills. The corresponding shutdown handling is already implemented on main, so this PR no longer has a separate fix for us to merge. I'm closing it as superseded. We'd be happy to review future work in this area.

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.

SDK close can hang indefinitely when a login child ignores SIGTERM

2 participants