Skip to content

Align the request pipeline with the AI SDK - #164

Merged
eric8810 merged 26 commits into
arcships:masterfrom
cunninghamcard-bit:codex/ai-sdk-operation-retry
Sep 12, 2026
Merged

Align the request pipeline with the AI SDK#164
eric8810 merged 26 commits into
arcships:masterfrom
cunninghamcard-bit:codex/ai-sdk-operation-retry

Conversation

@cunninghamcard-bit

@cunninghamcard-bit cunninghamcard-bit commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Aligns the request pipeline with the Vercel AI SDK contract (reference baseline 63db193). The design spec ships with the change as docs/ai-sdk-request-pipeline.md, including a difference table for every deliberate deviation. History is organized as six commits in build order: core, provider-utils, providers, FFI, bindings, docs.

What this fixes relative to master

The two headline problems are the retry layering and the error domain; everything else falls out of them.

  • Retry lived in the wrong layer. In master, retry was internal to the HTTP module — its own docs state the provider does not observe retries happening. That conflated one logical call with one HTTP exchange: errors carried no attempt history and no exhausted-vs-non-retryable classification, recordings could not distinguish an operation's Nth attempt from its Nth exchange, and no policy above the transport (idempotency, per-operation budgets) could see the retry boundary. Now Core owns operation retry (RetryError { maxRetriesExceeded | errorNotRetryable }, messages verbatim to the AI SDK, full attempt history) and provider-utils performs exactly one exchange per call — the AI SDK's own layering.
  • The error domain was lossy. Five providers (ElevenLabs, AssemblyAI, Deepgram, fal, Hume) parsed failed responses through the OpenAI-shaped DEFAULT_ERROR_STRUCTURE (error.message / error.type), so their real error formats (detail.*, plain {error}, err_code/err_msg, FastAPI detail, top-level message/code) surfaced as empty messages; and a failing response's body was buffered in full with no size bound before parsing. Each mapper now matches its provider's documented format, and error bodies read best-effort under a 64 KiB public cap (post-decode) with a 1 MiB parse bound so oversize-but-valid error JSON still reaches the mapper.
  • A retried video start could bill twice — a direct casualty of the old layering. The billable predictLongRunning POST went through the HTTP layer's silent retry with no idempotency key, so a retry after a lost response re-submitted a paid generation. One idempotency key is now minted per logical start, reused across retry attempts, and never sent on status polls (AI SDK generate-video parity).
  • One malformed frame killed the whole stream. Core consumers, both FFI pumps, and the Node and Python drivers each latched on the first parse error and truncated the stream. A recoverable frame error now travels as data and the stream continues; only transport/Core errors are terminal.
  • Stream termination depended on the provider closing the connection. With no timeout configured, a provider that sent the final chunk but held the connection open hung the consumer forever. The consumption wrapper is now an unconditional terminal fuse under plain default options.
  • One process-wide HTTP client leaked dead connections across runtimes. Pooled connections are driven by the runtime that made the request; a finished runtime left dead connections checked in, and the HTTP-layer retry masked the resulting failures (roughly one run in six flaky on direct SPI calls). The client is now keyed per runtime with a bounded cache.

Core

  • Operation retry moved into Core. The two-layer primitive mirrors retryWithExponentialBackoff(RespectingRetryHeaders): a generic loop that does not know ApiCallError, plus a wrapper that fills should_retry and get_delay_ms. RetryError { maxRetriesExceeded | errorNotRetryable } preserves the full attempt history; messages match the AI SDK fixtures verbatim. Full Jitter (RFC-0009) is the default get_delay_ms for the exponential branch; a server retry-after(-ms) hint is honored exactly (ms < 60s || ms < exponential).
  • Timeouts and cancellation. AbortSignal is a thin CancellationToken wrapper carrying only caller cancellation; total/step/first-chunk/chunk deadlines live in the operation future (tokio::select!, nothing spawned). The first-chunk budget is armed before do_stream setup; chunk timers reset only on semantic output (isOutputChunk parity).
  • Streams survive malformed frames, at every boundary — and always terminate. A bad JSON/schema frame is surfaced as an Err item and the stream continues; only transport/Core errors are terminal. Core alone was not enough: both FFI pumps, the Node napi driver, and the Python pyo3 driver each latched on the first Err and truncated the stream, so a recoverable frame error now travels as data — a StreamPart::Error frame on the plain path, skipped on the chunk-typed chat-completions path whose wire cannot carry it. The consumption wrapper doubles as the terminal fuse: Finish and non-recoverable errors end the stream under every configuration, including plain default options. StreamTextResult::text() / consume() keep consuming to the trailing Finish so usage and recording still land.
  • A transport error on the first SSE event fails the attempt inside the retry boundary (the RFC-0016 peek is retained — a deliberate deviation, see the spec).
  • VideoModel split into do_start / do_status. Core owns the poll loop and retries each phase independently, matching the AI SDK generate-video flow: do_start — billable — mints one idempotency key per logical start outside the retry closure (a caller-supplied key wins), and the poll budget paces the loop between status checks while a hung status GET is bounded by the per-exchange 30s response guard. VideoCallOptions defaults n and provider_options on deserialization — typed binding structs omit unset fields, and a strict parse rejected minimal options at the FFI boundary.

Provider utils / providers

  • Single-exchange helpers (post_json_to_api, post_form_data_to_api, post_to_api, get_from_api) with typed response handlers replace send* / send_with_retry_raw / ErrorStructure; every helper call performs exactly one fetch attempt, records exactly one exchange, and non-streaming exchanges keep a per-exchange 30s whole-response guard.
  • The error path keeps the provider's error. Error bodies read best-effort with a 64 KiB public cap (enforced after lossy UTF-8 decoding) and a 1 MiB parse bound so oversize-but-valid error JSON still reaches the provider's mapper; a handler failure on a 429/503 keeps the status's retryability; a mid-body transport failure reports no HTTP status. In-band stream errors are redacted and URL-sanitized at the shared stream_error_api_call entry — the raw request context providers hold never reaches the public error or recordings.
  • Failed-response mappers match each provider's documented format (ElevenLabs detail.status/detail.message, AssemblyAI's plain {error: string}, Deepgram err_code/err_msg, fal's FastAPI detail, Hume's top-level message/code) instead of assuming OpenAI's shape, and string provider codes are no longer JSON-quoted.
  • The shared HTTP client is keyed by runtime, and the cache is bounded. Pooled connections are driven by tasks on the runtime that made the request, so one process-wide client let a finished runtime leave dead connections checked in — and the OS can recycle their ports to fresh listeners, handing later requests a corpse. The old HTTP-layer retry masked this; with retry in Core, direct SPI calls (every wiremock provider test) hit it as flaky transport failures roughly one run in six. Runtimes leave no drop signal, so the per-runtime cache evicts wholesale at a small cap instead of growing without bound.
  • All providers migrated to the handler API; list_models exchanges apply the Core retry primitive with the provider's configured settings. Recording distinguishes operation attempts from exchange indices, with step-labeled contexts for Router/MoA children.

Scope notes for reviewers

  • Download/SSRF security is not in this PR. An earlier revision of the spec asked get_from_api to carry AI SDK's validateUrl / credentialedOrigin / trustedOrigin contract from day one, and the implementation grew a full inline guard — duplicating fix: validate and pin provider-supplied download URLs (SSRF) #163, which delivers exactly that. It has been removed here and the spec now hands the contract to fix: validate and pin provider-supplied download URLs (SSRF) #163, which rebases onto this pipeline and wires the provider call sites. Redirects follow reqwest's default policy in the meantime, recorded as one logical exchange.
  • Three files overlap fix: validate and pin provider-supplied download URLs (SSRF) #163 for lint reasons only. trace/hash.rs, openai/embedding.rs, and ws.rs needed the new clippy 1.98 lints silenced. Both PRs do it with the same #[allow] attributes and the same wording (as_chunks::<N>() is stable only since Rust 1.88, while the workspace MSRV is 1.85), so whichever lands second is a no-op in those files.

Verification

Organized by what this PR could break, and the gate that would catch it:

Risk Gate Where
Retry, backoff, retry-after semantics Ported AI SDK fixture tests; error messages match the AI SDK verbatim aimux-core retry tests
Timeout semantics (total / first-chunk / chunk-idle) Paused-time matrix tests for timer arming, reset-on-output, and cleanup aimux-core/tests/stream_timeout_matrix_test.rs
Per-provider request/response behavior 43 test files ported 1:1 from the AI SDK's own provider test suites (each header cites its reference/ai/packages/.../*.test.ts source); wiremock exercises the full pipeline over real local HTTP aimux-providers/tests
Error wire contract Golden error-variant test with a compile-time exhaustive match, so adding a variant cannot bypass the wire-tag check aimux-core/tests/error_value_golden_test.rs
C ABI drift Header/exports agreement test pins all 115 exported symbols against aimux-ffi.h aimux-ffi/tests/exports_smoke_test.rs
Typed TS surface drift ts-rs regeneration gate scripts/gen_ts_types.py --check
Binding regressions Per-binding CI jobs: Node (6 targets), Python, Go (2 OS), Java (JDK 8/11/17/21), Kotlin (JDK 17/21), Swift, Flutter (android/ios), plus contract tests CI matrix
Stream lifecycle across FFI Contract tests pinning recoverable-frame continuation on both pump shapes aimux-ffi/src/lib.rs tests

All 24 CI checks are green on the current head.

Real-provider smoke (DeepSeek, run at the current head)

A live end-to-end pass through the full pipeline (Core retry → single-exchange helpers → OpenAI-compat provider) against api.deepseek.com:

1. generate_text OK: text="pong" usage=Some(2) finish=Stop
2. stream_text OK: 11 deltas, finish seen, text="1\n2\n3\n4\n5"
3. 401 mapping OK: status=Some(401) retryable=false provider_message="Authentication Fails, Your api key: ****alid is invalid"
4. operation timeout OK: request timed out: Total timeout of 1ms exceeded
SMOKE PASSED

What each line proves: (1) a real success round-trip parses text/usage/finish; (2) a real SSE stream delivers deltas, Finish, and then terminates (the terminal fuse, under plain default options); (3) a real 401 surfaces the provider's own message, is classified non-retryable, and is not retried even with max_retries: 2; (4) a 1 ms budget trips the typed AiMuxError::Timeout, not a mislabeled transport error.

To reproduce with any OpenAI-compatible key (no repo changes needed):

let model = aimux_providers::provider("deepseek", Some(key), "deepseek-chat", None)?;
let r = aimux_core::generate::generate_text(model.as_ref(), "Reply with exactly: pong",
    GenerateTextOptions { max_output_tokens: Some(40), ..Default::default() }).await?;

Review guide

Where the diff concentrates, and why:

  • aimux-provider-utils/src/http.rs (the largest change, mostly deletions): retry, error parsing, and recording bookkeeping moved out. What remains is transport — build the request, perform one exchange, record it.
  • aimux-core/src/generate.rs (+710): the orchestration that used to hide inside send() is now visible at the operation layer — the retry closure around do_generate/do_stream, the timeout deadlines (tokio::select! in the operation future, nothing spawned), and the stream-consumption wrapper that doubles as the terminal fuse.
  • aimux-core/src/recording.rs (+455) plus plumbing: with retry above the HTTP layer, the transport no longer knows which attempt it is serving, so a RecordingContext is built once per operation and travels with the request — a shared attempt allocator, per-context exchange counters, and child() contexts that label Router/MoA steps while keeping attempt numbers unique across the whole call. Streaming response recording holds this context instead of a clone of the prepared request.
  • aimux-core/src/video_model.rs (+549): every video provider used to hand-roll its own start+poll loop; the do_start/do_status split moves that flow into Core once — poll pacing, per-phase retry, idempotency-key minting — ported from the AI SDK's executeStartStatusFlow. Roughly 40% of the file is its test module.

Six commits in build order (core → provider-utils → providers → ffi → bindings → docs), each message stating its rationale. Two external review rounds are folded in; every confirmed finding was either fixed or deliberately re-aligned with the AI SDK's reference behavior (the spec's §3 table records all deviations).

Related issues: advances #95 (error-domain alignment: retry error expression, provider error classification, stream error semantics, the C ABI error surface) and partially resolves #161 (video now has the user-facing operation layer — generate_video() over do_start/do_status).

Follow-up: #165 moves tool-call input parsing and validation to the Core boundary. It is based directly on master and independent of this PR; whichever lands second rebases over textual overlap only.

@cunninghamcard-bit
cunninghamcard-bit marked this pull request as draft August 24, 2026 02:12
@cunninghamcard-bit
cunninghamcard-bit force-pushed the codex/ai-sdk-operation-retry branch from 09a9090 to 12cdf9b Compare August 24, 2026 02:17
@cunninghamcard-bit
cunninghamcard-bit force-pushed the codex/ai-sdk-operation-retry branch 3 times, most recently from f9672cd to 4cdda39 Compare August 24, 2026 05:49
@cunninghamcard-bit cunninghamcard-bit changed the title Align the request pipeline with the AI SDK (RFC-0031) Align the request pipeline with the AI SDK Aug 24, 2026
@cunninghamcard-bit
cunninghamcard-bit force-pushed the codex/ai-sdk-operation-retry branch 5 times, most recently from 0f24a13 to 4034ac9 Compare August 24, 2026 14:05
Move retry and timeout from the HTTP layer into the user operations,
mirroring the AI SDK's prepareRetries/retryWithExponentialBackoff
RespectingRetryHeaders: a generic backoff loop that does not know
ApiCallError, plus a wrapper that fills should_retry and get_delay_ms.
RetryError { maxRetriesExceeded | errorNotRetryable } preserves the full
attempt history; Full Jitter drives the exponential branch and a server
retry-after(-ms) hint is honored exactly. Providers declare their policy
through retry_config(); Core executes it around do_generate/do_stream.

AbortSignal is a thin CancellationToken wrapper carrying only caller
cancellation; total/first-chunk/chunk deadlines live in the operation
future, with chunk timers reset only on semantic output (isOutputChunk
parity). The consumption wrapper is also the stream's terminal fuse:
Finish and non-recoverable errors end the stream under every
configuration, while malformed frames surface as Err items and the
stream continues (is_recoverable_stream_error), including through
text()/consume() aggregation.

VideoModel splits into do_start/do_status with a Core-owned poll loop
matching the AI SDK generate-video flow: each phase retries
independently, do_start — billable — mints one idempotency key per
logical start outside the retry closure (a caller-supplied key wins),
and the poll budget paces the loop while a hung status GET is bounded
by the per-exchange response guard in provider-utils. VideoCallOptions
defaults n and provider_options on deserialization for typed binding
structs.

Recording distinguishes operation attempts from exchange indices,
records composite (Router/MoA) children as steps, and widens sensitive-
key redaction to camelCase/kebab-case token keys.
Replace send/send_timed/send_stream_timed/send_with_retry_raw and the
JSON-path ErrorStructure with AI SDK-shaped single-exchange helpers
(post_json_to_api, post_form_data_to_api, post_to_api, get_from_api)
dispatching to typed success/failure response handlers. Every call
performs exactly one fetch attempt and records exactly one exchange;
retry, operation timeouts, and backoff belong to Core. Non-streaming
exchanges keep a per-exchange 30s whole-response guard; streaming
handlers are exempt because their body outlives the call.

Failure handling keeps the provider's error through every path: error
bodies are read best-effort with a 64 KiB public cap (enforced after
lossy UTF-8 decoding) and a 1 MiB parse bound so oversize-but-valid
error JSON still reaches the provider's mapper; a handler failure on a
429/503 keeps the status's retryability; a transport failure mid-body
reports no HTTP status; in-band stream errors built via
stream_error_api_call are redacted and URL-sanitized at the shared
entry. WebSocket handshake rejections classify by HTTP status like any
other exchange.

The shared HTTP client is keyed by runtime — pooled connections are
driven by tasks on the runtime that made the request, so a process-wide
client let a finished runtime leave dead pooled connections behind (the
old HTTP-layer retry masked this as ~1-in-6 flaky wiremock failures).
The cache is capped, evicting wholesale on overflow since runtimes
leave no drop signal.
Every provider moves from the deleted send*/ErrorStructure surface to
the single-exchange helpers with typed success/failure handlers; retry
policy is declared through retry_config() and executed by Core. In-band
stream errors route through stream_error_api_call (or redact their
request context) so credentials and data URLs never reach the public
error. list_models exchanges — not Core user operations — apply the
Core retry primitive locally with the provider's configured settings.

Failed-response mappers now match each provider's documented error
format instead of assuming OpenAI's {error:{message,code}} shape:
ElevenLabs detail.status/detail.message (FastAPI variants included),
AssemblyAI's plain {error: string}, Deepgram err_code/err_msg and
category/message, fal's FastAPI detail, Hume's top-level message/code.
provider_code extraction no longer JSON-serializes string codes into
quoted values.
Expose the retry/timeout error context (retryable flag, retry_ms hint,
provider code/message, response body, sanitized url/request values,
response headers, provider data) and the video start/status split across
the C surface; aimux_error_request_id is removed — request ids ride in
response_headers. Both stream pumps keep the stream alive across
recoverable frame errors: the plain pump forwards them as
StreamPart::Error data frames, while the chunk-typed OpenAI pump skips
them — that wire is typed as ChatCompletionChunk and an injected error
object would kill every typed consumer. The header's timeout keys and
code range are pinned by the header/exports contract test; the C
examples consume the new error getters.
Node, Python, Go, Java, Kotlin, Swift, and Flutter carry the pipeline's
typed error context (retryable, retry_ms, provider code/message,
request id, response body) and the VideoPollOptions surface. The Node
napi and Python pyo3 stream drivers forward recoverable frame errors as
data and keep pumping (skipping them on the chunk-typed OpenAI path);
terminal errors still end the stream. ts-rs generated types are
regenerated and gated by scripts/gen_ts_types.py --check; web example
fixtures pick up the recording step field.
The design spec lives at docs/ai-sdk-request-pipeline.md (moved out of
rfc/), with a difference table for every deliberate deviation from the
AI SDK baseline (63db193) and the §13 acceptance matrix. API docs across
all bindings pick up the retry/timeout error context, the video
start/status split with its idempotency gate, and the recoverable-frame
stream semantics; download/SSRF security is explicitly handed to the
PR that owns it.
@cunninghamcard-bit
cunninghamcard-bit force-pushed the codex/ai-sdk-operation-retry branch from 4034ac9 to 5ddfd0f Compare August 24, 2026 14:29
@cunninghamcard-bit
cunninghamcard-bit marked this pull request as ready for review August 24, 2026 14:47
cunninghamcard-bit added a commit to cunninghamcard-bit/aimux that referenced this pull request Aug 25, 2026
as_chunks::<8>() is stable only since Rust 1.88; unknown_lints keeps the
allow buildable on toolchains older than the lint. Same attribute and
wording as arcships#163/arcships#164, so whichever lands second is a no-op here.
@cunninghamcard-bit
cunninghamcard-bit marked this pull request as draft August 25, 2026 01:42
cunninghamcard-bit added a commit to cunninghamcard-bit/aimux that referenced this pull request Aug 25, 2026
as_chunks::<8>() is stable only since Rust 1.88; unknown_lints keeps the
allow buildable on toolchains older than the lint. Same attribute and
wording as arcships#163/arcships#164, so whichever lands second is a no-op here.
cunninghamcard-bit added a commit to cunninghamcard-bit/aimux that referenced this pull request Aug 25, 2026
as_chunks::<8>() is stable only since Rust 1.88; unknown_lints keeps the
allow buildable on toolchains older than the lint. Same attribute and
wording as arcships#163/arcships#164, so whichever lands second is a no-op here.
cunninghamcard-bit added a commit to cunninghamcard-bit/aimux that referenced this pull request Aug 25, 2026
CI's clippy is newer than the workspace MSRV allows us to follow:
as_chunks::<N>() is stable only since Rust 1.88 while the MSRV is 1.85,
so trace/hash.rs and openai/embedding.rs keep chunks_exact behind
unknown_lints; ws.rs's tungstenite error is consumed by its single
caller, so boxing it buys nothing. Same attributes and wording as
arcships#163/arcships#164, so whichever lands second is a no-op in these files.
…ion-retry

# Conflicts:
#	aimux-provider-utils/src/ws.rs
…ion-retry

master landed the download SSRF guard (arcships#163) against the pre-pipeline
transport helpers, while this branch rewrote the same call sites onto the
single-exchange helper API. Both intents are kept: the guard's parameters
become HttpRequest fields (validate_url / trusted_origin /
credentialed_origin, plus the per-exchange response_timeout) that
get_from_api and post_to_api honour, so downloads of provider-supplied URLs
stay validated while retries remain Core's job.

- 107 HttpRequest literals carry the new fields; only provider-supplied
  download URLs set validate_url.
- call_to_api now honours a request's own response_timeout instead of the
  flat 30s ceiling, which is what keeps Replicate's `prefer: wait` from
  being misread as a dead transport (the double-billing path this branch
  exists to fix).
- The SSRF tests move to the helper API they now exercise.
@cunninghamcard-bit
cunninghamcard-bit marked this pull request as ready for review August 31, 2026 05:52
Two defects this branch introduced, found by an adversarial review of the
diff (14 other candidate findings were refuted against the code).

xAI Responses yielded StreamPart::Error for a terminal in-stream error and
then continued reading. That was harmless on master, where both consumers
returned on the first Error part; this branch makes Error non-terminal so a
trailing Finish can still deliver usage and recording, which turns the
continue into a hang against a server that keeps the connection open after
the error. The sibling OpenAI Responses reducer was fixed this way in this
branch already, test included; xAI is its near-duplicate and was missed.

retry_after_hint() returns negative milliseconds for a Retry-After HTTP-date
in the past. The retry loop drops those via u64::try_from, but the C ABI
projects the raw value through aimux_error_retry_ms, whose header documents
-1 as 'absent' — so a hint one millisecond in the past reads as no hint, and
a larger negative reaches Go/Java as a sleep duration. Upstream's check is
'0 <= ms' (retry-with-exponential-backoff.ts); apply it at the source.
Unifying a mechanism should shrink the diff; this branch's provider layer
grew instead. Four repetitions carried no information, so they are gone:

- HttpRequest gains Default. 120 of the 131 construction sites listed the
  SSRF guard's four fields as all-defaults; only the eight sites that
  actually download a provider-supplied URL still spell them out.
- ResponseHandlerOutput.response_headers drops its Option. Every one of the
  twelve producers filled it with Some(..), so 63 files were unwrapping a
  value that could not be absent — google/files.rs even carried an
  .expect("header-only response handler always supplies headers").
- HttpRequest::new(url, headers, options) via a small ExchangeContext trait.
  111 sites copied the same three lines to inherit cancellation, call id and
  recording context from the caller's options; 72 collapse to one line. The
  rest keep the literal because they set a custom response_timeout or the
  download guard.
- Six providers whose error JSON is the shared { error: { message, type |
  code } } shape now call create_standard_json_error_response_handler.

Left alone deliberately: the 33 retry_config overrides return each
provider's configured value rather than the trait default, and 27 of the 32
error handlers parse genuinely different error shapes.

@eric8810 eric8810 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.

Summary

The direction of this PR is valuable: centralizing retry/timeout behavior, introducing structured errors, making malformed stream frames recoverable, and sharing clients per runtime are all worthwhile changes. The implementation is also well documented and the current CI matrix is green.

However, this refactor changes several public behavioral contracts in ways that can return incorrect results or silently remove reliability guarantees. I do not think the current revision is safe to merge yet.

Major issues

  1. Video generation ignores n and max_videos_per_call, so multi-video requests return only one result (and n = 0 still starts a request).
  2. Stream first-chunk/chunk deadlines include time when the caller is not polling the returned stream. Valid buffered output can therefore be replaced by a timeout based on consumer scheduling rather than provider output latency.
  3. File uploads no longer use the configured retry policy. The public retry configuration remains available, but OpenAI, Anthropic, and Google file operations now perform only one exchange.
  4. Core retries wrap entire multi-stage transcription operations. A transient failure after a successful upload can replay the upload instead of retrying only the failed stage.
  5. Video provider metadata from the start response is silently lost when the completion response uses the same provider key.
  6. Successful JSON responses may buffer up to 2 GiB and are then represented in multiple in-memory forms, which turns the limit into a realistic OOM risk rather than a protective bound.

Requested changes

Please preserve the existing public contracts while moving ownership into Core: split video calls according to the requested count, measure stream timeouts at the producer boundary, define explicit retry/idempotency behavior for file and multi-stage operations, merge nested provider metadata without dropping start data, and use a conservative/configurable success-body cap or streaming deserialization. Regression tests should cover each boundary above.

The targeted Core retry/video and provider-utils test sets pass, but they do not cover these contract failures; one existing first-chunk test currently codifies the consumer-delay behavior described above.

///
/// Returns the provider failure, retry exhaustion, poll or operation
/// timeout, or caller abort.
pub async fn generate_video(

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.

[P1] Honor the requested video count. This path performs exactly one do_start call and never reads options.n or max_videos_per_call. Consequently n > 1 still returns one video, while n = 0 still starts a provider request. Please validate the count, split it into provider-sized batches, execute all required calls, and aggregate their results/metadata.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 857f54c / f315b86: n = 0 is rejected before any request; n is split into max_videos_per_call batches run concurrently (AI SDK Promise.all parity), one idempotency key each, results concatenated. Test on ScriptedVideoModel asserts the batch count from a peak-in-flight counter.

.error());
break;
}
() = timeout::wait_for_deadline(chunk_deadline) => {

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.

[P2] Do not measure consumer delay as provider timeout. This deadline continues to elapse while the returned stream is not being polled, and the biased select chooses it before an already-buffered chunk. A provider can therefore produce a valid first chunk on time, yet a caller that polls later receives only a timeout. The same issue affects chunk_ms while the consumer processes the previous item. Please drive/timestamp provider output independently of consumer polling.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 7a83301: a pump task now drives the provider stream and arms/resets the first-chunk and chunk deadlines on arrival; the returned stream only reads from a channel and aborts the pump on drop. The first-chunk test that codified the consumer-delay behaviour now asserts the opposite, and consumer_delay_does_not_count_against_the_chunk_timer covers chunk_ms under paused time. Spec §3/§8.1 and acceptance 21b/21c record the pump and the unconditional tokio-runtime requirement.

Comment thread aimux-providers/src/openai/files.rs Outdated
},
self.config.retry_config,
&DEFAULT_ERROR_STRUCTURE,
let resp = aimux_provider_utils::post_to_api(

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.

[P2] File uploads have fallen out of the retry pipeline. This replacement changes a retrying send into a single exchange, while Files::upload_file has no Core wrapper and provider with_retry_config APIs are still exposed. The same regression exists in Anthropic and Google file implementations. Please define explicit retry/idempotency semantics for file uploads and either preserve the configured behavior or remove the misleading configuration surface.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 0baf004: OpenAI, Anthropic and Google file operations go through the Core retry primitive with the provider's configured retry settings, same as list_models. Uploads are not billable, so plain retry of the whole exchange is the documented policy. One 503-then-200 test per provider.

Comment thread aimux-providers/src/assemblyai.rs Outdated
},
RetryConfig::default(),
&DEFAULT_ERROR_STRUCTURE,
let resp = aimux_provider_utils::post_json_to_api(

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.

[P2] Avoid replaying completed stages. If this transcript-creation request fails transiently, the error reaches the Core retry around the whole do_generate, which uploads the audio again before retrying this request. The old request-level retry retried only the failing exchange. Please preserve the successful upload reference across retries or split the workflow into explicit start/status stages. Gladia has the same shape.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in c38bfd4: AssemblyAI and Gladia retry upload, submit and poll independently, so a transient submit failure reuses the existing upload_url. Exhaustion surfaces as AiMuxError::Retry, which the outer Core retry passes through rather than re-running the operation. Wiremock test asserts exactly one upload request. The other multi-stage providers (fal, luma, BFL, klingai, runwayml, revai, replicate) already retry their post-billing stages this way.

Comment thread aimux-core/src/video_model.rs Outdated
result.warnings = warnings;
if let Some(start_meta) = start.provider_metadata {
let merged = result
.provider_metadata

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.

[P2] Preserve metadata from both phases. or_insert keeps the completion object wholesale when both start and completion metadata use the same provider key, so all start-only fields under that provider are silently discarded. Please merge the nested provider objects (with a documented collision policy) instead of merging only at the top-level provider key.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 01da456 / f315b86: per provider key, object fields are unioned and the completion value wins on a field collision (documented on the merge function). Unit test with the same provider key in both phases.


use aimux_core::{AiMuxError, ApiCallError};

/// Default maximum buffered response size (2 GiB, matching AI SDK).

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.

[P2] Use a protective success-body limit. A 2 GiB cap still allows a response to be buffered into a Vec, parsed into serde_json::Value, and then cloned/deserialized, so a single response can exhaust process memory well before the nominal limit. Please use a conservative/configurable bound and preferably deserialize without retaining multiple full representations.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in ea9a33d: successful JSON bodies use a separate DEFAULT_MAX_JSON_RESPONSE_SIZE of 64 MiB, overridable per request via HttpRequest::max_json_response_bytes; typed handlers deserialize from the bytes once instead of via a Value clone. Binary downloads keep the 2 GiB bound. Test: an over-cap JSON success body fails with the size-limit error.

…neration

merge_provider_metadata() unions object fields within a shared provider key
instead of merged.entry(provider).or_insert(value), which silently dropped
every start-phase field whenever the completion phase reported metadata
under the same provider key. On a field collision the completion value wins;
this is documented on the helper.

Covered by merge_provider_metadata_unions_same_provider_key_across_phases in
aimux-core/src/video_model.rs.
…y downloads

create_json_response_handler() reused DEFAULT_MAX_DOWNLOAD_SIZE (2 GiB) for
successful JSON bodies, which are held simultaneously as raw bytes, a parsed
Value, and a deserialized struct — letting a single response balloon peak
memory well past the nominal cap. It now defaults to a separate 64 MiB
DEFAULT_MAX_JSON_RESPONSE_SIZE, configurable per request via the existing
HttpRequest struct (HttpRequest::max_json_response_bytes, threaded through
ResponseHandlerInput, matching how response_timeout is already overridden
per request). create_binary_response_handler is unaffected and keeps the
2 GiB bound.

Also drops the from_slice::<Value> + from_value(value.clone()) double parse
in favor of deserializing T directly from the bytes; raw_value (used by a
few providers for Usage.raw) is now a best-effort second from_slice call
instead of a clone of the first parse's tree.

Covered by oversize_json_success_body_is_rejected in
aimux-provider-utils/tests/api_helpers_test.rs.
…Google)

upload_file() did a single post_to_api/post_json_to_api exchange with no
retry wrapper, so each provider's configured RetryConfig was silently
ignored for file operations even though the config surface stays exposed.

Wraps each exchange with aimux_core::retry::prepare_retries +
PreparedRetries::retry, the same primitive execute_list_models already uses
for a non-Core-operation exchange. Uploads are not billable and a failed
create-a-file request never returns an id to replay against, so a plain
retry of the whole exchange is safe — documented as the new §9.4 policy in
docs/ai-sdk-request-pipeline.md, superseding the prior "Files gets no
retry" decision.

Google's three-stage init/upload/poll flow retries each stage
independently rather than wrapping the whole upload, so an upload-stage
retry reuses the already-minted upload_url instead of re-running init, and
a poll-stage retry never re-uploads.

Covered by transient_failure_is_retried_and_succeeds in the OpenAI and
Anthropic files tests, and transient_upload_failure_is_retried_without_re_initiating
in the Google files test (also asserts the init stage runs exactly once).
…pendently

do_generate() ran upload -> submit -> poll as three plain exchanges inside
one Core-retried operation, only the poll loop had its own retry. A
transient failure of the submit exchange caused Core's outer retry to
replay the whole do_generate, re-uploading the (potentially large) audio
file for no reason.

Each stage now gets its own retry via the Core primitive, using the same
retry config as the existing poll-stage retry. When a stage's own retries
are exhausted it returns AiMuxError::Retry, which the outer Core retry
passes through unchanged instead of re-wrapping (see
retry_with_exponential_backoff's Err(AiMuxError::Retry(_)) arm in
aimux-core/src/retry.rs), so do_generate itself is never replayed either.

Checked the other transcription/single-modality providers for the same
shape: revai combines the upload and submit into one multipart exchange
(no separate re-uploadable stage), and fal/luma/black_forest_labs inline
their input (base64 data URL or a provider-hosted reference) into a single
submit request rather than uploading separately — none of them have the
two-exchange upload-then-submit shape this fix addresses.

Covered by transient_submit_failure_is_retried_without_re_uploading
(assemblyai) and transient_initiate_failure_is_retried_without_re_uploading
(gladia), both asserting the upload endpoint received exactly one request.
generate_video() performed exactly one do_start and never read options.n or
VideoModel::max_videos_per_call(), so n > 1 silently returned one video and
n = 0 still started a billed request.

start_and_poll() now validates n (0 is InvalidArgument, before any network
call), splits n into max_videos_per_call-sized batches (AI SDK
generateVideo/generateImage's algorithm: every batch but the last is
full-sized, the last is the remainder or a full batch), and runs every
batch's independent do_start/poll cycle concurrently via
futures::future::try_join_all, matching the AI SDK's Promise.all — not
sequentially. Each batch mints its own idempotency key, so a retried batch
never collides with another batch's replay. Batch results are flattened
back in order; provider metadata is aggregated across batches with the
same deep-merge policy as the start/completion merge (finding 5).

One Rust-specific note documented in code and in the RFC: on the first
batch failure, Rust drops the other in-flight batch futures (unlike
Promise.all, where sibling promises keep running); this doesn't change
correctness since nothing here reconnects to or cancels an already-started
provider job either way.

Covered by three new tests in aimux-core/src/video_model.rs:
n_zero_is_rejected_before_any_network_call,
n_above_max_per_call_splits_into_batches_and_aggregates (also asserts
per-batch idempotency keys and cross-batch metadata merge), and
batches_run_concurrently_not_sequentially.
The stream-consumption wrapper observed first_chunk_ms and chunk_ms inside
its own poll_next, so the deadlines kept elapsing while the caller was not
polling, and the biased select reported a timeout ahead of a chunk the
provider had already delivered on time. A consumer that polled late, or
spent time processing the previous item, was charged against the provider's
output budget.

The provider stream is now driven by a pump task spawned before stream_text
returns. It arms and resets the first-chunk and chunk-idle deadlines when
output arrives and forwards items through an unbounded channel; the returned
stream only forwards from that channel, watches caller abort, and stops at
the terminal item. The channel is unbounded so the pump never blocks on the
consumer (the AI SDK's streamText also consumes eagerly); the pump is aborted
when the returned stream is dropped, which drops the provider stream and
cancels the underlying HTTP exchange.

The spec's "nothing is spawned" claim is narrowed to the operation
deadlines; §3, §8.1 and acceptance item 21b/21c record the pump. The
first-chunk test that codified the consumer-delay behaviour now asserts the
opposite, and a paused-time matrix test covers chunk_ms with a slow consumer.
…doc link

`generate_video` is public and `merge_provider_metadata` is not, so the
intra-doc link failed `cargo doc` under `-D warnings`
(private_intra_doc_links).
Same behaviour, less of it:

- inline the batch split, dropping `batch_video_counts` and its `Vec`;
- fold the per-batch aggregation into the first batch's `VideoResult`
  instead of rebuilding one field at a time, which also removes the
  `response.unwrap_or_default()` fallback that could never be taken;
- collapse `merge_provider_metadata`'s nested match into one
  `(remove, b_value)` match, halving its branches;
- replace `BatchedVideoModel` and its two tests with one test on the
  existing `ScriptedVideoModel`. Concurrency is now asserted from a
  peak-in-flight high-water mark rather than a 200ms wall-clock
  threshold, so it is deterministic and does not need a timer.
The six review fixes repeated the same rationale verbatim in five
provider modules and restated the code in three doc sections. Keep one
short statement of the invariant per site and let the spec carry the
argument; the deviation table rows and acceptance items are untouched.

Test collapse, one regression test per finding:

- drop the Anthropic files retry test (same single-exchange shape and
  same finding as the OpenAI one, different fixture data only) and the
  Gladia per-stage test (same upload-then-submit shape as AssemblyAI's).
  Google Files keeps its own test: three-stage independent retry is a
  distinct contract.
- give `google_files_test` one `provider_with_retries` helper instead of
  three inline `GoogleConfig` builders.
…clone

The pump comment restated the loop it sits on; keep the two points the
code cannot show (why the channel is unbounded, why the pump is spawned
even with nothing armed).

xAI's successful-response handler parses the body into a `Value`, hands
it back as `raw_value`, and used `from_value(raw.clone())` to build `T` —
a second full tree resident at peak, the same amplification the JSON
size-cap fix removed from `create_json_response_handler`. Deserialize
from a borrow instead.
`transcribe` and `rerank` restore the fields their explicit arguments
own after deserializing `opts_json` over the options struct; `search`
replaced the whole struct instead, so `search("cats", {"query":"dogs"})`
silently searched for dogs.
`fps` is `Option<u32>` in Rust, and serde rejects a JSON float for it,
so Jackson's `24.0` failed to deserialize the *whole* `VideoCallOptions`
at the FFI boundary. Every other integer field in this struct
(`duration`, `seed`) is already `Long`.
`RECORDING_SCHEMA` is 2 since the exchange records gained
`step`/`attempt`/`exchange_index`, so both fixture generators were
stamping schema-2 payloads with schema 1.
These three methods take their required data as explicit arguments and
everything else as `opts_json`, but they deserialized the caller's
options object into the full options struct first — and `audio`,
`media_type`, `query` and `documents` are not optional there. Any
non-empty `opts_json` therefore failed with `missing field` before the
explicit arguments could be restored, so `max_retries` and `timeout`
were unreachable from Node and Python for all three modalities, which is
exactly what this branch set out to expose.

Fill the argument-owned fields in before deserializing. The values
inserted are shape-only placeholders that the existing `parsed.x =
opts.x` lines overwrite, so nothing large is copied; they are built from
the real enums (`AudioInput::Base64`, `RerankingDocuments::Text`) so a
variant rename fails to compile instead of at runtime.

`opts_with_args` is deliberately free of binding error types: it is the
only part with logic, and keeping it pure makes it unit-testable without
a Python interpreter or a Node runtime (the pyo3 test binary cannot link
libpython here). The wire-error classification is factored out of
`wire_json` / `parse_wire_json` so the wrapper keeps reporting malformed
text as `InvalidWireJson` and schema violations as `InvalidArgument`.

The C ABI is unaffected: it takes the whole `RerankingCallOptions` /
`SearchCallOptions` as one JSON blob, so its required fields are always
present. That is also why this is fixed at the binding sites rather than
by making the core fields `#[serde(default)]`.
The pump task calls `tokio::spawn` unconditionally, where before a
runtime was only needed once a deadline was armed. Adds the missing §3
deviation row.
@cunninghamcard-bit

Copy link
Copy Markdown
Contributor Author

@eric8810 All six findings are addressed (replies on each thread point to the commits), plus a few defects found while auditing the rest of the diff: Node/Python search() options overriding the explicit query, Node/Python transcribe/rerank/search rejecting any non-empty options, Java VideoCallOptions.fps typed as Double, and aimux-web fixtures writing schema 1. All 24 checks are green on 2d850db. Ready for another look.

@eric8810
eric8810 dismissed their stale review September 12, 2026 07:47

Outdated: all six findings have fix commits (video batching/n validation, producer-side stream timers, file-upload retry, staged transcription retry, metadata deep-merge, JSON body cap). Maintainer requested merge.

@eric8810
eric8810 merged commit d5b8323 into arcships:master Sep 12, 2026
24 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.

Multimodal traits only expose do_* methods that their own docs say not to call — where is the user-facing layer?

2 participants