fix: bound app-server waits, identify processes before killing them, and make the stop gate converge - #1
Open
YmSaki wants to merge 5 commits into
Open
fix: bound app-server waits, identify processes before killing them, and make the stop gate converge#1YmSaki wants to merge 5 commits into
YmSaki wants to merge 5 commits into
Conversation
Fixes a family of first-start hangs and blind spots observed on Windows (Git Bash) where a cold `codex app-server` start could stall forever with no output, no error, and no entry in /codex:status: - app-server: add per-request timeout support to the JSON-RPC client. The initialize handshake is now bounded (60s default, override via CODEX_COMPANION_HANDSHAKE_TIMEOUT_MS) on both direct and broker transports, and control-plane requests (thread/start, thread/resume, thread/list, account/read, config/read, turn/interrupt, name/set, external-agent import) are bounded at 2 minutes. Streaming requests (turn/start, review/start) stay unbounded. On timeout the captured codex stderr is attached to the error, the wedged process/socket is torn down so the CLI can exit, and broker timeouts fall back to a direct runtime. - broker: add a 10s connect timeout, abort the startup wait as soon as the broker child exits, and raise the cold-start wait from 2s to 10s so slow starts stop silently falling back to per-call direct spawns. - status: jobs recorded without a session id are no longer hidden from every session-scoped view (they were invisible and uncancellable); /codex:status --all now bypasses the session filter entirely. - progress: connect/retry phases now emit progress events with a 30s heartbeat, so a stalled startup is distinguishable from a dead one in the job log and status phase. - windows: spawn shell commands as a single quoted string instead of an args array (silences DEP0190, avoids unquoted joining), and fix the stale cmd.exe assumption comment in close(). - state: write state.json, job files, and broker.json atomically (tmp+rename) to stop concurrent writers corrupting job records. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5cuwDkeEdktKwmZQhu4ko
YmSaki
marked this pull request as ready for review
August 23, 2026 17:20
Addresses review feedback on the timeout work: bounding the waits made failures visible, but three paths still leaked or lost work. - process: taskkill now runs with shell:false. It was inheriting the Windows default shell, so under Git Bash/MSYS the `/PID` and `/T` switches were rewritten into filesystem paths, every kill silently failed, and the process we meant to terminate was orphaned. - process: add resolveDefaultShell(). SHELL is still honored on Windows (upstream added that for Git Bash), but a POSIX-style value such as `/bin/bash` cannot be spawned on Windows and made every command fail with ENOENT; those now fall back to ComSpec/System32 cmd.exe. - broker: ensureBrokerSession defaults killProcess to terminateProcessTree. A broker that never became reachable had its files deleted but its process left running -- and since the session record was cleared, nothing could find it again. Stale sessions recovered from disk are only killed when the broker's pid file still claims that pid, so a reused pid is never targeted. - app-server: connect() tags failures with brokerAttempted/brokerEndpoint. Once connect() rejected there was no client left to inspect, so a broker that failed to connect or handshake looked like a direct-runtime failure and never fell back to a direct runtime. - codex: fall back to a direct runtime on any broker connect failure, and reclaim the unreachable broker (kill + clear its session) first. A busy broker is healthy and is left alone. - state: guard load-mutate-save with a cross-process lock (atomic mkdir, stale-lock reclaim, and a bounded wait that degrades instead of blocking). Atomic writes alone only prevented torn reads; interleaved read-modify-write cycles still dropped jobs. Tests: named-pipe-aware broker fixture so the timeout tests run on Windows too (they were all skipped there, on a Windows-reported bug), plus new coverage for broker reclamation, pid-file verification, shell resolution, taskkill invocation, and concurrent job registration. CI now runs on ubuntu-latest and windows-latest. Each new test was confirmed to fail against the pre-fix code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5cuwDkeEdktKwmZQhu4ko
An adversarially-verified sweep of the plugin surfaced five more defects in the same reliability family as the RPC timeouts, including a second infinite-hang path the timeouts could not cover. - codex: captureTurn now settles when the runtime disconnects mid-turn. Once turn/start answered there were no pending requests left, so a crashed app-server or killed broker rejected nothing: `await state.completion` waited forever (rejectCompletion was dead code), the job stayed "running", and the user saw no output and no error. A reproduction hung for the full 20s test deadline before this change. - broker: shut down when the backing codex app-server exits. The broker kept advertising a healthy endpoint that clients connected to and then waited on forever -- the other half of the same hang. - job-control: reconcile jobs whose worker pid is gone. A worker killed or crashed mid-run left its job pinned to "running" forever, so /codex:status reported progress that would never arrive and /codex:result refused to show anything. - cancel: only signal a pid that is actually running. terminateProcessTree signals a process group, so cancelling a long-dead worker could kill whatever unrelated process had since inherited the recycled pid. - stop gate: honor stop_hook_active. Nothing stopped the gate from reviewing again on the very turn it had just forced, which is how the Claude/Codex loop the README warns about starts. Infrastructure failures (timeout, bad JSON, spawn failure) now fail open with a note instead of holding the session hostage when Codex is unavailable, and the block decision is also emitted via hookSpecificOutput for current Claude Code. - args: stop shell-tokenizing prose. Every apostrophe opened a quote that swallowed the rest of the prompt (silently absorbing any flag after it) and every backslash was dropped, so "what's wrong here --background" became one mangled token and C:\Users\me became C:Usersme before Codex ever saw it. - background tasks: write the job file before spawning the worker. The worker reads its request from that file, so a fast worker could fail with "missing task request payload" and strand the job in "queued". Tests: new coverage for mid-turn disconnect (end-to-end, via a fake codex that dies after answering turn/start), stale-job reconciliation, pid liveness, and prose tokenization. Each new test was confirmed to fail against the pre-fix code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5cuwDkeEdktKwmZQhu4ko
…nverge Second review round: the previous fixes closed the reported paths but introduced two of their own, and left one Windows failure standing. - process: identify a process by pid AND start time. Comparing broker.json's pid against the pid file proved nothing -- both are stale artifacts written together, so after the broker died and its pid was recycled they still agreed while naming an unrelated process. isProcessAlive had the mirror problem: it answers "yes" about whatever now holds the number. Cancel and broker teardown now require the recorded start time to match before signalling, which matters because terminateProcessTree kills a whole process group. Records written before identities existed fall back to the old liveness check rather than becoming impossible to cancel. - broker: only signal the freshly spawned child when its handle still shows it running; an exited child's pid may already belong to something else. - stop gate: converge instead of skipping. Treating stop_hook_active as "allow" meant Codex's findings were never re-checked, so the gate passed anything on its second pass. The review target is now fingerprinted: an unchanged tree re-uses the stored verdict (no extra Codex run, and a block stays a block), a changed tree earns a fresh review, and the gate gives up after 3 rounds so a disagreement ends with the user instead of more usage. - stop gate: block via the documented top-level decision/reason pair only. The previous commit also emitted hookSpecificOutput and described it as the current format, which is backwards. - process: treat taskkill exit 128 as "already gone". The message-text check is English-only, so on a Japanese Windows an already-exited pid threw -- and handleCancel never caught it, so the job was left un-cancelled. The kill is now best-effort and the cancelled state is always recorded. - process: resolve the Windows spawn wrapper from SystemRoot instead of trusting SHELL or ComSpec. Wrapping a JSON-RPC stdio pipe in an arbitrary user shell is what hangs the handshake with no surfaced error; SHELL still reaches Codex through the inherited env, so its own command execution is unchanged. Verified against upstream: PRs openai#484, openai#577 and openai#580 do exist as open pull requests (fetched via refs/pull/N/head) and take these same approaches. The earlier claim that they could not be found was checked against upstream main, which by definition cannot contain unmerged work. Tests: new coverage for pid-reuse rejection, the legacy-record fallback, the taskkill exit code, the Windows shell resolution, and all four stop-gate outcomes (block, unchanged-reuse, fixed-and-cleared, give-up). The stop-gate tests fail against the pre-fix hook. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M5cuwDkeEdktKwmZQhu4ko
YmSaki
changed the title
Adding a windows-latest CI matrix surfaced 19 failures on real Windows. Verified on the actual dev box (Git Bash SHELL, real Codex): the runtime itself works -- cold-start handshake ~164ms, broker start ~514ms recording a process start-time, warm reuse ~31ms, PowerShell process-identity rejecting a tampered start-time. The failures were harness portability plus two genuine runtime gaps. Runtime fixes: - broker-endpoint: build the Unix-socket path with path.posix.join, not the platform default. On a Windows host path.join spliced in a backslash, producing an endpoint the socket layer cannot bind. - claude-session-transfer: honor CLAUDE_CONFIG_DIR for the import containment check instead of hardcoding os.homedir()/.claude. Claude Code relocates its config dir with that variable, and os.homedir() ignores HOME on Windows (it uses USERPROFILE), so the check rejected valid transcripts. Test-harness fixes (POSIX behavior unchanged): - stop-gate: resolve the hook path with fileURLToPath, not new URL().pathname -- the latter yields "/C:/..." on Windows, which path.resolve turns into "C:\C:\...", so the spawned hook module was never found. This alone broke all four stop-gate tests; the broker/app-server chain they exercise works on Windows. - job-visibility: use a file:// URL (not a bare Windows path) as a dynamic import specifier, and clear an ambient CODEX_COMPANION_SESSION_ID so the "no current session" case is genuine. - helpers: linkNodeExecutable (node.cmd shim where symlinks need privilege) and canCreateSymlinks (skip guard); isolate initGitRepo from the developer's global gitignore via an empty core.excludesFile (many exclude .claude/ globally, which hid the untracked paths a review-context test creates). - state / runtime: clear ambient CLAUDE_PLUGIN_DATA where a test asserts the os.tmpdir fallback, and set CLAUDE_CONFIG_DIR in the transfer tests. - fixture: resolve the Codex home via os.homedir() to match production rather than process.env.HOME (unset on Windows). Result on real Windows: 129 pass, 0 fail, 2 skipped (broken-symlink cases that require symlink privilege). The two runtime changes are POSIX-equivalent so the Linux suite is unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017i6xinUm8V9Hfe55HYRwyr
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概要
Windows (Git Bash) で発生した「初回
codex app-server起動時に出力ゼロ・エラーゼロで12分以上ハングし、/codex:statusにも載らない」事象を起点に、プロセスライフサイクルまわりの不具合をまとめて修正しています。1. 無音ハング（2 経路）
request()にtimeoutMsを追加。initializeを 60 秒（CODEX_COMPANION_HANDSHAKE_TIMEOUT_MSで変更可）、制御系リクエストを 2 分で有界化。ストリーミング系（turn/start・review/start）は無制限のまま。タイムアウト時は収集済み stderr をエラーに添付し、wedged なプロセス／ソケットを破棄turn/start応答後は保留中リクエストが 0 なので、app-server が死んでも何も reject されずawait state.completionが永久に返らなかった（rejectCompletionは呼び出し箇所のない dead code）。接続断を監視して失敗させるように2. プロセス識別（レビュー2巡目 P1）
broker.jsonの pid と pid ファイルの突き合わせは同時に書かれた同じ古い情報の比較で、pid 再利用後も一致したままでした。isProcessAliveも同様に「今その番号を持つプロセス」しか見ていません。pid とプロセス開始時刻の組で識別するようにしました（Linux
/proc/<pid>/stat、WindowsGet-Processの Ticks、その他ps -o lstart=）。terminateProcessTreeはプロセスグループごと殺すため、識別できない pid には一切シグナルを送りません。identity 未記録の古いレコードは従来の生存確認にフォールバックします（拒否すると永久にキャンセル不能な孤児が残るため）。保証がプラットフォームの開始時刻分解能に依存する点はコード内に明記しています。また、起動待ちに失敗した broker はハンドルが実行中を示す場合のみシグナルします。
3. Stop ゲートの収束（レビュー2巡目 P1）
stop_hook_activeで無条件 skip していたため、Codex の指摘を Claude が直したか検証されず 2 回目は素通りでした。レビュー対象（HEAD + staged/unstaged diff + status）をフィンガープリント化し、変更なしなら保存済み判定を再利用（Codex を再実行せず block は block のまま）、変更ありなら再レビュー、3 ラウンド収束しなければユーザーに制御を返します。block 出力はドキュメントどおりのトップレベル
decision/reasonのみです。4. Windows
shell: falseに — Git Bash/MSYS 下では/PID・/Tがパスに書き換えられ、kill が黙って失敗してプロセスが孤児化していたhandleCancelが catch していないのでキャンセル状態が記録されないまま落ちていた。kill は best-effort にし、cancel 状態は必ず永続化SHELL/ComSpecを信用しない。SHELLは環境変数として Codex に渡るため Codex 自身のコマンド実行には影響しない5. ジョブの可視性と状態
sessionId無しのジョブがセッションスコープの status から不可視かつキャンセル不能だった問題を修正。/codex:status --allでフィルタを完全バイパス/codex:resultも拒否していたload → mutate → saveをクロスプロセスロックで保護（アトミック書き込みだけでは torn read しか防げず、read-modify-write の交錯でジョブが消えていた）。state.json・ジョブファイル・broker.jsonは tmp+renamequeuedのまま残っていた6.
$ARGUMENTSのプロンプト破壊アポストロフィが shell クオートとして扱われ、
what's wrong here --backgroundが1 トークンに潰れてフラグが消え、C:\Users\meがC:Usersmeになっていました。散文用のトークナイザに変更しています。テスト
npm test131 件成功。CI をubuntu-latest+windows-latestのマトリクスに変更し、タイムアウトテストは named pipe 対応フィクスチャで Windows でも実行されます（従来は全 skip でした）。新規テストはいずれも修正前のコードでは失敗することを確認済みです（mid-turn 切断は 20 秒のデッドラインまでハング、stop-gate は 4 ケース中 3 ケース失敗、broker 回収と並行 lost update も同様）。
upstream PR について（前回記述の訂正）
PR openai#484 / openai#577 / openai#580 は実在する open PR でした。
refs/pull/N/headを直接 fetch して内容も確認し、本 PR の修正はいずれも同じ方針を採っています。前回「存在を確認できなかった」と書いたのは upstream main の履歴のみを見たためで、未マージの PR が main に含まれないのは当然でした。確認方法が誤っていました。なお
f17e7f8"fix: respect SHELL on Windows for Git Bash (openai#178)" は main に実在しますが、openai#484 の分析どおり app-server を包む外側のシェルと Codex 自身が使うシェルは別物であり、SHELLは env 経由で Codex に渡るため、外側を cmd.exe に固定しても Git Bash 利用者の体験は変わりません。🤖 Generated with Claude Code
https://claude.ai/code/session_01M5cuwDkeEdktKwmZQhu4ko