Skip to content

⏱️ fix: Fit Workspace Command Timeouts Inside the Configured Request Budget - #16464

Merged
danny-avila merged 7 commits into
devfrom
danny-avila/fit-command-timeout-budget
Sep 29, 2026
Merged

danny-avila merged 7 commits into
devfrom
danny-avila/fit-command-timeout-budget

Conversation

@danny-avila

@danny-avila danny-avila commented Sep 28, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

An attached (BYOM) code environment can set both a command timeout ceiling (limits.maxCommandTimeoutMs) and a total HTTP budget (limits.maxRequestTimeoutMs, #16421). The two were never reconciled. Background Bash calls default to the full command ceiling, and executeWorkspaceTool reserves that timeout plus 5 s settlement and 5 s delivery grace inside the budget. With a ceiling of 80 s and a budget of 90 s, the reserve is exactly 90 s, so every background command was refused before dispatch: "Workspace execution cannot fit within the remaining HTTP budget. The operation was not started." Nothing ran, but every such background task failed. One deployment hit this 13 times in six hours. The tool schema also advertised timeouts up to 80 s that could never start.

The command ceiling is now fitted to the budget wherever it is resolved. It becomes the lower of the configured ceiling and the budget minus settlement grace, delivery grace and an operator-configurable minimum admission allowance (10 s by default), so a fitted command can still wait briefly for a busy worker. With the configuration above, the ceiling becomes 70 s: background calls default to 70 s with up to 10 s of queue time before local dispatch overhead, the schema advertises 70 s, and an explicit 80 s request is rejected up front with the deployment-limit message instead of failing at dispatch. Without a request budget, nothing changes.

How it works

fitWorkspaceCommandTimeoutToBudget(budget, minCommandAdmissionMs = 10 s)
  = budget - 5 s settlement - 5 s delivery - minCommandAdmissionMs

resolveAttachedWorkspaceCommandTimeoutMax(configSchema, upstream)
  min(configured ceiling, upstream ceiling)            (unchanged)
  -> min(that, fit(limits.maxRequestTimeoutMs))         (new, only when a budget is configured)

createAttachedWorkspaceBashTool({ maxTimeoutMs, maxRequestTimeoutMs, minCommandAdmissionMs })
  applies the same fit, so a caller passing both limits directly gets the same ceiling

The resolver feeds both the Bash tool (ToolService.js) and the workspaceCommandTimeoutMaxMs that agent initialization passes on, so they agree. The helper lives next to the grace constants in code/workspace.ts, so the fit and the dispatch-time reserve check cannot drift apart. Set configSchema.limits.minCommandAdmissionMs from 1000 to 300000 milliseconds in an attached code environment to change the reserve; omission retains 10000 milliseconds. With an HTTP budget configured, validation requires maxRequestTimeoutMs > (minCommandAdmissionMs ?? 10000) + 10000 ms so the explicit or default admission reserve, settlement, delivery, and at least 1 ms of command execution fit. Omitting the admission setting with a budget of 20000 ms or less is rejected at configuration parsing; omitting the HTTP budget retains legacy behavior. Invalid combinations are rejected during configuration parsing instead of advertising a timeout that cannot honor the reserve. When rolling out the new setting, upgrade configuration readers before adding the field to librechat.yaml.

Type of change

  • Bug fix

Testing

Tested environments/configuration:

  • Attached BYOM environment with limits: { maxCommandTimeoutMs: 80000, maxRequestTimeoutMs: 90000 }, where background Bash calls were failing before dispatch.

Automated tests:

  • packages/api: npx jest src/code src/agents/__tests__/initialize (548 passed, 1 skipped). New cases:
    • the resolver fits 80 s into a 90 s budget as 70 s, leaves a 60 s ceiling alone, and is unchanged without a budget;
    • a background call built from that configuration dispatches with a 70 s timeout and a 10 s queue allowance;
    • a tool given both limits directly advertises 70 s and rejects an explicit 80 s request before any fetch.

The attached-environment YAML example shows the configured minimum admission allowance before local auth overhead: with a 125 s HTTP budget and the 10 s default allowance, a command can advertise at most 105 s; a 120 s command requires at least 140 s on the actual network path.

Current-head verification: Config schema: 266 passed; command/workspace: 125 passed; ToolService wiring: 1 passed. Data-provider npx tsc --noEmit, scoped ESLint, Prettier and import sort passed locally. CI verifies the API TypeScript workspace and the production build; local API typechecking is limited by incomplete shared dependencies. A simulated 200 ms credential-signing delay at the minimum 1 s admission reserve still dispatches with 800 ms of queue allowance. The Playwright test selectors follow the steering preference copy merged to dev in #16465, and the failed-call accessible-name assertion follows the counted label merged in #16468. Both changes only affect E2E assertions against GitHub’s actual dev merge.

@danny-avila

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-29T02:34:54.271537Z 2bb5e18 Manual request
ℹ️ 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" or "@codex security review".

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 chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 445a9895c1

ℹ️ 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".

Comment thread packages/api/src/code/workspace.ts Outdated
@lia-by-librechat

Copy link
Copy Markdown
Contributor

Exact-head review handoff: da76f75a265b0fa8c1b893d579c3fe726556764f. This PR fits Bash command timeouts within the request budget and makes the admission reserve configurable via limits.minCommandAdmissionMs (10 s by default). This head also updates Playwright's steering preference labels to match #16465 on the merged dev branch. Local focused tests passed: 251 config, 124 command/workspace and 1 ToolService wiring. The new CI run will validate the merged head. Please review this exact SHA.

@danny-avila

Copy link
Copy Markdown
Collaborator Author

@codex review the latest head

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: da76f75a26

ℹ️ 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".

Comment thread e2e/specs/mock/steering-escalation.spec.ts
Comment thread packages/data-provider/src/config.ts
@lia-by-librechat

Copy link
Copy Markdown
Contributor

Exact-head review handoff: 8aa19f1357b614324507fa91a87d4900658edc70. This head validates that an explicit attached-workspace command admission reserve fits the request budget with execution time remaining. The prior menu-label finding was checked against the PR’s dev merge and both affected Playwright shards passed; its evidence is in the inline reply. Config tests: 259 passed. Data-provider typecheck and production build passed. CI is running on this head; please review this exact SHA.

@danny-avila

Copy link
Copy Markdown
Collaborator Author

@codex review the latest head

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8aa19f1357

ℹ️ 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".

Comment thread packages/api/src/code/workspace.ts
Comment thread packages/data-provider/src/config.ts Outdated
@lia-by-librechat

Copy link
Copy Markdown
Contributor

Exact-head review handoff: 69e099ee443b00b6d8e9eaab197c093e22760038. The attached-workspace command reserve is configurable and impossible budget/reserve pairs are rejected. This head also matches the failed-call pill’s counted accessible name on dev after #16468. The previous CI run found the stale E2E assertion; the other tests passed. Focused local checks: 259 config tests, 124 command/workspace tests, 1 ToolService wiring test, data-provider typecheck and build. The new CI run is checking the merge head. Please review this exact SHA.

@lia-by-librechat

Copy link
Copy Markdown
Contributor

Exact-head review handoff: 2bb5e18b81c3feef4da708e1b67caeb28314b2d2. This head bounds the configurable admission reserve to 1–300 seconds (10 seconds by default), tests dispatch after credential-signing delay, and corrects the attached-environment YAML arithmetic. Invalid reserve/request-budget combinations fail config parsing. The dev-merge Playwright assertions match the tested UI. Focused local tests: 260 config, 125 command/workspace, and 1 attached ToolService wiring. Data-provider tsc --noEmit, production build, scoped lint, format and import sort passed. CI is validating this exact head; please review this SHA.

@danny-avila

Copy link
Copy Markdown
Collaborator Author

@codex review the latest head, final review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2bb5e18b81

ℹ️ 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".

Comment thread packages/data-provider/src/config.ts Outdated
@lia-by-librechat

Copy link
Copy Markdown
Contributor

Exact-head review handoff: 4b3fb5d8af0fb0ae2a64e5a15cbbb87f82e8ab30. This head closes the new P2: the configured request budget must contain the effective command admission reserve, even when the reserve is omitted and the existing 10 s default applies. Previously accepted budgets of 20 s or less now fail deployment config parsing rather than dispatching a 1 ms mutating Bash command; budgets without a request override remain unchanged. Added boundary and top-level schema tests (266 config tests passed); data-provider tsc --noEmit and production build passed. The PR’s other 125 command/workspace and 1 ToolService tests passed on the preceding head and are covered by CI here. Please review this exact SHA.

@danny-avila
danny-avila merged commit 63363a7 into dev Sep 29, 2026
43 checks passed
@danny-avila
danny-avila deleted the danny-avila/fit-command-timeout-budget branch September 29, 2026 02:53
@danny-avila danny-avila mentioned this pull request Sep 29, 2026
15 tasks
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.

2 participants