Repository navigation
fix: make the spawned app-server handshake deadline overridable (#55) - #56
Conversation
The spawned `codex app-server` initialize had a hard 10s deadline. The `spawnedInitializeTimeoutMs` option existed but nothing in production set it — the only caller was a test — so on a cold start, a CPU-quota'd container or a loaded CI runner, `/codex:*` failed with a message the user could neither act on nor tune. Add `CODEX_COMPANION_SPAWNED_INITIALIZE_TIMEOUT_MS`, mirroring the existing `CODEX_COMPANION_BROKER_IDLE_TIMEOUT_MS` resolver in app-server-broker.mjs (same env-name shape, same lenient parse, same fall-back-to-default on garbage). It rejects `<= 0` rather than treating 0 as "disabled": a 0ms handshake budget would just fail instantly. Also name the deadline that fired and the knob that raises it in the error message, via an optional `timeoutMessage` on request(). The shared request() default is unchanged, so the broker transport keeps its generic wording. Deliberately not implemented: the progress-based deadline (option 3 in the issue). Keeping this minimal. Closes #55 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01LA3HNpq2wRzWpRDBnHusvw
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a8a376711
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!Number.isFinite(parsed) || parsed <= 0) { | ||
| return DEFAULT_SPAWNED_INITIALIZE_TIMEOUT_MS; | ||
| } | ||
| return Math.floor(parsed); |
There was a problem hiding this comment.
When CODEX_COMPANION_SPAWNED_INITIALIZE_TIMEOUT_MS is a positive fraction such as 0.5, it passes the validation but is floored to 0; request() only installs its timer when timeoutMs > 0, so a wedged spawned app-server then hangs indefinitely instead of falling back to the default deadline. Validate the floored result or clamp valid positive values to at least 1ms.
Useful? React with 👍 / 👎.
The `parsed <= 0` guard ran before `Math.floor`, so a positive fraction such as `0.5` passed validation and then floored to `0`. request() only arms its timer when `timeoutMs > 0`, so that silently disarmed the handshake deadline entirely and a wedged spawned app-server would hang forever — exactly the failure mode this deadline exists to prevent (fork #29/#31). Floor first, then validate. A sub-millisecond budget is unusable input, so it falls back to the 10s default rather than clamping to an instantly-failing 1ms. Math.floor(NaN)/Math.floor(Infinity) stay non-finite, so the existing garbage-input cases are unaffected. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01LA3HNpq2wRzWpRDBnHusvw
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61db2436d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const parsed = Math.floor(Number(rawValue)); | ||
| if (!Number.isFinite(parsed) || parsed <= 0) { | ||
| return DEFAULT_SPAWNED_INITIALIZE_TIMEOUT_MS; | ||
| } | ||
| return parsed; |
There was a problem hiding this comment.
When CODEX_COMPANION_SPAWNED_INITIALIZE_TIMEOUT_MS exceeds 2,147,483,647, this resolver accepts and returns it, but Node's setTimeout reduces an out-of-range delay to 1ms and emits a TimeoutOverflowWarning. A user attempting to raise the deadline with such a value therefore makes the spawned handshake fail almost immediately; reject these values, fall back to the default, or clamp them to the maximum supported delay.
Useful? React with 👍 / 👎.
Node stores a timer's delay in a 32-bit signed int, so setTimeout silently reduces any delay above 2^31-1 to 1ms (with a TimeoutOverflowWarning). The resolver accepted such values, so a user raising the deadline to something enormous got a handshake that failed almost instantly — the opposite of what they asked for. This is the mirror image of the sub-millisecond case fixed in the previous commit. Both extremes are unusable input, so both fall back to the 10s default rather than clamping: clamping would silently run a deadline that means the opposite of what the user typed, while the default is the documented behaviour. The ceiling itself remains valid. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01LA3HNpq2wRzWpRDBnHusvw
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Closes #55 — options 1 and 2 from the issue. Option 3 (progress-based deadline) is deliberately not implemented; it stays available if 1+2 prove insufficient.
What changed
1. Env override —
CODEX_COMPANION_SPAWNED_INITIALIZE_TIMEOUT_MSSPAWNED_INITIALIZE_TIMEOUT_MS = 10000was a hard deadline. ThespawnedInitializeTimeoutMsoption existed, but nothing in production set it — the only caller was a test — so on a cold start, a CPU-quota'd container, or a loaded CI runner,/codex:*failed with something the user could neither act on nor tune.The resolver mirrors
resolveIdleTimeoutMsinapp-server-broker.mjs: same env-name shape, same lenientNumber()parse, same fall-back-to-default on garbage ("",abc,NaN,Infinity, negatives). One deliberate divergence: it rejects<= 0instead of treating0as "disabled".0disabling the idle timer means "never reap"; a0ms handshake budget would just fail instantly, which is not a useful knob position.Precedence is
spawnedInitializeTimeoutMsoption → env → 10s default, so the existing test that passes300keeps working regardless of the ambient environment.2. Actionable timeout message
Before:
After:
Implemented as an optional
timeoutMessageonrequest().request()is shared by both transports, so its default wording is unchanged and the broker path keeps its generic message — only the spawned handshake opts in.Tests
New
tests/spawned-initialize-timeout-override.test.mjs, written red-first:1500.9→1500)npm test— 198/198 pass.npm run build— clean.No version bump.