refactor(appkit): direct Arrow IPC streaming + hardened result delivery#472
Merged
Merged
Conversation
Rework of the Arrow/JSON result-delivery path on top of #329's arrow foundation (base branch is #329 merged with current main). Delivery: - ARROW_STREAM queries stream the raw Arrow IPC bytes on the /query response body — no SSE, no server-side stash, no second /arrow-result request. The inline stash was process-local (broke horizontal scaling) and buffered whole payloads; it and its route are removed. - Capability fallback centralized in result-delivery.ts (errorCode-first classification): INLINE+ARROW_STREAM (Reyden) -> EXTERNAL_LINKS -> a structured ARROW_DELIVERY_UNSUPPORTED error; the JSON path retries as ARROW_STREAM and decodes server-side when the warehouse is inline-only. - EXTERNAL_LINKS chunks pipe response.body one network read at a time (peak memory = one read buffer). Multi-chunk results resolve every chunk's links via next_chunk_index in the request's identity context (OBO-correct) — previously only chunk 0 was streamed (silent truncation). - Real column names ride an X-Appkit-Arrow-Columns header (statement-id ref + /columns endpoint for wide schemas); the client relabels the positional col_N schema. Hardening (pre-merge review): - writeChunk no longer hangs on a client disconnect mid-backpressure — it races drain vs close/error so the stream unwinds and the upstream reader is cancelled instead of leaking. - Fail-fast first-byte timeout returns 503 WAREHOUSE_UNAVAILABLE on a stuck warehouse. - /columns resolves under the user identity then the service principal (fixes OBO wide-schema fallback). - Retry backoff is abortable; arrow errors forward the actionable clientMessage. Green: typecheck, 3220 tests + 1 skipped, biome, build + publint. Multi-chunk external, OBO+external, and wide-schema /columns are unit-tested only (no warm standard warehouse to live-verify).
MarioCadenas
changed the base branch from
arrow329-merge-main
to
stack/arrow-3-inline-arrow-fix
…Chunk close guard Addresses three defects surfaced in external review of the Arrow rework: - Idle-timeout vs backpressure (arrow-stream-processor.ts): the per-chunk idle timeout was armed before reader.read() but stayed armed across the downstream `yield`, so a slow client backpressuring writeChunk past the timeout (60s) was mistaken for an upstream stall and aborted a healthy download. The timeout now covers only the read (armed before, cleared in a finally the instant it resolves). - OBO warehouse startup ran as the service principal (analytics.ts): the direct-binary Arrow path called getWorkspaceClient() + ensureWarehouseRunning bare in the handler body, so warehouse auto-start ran as the SP even for .obo.sql requests — unlike the SSE path, which runs readiness inside the executor (user) context. Readiness now runs via the request executor (asUser for OBO) through _ensureArrowWarehouseReady, matching the SSE path. - writeChunk hang on an already-closed socket (analytics.ts): if res was already destroyed/ended when writeChunk was entered, res.write returned false while drain/close/error had already fired, so the promise never settled and wedged the for-await loop + upstream reader. It now rejects up front when res.destroyed || res.writableEnded. Regression tests added: slow-consumer-between-reads (no false abort), already-closed writeChunk, and OBO readiness routed through the user executor. Green: typecheck, 3222 tests + 1 skipped, biome, build + publint. Signed-off-by: MarioCadenas <MarioCadenas@users.noreply.github.com>
atilafassina
approved these changes
Jul 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reworks and hardens the Arrow/JSON result-delivery path from #329.
Base:
arrow329-merge-main= #329's arrow work merged with currentmain. So this PR's diff is just the rework (not main's ~78 commits) — reviewable in isolation, then it can fold back toward #329 / main.What changed
Delivery
ARROW_STREAMstreams the raw Arrow IPC bytes on the/queryresponse body — no SSE, no server-side stash, no second/arrow-resultrequest. The stash was process-local (broke horizontal scaling) and buffered whole payloads; it and its route are removed.result-delivery.ts:INLINE+ARROW_STREAM(Reyden) →EXTERNAL_LINKS→ structuredARROW_DELIVERY_UNSUPPORTED; the JSON path retries asARROW_STREAMand decodes server-side for inline-only warehouses.EXTERNAL_LINKSchunks piperesponse.bodyone network read at a time (peak memory = one read buffer). Multi-chunk results now resolve every chunk vianext_chunk_indexin the request's identity context (OBO-correct) — previously only chunk 0 was streamed (silent truncation).X-Appkit-Arrow-Columns(+ a statement-id ref header +/columns/:idendpoint for very wide schemas); the client relabels the positionalcol_Nschema.Hardening (pre-merge review, 3 reviewers)
writeChunkno longer hangs on a client disconnect mid-backpressure — racesdrainvsclose/error, so the stream unwinds and the upstream reader is cancelled instead of leakingEXTERNAL_LINKSsilent truncation (fix above)INLINEarrow projectsstatement_idonto the result → wide-schema-Refheader works on Reyden/columnsresolves under the user identity, then the service principal (fixes OBO wide-schema fallback)500forwards the actionableclientMessage; retry backoff is abortableExecutionError.stashExhausted+ stalestash//arrow-result/"64 MiB" commentsVerification
pnpm -r typecheck,biome check,build:package+ publint — cleanwriteChunkbackpressure/disconnect, multi-chunk pagination, abortable backoff, OBO/columnsfallback/columnsare unit-tested only — no warm standard warehouse was available to live-verify. Gated integration test is ready:APPKIT_INTEGRATION_WAREHOUSE_ID=<wh> pnpm exec vitest run arrow-delivery.integrationBuilds on #329.