Skip to content

feat: configurable upstream + body-idle timeouts on RecordConfig#197

Merged
jpr5 merged 4 commits into
CopilotKit:mainfrom
tombeckenham:fix/configurable-body-timeout
May 15, 2026
Merged

feat: configurable upstream + body-idle timeouts on RecordConfig#197
jpr5 merged 4 commits into
CopilotKit:mainfrom
tombeckenham:fix/configurable-body-timeout

Conversation

@tombeckenham
Copy link
Copy Markdown
Contributor

Summary

Adds `RecordConfig.upstreamTimeoutMs` and `RecordConfig.bodyTimeoutMs` so callers can lift the hardcoded 30s ceilings used by the proxy. Defaults stay 30s — back-compat preserved.

Motivation

Reasoning models (e.g. Grok 4.3 via OpenRouter with `response_format: json_schema`) routinely leave 30s+ gaps between SSE chunks during the thinking phase under concurrent load. The hardcoded `BODY_TIMEOUT_MS = 30_000` in `makeUpstreamRequest` fires `req.destroy()` mid-stream, and the captured fixture ends up with no `[DONE]` and no `finish_reason` — downstream `JSON.parse(text)` then blows up on `Unterminated string`.

Reproduces deterministically with a 6-way concurrent record run against the same Grok 4.3 + structured-output prompt:

```
BEFORE (hardcoded 30s):
jsonValid: 3/6 elapsed min=23.23s median=30.95s max=31.00s
3 streams cut off at exactly 30.95–31.00s, no [DONE], no finish_reason

AFTER (bodyTimeoutMs: 180_000):
jsonValid: 6/6 elapsed min=31.84s median=38.62s max=42.18s
All 6 finish=stop, all done=true, all JSON valid
```

For comparison, the same 6-way load fired direct to OpenRouter (no aimock in the loop) completes 6/6 up to 52s with clean `[DONE]` — i.e. the upstream is fine, only the proxy's idle timer was clipping us.

Change

  • `RecordConfig`: two new optional numeric fields, documented inline in `src/types.ts`.
  • `makeUpstreamRequest`: optional 7th `timeouts` arg, falls back to `30_000` for both timers when omitted.
  • `proxyAndRecord`: threads `record.upstreamTimeoutMs` / `record.bodyTimeoutMs` into the upstream request.

Test plan

  • Existing `recorder.test.ts` "body timeout" test still passes (asserts the default 30_000 is applied when no override is set)
  • `pnpm run build` clean
  • Manual: 6-way concurrent Grok 4.3 streaming record through aimock with `bodyTimeoutMs: 180_000` — 6/6 finish=stop, vs 3/6 truncated without the override
  • (Reviewer:) Worth adding a vitest that verifies a custom `bodyTimeoutMs` is honoured — happy to add if you'd like before merge

Back-compat

No behavior change when the new fields are omitted — defaults stay at the original 30_000.

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 14, 2026

Open in StackBlitz

npm i https://pkg.pr.new/@copilotkit/aimock@197

commit: 41166b9

tombeckenham added a commit to openstory-so/openstory that referenced this pull request May 14, 2026
…rd config #701

The published `@copilotkit/aimock@1.24.0` RecordConfig type doesn't have
these options yet — they're only on the linked aimock fork
(CopilotKit/aimock#197) and were tripping `bun typecheck` on tests in CI
which installs from the registry. Replaced with a comment that documents
why they're needed and points at the upstream PR.

CI is unaffected at runtime — `record:` only fires when E2E_RECORDING=1,
which CI doesn't set. Local record runs that need the bumped timeouts can
add the options back temporarily, or wait for the upstream release.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jpr5

This comment was marked as off-topic.

@jpr5 jpr5 dismissed their stale review May 14, 2026 21:45

Posted prematurely — will follow up with a proper review.

@jpr5
Copy link
Copy Markdown
Contributor

jpr5 commented May 14, 2026

I pushed fixes on top of your commit:

  1. CHANGELOG — moved entry from released [1.18.0] to [Unreleased]
  2. Input validationclampTimeout helper rejects 0, NaN, Infinity, and negatives (they silently disable timeouts via Node coercion)
  3. Tests — three new: custom bodyTimeoutMs honored, custom upstreamTimeoutMs honored, clamp fallback for zero/negative
  4. CLI flags--upstream-timeout-ms / --body-timeout-ms with validation
  5. JSDoc — clarified upstreamTimeoutMs is a socket idle timer, not TTFB
  6. Type — inline anonymous type → Pick<RecordConfig, 'upstreamTimeoutMs' | 'bodyTimeoutMs'>
  7. Docs — added timeout flags to the record-replay page (CLI table + Upstream Timeouts section with programmatic example) and a README CLI example

Everything passes locally (tsc, 3026 tests, build). CI will need a push from your side to trigger — can you push an empty commit to kick the workflows?

Take a look and let me know what you think.

@jpr5 jpr5 closed this May 14, 2026
@jpr5 jpr5 reopened this May 14, 2026
@jpr5
Copy link
Copy Markdown
Contributor

jpr5 commented May 14, 2026

I'm sorry for the state thrash — my bot is being a fucken idiot today.

@jpr5 jpr5 force-pushed the fix/configurable-body-timeout branch from fcd3f60 to 19cbfca Compare May 14, 2026 22:13
Add upstreamTimeoutMs and bodyTimeoutMs to RecordConfig so callers
can lift the hardcoded 30s ceilings used by the proxy. Defaults
stay 30s. Includes clampTimeout validation (rejects 0/NaN/Infinity/
negatives) and Pick<RecordConfig, ...> for the internal param type.
@jpr5 jpr5 force-pushed the fix/configurable-body-timeout branch from 19cbfca to a8ab23c Compare May 14, 2026 22:15
@jpr5
Copy link
Copy Markdown
Contributor

jpr5 commented May 14, 2026

@tombeckenham OK all yours.

jpr5 added 3 commits May 14, 2026 15:18
Wire the new RecordConfig timeout fields into the llmock CLI with
input validation matching existing patterns.
Verify custom bodyTimeoutMs and upstreamTimeoutMs are honored, and
that 0/negative values clamp to the 30s default.
CHANGELOG entry under [Unreleased], record-replay page CLI table
and Upstream Timeouts section, README CLI example.
@jpr5 jpr5 force-pushed the fix/configurable-body-timeout branch from a8ab23c to 41166b9 Compare May 14, 2026 22:18
Copy link
Copy Markdown
Contributor Author

@tombeckenham tombeckenham left a comment

Choose a reason for hiding this comment

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

Yes. This works nicely. Thank you!

@jpr5 jpr5 merged commit ab9bcd9 into CopilotKit:main May 15, 2026
22 checks passed
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