⏳ fix: Let BYOM Workspace Calls Queue Past Thirty Seconds - #264
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 52d9a9d4ff
ℹ️ 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 queueBudgetMs = options.queueTimeoutMs ?? Math.min( | ||
| options.timeoutMs ?? 30_000, | ||
| MAX_WORKSPACE_QUEUE_WAIT_MS, | ||
| ); |
There was a problem hiding this comment.
With the default JOB_TIMEOUT, this raises admission from 30 to 300 seconds, but docs/byom-worker-admission.md still specifies a 30-second maximum and caller/proxy totals of 65/70/340 seconds, while packages/code/README.md also promises admission waits at most 30 seconds. Operators following those timeout instructions will disconnect well before the new queue-plus-execution budgets (approximately 335/340/610 seconds including the documented delivery allowance), cancelling the wait and defeating this fix under contention. Update both canonical documents and their timeout calculations alongside this default.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, addressed in bbd246a. Updated docs/byom-worker-admission.md and packages/code/README.md, with a link from docs/remote-bridge/README.md. They now calculate 335/340/610 seconds at the default five-minute JOB_TIMEOUT and explain the proxy/client/tool-disconnect and retry risks. I also traced LibreChat dev (129a1de52c): its per-attempt HTTP timer still assumes 30 seconds of admission, even though maxQueueWaitMs allows five minutes of retries after typed 503s. This PR does not change that other service, so it does not yet guarantee the full extended wait; the limitation and required client/intermediary follow-up are explicit in the docs and PR summary.
| // The HTTP disconnect cancels waiting; this bounds admission while the caller remains connected. | ||
| const queueBudgetMs = options.queueTimeoutMs ?? Math.min( | ||
| options.timeoutMs ?? 30_000, | ||
| MAX_WORKSPACE_QUEUE_WAIT_MS, |
There was a problem hiding this comment.
When a request waits near this new 300-second maximum before admission, RedisBridgeStore.dispatch has already computed ttlSeconds from the full queue deadline plus execution budget, and then applies that entire relative TTL when the lock or workspace slot is finally acquired. If the API crashes or cleanup fails after dispatch, a maximum-duration command admitted at the end of the wait therefore leaves its reservation alive for roughly 10.5 minutes from acquisition even though its fresh execution deadline ends after about 5 minutes, blocking subsequent work for an extra queue-length interval rather than only the intended 30-second cleanup grace. Recompute the reservation TTL at acquisition from the post-admission execution deadline, or otherwise subtract elapsed admission time.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, fixed in bbd246a. RedisBridgeStore.dispatch now calculates TTL on each actual serial-lock or workspace-slot acquisition, using a fresh execution allowance for split deadlines or only the remaining time for legacy absolute deadlines; enqueue recalculates assignment/queue metadata TTL from the final expiresAt. The new worker-admission and concurrent-store tests reproduced the ~635-second stale reservation with a 300-second queue allowance plus 305-second execution, then confirmed lock, slot and assignment expiries near the final execution deadline plus ~30 seconds. A separate test covers the absolute-deadline path. Existing cancellation, identity revalidation and durable workspace fences remain unchanged and were covered by focused tests.
|
Exact-head review handoff for bbd246a: both Codex findings on 52d9a9d are addressed and answered in their inline threads. This head adjusts serial and workspace-slot TTLs, updates all canonical BYOM timeout docs, and has 204 passing focused tests (12 existing skips); changed-source TypeScript passes and full service typecheck remains blocked by the same seven baseline diagnostics. Please have a maintainer trigger a fresh Codex review for this exact head; the Lia GitHub App cannot trigger one. End-to-end caveat: LibreChat dev still budgets a 30-second admission window per HTTP attempt, so its client/tool/proxy timeout also needs follow-up before the full server window is usable. CI was still running when this handoff was posted. |
|
@codex review the latest head |
|
Codex Review: Didn't find any major issues. Nice work! 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". |
2 participants
Summary
JOB_TIMEOUTand five minutes. Admission and execution still have separate budgets.End-to-end limitation
Code API does not receive the caller's absolute deadline. LibreChat dev
129a1de52cstill budgets 30 seconds of queue wait in each HTTP attempt, producing 65/70/340-second total transport limits for non-command/default-command/five-minute-command operations. The server's default five-minute queue requires caller/proxy limits of at least 335/340/610 seconds respectively, including delivery allowance. An earlier tool signal, client, or proxy timeout disconnects the request, possibly after a mutation starts. Align the client and intermediaries separately before relying on the full window. No deployment or VM configuration changes are included.Verification
tsc --noEmitis blocked by the same seven unrelated diagnostics seen before the runtime edit.git diff --checkpassed.