Skip to content

maxTotalTimeout is never enforced unless a progress notification arrives #2695

Description

@sainikhiljuluri

What happens

maxTotalTimeout is documented as a hard cap:

Maximum total time (in milliseconds) to wait for a response. If exceeded, an SdkError with code SdkErrorCode.RequestTimeout will be raised, regardless of progress notifications.

It is not enforced unless a progress notification arrives and resetTimeoutOnProgress is set. Without both, the budget does nothing.

Measured on main, with a transport that never responds:

{ timeout: 1000, maxTotalTimeout: 150 }
  at 200ms  → still pending          (cap already exceeded)
  at 1200ms → "Request timed out"    (fired on `timeout`, not the cap)

{ maxTotalTimeout: 400 }
  after 3000ms → still pending       (waits for the 60s default timeout)

So a caller asking for a 150ms ceiling got 1000ms, and a caller asking for 400ms got 60s.

Why it happens

In packages/core-internal/src/shared/protocol.ts, _setupTimeout arms the pending timer with timeout only:

timeoutId: setTimeout(onTimeout, timeout),

maxTotalTimeout is read in exactly one place — the elapsed check inside _resetTimeout — and _resetTimeout has a single caller, guarded by:

if (timeoutInfo && responseHandler && timeoutInfo.resetTimeoutOnProgress) {

So the only way the budget is ever consulted is the progress path. A request with no progress notifications, or with resetTimeoutOnProgress left at its default of false, never checks it.

Why it survived

The existing test (test/shared/protocol.test.ts, "should respect maxTotalTimeout") passes resetTimeoutOnProgress: true and feeds two progress notifications, so it exercises the one path that works.

Expected

The pending timer should be armed for whichever limit comes first, and re-armed against the remaining budget on each progress notification, so the cap holds whether or not progress arrives.

Environment

main at 3924de99 (2.0.0-alpha.0). The same shape is present on v1.x.

Not related to #2224, #2136, #1870 or #2082 — those concern resetTimeoutOnProgress needing an onprogress callback, and none of them touch _setupTimeout or the arming.

I have a fix and tests ready and am happy to open a PR if useful.


AI assistance was used to investigate and write this report.

Activity

  1. added
    v1Issues / PRs related to v1.x
    v2Ideas, requests and plans for v2 of the SDK which will incorporate major changes and fixes
    on Aug 21, 2026
  2. Ben-Home commented on Aug 21, 2026

    @Ben-Home
  3. Lstarsky0 commented on Aug 23, 2026

    @Lstarsky0

    Mechanism confirmed on main at 3924de99. One correction to "read in exactly one place": that's true inside protocol.ts, but there is a second reader, and it's what makes this bigger than one option not working.

    packages/core-internal/src/shared/inputRequiredDriver.ts:276-286 shrinks the budget between legs of an input_required flow:

    const remaining = requestOptions.maxTotalTimeout - totalElapsed;
    if (remaining <= 0) throw new SdkError(SdkErrorCode.RequestTimeout, 'Maximum total timeout exceeded', ...);
    legOptions.maxTotalTimeout = remaining;

    remaining goes through inputRequiredEngine.ts:188 back into the request funnel and lands on _setupTimeout at protocol.ts:1569 — the arming that ignores it. The module doc at inputRequiredDriver.ts:17 states the design as "the per-leg timeout applies to every wire leg unchanged, and maxTotalTimeout bounds the whole flow by shrinking the budget passed to each leg — no new timer system."

    So the budget is enforced only at leg boundaries today. One leg that hangs sits on the per-leg timeout (60s by default) and the driver doesn't get to look at the budget again until that leg returns, so a flow asked to finish in 5s can run past 60s with nothing noticing. The synthetic progress the driver emits at :244 doesn't help either — it's a local call to the caller's onprogress, not an incoming notifications/progress, so it never reaches _resetTimeout.

    That's an argument for the fix you describe over any narrower one: arming on whichever of timeout and the remaining budget comes first makes the driver's stated contract true for free, whereas anything that only touches the progress path leaves it broken.

  4. jstar0 commented on Aug 23, 2026

    @jstar0

    I intend to prepare a focused fix for this issue, starting from main/v2. The implementation will arm each request timeout against the earlier of the per-request timeout and the remaining maxTotalTimeout budget, while preserving progress-based reset behavior. I will add regressions for a non-progressing request and for an input_required flow whose per-leg timeout exceeds the total budget, then check whether the same patch applies cleanly to v1.x before proposing a backport.

  5. mturac commented on Aug 24, 2026

    @mturac

    I pushed a fix for this on mturac:fix/issue-2695 (commit cf95432):

    • Arm the request timer with min(timeout, maxTotalTimeout) so the hard ceiling applies even without progress.
    • Re-arm against remaining total budget on progress resets.
    • Reject with Maximum total timeout exceeded when the total budget is what fired.
    • Regression in packages/core-internal/test/shared/protocol.test.ts (should enforce maxTotalTimeout without progress notifications).

    Local: npm test -- --run test/shared/protocol.test.ts in packages/core-internal — 50 passed.

    I could not open the PR from this account right now (CreatePullRequest is forbidden). Happy to open it as soon as that path works, or a maintainer can pull from https://github.com/mturac/typescript-sdk/tree/fix/issue-2695.

  6. mturac commented on Aug 24, 2026

    @mturac

    Opened draft PR #2712 with CI green on afadc39 (prettier fix included). MCP org policy blocks marking fork PRs ready via API (markPullRequestReadyForReview forbidden); will mark ready in the GitHub UI when available.

    #2712

  7. mturac commented on Aug 24, 2026

    @mturac

    Opened draft PR: #2714

  8. mturac commented on Aug 24, 2026

    @mturac

    Draft PR #2714 is up with 14/14 CI green on afadc39 (includes the prettier fix).

    Fork authors cannot mark MCP org fork PRs ready via API (markPullRequestReadyForReview / gh pr ready both forbidden). If this looks good for triage, marking it ready for review from the GitHub UI would unblock maintainer review.

    Happy to adjust anything still off.

  9. mturac commented on Aug 24, 2026

    @mturac

    PR #2714 is up for this issue with 14/14 CI green on afadc39.

    GitHub blocks fork authors from marking MCP org fork PRs ready via API (markPullRequestReadyForReview forbidden). If the fix looks good for triage, marking the PR ready for review from the UI would unblock maintainer review.

    Happy to adjust anything still off.

  10. claxman commented on Sep 7, 2026

    @claxman

    Reproduced on main at 5119ee7: TestProtocol plus a silent MockTransport with { timeout: 1000, maxTotalTimeout: 150 } is still pending at 150ms (settled undefined), and the existing should respect maxTotalTimeout test stays green because it feeds progress. I will arm _setupTimeout with min(timeout, maxTotalTimeout), re-arm remaining budget on progress reset, reject with Maximum total timeout exceeded when the cap fires, and add no-progress plus remaining-budget regressions.

  11. veriton-dev commented on Sep 7, 2026

    @veriton-dev

    Host-side note from agent runtimes that treat maxTotalTimeout as the hard budget for tool/RPC slots:

    Failure modes this creates in practice

    1. Silent budget lie — caller sets { timeout: 30_000, maxTotalTimeout: 5_000 } expecting a 5s ceiling; without progress (or with resetTimeoutOnProgress: false, the default) the pending timer is armed only with timeout, so the call can run ~30s before RequestTimeout. The documented “regardless of progress” contract is inverted.
    2. Progress-gated enforcement — the only read of maxTotalTimeout sits inside _resetTimeout, which is itself gated on resetTimeoutOnProgress. No progress ⇒ never consulted. Quiet long tools (common for MCP) never trip the cap.
    3. Test blind spot — the existing “should respect maxTotalTimeout” case feeds progress + resetTimeoutOnProgress: true, so it only covers the one path that works and can’t catch regressions on the no-progress path.

    Minimal shape that matches the docs

    • On _setupTimeout, arm the timer with Math.min(timeout, maxTotalTimeout ?? timeout) (or whichever absolute deadline is sooner).
    • Keep an absolute deadline = start + maxTotalTimeout on the timeout info.
    • On progress reset: re-arm with Math.min(timeout, remainingUntilDeadline) — never extend past the absolute cap.
    • If maxTotalTimeout is set and already elapsed at arm/reset time, fire SdkErrorCode.RequestTimeout immediately (don’t wait for the next tick of the soft timer).

    Acceptance checks

    • { timeout: 1000, maxTotalTimeout: 150 }, silent transport → reject by ~150ms, not ~1000ms.
    • { maxTotalTimeout: 400 } alone (default soft timeout) → reject by ~400ms, not 60s.
    • Progress + resetTimeoutOnProgress: true still resets the soft timer but never past the absolute deadline.
    • resetTimeoutOnProgress: false + progress noise still enforces the absolute cap.
    • Error code stays RequestTimeout (or a distinct MaxTotalTimeout if you want callers to distinguish soft vs hard — optional; docs currently say RequestTimeout).

    Happy to review a PR that moves the arming into _setupTimeout and adds a no-progress case next to the existing progress test. No product pitch — just the lifecycle gap.

  12. Sanath91009 commented on Nov 6, 2026

    @Sanath91009

    Opened #3155 with a fix and three tests.

    Reproduced on main at 1e608ca: { timeout: 1000, maxTotalTimeout: 150 } with no progress is still pending at 151ms and rejects at 1000ms with Request timed out. With { timeout: 1000, maxTotalTimeout: 300, resetTimeoutOnProgress: true } and one progress notification at 200ms, the timer is re-armed for the full 1000ms, so it settles at 1200ms instead of 300ms.

    The change arms the timer with min(timeout, remaining maxTotalTimeout), re-arms with the remaining budget on a progress reset, and reports Maximum total timeout exceeded when the cap is what fired. Two of the three new tests fail on main; all three pass with the fix. packages/core-internal 78 files / 1853 tests and packages/server 55 files / 674 tests are green, typecheck and lint clean.

    One thing I could not do myself: the PR is a draft. Creating a non-draft PR from this account on this repo returns 404 from the API, and markPullRequestReadyForReview returns FORBIDDEN, so a maintainer needs to mark it ready. The diff and CI are complete.

    #2763 covers the same issue and has been conflicted against main since 2026-09-23. Happy to close #3155 if that one is rebased and lands first.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    v1Issues / PRs related to v1.xv2Ideas, requests and plans for v2 of the SDK which will incorporate major changes and fixes

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions