feat(#630): migrate query execution/export off generic client mechanics, delete transport seam (phase 7)

[BorisTyshkevich]

Summary

Phase 7 of #630 ("Epic: extract the Fetch-native ClickHouse client"). Migrates SQL
Browser's query execution and export off the generic runQuery()/exportQuery()/
ordinary (mutable-context) killQuery() functions and onto the authenticated request
layer established in Phase 6, then deletes the now-superseded generic functions and
the legacy transport seam (src/net/clickhouse-http-transport.ts,
src/net/clickhouse-transport.types.ts) outright — no forwarding alias, no second
implementation left anywhere.

QueryExecutionService/ExportService keep all SQL Browser policy (format mapping,
row-limit caps — now uniformly across Table/KPI/TSV/explicit-raw with regression
coverage that didn't exist before — retry classification, script stop-on-first-failure,
owner-scoped cancellation) while consuming the authenticated seam directly.
ConnectionSession.captureCancellationLease widened to accept an expectedEpoch
parameter (internal semantics unchanged — verified byte-identical against origin/main
by two independent reviewers). killQueryWithLease was rewritten onto the package's
own stateless client.killQuery(...) — confirmed it still never touches mutable
ChCtx/credential/refresh/lifecycle state, the same invariant established in Phase 6.

Claims A14 (QueryExecutionService owns logical policy, not generic HTTP/stream
mechanics), A15 (ExportService streams native bytes via package classification/
late-exception framing), A16 (superseded generic mechanics deleted, not retained as
a second implementation). A1-A13 already shipped; A17/A18 remain deferred to Phase 8.

Implementation

Approved plan (High + Large risk classification) implemented via a decompose-and-implement
loop: 7 sub-tasks, sequential (the plan's own dependsOn graph left no genuinely
independent sub-tasks for this migrate-then-delete unit). One sub-task died mid-response
from a transient API error and was cleanly re-run from the same definition. Plus 4 more
commits from the mandatory High-risk + security pre-PR review pass (below) and one
documentation-reconciliation commit — 12 commits total.

Pre-PR review (mandatory for this High-risk, 7-sub-task decomposed unit)

Two independent read-only reviews (a High-risk pass, and a security-focused pass
standing in for this repo's security-review skill, which isn't installed in this
environment) both converged on the same findings — no security vulnerability found in
either. All were fixed and independently re-verified before this PR opened:

• (High) Missing real-browser export-cancellation-after-headers proof — the plan's
  own Checkpoint 3/A15 requirement. Added tests/e2e/export-post-header-cancel.{html,spec.js}
  driving the real ExportService/authenticatedResponse/package killQuery against a
  real cross-origin fault server, proving header settlement, a 32 KiB-holdback commit, a
  deterministic mid-read cancel, writer cleanup + .partial semantics, a correct
  owner-epoch remote KILL, and no offline/refresh misclassification. Green on both
  Chromium and WebKit, verified 20+ consecutive runs for flake.
• (High) Missing check:arch resurrection guards — the plan's own §22/A16
  requirement. Extended build/lib/check-legacy-owners.mjs (the existing
  real-TypeScript-parser mechanism from Phases 3/5/6 — no new hand-rolled scanner) with
  a declaration-scoped AST check banning top-level resurrection of runQuery/
  exportQuery/ordinary killQuery/the retired types, plus path-existence guards for
  the two deleted transport files. Verified live: reintroducing any of these makes
  check:arch fail for the stated reason; the package's own client.killQuery(...)
  call inside the frozen-lease path is correctly never flagged (it's a property access,
  not a top-level declaration).
• (Medium) Stale, factually-wrong cast in src/ui/app.ts — a "CRITICAL bridge" cast
  asserting killQueryWithLease still needed a 3rd argument, left over from an
  intermediate checkpoint and never cleaned up when the real cutover commit rewrote the
  signature to 2 params. Removed; both call sites now call the real 2-arg signature
  directly.
• Documentation/CHANGELOG reconciliation — docs/ARCHITECTURE.md gained a new
  Phase 7 section and every stale present-tense reference to the deleted
  runQuery/exportQuery/transport seam elsewhere in the file was corrected;
  CHANGELOG.md [Unreleased] Phase 7 entry added; .wiki/{Architecture,Source-Map, Decisions-and-Roadmap}.md updated to match.

Gate

npm run check:types    ✅
npm run check:arch      ✅ (11 active rules, 0 violations; sabotage-verified live)
npm run check:schemas   ✅
npm run check:examples  ✅
npm test                ✅ 100/97.11/100/100 coverage
npm run build            ✅ dist/sql.html built
npm run test:client-spike ✅ 118 passed, 19 pre-existing skips
tests/e2e/export-post-header-cancel.spec.js (Chromium + WebKit) ✅

Part of #630.

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01LwFPT465eDJqYcRa8HGNLz

[BorisTyshkevich]

ChatGPT review pass 2

Reviewed head: c97ea0f3d03a2c5d8fe50b755e41934bb165bddc

P2 — the new export cancellation e2e does not actually cancel an already-pending reader.read()

tests/e2e/export-post-header-cancel.html calls exportService.cancelExport() synchronously from the first progress update() callback. In production ExportService.streamToFile(), that callback is invoked after await writable.write(...) but before the loop reaches its next await reader.read(). Therefore the abort happens before the second reader.read() is issued, not while that read is pending.

The fixture still proves useful post-header behavior: bytes cross the 32 KiB hold-back and are written, cancellation leads to cleanup / .partial, the owner epoch + query id reach remote KILL, and there is no refresh/offline/response.text() regression. But the spec's stronger claim that it exercises “the pending second reader.read()” is not demonstrated, so the explicit Phase-7 mid-read acceptance proof remains incomplete.

A deterministic fix is to defer cancelExport() from the first progress callback, e.g. queueMicrotask(() => exportService.cancelExport()). After update() returns, the same streamToFile() continuation advances to the loop top, issues the held next reader.read(), and suspends; the queued cancellation then aborts that already-pending read. Keep the server-side hold and the existing no-later-write/progress assertions.

Reassessment of prior findings / high-risk seams

• QES now owns Table/KPI/TSV/explicit-raw format mapping and applies positive ordinary row caps to all four branches; the dedicated explicit FORMAT CSV regression is present.
• Script over-fetch stays in params, with service values winning collisions; it is not duplicated into settings.
• The shared request contract is retargeted to package public request/deps types; the deleted local transport contract is not recreated.
• clickhouse-http-package-policy.test.js is deletion-aware while retaining the historical Phase-3 owner set.
• run-matrix.mjs no longer reads the deleted transport seam and retains symmetric source↔classification drift checks with a real-tree regression.
• official-adapter.ts no longer imports the retired ChCtx / RunQueryOptions / RunQueryResult shim types.
• killQueryWithLease uses the package client with only the frozen lease fetch/origin/authorization; no mutable ChCtx, token lookup, refresh, lifecycle callback, or local quoting path is reintroduced.
• captureCancellationLease(expectedEpoch) preserves the pre-existing same-epoch/replacement-epoch semantics; Phase 7 exposes the existing fence rather than changing its body.
• runQuery, exportQuery, ordinary killQuery, src/net/clickhouse-http-transport.ts, and src/net/clickhouse-transport.types.ts are absent at this head, with parser/path resurrection guards.
• No packages/clickhouse-http/** file is changed by this PR.

I also checked the current-head GitHub Actions e2e job: it ran 225 Chromium tests and all 225 passed. I could inspect the WebKit/client-spike tests and their current-head assertions, but I could not independently execute those local-only runs through this review environment.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review pass 3

Previously reviewed head: c97ea0f3d03a2c5d8fe50b755e41934bb165bddc

Reviewed head: 1319fcc1279edc0b5a0dd7723d220ec9d5f9640c

The new head is exactly one commit ahead of the pass-2 head, and that commit changes only tests/e2e/export-post-header-cancel.html.

P2 — queueMicrotask() proves the next reader.read() was issued, but still not that it was pending

The pass-2 ordering bug is partially fixed correctly: cancelExport() is no longer called synchronously from onProgress(), so streamToFile() returns from the progress callback, reaches the next loop iteration, and invokes reader.read() before the queued cancel microtask runs.

However, that is not sufficient to prove the read is unresolved at cancellation time. The spec itself documents that WebKit has been observed splitting the fixture's initial ~40 KiB server write into several native Fetch-stream reads. The fault server writes that whole ~40 KiB burst with one res.write(...), then waits 3 seconds before its next server write, but a server write() boundary is not a browser ReadableStream chunk boundary.

A concrete passing-without-pending-read interleaving is:

1. WebKit delivers enough of the first 40 KiB burst to cross the 32 KiB hold-back and trigger the first onProgress().
2. onProgress() queues cancelExport() as a microtask.
3. The loop invokes its next reader.read() as intended, but the remaining bytes from that same first server write are already queued in the browser stream, so read() returns an already-fulfilled promise rather than an actually pending one.
4. await yields; the earlier cancel microtask runs first and aborts the signal.
5. The fulfilled read's continuation resumes, observes signal.aborted, and throws before writing those bytes.

All current assertions still pass in that interleaving: no later write/progress, .partial cleanup, owner-epoch KILL, no refresh/offline classification. But the acceptance claim “cancel while a native reader.read() is genuinely pending” is still not distinguished from “cancel after an already-buffered read was issued but before its continuation ran.” This is especially material because the test comments explicitly acknowledge WebKit's multi-read delivery of the initial burst.

A deterministic way to close the gap is a task-queue hop, not another guessed wall-clock delay: schedule the cancel with setTimeout(() => exportService.cancelExport(), 0) (or an equivalent explicit pending-read synchronization). Any immediately-fulfilled buffered reads and their await continuations drain as microtasks before that timer task; because the server withholds further bytes/EOF for 3 seconds, the loop must then be suspended on an unresolved reader.read() when the timer fires. Keep the existing server hold and no-later-write/progress assertions.

Reassessment of the earlier findings / complete PR

• QES still owns Table/KPI/TSV/explicit-raw mapping and applies a positive ordinary row cap uniformly to all four formats.
• Script over-fetch remains in params, with the service cap overriding colliding statement params and never duplicated into settings.
• authenticatedResponse remains the thin authenticatedRequest + package success classifier, returning the exact successful native Response unread.
• Production composition still wires QES to authenticatedProgress/authenticatedText and ExportService to authenticatedResponse/authenticatedText, through the shared owner-scoped cancel callback; no signature drift was introduced.
• killQueryWithLease still builds a one-shot package client only from the frozen lease's fetch/origin/authorization and never reads mutable ChCtx/credential/refresh/lifecycle state.
• captureCancellationLease(expectedEpoch) still permits same-epoch capture (including a same-epoch refreshed credential) and rejects a replacement epoch.
• run-matrix.mjs still has the symmetric source↔classification drift guard and no dependency on the deleted transport file.
• The retired generic runQuery/exportQuery/ordinary killQuery and local transport seam remain deleted; the new fix commit touches none of that production code.
• No packages/clickhouse-http/** file is changed by the PR.

I sampled the current-head CI rather than relying only on the PR description: the new-head CI run is green, and its Chromium e2e job ran 225 tests with 225 passing. I could not independently execute the local-only WebKit/client-spike runs in this review environment.

I also could not independently reconstruct the full 13-commit PR chain through the available connector to certify the requested commit-by-commit migration ordering; the old→new fix comparison itself is exact (one commit, one e2e file), and the current-tree migrate/delete invariants above were rechecked.

VERDICT: REVISE

[BorisTyshkevich]

ChatGPT review — final certification (manual continuation, same conversation)

This session's own pr-mode pass cap (3) was spent on pass 3's initial (re-raised)
finding, so this final exchange was carried out by driving the existing conversation
tab directly (https://chatgpt.com/c/6a770007-383c-83ed-9ad9-4e925cc13d2c) rather than
opening a new session, per this repo's "one unit, one ChatGPT conversation" rule.

Pass 3 initially re-raised the pass-2 finding about the queueMicrotask() fix (commit
1319fcc) without engaging with the independent verifier's rebuttal from pass 2. The
coordinator pushed back with the precise microtask-FIFO ordering argument (queued
cancelExport() job vs. the read continuation's own .then() job, both sharing one
FIFO queue, cancel enqueued strictly first) and the concrete reason the suggested
setTimeout(..., 0) alternative would be strictly worse (a macrotask only fires after
the entire microtask queue drains, so buffered chunks could commit additional writes
before it fires). ChatGPT's response:

I was conflating two different properties: whether the underlying read() promise is
physically unresolved at the instant of abort; and whether cancellation is
deterministically inserted after the next native read has been issued but before any
result of that read can be processed by ExportService.

The first is not provable merely from the HTTP server's write boundaries if a browser
has buffered bytes. But it is not a meaningful gap in this acceptance proof, because
the second property is what governs the observable cancellation semantics of this
production loop. Buffered data cannot escape the abort fence.

Your criticism of my proposed setTimeout(..., 0) fix is also correct. [...] I retract
that recommendation.

So the pass-3 P2 finding should be withdrawn. Given the prior full review found no
other production-path issue, and the sole pass-2 defect has been corrected by the
current head 1319fcc1279edc0b5a0dd7723d220ec9d5f9640c, I have no remaining
merge-blocking finding.

VERDICT: SHIP

Reviewed head: 1319fcc1279edc0b5a0dd7723d220ec9d5f9640c