Skip to content

fix(daemon): extend AMR first-output deadline to 150s - #7406

Closed
alchemistklk wants to merge 1 commit into
mainfrom
fix/opend-2133-first-output-150s
Closed

fix(daemon): extend AMR first-output deadline to 150s#7406
alchemistklk wants to merge 1 commit into
mainfrom
fix/opend-2133-first-output-150s

Conversation

@alchemistklk

Copy link
Copy Markdown
Contributor

Summary

  • extend the AMR absolute first-output deadline from 120 seconds to 150 seconds
  • update the runtime default contract and comments to 2.5 minutes
  • keep the operator override/disable path and one same-run retry unchanged
  • assert the user-facing stall duration is derived from the configured timeout rather than a stale literal

Motivation

OPEND-2133 captured a cloud Fable5 run that was cancelled after the outer 120-second deadline. Production correlation showed that runtime startup left the provider about 116 seconds, while a nearby successful Fable5 request produced its first token at 119.464 seconds.

A 150-second outer deadline leaves roughly 146 seconds of provider startup budget and should cover that observed long-tail success without changing the broader 30-minute inactivity policy.

Related:

Behavior and trade-off

Unchanged:

  • 30-minute inactivity timeout
  • OD_CHAT_RUN_FIRST_OUTPUT_TIMEOUT_MS override and 0 disable semantics
  • ACP stage timeout
  • one safe same-run retry
  • timeout/failure classification and terminal trigger
  • routing, fallback, billing, and provider behavior

Trade-off: two completely silent attempts can now take about 300 seconds instead of about 240 seconds before terminal failure. The new Vela establishment metrics will provide the rollout evidence needed to evaluate this tail.

Validation

Passed:

  • focused first-output timeout/retry/failure tests
  • daemon typecheck
  • root typecheck
  • pnpm guard
  • fresh scope/behavior review: no P0/P1/P2 findings

The full daemon suite exceeded the 1200-second command limit and its partial output contained unrelated pre-existing Codex model-preflight/media route failures plus post-close SQLite watcher warnings, so it is reported as inconclusive rather than passed.

@lefarcen
lefarcen requested a review from mrcfps August 25, 2026 15:02
@lefarcen lefarcen added size/S PR changes 20-100 lines risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps type/bugfix Bug fix labels Aug 25, 2026
@lefarcen

Copy link
Copy Markdown
Contributor

@alchemistklk Heads-up: this overlaps pretty closely with #7342 — both PRs touch the AMR first-output timeout path (apps/daemon/src/runtimes/defs/amr.ts plus the timeout tests), so it would be good to compare the narrower 150s change here with the broader timeout/copy change there before we pick a merge path.

Also, could you add the PR template's Surface area checkboxes? For this one, Default behavior change looks like the relevant box. I also tagged this for QA, since the timeout sits on a live runtime path that users will feel if it regresses.

@mrcfps mrcfps left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@alchemistklk thanks for the tight, evidence-backed timeout bump — this is a clean fix.

Verified the scoped change:

  • AMR firstOutputTimeoutMs moves from 120s → 150s with matching comment wording
  • resolver/default contract tests and the AMR def assertion track the new default
  • the dual-stall retry test now asserts the user-facing duration is derived from the configured deadline (1200ms1s) rather than a stale literal substring
  • override/disable path, inactivity policy, retry shape, and failure classification remain untouched

No correctness, safety, or test issues in the changed ranges. Nice work on keeping the blast radius small while covering the observed Fable5 long-tail first-token case.

🔁 Powered by Looper · runner=reviewer · agent=opencode · An autonomous AI dev team for your GitHub repos.

@lefarcen lefarcen added the needs-validation Runtime change detected; needs human or /explore agent validation. label Aug 25, 2026
@lefarcen
lefarcen requested a review from ivy-ting August 25, 2026 15:12
@lefarcen

Copy link
Copy Markdown
Contributor

@alchemistklk quick follow-up before this moves on: the description is still missing two pieces from the current PR template — the Surface area checkboxes, plus a short Bug fix verification section. Since the Why / What users will see content is already covered by your Summary + Motivation, this is mostly just filling in the remaining checklist info for reviewers.

@alchemistklk

Copy link
Copy Markdown
Contributor Author

Superseded by #7342. We are keeping the broader timeout-policy and user-copy direction and will land only one overlapping client timeout implementation.

@lefarcen

Copy link
Copy Markdown
Contributor

Thanks for closing the loop here. Consolidating on #7342 for the broader timeout-policy and user-copy direction makes sense — we'll treat this PR as superseded and keep follow-up there.

@alchemistklk

Copy link
Copy Markdown
Contributor Author

The maintained client fix is now #7495. It keeps the product-approved Cloud 30-minute policy from #7342 rather than reopening this narrower 150-second variant. Production Vela telemetry now shows 145/146 relevant Aiping DeepSeek cancellations clustering at 122–125 seconds, reinforcing that the shared 120-second cutoff is the issue.

@lefarcen

Copy link
Copy Markdown
Contributor

Thanks for the update — noted on #7495. Keeping the maintained follow-up there, with the Cloud 30-minute policy and the newer Vela telemetry, makes sense; we'll treat this PR as fully superseded by that path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-validation Runtime change detected; needs human or /explore agent validation. risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps size/S PR changes 20-100 lines type/bugfix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants