fix(models): backend and analytics correctness batch for 5.1 GA - #1236
Conversation
Five verified bugs found in the models backend:
1. hdb_model_calls phantom indexes: dropped `indexed: true` from all
non-PK attributes in analyticsTable.ts. flush() and cleanup() write
via primaryStore.put/remove which bypass updateIndices, so secondary
indexes would stay permanently empty. Matches hdb_raw_analytics pattern.
2. Bedrock inference-profile model IDs: familyOf() now walks all dot-
separated segments to find the first known family, resolving cross-
region IDs like `us.anthropic.claude-3-5-sonnet-…` that previously
resolved to family 'unknown'. Also added a descriptive error for
amazon.nova-* models before they hit the wrong (Titan) body shape.
3. Abort gate at tool dispatch: added ctx.signal?.throwIfAborted() at the
top of runSingleToolCall so pre-aborted signals are caught before any
side-effecting handler starts. The serial path between multiple handlers
also benefits since each handler entry now checks the signal.
4. OpenAI max_tokens vs max_completion_tokens: OpenAI's reasoning/gpt-5
models reject `max_tokens` with a 400. Send `max_completion_tokens`
when talking to api.openai.com; keep `max_tokens` for any custom
baseUrl to preserve compatibility with vLLM, Ollama-compat, and other
OpenAI-compatible shims.
5. Upstream memory caps:
a. Non-streaming bodies: replaced bare `await res.json()` in
parseJsonResponse and readErrorSuffix (openai + anthropic) with a
bounded streaming reader (64 MiB success cap, 256 KiB error cap).
b. Tool-call accumulator cardinality: added a 128-entry cap on the
streaming accumulator maps in all three backends (openai, anthropic,
bedrock). `index` is upstream-controlled and without a cardinality
cap a hostile stream can allocate unbounded map entries.
Tests: 219 passing (was 317/1-pre-existing-EMFILE in the wider suite,
unchanged). All new tests cover the specific failure modes described.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d-reader hardening, total tool-arg cap Gemini review findings adjudicated and applied: the analyticsTable schema test asserts on the exported attribute list instead of regexing the source; #isNativeOpenAI parses the URL hostname (port and spoofed-suffix safe); readBoundedJson wraps stream errors in the backend error class, cancels the body before throwing on the cap, and reuses a module-level TextDecoder; an 8 MiB total tool-argument cap bounds the accumulator maps across entries in all three streaming parsers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Reviewed; no blockers found. |
- prettier reformatted three test files - rename unused `err` parameter to `_err` in agentLoop.test.js (oxlint no-unused-vars) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Codex review leg completed (was quota-blocked at PR open). One finding, adjudicated real and addressed in the latest commit: the 64 MiB success-body cap could reject a maximal legal OpenAI embedding batch (2048 inputs × 3072 dims ≈ 125–190 MiB of JSON) — raised to 256 MiB, which still bounds a runaway upstream. No other blocking issues found. 🤖 Posted by Claude on Nathan's behalf |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Codex review: a legal OpenAI embedding batch (2048 inputs x 3072 dims) serializes to ~125-190 MiB of JSON, over the 64 MiB cap — successful calls would have been rejected. 256 MiB clears the largest legal batch while still bounding a runaway upstream. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The constant was raised from 64 MiB to 256 MiB in this PR to handle large OpenAI embedding batch responses (125–190 MiB JSON). The unit test assertion was not updated alongside it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The concurrency: false option on the suite caused "Column family already dropped!" failures on Node.js v22, v26, and Windows (all passing on main without the option). The Bun failures in #1236 are pre-existing flakes: the terminology test passes on main including all Bun shards. Closing investigation — no change needed here; re-run when Bun flakes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
Approve — five well-scoped, correctly-fixed bugs with solid new test coverage. The inline comments explaining each why are excellent, and the structural schema-test approach for the phantom-index fix is a nice touch. Two small follow-ups before merge, neither blocking the code:
1. PR description vs. code: body-cap value (doc nit). The summary (fix #5 and "Where to put attention") says "64 MiB success / 256 KiB error", but the code is MAX_RESPONSE_BODY_BYTES = 256 << 20 (256 MiB), justified by the max legal embedding batch in the source comment. Looks like the cap was raised during the cross-model-review revision but the description didn't follow. Worth syncing the number, since that's exactly the constant the description asks reviewers to sanity-check.
2. Azure OpenAI is a false negative in the native-OpenAI heuristic (worth a comment, not a blocker). The known-limitation note calls out gateways (Cloudflare AI Gateway, Helicone), but the more common real case is Azure OpenAI (*.openai.azure.com), which also requires max_completion_tokens for o-series / reasoning deployments. Under hostname === 'api.openai.com', Azure users hit the exact max_tokens → 400 this fix resolves. The per-entry config-override escape hatch you mention would cover it — I'd just name Azure explicitly in the limitation note since it's a first-class deployment target, not a niche proxy.
Minor, non-blocking observation: the 256 MiB cap bounds the input bytes, but readBoundedJson transiently holds chunks + merged buffer + decoded string + parsed object, so a single max-legal embedding response can peak ~1 GiB, and the cap is per-call rather than global. Fine to ship for the legal-max case — just noting it for future reference.
Nice work on this batch — the failure-mode reasoning on each fix made it easy to follow.
— Reviewed by Claude (Opus 4.8)
2 participants
Summary
Five verified correctness fixes to the models subsystem from the 5.1 GA readiness audit — relates to #1235 (PR 2 of 2, companion to #1234).
hdb_model_callsphantom indexes — droppedindexed: truefrom all nine non-PK attributes. Writes go throughprimaryStore.put, which bypasses index maintenance, so the declared indexes were permanently empty and any attribute-filtered billing/usage query silently returned zero rows. Matches thehdb_raw_analyticspattern. The attribute list is now an exported const so the schema test asserts structurally.familyOf()walks the dot-segments until a known family is found, sous.anthropic.claude-…/eu.meta.…/global.…dispatch correctly (previously: family'us'→ unknown → throw, breaking effectively all current Claude-on-Bedrock).amazon.nova-*now throws a descriptive "not yet supported" error instead of being sent a malformed Titan-shaped body.runSingleToolCall()checks the composed signal before invoking a handler, so side-effecting tool handlers never start after the caller has aborted (closes the serial-dispatch gap too).max_completion_tokens— sent whenbaseUrlresolves to nativeapi.openai.com(current reasoning models rejectmax_tokens); custom baseUrls keepmax_tokenssince OpenAI-compatible shims may not know the newer param. Detection parses the URL hostname (port-suffix and spoofed-host safe).readBoundedJson()bounds non-streaming bodies (64 MiB success / 256 KiB error — previouslyres.json()buffered unbounded and the 500-char error cap only trimmed after parsing), wraps stream errors in the backend error class, and cancels the body on breach. The streaming tool-call accumulator maps are now capped at 128 entries and 8 MiB total argument chars across entries (per-entry 1 MiB cap alone allowed ~128 MiB) in all three streaming parsers.Where to put attention
api.openai.com. Proxies/gateways fronting OpenAI (Cloudflare AI Gateway, Helicone) will getmax_tokensand fail against reasoning models — by design for now (no reliable detection); flagged by the cross-model review as the residual case. A per-entry config override would be the escape hatch if users hit it.Tests: 75 passing across the targeted unit files (includes new cases for inference-profile dispatch, Nova rejection, pre-aborted dispatch, native-vs-shim token param, bounded-reader limits, and accumulator caps); broader models+components suites green except one pre-existing EMFILE failure confirmed on main.
Cross-model review: Gemini findings adjudicated and applied in the second commit (structural test, URL-parsed detection, bounded-reader hardening, total-arg cap). Codex leg unavailable (weekly session limit). Generated by an LLM (Claude, Fable 5).
🤖 Generated with Claude Code