diff --git a/.wiki/Architecture.md b/.wiki/Architecture.md index bf8da99c..009c5f38 100644 --- a/.wiki/Architecture.md +++ b/.wiki/Architecture.md @@ -51,17 +51,26 @@ module mocking. ## Query path 1. The editor/controller prepares SQL and typed parameters. -2. `src/net/ch-client.js`'s exported `queryJson`/`runQuery`/`exportQuery` send the - HTTP request through `src/net/authenticated-clickhouse-request.js` (#630 - Phase 6), which owns auth/epoch/retry/lifecycle policy (moved out of - `ch-client.js`'s former `authedFetch`/`transportFor(ctx)`, deleted outright) - and builds the `@altinity/clickhouse-http` package client directly, composing - it with the package's response consumers; the callers keep their own - product-level result/error handling. The narrow transport contract - (`src/net/clickhouse-transport.types.js` + `src/net/clickhouse-http-transport.js`, - #585 Phase 1) is no longer the ordinary path — it now remains only as the - frozen-lease `killQueryWithLease` bypass's compatibility route, through - Phase 6; Phase 7 is expected to retire it. +2. `src/application/query-execution-service.js` (normal/script reads) and + `src/application/export-service.js` (exports) send their HTTP requests + through `src/net/authenticated-clickhouse-request.js`'s + `authenticatedProgress`/`authenticatedText`/`authenticatedResponse` + entrypoints (#630 Phases 6-7); `src/net/ch-client.js`'s `queryJson` (its + one remaining schema/catalog/reference caller) goes through that same + module's `authenticatedJson`. That module owns auth/epoch/retry/ + lifecycle policy (moved out of `ch-client.js`'s former `authedFetch`/ + `transportFor(ctx)`, deleted outright) and builds the + `@altinity/clickhouse-http` package client directly, composing it with + the package's response consumers. Query-execution's own Table/KPI/TSV/ + explicit-format mapping and row-cap policy now live in + `query-execution-service.js` itself (#630 Phase 7, moved off the deleted + `net/ch-client.js` `runQuery`/`exportQuery`). The narrow transport + contract (`src/net/clickhouse-transport.types.js` + + `src/net/clickhouse-http-transport.js`, #585 Phase 1) is deleted + outright in #630 Phase 7 — `killQueryWithLease`'s frozen-lease bypass + now calls the package's own stateless `killQuery` directly, and there + is exactly one generic ClickHouse HTTP transport implementation left in + the repository. 3. `JSONStringsEachRowWithProgress` is folded line by line by pure stream logic. 4. Results resolve through the panel registry to table, chart, logs, KPI, filter, text, or graph-oriented renderers. diff --git a/.wiki/Decisions-and-Roadmap.md b/.wiki/Decisions-and-Roadmap.md index b7c540a4..1e52b559 100644 --- a/.wiki/Decisions-and-Roadmap.md +++ b/.wiki/Decisions-and-Roadmap.md @@ -279,23 +279,87 @@ Two roadmap tracks are current: identical native Fetch/Response/cancellation semantics survive being driven through a real, production-shaped `AuthenticatedRequestCtx` (synthetic test credentials, one deterministic epoch) in both Chromium - and WebKit. Still deferred to **Phase 7**: `runQuery`/`exportQuery`'s - cutover onto the package's convenience consuming query APIs and their - own result/export ownership migration, the remaining - `killQuery`/`killQueryWithLease` transport migration, and deletion of - the now-superseded transport-adapter compatibility seam. See - [[Source-Map]] and [[Architecture]] for the file-level detail and - `build/check-boundaries.mjs`'s Rules A–D plus the Phase 3/5 narrow + and WebKit. **Phase 7** (below) completes the deferred work from here: + `runQuery`/`exportQuery`'s cutover and their own result/export ownership + migration, the remaining `killQuery`/`killQueryWithLease` transport + migration, and deletion of the now-superseded transport-adapter + compatibility seam. + + **Phase 7** (merged) migrates `src/application/query-execution-service.ts` + and `export-service.ts` off the generic `runQuery`/`exportQuery` and the + ordinary mutable-context `killQuery`, then deletes all three outright — + no forwarding wrapper. QES is injected three narrow authenticated + primitives (`runProgress`/`runText`/`cancel`) instead of a `ctx()` + provider, and now OWNS the Table/KPI/TSV/explicit-raw format→settings + mapping `runQuery` used to own. `runQuery` itself already computed a + positive ordinary row limit's `max_result_rows`/ + `result_overflow_mode=break` cap independently of format and applied it + uniformly on every branch; QES's rewrite preserves that exact behavior on + ALL FOUR format branches and adds the per-branch regression coverage that + behavior never previously had at this granularity, including a dedicated + explicit-FORMAT-with-row-limit case — guarding against a future rewrite + scoping the cap to only Table/KPI. `ExportService` is injected + `exportResponse`/`runEffectText`/ + `cancel` the same way; pre-header failure classification moves from its + own `resp.ok`/`resp.text()` check onto the package's + `ensureClickHouseSuccess()`, reached through a new fourth + `authenticated-clickhouse-request.ts` wrapper, `authenticatedResponse()`. + `src/ui/app.ts` adds one shared `cancelOwnedQuery(ownerEpoch, queryId)` + callback that QES's `kill()`, the workbench session's cancel, and both + `ExportService` cancel paths all delegate to. + `ConnectionSession.captureCancellationLease` widens to take an optional + `expectedEpoch` parameter (default: the current epoch): a caller holding + an older operation's owner epoch gets `null` once the session has moved + to a replacement epoch, while a same-epoch refreshed credential still + succeeds. `killQueryWithLease` is rewritten onto the package's own + stateless `client.killQuery(...)` (dropping its `sqlString` argument — + the package now owns that quoting) instead of the local transport + adapter, preserving the exact same no-`ChCtx`/no-refresh/no-retry + invariant Phase 6 established for this bypass. With every caller + migrated, `src/net/clickhouse-http-transport.ts`/ + `clickhouse-transport.types.ts` (the local compatibility transport seam + Phase 3 introduced) are deleted outright, along with + `tests/unit/clickhouse-http-transport.test.ts` — there is now exactly + one generic ClickHouse HTTP transport implementation in the repository, + the package's. + `build/check-boundaries.mjs`/`build/lib/check-legacy-owners.mjs` gain two + new resurrection guards: a path-existence check on the two deleted + transport files, and `findRetiredTopLevelApiViolations` — a real-parser + check scoped to a module's own top-level statements (never descending + into function/class/block bodies) — banning top-level `runQuery`/ + `exportQuery`/ordinary `killQuery` (and their types) from returning + anywhere under `src/**`, without rejecting the legitimate surviving + `client.killQuery(...)` member call inside `killQueryWithLease`. + `tests/unit/clickhouse-http-package-policy.test.js`'s Phase 3 + former-owner registry (`PHASE3_LEGACY_OWNER_FILES`) stays unchanged as a + historical record; its own file-read loop is replaced with explicit + absence assertions for the two Phase 7 files plus a real scan of the + surviving `src/core/stream.ts`. A new real-browser (Chromium and WebKit) + e2e fixture, `tests/e2e/export-post-header-cancel.{html,spec.js}`, proves + native post-header cancellation semantics through the actual export + path, and `tests/spike/clickhouse-client/run-matrix.mjs`'s + deletion-estimate classification is reconciled with the post-cutover + tree (its spike consumers — `current-adapter.ts`/`official-adapter.ts`/ + `parity.test.ts`/`live-sessions.test.ts` — retargeted off the retired + types onto the Phase 7 production seams, without otherwise redesigning + the official-client spike). Claims **A14**/**A15**/**A16**; **A17** + (standalone package build/pack/typecheck proof) and **A18** (final + ownership cleanup, `@clickhouse/client-web`/vendor-spike-wiring removal, + and the tested #639 extraction handoff) remain deferred to **Phase 8**. + See [[Source-Map]] and [[Architecture]] for the file-level detail and + `build/check-boundaries.mjs`'s Rules A–D plus the Phase 3/5/7 narrow legacy-owner rules for the mechanical boundary enforcement: package↔root-src ban, package zero-bare-specifier ban, root↔package-deep-import ban, the former transport/contract/`core/stream.ts` owners (Phase 3) and the former SQL-quoting owner `format.ts` plus the retired Phase-4 killQuery stopgap owner (Phase 5) all rejected from regaining any moved identifier, - and deleted implementation files (`clickhouse-type.ts`/`sql-spans.ts`/ - `quoted-span.ts`) mechanically required to stay absent. The bare-import - boundary is no longer a blanket "`src/net/**` only" rule: transport/ - protocol package APIs (`createClickHouseHttpClient`, `chUrl`, - `streamLines`, the response consumers, `ClickHouseError`) remain + deleted implementation files (`clickhouse-type.ts`/`sql-spans.ts`/ + `quoted-span.ts`) mechanically required to stay absent, and the retired + top-level `runQuery`/`exportQuery`/ordinary-`killQuery` + declarations/deleted transport files (Phase 7) mechanically banned from + returning. The bare-import boundary is no longer a blanket "`src/net/**` + only" rule: transport/protocol package APIs (`createClickHouseHttpClient`, + `chUrl`, `streamLines`, the response consumers, `ClickHouseError`) remain `src/net/**`-only, while the mechanically allowlisted pure-language exports (SQL quoting, the generic type grammar, the shared scanner) may be imported by their actual SQL Browser consumers outside `src/net/**` too — diff --git a/.wiki/Source-Map.md b/.wiki/Source-Map.md index b1d3201f..f9e94a2a 100644 --- a/.wiki/Source-Map.md +++ b/.wiki/Source-Map.md @@ -16,11 +16,9 @@ Back to [[Home]]. Related: [[Architecture]], [[Product-and-Features]]. | `src/dashboard/application/dashboard-repaint-plan.js` | pure repaint-decision arbitration extracted from `ui/dashboard.js`'s `renderDashboard` effect (#589) | | `src/ui/dashboard-tile-gestures.js` | Dashboard corner-drag resize, Command/Ctrl-drag reorder, and modifier-cue controller, extracted from `ui/dashboard.js` behind an injected `TileGestureDeps` seam (#589) | | `src/state.js` | signals-backed state model and persistence operations | -| `src/net/ch-client.js` | ClickHouse HTTP execution and schema calls; product operations, `ChCtx` (#585 Phase 1: generic request/stream mechanics delegate through the transport seam below; #630 Phase 2: `chUrl` re-exported from `@altinity/clickhouse-http`; #630 Phase 3: `streamLines` called directly, `parseExceptionText`/`findExceptionFrame`/`StreamLine`/`StreamCallbacks` re-exported; #630 Phase 4: unaffected — the package's new consuming query APIs/`killQuery` are additive and not yet consumed here; #630 Phase 5: `sqlString` also imported directly from the package, replacing the retired `../core/format.js` import; #630 Phase 6: auth/epoch/retry/lifecycle policy (`authedFetch`/`transportFor(ctx)`) MOVED to `authenticated-clickhouse-request.js` below — `ch-client.js` is now the product/query/export COMPATIBILITY owner: `ChCtx` `extends AuthenticatedRequestCtx` and adds only `dataLakeCatalogSettingUnsupported`; `queryJson()` delegates to `authenticatedJson()` with a `ClickHouseError`→`Error` compatibility translation; `runQuery`/`exportQuery` call the new module's raw `authenticatedRequest()`, keeping their own result/error/body handling; `killQueryWithLease`'s frozen-lease bypass is untouched) | -| `src/net/authenticated-clickhouse-request.js` | **New in #630 Phase 6.** The sole normal-request auth/epoch/refresh/lifecycle owner: `authenticatedRequest()` (the moved `authedFetch` trust-boundary loop, now building the package's `createClickHouseHttpClient(...).request()` directly instead of going through the compatibility transport) plus `authenticatedJson()`/`authenticatedText()`/`authenticatedProgress()`, each composing it with exactly one matching package response consumer (`consumeJsonResponse`/`consumeTextResponse`/`consumeProgressResponse`). Declares the narrow `AuthenticatedRequestCtx` seam `ch-client.js`'s `ChCtx` now extends. Named in `build/check-boundaries.mjs`'s #585 transport-leaf forbidden lists and the #512 `connectionAuthorityFiles` lifecycle-authority list | -| `src/net/clickhouse-transport.types.js` | Type-only `ClickHouseTransport` contract — `send()` ONLY since #630 Phase 3 (`streamLines`/`StreamCallbacks` moved to the package); `TransportDeps`/`TransportRequest` alias the package's own types (#585 Phase 1; #630 Phase 2). Since #630 Phase 6, its one remaining production caller is `killQueryWithLease`'s frozen-lease bypass — the normal-request path moved to `authenticated-clickhouse-request.js`, which builds the package client directly | -| `src/net/clickhouse-http-transport.js` | `createHttpTransport` — temporary compatibility adapter, REQUEST/SEND-ONLY since #630 Phase 3: `send()` delegates to `@altinity/clickhouse-http`'s `request()`; no stream member at all (`ch-client.ts`'s `runQuery` calls the package's `streamLines` directly instead) (#585 Phase 1; #630 Phases 2-3). Since #630 Phase 6, its one remaining production caller is `killQueryWithLease` | -| `packages/clickhouse-http/src/` | First-party npm workspace package (repo's first) — `url.ts` (`chUrl`, the ONE URL-serializer implementation), `client.ts` (`createClickHouseHttpClient`, the low-level request/Fetch invocation, plus #630 Phase 4's `queryJson`/`queryText`/`queryProgress` convenience methods and stateless `killQuery` — since #630 Phase 5, `killQuery` quotes through this package's own `sql-quote.ts` `sqlString`, and the Phase-4 private `quoteKillQueryId` stopgap is gone), `progress-stream.ts` (`streamLines`, the ONE progress-bearing JSON-lines read loop, plus the canonical `StreamLine`/`StreamCallbacks`/`ProgressMetaColumn` wire types), `exceptions.ts` (`parseExceptionText`, `findExceptionFrame`/`ExceptionFrame` — byte-oriented, no caller-side latin1 conversion — plus #630 Phase 4's minimal `ClickHouseError`), `response.ts` (#630 Phase 4, new — `ensureClickHouseSuccess`, `consumeJsonResponse`/`consumeTextResponse`/`consumeProgressResponse`), and — new in #630 Phase 5 — `sql-quote.ts` (`sqlString`/`quoteIdent`/`qualifyIdent`, the ONE ClickHouse SQL-quoting implementation, moved verbatim from `src/core/format.ts`), `clickhouse-type.ts` (`parseClickHouseType`/`analyzeTypeModifiers`/`canonicalType`/the wrapper+enum helpers, the ONE generic type-expression grammar, moved verbatim from `src/core/clickhouse-type.ts` minus SQL Browser's `isSupportedOptionScalar` policy, which stayed at `src/core/param-type.ts`), `sql-spans.ts` (`scanSpans`/`Span`/`SpanKind`, the ONE shared lexical scanner, re-exported because surviving SQL Browser SQL-analysis modules still need it, moved verbatim from `src/core/sql-spans.ts`), and package-private `quoted-span.ts` (`scanDelimited`, moved verbatim from `src/core/quoted-span.ts`, not re-exported) — public export only, zero runtime dependencies, zero bare-specifier imports, no SQL Browser `src/**` dependency (#630 Phase 2; progress-stream/exceptions since Phase 3; response/query/kill APIs since Phase 4 — additive, not consumed by any `src/**` caller until Phase 6; SQL quoting/type grammar/scanner since Phase 5 — real production consumers retargeted). Since #630 Phase 6, `src/net/authenticated-clickhouse-request.js` is the first real `src/**` consumer of `request()` plus the non-consuming classifier/JSON/text/progress consumers — the convenience `queryJson`/`queryText`/`queryProgress` client methods themselves still have no `src/**` consumer (Phase 7). Bare package access is now two categories: transport/protocol APIs stay `src/net/**`-only; the pure-language exports above (quoting, type grammar, scanner) may be imported by their real SQL Browser consumers anywhere outside `src/net/**` too (mechanically allowlisted, `build/check-boundaries.mjs` Rule D) | +| `src/net/ch-client.js` | ClickHouse HTTP execution and schema calls; product operations, `ChCtx` (#585 Phase 1: generic request/stream mechanics delegate through the transport seam, since deleted; #630 Phase 2: `chUrl` re-exported from `@altinity/clickhouse-http`; #630 Phase 3: `parseExceptionText`/`findExceptionFrame`/`StreamLine`/`StreamCallbacks` re-exported; #630 Phase 4: unaffected; #630 Phase 5: `sqlString` also imported directly from the package, replacing the retired `../core/format.js` import; #630 Phase 6: auth/epoch/retry/lifecycle policy (`authedFetch`/`transportFor(ctx)`) MOVED to `authenticated-clickhouse-request.js` below — `ChCtx` `extends AuthenticatedRequestCtx` and adds only `dataLakeCatalogSettingUnsupported`; `queryJson()` delegates to `authenticatedJson()` with a `ClickHouseError`→`Error` compatibility translation; #630 Phase 7: the generic `runQuery`/`RunQueryOptions`/`RunQueryResult`, `exportQuery`/`ExportQueryOptions`, and the ordinary mutable-context `killQuery` are DELETED outright — their SQL Browser policy moved to `src/application/query-execution-service.js`/`export-service.js`; `killQueryWithLease`'s frozen-lease bypass is rewritten onto the package's own stateless `client.killQuery(...)` (dropping its `sqlString` argument — the package now owns that quoting) instead of the retired local transport adapter) | +| `src/net/authenticated-clickhouse-request.js` | **New in #630 Phase 6.** The sole normal-request auth/epoch/refresh/lifecycle owner: `authenticatedRequest()` (the moved `authedFetch` trust-boundary loop, building the package's `createClickHouseHttpClient(...).request()` directly) plus `authenticatedJson()`/`authenticatedText()`/`authenticatedProgress()`, each composing it with exactly one matching package response consumer (`consumeJsonResponse`/`consumeTextResponse`/`consumeProgressResponse`). Declares the narrow `AuthenticatedRequestCtx` seam `ch-client.js`'s `ChCtx` now extends. Named in `build/check-boundaries.mjs`'s #585 transport-leaf forbidden lists and the #512 `connectionAuthorityFiles` lifecycle-authority list. **#630 Phase 7** adds a fourth wrapper, `authenticatedResponse()` (`authenticatedRequest()` + the package's `ensureClickHouseSuccess()` — the exact successful `Response` by identity, a thrown `ClickHouseError` on non-2xx, no retry): this is now the first real `src/**` consumer of every one of the package's response consumers, wired by `src/ui/app.js` into `query-execution-service.js`'s `runProgress`/`runText` and `export-service.js`'s `exportResponse`/`runEffectText` | +| `packages/clickhouse-http/src/` | First-party npm workspace package (repo's first) — `url.ts` (`chUrl`, the ONE URL-serializer implementation), `client.ts` (`createClickHouseHttpClient`, the low-level request/Fetch invocation, plus #630 Phase 4's `queryJson`/`queryText`/`queryProgress` convenience methods and stateless `killQuery` — since #630 Phase 5, `killQuery` quotes through this package's own `sql-quote.ts` `sqlString`, and the Phase-4 private `quoteKillQueryId` stopgap is gone; since #630 Phase 7 this is also the ONLY generic ClickHouse HTTP transport implementation left in the repository, since `killQueryWithLease` now calls `client.killQuery(...)` directly), `progress-stream.ts` (`streamLines`, the ONE progress-bearing JSON-lines read loop, plus the canonical `StreamLine`/`StreamCallbacks`/`ProgressMetaColumn` wire types), `exceptions.ts` (`parseExceptionText`, `findExceptionFrame`/`ExceptionFrame` — byte-oriented, no caller-side latin1 conversion — plus #630 Phase 4's minimal `ClickHouseError`), `response.ts` (#630 Phase 4, new — `ensureClickHouseSuccess`, `consumeJsonResponse`/`consumeTextResponse`/`consumeProgressResponse`), and — new in #630 Phase 5 — `sql-quote.ts` (`sqlString`/`quoteIdent`/`qualifyIdent`, the ONE ClickHouse SQL-quoting implementation, moved verbatim from `src/core/format.ts`), `clickhouse-type.ts` (`parseClickHouseType`/`analyzeTypeModifiers`/`canonicalType`/the wrapper+enum helpers, the ONE generic type-expression grammar, moved verbatim from `src/core/clickhouse-type.ts` minus SQL Browser's `isSupportedOptionScalar` policy, which stayed at `src/core/param-type.ts`), `sql-spans.ts` (`scanSpans`/`Span`/`SpanKind`, the ONE shared lexical scanner, re-exported because surviving SQL Browser SQL-analysis modules still need it, moved verbatim from `src/core/sql-spans.ts`), and package-private `quoted-span.ts` (`scanDelimited`, moved verbatim from `src/core/quoted-span.ts`, not re-exported) — public export only, zero runtime dependencies, zero bare-specifier imports, no SQL Browser `src/**` dependency (#630 Phase 2; progress-stream/exceptions since Phase 3; response/query/kill APIs since Phase 4 — additive, not consumed by any `src/**` caller until Phase 6; SQL quoting/type grammar/scanner since Phase 5 — real production consumers retargeted). Since #630 Phase 6, `src/net/authenticated-clickhouse-request.js` is the first real `src/**` consumer of `request()` plus the non-consuming classifier/JSON/text/progress consumers; since #630 Phase 7 it also consumes `ensureClickHouseSuccess()` through the new `authenticatedResponse()` wrapper — the convenience `queryJson`/`queryText`/`queryProgress` client methods THEMSELVES still have no `src/**` consumer (Phase 8's concern, not reopened by Phase 7). `src/net/clickhouse-transport.types.js`/`clickhouse-http-transport.js` (the local compatibility transport seam #585 Phase 1 introduced) are deleted outright in #630 Phase 7 — no rows of their own remain here, matching how Phase 5's deleted `src/core/clickhouse-type.ts`/`sql-spans.ts`/`quoted-span.ts` were folded into this row rather than kept as separate entries. Bare package access is now two categories: transport/protocol APIs stay `src/net/**`-only; the pure-language exports above (quoting, type grammar, scanner) may be imported by their real SQL Browser consumers anywhere outside `src/net/**` too (mechanically allowlisted, `build/check-boundaries.mjs` Rule D) | | `src/net/oauth.js` | OAuth flow/token exchange | | `src/editor/editor-port.js` | SQL editor contract and safe no-op port | | `src/editor/codemirror-adapter.js` | SQL CodeMirror 6 adapter | diff --git a/CHANGELOG.md b/CHANGELOG.md index 27ea2f28..4dd07a66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,108 @@ auto-generated per-PR notes; this file is the curated, human-readable history. ## [Unreleased] ### Added +- **#630 Phase 7: migrate query execution and export off generic + `runQuery`/`exportQuery`/mutable-context `killQuery`, then delete those + APIs and the local transport seam.** `query-execution-service.ts` no + longer takes a `ctx()` auth-context provider; it is injected three narrow + authenticated primitives instead — `runProgress` (streaming Table/KPI), + `runText` (whole-body TSV/explicit-format reads and every script + statement), and `cancel` (owner-scoped `KILL QUERY`) — and now OWNS the + Table/KPI/TSV/explicit-raw format→settings mapping `runQuery` used to + own. `runQuery` itself already computed a positive ordinary + `resultRowLimit`'s `max_result_rows`/`result_overflow_mode=break` cap + independently of format and applied it uniformly on every branch; QES's + rewrite preserves that exact behavior on ALL FOUR branches (Table/KPI/ + TSV/explicit-raw) and adds the per-branch regression coverage that + behavior never previously had at this granularity, including a + dedicated explicit-FORMAT-with-row-limit case (`FORMAT CSV` gets the same + server-side cap a Table result does) guarding against a future rewrite + naively scoping the cap to only Table/KPI. Only a caller passing `0` + (EXPLAIN/PIPELINE/ESTIMATE) stays uncapped. The script transport loop's + `SELECT_ROW_CAP` over-fetch stays in `params`, spread after `stmt.params` + so it always wins a collision, never + duplicated into `settings`. `export-service.ts` is injected + `exportResponse`/`runEffectText`/`cancel` the same way; its `ctx()` + narrows to a `SignedOutCtx` (`onSignedOut()` only) since no export path + reads mutable `ChCtx` for a transport call any more, and pre-header + failure classification is now the package's own + `ensureClickHouseSuccess()` (through the new `authenticatedResponse()` + below) instead of `ExportService`'s own `resp.ok`/`resp.text()` check — + no writable/read loop starts for a failed status. The successful + `Response`'s raw-byte streaming (32 KiB hold-back, `findExceptionFrame` + on the retained tail, `.partial` on incomplete data) and export UX are + otherwise unchanged. + + `authenticated-clickhouse-request.ts` gains a fourth wrapper, + `authenticatedResponse(ctx, request)` (`authenticatedRequest()` + the + package's `ensureClickHouseSuccess()` — exact successful `Response` by + identity, thrown `ClickHouseError` on non-2xx, no retry). `src/ui/app.ts` + wires one new shared callback, `cancelOwnedQuery(ownerEpoch, queryId)` — + QES's `kill()`, the workbench session's cancel, and both `ExportService` + cancel paths all delegate to it rather than each building their own + `killQueryWithLease` call. + `ConnectionSession.captureCancellationLease` widens to take an optional + `expectedEpoch` parameter (default: the current epoch): a caller holding + an older operation's owner epoch gets `null` once the session has moved + to a replacement epoch (new sign-in / auth-required transition), while a + same-epoch refreshed credential still succeeds. `ch-client.ts`'s + `killQueryWithLease` is rewritten onto the package's own stateless + `client.killQuery(...)` instead of the local transport adapter (same + frozen-lease invariants Phase 6 established: no `ChCtx`/token lookup, no + refresh, no lifecycle callback, no retry) and drops its `sqlString` + parameter — the package now owns `KILL QUERY`'s quoting. + + With every caller migrated, `runQuery`/`RunQueryOptions`/ + `RunQueryResult`, `exportQuery`/`ExportQueryOptions`, and the ordinary + `killQuery` are deleted from `ch-client.ts` with no forwarding wrapper, + and `src/net/clickhouse-http-transport.ts`/`clickhouse-transport.types.ts` + — the local compatibility transport seam Phase 3 introduced — are deleted + outright along with `tests/unit/clickhouse-http-transport.test.ts`: there + is now exactly one generic ClickHouse HTTP transport implementation in + the repository, the package's. + `tests/e2e/clickhouse-http-transport.{html,spec.js}`'s generic + request/progress scenarios retarget onto the package's own + `createClickHouseHttpClient(...).request()` directly, preserving every + original behavioral assertion. `build/check-boundaries.mjs`/ + `build/lib/check-legacy-owners.mjs` gain two new resurrection guards: a + path-existence check on the two deleted transport files, and + `findRetiredTopLevelApiViolations` — a real-parser check scoped to a + module's own top-level statements (never descending into function/class/ + block bodies) — banning top-level `runQuery`/`exportQuery`/ordinary + `killQuery` (and their types) from returning anywhere under `src/**`, + without rejecting the legitimate surviving `client.killQuery(...)` member + call inside `killQueryWithLease`. `tests/unit/clickhouse-http-package- + policy.test.js`'s Phase 3 former-owner registry + (`PHASE3_LEGACY_OWNER_FILES`) is unchanged — it is a historical record, + not a claim any of its files still exist — but its own unconditional + file-read loop is replaced with explicit absence assertions for the two + Phase 7 files plus a real scan of the surviving `src/core/stream.ts`. + + A new real-browser (Chromium and WebKit) e2e fixture, + `tests/e2e/export-post-header-cancel.{html,spec.js}`, proves native + post-header cancellation semantics through the actual export path — real + `createExportService`/`authenticatedResponse`/`window.fetch`/ + `AbortController`/raw stream loop/owner-scoped cancellation, with an + in-page fake file handle standing in only for the File System Access + API. `tests/spike/clickhouse-client/run-matrix.mjs`'s deletion-estimate + classification is reconciled with the post-cutover tree (retired symbols + and the transport-file disk read removed; historical #585 evidence is + not regenerated), and the spike's other consumers + (`current-adapter.ts`/`official-adapter.ts`/`parity.test.ts`/ + `live-sessions.test.ts`) are retargeted off the retired + `runQuery`/`exportQuery`/`killQuery` types onto the Phase 7 production + seams, without otherwise redesigning the official-client spike. + + Claims **A14** (QueryExecutionService owns format/cap/retry policy with + no generic HTTP/stream mechanics of its own), **A15** (ExportService + receives an authenticated native `Response`, streams bytes, and proves + post-header cancellation in both required browsers), and **A16** (the + generic run/export/ordinary-kill APIs and both local transport files are + gone, with architecture guards preventing their return). **A17** + (standalone package build/pack/typecheck proof) and **A18** (final + ownership cleanup, `@clickhouse/client-web`/vendor-spike-wiring removal, + and the tested #639 extraction handoff) remain deferred to Phase 8. + - **#630 Phase 6: compose SQL Browser authentication through one `authenticated-clickhouse-request.ts` layer over the package's `request()` and response consumers.** The normal-request auth/epoch/ @@ -42,9 +144,11 @@ auto-generated per-PR notes; this file is the curated, human-readable history. API. `runQuery()`/`exportQuery()` switch only their `authedFetch()` call to the new raw `authenticatedRequest()` entrypoint, keeping their own Table/KPI/raw format mapping, row-cap settings, non-2xx parsing, and - streaming exactly as before — their full package-consumer/result/export - cutover remains Phase 7, as does `authenticatedText()`/ - `authenticatedProgress()`'s adoption by any other caller. + streaming exactly as before at this point — their full package-consumer/ + result/export cutover, and `authenticatedText()`/`authenticatedProgress()`'s + adoption by another caller, happened in Phase 7 (above): both generic + functions are deleted outright there, not superseded by a forwarding + wrapper. `build/check-boundaries.mjs`'s two existing #585 transport-leaf forbidden lists (`clickhouse-http-transport.ts`, @@ -52,7 +156,8 @@ auto-generated per-PR notes; this file is the curated, human-readable history. lifecycle-authority list now name the new module as the current auth/ lifecycle owner they must not reach/regain — a data extension of existing rules, not a new scanner. `ch-client.ts` stays in the - transport-leaf forbidden lists too through Phase 7. + transport-leaf forbidden lists too (Phase 7 keeps it there — the two + named files themselves are what Phase 7 deletes). Real-browser coverage: `tests/e2e/clickhouse-http-transport.{html,spec.js}` gains authenticated-path variants of the existing post-header @@ -67,9 +172,9 @@ auto-generated per-PR notes; this file is the curated, human-readable history. Only A12 (one authenticated request owner over the package) and A13 (epoch/refresh/lifecycle/cancellation invariants remain regression- - tested and unchanged) are newly claimed; A14-A18 (the remaining - `runQuery`/`exportQuery`/transport-seam migration and deletion) stay - deferred to Phase 7. + tested and unchanged) are newly claimed at this point; A14-A16 (the + `runQuery`/`exportQuery`/transport-seam migration and deletion) are + claimed by Phase 7 (above), and A17/A18 remain deferred to Phase 8. - **#630 Phase 5: move ClickHouse SQL quoting and generic type-expression grammar into `@altinity/clickhouse-http`.** `sqlString`, `quoteIdent`, and @@ -120,7 +225,7 @@ auto-generated per-PR notes; this file is the curated, human-readable history. the moved `isSupportedOptionScalar` describe block now lives in `tests/unit/param-type.test.ts` alongside its relocated implementation. Phase 6 auth composition landed next (see above); Phase 7's - query/export/transport-seam cutover remains deferred. + query/export/transport-seam cutover landed after that (see above). - **#630 Phase 4: add consuming query APIs, a minimal ClickHouse HTTP error, and a stateless `KILL QUERY` to `@altinity/clickhouse-http`.** Purely diff --git a/build/check-boundaries.mjs b/build/check-boundaries.mjs index 90499cb3..b7b1acc9 100644 --- a/build/check-boundaries.mjs +++ b/build/check-boundaries.mjs @@ -44,6 +44,10 @@ import { findPackageImportUsages, PHASE5_PACKAGE_LANGUAGE_EXPORTS, mightReferencePackage, + findRetiredTopLevelApiViolations, + PHASE7_RETIRED_TOP_LEVEL_NAMES, + PHASE7_DELETED_TRANSPORT_FILES, + mightReferenceRetiredTopLevelApi, } from './lib/check-legacy-owners.mjs'; const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); @@ -493,6 +497,39 @@ for (const relFile of PHASE5_DELETED_ROOT_FILES) { } } +// Issue #630 Phase 7 — the local SQL Browser compatibility transport seam is +// retired: `killQueryWithLease`'s own rewrite (plan §10) onto the package's +// stateless `killQuery` left it with no remaining production caller, so +// there is exactly one generic ClickHouse HTTP transport implementation left +// in the repository (the package's). A path-existence check, same mechanism +// as `PHASE5_DELETED_ROOT_FILES` just above — even an empty or +// differently-implemented file at either path must fail, regardless of its +// contents (plan §29 rollback rule: never "fix" this by reintroducing either +// deleted file). +for (const relFile of PHASE7_DELETED_TRANSPORT_FILES) { + checkedFiles += 1; + if (fs.existsSync(path.join(repoRoot, relFile))) { + violations.push(`${relFile} → recreated (issue #630 Phase 7: the local compatibility transport seam is retired onto @altinity/clickhouse-http and must not be recreated)`); + } +} + +// Issue #630 Phase 7 — ban top-level resurrection of the retired generic +// runQuery/exportQuery/ordinary-killQuery APIs and their request/result +// types, anywhere under SQL Browser src/**. `findRetiredTopLevelApiViolations` +// is declaration-scoped (see its own doc comment in check-legacy-owners.mjs), +// not a blanket identifier walk, so the frozen-lease cancellation path's own +// `client.killQuery(...)` member call (the package's stateless kill) can +// never trip it — no name-based exception needed. +for (const file of collectFiles(path.join(repoRoot, 'src'))) { + const relFile = path.relative(repoRoot, file).split(path.sep).join('/'); + checkedFiles += 1; + const source = fs.readFileSync(file, 'utf8'); + if (!mightReferenceRetiredTopLevelApi(source, PHASE7_RETIRED_TOP_LEVEL_NAMES)) continue; + for (const name of findRetiredTopLevelApiViolations(source, relFile, PHASE7_RETIRED_TOP_LEVEL_NAMES)) { + violations.push(`${relFile} → top-level ${name} (issue #630 Phase 7: the generic runQuery/exportQuery/ordinary-killQuery APIs are deleted and must not be resurrected)`); + } +} + if (violations.length) { console.error('check-boundaries: architecture violations:'); for (const line of violations) console.error(` ${line}`); diff --git a/build/lib/check-legacy-owners.mjs b/build/lib/check-legacy-owners.mjs index 697d71f3..924d967e 100644 --- a/build/lib/check-legacy-owners.mjs +++ b/build/lib/check-legacy-owners.mjs @@ -243,6 +243,130 @@ export function findKillStopgapOwnerViolations(source, filename) { return findNamedIdentifierViolations(source, filename, PHASE5_KILL_STOPGAP_OWNER_FILES, PHASE5_KILL_STOPGAP_MOVED_NAMES); } +// ── Phase 7 — retired top-level API resurrection guard ────────────────────── +// +// Issue #630 Phase 7 deletes the generic, format-agnostic `runQuery`/ +// `exportQuery` functions and their request/result types, plus the ordinary +// mutable-context `killQuery` (distinct from the frozen-lease +// `killQueryWithLease` that survives, and from the package's OWN stateless +// `client.killQuery(...)` member method `killQueryWithLease` now calls +// through — plan §10/§21). Unlike the Phase 3/5 rules above +// (`findNamedIdentifierViolations`, a blanket identifier walk scoped to a +// short former-owner allowlist), this rule cannot be a blanket identifier +// walk: `client.killQuery(...)` is a legitimate, surviving production call +// (inside `killQueryWithLease` itself) whose right-hand side is ALSO a plain +// `killQuery` Identifier node to the parser — a blanket walk would reject the +// exact call the plan requires to keep working. `findRetiredTopLevelApiViolations` +// below instead inspects only `sourceFile.statements` (the module's OWN +// top-level declarations/import-bindings/export-bindings) — a +// PropertyAccessExpression like `client.killQuery` is never a top-level +// statement itself (it's an expression nested inside one), so it structurally +// cannot trip this check; no name-based exception is needed to carve it out. + +/** The exact top-level API surface Phase 7 retires. `killQuery` here means + * the ORDINARY mutable-context function (`killQuery(ctx, queryId, ...)`) + * `ch-client.ts` used to export — never `killQueryWithLease` (a different + * identifier) and never the package's own `client.killQuery(...)` member + * (never a top-level declaration in SQL Browser source at all). */ +export const PHASE7_RETIRED_TOP_LEVEL_NAMES = Object.freeze([ + 'runQuery', + 'RunQueryOptions', + 'RunQueryResult', + 'exportQuery', + 'ExportQueryOptions', + 'killQuery', +]); + +/** Issue #630 Phase 7 — the two local compatibility transport files this + * phase deletes outright (plan §14/§22): `killQueryWithLease`'s own rewrite + * onto the package's stateless `killQuery` (plan §10) leaves this transport + * with no remaining production caller, so there is exactly one generic + * ClickHouse HTTP transport implementation left in the repository (the + * package's) and neither path may return, in any form (even empty, even + * differently implemented) — a path-existence check, not a content scan. */ +export const PHASE7_DELETED_TRANSPORT_FILES = Object.freeze([ + 'src/net/clickhouse-http-transport.ts', + 'src/net/clickhouse-transport.types.ts', +]); + +/** + * Cheap textual pre-filter, same accepted-risk convention as + * `mightReferencePackage` above: a plain substring test against each + * candidate name. Unlike the package-specifier pre-filter, this deliberately + * does NOT widen for backslash-escaped spellings — an identifier spelled + * through a Unicode identifier escape (`runQuery`) is exotic enough, + * and absent from every real caller in this codebase today, that it stays + * outside this check's threat model (matching this module's own stated scope + * — "intentionally obfuscated constructs...are outside this check's threat + * model"). Gates whether a file is even worth handing to the real parser. + * + * @param {string} source + * @param {readonly string[]} [names] + * @returns {boolean} true if `source` might declare/bind one of `names` + */ +export function mightReferenceRetiredTopLevelApi(source, names = PHASE7_RETIRED_TOP_LEVEL_NAMES) { + return names.some((name) => source.includes(name)); +} + +/** + * Find every TOP-LEVEL (module-scope) declaration or import/export binding + * in `source` whose name is one of `names` — a real TypeScript parse that + * inspects only `sourceFile.statements` (never descending into function/ + * class/block bodies), so this is deliberately narrower than + * `findNamedIdentifierViolations`'s blanket identifier walk. Covers: + * - a top-level function/class/interface/type-alias declaration + * (`export function runQuery(...)`, `export interface RunQueryOptions`); + * - a top-level const/let/var binding (`export const runQuery = ...`); + * - a named import whose LOCAL binding takes one of these names — a + * forwarding-alias vector (`import { foo as runQuery } from './x.js'`); + * - a named export specifier binding one of these names — a + * forwarding-re-export vector (`export { runQuery }` / `export { foo as + * runQuery }`, including the `export { x } from 'pkg'` gateway form). + * A nested reference — a call expression, a property/member access such as + * `client.killQuery(...)`, a local variable inside a function body — is never + * a top-level statement and therefore never inspected; this is precisely how + * the plan's carve-out ("the killQuery guard must not reject + * client.killQuery(...) inside frozen cancellation") is satisfied, with no + * separate name-based exception required. Comments/strings/template literals + * are parser trivia, never AST nodes, so prose narrating the deletion can + * never false-positive. + * + * @param {string} source + * @param {string} filename repo-relative, forward-slash separated + * @param {readonly string[]} [names] + * @returns {string[]} the forbidden names found, in `names` order, deduplicated + */ +export function findRetiredTopLevelApiViolations(source, filename, names = PHASE7_RETIRED_TOP_LEVEL_NAMES) { + const banned = new Set(names); + return withParsedSource(source, filename, (sourceFile) => { + const found = new Set(); + const note = (name) => { if (name && banned.has(name)) found.add(name); }; + for (const stmt of sourceFile.statements) { + if ( + is.isFunctionDeclaration(stmt) || is.isClassDeclaration(stmt) + || is.isInterfaceDeclaration(stmt) || is.isTypeAliasDeclaration(stmt) + ) { + note(stmt.name && stmt.name.text); + } else if (is.isVariableStatement(stmt)) { + for (const decl of stmt.declarationList.declarations) { + if (decl.name && decl.name.kind === SyntaxKind.Identifier) note(decl.name.text); + } + } else if (is.isImportDeclaration(stmt)) { + const bindings = stmt.importClause && stmt.importClause.namedBindings; + if (bindings && is.isNamedImports(bindings)) { + for (const el of bindings.elements) note(el.name.text); + } + } else if (is.isExportDeclaration(stmt)) { + const clause = stmt.exportClause; + if (clause && is.isNamedExports(clause)) { + for (const el of clause.elements) note(el.name.text); + } + } + } + return names.filter((name) => found.has(name)); + }); +} + // ── Shared cheap pre-filter (review pass 2 hardening) ─────────────────────── /** diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index a34dfb6f..cc8a6c8e 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -81,11 +81,11 @@ module is tested with plain stubs at the per-file coverage gate. | Module | Owns | |---|---| | `authenticated-execution-scope` (`app.executionScope`) | one disposable, epoch-fenced registry for authenticated operation owners; closes local work synchronously and performs best-effort remote cancellation from an immutable credential lease | -| `query-execution-service` (`app.exec`) | the shared request/stream/normalize read core + the script transport loop (retry classification, stop-on-first-failure, per-attempt `query_id`); stateless `kill(queryId)` — cancellation is caller-owned (`AbortController`s live with the owning session) | +| `query-execution-service` (`app.exec`) | SQL Browser's own Table/KPI/TSV/explicit-format request mapping and positive-row-cap policy across every branch (#630 Phase 7, moved off the deleted `net/ch-client.ts` `runQuery`); the shared request/stream/normalize read core + the script transport loop (retry classification, stop-on-first-failure, per-attempt `query_id`); owner-scoped best-effort `kill(ownerEpoch, queryId)` — cancellation is caller-owned (`AbortController`s live with the owning session) | | `connection-session` (`app.conn`) | authoritative auth + connection lifecycle (`starting` / `connected` / `refreshing` / `offline` / `auth-required` / `reauthenticating` / `signed-out`), OAuth PKCE login/refresh, Basic probing, IdP config, identity, token storage, sign-out, and **the single live `chCtx` object** (mutated in place — `authConfirmed` by `net/authenticated-clickhouse-request`, `origin` by sign-in — never reconstructed) | | `schema-catalog-service` (`app.catalog`) | server version, schema tree, lazy columns, SQL reference/completions, entity-doc cache; catalog/schema/reference/docs transports share a connection-generation abort signal, and `invalidate()` synchronously aborts them while generation fences reject stale writes | | `workbench-parameter-session` (`app.params`) | `{name:Type}` analysis/prepare/gate policy, input-vs-execute hardening, enum inference, recent values; reads the live shared `AppState` slices through accessors | -| `export-service` (`app.exports`) | direct + script export behind an injectable `ExportSink` (`pickFile`/`pickDirectory`); hold-back exception inspection, `.partial` semantics, its own cancellation state | +| `export-service` (`app.exports`) | direct + script export behind an injectable `ExportSink` (`pickFile`/`pickDirectory`); hold-back exception inspection, `.partial` semantics, its own owner-scoped cancellation state (#630 Phase 7) | | `query-document-session` (`app.queryDoc`) | Spec evaluation/diagnostics/dirty flags over `QueryTab`s, editor-mode policy | | `saved-query-service` (`app.saved`) | create/commit saved queries (validate-before-persist), history recording, share-URL building — typed results; the shell renders messages | | `schema-graph-session` (`app.graph`) | lineage load/expand/node-detail lifecycle with stale-request guards; abort state is session-private | @@ -227,10 +227,15 @@ suffice, and coverage is genuine. ## Query execution -`runQuery` in `net/ch-client.ts` streams `JSONStringsEachRowWithProgress`, -folded via the pure `applyStreamLine`; a single automatic token refresh on -401/403/`token_verification_exception` (before `authConfirmed` flips, an auth -failure signs out; after, it is a query error). +`query-execution-service.ts`'s Table branch streams +`JSONStringsEachRowWithProgress` (KPI streams `JSONEachRowWithProgress`) +through `authenticated-clickhouse-request.ts`'s `authenticatedProgress()`, +folded via the pure `applyStreamLine` (**#630 Phase 7** — this mapping used +to live in `net/ch-client.ts`'s `runQuery`, deleted that phase; see its own +section below). A single automatic token refresh on 401/403/ +`token_verification_exception` happens one layer down, in +`authenticatedRequest()` (#630 Phase 6): before `authConfirmed` flips, an +auth failure signs out; after, it is a query error. ### Transport seam (#585 Phase 1) and the clickhouse-http package (#630 Phases 2-4) @@ -269,10 +274,13 @@ and `ChCtx` exactly as before; a module-private `transportFor(ctx)` delegated unconditionally to `createHttpTransport` for the request/send half — `ChCtx` gained no field and there was no runtime transport switch. (**#630 Phase 6**, documented in its own section below, later moves that auth/epoch/retry/ -lifecycle policy itself out of `ch-client.ts` into a new module.) `runQuery` -(itself under `src/net/**`) calls the package's `streamLines` directly rather -than going through the transport seam, since there is exactly one production -stream implementation and no longer a stream member on the contract. Through +lifecycle policy itself out of `ch-client.ts` into a new module.) At this +point, `runQuery` (itself under `src/net/**`) called the package's +`streamLines` directly rather than going through the transport seam, since +there is exactly one production stream implementation and no longer a stream +member on the contract — **#630 Phase 7** later deletes `runQuery` outright +and moves that direct-`streamLines`-via-`authenticatedProgress()` call into +`query-execution-service.ts` (its own section below). Through Phase 5, `authedFetch` snapshotted the caller's `settings`/`params` synchronously at entry, before its first await, calling the package's `chUrl` directly as an eager pre-credential preflight (a malformed value @@ -350,7 +358,9 @@ package consuming-query APIs. **Phase 6** (below) moves that auth/epoch/ retry/lifecycle policy to a new module and switches `queryJson` onto its JSON response consumer; `runQuery`/`exportQuery`'s consuming-query-API cutover, and the remaining `killQuery`/`killQueryWithLease` migration, -stay Phase 7. +happened in **Phase 7** ("Query execution and export migration" below): +both generic functions are deleted outright, not superseded by a forwarding +wrapper. ### SQL quoting and the generic type grammar (#630 Phase 5) @@ -457,6 +467,9 @@ adopts the new consumer without changing an existing SQL Browser API. new raw `authenticatedRequest()` entrypoint, keeping their own Table/KPI/raw format mapping, row-cap settings, non-2xx parsing, and streaming exactly as before. `killQuery()` inherits the new path indirectly through `queryJson()`. +(**#630 Phase 7** deletes all three of `runQuery`/`exportQuery`/this +ordinary `killQuery` outright once their SQL Browser policy moves to +`query-execution-service.ts`/`export-service.ts` — see below.) `killQueryWithLease()`'s frozen-lease bypass is untouched: it already built its own one-shot transport directly from the frozen lease's exact origin/ Authorization/Fetch authority, never through `ChCtx`, so it does not — and @@ -470,12 +483,186 @@ owner they must not reach or regain, alongside `ch-client.ts` (kept through Phase 7). This is a data extension of two existing dependency rules plus one existing lifecycle-authority list — no new scanner. -Deferred to **Phase 7**: `runQuery`/`exportQuery`'s cutover onto the -package's consuming query APIs and result/export ownership, the remaining -`killQuery`/`killQueryWithLease` transport migration, and deletion of the +**#630 Phase 7** (below) completed this deferred work: `runQuery`/ +`exportQuery`'s cutover onto SQL Browser's own `query-execution-service.ts`/ +`export-service.ts` policy layers (not the package's convenience consuming +query APIs — `queryJson`/`queryText`/`queryProgress` still have no `src/**` +consumer, Phase 8's concern), the `killQuery`/`killQueryWithLease` transport +migration onto the package's own stateless `killQuery`, and deletion of the now-superseded `clickhouse-http-transport.ts`/`clickhouse-transport.types.ts` -compatibility seam (still used by `killQueryWithLease` and by the real- -browser harness's raw/unauthenticated scenarios through Phase 6). +compatibility seam — which had been kept alive by `killQueryWithLease` and +by the real-browser harness's raw/unauthenticated scenarios through Phase 6. +See "Query execution and export migration (#630 Phase 7)" below. + +### Query execution and export migration (#630 Phase 7) + +Phase 7 moves SQL Browser's own request-shape and row-cap policy out of +`net/ch-client.ts` and into the two application services that already +owned everything downstream of it, then deletes the generic mechanics those +services used to call through. + +`query-execution-service.ts` no longer takes a `ctx()` auth-context +provider at all — it is injected exactly three narrow authenticated +primitives instead: `runProgress` (streaming Table/KPI reads), `runText` +(whole-body TSV/explicit-format reads, plus every script statement, effect +or row-returning alike), and `cancel` (owner-scoped best-effort +`KILL QUERY`). It now OWNS the Table/KPI/TSV/explicit-raw format→settings +mapping that used to live inside `runQuery` +(`JSONStringsEachRowWithProgress`/`JSONEachRowWithProgress` for Table/KPI +with no `wait_end_of_query`; `TabSeparatedWithNamesAndTypes`/the caller's +own format for TSV/explicit-raw with `wait_end_of_query=1`; +`add_http_cors_header=1` on every branch). `runQuery` itself already +computed a positive ordinary `resultRowLimit`'s +`max_result_rows`/`result_overflow_mode=break` cap independently of format +and spread it into `settings` uniformly for every branch — QES's rewrite +preserves that exact behavior on ALL FOUR branches (Table/KPI/TSV/ +explicit-raw), and adds the per-branch regression coverage that behavior +never previously had at this granularity, including a dedicated +explicit-FORMAT-with-row-limit case (an explicit-FORMAT SELECT such as +`FORMAT CSV` gets the same server-side cap a Table result does) — guarding +against a future rewrite naively scoping the cap to only Table/KPI. Only a +caller that deliberately passes `0` (EXPLAIN/PIPELINE/ESTIMATE) stays +uncapped. The script transport loop's own `SELECT_ROW_CAP` over-fetch stays +exactly where it was: in `params`, spread after `stmt.params` so it always +wins a collision, and never duplicated into `settings`. + +`export-service.ts` is injected two narrow authenticated primitives the +same way — `exportResponse` (the raw native `Response`, for both the +single-file export and a script's row-returning statements) and +`runEffectText` (a script's non-row effect statements) — plus the same +`cancel` callback QES uses. Its `ctx()` dependency narrows to a +`SignedOutCtx` (`onSignedOut()` only): no export path reads mutable `ChCtx` +for a transport call any more. Pre-header failure classification is now the +package's own `ensureClickHouseSuccess()`, reached through +`authenticatedResponse()` (below) — `ExportService` no longer does its own +`resp.ok`/`resp.text()` check, and no writable/read loop starts for a +failed status. The successful `Response`, its raw-byte streaming +(`body.getReader()`, the 32 KiB hold-back, `findExceptionFrame` on the +retained tail, `.partial` on incomplete data), and export UX +(picker/progress ordering) are all unchanged from before Phase 7 — only how +the `Response` is obtained and classified moved. + +`authenticated-clickhouse-request.ts` gains a fourth wrapper, +`authenticatedResponse(ctx, request)`: `authenticatedRequest()` + the +package's `ensureClickHouseSuccess()` — the exact successful `Response` by +identity (`bodyUsed` stays `false`), a thrown package `ClickHouseError` on +a non-2xx, native abort/network failures propagating unmodified, no retry +added. `src/ui/app.ts`'s composition root wires `runProgress`/`runText` over +`authenticatedProgress`/`authenticatedText` (unchanged from Phase 6) and +`exportResponse`/`runEffectText` over the new `authenticatedResponse`/ +`authenticatedText`, plus one new shared callback, +`cancelOwnedQuery(ownerEpoch, queryId)` — QES's `kill()`, the workbench +session's cancel, and both `ExportService` cancel paths (direct export, +export script) all delegate to this single function rather than each +building their own `killQueryWithLease` call. It captures a lease at +`conn.captureCancellationLease(ownerEpoch)` and, only if one is returned, +calls `ch.killQueryWithLease(lease, queryId)`. + +`ConnectionSession.captureCancellationLease` widens to take an optional +`expectedEpoch` parameter (default: the current epoch) without changing its +existing internal semantics: a caller holding an older operation's owner +epoch gets `null` — not the live credential — once the session has since +moved to a REPLACEMENT epoch (a new sign-in or an auth-required +transition), while a same-epoch refreshed credential still succeeds. +Callers capture their own owner epoch once, at operation registration/start +time (the workbench session's `ActiveRun.ownerEpoch`, `ExportService`'s +`exportOwnerEpoch`/`exportScriptOwnerEpoch`), never re-reading it at cancel +time. + +`ch-client.ts`'s `killQueryWithLease` is rewritten onto the package's own +stateless kill instead of the local transport adapter: + +```ts +const client = createClickHouseHttpClient({ fetch: () => lease.fetch, origin: () => lease.origin }); +await client.killQuery({ queryId, authorization: lease.authorization }); +``` + +The exact same invariant Phase 6 already established for this bypass still +holds: no `ChCtx`/token lookup, no refresh, no lifecycle callback, no +retry — the frozen lease's own `fetch`/`origin`/`authorization` are the +only inputs — and the package (not this call site) now owns the +`KILL QUERY` SQL and its quoting, so `killQueryWithLease` drops its +`sqlString` parameter. The ordinary mutable-context +`killQuery(ctx, queryId, sqlString)` `ch-client.ts` used to export is +deleted outright — no forwarding wrapper. + +With QES, `ExportService`, and both export cancellation paths migrated off +them, the generic `runQuery`/`RunQueryOptions`/`RunQueryResult`, +`exportQuery`/`ExportQueryOptions`, and the ordinary `killQuery` are deleted +from `ch-client.ts`, and `src/net/clickhouse-http-transport.ts`/ +`clickhouse-transport.types.ts` — the local compatibility transport seam +Phase 3 introduced and Phase 6 left with `killQueryWithLease` as its one +remaining caller — are deleted outright, along with +`tests/unit/clickhouse-http-transport.test.ts`. There is now exactly one +generic ClickHouse HTTP transport implementation in the repository: the +package's. `tests/e2e/clickhouse-http-transport.{html,spec.js}`'s generic +request/progress scenarios retarget onto the package's own +`createClickHouseHttpClient(...).request()` directly, preserving every +original behavioral assertion (identity, call count, exact SQL/ +Authorization, cancellation semantics, byte fidelity) — Scenario 9 remains +`queryProgress()` coverage, not export coverage. + +`build/check-boundaries.mjs`/`build/lib/check-legacy-owners.mjs` gain two +new resurrection guards: a path-existence check that fails if either +deleted transport file reappears in any form (even empty, even +reimplemented under a different name), and +`findRetiredTopLevelApiViolations` — a real-parser check scoped to a +module's OWN top-level statements (declarations, import/export bindings), +never descending into function/class/block bodies — banning top-level +`runQuery`/`RunQueryOptions`/`RunQueryResult`/`exportQuery`/ +`ExportQueryOptions`/ordinary `killQuery` from returning anywhere under +`src/**`. Because it is declaration-scoped rather than a blanket identifier +walk, it cannot reject the legitimate surviving `client.killQuery(...)` +member call inside `killQueryWithLease` itself — a property access is +never a top-level statement, so no name-based carve-out is needed. +`tests/unit/clickhouse-http-package-policy.test.js`'s Phase 3 former-owner +registry (`PHASE3_LEGACY_OWNER_FILES`) stays exactly as it was — it is a +historical record of former owners, not a claim any of them still exist — +but the suite's own unconditional file-read loop is replaced with explicit +assertions that the two Phase 7 files are absent and that the surviving +`src/core/stream.ts` still carries no moved-name violations. + +A new real-browser (Chromium and WebKit) e2e fixture, +`tests/e2e/export-post-header-cancel.{html,spec.js}`, proves native +post-header cancellation semantics through the ACTUAL export path — real +`createExportService`/`authenticatedResponse`/`window.fetch`/ +`AbortController`/raw stream loop/owner-scoped cancellation, with an +in-page fake file handle standing in only for the File System Access API — +rather than the generic transport harness's own synthetic scenarios: a +first chunk past the 32 KiB hold-back forces an actual file write before +the fixture holds the next read pending and cancels mid-read, proving the +pending read aborts, no later write/progress occurs, writer cleanup and +`.partial` still happen, and the correct owner epoch/query ID reach remote +cancellation. + +`tests/spike/clickhouse-client/run-matrix.mjs`'s deletion-estimate +classification — which exhaustively classifies every `ch-client.ts` +top-level symbol and throws on a stale entry — is reconciled with the +post-cutover tree: the retired symbols' classifications and the +transport-file disk read are removed, and the estimator manifest/formula +match the surviving declarations. The historical #585 evidence corpus is +not regenerated just because the executable estimator changed. The spike +tree's other consumers (`current-adapter.ts`, `official-adapter.ts`, +`parity.test.ts`, `live-sessions.test.ts`) are retargeted off the retired +`runQuery`/`exportQuery`/`killQuery` types onto the Phase 7 production +seams/QES dependency shape, without otherwise redesigning the official- +client spike (that stays out of scope for this phase). + +Claims **A14** (QueryExecutionService owns format/cap/retry policy with no +generic HTTP/stream mechanics of its own), **A15** (ExportService receives +an authenticated native `Response`, streams bytes, and proves post-header +cancellation in both required browsers), and **A16** (the generic +run/export/ordinary-kill APIs and both local transport files are gone, +with architecture guards preventing their return). + +Deferred to **Phase 8**: making `packages/clickhouse-http` independently +buildable/packable/typecheckable in isolation (no root-source fallback), +removing migration scaffolding no longer needed after this phase +(compatibility-only package/root aliases, `@clickhouse/client-web` and its +executable vendor-spike wiring), and the final in-repo ownership +cleanup/architecture-guard hardening that prepares the package for +extraction into its own repository (issue #639, which starts only after +Phase 8 ships) — **A17**/**A18**. ## Build diff --git a/src/application/connection-session.ts b/src/application/connection-session.ts index dd300f50..7357ec46 100644 --- a/src/application/connection-session.ts +++ b/src/application/connection-session.ts @@ -150,10 +150,16 @@ export interface ConnectionSession { connectBasic(input: { username: string; password: string; host?: string }): Promise; signOut(): void; ensureFreshToken(): Promise; - /** Snapshot exact cancellation authority for the current credential epoch. - * The returned header already includes its scheme; consumers must treat it - * as opaque and never route it through normal auth/refresh code. */ - captureCancellationLease(): AuthenticatedCancellationLease | null; + /** Snapshot exact cancellation authority for the given credential epoch + * (default: the current epoch). The epoch fence rejects a stale capture: if + * the live session has since moved to a replacement epoch (a new sign-in or + * an auth-required transition, not a same-epoch token refresh), this + * returns null instead of the current credential — a caller holding an + * older operation's owner epoch can never authorize a KILL against a + * different login/session. The returned header already includes its + * scheme; consumers must treat it as opaque and never route it through + * normal auth/refresh code. */ + captureCancellationLease(expectedEpoch?: number): AuthenticatedCancellationLease | null; } export function createConnectionSession(deps: ConnectionSessionDeps): ConnectionSession { diff --git a/src/application/export-service.ts b/src/application/export-service.ts index 012c4e2b..6579fb76 100644 --- a/src/application/export-service.ts +++ b/src/application/export-service.ts @@ -46,6 +46,23 @@ // vs. results.ts's re-export of it. `src/application/**` may never import // `src/ui/**` (check:arch), so a structural mirror — not an import — is the // only option; the two are kept in sync by hand (small, stable shapes). +// +// Issue #630 Phase 7 — this service no longer depends on generic +// `exportQuery`/`runQuery`/mutable-context `killQuery`. It is injected two +// narrow authenticated primitives instead — `exportResponse` (the raw native +// `Response` for both the direct-export and script-row-export byte-stream +// paths, mirroring `authenticatedResponse`) and `runEffectText` (a script's +// non-row effect statements, mirroring `authenticatedText`) — plus `cancel`, +// the SAME owner-scoped best-effort KILL QUERY callback QES's own `kill()` +// delegates to (app.ts's `cancelOwnedQuery`, #630 Phase 7 §9.2/9.5). `ctx()` +// survives only as the narrow signed-out notifier `exportDirect`/ +// `exportScriptEntry` call on a lost token — no transport call reads it any +// more. Request shapes below (`ExportRequest`) are this service's OWN +// narrow type, never a re-export of a `net/**`/package name, so this file +// carries zero coupling to `@altinity/clickhouse-http`'s exports; a thrown +// package `ClickHouseError`'s `.message` is already the safe parsed text, so +// the existing generic `String((e instanceof Error && e.message) || e)` +// fallbacks below classify it correctly with no name check or import. import type { Signal } from '@preact/signals-core'; import { splitStatements, isRowReturning } from '../core/sql-split.js'; @@ -58,19 +75,41 @@ import { formatFileMeta, exportFilename, scriptExportName } from '../core/export // caller-side latin1 conversion — see the deleted `latin1()` helper this // file used to carry). `src/application/**` cannot import the package // directly (Rule D), so this goes through `ch-client.ts`'s zero-logic -// re-export, the same gateway this file already depends on for `exportQuery`/ -// `runQuery`/`killQuery`. +// re-export (#630 Phase 7 — the ONLY remaining `net/ch-client.ts` import this +// file needs; the transport-mechanics re-exports it used to depend on are +// gone). import { findExceptionFrame } from '../net/ch-client.js'; import type { QueryTab } from '../state.js'; import { variableDoc } from '../state.js'; import type { ResultSort } from '../core/sort.js'; -import type { ChCtx, exportQuery, runQuery, killQuery } from '../net/ch-client.js'; import type { WorkbenchParameterSession } from './workbench-parameter-session.js'; import type { AuthenticatedExecutionRegistration, AuthenticatedExecutionScope, } from './authenticated-execution-scope.js'; +// ── Injected transport request shape ──────────────────────────────────────── + +/** One authenticated ClickHouse HTTP request, exactly as this service builds + * it — this service's OWN shape (mirrors `query-execution-service.ts`'s own + * `QueryExecutionRequest`; never a re-export of a `net/**`/package type). */ +export interface ExportRequest { + sql: string; + defaultFormat: string; + settings?: Record; + params?: Record; + signal?: AbortSignal; +} + +/** The narrow signed-out-notifier surface `ctx()` still needs — no export + * path performs a transport request through it any more (#630 Phase 7): + * `exportResponse`/`runEffectText` own that now, and cancellation goes + * through `deps.cancel` instead of a mutable-context `killQuery`. A real + * `ChCtx` (`net/ch-client.ts`) satisfies this structurally without a cast. */ +export interface SignedOutCtx { + onSignedOut(): void; +} + // ── File System Access seam (moved from app.ts) ───────────────────────────── /** A `FileSystemWritableFileStream`-shaped handle — narrower than the DOM @@ -174,22 +213,35 @@ export interface ExportHooks { /** Every side effect this service needs, injected as a narrow bag — mirrors * `query-execution-service.ts`'s own `QueryExecutionDeps`/`workbench- - * session.ts`'s own `WorkbenchSessionDeps` conventions. Transport deps carry - * the exact `ch-client.js` functions this service's export paths use - * (`exportQuery` for both the single-file and per-statement-rows paths, - * `runQuery` for a script's non-row effect statements, `killQuery` for both - * cancel paths) plus a live `ctx` PROVIDER (not a snapshot — the caller may - * rebuild it after a token refresh, same as `QueryExecutionDeps.ctx`). Kept - * as the raw ch-client functions + `ctx()` rather than routed through - * `app.exec` — `app.exec`'s `executeRead`/`executeScript` return already- - * parsed results, but the export paths need the raw streaming `Response` - * itself (for `streamToFile`'s hold-back-buffer inspection), which - * `app.exec`'s surface doesn't expose. */ + * session.ts`'s own `WorkbenchSessionDeps` conventions. `exportResponse` + * (both the single-file and per-statement-rows byte-stream paths) and + * `runEffectText` (a script's non-row effect statements) are thin closures + * production wires over `authenticatedResponse`/`authenticatedText` + * (`net/authenticated-clickhouse-request.ts`); `cancel` is the SAME + * owner-scoped best-effort KILL QUERY callback (app.ts's + * `cancelOwnedQuery`) `QueryExecutionDeps.cancel` also delegates to (#630 + * Phase 7 §9.5). Kept as two narrow request-shaped functions rather than + * routed through `app.exec` — `app.exec`'s `executeRead`/`executeScript` + * return already-parsed results, but the export paths need the raw + * streaming `Response` itself (for `streamToFile`'s hold-back-buffer + * inspection), which `app.exec`'s surface doesn't expose. */ export interface ExportServiceDeps { - exportQuery: typeof exportQuery; - runQuery: typeof runQuery; - killQuery: typeof killQuery; - ctx(): ChCtx; + /** Authenticated native-Response request for a raw byte-stream export — + * the exact successful `Response` untouched (never `.text()`/`.json()`), + * package HTTP success classification, no `wait_end_of_query` (mirrors + * `authenticatedResponse`, #630 Phase 7 §12.1-12.3). Used by both + * `exportDirect` and a script's row-returning statements. */ + exportResponse(request: ExportRequest): Promise; + /** Authenticated whole-body text request for a script's non-row effect + * statements (mirrors `authenticatedText`, #630 Phase 7 §13). */ + runEffectText(request: ExportRequest): Promise; + /** Best-effort owner-scoped `KILL QUERY` (#630 Phase 7 §9.5) — local abort + * always happens first (both `cancelExport`/`cancelExportScript` below + * abort their own signal before calling this); a replacement (non-owner) + * epoch never reaches a live connection's frozen kill. */ + cancel(ownerEpoch: number | null | undefined, queryId: string | null | undefined): Promise; + /** Narrow signed-out notifier — no transport call reads this any more. */ + ctx(): SignedOutCtx; /** The disposable authenticated epoch which owns newly-started export work. * A null scope preserves this application's narrow unit-test seam; normal * UI entry points only call export while a scope is available. */ @@ -202,9 +254,6 @@ export interface ExportServiceDeps { * `onAuthFailed` hook, this service already depends on `ctx()` directly * (see above), so there's no separate hook to keep it ignorant of chCtx. */ getToken(): Promise; - /** SQL-string-quoting function `killQuery` needs (matches - * `@altinity/clickhouse-http`'s `sqlString`, issue #630 Phase 5). */ - sqlString: (s: unknown) => string; /** Perf clock — export/script-row elapsed ms, matches app.ts's `now`. */ now(): number; /** The #173 wave wall clock (epoch ms) — matches app.ts's `wallNow`; @@ -253,12 +302,18 @@ export function createExportService(deps: ExportServiceDeps): ExportService { // a grid run never clobber each other's cancel state. let exportAbort: AbortController | null = null; let exportQueryId: string | null = null; + // #630 Phase 7 §9.5 — the operation-owner epoch (the authenticated + // execution scope's `.epoch`, captured at registration/start), stored + // alongside the query id so `cancelExport`'s owner-scoped remote KILL can + // never reach a live connection with a replacement (non-owner) epoch. + let exportOwnerEpoch: number | null = null; // Script-export state (issue #99) — its own abort/query-id, reassigned each // iteration so Cancel reaches the in-flight statement, and kept distinct // from both the workbench session's own run bookkeeping and the single- // export state above. let exportScriptAbort: AbortController | null = null; let exportScriptQueryId: string | null = null; + let exportScriptOwnerEpoch: number | null = null; let exportScriptCancelled = false; let exportScriptTick: ReturnType | null = null; let nextScriptWave = 0; @@ -275,6 +330,7 @@ export function createExportService(deps: ExportServiceDeps): ExportService { if (exportAbort !== controller) return; exportAbort = null; exportQueryId = null; + exportOwnerEpoch = null; deps.state.exporting.value = false; } @@ -284,6 +340,7 @@ export function createExportService(deps: ExportServiceDeps): ExportService { exportScriptTick = null; exportScriptAbort = null; exportScriptQueryId = null; + exportScriptOwnerEpoch = null; activeScriptWave = null; deps.state.exporting.value = false; } @@ -341,10 +398,15 @@ export function createExportService(deps: ExportServiceDeps): ExportService { // Register before the native picker. Auth can be lost while that modal is // open, and a picker that eventually resolves must not proceed to config, // token, transport, or a late toast in the next authenticated epoch. + const scope = deps.executionScope(); exportAbort = controller; exportQueryId = null; + // #630 Phase 7 §9.3/9.5 — captured once, at wave start: an explicit + // Cancel presses this wave's OWN epoch, permitting a same-epoch + // refreshed credential but rejecting a replacement (non-owner) one. + exportOwnerEpoch = scope?.epoch ?? null; deps.state.exporting.value = true; - const registration = deps.executionScope()?.register({ + const registration = scope?.register({ name: 'single-file export', abort: () => { controller.abort(); @@ -388,11 +450,17 @@ export function createExportService(deps: ExportServiceDeps): ExportService { progress = deps.hooks.showExportProgress(cancelExport); if (!isCurrent(registration)) return; try { - const resp = await deps.exportQuery(deps.ctx(), sql, { - queryId: waveQueryId, signal: controller.signal, format, - // Native query-parameter substitution (#134/#173), same as run() — - // paramArgs is the wave-start snapshot captured above (review F6). - params: { ...deps.sessionParamsFor(tab, [sql]), ...paramArgs }, + const resp = await deps.exportResponse({ + sql, + defaultFormat: format || 'TabSeparatedWithNames', + params: { + ...(waveQueryId ? { query_id: waveQueryId } : {}), + // Native query-parameter substitution (#134/#173), same as run() — + // paramArgs is the wave-start snapshot captured above (review F6). + ...deps.sessionParamsFor(tab, [sql]), + ...paramArgs, + }, + signal: controller.signal, }); if (!isCurrent(registration)) return; const tag = resp.headers.get('X-ClickHouse-Exception-Tag'); // null on servers < 24.11 @@ -497,10 +565,12 @@ export function createExportService(deps: ExportServiceDeps): ExportService { } } - // Mirrors cancel() (the grid run) but on the export's own id/abort. + // Mirrors cancel() (the grid run) but on the export's own id/abort. #630 + // Phase 7 §9.5: local abort happens first, then a best-effort owner-scoped + // remote KILL (fire-and-forget, same as before). function cancelExport(): void { if (exportAbort) exportAbort.abort(); - deps.killQuery(deps.ctx(), exportQueryId, deps.sqlString); + deps.cancel(exportOwnerEpoch, exportQueryId); } // Directory picker first (transient-activation rule, same as exportDirect's @@ -530,7 +600,11 @@ export function createExportService(deps: ExportServiceDeps): ExportService { activeScriptWave = wave; exportScriptCancelled = false; deps.state.exporting.value = true; - const registration = deps.executionScope()?.register({ + const scope = deps.executionScope(); + // #630 Phase 7 §9.3/9.5 — captured once, at wave start (mirrors + // exportDirect's `exportOwnerEpoch`). + exportScriptOwnerEpoch = scope?.epoch ?? null; + const registration = scope?.register({ name: 'script export', abort: () => { exportScriptCancelled = true; @@ -619,10 +693,19 @@ export function createExportService(deps: ExportServiceDeps): ExportService { deps.hooks.renderResults(); try { if (e.type !== 'rows') { - const out = await deps.runQuery(deps.ctx(), execStmt, - { format: 'TSV', signal, queryId: exportScriptQueryId, params }); + // #630 Phase 7 §13 — effect statement wire shape: whole-body + // authenticated text, TabSeparatedWithNamesAndTypes, + // wait_end_of_query=1 + CORS. A non-2xx/abort/network failure now + // THROWS (package consumers throw, §6.5) straight into the + // shared `catch (ex)` below — no local `out.error` check needed. + await deps.runEffectText({ + sql: execStmt, + defaultFormat: 'TabSeparatedWithNamesAndTypes', + settings: { wait_end_of_query: 1, add_http_cors_header: 1 }, + params: { ...(exportScriptQueryId ? { query_id: exportScriptQueryId } : {}), ...params }, + signal, + }); if (!current()) return; - if (out.error != null) throw new Error(out.error); e.status = 'ok'; } else { const { ext } = formatFileMeta(format); @@ -631,8 +714,12 @@ export function createExportService(deps: ExportServiceDeps): ExportService { e.file = name; const fileHandle = await dir.getFileHandle(name, { create: true }); if (!current()) return; - const resp = await deps.exportQuery(deps.ctx(), sql, - { queryId: exportScriptQueryId, signal, format, params }); + const resp = await deps.exportResponse({ + sql, + defaultFormat: format || 'TabSeparatedWithNames', + params: { ...(exportScriptQueryId ? { query_id: exportScriptQueryId } : {}), ...params }, + signal, + }); if (!current()) return; const tag = resp.headers.get('X-ClickHouse-Exception-Tag'); const midErr = await streamToFile(resp, fileHandle, @@ -671,11 +758,12 @@ export function createExportService(deps: ExportServiceDeps): ExportService { } } - // Mirrors cancelExport but on the script's own active id/abort. + // Mirrors cancelExport but on the script's own active id/abort (#630 Phase + // 7 §9.5: local abort first, then a best-effort owner-scoped remote KILL). function cancelExportScript(): void { exportScriptCancelled = true; // stops the loop from starting the next statement if (exportScriptAbort) exportScriptAbort.abort(); - deps.killQuery(deps.ctx(), exportScriptQueryId, deps.sqlString); + deps.cancel(exportScriptOwnerEpoch, exportScriptQueryId); } return { exportEntry, exportDirect, cancelExport, cancelExportScript }; diff --git a/src/application/query-execution-service.ts b/src/application/query-execution-service.ts index f3710638..0e041a66 100644 --- a/src/application/query-execution-service.ts +++ b/src/application/query-execution-service.ts @@ -13,9 +13,30 @@ // it; `kill()` here is a stateless, one-shot best-effort `KILL QUERY` — // deliberately NOT a `cancel(operationId)` registry (see the issue #276 // discussion on why the service itself never tracks in-flight operations). +// +// Issue #630 Phase 7 — this service no longer depends on generic `runQuery`/ +// mutable-context `killQuery`, and no longer takes a `ctx()` auth-context +// provider at all: it is injected exactly THREE narrow authenticated +// primitives instead — `runProgress` (streaming Table/KPI reads), +// `runText` (whole-body TSV/explicit-format reads, plus every script +// statement — both effect and row-returning), and `cancel` (owner-scoped +// best-effort KILL QUERY, delegating to app.ts's `cancelOwnedQuery`, #630 +// Phase 7 §9.2-9.4). This service now OWNS the SQL Browser format/settings +// mapping (Table/KPI/TSV/explicit-raw — §6.1-6.4) and the ordinary positive +// row-cap policy (§2.5: applies to every one of those four branches, never +// only Table/KPI) that used to live inside `net/ch-client.ts`'s `runQuery`; +// it never imports the package's transport/protocol surface directly (Rule +// D) — `runProgress`/`runText`/`cancel` are the only side effects, and their +// request/callback shapes below are this service's OWN narrow types, not a +// re-export of any package or `net/ch-client.ts` name, so this file carries +// zero coupling to `@altinity/clickhouse-http`'s exports. A package +// `ClickHouseError` thrown by the injected primitives is never imported or +// special-cased here: it is a plain `Error` subclass whose `.message` is +// already the safe, parsed exception text, so the EXISTING generic +// `String((e instanceof Error && e.message) || e)` fallback below classifies +// it correctly with no name check — no package error TYPE ever leaks into +// this service's own result contracts (§6.5). -import type { ChCtx, RunQueryOptions, RunQueryResult } from '../net/ch-client.js'; -import type { runQuery, killQuery } from '../net/ch-client.js'; import { applyStreamLine } from '../core/stream.js'; import type { StreamResult } from '../core/stream.js'; import { isRowReturning } from '../core/sql-split.js'; @@ -24,19 +45,48 @@ import type { ScriptEntry } from '../core/script-result.js'; // ── Injected dependency seam ───────────────────────────────────────────────── +/** One authenticated ClickHouse HTTP request, exactly as this service builds + * it — this service's OWN shape (never a re-export of a `net/**`/package + * type): opaque SQL text, the exact wire format name, HTTP query-string + * settings/params, and the caller's own `AbortSignal`. */ +export interface QueryExecutionRequest { + sql: string; + defaultFormat: string; + settings?: Record; + params?: Record; + signal?: AbortSignal; +} + +/** Callbacks `runProgress` drives while streaming — `onLine`'s parameter is + * intentionally the generic shape `applyStreamLine` already accepts + * (`Record`), not a named `StreamLine` type, so this file + * never needs to reference the package's own progress-stream wire type. */ +export interface QueryProgressCallbacks { + onLine?: (line: Record) => void; + onChunk?: () => void; +} + /** Every side effect this service needs, injected as a narrow bag — production - * wires the real `net/ch-client.js` functions + browser clock/crypto/timer; - * tests inject plain stubs. Mirrors `ch-client.ts`'s own `ChCtx` seam. */ + * wires thin closures over `authenticatedProgress`/`authenticatedText` + * (`net/authenticated-clickhouse-request.ts`) and app.ts's own + * `cancelOwnedQuery`; tests inject plain stubs. */ export interface QueryExecutionDeps { - /** Runs one statement and returns its parsed/streamed outcome. */ - runQuery: typeof runQuery; - /** Best-effort `KILL QUERY` for a query_id. */ - killQuery: typeof killQuery; - /** The live ClickHouse auth context — a *provider*, not a value: the caller - * may rebuild it (e.g. after a token refresh) between calls, so the - * service always reads the current one rather than closing over a stale - * snapshot. */ - ctx: () => ChCtx; + /** Runs one authenticated request in progress-streaming mode (Table/KPI): + * drives `request`'s body through `callbacks` until the stream settles. + * Throws on a non-2xx response, an aborted signal, or a network failure — + * never returns a generic `{error}` shape (package consumers throw now, + * §6.5). */ + runProgress(request: QueryExecutionRequest, callbacks: QueryProgressCallbacks): Promise; + /** Runs one authenticated request in whole-body text mode (TSV/explicit + * format, and every script statement — effect or row-returning alike), + * resolving with the complete response text. Throws under the same + * conditions as `runProgress`. */ + runText(request: QueryExecutionRequest): Promise; + /** Best-effort owner-scoped `KILL QUERY` — delegates to app.ts's + * `cancelOwnedQuery(ownerEpoch, queryId)` (#630 Phase 7 §9.2/9.4): a + * replacement (non-owner) epoch never reaches a live connection's frozen + * kill. */ + cancel(ownerEpoch: number | null | undefined, queryId: string | null | undefined): Promise; /** Perf clock for per-statement elapsed ms. Deliberately NOT the wall clock * (`wallNow`) the #173 parameter pipeline uses for epoch-relative values — * that F6 invariant (one wall-clock snapshot per run wave, resolved before @@ -49,10 +99,6 @@ export interface QueryExecutionDeps { retryMs: number; /** Injected timer — `sleep(retryMs)` before a retry attempt. */ sleep: (ms: number) => Promise; - /** SQL-string-quoting function `killQuery` needs to build its - * `KILL QUERY WHERE query_id = …` literal (matches `core/format.js`'s - * `sqlString`). */ - sqlString: (s: unknown) => string; } // ── executeRead ────────────────────────────────────────────────────────────── @@ -120,10 +166,13 @@ export interface ScriptExecutionResult { aborted: boolean; } -/** `attemptStatement`'s outcome — `ch.runQuery`'s own `RunQueryResult` - * (`streamed` unused here), plus the two classified failures the retry - * logic branches on. */ -export interface AttemptResult extends RunQueryResult { +/** `attemptStatement`'s outcome — the successful raw text body (unused for a + * non-row-returning statement), plus the two classified failures the retry + * logic branches on. This service's own local shape (never a `net/**`/ + * package result type — §6.5). */ +export interface AttemptResult { + error?: string; + raw?: string; aborted?: boolean; transient?: boolean; } @@ -136,7 +185,10 @@ const SESSION_BUSY = /SESSION_IS_LOCKED|session .* is locked|locked by a concurr export interface QueryExecutionService { executeRead(result: StreamResult, request: ExecuteReadRequest): Promise; executeScript(request: ScriptExecutionRequest): Promise; - kill(queryId: string | null | undefined): Promise; + /** Best-effort owner-scoped `KILL QUERY` (#630 Phase 7 §9.4) — `ownerEpoch` + * is the operation's authenticated-execution-scope epoch, captured by the + * caller at registration/start time, never re-read at cancel time. */ + kill(ownerEpoch: number | null | undefined, queryId: string | null | undefined): Promise; } /** Build a `QueryExecutionService` bound to `deps`. Trivial constructor — no @@ -145,16 +197,17 @@ export interface QueryExecutionService { export function createQueryExecutionService(deps: QueryExecutionDeps): QueryExecutionService { // Run one script statement, classifying the outcome for the retry logic: a // Cancel → { aborted }; a connection-level fetch failure → { error:'Network - // error', transient } (retryable); any other throw → { error }. Otherwise the - // runQuery result itself ({ raw } | { error }). + // error', transient } (retryable); any other throw (including the package's + // ClickHouseError, whose `.message` is already the safe parsed text) → + // { error: e.message }. Otherwise the successful raw text body ({ raw }). async function attemptStatement( - stmt: string, - opts: RunQueryOptions, + request: QueryExecutionRequest, isCurrent: () => boolean, ): Promise { if (!isCurrent()) return { aborted: true }; try { - return await deps.runQuery(deps.ctx(), stmt, opts); + const raw = await deps.runText(request); + return { raw }; } catch (e) { if (e instanceof Error && e.name === 'AbortError') return { aborted: true }; return { error: e instanceof TypeError ? 'Network error' : String((e instanceof Error && e.message) || e), transient: e instanceof TypeError }; @@ -171,6 +224,15 @@ export function createQueryExecutionService(deps: QueryExecutionDeps): QueryExec // query_id, parameter preparation, session_id, and any recent-value recording. // `onChunk` is the per-read repaint hook (the workbench repaints its pane; a // tile/detached view repaints its own surface). Returns the mutated `result`. + // + // Format/settings mapping (#630 Phase 7 §6.1-6.4, moved here from + // `net/ch-client.ts`'s `runQuery`): Table/KPI stream the progress-bearing + // JSON wire formats with no `wait_end_of_query`; TSV and an explicit/raw + // caller format read the whole body as text with `wait_end_of_query=1`. + // Every branch gets `add_http_cors_header=1`, and — independently of which + // branch it is (§2.5) — a positive `rowLimit` adds the SAME + // `max_result_rows`/`result_overflow_mode` cap to `settings`. Only a caller + // that deliberately passes 0 (EXPLAIN/PIPELINE/ESTIMATE) stays uncapped. async function executeRead( result: StreamResult, { @@ -178,25 +240,37 @@ export function createQueryExecutionService(deps: QueryExecutionDeps): QueryExec }: ExecuteReadRequest, ): Promise { if (!isCurrent()) return result; + const isStreaming = format === 'Table' || format === 'KPI'; + const defaultFormat = isStreaming + ? (format === 'KPI' ? 'JSONEachRowWithProgress' : 'JSONStringsEachRowWithProgress') + : format === 'TSV' ? 'TabSeparatedWithNamesAndTypes' : format; + const cap: Record = rowLimit > 0 + ? { max_result_rows: rowLimit, result_overflow_mode: 'break' } + : {}; + const request: QueryExecutionRequest = { + sql, + defaultFormat, + settings: { + ...(isStreaming ? {} : { wait_end_of_query: 1 }), + ...cap, + add_http_cors_header: 1, + }, + params: { ...(queryId ? { query_id: queryId } : {}), ...(params || {}) }, + signal, + }; try { - const out = await deps.runQuery(deps.ctx(), sql, { - format, - resultRowLimit: rowLimit, - queryId, - signal, - params, - onLine: (json) => { - if (isCurrent()) applyStreamLine(json, result); - }, - onChunk: onChunk - ? () => { if (isCurrent()) onChunk(); } - : undefined, - }); - if (!isCurrent()) return result; - if (out.error != null) result.error = out.error; - else if (out.raw != null) { - result.rawText = out.raw; - result.progress.bytes = out.raw.length; + if (isStreaming) { + await deps.runProgress(request, { + onLine: (json) => { if (isCurrent()) applyStreamLine(json, result); }, + onChunk: onChunk + ? () => { if (isCurrent()) onChunk(); } + : undefined, + }); + } else { + const raw = await deps.runText(request); + if (!isCurrent()) return result; + result.rawText = raw; + result.progress.bytes = raw.length; } } catch (e) { if (!isCurrent()) return result; @@ -213,6 +287,13 @@ export function createQueryExecutionService(deps: QueryExecutionDeps): QueryExec // request), stopping on the first failure. Row-returning statements // (SELECT/WITH/SHOW/…) are fetched as JSONCompact capped at // SELECT_ROW_CAP; everything else runs for effect and reports OK. + // + // Script over-fetch cap placement (#630 Phase 7 §8): the row-returning cap + // lives in `params`, spread AFTER `stmt.params`, so it always wins a + // collision with a caller-supplied `max_result_rows`/`result_overflow_mode` + // — and it is NEVER also placed in `settings` (§2.3): `settings` here only + // ever carries `wait_end_of_query`/`add_http_cors_header`, the same for + // every script statement regardless of row-returning-ness. async function executeScript(req: ScriptExecutionRequest): Promise { const { statements, signal, onStatementStart, onStatementResult, @@ -226,10 +307,14 @@ export function createQueryExecutionService(deps: QueryExecutionDeps): QueryExec const rowReturning = isRowReturning(stmt.sql); // Over-fetch SELECTs by one past the display cap so a truncated result is // detectable (at exactly the cap it isn't). - const opts: RunQueryOptions = { - format: rowReturning ? 'JSONCompact' : 'TSV', - signal, - params: { ...stmt.params, ...(rowReturning ? { max_result_rows: SELECT_ROW_CAP + 1, result_overflow_mode: 'break' } : {}) }, + const defaultFormat = rowReturning ? 'JSONCompact' : 'TabSeparatedWithNamesAndTypes'; + const settings = { wait_end_of_query: 1, add_http_cors_header: 1 }; + const buildRequest = (queryId: string): QueryExecutionRequest => { + const baseParams = { query_id: queryId, ...stmt.params }; + const params = rowReturning + ? { ...baseParams, max_result_rows: SELECT_ROW_CAP + 1, result_overflow_mode: 'break' } + : baseParams; + return { sql: stmt.execSql, defaultFormat, settings, params, signal }; }; const s0 = deps.now(); // this statement's own wall-clock (grid Time column) // Fresh query_id per attempt, published before the request so Cancel @@ -237,7 +322,7 @@ export function createQueryExecutionService(deps: QueryExecutionDeps): QueryExec let queryId = deps.uid('q'); if (!isCurrent()) { aborted = true; break; } onStatementStart(i, { queryId, attempt: 1 }); - let out = await attemptStatement(stmt.execSql, { ...opts, queryId }, isCurrent); + let out = await attemptStatement(buildRequest(queryId), isCurrent); if (!isCurrent()) { aborted = true; break; } // Retry ONLY when it's safe. SESSION_IS_LOCKED means the statement was // rejected before running → safe to retry (any statement). A connection @@ -250,7 +335,7 @@ export function createQueryExecutionService(deps: QueryExecutionDeps): QueryExec if (!isCurrent()) { aborted = true; break; } queryId = deps.uid('q'); onStatementStart(i, { queryId, attempt: 2 }); - out = await attemptStatement(stmt.execSql, { ...opts, queryId }, isCurrent); + out = await attemptStatement(buildRequest(queryId), isCurrent); if (!isCurrent()) { aborted = true; break; } } if (out.aborted) { aborted = true; break; } @@ -279,11 +364,13 @@ export function createQueryExecutionService(deps: QueryExecutionDeps): QueryExec return { entries, aborted }; } - // Stop an in-flight query: best-effort KILL QUERY for `queryId` (mirrors - // app.ts's cancel(), minus the AbortController.abort() the caller performs - // itself — cancellation stays caller-owned; see the module doc above). - function kill(queryId: string | null | undefined): Promise { - return deps.killQuery(deps.ctx(), queryId, deps.sqlString); + // Stop an in-flight query: best-effort owner-scoped KILL QUERY for + // `queryId` (mirrors app.ts's cancel(), minus the AbortController.abort() + // the caller performs itself — cancellation stays caller-owned; see the + // module doc above). `ownerEpoch` fences a replacement-epoch caller from + // reaching a live connection's frozen kill (#630 Phase 7 §9.2/9.4). + function kill(ownerEpoch: number | null | undefined, queryId: string | null | undefined): Promise { + return deps.cancel(ownerEpoch, queryId); } return { executeRead, executeScript, kill }; diff --git a/src/net/authenticated-clickhouse-request.ts b/src/net/authenticated-clickhouse-request.ts index 952cf50c..59294c65 100644 --- a/src/net/authenticated-clickhouse-request.ts +++ b/src/net/authenticated-clickhouse-request.ts @@ -31,6 +31,7 @@ import { createClickHouseHttpClient, chUrl, parseExceptionText, consumeJsonResponse, consumeTextResponse, consumeProgressResponse, + ensureClickHouseSuccess, } from '@altinity/clickhouse-http'; import type { ClickHouseHttpRequest, StreamCallbacks } from '@altinity/clickhouse-http'; import { isAuthExpiredBody, authDeniedMessage } from '../core/stream.js'; @@ -208,6 +209,26 @@ export async function authenticatedRequest( } } +/** One `authenticatedRequest()` + the package's `ensureClickHouseSuccess()` — + * the authenticated counterpart of the package's own response classifier, + * for callers (raw byte-stream export) that must own body consumption + * themselves rather than going through one of the three consumer wrappers + * below. `authenticatedRequest` remains the sole owner of token/epoch/ + * refresh/offline-classification/lifecycle callbacks; this adds exactly + * ONE package HTTP success/error classification after settlement, with no + * retry and no additional Fetch. On success, resolves to the exact same + * native `Response` by identity — never cloned, never body-read, so + * `bodyUsed` stays `false` for the caller's own consumption. On a resolved + * non-2xx status, throws the package's `ClickHouseError`. A native + * abort/network/body failure from `authenticatedRequest` itself propagates + * unmodified, by identity — never wrapped as `ClickHouseError`. */ +export async function authenticatedResponse( + ctx: AuthenticatedRequestCtx, + request: AuthenticatedClickHouseRequest, +): Promise { + return ensureClickHouseSuccess(await authenticatedRequest(ctx, request)); +} + /** One `authenticatedRequest()` + the package's `consumeJsonResponse()`. * Throws the package's `ClickHouseError` on a resolved non-2xx response; * native JSON/network/abort errors propagate unchanged. */ diff --git a/src/net/ch-client.ts b/src/net/ch-client.ts index b13f25e1..0d882e90 100644 --- a/src/net/ch-client.ts +++ b/src/net/ch-client.ts @@ -13,33 +13,30 @@ import type { SchemaGraphTableRow, SchemaGraphDictRow } from '../core/schema-gra // `clickhouse-http-transport.ts`; re-exported here (with its `ChUrlOpts` // parameter type) so every existing importer — including // `tests/spike/clickhouse-client/current-adapter.ts` — keeps resolving. The -// generic request-construction/fetch mechanics live in `createHttpTransport`; +// generic request-construction/fetch mechanics lived in `createHttpTransport`; // at the time this module kept every auth/epoch/retry policy, product // operation, and `ChCtx` exactly as before, delegating through the transport // instead of calling `chUrl`/`ctx.fetch` directly (the auth/epoch/retry -// policy itself later moved out — see the Phase 6 note below). +// policy itself later moved out — see the Phase 6 note below; the transport +// itself is deleted — see the Phase 7 note below). // // Issue #630 Phase 2 — `chUrl` now comes from `@altinity/clickhouse-http` // (the package is the ONE serializer implementation, contract A5); this // module's re-export below keeps every existing importer (including the // historical official-client spike, `tests/spike/clickhouse-client/current- -// adapter.ts`) resolving unchanged. `createHttpTransport` stays imported -// from the local compatibility adapter — its composition graph is untouched. +// adapter.ts`) resolving unchanged. // // Issue #630 Phase 3 — the progress-stream read loop and the HTTP // exception-text/late-exception-frame parser are also package-owned now // (`streamLines`/`parseExceptionText`/`findExceptionFrame`, plus the -// canonical `StreamLine`/`StreamCallbacks` wire types). `runQuery` calls -// package `streamLines` directly (it is itself under `src/net/**`, so no -// seam violation) instead of going through a ChCtx-based transport for the -// stream half. `parseExceptionText`/`findExceptionFrame`/`StreamLine`/ -// `StreamCallbacks` are re-exported below as zero-logic migration plumbing: -// `src/application/**` cannot import the package directly (Rule D — its -// language-export allowlist is for the SQL Browser layers that consume -// generic ClickHouse quoting/type-grammar directly, not a general escape -// hatch), so `export-service.ts`'s `findExceptionFrame` use and this -// module's own callers of the removed root `core/stream.js` exports resolve -// through this one gateway instead. +// canonical `StreamLine`/`StreamCallbacks` wire types). +// `parseExceptionText`/`findExceptionFrame`/`StreamLine`/`StreamCallbacks` +// are re-exported below as zero-logic migration plumbing: `src/application/**` +// cannot import the package directly (Rule D — its language-export allowlist +// is for the SQL Browser layers that consume generic ClickHouse +// quoting/type-grammar directly, not a general escape hatch), so +// `export-service.ts`'s `findExceptionFrame` use resolves through this one +// gateway instead. // // Issue #630 Phase 5 — `sqlString` now comes from the package too (the ONE // quoting implementation, `sql-quote.ts`); this module is itself under @@ -55,25 +52,37 @@ import type { SchemaGraphTableRow, SchemaGraphDictRow } from '../core/schema-gra // alias, no second retry loop, and no second Authorization constructor. // `queryJson` below now delegates to that module's `authenticatedJson()` // (the first real production consumer of the package's response-consumer -// layer); `runQuery`/`exportQuery` call its raw `authenticatedRequest()` -// entrypoint directly, keeping their own result/error/body handling exactly -// as before (that further cutover is Phase 7). `ChCtx` extends the new -// module's narrower `AuthenticatedRequestCtx` rather than duplicating its -// fields — `dataLakeCatalogSettingUnsupported` is the one field genuinely -// specific to this product client, so it stays declared here, not there. -// `killQueryWithLease`'s frozen-lease bypass is UNTOUCHED and does not -// route through the new module — it never read mutable `ChCtx` auth state -// even before this move (see its own doc comment below). +// layer). `ChCtx` extends the new module's narrower `AuthenticatedRequestCtx` +// rather than duplicating its fields — `dataLakeCatalogSettingUnsupported` +// is the one field genuinely specific to this product client, so it stays +// declared here, not there. +// +// Issue #630 Phase 7 — the generic, format-agnostic `runQuery`/`exportQuery` +// and the ordinary mutable-context `killQuery` are DELETED, not superseded +// by a forwarding wrapper: their SQL Browser policy (Table/KPI/TSV/raw +// mapping, ordinary row caps, script over-fetch caps, retry/result mapping, +// export UX/streaming/late-exception handling) now lives in +// `src/application/query-execution-service.ts` and +// `src/application/export-service.ts`, driving +// `authenticated-clickhouse-request.ts`'s `authenticatedProgress`/ +// `authenticatedText`/`authenticatedResponse` directly (Checkpoints 2A/2B). +// `killQueryWithLease` (below) is REWRITTEN onto the package's own stateless +// `createClickHouseHttpClient(...).killQuery(...)` instead of the retired +// local transport adapter — the package now owns the KILL QUERY SQL and its +// quoting, so this function no longer takes a `sqlString` argument. The +// local compatibility transport (`clickhouse-http-transport.ts`/ +// `clickhouse-transport.types.ts`) is deleted in the same change — with +// `killQueryWithLease` off it, this module's last caller is gone, and there +// is exactly one generic ClickHouse HTTP transport implementation left in +// the repository (the package's). import { - chUrl, streamLines, parseExceptionText, findExceptionFrame, sqlString, ClickHouseError, + chUrl, parseExceptionText, findExceptionFrame, sqlString, ClickHouseError, + createClickHouseHttpClient, } from '@altinity/clickhouse-http'; -import type { StreamLine } from '@altinity/clickhouse-http'; -import { createHttpTransport } from './clickhouse-http-transport.js'; -import { authenticatedJson, authenticatedRequest } from './authenticated-clickhouse-request.js'; +import { authenticatedJson } from './authenticated-clickhouse-request.js'; import type { AuthenticatedRequestCtx } from './authenticated-clickhouse-request.js'; export { chUrl, parseExceptionText, findExceptionFrame }; export type { ChUrlOpts, StreamLine, StreamCallbacks } from '@altinity/clickhouse-http'; -export type { ClickHouseTransport, TransportDeps, TransportRequest } from './clickhouse-transport.types.js'; // ── Injected ctx seam ──────────────────────────────────────────────────────── @@ -249,39 +258,28 @@ async function loadDataLakeCatalogTableNames(ctx: ChCtx, db: string, signal?: Ab } } -/** - * Best-effort `KILL QUERY` for the given query_id (the client also aborts the - * stream; this stops the server-side work). Swallows errors — cancellation must - * never throw at the call site, and the user lacking the privilege is non-fatal. - */ -export async function killQuery(ctx: ChCtx, queryId: string | null | undefined, sqlString: SqlStringFn): Promise { - if (!queryId) return; - try { - await queryJson(ctx, 'KILL QUERY WHERE query_id = ' + sqlString(queryId) + ' ASYNC'); - } catch { /* best-effort */ } -} - /** Best-effort server cancellation through a frozen execution-scope lease. - * Unlike `killQuery`, this deliberately bypasses `authenticatedRequest` + * This deliberately bypasses `authenticatedRequest` * (`authenticated-clickhouse-request.ts`): no token read, refresh, retry, * lifecycle callback, or mutable auth-scheme lookup is allowed while a dead - * scope is closing. */ + * scope is closing. #630 Phase 7 — routes through the package's own + * stateless `createClickHouseHttpClient(...).killQuery(...)` instead of the + * retired local transport adapter; the package now owns the KILL QUERY SQL + * and its quoting (`sqlString`, applied internally), so this function no + * longer takes a `sqlString` parameter — the ordinary mutable-context + * `killQuery` this module used to export (which did take one) is deleted + * outright, not superseded by a forwarding wrapper. */ export async function killQueryWithLease( lease: AuthenticatedCancellationLease, queryId: string | null | undefined, - sqlString: SqlStringFn, ): Promise { if (!queryId) return; try { - // A one-shot transport built directly from the frozen lease — never the + // A one-shot client built directly from the frozen lease — never the // mutable-`ChCtx` `authenticatedRequest` — so cleanup reads no mutable // auth, token, or refresh state (hard invariant 8/13). - const transport = createHttpTransport({ fetch: () => lease.fetch, origin: () => lease.origin }); - await transport.send({ - sql: 'KILL QUERY WHERE query_id = ' + sqlString(queryId) + ' ASYNC', - defaultFormat: 'JSON', - authorization: lease.authorization, - }); + const client = createClickHouseHttpClient({ fetch: () => lease.fetch, origin: () => lease.origin }); + await client.killQuery({ queryId, authorization: lease.authorization }); } catch { /* best-effort */ } } @@ -887,141 +885,3 @@ export function loadFunctionsDocColumns(ctx: ChCtx, signal?: AbortSignal): Promi export function loadFunctionDocRow(ctx: ChCtx, sql: string, signal?: AbortSignal): Promise[] | null> { return loadDocRow(ctx, sql, signal); } - -/** `exportQuery`'s options. */ -export interface ExportQueryOptions { - queryId?: string; - signal?: AbortSignal; - format?: string; - params?: Record; -} - -/** - * Issue an uncapped export query and return the raw streaming Response so the - * caller can pipe `resp.body` straight to disk (issue #87). `format` (from - * `prepareExportSql` — the query's own FORMAT, or TSV) is set as - * `default_format`; the SQL's own FORMAT clause wins when present, so this only - * matters when the caller appended one. `queryId` tags the request so cancel - * can KILL QUERY it. No `wait_end_of_query`: that buffers the whole response - * server-side and would defeat the point of streaming to disk (see the comment - * on `runQuery`'s `extra` above) — a failure *after* headers is instead - * detected by the caller from the response body (findExceptionFrame) plus the - * `X-ClickHouse-Exception-Tag` header. A failure *before* headers throws the - * parsed CH exception, same as `queryJson`. `params` rides alongside query_id - * (the caller passes the tab's `sessionParamsFor` so an export that depends on - * an earlier `CREATE TEMPORARY TABLE` / session `SET` in the same tab sees it — - * same as `runQuery`). - */ -export async function exportQuery(ctx: ChCtx, sql: string, opts: ExportQueryOptions = {}): Promise { - const { queryId, signal, format, params } = opts; - // #630 Phase 6 — routes through the new module's raw `authenticatedRequest` - // entrypoint (was `authedFetch`); its own non-2xx parsing and successful - // raw-`Response` ownership below are unchanged (Phase 7 concern). - const resp = await authenticatedRequest(ctx, { - sql, - defaultFormat: format || 'TabSeparatedWithNames', - params: { ...(queryId ? { query_id: queryId } : {}), ...(params || {}) }, - signal, - }); - if (!resp.ok) throw new Error(parseExceptionText(await resp.text())); - return resp; -} - -/** `runQuery`'s options. - * @param format output format (default 'Table') - * @param signal aborts the request - * @param resultRowLimit caps a normal result server-side (max_result_rows + - * result_overflow_mode); 0/absent = uncapped - * @param queryId tags the request so Cancel can KILL QUERY it - * @param params extra query-string options that ride alongside query_id - * (e.g. multiquery SELECTs pass their own cap + session_id) - * @param onLine called per parsed stream object in streaming mode - * @param onChunk called once per read chunk in streaming mode - */ -export interface RunQueryOptions { - format?: string; - signal?: AbortSignal; - resultRowLimit?: number; - queryId?: string; - params?: Record; - onLine?: (line: StreamLine) => void; - onChunk?: () => void; -} - -/** `runQuery`'s result: a query error, a raw-mode body, or a completed stream. */ -export interface RunQueryResult { - error?: string; - raw?: string; - streamed?: boolean; -} - -/** - * Run a query in streaming mode (JSONStringsEachRowWithProgress) or raw mode - * (TSV/JSON). `onLine(parsedObj)` is called per stream object in streaming - * mode. Returns { error } or { raw } shape via - * the result object the caller passes in `apply`. - * - * @param ctx - * @param sql - * @param o { format, signal, resultRowLimit, params, onLine(json), onChunk() } - * `resultRowLimit` caps a normal result server-side (max_result_rows + - * result_overflow_mode); `params` are extra query-string options that ride - * alongside query_id (e.g. multiquery SELECTs pass their own cap + session_id). - */ -export async function runQuery(ctx: ChCtx, sql: string, o: RunQueryOptions = {}): Promise { - const fmt = o.format || 'Table'; - // #447 removed the `Filter` transport arm along with the Filter role — nothing - // can request that format any more. - const isStreaming = fmt === 'Table' || fmt === 'KPI'; - // Streaming gets the progress-bearing JSON; raw mode sends the requested format - // verbatim as default_format (a real ClickHouse format name from a FORMAT clause - // or an implicit EXPLAIN). 'TSV' keeps its with-names-and-types expansion. - const fmtParam = isStreaming - ? (fmt === 'KPI' ? 'JSONEachRowWithProgress' : 'JSONStringsEachRowWithProgress') - : fmt === 'TSV' - ? 'TabSeparatedWithNamesAndTypes' - : fmt; - // Cap a normal result query server-side: max_result_rows stops the read at N - // and result_overflow_mode='break' makes ClickHouse stop cleanly at a block - // boundary (no error, no further data pulled) rather than throwing. The caller - // decides scope — it passes resultRowLimit for normal SELECTs (Table + explicit - // FORMAT) and 0 for EXPLAIN/PIPELINE/ESTIMATE (which also run as 'Table', so the - // exemption can't be told apart by format here). `break` can overshoot by up to - // a block on the streaming path, which the applyStreamLine guard trims. - const cap: Record = (o.resultRowLimit ?? 0) > 0 - ? { max_result_rows: o.resultRowLimit!, result_overflow_mode: 'break' } - : {}; - // #630 Phase 6 — routes through the new module's raw `authenticatedRequest` - // entrypoint (was `authedFetch`); the Table/KPI/raw format mapping, row-cap - // settings, non-2xx parsing, and streaming below are unchanged (Phase 7 - // concern). - const resp = await authenticatedRequest(ctx, { - sql, - defaultFormat: fmtParam, - // wait_end_of_query buffers the whole response server-side so the HTTP - // status reflects errors — but it defeats progressive streaming (first rows - // wait for the query to finish: ~16s vs ~0.5s on a 1.3M-row scan). Keep it - // only for raw modes (read whole anyway); the streaming Table path drops it - // and surfaces mid-stream errors via the in-band `exception` line instead. - settings: { ...(isStreaming ? {} : { wait_end_of_query: 1 }), ...cap, add_http_cors_header: 1 }, - // Tagging the request with a query_id lets Cancel issue KILL QUERY for it. - // Caller-supplied params (o.params) ride alongside — e.g. multiquery SELECTs - // add max_result_rows / result_overflow_mode to cap the result server-side. - params: { ...(o.queryId ? { query_id: o.queryId } : {}), ...(o.params || {}) }, - signal: o.signal, - }); - - if (!resp.ok) { - return { error: parseExceptionText(await resp.text()) }; - } - if (!isStreaming) { - return { raw: await resp.text() }; - } - // Issue #630 Phase 3 — calls the package's `streamLines` directly rather - // than through a ChCtx-based transport's own stream member (retired that - // phase): this module is itself under `src/net/**` (the one layer allowed - // to import the package by bare specifier), and there is exactly one - // production stream implementation now — the package's. - await streamLines(resp.body!, { onLine: o.onLine, onChunk: o.onChunk }); - return { streamed: true }; -} diff --git a/src/net/clickhouse-http-transport.ts b/src/net/clickhouse-http-transport.ts deleted file mode 100644 index da2add59..00000000 --- a/src/net/clickhouse-http-transport.ts +++ /dev/null @@ -1,52 +0,0 @@ -// Issue #585 Phase 1 — the current custom ClickHouse HTTP transport, -// re-seated behind the `ClickHouseTransport` contract (`clickhouse-transport.types.ts`). -// Issue #630 Phase 2 — this file is now a temporary COMPATIBILITY ADAPTER: -// `chUrl`/`ChUrlOpts`, the URL construction, and the direct injected -// `fetch()` invocation moved to `@altinity/clickhouse-http` (mechanically, -// behaviorally unchanged — see that package's `url.ts`/`client.ts`). `send()` -// below delegates to the package's `request()` instead of building the -// request itself. -// -// Issue #630 Phase 3 — this file is now SEND-ONLY. `streamLines()` (the -// progress-bearing JSON-lines read loop) moved to -// `@altinity/clickhouse-http`'s own `streamLines` — `ch-client.ts`'s -// `runQuery` (itself under `src/net/**`) calls the package function -// directly instead of going through this transport seam, so this adapter no -// longer has (or forwards to) a stream member at all. There is exactly one -// production stream implementation in the repository now — the package's; -// this file does not reintroduce a second one, forwarding or otherwise. -// -// Issue #630 Phase 6 — this adapter's one remaining production caller is -// `killQueryWithLease`'s frozen-lease bypass (`ch-client.ts`); the normal -// mutable-`ChCtx` request path moved to -// `src/net/authenticated-clickhouse-request.ts`, which builds the package -// client directly rather than through this adapter. -// -// Ownership boundary: this file may depend only on `src/core` and the -// `@altinity/clickhouse-http` public package export — never on -// `ch-client.ts`, `authenticated-clickhouse-request.ts`, `oauth.ts`, -// `oauth-config.ts`, `src/application/`, or `src/ui/`. `build/check- -// boundaries.mjs` enforces this mechanically. - -import { createClickHouseHttpClient } from '@altinity/clickhouse-http'; -import type { ClickHouseTransport, TransportDeps, TransportRequest } from './clickhouse-transport.types.js'; - -/** The current custom HTTP implementation of `ClickHouseTransport`. `deps`' - * accessors are read per-request (REQUIRED-PURE — see the contract's doc - * comment) so a live, mutable `origin`/`fetch` (e.g. `ConnectionSession`'s - * `chCtx`, mutated in place on sign-in) is always observed at its current - * value, never pinned to a stale snapshot (Adaptation A5). */ -export function createHttpTransport(deps: TransportDeps): ClickHouseTransport { - const client = createClickHouseHttpClient(deps); - return { - // Kept `async` even though its body is a single delegating call: this - // exactly matches today's adapter-level settlement shape (a synchronous - // preparation error, e.g. a URIError from URL encoding, must surface as - // a REJECTED promise here too, never a synchronous throw out of - // `send()`), and makes the compatibility intent explicit rather than - // relying solely on the package implementation's own async-ness. - async send(request: TransportRequest): Promise { - return client.request(request); - }, - }; -} diff --git a/src/net/clickhouse-transport.types.ts b/src/net/clickhouse-transport.types.ts deleted file mode 100644 index d8d18004..00000000 --- a/src/net/clickhouse-transport.types.ts +++ /dev/null @@ -1,117 +0,0 @@ -// Issue #585 Phase 1 — the narrow SQL Browser ClickHouse-transport contract. -// Type-only (ADR-0002 phase-0 convention for seam contracts, hence the -// `.types.ts` suffix rather than the issue's suggested `clickhouse-transport.ts` -// — "exact names may follow repository conventions" per the issue). Puts the -// CURRENT custom HTTP implementation behind a contract a future official -// transport (gated on a new decision — ADR-0005 is Rejected) could also -// satisfy, without moving any product SQL or auth/lifecycle policy here. -// -// Issue #630 Phase 2 — `TransportDeps`/`TransportRequest` are now ALIASES of -// the low-level request/dependency types owned by `@altinity/clickhouse-http` -// (see that package's `client.ts`), not separate shapes: the package is the -// single source of truth for the low-level request boundary. `ClickHouseTransport` -// itself stays here — Phase 2 still had the SQL-Browser-local `streamLines` -// method, deferred to Phase 3. -// -// Issue #630 Phase 3 — `streamLines`/`StreamCallbacks` are GONE from this -// contract: the progress-bearing JSON-lines read loop and its callback shape -// are now package-owned (`@altinity/clickhouse-http`'s `streamLines`/ -// `StreamCallbacks`), consumed directly by `ch-client.ts`'s `runQuery` -// (which is itself under `src/net/**`) rather than through this transport -// seam. `ClickHouseTransport` is now a REQUEST/SEND-ONLY compatibility -// adapter contract — `send()` is its only member. There is exactly one -// stream implementation in the repository (the package's); this seam no -// longer describes one. -// -// Issue #630 Phase 6 — the normal-request auth/epoch/refresh/lifecycle -// policy moved out of `ch-client.ts` into -// `src/net/authenticated-clickhouse-request.ts`; this contract's own -// boundary is unaffected (this file never described that policy), but the -// forbidden-owner list below now names the new module too, since it is the -// current auth-policy owner this transport-leaf contract must not reach. -// -// Ownership boundary: this file (and its implementation, -// `clickhouse-http-transport.ts`) may depend only on `src/core` and the -// `@altinity/clickhouse-http` public package export — never on -// `ch-client.ts`, `authenticated-clickhouse-request.ts`, `oauth.ts`, -// `oauth-config.ts`, `src/application/`, or `src/ui/`, even type-only. -// `build/check-boundaries.mjs` enforces this mechanically (twin `RULES` -// entries for this file and the implementation file). - -import type { ClickHouseHttpClientDeps, ClickHouseHttpRequest } from '@altinity/clickhouse-http'; - -/** What the transport is allowed to see of the environment. Deliberately - * excludes tokens, refresh, epochs, lifecycle callbacks: a transport - * implementation is compile-time incapable of ACQUIRING credentials or - * signaling lifecycle. (It still receives the resolved Authorization header - * per request — Adaptation A6 — so single-send/no-retry/no-caching discipline - * is contract- and test-enforced, not compiler-enforced.) Accessors, not - * snapshots — the live chCtx's origin is mutated in place on sign-in and the - * transport must observe the current value per request. - * - * REQUIRED-PURE: both accessors must be synchronous, side-effect-free plain - * property reads (production: `() => ctx.fetch` / `() => ctx.origin`, or — - * for `killQueryWithLease`, this contract's one remaining production - * caller since #630 Phase 6 — `() => lease.fetch` / `() => lease.origin`). - * This matters because `send` evaluates them immediately before the fetch - * itself; the type system cannot express purity, so — exactly like A6's - * single-send discipline — this rule is enforced by this doc comment and - * review, not by the compiler or the existing epoch race test (whose proof - * stops at the `send` invocation boundary). (Until #630 Phase 6, this same - * accessor timing mattered relative to `ch-client.ts`'s own `authedFetch` - * final epoch fence; that normal-request caller now builds its package - * client directly in `src/net/authenticated-clickhouse-request.ts` instead - * of going through this contract at all — see that module's own final-fence - * comment.) */ -export type TransportDeps = ClickHouseHttpClientDeps; - -/** One ClickHouse HTTP request, fully specified. No client-level defaults - * exist: `authorization` is the complete header value (scheme + credential), - * resolved by the caller (SQL Browser auth policy) for THIS request. - * - * Field-level docs (moved to `@altinity/clickhouse-http`'s `client.ts`): - * `sql` is opaque (never parsed/rewritten/appended to — hard invariant 16: - * an authored FORMAT clause always wins over `defaultFormat` server-side); - * `settings`/`params` are today's exact wire vocabulary, unchanged - * (Adaptation A2); `authorization` is never optional or defaulted. */ -export type TransportRequest = ClickHouseHttpRequest; - -// No TransportResponse type in Phase 1 (Adaptation A3): `send` resolves with -// the NATIVE fetch `Response`. A structural subset would be assignable only in -// the direction Response -> subset, so a caller needing the real Response -// (killQueryWithLease today; `authedFetch`/`exportQuery` before #630 Phase 6 -// moved the normal-request path off this contract) could not keep a -// `Promise` signature without an unsafe cast. Native Response gives -// raw bytes (`body`, hard invariant 17) and `clone()` for a non-destructive -// error-body peek for free. - -/** The SQL Browser transport contract. Since #630 Phase 3, request/send is - * the ONLY thing this contract describes — see the module doc above for why - * `streamLines` is gone. In Phase 1 exactly one implementation exists - * (`createHttpTransport`, `clickhouse-http-transport.ts`); a Phase 2 - * official-client implementation (does not proceed without a new decision) - * would satisfy the same contract. */ -export interface ClickHouseTransport { - /** POST one query; resolves at HTTP settlement (headers received) with the - * NATIVE fetch `Response` — Phase 1 defines no adapter-owned response type - * (Adaptation A3), which is what preserves `export-service.ts`'s - * `streamToFile(resp: Response, …)` consumer without casts, and — since - * #630 Phase 6 — what lets `killQueryWithLease` (this contract's one - * remaining production caller) keep its own `Promise` best-effort - * wrapper without a cast either. Exactly one fetch invocation - * (contract-suite-asserted, incl. on non-2xx — A6); no retry, no token - * read, no lifecycle callback, no error classification, no body - * consumption. HTTP error statuses resolve normally (they are responses); - * network I/O failure / abort rejects the returned promise natively. Since - * #630 Phase 2, `send` is implemented by delegating to - * `@altinity/clickhouse-http`'s async `request()`, which itself builds the - * request URL — so a REQUEST-PREPARATION failure (e.g. a `URIError` from - * malformed `settings`/`params`) also surfaces as a rejected promise here, - * not a synchronous throw. The transport performs no error classification - * or wrapping of either failure kind — that policy distinction is made by - * the caller (before #630 Phase 6, `ch-client.ts`'s own `authedFetch`; the - * normal-request path now builds the package client directly in - * `src/net/authenticated-clickhouse-request.ts` instead, never through - * this contract). */ - send(request: TransportRequest): Promise; -} diff --git a/src/state.ts b/src/state.ts index ca10dc08..95c3dbbb 100644 --- a/src/state.ts +++ b/src/state.ts @@ -669,7 +669,8 @@ export function createState(read: StateReader = { loadJSON, loadStr }): AppState theme: read.loadStr(KEYS.theme, 'light'), density: 'comfortable', // Global cap on how many rows a normal SELECT fetches (server-side - // max_result_rows + a client-side guard; see runQuery / applyStreamLine). + // max_result_rows + a client-side guard; see query-execution-service's + // ordinary row-cap settings / applyStreamLine). // One persisted preference, default 500; a non-option stored value snaps // back to the default so the selector always reflects a real choice. resultRowLimit: normalizeRowLimit(parseInt(read.loadStr(KEYS.resultRowLimit, '500'), 10)), diff --git a/src/ui/app.ts b/src/ui/app.ts index 150a86cb..7814d8cb 100644 --- a/src/ui/app.ts +++ b/src/ui/app.ts @@ -83,6 +83,14 @@ import { createAuthenticatedExecutionScope, type AuthenticatedExecutionScope, } from '../application/authenticated-execution-scope.js'; +// Issue #630 Phase 7 — the composition root wires QES's/ExportService's +// injected authenticated progress/text/response primitives directly over +// these three seam functions (never a package import — Rule D restricts the +// package's transport/protocol surface to `src/net/**`; this module is +// `src/net/authenticated-clickhouse-request.ts`, a local file). +import { + authenticatedProgress, authenticatedText, authenticatedResponse, +} from '../net/authenticated-clickhouse-request.js'; import { createSchemaCatalogService } from '../application/schema-catalog-service.js'; import { createWorkbenchParameterSession } from '../application/workbench-parameter-session.js'; import { createChSessionParams } from '../application/ch-session-params.js'; @@ -439,6 +447,26 @@ export function createApp(env: CreateAppEnv = {}): App { const getToken = conn.getToken; const ensureConfig = conn.ensureConfig; + // #630 Phase 7 §9.2-9.4 — the SINGLE owner-scoped explicit-cancel callback: + // QES (`exec.kill`), the workbench session, and both ExportService cancel + // paths all delegate here. `ownerEpoch` is the operation's authenticated- + // execution-scope epoch, captured by the caller at registration/start time + // (never re-read at cancel time) — `conn.captureCancellationLease` fences a + // replacement (non-owner) epoch, permitting only a same-epoch refreshed + // credential (§9.3). `ch.killQueryWithLease` was rewritten onto the + // package's own stateless `killQuery` (plan §10) and now takes only + // `(lease, queryId)` — the package owns KILL QUERY's SQL and quoting, so + // this call site no longer supplies `sqlString`. + async function cancelOwnedQuery( + ownerEpoch: number | null | undefined, + queryId: string | null | undefined, + ): Promise { + if (ownerEpoch == null || !queryId) return; + const lease = conn.captureCancellationLease(ownerEpoch); + if (!lease) return; + await ch.killQueryWithLease(lease, queryId); + } + // Identity/auth/config all live on `conn` (see app.types.ts's own doc // comment) — no flat `App` delegates (#276 Phase 5 deleted them). // `showLogin`/`signOut` stay app.ts-owned: they compose rendering, not @@ -542,10 +570,20 @@ export function createApp(env: CreateAppEnv = {}): App { const sleep = (ms: number): Promise => new Promise((r) => win.setTimeout(r, ms)); // The shared request/stream/normalize + multiquery-script transport service // (#276 Phase 1) — `run()`'s single read and `runScript()`'s per-statement - // retry/classify loop both delegate to it now; `ctx: () => chCtx` keeps the - // live (possibly refreshed) auth context rather than a stale snapshot. + // retry/classify loop both delegate to it now. #630 Phase 7 — its three + // injected deps are thin closures over the authenticated request seam + // (`chCtx` read live, never snapshotted, exactly like the pre-Phase-7 + // `ctx: () => chCtx` provider did) plus the shared owner-scoped cancel + // callback defined above. const exec = createQueryExecutionService({ - runQuery: ch.runQuery, killQuery: ch.killQuery, ctx: () => chCtx, now, uid, retryMs, sleep, sqlString, + // `authenticatedProgress` resolves with the settled `Response` (unused + // here — the caller only cares that the stream was fully driven through + // `callbacks`); the `async` block body discards it so this closure's own + // return type is genuinely `Promise`, matching `QueryExecutionDeps`. + runProgress: async (request, callbacks) => { await authenticatedProgress(chCtx, request, callbacks); }, + runText: (request) => authenticatedText(chCtx, request), + cancel: cancelOwnedQuery, + now, uid, retryMs, sleep, }); // #457 removed `app.runOptionQuery` (#447 phase 2's per-variable option-query // transport): it existed only for the variable DRAWER's Test action. A variable @@ -626,8 +664,13 @@ export function createApp(env: CreateAppEnv = {}): App { pickDirectory: (input) => app.showDirectoryPicker!(input) as Promise, }; const exportService = createExportService({ - exportQuery: ch.exportQuery, runQuery: ch.runQuery, killQuery: ch.killQuery, - ctx: () => chCtx, ensureConfig, getToken, sqlString, now, wallNow, uid, + // #630 Phase 7 — the same authenticated-request seam `exec` above wires, + // plus the shared owner-scoped cancel callback; `ctx` survives only for + // its `.onSignedOut()` call (no transport reads it any more). + exportResponse: (request) => authenticatedResponse(chCtx, request), + runEffectText: (request) => authenticatedText(chCtx, request), + cancel: cancelOwnedQuery, + ctx: () => chCtx, ensureConfig, getToken, now, wallNow, uid, executionScope: () => app.executionScope(), canExport: () => app.canExport(), canExportScript: () => app.canExportScript(), sink: exportSink, @@ -2054,7 +2097,7 @@ export function createApp(env: CreateAppEnv = {}): App { activeExecutionScope?.close(); const scope = createAuthenticatedExecutionScope({ epoch, - cancelRemote: (lease, queryId) => ch.killQueryWithLease(lease, queryId, sqlString), + cancelRemote: (lease, queryId) => ch.killQueryWithLease(lease, queryId), }); activeExecutionScope = scope; // Connection-scoped caches/panes are owners even when they have no live diff --git a/src/ui/workbench/workbench-session.ts b/src/ui/workbench/workbench-session.ts index 9b23af5f..988226d4 100644 --- a/src/ui/workbench/workbench-session.ts +++ b/src/ui/workbench/workbench-session.ts @@ -208,6 +208,12 @@ interface ActiveRun { cancelled: boolean; registration: AuthenticatedExecutionRegistration | null; registrationReleased: boolean; + /** #630 Phase 7 §9.3/9.4 — the operation's owner epoch (the authenticated + * execution scope's `.epoch`), captured once at `registerWave` time — + * never re-read at cancel time. `deps.exec.kill` fences a cancel against + * this exact epoch, permitting a same-epoch refreshed credential but + * rejecting a replacement (non-owner) one. */ + ownerEpoch: number | null; } export interface WorkbenchSession { @@ -257,6 +263,7 @@ export function createWorkbenchSession(deps: WorkbenchSessionDeps): WorkbenchSes cancelled: false, registration: null, registrationReleased: false, + ownerEpoch: null, }; activeRun = operation; try { @@ -302,6 +309,8 @@ export function createWorkbenchSession(deps: WorkbenchSessionDeps): WorkbenchSes operation: ActiveRun, ): void { const scope = deps.executionScope(); + // #630 Phase 7 §9.3 — captured once, at registration/start time. + operation.ownerEpoch = scope?.epoch ?? null; operation.registration = scope?.register({ name, abort: () => { @@ -444,7 +453,8 @@ export function createWorkbenchSession(deps: WorkbenchSessionDeps): WorkbenchSes // Cap a normal result query (Table or explicit-FORMAT SELECT) at the global // row limit; EXPLAIN/PIPELINE/ESTIMATE are exempt (small output, and a cap // would truncate a plan oddly). The streaming guard reads it off the result; - // runQuery adds the server-side max_result_rows for the Table path. + // the query-execution-service adds the server-side max_result_rows for + // every positive rowLimit, regardless of format (#630 Phase 7 §2.5). const rowLimit = explainMode ? 0 : panelIsKpi ? kpiExecution.rowLimit! : state.resultRowLimit; const t0 = deps.now(); const result: QueryResult = newResult(fmt, rowLimit); @@ -806,7 +816,7 @@ export function createWorkbenchSession(deps: WorkbenchSessionDeps): WorkbenchSes if (!operation) return; operation.cancelled = true; operation.controller.abort(); - deps.exec.kill(operation.queryId); // fire-and-forget, same as before + deps.exec.kill(operation.ownerEpoch, operation.queryId); // fire-and-forget, same as before } function attachShell(shellEffects: WorkbenchShellEffects): void { @@ -842,7 +852,7 @@ export function createWorkbenchSession(deps: WorkbenchSessionDeps): WorkbenchSes if (!operation) return; operation.cancelled = true; operation.controller.abort(); - if (operation.queryId != null) deps.exec.kill(operation.queryId); + if (operation.queryId != null) deps.exec.kill(operation.ownerEpoch, operation.queryId); retireWave(operation); } diff --git a/tests/e2e/clickhouse-http-transport.html b/tests/e2e/clickhouse-http-transport.html index c8490e7b..fc2da6c8 100644 --- a/tests/e2e/clickhouse-http-transport.html +++ b/tests/e2e/clickhouse-http-transport.html @@ -38,17 +38,7 @@ NEW Phase-4 API preserves the identical native post-header cancellation semantics (one real Fetch, the caller's own AbortSignal driving cancellation for the response's whole lifetime, no callbacks after - rejection). Scenarios 1-8 are otherwise UNCHANGED: they still exercise - the raw `createHttpTransport().send()` -> package `streamLines` - composition directly, which was the ordinary SQL Browser production - path through #630 Phase 5. Since Phase 6, the actual production path - for `queryJson`/`runQuery`/`exportQuery` is the authenticated-path - composition below (`authenticatedRequest()`/`authenticatedProgress()` - -> package `request()`/response consumers) — `createHttpTransport` - itself now remains live only as the frozen-lease `killQueryWithLease` - bypass's compatibility route. Scenarios 1-8 stay as lower-layer - package/transport regression coverage; they are not claimed to - exercise the current ordinary production path. + rejection). #630 Phase 6 — authenticated-path variants of the post-header cancellation scenarios (5-9), proving the SAME native @@ -66,14 +56,30 @@ original `AbortController.signal` straight through, exactly like the raw scenarios above. Scenarios 1-4 stay raw/unauthenticated (pre-header timing — optional to duplicate through auth per the plan); - only the post-header family (5-9) gets an authenticated variant. --> + only the post-header family (5-9) gets an authenticated variant. + + #630 Phase 7 — the local SQL Browser compatibility transport adapter + (`src/net/clickhouse-http-transport.ts`, `createHttpTransport`) is + retired: `killQueryWithLease` (its last production caller) moved to + the package's own stateless `client.killQuery(...)`, so this harness + no longer imports that file at all. Scenarios 1-4 and the raw + post-header family (5-8) plus the invalid-UTF-8 scenario, which used + to build a `createHttpTransport(...)` and call its `.send()`, now + build the package's OWN `createClickHouseHttpClient(...)` directly + (via `makeClient`, the same helper Scenario 9 already used) and call + its public `.request()` method — the exact production path + `authenticated-clickhouse-request.ts`'s `authenticatedRequest()` + itself calls one layer up. This is a pure retarget: every original + behavioral assertion (identity, call count, exact SQL/Authorization, + cancellation semantics, byte fidelity) is unchanged — only the + compatibility indirection is gone. Scenario 9 remains + `queryProgress()` coverage; it does not become export coverage. --> + + + diff --git a/tests/e2e/export-post-header-cancel.spec.js b/tests/e2e/export-post-header-cancel.spec.js new file mode 100644 index 00000000..0cb32580 --- /dev/null +++ b/tests/e2e/export-post-header-cancel.spec.js @@ -0,0 +1,113 @@ +import { test, expect } from '@playwright/test'; +import { startFaultServer } from '../spike/clickhouse-client/fault-server.mjs'; + +// #630 Phase 7 (pre-PR review Finding 1) — Plan §18/Checkpoint 3 and A15's +// Definition of Done require a dedicated EXPORT-shaped real-browser fixture +// proving native post-header cancellation semantics survive through the +// ACTUAL export path (`ExportService.streamToFile()`/`exportDirect`/ +// `authenticatedResponse`), not just query/progress +// (`clickhouse-http-transport.{html,spec.js}`'s Scenarios 5-9, which remain +// query-progress-only per that spec's own file-level comment). This spec owns +// the fault server's Node-side lifecycle for this fixture, exactly like +// `clickhouse-http-transport.spec.js` does for its own scenarios — the root +// Playwright config only starts the static harness host (`build/e2e-serve.mjs` +// on :5599); it knows nothing about this ephemeral server. Firefox cannot +// launch locally (repo-wide constraint, `playwright.config.js`'s own +// comment); Chromium and WebKit are this fixture's real acceptance signal, +// matching every other native-cancellation e2e spec in this repo. + +test.describe('#630 Phase 7 — export post-header cancellation (real ExportService, real fetch, real AbortController)', () => { + test.skip( + ({ browserName }) => browserName === 'firefox', + 'native post-header cancellation acceptance is explicitly Chromium/WebKit, matching clickhouse-http-transport.spec.js', + ); + + /** @type {Awaited>} */ + let fault; + + test.beforeAll(async () => { + fault = await startFaultServer({ cors: true }); + }); + + test.afterAll(async () => { + await fault?.close(); + }); + + test.beforeEach(async ({ page }) => { + await page.goto('/tests/e2e/export-post-header-cancel.html'); + await page.waitForFunction(() => window.__ready === true); + }); + + test('settles headers, commits bytes past the 32 KiB hold-back, then a mid-read cancel stops the export with full cleanup and a correct owner-scoped remote KILL', async ({ page }) => { + test.setTimeout(30_000); + const result = await page.evaluate( + ({ baseUrl }) => window.__exportPostHeaderCancel(baseUrl), + { baseUrl: fault.baseUrl }, + ); + + // Headers/first-chunk fidelity — exactly one direct-export request, 2xx. + expect(result.directRequestCount).toBe(1); + expect(result.directRequestOk).toBe(true); + + // File bytes were committed (at least one real write + progress event) + // BEFORE the held tail was ever released — proves this is genuine + // post-header, past-hold-back streaming, not a headers-only proof. The + // exact number of write/progress pairs the initial ~40 KiB burst + // produces is engine-dependent (Chromium delivers it as a single native + // read; WebKit has been observed splitting it into a few smaller reads, + // each individually crossing the 32 KiB hold-back on its own) — the + // invariant this asserts is "comfortably past the hold-back, at least + // once", not an exact read count. + expect(result.progressCountBeforeCancel).toBeGreaterThanOrEqual(1); + expect(result.writesBeforeCancelCount).toBe(result.progressCountBeforeCancel); + // The fixture's first chunk is ~40 KiB, comfortably past ExportService's + // 32 KiB hold-back — but the amount actually COMMITTED to the file is + // (bytes received so far) minus the 32 KiB still retained in the + // hold-back buffer, so this is a small positive number (a few KiB), not + // itself > 32 KiB. ">0" is the real invariant: a real write happened at + // all, proving the hold-back threshold was genuinely crossed rather than + // this being a headers-only proof. + expect(result.bytesBeforeCancel).toBeGreaterThan(0); + expect(result.totalWrittenBytes).toBe(result.bytesBeforeCancel); + + // Cancel occurred during the pending second reader.read(); that read + // aborted, and NO later write/progress occurred (the fixture's held + // second chunk, sent ~3s later to an already-torn-down connection, never + // reached the file). + expect(result.progressCountFinal).toBe(result.progressCountBeforeCancel); + expect(result.writesFinalCount).toBe(result.writesBeforeCancelCount); + + // Writer cleanup + .partial semantics — no successful final file for + // incomplete data. + expect(result.writerClosed).toBe(true); + expect(result.writerAborted).toBe(false); + expect(result.movedToPartial).toBe('export.tsv.partial'); + + // Owner-scoped remote cancellation: the exact epoch/query id this export + // registered with reached the cancel callback, and a REAL KILL QUERY + // request (through the package's own stateless `client.killQuery(...)`, + // the same mechanism `killQueryWithLease` calls, #630 Phase 7 §10) landed + // on the server naming that exact query id. + expect(result.cancelCallCount).toBe(1); + expect(result.cancelOwnerEpoch).toBe(4242); + expect(result.cancelQueryId).toBe(result.directQueryId); + expect(result.cancelQueryId).toMatch(/^export-post-header-abort-hold__/); + expect(result.killRequestCount).toBe(1); + expect(result.killRequestSqlContainsKillQuery).toBe(true); + expect(result.killRequestSqlContainsQueryId).toBe(true); + + // No offline/sign-out classification; no refresh attempt — cancellation + // must never be misclassified as a connectivity/auth failure. + expect(result.onTransportOfflineCalls).toBe(0); + expect(result.onSignedOutCalls).toBe(0); + expect(result.refreshCalls).toBe(0); + + // No dependency on a successful response's .text() anywhere in the raw + // export byte-stream path. + expect(result.textCalledOnSuccessfulResponse).toBe(false); + + // exportDirect swallows the AbortError internally (no user-facing + // "Export failed" toast for an explicit cancel). + expect(result.toastMessages).toEqual([]); + }); +}); diff --git a/tests/spike/clickhouse-client/candidate-entry.ts b/tests/spike/clickhouse-client/candidate-entry.ts index 29dd38eb..47865a03 100644 --- a/tests/spike/clickhouse-client/candidate-entry.ts +++ b/tests/spike/clickhouse-client/candidate-entry.ts @@ -46,7 +46,7 @@ import { officialAuthFor, runOfficial, runOfficialRefreshThenRetry, - makeOfficialRunQueryShim, + makeOfficialQueryExecutionAdapter, } from './official-adapter.js'; declare global { @@ -65,5 +65,5 @@ globalThis.__ASB_SPIKE_CANDIDATE_CLIENT_WEB__ = { officialAuthFor, runOfficial, runOfficialRefreshThenRetry, - makeOfficialRunQueryShim, + makeOfficialQueryExecutionAdapter, }; diff --git a/tests/spike/clickhouse-client/current-adapter.ts b/tests/spike/clickhouse-client/current-adapter.ts index a472b684..709e3865 100644 --- a/tests/spike/clickhouse-client/current-adapter.ts +++ b/tests/spike/clickhouse-client/current-adapter.ts @@ -1,20 +1,36 @@ // Phase 0 / issue #585 — the "current-side adapter" (plan §7): a thin // SPIKE-OWNED wrapper around the REAL production functions from -// `src/net/ch-client.ts`. It does not reimplement request construction, -// streaming, or error classification — it only translates the test-owned -// `SpikeRequest`/`SpikeOutcome` vocabulary at the boundary, exactly as the -// plan requires ("Do not reimplement current behavior in a test helper and -// compare that replica with the official client"). - -// Issue #630 Phase 3 — `parseExceptionText` is package-owned now -// (`@altinity/clickhouse-http`); obtained here through `ch-client.ts`'s own -// zero-logic re-export (the same gateway `chUrl` already came through since -// Phase 2), in the same import declaration. `applyStreamLine`/`newResult` -// stay SQL-Browser-owned result policy, imported from `src/core/stream.js` -// unchanged. +// `src/net/authenticated-clickhouse-request.ts` (and `ch-client.ts`'s own +// zero-logic re-exports of package protocol helpers). It does not reimplement +// request construction, streaming, or error classification — it only +// translates the test-owned `SpikeRequest`/`SpikeOutcome` vocabulary at the +// boundary, exactly as the plan requires ("Do not reimplement current +// behavior in a test helper and compare that replica with the official +// client"). +// +// Issue #630 Phase 7 (plan §19/§2.4, Checkpoint 2C's spike portion) — this +// file no longer depends on `ch-client.ts`'s generic, now-retiring +// `runQuery`/`exportQuery`/mutable-context `killQuery` or its `ChCtx` type: +// it drives the SAME production request path those functions themselves now +// delegate to — `authenticated-clickhouse-request.ts`'s `authenticatedProgress` +// (Table/KPI streaming), `authenticatedText` (TSV/explicit-format whole-body +// reads), and `authenticatedResponse` (the raw/export path) — plus the +// package's own stateless `createClickHouseHttpClient(...).killQuery(...)` +// for best-effort cancellation. The Table/KPI/TSV/explicit-format mapping and +// the non-2xx/in-band-exception classification below are this file's OWN +// mirror of that mapping (the same one `QueryExecutionService` +// (`src/application/query-execution-service.ts`) now owns for production — +// see its module doc), not a reimplementation of the transport itself. +// `applyStreamLine`/`newResult` stay SQL-Browser-owned result policy, +// imported from `src/core/stream.js` unchanged. import { - runQuery, exportQuery, killQuery, chUrl, parseExceptionText, type ChCtx, + chUrl, parseExceptionText, } from '../../../src/net/ch-client.js'; +import { + authenticatedResponse, authenticatedProgress, authenticatedText, +} from '../../../src/net/authenticated-clickhouse-request.js'; +import type { AuthenticatedRequestCtx } from '../../../src/net/authenticated-clickhouse-request.js'; +import { createClickHouseHttpClient } from '@altinity/clickhouse-http'; import { applyStreamLine, newResult } from '../../../src/core/stream.js'; import type { AdapterRunResult, SpikeCredential, SpikeRequest, SpikeOutcome } from './types.js'; import { emptyOutcome, IncrementalSha256 } from './normalize.js'; @@ -22,9 +38,9 @@ import { emptyOutcome, IncrementalSha256 } from './normalize.js'; /** Build the `Authorization` header for a `SpikeCredential` — the harness's * own request-local credential concept, translated into exactly the header * production's authenticated request path would send for that credential - * kind (at the time this spike was written, `ch-client.ts`'s `authedFetch`; - * since #630 Phase 6, `authenticated-clickhouse-request.ts`'s - * `authenticatedRequest`, unchanged in shape). */ + * kind (`authenticated-clickhouse-request.ts`'s `authenticatedRequest`, + * since #630 Phase 6; formerly `ch-client.ts`'s `authedFetch`, unchanged in + * shape). */ export function credentialAuthHeader(credential: SpikeCredential): string { // `btoa` (standard Web API, global in Node >=18 and every target browser) // rather than `Buffer` — see normalize.ts's `IncrementalSha256` docstring @@ -38,7 +54,8 @@ export function credentialAuthHeader(credential: SpikeCredential): string { return 'Bearer ' + credential.token; case 'jwt-as-basic': // Matches the app's real JWT-as-Basic-password composition (username + - // the JWT used as the Basic password) — see ch-client.ts's authHeader seam. + // the JWT used as the Basic password) — see authenticated-clickhouse- + // request.ts's `authHeader` seam. return 'Basic ' + btoa(`${credential.username}:${credential.jwt}`); case 'invalid': default: @@ -46,14 +63,12 @@ export function credentialAuthHeader(credential: SpikeCredential): string { } } -/** Optional hooks `makeCurrentCtx` wires onto `ChCtx`'s own epoch/lifecycle - * seam (plan §21's "stale before request" / "stale during refresh" / - * "stale response" cases need REAL `ch-client.ts` epoch fencing exercised - * through its real production request path — at the time this spike was - * written, `authedFetch`; since #630 Phase 6, `authenticated-clickhouse- - * request.ts`'s `authenticatedRequest`, reached the same way, through - * `runQuery`/`exportQuery`/`killQuery` — not a harness reimplementation of - * it). Every field +/** Optional hooks `makeCurrentCtx` wires onto the real production + * `AuthenticatedRequestCtx`'s own epoch/lifecycle seam (plan §21's "stale + * before request" / "stale during refresh" / "stale response" cases need + * REAL `authenticated-clickhouse-request.ts` epoch fencing exercised through + * its real production request path, `authenticatedRequest` — not a harness + * reimplementation of it). Every field * is optional and defaults to the pre-existing no-op behavior, so no * existing call site needs to change. */ export interface CurrentCtxHooks { @@ -72,26 +87,24 @@ export interface CurrentCtxHooks { getToken?: () => Promise; /** Fires the instant a delegate fetch RESOLVES — before `runCurrent`'s own * `lastResponse` capture and before production's own post-fetch epoch - * check runs (at the time this spike was written, `authedFetch`'s; since - * #630 Phase 6, `authenticated-clickhouse-request.ts`'s - * `authenticatedRequest`'s, unchanged in shape) (plan §21 "stale - * response"). A test flips a shared epoch + * check runs (`authenticated-clickhouse-request.ts`'s `authenticatedRequest`) + * (plan §21 "stale response"). A test flips a shared epoch * variable here to deterministically land the flip in that exact window, * with no timing race. */ onFetchResponse?: (resp: Response) => void; } -/** Build a `ChCtx` bound to one `SpikeRequest`'s credential and origin, using - * the real production `fetch` seam contract. `onFetch` is called once per - * underlying fetch invocation (constructor/fetch-count invariants); - * `onResponse` observes each settled `Response` (status/headers) — pure - * instrumentation at the already-injected fetch boundary, not a second - * request path: production's `RunQueryResult` doesn't surface headers to - * `runQuery`'s caller, so this is how the harness reads them without - * reimplementing `runQuery`'s own request/parsing logic. `hooks` (optional) - * wires the real epoch/lifecycle seam (`CurrentCtxHooks`, above) — omitted - * entirely preserves the exact previous behavior (no epoch hook, `refresh()` - * always resolves false, `onSignedOut` a no-op). */ +/** Build an `AuthenticatedRequestCtx` bound to one `SpikeRequest`'s + * credential and origin, using the real production `fetch` seam contract. + * `onFetch` is called once per underlying fetch invocation + * (constructor/fetch-count invariants); `onResponse` observes each settled + * `Response` (status/headers) — pure instrumentation at the already-injected + * fetch boundary, not a second request path: production's authenticated + * response consumers don't surface headers to their caller, so this is how + * the harness reads them without reimplementing that request/parsing logic. + * `hooks` (optional) wires the real epoch/lifecycle seam (`CurrentCtxHooks`, + * above) — omitted entirely preserves the exact previous behavior (no epoch + * hook, `refresh()` always resolves false, `onSignedOut` a no-op). */ export function makeCurrentCtx( request: SpikeRequest, baseUrl: string, @@ -100,7 +113,7 @@ export function makeCurrentCtx( onResponse?: (resp: Response) => void, initialAuthConfirmed?: boolean, hooks?: CurrentCtxHooks, -): ChCtx { +): AuthenticatedRequestCtx { const authHeader = credentialAuthHeader(request.credential); return { origin: baseUrl, @@ -129,10 +142,10 @@ export function makeCurrentCtx( * Restricted, on purpose, to exactly the shapes this spike's fixtures use * (digit-string / number scalars and arrays of them — no escaping of * tab/newline/quote/backslash, which the real vendor formatter also handles - * but no spike fixture exercises) — `ch-client.ts`'s own `params` field has - * no array-value concept at all, so the CURRENT adapter must pre-format an - * array-valued native parameter into the exact wire string itself before - * handing it to `runQuery`'s plain `Record` params + * but no spike fixture exercises) — production's authenticated request path + * has no array-value concept at all, so the CURRENT adapter must pre-format + * an array-valued native parameter into the exact wire string itself before + * handing it to the request's plain `Record` params * bag; the OFFICIAL adapter instead hands the array straight to * `query_params` and lets the vendor library's own formatter do this. A * match between the two proves this hand-written mirror is correct — see @@ -145,15 +158,15 @@ export function formatNativeParamValue(value: string | number | (string | number } /** Fold a `SpikeRequest`'s settings/native-params/role/session into the flat - * `Record` bag `ch-client.ts`'s `runQuery`/ - * `exportQuery` accept — settings ride as bare keys (matching the official + * `Record` bag the production authenticated request + * path accepts — settings ride as bare keys (matching the official * adapter's `clickhouse_settings`); native params are prefixed `param_` * here (the CURRENT side's own responsibility — see `formatNativeParamValue`'s * docstring for why the official side instead delegates this to the vendor * library); `role`/`sessionId` become the same `role`/`session_id` bare keys * the official client's own `toSearchParams` emits (array-valued `role` is - * deliberately unsupported here — `ch-client.ts`'s params bag cannot repeat a - * key, so every spike scenario exercising `role` uses a single string). */ + * deliberately unsupported here — the params bag cannot repeat a key, so + * every spike scenario exercising `role` uses a single string). */ function nativeParamsForCurrent(request: SpikeRequest): Record { const out: Record = { ...(request.settings || {}) }; for (const [k, v] of Object.entries(request.params || {})) { @@ -164,10 +177,13 @@ function nativeParamsForCurrent(request: SpikeRequest): Record = { + ...(isStreaming ? {} : { wait_end_of_query: 1 }), + add_http_cors_header: 1, + }; + const params = { ...(request.queryId ? { query_id: request.queryId } : {}), ...nativeParamsForCurrent(request) }; try { - const out = await runQuery(ctx, request.sql, { - format: request.format, - queryId: request.queryId, - signal: request.signal, - params: nativeParamsForCurrent(request), - onLine: (line) => { - applyStreamLine(line, result); - if (line.row && !firstRow) { firstRow = true; outcome.firstRowAtMs = Date.now() - t0; } - if (line.exception) outcome.chMessage = line.exception; - }, - }); - outcome.completedAtMs = Date.now() - t0; - if (out.error != null) outcome.error = out.error; - if (out.raw != null) { - outcome.rawByteCount = new TextEncoder().encode(out.raw).byteLength; + if (isStreaming) { + await authenticatedProgress(ctx, { sql: request.sql, defaultFormat, settings, params, signal: request.signal }, { + onLine: (line) => { + applyStreamLine(line, result); + if (line.row && !firstRow) { firstRow = true; outcome.firstRowAtMs = Date.now() - t0; } + if (line.exception) outcome.chMessage = line.exception; + }, + }); + outcome.completedAtMs = Date.now() - t0; + } else { + const raw = await authenticatedText(ctx, { sql: request.sql, defaultFormat, settings, params, signal: request.signal }); + outcome.completedAtMs = Date.now() - t0; + outcome.rawByteCount = new TextEncoder().encode(raw).byteLength; } } catch (e) { if (e instanceof Error && e.name === 'AbortError') outcome.cancelled = true; @@ -255,10 +285,23 @@ export async function runCurrent( return { outcome, constructorCalls: 1, fetchCalls }; } -/** Best-effort server cancellation via the real `killQuery` (plan §22 - * "Server cancellation"). */ -export async function currentKillQuery(ctx: ChCtx, queryId: string | null | undefined): Promise { - return killQuery(ctx, queryId, (s) => `'${String(s).replace(/'/g, "\\'")}'`); +/** Best-effort server cancellation (plan §22 "Server cancellation") through + * the package's stateless `createClickHouseHttpClient(...).killQuery(...)` — + * #630 Phase 7 §19: no longer routes through `ch-client.ts`'s retiring + * mutable-context `killQuery`. Resolves the CURRENT Authorization from `ctx` + * itself (the same `getToken()`/`authHeader()` seam `makeCurrentCtx` wires + * up) and issues exactly one `KILL QUERY ... ASYNC`, swallowing every + * failure — matching the retired function's own best-effort contract. A + * missing token (never signed in) is a no-op, same as a missing `queryId`. */ +export async function currentKillQuery(ctx: AuthenticatedRequestCtx, queryId: string | null | undefined): Promise { + if (!queryId) return; + try { + const token = await ctx.getToken(); + if (!token) return; + const authHeader = ctx.authHeader || ((t: string) => 'Bearer ' + t); + const client = createClickHouseHttpClient({ fetch: () => ctx.fetch, origin: () => ctx.origin }); + await client.killQuery({ queryId, authorization: authHeader(token) }); + } catch { /* best-effort */ } } /** Re-exported so scenario/harness code has one place to build the diff --git a/tests/spike/clickhouse-client/fault-server.mjs b/tests/spike/clickhouse-client/fault-server.mjs index 0c710e31..9a6670ea 100644 --- a/tests/spike/clickhouse-client/fault-server.mjs +++ b/tests/spike/clickhouse-client/fault-server.mjs @@ -362,6 +362,26 @@ export function startFaultServer(opts = {}) { res.end(); return; } + case 'export-post-header-abort-hold': { + // #630 Phase 7 (pre-PR review Finding 1) — the EXPORT-shaped analogue + // of 'post-header-abort-hold' above: raw byte content (never NDJSON), + // and the FIRST chunk is deliberately larger than ExportService's own + // 32 KiB hold-back buffer (`streamToFile`'s `HOLDBACK` constant, + // `src/application/export-service.ts`), so a real file WRITE and + // PROGRESS event fire on the very first `reader.read()` — before any + // hold — proving the mid-read-abort assertions this fixture backs + // exercise actual already-committed bytes, not merely headers. The + // second (small) chunk is then held for POST_HEADER_ABORT_HOLD_MS, + // exactly like 'post-header-abort-hold', so a genuinely pending + // second `reader.read()` is guaranteed at the moment of cancellation. + res.writeHead(200, { 'content-type': 'text/tab-separated-values' }); + const FIRST_CHUNK_BYTES = 40 * 1024; // > HOLDBACK (32 KiB) + margin + res.write('col1\n' + 'x'.repeat(FIRST_CHUNK_BYTES) + '\n'); + await sleep(POST_HEADER_ABORT_HOLD_MS); + res.write('after-hold\n'); + res.end(); + return; + } case 'slow-headers': { // Headers themselves are delayed (plan §18 "cancel awaiting headers"; // §21 "timeout") — unlike 'delayed-headers-scheduled-rows', where diff --git a/tests/spike/clickhouse-client/live-sessions.test.ts b/tests/spike/clickhouse-client/live-sessions.test.ts index 752ad7fa..2ca0105b 100644 --- a/tests/spike/clickhouse-client/live-sessions.test.ts +++ b/tests/spike/clickhouse-client/live-sessions.test.ts @@ -6,13 +6,12 @@ // header for why this env-gate is mandatory, not optional. import { describe, it, expect } from 'vitest'; -import { ClickHouseError } from '@clickhouse/client-web'; import { runCurrent } from './current-adapter.js'; import { createOfficialConnection, runOfficial, officialAuthFor, type OfficialConnection } from './official-adapter.js'; import { bridgeNdjsonProgress } from './progress-bridge.js'; import { createQueryExecutionService } from '../../../src/application/query-execution-service.js'; import { BASIC_USER_A } from './auth-fixtures.js'; -import type { ChCtx, RunQueryOptions, RunQueryResult } from '../../../src/net/ch-client.js'; +import type { QueryExecutionRequest } from '../../../src/application/query-execution-service.js'; import type { SpikeCredential, SpikeRequest } from './types.js'; // See live-precision.test.ts's header comment for why this reads `process` @@ -36,39 +35,44 @@ function baseReq(overrides: Partial = {}): SpikeRequest { /** * A SESSION-AWARE variant of `official-adapter.ts`'s own - * `makeOfficialRunQueryShim` — that exported shim has no `session_id` - * parameter at all (every deterministic scenario that needs one drives - * `runOfficial` directly instead — see its own docstring), so a session- - * carrying shim for the LIVE `SESSION_IS_LOCKED` proof below is written - * locally rather than expanding `official-adapter.ts`'s public surface - * outside this sub-task's declared file scope. Mirrors that function's own - * throw/return contract EXACTLY (a ClickHouseError response classifies as - * `{ error }`; any other rejection propagates as a throw, matching real - * `runQuery`'s contract) so `QueryExecutionService`'s real, unmodified - * `attemptStatement`/`SESSION_BUSY` retry logic runs unmodified against it — + * `makeOfficialQueryExecutionAdapter` — that adapter's `runText` has no + * `session_id` parameter at all (every deterministic scenario that needs one + * drives `runOfficial` directly instead — see its own docstring), so a + * session-carrying `QueryExecutionDeps['runText']` for the LIVE + * `SESSION_IS_LOCKED` proof below is written locally rather than expanding + * `official-adapter.ts`'s public surface outside this sub-task's declared + * file scope (#630 Phase 7, plan §19). Uses `exec()` + + * `FORMAT JSONStringsEachRowWithProgress` (the same Table-shaped bridge + * `official-adapter.ts`'s `runProgress` uses) rather than `command()` (unlike + * `runOfficialCommand` below) — this test's ONLY statement routed through it + * is `SELECT 1` (row-returning). Matching the new "package consumers throw" + * contract (#630 Phase 7 §6.5): a pre-header rejection (a `ClickHouseError` + * thrown by `exec()` itself), a mid-stream network failure, and an in-band + * `{"exception"}` line ALL propagate as a throw now, never a returned + * `{error}` — so `QueryExecutionService`'s real, unmodified + * `attemptStatement`/`SESSION_BUSY` retry logic runs unmodified against it, * never a reimplementation of that policy, only of the session_id plumbing - * `makeOfficialRunQueryShim` doesn't carry. + * `makeOfficialQueryExecutionAdapter` doesn't carry. */ -function makeSessionAwareRunQueryShim(conn: OfficialConnection, credential: SpikeCredential, sessionId: string) { - return async function sessionAwareShim(_ctx: ChCtx, sql: string, o: RunQueryOptions = {}): Promise { +function makeSessionAwareRunText(conn: OfficialConnection, credential: SpikeCredential, sessionId: string): (request: QueryExecutionRequest) => Promise { + return async function sessionAwareRunText(request: QueryExecutionRequest): Promise { + const { query_id: queryId, ...nativeParams } = request.params || {}; const auth = officialAuthFor(credential); - const fullSql = `${sql}\nFORMAT JSONStringsEachRowWithProgress`; - let res; - try { - res = await conn.client.exec({ - query: fullSql, query_id: o.queryId, session_id: sessionId, abort_signal: o.signal, auth, query_params: o.params, - }); - } catch (e) { - if (e instanceof ClickHouseError) return { error: e.message }; - throw e; // network-level — propagate for attemptStatement's own classification - } + const fullSql = `${request.sql}\nFORMAT JSONStringsEachRowWithProgress`; + const res = await conn.client.exec({ + query: fullSql, + query_id: queryId != null ? String(queryId) : undefined, + session_id: sessionId, + abort_signal: request.signal, + auth, + query_params: nativeParams, + }); let sawException: string | null = null; await bridgeNdjsonProgress(res.stream, (line) => { if (line.exception) sawException = line.exception; - o.onLine?.(line); }); - if (sawException) return { error: sawException }; - return { streamed: true }; + if (sawException) throw new Error(sawException); + return ''; }; } @@ -177,14 +181,14 @@ describe.skipIf(!CH_URL)('live sessions, temporary tables, and SESSION_IS_LOCKED const attempts: number[] = []; const svc = createQueryExecutionService({ - runQuery: makeSessionAwareRunQueryShim(conn, BASIC_USER_A, sessionId) as unknown as typeof import('../../../src/net/ch-client.js').runQuery, - killQuery: async () => {}, - ctx: () => ({} as ChCtx), + // Never exercised — this test only calls `executeScript`. + runProgress: async () => { throw new Error('runProgress not exercised by this spike helper'); }, + runText: makeSessionAwareRunText(conn, BASIC_USER_A, sessionId), + cancel: async () => {}, now: () => Date.now(), uid: (prefix: string) => `${prefix}-${Date.now()}-${Math.random().toString(36).slice(2)}`, retryMs: 3000, // >= the holder's own ~2s runtime, so the one retry lands after it releases the lock sleep: (ms) => new Promise((r) => setTimeout(r, ms)), - sqlString: (s) => `'${String(s)}'`, }); const result = await svc.executeScript({ diff --git a/tests/spike/clickhouse-client/official-adapter.ts b/tests/spike/clickhouse-client/official-adapter.ts index c5957a94..213f2bf8 100644 --- a/tests/spike/clickhouse-client/official-adapter.ts +++ b/tests/spike/clickhouse-client/official-adapter.ts @@ -369,62 +369,75 @@ export async function runOfficialRefreshThenRetry( } } -// ── QueryExecutionService shim ────────────────────────────────────────────── -// Plan §23 "Overlap two requests in one session and feed official-spike -// outcomes through existing QueryExecutionService" / invariant map's "Retry -// safety remains unchanged — official outcomes feed existing execution -// policy". This shim satisfies `typeof runQuery` from `src/net/ch-client.ts` -// exactly (same signature, same `RunQueryResult` shape) so the REAL, -// unmodified `createQueryExecutionService` (src/application/ -// query-execution-service.ts) can run its real retry/classification logic -// against the official client — never a reimplementation of that policy. - -import type { ChCtx, RunQueryOptions, RunQueryResult } from '../../../src/net/ch-client.js'; +// ── QueryExecutionService adapter (post-#630 Phase 7) ─────────────────────── +// Plan §19/§2.4 (Checkpoint 2C's spike portion) — replaces the retired +// `makeOfficialRunQueryShim`, which satisfied `typeof runQuery` from +// `src/net/ch-client.ts` (`RunQueryOptions`/`RunQueryResult` — both retiring, +// #630 Phase 7). This adapter instead satisfies `QueryExecutionService`'s OWN +// narrow `QueryExecutionDeps['runProgress' | 'runText']` shape +// (`src/application/query-execution-service.ts`) directly — never a +// reimplementation of that service's retry/classification policy, which +// still runs, real and unmodified, against whichever client (current or +// official) is injected (plan §23 "official outcomes feed existing execution +// policy"). +// +// `runProgress` mirrors the retired shim's 'Table'/'KPI' branches +// (exec()+bridgeNdjsonProgress / query()+stream reading respectively), +// dispatching on `request.defaultFormat` (QES's own wire-format names) +// instead of a SQL-Browser format string. An in-band `{"exception"}` line is +// delivered through `callbacks.onLine`, exactly like the real authenticated +// progress path (`core/stream.ts`'s `applyStreamLine` turns it into +// `result.error`) — never a thrown/returned `{error}` shape: only a +// pre-header rejection (a `ClickHouseError` thrown by `exec()`/`query()` +// itself) or a mid-stream network failure throws, matching the new "package +// consumers throw" contract `QueryExecutionDeps.runProgress`'s own doc +// requires (#630 Phase 7 §6.5). +// +// `runText` mirrors the retired shim's ELSE/raw branch exactly: EVERY +// `executeScript` statement this spike suite ever drives through it (row- +// returning or effect alike) used that branch — `serviceFor()`'s +// `QueryExecutionRequest.defaultFormat` is always 'JSONCompact' or +// 'TabSeparatedWithNamesAndTypes', neither of which is 'Table'/'KPI' — so +// `runText` keeps using `command()` verbatim on `request.sql` UNCHANGED (no +// FORMAT clause appended: installed 1.23.1 hard SYNTAX_ERRORs on `SET .../ +// INSERT ... VALUES (...)` with one appended — see `live-sessions.test.ts`'s +// own `runOfficialCommand` docstring for the same finding), per plan §7 "use +// command() only when discarding output is intentional", always resolving +// `''`. No spike test routed through `runText` has ever needed the real row/ +// effect body text back (only attempt-count/status/message classification) — +// this is a mechanical reshape of the retired shim's existing behavior, not a +// redesign of the vendor side (plan §19 "do not redesign the vendor side +// beyond compilation and existing test intent"). +import type { QueryExecutionRequest, QueryProgressCallbacks } from '../../../src/application/query-execution-service.js'; -/** Faithfully mirrors `runQuery`'s own throw/return contract (plan's "Retry - * safety remains unchanged" invariant needs this EXACTLY, not an - * approximation): a ClickHouse-level query error (non-2xx with a parseable - * exception, or an in-band `{"exception"}` line) RETURNS `{ error }`; a - * network-level failure (rejected fetch, mid-stream reset) THROWS, exactly - * like production's `runQuery` does when its authenticated request (since - * #630 Phase 6, `authenticated-clickhouse-request.ts`'s - * `authenticatedRequest`; formerly `authedFetch`)/the streaming read - * loop rejects — so `QueryExecutionService`'s real `attemptStatement` - * (`e instanceof TypeError` -> `transient`) classifies it identically - * regardless of which client produced the exception. */ -export function makeOfficialRunQueryShim(conn: OfficialConnection, credentialFor: (ctx: ChCtx) => SpikeCredential) { - return async function officialRunQueryShim(ctx: ChCtx, sql: string, o: RunQueryOptions = {}): Promise { - const fmt = o.format || 'Table'; - const auth = officialAuthFor(credentialFor(ctx)); - const common = { query_id: o.queryId, abort_signal: o.signal, auth, query_params: o.params }; +export interface OfficialQueryExecutionAdapter { + runProgress(request: QueryExecutionRequest, callbacks: QueryProgressCallbacks): Promise; + runText(request: QueryExecutionRequest): Promise; +} - if (fmt === 'Table') { - const fullSql = `${sql}\nFORMAT JSONStringsEachRowWithProgress`; - let res; - try { - res = await conn.client.exec({ query: fullSql, ...common }); - } catch (e) { - if (e instanceof ClickHouseError) return { error: e.message }; - throw e; // network-level — propagate for attemptStatement's own classification - } - let sawException: string | null = null; - await bridgeNdjsonProgress(res.stream, (line) => { - if (line.exception) sawException = line.exception; - o.onLine?.(line); - }); - if (sawException) return { error: sawException }; - return { streamed: true }; - } +/** Build a `QueryExecutionDeps`-shaped `{runProgress, runText}` pair bound to + * one official-client connection and credential — the direct replacement for + * the retired `makeOfficialRunQueryShim`. `credentialFor` takes no `ChCtx` + * argument (that type is retiring too): every existing call site already + * ignored it (`() => BASIC_USER_A`), so dropping it is a mechanical signature + * narrowing, not a behavior change. */ +export function makeOfficialQueryExecutionAdapter( + conn: OfficialConnection, + credentialFor: () => SpikeCredential, +): OfficialQueryExecutionAdapter { + async function runProgress(request: QueryExecutionRequest, callbacks: QueryProgressCallbacks): Promise { + const { query_id: queryId, ...nativeParams } = request.params || {}; + const auth = officialAuthFor(credentialFor()); + const common = { + query_id: queryId != null ? String(queryId) : undefined, + abort_signal: request.signal, + auth, + clickhouse_settings: request.settings, + query_params: nativeParams, + }; - if (fmt === 'KPI') { - let rs; - try { - rs = await conn.client.query({ query: sql, format: 'JSONEachRowWithProgress', ...common }); - } catch (e) { - if (e instanceof ClickHouseError) return { error: e.message }; - throw e; - } - let sawException: string | null = null; + if (request.defaultFormat === 'JSONEachRowWithProgress') { + const rs = await conn.client.query({ query: request.sql, format: 'JSONEachRowWithProgress', ...common }); const stream = rs.stream>(); const reader = stream.getReader(); for (;;) { @@ -433,29 +446,41 @@ export function makeOfficialRunQueryShim(conn: OfficialConnection, credentialFor for (const wrapped of value) { const row = wrapped.json() as unknown; if (row && typeof row === 'object' && 'exception' in (row as object)) { - sawException = String((row as { exception: unknown }).exception); + callbacks.onLine?.({ exception: String((row as { exception: unknown }).exception) }); } else if (isRow>(row)) { - o.onLine?.({ row: row.row }); + callbacks.onLine?.({ row: row.row }); } else if (isProgressRow(row)) { - o.onLine?.({ progress: { read_rows: row.progress.read_rows, read_bytes: row.progress.read_bytes, total_rows_to_read: row.progress.total_rows_to_read, elapsed_ns: row.progress.elapsed_ns } }); + callbacks.onLine?.({ progress: { read_rows: row.progress.read_rows, read_bytes: row.progress.read_bytes, total_rows_to_read: row.progress.total_rows_to_read, elapsed_ns: row.progress.elapsed_ns } }); } + callbacks.onChunk?.(); } } - if (sawException) return { error: sawException }; - return { streamed: true }; + return; } - // Raw/explicit-format, no-output-of-interest (INSERT/DDL/command) path — - // `command()` per plan §7 "use command() only when discarding output is - // intentional". - try { - await conn.client.command({ query: sql, ...common }); - return { raw: '' }; - } catch (e) { - if (e instanceof ClickHouseError) return { error: e.message }; - throw e; - } - }; + // Table streaming (QES's `defaultFormat: 'JSONStringsEachRowWithProgress'`). + const fullSql = `${request.sql}\nFORMAT ${request.defaultFormat}`; + const res = await conn.client.exec({ query: fullSql, ...common }); + await bridgeNdjsonProgress(res.stream, (line) => { + callbacks.onLine?.(line); + callbacks.onChunk?.(); + }); + } + + async function runText(request: QueryExecutionRequest): Promise { + const { query_id: queryId, ...nativeParams } = request.params || {}; + await conn.client.command({ + query: request.sql, + query_id: queryId != null ? String(queryId) : undefined, + abort_signal: request.signal, + auth: officialAuthFor(credentialFor()), + clickhouse_settings: request.settings, + query_params: nativeParams, + }); + return ''; + } + + return { runProgress, runText }; } function flattenHeaders(h: Record | undefined): Record { diff --git a/tests/spike/clickhouse-client/parity.test.ts b/tests/spike/clickhouse-client/parity.test.ts index 66cf76e2..37e0851f 100644 --- a/tests/spike/clickhouse-client/parity.test.ts +++ b/tests/spike/clickhouse-client/parity.test.ts @@ -12,12 +12,12 @@ import { afterAll, afterEach, beforeAll, describe, expect, it } from 'vitest'; import { startFaultServer, closedLoopbackUrl } from './fault-server.mjs'; import { runCurrent } from './current-adapter.js'; -import { createOfficialConnection, runOfficial, makeOfficialRunQueryShim, runOfficialRefreshThenRetry, officialAuthFor } from './official-adapter.js'; +import { createOfficialConnection, runOfficial, makeOfficialQueryExecutionAdapter, runOfficialRefreshThenRetry, officialAuthFor } from './official-adapter.js'; import { createEpochFence } from './guarded-fetch.js'; import { BASIC_USER_A, BASIC_USER_B, DENIED_USER, BEARER_FIXTURE, JWT_AS_BASIC_FIXTURE } from './auth-fixtures.js'; import { createQueryExecutionService } from '../../../src/application/query-execution-service.js'; import { killQueryWithLease } from '../../../src/net/ch-client.js'; -import type { ChCtx, AuthenticatedCancellationLease } from '../../../src/net/ch-client.js'; +import type { AuthenticatedCancellationLease } from '../../../src/net/ch-client.js'; import type { ScriptEntry } from '../../../src/core/script-result.js'; import type { SpikeCredential, SpikeRequest } from './types.js'; @@ -89,19 +89,27 @@ function capturingFetch(realFetch: typeof fetch): { fetch: typeof fetch; lastAut * always mints a fresh id under `fixturePrefix` — `executeScript` always * calls `deps.uid('q')` internally (a fixed literal, ignoring the actual * fixture the test wants), so routing a full `executeScript` run to a - * SPECIFIC fault-server fixture requires this override. */ + * SPECIFIC fault-server fixture requires this override. + * + * #630 Phase 7 (plan §19, Checkpoint 2C's spike portion) — `official.runText` + * below is the retired `makeOfficialRunQueryShim` + this file's own + * `runTextViaShim` compile-compat bridge, replaced by the real + * `QueryExecutionDeps` shape `makeOfficialQueryExecutionAdapter` now supplies + * directly: no intermediate `(ctx, sql, RunQueryOptions)` shim, no + * `{error}`-to-throw translation layer. */ function serviceFor(conn: ReturnType, fixturePrefix: string) { let n = 0; - const runQueryShim = makeOfficialRunQueryShim(conn, () => BASIC_USER_A); + const official = makeOfficialQueryExecutionAdapter(conn, () => BASIC_USER_A); return createQueryExecutionService({ - runQuery: runQueryShim as unknown as typeof import('../../../src/net/ch-client.js').runQuery, - killQuery: async () => {}, - ctx: () => ({} as ChCtx), + // Never exercised by the `serviceFor()`-routed tests below — they only + // ever call `executeScript` (whole-body text mode). + runProgress: async () => { throw new Error('runProgress not exercised by this spike helper'); }, + runText: official.runText, + cancel: async () => {}, now: () => Date.now(), uid: () => { n += 1; return `${fixturePrefix}__${n}`; }, retryMs: 1, sleep: () => Promise.resolve(), - sqlString: (s) => `'${String(s)}'`, }); } @@ -388,7 +396,7 @@ describe('cancellation lease — the REAL production killQueryWithLease uses the const rotatedAuthorization = `Basic ${btoa('rotated-user:rotated-pass')}`; expect(rotatedAuthorization).not.toBe(frozenAuthorization); // sanity: the two really differ - await killQueryWithLease(lease, qid('ordinary-query'), (s) => `'${String(s)}'`); + await killQueryWithLease(lease, qid('ordinary-query')); expect(lastAuth()).toBe(frozenAuthorization); }); @@ -398,8 +406,8 @@ describe('cancellation lease — the REAL production killQueryWithLease uses the const lease: AuthenticatedCancellationLease = { epoch: 1, origin: fault.baseUrl, authorization: 'Basic irrelevant', fetch: capturing, }; - await killQueryWithLease(lease, null, (s) => `'${String(s)}'`); - await killQueryWithLease(lease, undefined, (s) => `'${String(s)}'`); + await killQueryWithLease(lease, null); + await killQueryWithLease(lease, undefined); expect(lastAuth()).toBeNull(); }); }); @@ -431,35 +439,36 @@ describe('retry safety — official outcomes fed through the REAL, unmodified Qu expect(result.entries[0].status).not.toBe('error'); }); - it('SESSION_IS_LOCKED: raw shim retried once by hand-driving the same policy the service applies', async () => { + it('SESSION_IS_LOCKED: raw adapter retried once by hand-driving the same policy the service applies', async () => { const conn = createOfficialConnection(fault.baseUrl, fetch); - const runQueryShim = makeOfficialRunQueryShim(conn, () => BASIC_USER_A); + const official = makeOfficialQueryExecutionAdapter(conn, () => BASIC_USER_A); const id = qid('session-is-locked'); - const first = await runQueryShim({} as ChCtx, 'SELECT 1', { format: 'Table', queryId: id }); + const request = { sql: 'SELECT 1', defaultFormat: 'JSONStringsEachRowWithProgress', params: { query_id: id } }; // The retry policy's own `SESSION_BUSY` regex (query-execution-service.ts) // matches "locked by a concurrent" case-insensitively — it does not // depend on the "(SESSION_IS_LOCKED)" code-name suffix the official // client's `ClickHouseError` strips from the message (see the // pre-header-rejection scenario's comment above for the same finding). - expect(first.error).toContain('locked by a concurrent'); - const second = await runQueryShim({} as ChCtx, 'SELECT 1', { format: 'Table', queryId: id }); - expect(second).toEqual({ streamed: true }); + // A pre-header rejection now THROWS (matching the new "package consumers + // throw" contract, #630 Phase 7 §6.5), never a returned `{error}`. + await expect(official.runProgress(request, {})).rejects.toThrow(/locked by a concurrent/); + await expect(official.runProgress(request, {})).resolves.toBeUndefined(); }); - it('a mid-stream connection reset on a read propagates as a throw (matching runQuery\'s own throw contract, not a swallowed {error})', async () => { + it('a mid-stream connection reset on a read propagates as a throw (matching the new "package consumers throw" contract, not a swallowed {error})', async () => { const conn = createOfficialConnection(fault.baseUrl, fetch); - const runQueryShim = makeOfficialRunQueryShim(conn, () => BASIC_USER_A); + const official = makeOfficialQueryExecutionAdapter(conn, () => BASIC_USER_A); const id = qid('post-header-connection-reset'); - await expect(runQueryShim({} as ChCtx, 'SELECT 1', { format: 'Table', queryId: id })).rejects.toBeTruthy(); + await expect(official.runProgress({ sql: 'SELECT 1', defaultFormat: 'JSONStringsEachRowWithProgress', params: { query_id: id } }, {})).rejects.toBeTruthy(); }); it('read-reset-retries-once: a read retries once after a mid-stream reset and then succeeds (hand-driven, same policy shape as the SESSION_IS_LOCKED case above)', async () => { const conn = createOfficialConnection(fault.baseUrl, fetch); - const runQueryShim = makeOfficialRunQueryShim(conn, () => BASIC_USER_A); + const official = makeOfficialQueryExecutionAdapter(conn, () => BASIC_USER_A); const id = qid('read-reset-then-success'); - await expect(runQueryShim({} as ChCtx, 'SELECT 1', { format: 'Table', queryId: id })).rejects.toBeTruthy(); - const second = await runQueryShim({} as ChCtx, 'SELECT 1', { format: 'Table', queryId: id }); - expect(second).toEqual({ streamed: true }); + const request = { sql: 'SELECT 1', defaultFormat: 'JSONStringsEachRowWithProgress', params: { query_id: id } }; + await expect(official.runProgress(request, {})).rejects.toBeTruthy(); + await expect(official.runProgress(request, {})).resolves.toBeUndefined(); }); it('ambiguous INSERT reset: no retry through the REAL QueryExecutionService; the ambiguous-write message is preserved', async () => { diff --git a/tests/spike/clickhouse-client/run-matrix.mjs b/tests/spike/clickhouse-client/run-matrix.mjs index 1bc2690b..d06c71b0 100644 --- a/tests/spike/clickhouse-client/run-matrix.mjs +++ b/tests/spike/clickhouse-client/run-matrix.mjs @@ -753,39 +753,50 @@ async function checkFormatTypeProbeCompiles() { // mechanics the official client's own // exec()/query()/command() supersede. // rewrite-narrow-adapter — credential-epoch fencing folded into -// authedFetch; a Phase 2 adapter would +// the now-deleted authedFetch (issue #630 +// Phase 6 moved that wiring into +// authenticated-clickhouse-request.ts; a +// Phase 2 official-client adapter would // reimplement this as its own narrow -// guard (this spike's guarded-fetch.ts is -// the working precedent). -// retain-temporary-bridge — KILL QUERY + the frozen cancellation -// lease; plan §28 names both explicitly -// as their own bucket, not deletion- -// eligible. +// guard — this spike's guarded-fetch.ts +// is the working precedent). No current +// ch-client.ts symbol carries this bucket +// any more; the definition is kept for +// completeness. +// retain-temporary-bridge — the frozen-lease KILL QUERY policy +// (killQueryWithLease); plan §28 named +// this its own bucket, not deletion- +// eligible — issue #630 Phase 7 kept it +// as a permanent SQL Browser policy seam, +// never a forwarding wrapper. // unrelated-product-operation — schema/lineage/reference-data/doc // browsing: domain-specific SQL, never // generic transport, unaffected by which // client library issues the request. // -// `chUrl` is NOT here — issue #585 Phase 1 (PR #621) moved it verbatim to -// `clickhouse-http-transport.ts` (see HTTP_TRANSPORT_CLASSIFICATION below); +// `chUrl` is NOT here — the package (`@altinity/clickhouse-http`) owns it; // ch-client.ts only re-exports the name (`export { chUrl };`, no `const`/ // `function` keyword), which the boundary regex correctly does not match, so // keeping a `chUrl` entry here would itself be exactly the stale- // classification drift the mirror guard now catches. +// +// Issue #630 Phase 7 deleted `isCurrentEpoch`, `staleEpochAbort`, +// `transportFor`, and `authedFetch` outright (their credential-epoch/generic- +// transport wiring had already folded into `authenticated-clickhouse- +// request.ts` in Phase 6, with no forwarding wrapper left in this file), +// plus the ordinary mutable-context `killQuery`, `exportQuery`, and +// `runQuery` (their SQL Browser policy moved to `query-execution- +// service.ts`/`export-service.ts`, driving the package's request/response +// primitives directly — Checkpoint 2D). None of those seven names has a +// classification entry below any more — re-adding one without the symbol +// itself returning would be exactly the stale-classification drift the +// mirror guard now catches. const CH_CLIENT_CLASSIFICATION = { isAbort: 'delete-after-cutover', errMessage: 'delete-after-cutover', - isCurrentEpoch: 'rewrite-narrow-adapter', - staleEpochAbort: 'rewrite-narrow-adapter', - // Thin wiring from `ChCtx` to the current generic transport — no auth/ - // epoch policy of its own (that stays in authedFetch); Phase 2 replaces - // `createHttpTransport` itself, so this wiring goes with it. - transportFor: 'delete-after-cutover', - authedFetch: 'rewrite-narrow-adapter', queryJson: 'delete-after-cutover', querySystemAware: 'unrelated-product-operation', loadDataLakeCatalogTableNames: 'unrelated-product-operation', - killQuery: 'retain-temporary-bridge', killQueryWithLease: 'retain-temporary-bridge', loadServerVersion: 'unrelated-product-operation', byUnderscoreThenName: 'unrelated-product-operation', @@ -808,23 +819,20 @@ const CH_CLIENT_CLASSIFICATION = { loadDocRow: 'unrelated-product-operation', loadFunctionsDocColumns: 'unrelated-product-operation', loadFunctionDocRow: 'unrelated-product-operation', - exportQuery: 'delete-after-cutover', - runQuery: 'delete-after-cutover', }; -// src/net/clickhouse-http-transport.ts (issue #585 Phase 1, PR #621) — the -// current custom generic-transport implementation `chUrl` moved into, -// alongside the progress-line stream-read loop and the transport factory -// itself. All three are generic HTTP/URL/stream mechanics (ADR-0005's -// "Official client owns" column — request construction, response streaming), -// never SQL Browser policy, so all three are `delete-after-cutover`: a Phase -// 2 official-client-backed transport implementation replaces this whole -// file, not just parts of it. -const HTTP_TRANSPORT_CLASSIFICATION = { - chUrl: 'delete-after-cutover', - streamLines: 'delete-after-cutover', - createHttpTransport: 'delete-after-cutover', -}; +// src/net/clickhouse-http-transport.ts — the custom generic-transport +// implementation `chUrl`/`streamLines`/`createHttpTransport` moved into +// after issue #585 Phase 1 — is DELETED as of issue #630 Phase 7 +// Checkpoint 2D: `killQueryWithLease` (above) was rewritten onto the +// package's own stateless `createClickHouseHttpClient(...).killQuery(...)`, +// which was that file's last remaining caller, and `src/net/clickhouse- +// transport.types.ts` went with it. There is exactly one generic ClickHouse +// HTTP transport implementation left in the repository now (the package's) +// — no manifest entry, classification table, or disk read for either +// deleted file remains in this module (see the real-tree regression in +// run-matrix.test.ts proving `computeDeletionEstimate()` works with the +// adapter file absent). // official-adapter.ts symbols that are SPIKE-TEST-ONLY harness scaffolding // (exist to let parity.test.ts hand-drive specific retry-safety scenarios), @@ -832,7 +840,13 @@ const HTTP_TRANSPORT_CLASSIFICATION = { // test or orchestration code counted as production deletion". Excluded from // the "estimated official adapter executable LOC" figure entirely (not // classified into a bucket at all). -const OFFICIAL_ADAPTER_TEST_ONLY_SYMBOLS = ['RefreshDrivenResult', 'runOfficialRefreshThenRetry', 'makeOfficialRunQueryShim']; +// `makeOfficialRunQueryShim` (retired, issue #630 Phase 7 Checkpoint 2C's +// spike portion) is replaced here by `makeOfficialQueryExecutionAdapter` — +// same test-only-harness role (per its own doc comment, "the direct +// replacement for the retired `makeOfficialRunQueryShim`"), just satisfying +// `QueryExecutionService`'s narrow post-Phase-7 dependency shape instead of +// the retiring `typeof runQuery`. +const OFFICIAL_ADAPTER_TEST_ONLY_SYMBOLS = ['RefreshDrivenResult', 'runOfficialRefreshThenRetry', 'makeOfficialQueryExecutionAdapter']; // NOTE: `OfficialConnection` is deliberately NOT a key here (and never was // matched by either the old or the broadened boundary regex): it is an // `export interface`, and this function's boundary detection only ever @@ -868,14 +882,13 @@ const OFFICIAL_ADAPTER_CORE_CLASSIFICATION = { export async function computeDeletionEstimate() { const chClientPath = join(repoRoot, 'src/net/ch-client.ts'); const chClientBuckets = await classifyFunctionRanges(chClientPath, CH_CLIENT_CLASSIFICATION); - // Issue #585 Phase 1 (PR #621) moved chUrl/streamLines/createHttpTransport - // out of ch-client.ts into their own file — classified and measured - // separately here (its own manifest entry, own boundary set) so a drift in - // EITHER file fails loudly on its own, then combined into one - // `currentGenericLoc` below since both files together are the current - // generic-transport surface the deletion estimate is about. - const httpTransportPath = join(repoRoot, 'src/net/clickhouse-http-transport.ts'); - const httpTransportBuckets = await classifyFunctionRanges(httpTransportPath, HTTP_TRANSPORT_CLASSIFICATION); + // Issue #630 Phase 7 deleted src/net/clickhouse-http-transport.ts (and its + // type seam) outright — there is no second file's `delete-after-cutover` + // bucket to classify/measure/combine any more. `currentGenericLoc` below + // is ch-client.ts's own bucket alone, and this function deliberately never + // opens `src/net/clickhouse-http-transport.ts` (confirmed absent by the + // real-tree regression in run-matrix.test.ts) — reintroducing a read of + // that path here would just reopen the ENOENT this checkpoint removed. const officialAdapterPath = join(spikeDir, 'official-adapter.ts'); const officialBuckets = await classifyFunctionRanges( officialAdapterPath, OFFICIAL_ADAPTER_CORE_CLASSIFICATION, OFFICIAL_ADAPTER_TEST_ONLY_SYMBOLS, @@ -886,8 +899,7 @@ export async function computeDeletionEstimate() { const streamTsLines = (await readFile(join(repoRoot, 'src/core/stream.ts'), 'utf8')).split('\n').length; const qesLines = (await readFile(join(repoRoot, 'src/application/query-execution-service.ts'), 'utf8')).split('\n').length; - const currentGenericLoc = (chClientBuckets['delete-after-cutover'] || 0) - + (httpTransportBuckets['delete-after-cutover'] || 0); + const currentGenericLoc = chClientBuckets['delete-after-cutover'] || 0; const estimatedOfficialAdapterLoc = officialBuckets['official-adapter-core'] || 0; const acceptedBridgeGuardLoc = bridgeLoc.physical + guardLoc.physical; const netExecutableDeletion = currentGenericLoc - estimatedOfficialAdapterLoc - acceptedBridgeGuardLoc; @@ -899,7 +911,6 @@ export async function computeDeletionEstimate() { netExecutableDeletion, manifest: { 'ch-client.ts': chClientBuckets, - 'clickhouse-http-transport.ts': httpTransportBuckets, 'official-adapter.ts (test-only harness excluded)': officialBuckets, 'progress-bridge.ts (physical)': bridgeLoc.physical, 'guarded-fetch.ts (physical)': guardLoc.physical, @@ -917,12 +928,14 @@ export function renderDeletionEstimateMd(d) { L.push('# Future production deletion estimate (plan §28)'); L.push(''); L.push('Estimate only — actual deletion is Phase 4, per plan §4/§28. Computed mechanically'); - L.push('from `src/net/ch-client.ts`\'s and `src/net/clickhouse-http-transport.ts`\'s own top-level'); - L.push('symbol boundaries (see `run-matrix.mjs`\'s `CH_CLIENT_CLASSIFICATION`/'); - L.push('`HTTP_TRANSPORT_CLASSIFICATION` data tables) so the figures stay tied to the real files'); - L.push('rather than a hand-typed guess; an unclassified symbol, or a classification-table entry'); - L.push('that no longer matches anything, makes `run-matrix.mjs` throw instead of silently under/'); - L.push('over-counting in either direction.'); + L.push('from `src/net/ch-client.ts`\'s own top-level symbol boundaries (see `run-matrix.mjs`\'s'); + L.push('`CH_CLIENT_CLASSIFICATION` data table) so the figures stay tied to the real file rather'); + L.push('than a hand-typed guess; an unclassified symbol, or a classification-table entry that no'); + L.push('longer matches anything, makes `run-matrix.mjs` throw instead of silently under/over-'); + L.push('counting in either direction. `src/net/clickhouse-http-transport.ts` — a second file this'); + L.push('estimate used to classify separately and sum in (issue #585 Phase 1, PR #621) — is deleted'); + L.push('as of issue #630 Phase 7 Checkpoint 2D; this estimate has no classification table, manifest'); + L.push('entry, or disk read for it any more.'); L.push(''); L.push('## `src/net/ch-client.ts` buckets (physical LOC per top-level symbol range)'); L.push(''); @@ -930,17 +943,6 @@ export function renderDeletionEstimateMd(d) { L.push('|---|---|'); for (const [bucket, loc] of Object.entries(d.manifest['ch-client.ts'])) L.push(`| \`${bucket}\` | ${loc} |`); L.push(''); - L.push('## `src/net/clickhouse-http-transport.ts` buckets (physical LOC per top-level symbol range)'); - L.push(''); - L.push('Issue #585 Phase 1 (PR #621) moved `chUrl` (+ the progress-line stream-read loop and the'); - L.push('transport factory) out of `ch-client.ts` into this file — classified here on its own so it'); - L.push('stays tied to the real file, then combined with `ch-client.ts`\'s own `delete-after-cutover`'); - L.push('bucket below for the net-deletion formula.'); - L.push(''); - L.push('| Bucket | Physical LOC |'); - L.push('|---|---|'); - for (const [bucket, loc] of Object.entries(d.manifest['clickhouse-http-transport.ts'])) L.push(`| \`${bucket}\` | ${loc} |`); - L.push(''); L.push('## Other named responsibilities'); L.push(''); L.push('| Responsibility | Final owner / bucket | Physical LOC |'); @@ -960,13 +962,13 @@ export function renderDeletionEstimateMd(d) { L.push('official-adapter.ts terms, which inflated those two terms (concentrated in'); L.push('comment-heavy functions like `runOfficial`) relative to the bridge/guard terms.'); L.push(''); - L.push('Issue #585 Phase 1 (PR #621) split the current generic-transport surface across TWO files —'); - L.push('`ch-client.ts`\'s own `delete-after-cutover` bucket plus `clickhouse-http-transport.ts`\'s'); - L.push('(where `chUrl` now lives); the formula\'s first term is their SUM, not `ch-client.ts` alone.'); + L.push('Issue #630 Phase 7 Checkpoint 2D deleted `src/net/clickhouse-http-transport.ts` outright (the'); + L.push('second file issue #585 Phase 1, PR #621 had split the generic-transport surface across) — the'); + L.push('formula\'s first term is `ch-client.ts`\'s own `delete-after-cutover` bucket alone now, not a sum.'); L.push(''); L.push('```text'); L.push('current generic physical LOC eligible for deletion'); - L.push(` = ${d.currentGenericLocEligibleForDeletion} (ch-client.ts "delete-after-cutover" bucket + clickhouse-http-transport.ts "delete-after-cutover" bucket)`); + L.push(` = ${d.currentGenericLocEligibleForDeletion} (ch-client.ts "delete-after-cutover" bucket)`); L.push('- estimated official adapter physical LOC'); L.push(` = ${d.estimatedOfficialAdapterLoc} (official-adapter.ts production-shaped core)`); L.push('- accepted narrow bridge/guard physical LOC'); @@ -977,22 +979,11 @@ export function renderDeletionEstimateMd(d) { L.push(''); L.push(`Net deletion is ${d.positiveNetDeletion ? 'POSITIVE' : 'NOT positive'} — an Accepted ADR requires positive net deletion (plan §30 "Mark Accepted only if ... future net deletion is positive").`); L.push(''); - if (!d.positiveNetDeletion) { - L.push('**Caveat on this specific measurement**: the `delete-after-cutover` bucket above is computed'); - L.push('at WHOLE-FUNCTION granularity (a function is classified in full, never split). `authedFetch`'); - L.push('(56 physical lines) is classified entirely as `rewrite-narrow-adapter` because it currently'); - L.push('interleaves generic fetch/response mechanics with the narrow credential-epoch guard — a finer,'); - L.push('sub-function split (out of scope for this mechanical pass) would likely move a meaningful'); - L.push('fraction of those lines into `delete-after-cutover` instead, which would make the net figure'); - L.push('less negative or positive. Reported as computed, not adjusted, so the ADR sees the real'); - L.push('mechanical result and can decide whether a finer split is warranted before relying on it.'); - L.push(''); - } L.push('Buckets NOT counted toward deletion (retained, rewritten, or unrelated — each with exactly'); L.push('one final owner, per plan §28 "no permanent dual generic transport"):'); L.push(''); - L.push('- `rewrite-narrow-adapter` — credential-epoch fencing folds into the official adapter\'s own request construction (this spike\'s `guarded-fetch.ts` is the working precedent).'); - L.push('- `retain-temporary-bridge` — `KILL QUERY` + the frozen cancellation lease.'); + L.push('- `rewrite-narrow-adapter` — credential-epoch fencing folds into the official adapter\'s own request construction (this spike\'s `guarded-fetch.ts` is the working precedent). No current `ch-client.ts` symbol carries this bucket (issue #630 Phase 7 deleted its former members, `isCurrentEpoch`/`staleEpochAbort`/`authedFetch`, outright); kept for definitional completeness.'); + L.push('- `retain-temporary-bridge` — the frozen-lease `killQueryWithLease` policy (the ordinary mutable-context `killQuery` this bucket also used to cover was deleted outright in issue #630 Phase 7, with no forwarding wrapper).'); L.push('- `unrelated-product-operation` — schema/lineage/reference-data/doc-browsing SQL: never generic transport.'); L.push('- `retain-as-sql-browser-policy` — `src/core/stream.ts` (normalized outcome) and `src/application/query-execution-service.ts` (retry safety): both already isolated from ch-client.ts and untouched by transport choice.'); L.push(''); diff --git a/tests/spike/clickhouse-client/run-matrix.test.ts b/tests/spike/clickhouse-client/run-matrix.test.ts index 5599b470..45a66a3b 100644 --- a/tests/spike/clickhouse-client/run-matrix.test.ts +++ b/tests/spike/clickhouse-client/run-matrix.test.ts @@ -12,6 +12,9 @@ // requirement — it is regression coverage for two review findings, per // CLAUDE.md hard rule 1 ("add tests in the same change as the code"). import { describe, expect, it } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { dirname, join } from 'node:path'; import { selectEarliestPassingVersion, collectBrowserFailureDetail as collectBrowserFailureDetailUntyped, @@ -23,6 +26,29 @@ import { deriveDecision as deriveDecisionUntyped, } from './run-matrix.mjs'; +// Same repoRoot derivation run-matrix.mjs itself uses (three levels up from +// tests/spike/clickhouse-client/), built from only `dirname`/`join` — the +// repo's ambient `node:path` shim (tests/types/node-fs-url.d.ts) declares no +// `resolve`, and `join(...)` normalizes its own `..` segments exactly like +// `resolve` would for an already-absolute base. Needed only for the +// real-tree "adapter file absent" regression below, which asserts against +// the actual repo tree rather than a fixture. +const repoRootForTest = join(dirname(fileURLToPath(import.meta.url)), '..', '..', '..'); + +// `existsSync` isn't in the repo's ambient `node:fs` shim either (only +// `readFileSync`/`readdirSync` are) — reusing `readFileSync` under a +// try/catch keeps this real-tree regression inside this sub-task's declared +// file scope (run-matrix.mjs/run-matrix.test.ts only) instead of also +// touching the shared ambient-types file for one call site. +function fileExists(path: string): boolean { + try { + readFileSync(path, 'utf8'); + return true; + } catch { + return false; + } +} + const computeGates = computeGatesUntyped as (r: unknown) => Record; const deriveDecision = deriveDecisionUntyped as (gates: Record) => { status: string; rationale: string[] }; @@ -665,28 +691,37 @@ describe('computeDeletionEstimate (P3 review finding: the whole-formula regressi expect(d.acceptedBridgeGuardLoc).toBe(d.bridgeLoc.physical + d.guardLoc.physical); }); - it('classifies src/net/clickhouse-http-transport.ts on its own (issue #585 Phase 1, PR #621 moved chUrl/streamLines/createHttpTransport there) and combines its delete-after-cutover bucket into currentGenericLocEligibleForDeletion alongside ch-client.ts\'s own', async () => { + // Issue #630 Phase 7 Checkpoint 2D deleted src/net/clickhouse-http-transport.ts + // outright (killQueryWithLease's rewrite onto the package's own stateless + // killQuery() removed that file's last caller) — the real-tree regression + // this sub-task's definition of done requires: computeDeletionEstimate() + // must genuinely work with the adapter file absent, not merely stop + // referencing it in source. + it('src/net/clickhouse-http-transport.ts is absent from the tree, and computeDeletionEstimate() works without it — the real-tree regression for issue #630 Phase 7 Checkpoint 2D\'s deletion', async () => { + const transportPath = join(repoRootForTest, 'src/net/clickhouse-http-transport.ts'); + expect(fileExists(transportPath)).toBe(false); + const typesPath = join(repoRootForTest, 'src/net/clickhouse-transport.types.ts'); + expect(fileExists(typesPath)).toBe(false); + const d = await computeDeletionEstimate(); - // The transport file gets its OWN manifest entry, distinct from - // ch-client.ts's — per-file transparency is preserved even though the - // two are summed for the headline figure. - // `computeDeletionEstimate`'s return value comes from the untyped .mjs - // orchestrator (same interop limitation `classifyFunctionRangesFromSource` - // is cast for above) — the manifest's per-file bucket objects need the - // same explicit local type for indexing. - const httpTransportBuckets = d.manifest['clickhouse-http-transport.ts'] as Record; + // No second file's bucket to classify/measure/combine any more — the + // manifest carries ch-client.ts's own entry only. `d.manifest`'s inferred + // shape (from the untyped .mjs orchestrator) has no + // `'clickhouse-http-transport.ts'` property at all any more, which is + // itself a compile-time proof of removal; a plain runtime index (cast to + // a wide manifest type) still confirms it's absent at runtime too. + const manifest = d.manifest as Record; + expect(manifest['clickhouse-http-transport.ts']).toBeUndefined(); + // The manifest's per-file bucket object needs the same explicit local + // type for indexing `classifyFunctionRangesFromSource` is cast for above. const chClientBuckets = d.manifest['ch-client.ts'] as Record; - expect(httpTransportBuckets).toBeDefined(); - expect(Object.keys(httpTransportBuckets)).toEqual(['delete-after-cutover']); - expect(httpTransportBuckets['delete-after-cutover']).toBeGreaterThan(0); - // currentGenericLocEligibleForDeletion is the SUM of both files' own - // delete-after-cutover buckets, not either one alone. - const expectedCombined = (chClientBuckets['delete-after-cutover'] || 0) - + httpTransportBuckets['delete-after-cutover']; - expect(d.currentGenericLocEligibleForDeletion).toBe(expectedCombined); + expect(chClientBuckets['delete-after-cutover']).toBeGreaterThan(0); + // currentGenericLocEligibleForDeletion is ch-client.ts's OWN bucket + // alone now — never a sum with a second (deleted) file's bucket. + expect(d.currentGenericLocEligibleForDeletion).toBe(chClientBuckets['delete-after-cutover']); }); - it('does not throw for the real ch-client.ts / clickhouse-http-transport.ts / official-adapter.ts files under the broadened boundary regex and the new symmetric drift guard — the real-file proof that CH_CLIENT_CLASSIFICATION and HTTP_TRANSPORT_CLASSIFICATION stay exhaustive in both directions', async () => { + it('does not throw for the real ch-client.ts / official-adapter.ts files under the broadened boundary regex and the symmetric drift guard — the real-file proof that CH_CLIENT_CLASSIFICATION and OFFICIAL_ADAPTER_CORE_CLASSIFICATION stay exhaustive in both directions post-Phase-7', async () => { await expect(computeDeletionEstimate()).resolves.toBeDefined(); }); }); diff --git a/tests/unit/app.test.ts b/tests/unit/app.test.ts index 6b7baabc..98ec8266 100644 --- a/tests/unit/app.test.ts +++ b/tests/unit/app.test.ts @@ -2924,6 +2924,28 @@ describe('query run', () => { resolveRunFetch(Promise.reject(Object.assign(new Error('aborted'), { name: 'AbortError' }))); await pending; }); + // #630 Phase 7 §9.2/9.3 — a replacement (non-owner) epoch must never reach + // the frozen kill: `cancelOwnedQuery` checks `captureCancellationLease`'s + // null return and skips the remote KILL QUERY entirely (still a silent, + // safe no-op — the local abort above already stopped the client side). + it('cancel() skips the remote KILL QUERY when captureCancellationLease rejects a stale owner epoch', async () => { + let resolveRunFetch!: (value: FakeResponse | Promise) => void; + const fetch = asFetch(vi.fn((_url: string, init?: { body?: string }) => (init && /SELECT 1/.test(init.body || '') + ? new Promise((res) => { resolveRunFetch = res; }) + : Promise.resolve(resp({ json: { data: [] } }))))); + const { app, e } = appForRun([], { fetch }); + app.activeTab().sqlDraft = 'SELECT 1'; + const pending = app.actions.run(); + await new Promise((r) => setTimeout(r)); + expect(app.state.running.value).toBe(true); + asMock(e.fetch!).mockClear(); + vi.spyOn(app.conn, 'captureCancellationLease').mockReturnValueOnce(null); + app.actions.cancel(); + await new Promise((r) => setTimeout(r)); + expect(asMock(e.fetch!).mock.calls.some((c) => /KILL QUERY/.test((c[1] && c[1].body) || ''))).toBe(false); + resolveRunFetch(Promise.reject(Object.assign(new Error('aborted'), { name: 'AbortError' }))); + await pending; + }); // #276 Phase 5: signOut is the first production wiring of the sessions' // teardown surfaces — an in-flight run must be aborted + server-killed and // the catalog caches dropped BEFORE the login screen appears, and the @@ -5868,6 +5890,32 @@ describe('streaming export (issue #87)', () => { expect(qs(document, '.share-toast').textContent).toBe('Nothing to export'); }); + // #630 Phase 7 — ExportService's `ctx()` survives only for its + // `.onSignedOut()` call (no transport reads it any more); this proves + // app.ts's own real `ctx: () => chCtx` wiring (not just export-service.ts's + // unit-level fake) is reached. Called directly on `app.exports` (not + // `app.actions.exportEntry`, which is gated behind + // `withAuthenticatedExecution` — never invoking export-service at all when + // signed out): with no token and no execution scope, `getToken()` resolves + // null via its plain `!token` branch with NO auth-loss side effect, so the + // picker opens (transient-activation ordering) and export-service's own + // `isCurrent(null)` fence (a null scope is always "current") lets + // `ctx().onSignedOut()` actually fire. + it('signed-out direct export (no token, no scope): the picker still opens, but no query runs', async () => { + const { handle } = fakeFileHandle(); + const showSaveFilePicker = vi.fn(async () => handle); + const app = createApp(env({ + window: fakeWin(), showSaveFilePicker, isSecureContext: true, sessionStorage: memSession({}), + })); + app.activeTab().sqlDraft = 'SELECT 1'; + const onSignedOut = vi.spyOn(app.conn.chCtx, 'onSignedOut'); + await app.exports.exportDirect('SELECT 1', 0); + expect(showSaveFilePicker).toHaveBeenCalledTimes(1); + expect(handle.createWritable).not.toHaveBeenCalled(); + expect(onSignedOut).toHaveBeenCalledTimes(1); + expect(app.state.exporting.value).toBe(false); + }); + it('streams a clean result to disk (default TSV) and reports completion — a real round trip through app.ts\'s own ExportSink/hooks wiring', async () => { const { handle, writable, chunks } = fakeFileHandle(); let pickerOpts: SaveFilePickerOpts | undefined; diff --git a/tests/unit/authenticated-clickhouse-request.test.ts b/tests/unit/authenticated-clickhouse-request.test.ts index 406c25dc..4721b5a5 100644 --- a/tests/unit/authenticated-clickhouse-request.test.ts +++ b/tests/unit/authenticated-clickhouse-request.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect, vi } from 'vitest'; import type { Mock } from 'vitest'; import { - authenticatedRequest, authenticatedJson, authenticatedText, authenticatedProgress, + authenticatedRequest, authenticatedResponse, authenticatedJson, authenticatedText, authenticatedProgress, } from '../../src/net/authenticated-clickhouse-request.js'; import type { AuthenticatedRequestCtx } from '../../src/net/authenticated-clickhouse-request.js'; import { ClickHouseError } from '@altinity/clickhouse-http'; @@ -536,6 +536,55 @@ describe('authenticatedRequest — live origin authority across retry (Adaptatio }); }); +// Issue #630 Phase 7 §5/§23 — `authenticatedResponse` composes +// `authenticatedRequest` with exactly the package's `ensureClickHouseSuccess` +// classifier (no consumer, unlike `authenticatedJson`/`authenticatedText`/ +// `authenticatedProgress` below): it hands the caller back the exact +// successful native `Response`, untouched, so a caller that must own its own +// byte-stream consumption (raw export) can read the body itself. Only ONE +// package classification happens after settlement; `authenticatedRequest` +// remains the sole owner of auth/epoch/refresh/lifecycle, and this adds no +// retry and no second Fetch. +describe('authenticatedResponse — package classification composition', () => { + it('resolves the exact successful native Response by identity, with its body left completely unread', async () => { + const textSpy = vi.fn(async () => 'unused'); + const response: FakeResponse = { ok: true, status: 200, text: textSpy, clone() { return response; } }; + const ctx = ctxWith(async () => response); + const resp = await authenticatedResponse(ctx, { sql: 'SELECT 1', defaultFormat: 'TSV' }); + expect(resp).toBe(response); + expect(textSpy).not.toHaveBeenCalled(); + expect(ctx.fetchMock).toHaveBeenCalledTimes(1); + }); + it('throws the package ClickHouseError on a resolved non-2xx response, performing no second Fetch for classification', async () => { + const ctx = ctxWith(async () => textResp('Code: 999. DB::Exception: boom', false, 500), { authConfirmed: true }); + const err: unknown = await authenticatedResponse(ctx, { sql: 'bad', defaultFormat: 'TSV' }).catch((e: unknown) => e); + expect(err).toBeInstanceOf(ClickHouseError); + expect((err as ClickHouseError).message).toBe('Code: 999. DB::Exception: boom'); + expect((err as ClickHouseError).status).toBe(500); + expect(ctx.fetchMock).toHaveBeenCalledTimes(1); + }); + it('never starts non-2xx classification when the request itself was superseded (abort), and never wraps that rejection', async () => { + const abortError = Object.assign(new Error('cancelled request'), { name: 'AbortError' }); + const ctx = ctxWith(async () => { throw abortError; }); + await expect(authenticatedResponse(ctx, { sql: 'SELECT 1', defaultFormat: 'TSV' })).rejects.toBe(abortError); + }); + it('propagates a native fetch network TypeError rejection by identity, never wrapped as ClickHouseError', async () => { + const networkError = new TypeError('Failed to fetch'); + const ctx = ctxWith(async () => { throw networkError; }); + await expect(authenticatedResponse(ctx, { sql: 'SELECT 1', defaultFormat: 'TSV' })).rejects.toBe(networkError); + }); + it('still refreshes exactly once on 401 before classifying the retried response — unchanged refresh bounds', async () => { + let n = 0; + const ctx = ctxWith(async () => (n++ === 0 ? jsonResp({}, false, 401) : jsonResp({ ok: 1 })), { + refresh: vi.fn(async () => true), + }); + const resp = await authenticatedResponse(ctx, { sql: 'SELECT 1', defaultFormat: 'JSON' }); + expect(resp.ok).toBe(true); + expect(ctx.refresh).toHaveBeenCalledTimes(1); + expect(ctx.fetchMock).toHaveBeenCalledTimes(2); + }); +}); + // Package-consumer composition (plan §10 "Package-consumer composition // tests"): `authenticatedJson`/`authenticatedText`/`authenticatedProgress` // each compose `authenticatedRequest` with exactly one matching package diff --git a/tests/unit/ch-client.test.ts b/tests/unit/ch-client.test.ts index 16bf7aeb..39de7f24 100644 --- a/tests/unit/ch-client.test.ts +++ b/tests/unit/ch-client.test.ts @@ -1,8 +1,8 @@ import { describe, it, expect, vi } from 'vitest'; import { - chUrl, queryJson, loadServerVersion, loadSchema, loadColumns, loadReferenceData, loadFunctionsDocColumns, loadFunctionDocRow, loadDocTableColumns, loadDocRow, runQuery, killQuery, killQueryWithLease, exportQuery, loadSchemaLineage, loadSchemaCards, loadLineageTransitive, loadTableDetail, AST_PROGRESSIVE_THRESHOLD, byUnderscoreThenName, + chUrl, queryJson, loadServerVersion, loadSchema, loadColumns, loadReferenceData, loadFunctionsDocColumns, loadFunctionDocRow, loadDocTableColumns, loadDocRow, killQueryWithLease, loadSchemaLineage, loadSchemaCards, loadLineageTransitive, loadTableDetail, AST_PROGRESSIVE_THRESHOLD, byUnderscoreThenName, } from '../../src/net/ch-client.js'; -import type { AuthenticatedCancellationLease, ChCtx, DocProbeTable, StreamLine } from '../../src/net/ch-client.js'; +import type { AuthenticatedCancellationLease, ChCtx, DocProbeTable } from '../../src/net/ch-client.js'; // Issue #630 Phase 5 — sqlString now has one implementation, owned by the // package; format.js no longer declares it. Issue #630 Phase 6 — ClickHouseError // is the package's own non-2xx error class; queryJson's compatibility test @@ -100,13 +100,17 @@ function deferred() { // test (including its malformed-URL preflight-ordering proof) MOVED to // `tests/unit/authenticated-clickhouse-request.test.ts`, retargeted onto // `authenticatedRequest` — this file now keeps only caller-level proofs that -// the exported `queryJson`/`runQuery`/`exportQuery` still reach that new -// module correctly. +// the exported `queryJson` still reaches that new module correctly. Issue +// #630 Phase 7 — the generic, format-agnostic `runQuery`/`exportQuery` and +// the ordinary mutable-context `killQuery` this file used to also cover are +// DELETED (see the note just above the `killQueryWithLease` describe block +// below for where their coverage moved). // (queryDashboardTile was retired in #193 — dashboard tiles now stream through -// runQuery via the shared app.exec.executeRead seam (#276), carrying readonly:2 / -// max_result_bytes / param_* in `params` and capping with resultRowLimit. Its -// URL-shaping is covered by runQuery's tests below; the dashboard's use of the +// the shared app.exec.executeRead seam (#276), carrying readonly:2 / +// max_result_bytes / param_* in `params` and capping with resultRowLimit — +// now QueryExecutionService's own row-cap/format mapping (#630 Phase 7, see +// tests/unit/query-execution-service.test.ts); the dashboard's use of the // seam is covered in dashboard.test.js.) describe('queryJson', () => { @@ -604,113 +608,18 @@ describe('loadFunctionsDocColumns / loadFunctionDocRow delegate to the generaliz }); }); -describe('runQuery', () => { - it('uses typed progress streaming for the KPI transport alias', async () => { - const ctx = ctxWith(async () => streamResp([ - '{"meta":[{"name":"metric","type":"Tuple(value Decimal(38, 2), delta Decimal(38, 2))"}]}\n', - '{"row":{"metric":{"value":"9007199254740993.25","delta":"-9007199254740993.25"}}}\n', - ])); - const lines: StreamLine[] = []; - await runQuery(ctx, 'SELECT 1', { format: 'KPI', params: { output_format_json_named_tuples_as_objects: 1, output_format_json_quote_decimals: 1 }, onLine: (line) => lines.push(line) }); - expect(ctx.fetchMock.mock.calls[0][0]).toContain('default_format=JSONEachRowWithProgress'); - expect(ctx.fetchMock.mock.calls[0][0]).toContain('output_format_json_named_tuples_as_objects=1'); - expect(ctx.fetchMock.mock.calls[0][0]).toContain('output_format_json_quote_decimals=1'); - expect((lines[1].row as Record).metric).toEqual({ value: '9007199254740993.25', delta: '-9007199254740993.25' }); - }); - it('streams lines and reports an error result on !ok', async () => { - const ctx = ctxWith(async () => textResp('{"exception":"boom"}', false, 500)); - const out = await runQuery(ctx, 'bad', { format: 'Table' }); - expect(out).toEqual({ error: 'boom' }); - }); - it('parses a streaming body, calling onLine + onChunk', async () => { - const lines = [ - '{"meta":[{"name":"a","type":"UInt8"}]}\n', - // blank line between objects exercises the `if (!line) continue` guard - '{"row":{"a":"1"}}\n\n{"progress":{"read_rows":"1"}}\n', - '{"row":{"a":"2"}}', // trailing, no newline - ]; - const ctx = ctxWith(async () => streamResp(lines)); - const got: StreamLine[] = []; - const out = await runQuery(ctx, 'SELECT a', { format: 'Table', onLine: (j) => got.push(j), onChunk: () => {} }); - expect(out).toEqual({ streamed: true }); - expect(got.filter((j) => j.row)).toHaveLength(2); - expect(got.some((j) => j.meta)).toBe(true); - }); - it('skips malformed lines and a malformed trailing buffer', async () => { - const ctx = ctxWith(async () => streamResp(['not json\n', '{bad trailing'])); - const got: StreamLine[] = []; - const out = await runQuery(ctx, 'x', { onLine: (j) => got.push(j) }); - expect(out).toEqual({ streamed: true }); - expect(got).toEqual([]); - }); - it('defaults format to Table (streaming)', async () => { - const ctx = ctxWith(async () => streamResp(['{"row":{}}\n'])); - const out = await runQuery(ctx, 'x', {}); - expect(out).toEqual({ streamed: true }); - }); - it('TSV raw mode returns the text body', async () => { - const ctx = ctxWith(async () => textResp('a\tb\n1\t2')); - expect(await runQuery(ctx, 'x', { format: 'TSV' })).toEqual({ raw: 'a\tb\n1\t2' }); - }); - it('JSON raw mode returns the text body', async () => { - const ctx = ctxWith(async () => textResp('{"x":1}')); - expect(await runQuery(ctx, 'x', { format: 'JSON' })).toEqual({ raw: '{"x":1}' }); - }); - it('passes the abort signal through', async () => { - const ctx = ctxWith(async () => streamResp(['{"row":{}}\n'])); - const signal = new AbortController().signal; - await runQuery(ctx, 'x', { signal }); - expect(ctx.fetchMock.mock.calls[0][1].signal).toBe(signal); - }); - it('tags the run request with query_id when given', async () => { - const ctx = ctxWith(async () => streamResp(['{"row":{}}\n'])); - await runQuery(ctx, 'x', { queryId: 'abc-123' }); - expect(ctx.fetchMock.mock.calls[0][0]).toContain('query_id=abc-123'); - }); - it('passes caller params (e.g. result caps) alongside query_id', async () => { - const ctx = ctxWith(async () => textResp('{"meta":[],"data":[]}')); - await runQuery(ctx, 'x', { format: 'JSONCompact', queryId: 'q1', params: { max_result_rows: 100, result_overflow_mode: 'break' } }); - const url = ctx.fetchMock.mock.calls[0][0]; - expect(url).toContain('query_id=q1'); - expect(url).toContain('max_result_rows=100'); - expect(url).toContain('result_overflow_mode=break'); - }); - it('streams without wait_end_of_query; raw modes keep it for clean error status', async () => { - const s = ctxWith(async () => streamResp(['{"row":{}}\n'])); - await runQuery(s, 'x', { format: 'Table' }); - expect(s.fetchMock.mock.calls[0][0]).not.toContain('wait_end_of_query'); // progressive first rows - const raw = ctxWith(async () => textResp('a\tb')); - await runQuery(raw, 'x', { format: 'TSV' }); - expect(raw.fetchMock.mock.calls[0][0]).toContain('wait_end_of_query=1'); - }); - it('adds the server-side row cap when resultRowLimit is set; omits it otherwise', async () => { - const capped = ctxWith(async () => streamResp(['{"row":{}}\n'])); - await runQuery(capped, 'x', { format: 'Table', resultRowLimit: 500 }); - const url = capped.fetchMock.mock.calls[0][0]; - expect(url).toContain('max_result_rows=500'); - expect(url).toContain('result_overflow_mode=break'); - const uncapped = ctxWith(async () => streamResp(['{"row":{}}\n'])); - await runQuery(uncapped, 'x', { format: 'Table' }); // no limit → no cap params - expect(uncapped.fetchMock.mock.calls[0][0]).not.toContain('max_result_rows'); - }); -}); - -describe('killQuery', () => { - it('POSTs KILL QUERY for the query_id', async () => { - const ctx = ctxWith(async () => jsonResp({ data: [] })); - await killQuery(ctx, 'abc-123', sqlString); - expect(ctx.fetchMock.mock.calls[0][1].body).toBe("KILL QUERY WHERE query_id = 'abc-123' ASYNC"); - }); - it('no-ops without a query_id', async () => { - const ctx = ctxWith(async () => jsonResp({ data: [] })); - await killQuery(ctx, null, sqlString); - expect(ctx.fetchMock).not.toHaveBeenCalled(); - }); - it('swallows errors (cancellation must never throw)', async () => { - const ctx = ctxWith(async () => { throw new Error('boom'); }); - await expect(killQuery(ctx, 'q', sqlString)).resolves.toBeUndefined(); - }); -}); +// Issue #630 Phase 7 (plan §23) — the generic, format-agnostic `runQuery`, +// `exportQuery`, and the ordinary mutable-context `killQuery` are DELETED +// from `ch-client.ts` (Checkpoint 2D): their coverage moved to the services +// that now own that policy — `QueryExecutionService` +// (tests/unit/query-execution-service.test.ts, Table/KPI/TSV/raw mapping, +// row caps, retry/result semantics) and `ExportService` +// (tests/unit/export-service.test.ts, direct/script export, raw-byte +// streaming, late-exception handling) — moved there in the Checkpoint 2A/2B +// migration (#630 Phase 7). `killQueryWithLease` below is retargeted onto +// the package's own stateless `createClickHouseHttpClient(...).killQuery(...)` +// and no longer takes a `sqlString` parameter — the package owns the KILL +// QUERY SQL and its quoting now. describe('killQueryWithLease', () => { const lease = ( @@ -729,9 +638,9 @@ describe('killQueryWithLease', () => { return { fetchMock, lease: lease(asFetch(fetchMock), authorization) }; }; - it('uses the exact frozen origin and complete Authorization header', async () => { + it('uses the exact frozen origin, queryId, and complete Authorization header, with exactly one fetch', async () => { const { fetchMock, lease: frozen } = leaseFetch(async () => jsonResp({ data: [] })); - await killQueryWithLease(frozen, 'scope-q', sqlString); + await killQueryWithLease(frozen, 'scope-q'); expect(fetchMock).toHaveBeenCalledTimes(1); const [url, init] = fetchMock.mock.calls[0]; expect(url).toBe('https://old-cluster.example:8443?default_format=JSON&enable_http_compression=1'); @@ -739,25 +648,38 @@ describe('killQueryWithLease', () => { expect(init.body).toBe("KILL QUERY WHERE query_id = 'scope-q' ASYNC"); }); + it('the package owns KILL QUERY quoting — an embedded single quote in the query id is escaped, never locally (#630 Phase 7: sqlString is no longer a killQueryWithLease parameter)', async () => { + const { fetchMock, lease: frozen } = leaseFetch(async () => jsonResp({ data: [] })); + await killQueryWithLease(frozen, "a'b"); + expect(fetchMock.mock.calls[0][1].body).toBe("KILL QUERY WHERE query_id = 'a''b' ASYNC"); + }); + it('treats the frozen Authorization value as opaque for OAuth too', async () => { const { fetchMock, lease: frozen } = leaseFetch( async () => jsonResp({ data: [] }), 'Bearer old-token', ); - await killQueryWithLease(frozen, 'q', sqlString); + await killQueryWithLease(frozen, 'q'); expect(fetchMock.mock.calls[0][1].headers).toEqual({ Authorization: 'Bearer old-token' }); }); it('does not retry and swallows cleanup transport failures', async () => { const { fetchMock, lease: frozen } = leaseFetch(async () => { throw new Error('offline'); }); - await expect(killQueryWithLease( - frozen, 'q', sqlString, - )).resolves.toBeUndefined(); + await expect(killQueryWithLease(frozen, 'q')).resolves.toBeUndefined(); expect(fetchMock).toHaveBeenCalledTimes(1); }); - it('no-ops without a query id', async () => { + it('swallows a non-2xx package ClickHouseError too (best-effort — no distinction from a native network failure)', async () => { + const { fetchMock, lease: frozen } = leaseFetch( + async () => textResp('Code: 999. DB::Exception: access denied', false, 403), + ); + await expect(killQueryWithLease(frozen, 'q')).resolves.toBeUndefined(); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it('no-ops without a query id (null or undefined) — no fetch at all', async () => { const { fetchMock, lease: frozen } = leaseFetch(async () => jsonResp({ data: [] })); - await killQueryWithLease(frozen, null, sqlString); + await killQueryWithLease(frozen, null); + await killQueryWithLease(frozen, undefined); expect(fetchMock).not.toHaveBeenCalled(); }); @@ -767,50 +689,15 @@ describe('killQueryWithLease', () => { const refresh = vi.fn(); // `AuthenticatedCancellationLease` has exactly 4 fields; this widens the // VALUE (not killQueryWithLease's declared parameter type) to prove the - // one-shot lease-scoped transport genuinely never calls these, rather + // one-shot lease-scoped client genuinely never calls these, rather // than merely never being GIVEN them. const leaseWithExtras = Object.freeze({ ...frozen, getToken, refresh }); - await killQueryWithLease(leaseWithExtras, 'scope-q', sqlString); + await killQueryWithLease(leaseWithExtras, 'scope-q'); expect(getToken).not.toHaveBeenCalled(); expect(refresh).not.toHaveBeenCalled(); }); }); -describe('exportQuery', () => { - it('sets query_id + default_format, passes the signal, and returns the raw Response', async () => { - const signal = new AbortController().signal; - const stream = streamResp(['a\tb\n1\tx\n']); - const ctx = ctxWith(async () => stream); - const resp = await exportQuery(ctx, 'SELECT 1 FORMAT TabSeparatedWithNames', { - queryId: 'export-abc', signal, format: 'TabSeparatedWithNames', - }); - expect(resp).toBe(stream); - const [url, init] = ctx.fetchMock.mock.calls[0]; - expect(url).toContain('default_format=TabSeparatedWithNames'); - expect(url).toContain('query_id=export-abc'); - expect(init.signal).toBe(signal); - expect(init.body).toBe('SELECT 1 FORMAT TabSeparatedWithNames'); - }); - it('defaults to TabSeparatedWithNames and omits query_id when absent', async () => { - const ctx = ctxWith(async () => streamResp(['x'])); - await exportQuery(ctx, 'SELECT 1'); - const url = ctx.fetchMock.mock.calls[0][0]; - expect(url).toContain('default_format=TabSeparatedWithNames'); - expect(url).not.toContain('query_id'); - }); - it('throws the parsed CH exception on a non-OK (pre-header) response', async () => { - const ctx = ctxWith(async () => textResp('{"exception":"DB::Exception: nope"}', false)); - await expect(exportQuery(ctx, 'SELECT 1', { format: 'CSV' })).rejects.toThrow('DB::Exception: nope'); - }); - it('forwards caller params (e.g. session_id) alongside query_id (#99: script export)', async () => { - const ctx = ctxWith(async () => streamResp(['x'])); - await exportQuery(ctx, 'SELECT 1', { queryId: 'export-abc', params: { session_id: 'sess-1' } }); - const url = ctx.fetchMock.mock.calls[0][0]; - expect(url).toContain('query_id=export-abc'); - expect(url).toContain('session_id=sess-1'); - }); -}); - describe('loadSchemaLineage', () => { it('fetches scoped system.tables + dictionaries and attaches EXPLAIN AST sources', async () => { const seen: string[] = []; diff --git a/tests/unit/clickhouse-http-package-policy.test.js b/tests/unit/clickhouse-http-package-policy.test.js index b7ea2f5d..0da71081 100644 --- a/tests/unit/clickhouse-http-package-policy.test.js +++ b/tests/unit/clickhouse-http-package-policy.test.js @@ -61,6 +61,10 @@ import { findPackageImportUsages, PHASE5_PACKAGE_LANGUAGE_EXPORTS, mightReferencePackage, + findRetiredTopLevelApiViolations, + PHASE7_RETIRED_TOP_LEVEL_NAMES, + PHASE7_DELETED_TRANSPORT_FILES, + mightReferenceRetiredTopLevelApi, } from '../../build/lib/check-legacy-owners.mjs'; const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), '../..'); @@ -260,6 +264,38 @@ function packageNameShapeViolations(dir, virtualFiles = []) { return found; } +// Issue #630 Phase 7 — retired top-level runQuery/exportQuery/ordinary- +// killQuery resurrection guard. Calls the SAME real-parser helper the +// production `check:arch` gate calls (`findRetiredTopLevelApiViolations`) — +// not independently reimplemented, same reason as the Phase 3/5 rules and +// Rule D above. Same memoization rationale as `deepImportViolations`/ +// `packageNameShapeViolations` above. +const realTreeRetiredApiCache = new Map(); +function retiredApiViolations(dir, virtualFiles = []) { + let cached = realTreeRetiredApiCache.get(dir); + if (!cached) { + cached = []; + for (const file of collectFiles(dir)) { + const relFile = relative(repoRoot, file).split(sep).join('/'); + const text = readFileSync(file, 'utf8'); + if (!mightReferenceRetiredTopLevelApi(text, PHASE7_RETIRED_TOP_LEVEL_NAMES)) continue; + for (const name of findRetiredTopLevelApiViolations(text, relFile, PHASE7_RETIRED_TOP_LEVEL_NAMES)) { + cached.push(`${relFile} → ${name}`); + } + } + realTreeRetiredApiCache.set(dir, cached); + } + const found = [...cached]; + for (const [rel, source] of virtualFiles) { + const relFile = relative(repoRoot, join(repoRoot, rel)).split(sep).join('/'); + if (!mightReferenceRetiredTopLevelApi(source, PHASE7_RETIRED_TOP_LEVEL_NAMES)) continue; + for (const name of findRetiredTopLevelApiViolations(source, relFile, PHASE7_RETIRED_TOP_LEVEL_NAMES)) { + found.push(`${relFile} → ${name}`); + } + } + return found; +} + describe('root workspace/manifest declares the clickhouse-http package (issue #630 Phase 2)', () => { const rootPkg = readJson(join(repoRoot, 'package.json')); const pkgPkg = readJson(join(PACKAGE_DIR, 'package.json')); @@ -360,11 +396,11 @@ describe('Rule C — SQL Browser source does not deep-import the package\'s own }); }); -// Pre-warm both real-tree caches once, in `beforeAll`, rather than letting -// whichever test happens to run first inside the two Rule D describe blocks -// below pay for it under the DEFAULT per-test timeout: the cache-filling -// pass spawns the real TypeScript-parser child process once per real file -// that matches the widened `mightReferencePackage` filter, and that one-time +// Pre-warm all three real-tree caches once, in `beforeAll`, rather than +// letting whichever test happens to run first inside the Rule D / Phase 7 +// describe blocks below pay for it under the DEFAULT per-test timeout: the +// cache-filling pass spawns the real TypeScript-parser child process once per +// real file that matches each check's own pre-filter, and that one-time // cost — comfortably under a few seconds on a normal dev machine — has // exceeded vitest's 5000ms per-test default under CI's more constrained // scheduling. Explicit longer timeout here, attributed to setup rather than @@ -372,6 +408,7 @@ describe('Rule C — SQL Browser source does not deep-import the package\'s own beforeAll(() => { deepImportViolations(join(repoRoot, 'src')); packageNameShapeViolations(join(repoRoot, 'src')); + retiredApiViolations(join(repoRoot, 'src')); }, 30000); describe('Rule D, deep-import half — the deep-import subpath form is forbidden everywhere under src/**', () => { @@ -707,12 +744,28 @@ const CONTRACT_OWNER = 'src/net/clickhouse-transport.types.ts'; const STREAM_OWNER = 'src/core/stream.ts'; describe('Phase 3 legacy-owner rule — the moved stream/exception primitives cannot regain their former owners', () => { - for (const file of PHASE3_LEGACY_OWNER_FILES) { - it(`the real ${file} carries none of its former declarations`, () => { - const text = readFileSync(join(repoRoot, file), 'utf8'); - expect(findLegacyOwnerViolations(text, file)).toEqual([]); - }); - } + // Issue #630 Phase 7 (plan §16/§2.6) — `PHASE3_LEGACY_OWNER_FILES` stays + // pinned to its historical three-file former-owner set UNCHANGED (asserted + // below, in the drift-bind describe block) even though two of those three + // files are now intentionally deleted: the constant describes former + // owners, not necessarily currently-existing files. Blindly `readFileSync`- + // ing all three (the pre-Phase-7 shape) would ENOENT the moment the first + // deleted file is read — replaced with explicit absence assertions for the + // two retired production files, plus a real read+clean-scan of the one + // survivor, `src/core/stream.ts`. Never "fixed" by reintroducing either + // deleted file (plan §29 rollback rule). + it(`${TRANSPORT_OWNER} is absent (issue #630 Phase 7 — deleted; the local compatibility transport moved wholly onto @altinity/clickhouse-http)`, () => { + expect(existsSync(join(repoRoot, TRANSPORT_OWNER))).toBe(false); + }); + + it(`${CONTRACT_OWNER} is absent (issue #630 Phase 7 — deleted alongside its implementation)`, () => { + expect(existsSync(join(repoRoot, CONTRACT_OWNER))).toBe(false); + }); + + it(`the real ${STREAM_OWNER} carries none of its former declarations`, () => { + const text = readFileSync(join(repoRoot, STREAM_OWNER), 'utf8'); + expect(findLegacyOwnerViolations(text, STREAM_OWNER)).toEqual([]); + }); it('flags a re-added streamLines forwarding-property wrapper in the transport adapter (sabotage probe, not written to disk)', () => { const probe = ` @@ -1025,6 +1078,36 @@ describe('build/check-boundaries.mjs still declares the Rules A-D this spec mirr expect(checkerSource).toMatch(/src\/core\/sql-spans\.ts/); expect(checkerSource).toMatch(/src\/core\/quoted-span\.ts/); }); + + // Issue #630 Phase 7 (Finding 2) — the checker must still wire the two + // deleted-transport-path guards and the top-level retired-API-name guard + // through the shared helper, not a reintroduced hand-rolled scanner. + it('declares the Phase 7 deleted-transport-path existence check for both retired files', () => { + expect(checkerSource).toMatch(/PHASE7_DELETED_TRANSPORT_FILES/); + expect(checkerSource).toMatch(/for \(const relFile of PHASE7_DELETED_TRANSPORT_FILES\)/); + }); + + it('delegates the Phase 7 retired-top-level-API rule to build/lib/check-legacy-owners.mjs', () => { + expect(checkerSource).toMatch(/from '\.\/lib\/check-legacy-owners\.mjs'/); + expect(checkerSource).toMatch(/findRetiredTopLevelApiViolations\(/); + expect(checkerSource).toMatch(/mightReferenceRetiredTopLevelApi\(/); + expect(checkerSource).toMatch(/PHASE7_RETIRED_TOP_LEVEL_NAMES/); + }); + + it('the shared helper still names the exact Phase 7 retired top-level API names and deleted transport files', () => { + expect([...PHASE7_RETIRED_TOP_LEVEL_NAMES]).toEqual([ + 'runQuery', + 'RunQueryOptions', + 'RunQueryResult', + 'exportQuery', + 'ExportQueryOptions', + 'killQuery', + ]); + expect([...PHASE7_DELETED_TRANSPORT_FILES]).toEqual([ + 'src/net/clickhouse-http-transport.ts', + 'src/net/clickhouse-transport.types.ts', + ]); + }); }); // Issue #630 Phase 5 — the moved implementation files must remain absent @@ -1096,3 +1179,120 @@ describe('Phase 5 killQuery-stopgap former-owner rule — packages/clickhouse-ht expect(findKillStopgapOwnerViolations(probe, 'packages/clickhouse-http/src/client.ts')).toEqual([]); }); }); + +// Issue #630 Phase 7 (Finding 2) — the two local compatibility transport +// files must remain absent; same path-existence mechanism as the Phase 5 +// deleted-implementation-file check above. +describe('the retired local compatibility transport files no longer exist under SQL Browser src/** (issue #630 Phase 7)', () => { + it.each([...PHASE7_DELETED_TRANSPORT_FILES])('%s does not exist', (relFile) => { + expect(existsSync(join(repoRoot, relFile))).toBe(false); + }); +}); + +// Issue #630 Phase 7 (Finding 2) — top-level resurrection guard for the +// retired generic runQuery/exportQuery/ordinary-killQuery APIs and their +// request/result types. Exercised through the SAME shared helper the +// production `check:arch` gate calls (`findRetiredTopLevelApiViolations`), a +// real TypeScript parse scoped to `sourceFile.statements` only (never a +// blanket identifier walk) — see that function's own doc comment in +// `build/lib/check-legacy-owners.mjs` for why this structurally cannot +// reject the frozen-lease cancellation path's legitimate +// `client.killQuery(...)` member call. +describe('Phase 7 retired-top-level-API rule — runQuery/exportQuery/ordinary killQuery cannot be resurrected', () => { + it('the real src/** tree declares none of the retired top-level names', () => { + expect(retiredApiViolations(join(repoRoot, 'src'))).toEqual([]); + }); + + it('the real src/net/ch-client.ts (killQueryWithLease\'s home) carries none of the retired declarations', () => { + const text = readFileSync(join(repoRoot, 'src/net/ch-client.ts'), 'utf8'); + expect(findRetiredTopLevelApiViolations(text, 'src/net/ch-client.ts')).toEqual([]); + }); + + it('flags a top-level function declaration named runQuery (sabotage probe, not written to disk)', () => { + const found = retiredApiViolations(join(repoRoot, 'src'), [ + ['src/net/__boundary_probe_630p7_runquery__.ts', + 'export async function runQuery(ctx, req) { return null; }\n'], + ]); + expect(found).toContain('src/net/__boundary_probe_630p7_runquery__.ts → runQuery'); + }); + + it('flags a top-level function declaration named exportQuery (sabotage probe, not written to disk)', () => { + const found = retiredApiViolations(join(repoRoot, 'src'), [ + ['src/net/__boundary_probe_630p7_exportquery__.ts', 'export function exportQuery() { return null; }\n'], + ]); + expect(found).toContain('src/net/__boundary_probe_630p7_exportquery__.ts → exportQuery'); + }); + + it('flags a top-level function declaration named killQuery — the ordinary mutable-context signature (sabotage probe, not written to disk)', () => { + const found = retiredApiViolations(join(repoRoot, 'src'), [ + ['src/net/__boundary_probe_630p7_killquery__.ts', + 'export async function killQuery(ctx, queryId, sqlStringFn) { return; }\n'], + ]); + expect(found).toContain('src/net/__boundary_probe_630p7_killquery__.ts → killQuery'); + }); + + it('flags top-level RunQueryOptions/RunQueryResult/ExportQueryOptions type declarations (sabotage probe, not written to disk)', () => { + const probe = [ + 'export interface RunQueryOptions { sql: string; }', + 'export interface RunQueryResult { rows: unknown[]; }', + 'export interface ExportQueryOptions { sql: string; }', + ].join('\n'); + const found = retiredApiViolations(join(repoRoot, 'src'), [ + ['src/net/__boundary_probe_630p7_types__.ts', probe], + ]); + expect(found).toEqual([ + 'src/net/__boundary_probe_630p7_types__.ts → RunQueryOptions', + 'src/net/__boundary_probe_630p7_types__.ts → RunQueryResult', + 'src/net/__boundary_probe_630p7_types__.ts → ExportQueryOptions', + ]); + }); + + it('flags a top-level const runQuery binding (sabotage probe, not written to disk)', () => { + const found = retiredApiViolations(join(repoRoot, 'src'), [ + ['src/net/__boundary_probe_630p7_const__.ts', 'export const runQuery = async (ctx, req) => null;\n'], + ]); + expect(found).toContain('src/net/__boundary_probe_630p7_const__.ts → runQuery'); + }); + + it('flags a forwarding re-export alias (export { foo as runQuery }) (sabotage probe, not written to disk)', () => { + const probe = 'function foo() { return null; }\nexport { foo as runQuery };\n'; + expect(findRetiredTopLevelApiViolations(probe, 'src/net/ch-client.ts')).toEqual(['runQuery']); + }); + + it('flags a forwarding import alias (import { foo as exportQuery }) (sabotage probe, not written to disk)', () => { + const probe = "import { foo as exportQuery } from './somewhere.js';\n"; + expect(findRetiredTopLevelApiViolations(probe, 'src/net/ch-client.ts')).toEqual(['exportQuery']); + }); + + // The exact carve-out the plan requires: a member-access call on the + // package's own stateless client (`client.killQuery(...)`, the frozen-lease + // cancellation path's real implementation) is a PropertyAccessExpression + // nested inside a function body — never a top-level statement — so it + // structurally cannot trip this check. No name-based exception needed. + it('does NOT flag the legitimate client.killQuery(...) member call inside frozen-lease cancellation', () => { + const probe = ` + export async function killQueryWithLease(lease, queryId) { + if (!queryId) return; + const client = createClickHouseHttpClient({ fetch: () => lease.fetch, origin: () => lease.origin }); + await client.killQuery({ queryId, authorization: lease.authorization }); + } + `; + expect(findRetiredTopLevelApiViolations(probe, 'src/net/ch-client.ts')).toEqual([]); + }); + + it('does not flag a comment merely narrating the deletion (comments are parser trivia, never AST nodes)', () => { + const probe = '// runQuery/exportQuery/killQuery were all deleted this phase.\nexport function foo() {}\n'; + expect(findRetiredTopLevelApiViolations(probe, 'src/net/ch-client.ts')).toEqual([]); + }); + + it('does not flag a nested local function/variable named runQuery inside a function body (declaration-scoped, not a blanket identifier walk)', () => { + const probe = ` + export function outer() { + function runQuery() { return null; } + const exportQuery = () => null; + return runQuery() ?? exportQuery(); + } + `; + expect(findRetiredTopLevelApiViolations(probe, 'src/net/ch-client.ts')).toEqual([]); + }); +}); diff --git a/tests/unit/clickhouse-http-transport.test.ts b/tests/unit/clickhouse-http-transport.test.ts deleted file mode 100644 index 91d4f277..00000000 --- a/tests/unit/clickhouse-http-transport.test.ts +++ /dev/null @@ -1,80 +0,0 @@ -import { describe, expect, it, vi } from 'vitest'; -import { createHttpTransport } from '../../src/net/clickhouse-http-transport.js'; -import { runTransportContractSuite } from './clickhouse-transport-contract.js'; - -// Issue #585 Phase 1 — direct spec for the moved current-HTTP transport -// implementation. Registers the shared contract suite once (this is the -// ONLY implementation Phase 1 registers — see the suite factory's header -// comment). -// -// Issue #630 Phase 2 — `chUrl`'s own exact-URL-shape suite moved (not -// duplicated) to tests/unit/clickhouse-http-package.test.ts, since `chUrl` -// itself moved to @altinity/clickhouse-http. This file keeps the -// COMPATIBILITY-ADAPTER-specific tests: `send()`'s exact request shape -// (still true through delegation) and its promise-settlement shape on a -// request-preparation failure. -// -// Issue #630 Phase 3 — the moved progress-line stream loop's own mechanics -// tests (the split-multi-byte-UTF-8/onLine-before-onChunk-ordering cases this -// file used to cover locally) moved to -// `tests/unit/clickhouse-http-progress-stream.test.ts`, directly against the -// package's own `streamLines` — this adapter no longer has a stream member -// at all. - -runTransportContractSuite('createHttpTransport', createHttpTransport); - -function deps(fetchImpl: (url: string, init: RequestInit) => Response | Promise, origin = 'https://ch.example') { - const fetchMock = vi.fn(fetchImpl); - return { fetchMock, deps: { fetch: () => fetchMock as unknown as typeof fetch, origin: () => origin } }; -} - -describe('createHttpTransport().send — exact request shape', () => { - it('builds the exact literal URL from origin/format/settings/params, POSTs the SQL body, and sends the complete Authorization header', async () => { - const { fetchMock, deps: d } = deps(() => new Response('ok')); - const transport = createHttpTransport(d); - await transport.send({ - sql: 'SELECT 1', - defaultFormat: 'JSONCompact', - settings: { wait_end_of_query: 1 }, - params: { param_id: '5', query_id: 'q1', session_id: 's1', role: 'analyst' }, - authorization: 'Bearer tok', - }); - const [url, init] = fetchMock.mock.calls[0] as [string, RequestInit]; - expect(url).toBe( - 'https://ch.example?default_format=JSONCompact&enable_http_compression=1' - + '&wait_end_of_query=1¶m_id=5&query_id=q1&session_id=s1&role=analyst', - ); - expect(init.method).toBe('POST'); - expect(init.body).toBe('SELECT 1'); - expect((init.headers as Record).Authorization).toBe('Bearer tok'); - }); - - it('threads the abort signal through to fetch', async () => { - const controller = new AbortController(); - const { fetchMock, deps: d } = deps(() => new Response('ok')); - const transport = createHttpTransport(d); - await transport.send({ sql: 'x', defaultFormat: 'JSON', authorization: 'Bearer t', signal: controller.signal }); - expect((fetchMock.mock.calls[0][1] as RequestInit).signal).toBe(controller.signal); - }); - - // Issue #630 Phase 2 — locks in the production compatibility adapter's - // settlement shape across the extraction: `send()` delegates to the - // package's async `request()`, so a request-preparation failure (chUrl - // throwing URIError on an unencodable value) must still surface as a - // REJECTED promise here too, never a synchronous throw out of `send()`. - it('rejects the returned promise with URIError on malformed URL data, without throwing synchronously and without invoking fetch', async () => { - const { fetchMock, deps: d } = deps(() => new Response('ok')); - const transport = createHttpTransport(d); - let result!: Promise; - expect(() => { - result = transport.send({ - sql: 'SELECT 1', - defaultFormat: 'JSON', - settings: { broken: '\uD800' }, - authorization: 'Bearer x', - }); - }).not.toThrow(); - await expect(result).rejects.toBeInstanceOf(URIError); - expect(fetchMock).not.toHaveBeenCalled(); - }); -}); diff --git a/tests/unit/clickhouse-transport-contract.ts b/tests/unit/clickhouse-transport-contract.ts index fa76792b..a76fee3c 100644 --- a/tests/unit/clickhouse-transport-contract.ts +++ b/tests/unit/clickhouse-transport-contract.ts @@ -25,20 +25,41 @@ // proof) rather than staying here — there is intentionally only one // production stream implementation now, so this shared suite has nothing // left to register it against. +// +// Issue #630 Phase 7 (plan §2.1/§15) — the local `ClickHouseTransport`/ +// `TransportDeps`/`TransportRequest` seam this suite used to import from +// `src/net/clickhouse-transport.types.ts` is deleted along with the rest of +// the local compatibility transport (Checkpoint 2D): that whole file is +// gone. This suite now retypes directly against the package's own PUBLIC +// request types (`ClickHouseHttpClientDeps`/`ClickHouseHttpRequest`) and a +// test-local minimal `send(request)` façade (`RequestSender`) rather than +// recreating the deleted production `ClickHouseTransport` abstraction — +// there is exactly one generic transport implementation left in the +// repository (the package's), and this suite proves it against that +// implementation's own request() (registered in +// `clickhouse-http-package.test.ts`), with nothing else left to register it +// against. import { describe, expect, it, vi } from 'vitest'; -import type { ClickHouseTransport, TransportDeps, TransportRequest } from '../../src/net/clickhouse-transport.types.js'; +import type { ClickHouseHttpClientDeps, ClickHouseHttpRequest } from '@altinity/clickhouse-http'; type FetchImpl = (url: string, init: RequestInit) => Response | Promise; type HeadersRecord = Record; -/** A concrete `ClickHouseTransport` implementation under test, built from a - * `TransportDeps` this factory constructs and controls (so cases can flip - * `setOrigin` mid-test — Adaptation A5 / sabotage case 2 — and inspect every - * call the implementation made to the stub fetch). */ -export type MakeTransport = (deps: TransportDeps) => ClickHouseTransport; +/** Test-local minimal send-only façade — deliberately NOT a recreation of + * the deleted production `ClickHouseTransport` interface, just the single + * member this suite actually needs to drive an implementation under test. */ +type RequestSender = { + send(request: ClickHouseHttpRequest): Promise; +}; + +/** Builds a concrete `RequestSender` under test from a + * `ClickHouseHttpClientDeps` this factory constructs and controls (so cases + * can flip `setOrigin` mid-test — Adaptation A5 / sabotage case 2 — and + * inspect every call the implementation made to the stub fetch). */ +export type MakeRequestSender = (deps: ClickHouseHttpClientDeps) => RequestSender; -function baseRequest(overrides: Partial = {}): TransportRequest { +function baseRequest(overrides: Partial = {}): ClickHouseHttpRequest { return { sql: 'SELECT 1', defaultFormat: 'JSON', @@ -47,7 +68,7 @@ function baseRequest(overrides: Partial = {}): TransportReques }; } -export function runTransportContractSuite(name: string, makeTransport: MakeTransport): void { +export function runTransportContractSuite(name: string, makeTransport: MakeRequestSender): void { describe(`ClickHouseTransport contract — ${name}`, () => { function harness(fetchImpl: FetchImpl) { const fetchMock = vi.fn(fetchImpl); diff --git a/tests/unit/connection-session.test.ts b/tests/unit/connection-session.test.ts index e0005070..77acb81b 100644 --- a/tests/unit/connection-session.test.ts +++ b/tests/unit/connection-session.test.ts @@ -1504,6 +1504,62 @@ describe('chCtx.onSignedOut', () => { }); }); +// ── captureCancellationLease (#630 Phase 7 p7-02: expectedEpoch fence) ────── + +describe('captureCancellationLease(expectedEpoch?)', () => { + it('captures the current epoch when no expected epoch is given', () => { + const { session } = setup({ storage: memStorage({ oauth_id_token: validToken }) }); + const lease = session.captureCancellationLease(); + expect(lease).toEqual({ + epoch: session.connection.value.epoch, + origin: session.chCtx.origin, + authorization: `Bearer ${validToken}`, + fetch: session.chCtx.fetch, + }); + expect(Object.isFrozen(lease)).toBe(true); + }); + + it('captures a lease frozen to the given, currently-matching expected epoch', () => { + const { session } = setup({ storage: memStorage({ oauth_id_token: validToken }) }); + const ownerEpoch = session.connection.value.epoch; + const lease = session.captureCancellationLease(ownerEpoch); + expect(lease).toEqual({ + epoch: ownerEpoch, + origin: session.chCtx.origin, + authorization: `Bearer ${validToken}`, + fetch: session.chCtx.fetch, + }); + }); + + it('rejects a mismatching (replacement) expected epoch', () => { + const { session } = setup({ storage: memStorage({ oauth_id_token: validToken }) }); + const replacementEpoch = session.connection.value.epoch + 1; + expect(session.captureCancellationLease(replacementEpoch)).toBeNull(); + }); + + it('reflects a same-epoch refreshed credential captured at cancel time', async () => { + const { session } = setup({ + storage: memStorage({ oauth_id_token: expiredToken, oauth_refresh_token: 'r0' }), + routes: [(url) => (url.endsWith('/token') + ? jsonResponse(200, { id_token: validToken, refresh_token: 'r1' }) + : null)], + }); + // The owner captures its epoch at registration/start, before the token + // has expired-and-refreshed underneath it. + const ownerEpoch = session.connection.value.epoch; + await expect(session.getToken()).resolves.toBe(validToken); + // A same-session refresh does not create a new epoch. + expect(session.connection.value.epoch).toBe(ownerEpoch); + const lease = session.captureCancellationLease(ownerEpoch); + expect(lease).toEqual({ + epoch: ownerEpoch, + origin: session.chCtx.origin, + authorization: `Bearer ${validToken}`, + fetch: session.chCtx.fetch, + }); + }); +}); + // ── ensureFreshToken ───────────────────────────────────────────────────────── describe('ensureFreshToken', () => { diff --git a/tests/unit/export-service.test.ts b/tests/unit/export-service.test.ts index 291c6660..ef42f9dc 100644 --- a/tests/unit/export-service.test.ts +++ b/tests/unit/export-service.test.ts @@ -1,18 +1,14 @@ import { describe, it, expect, vi } from 'vitest'; import type { Mock } from 'vitest'; import { signal } from '@preact/signals-core'; -// Issue #630 Phase 5 — sqlString now has one implementation, owned by the -// package; format.js no longer declares it. -import { sqlString } from '@altinity/clickhouse-http'; import { splitStatements } from '../../src/core/sql-split.js'; import { createExportService } from '../../src/application/export-service.js'; import type { - ExportServiceDeps, ExportStateSlice, ExportHooks, ExportSink, + ExportServiceDeps, ExportStateSlice, ExportHooks, ExportSink, ExportRequest, SignedOutCtx, FileHandleLike, DirectoryHandleLike, WritableFileStreamLike, } from '../../src/application/export-service.js'; import { newTabObj } from '../../src/state.js'; import type { QueryTab } from '../../src/state.js'; -import type { ChCtx, RunQueryResult } from '../../src/net/ch-client.js'; import type { PreparedSource, PreparedStatement } from '../../src/core/param-pipeline.js'; import type { WorkbenchParameterSession } from '../../src/application/workbench-parameter-session.js'; import { @@ -60,7 +56,7 @@ function preparedSource(over: Partial = {}): PreparedSource { // ── Streaming-response / File System Access fakes (ported from // app.test.ts's own identically-named helpers — see that file's header // comment on why these aren't a shared tests/helpers/ module: this service's -// tests mock `exportQuery`/`runQuery` directly rather than a `fetch` seam, so +// tests mock `exportResponse`/`runEffectText` directly rather than a `fetch` seam, so // only the Response/file-handle SHAPES are shared, not the fetch-routing // machinery). ────────────────────────────────────────────────────────────── @@ -81,7 +77,7 @@ interface FakeExportResponse { headers: { get(name: string): string | null }; bo function fakeExportResponse(opts: { body?: FakeBody | null; headers?: Record } = {}): FakeExportResponse { return { body: opts.body, headers: { get: (name) => (opts.headers && opts.headers[name]) ?? null } }; } -// `ExportServiceDeps.exportQuery`'s real signature returns a genuine DOM +// `ExportServiceDeps.exportResponse`'s real signature returns a genuine DOM // `Response`; a `{headers,body}`-only fake doesn't overlap enough of the real // interface for a direct `as Response` (same "object"-parameter bridge as // app.test.ts's own `asFetch`/`asWindow`). @@ -168,11 +164,15 @@ void asWritableLike; // ── Fakes for the service's own injected deps ─────────────────────────────── -function makeCh(): { exportQuery: Mock; runQuery: Mock; killQuery: Mock } { - const exportQuery = vi.fn(async () => asResponse(fakeExportResponse({ body: streamBody([]) }))); - const runQuery = vi.fn(async (): Promise => ({})); - const killQuery = vi.fn(async () => {}); - return { exportQuery, runQuery, killQuery }; +// #630 Phase 7 — `exportResponse`/`runEffectText` mirror +// `authenticatedResponse`/`authenticatedText`: package consumers now THROW +// instead of returning a generic `{error}` shape, so a failure fixture is +// `mockRejectedValue(new Error(...))`, never `mockResolvedValue({error})`. +function makeCh(): { exportResponse: Mock; runEffectText: Mock; cancel: Mock } { + const exportResponse = vi.fn(async () => asResponse(fakeExportResponse({ body: streamBody([]) }))); + const runEffectText = vi.fn(async (): Promise => ''); + const cancel = vi.fn(async () => {}); + return { exportResponse, runEffectText, cancel }; } function makeState(over: Partial = {}): ExportStateSlice { @@ -211,13 +211,20 @@ function makeSink(over: Partial = {}): ExportSink { }; } +// #630 Phase 7 — `ctx()` survives only as the narrow signed-out notifier; a +// couple of tests also read `.fetch`/`.origin` off it purely as convenient +// FIXTURE VALUES for a frozen `AuthenticatedCancellationLease`'s own +// `fetch`/`origin` fields (unrelated to transport — no export path reads +// `ctx()` for a request any more). +interface FakeCtx extends SignedOutCtx { fetch: typeof fetch; origin: string } + interface Harness { deps: ExportServiceDeps; state: ExportStateSlice; hooks: ExportHooks; sink: ExportSink; ch: ReturnType; - ctx: ChCtx; + ctx: FakeCtx; tab: QueryTab; params: ExportParamsDeps; } @@ -241,18 +248,17 @@ function makeHarness(opts: { const ch = makeCh(); const tab: QueryTab = { ...newTabObj('t1'), ...opts.tab }; const params = makeParams(opts.params); - const ctx: ChCtx = { + const ctx: FakeCtx = { fetch: (undefined as unknown) as typeof fetch, origin: 'https://ch.example', - getToken: async () => null, refresh: async () => false, onSignedOut: vi.fn(), + onSignedOut: vi.fn(), }; const uidSeq = { n: 0 }; const deps: ExportServiceDeps = { - exportQuery: ch.exportQuery, runQuery: ch.runQuery, killQuery: ch.killQuery, + exportResponse: ch.exportResponse, runEffectText: ch.runEffectText, cancel: ch.cancel, ctx: () => ctx, executionScope: opts.executionScope || (() => null), ensureConfig: opts.ensureConfig || vi.fn(async () => undefined), getToken: opts.getToken || vi.fn(async () => 'tok'), - sqlString, now: () => { uidSeq.n += 10; return uidSeq.n; }, wallNow: () => 1_700_000_000_000, uid: (prefix: string) => `${prefix}${++uidSeq.n}`, @@ -268,6 +274,16 @@ function makeHarness(opts: { return { deps, state, hooks, sink, ch, ctx, tab, params }; } +/** `h.ch.exportResponse`'s recorded request at call index `i` (default 0) — + * a small accessor so assertions read almost like the pre-Phase-7 + * `mock.calls[i][2]` options-object shape did. */ +function exportCall(h: Harness, i = 0): ExportRequest { + return (h.ch.exportResponse as Mock).mock.calls[i][0] as ExportRequest; +} +function effectCall(h: Harness, i = 0): ExportRequest { + return (h.ch.runEffectText as Mock).mock.calls[i][0] as ExportRequest; +} + // ── exportEntry (dispatch) ────────────────────────────────────────────────── describe('createExportService: exportEntry (dispatch)', () => { @@ -390,7 +406,7 @@ describe('createExportService: exportDirect (issue #87)', () => { await createExportService(h.deps).exportDirect('SELECT 1', 0); expect(h.sink.pickFile).toHaveBeenCalledTimes(1); expect(h.ctx.onSignedOut).toHaveBeenCalledTimes(1); - expect(h.ch.exportQuery).not.toHaveBeenCalled(); + expect(h.ch.exportResponse).not.toHaveBeenCalled(); expect(h.state.exporting.value).toBe(false); }); @@ -403,7 +419,7 @@ describe('createExportService: exportDirect (issue #87)', () => { }, tab: { name: 'My Query!' }, }); - h.ch.exportQuery.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody(['a'.repeat(100)]) }))); + h.ch.exportResponse.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody(['a'.repeat(100)]) }))); await createExportService(h.deps).exportDirect('SELECT 1', 0); expect(pickerOpts!.suggestedName).toBe('My_Query.tsv'); expect(pickerOpts!.types[0].accept).toEqual({ 'text/tab-separated-values': ['.tsv'] }); @@ -412,9 +428,9 @@ describe('createExportService: exportDirect (issue #87)', () => { expect(writable.abort).not.toHaveBeenCalled(); expect(h.hooks.toast).toHaveBeenCalledWith('Export complete'); expect(h.state.exporting.value).toBe(false); - const call = h.ch.exportQuery.mock.calls[0]; - expect(call[1]).toBe('SELECT 1\nFORMAT TabSeparatedWithNames'); - expect(call[2].format).toBe('TabSeparatedWithNames'); + const call = exportCall(h); + expect(call.sql).toBe('SELECT 1\nFORMAT TabSeparatedWithNames'); + expect(call.defaultFormat).toBe('TabSeparatedWithNames'); }); it('honors an explicit FORMAT in the query for the picker + the request', async () => { @@ -424,12 +440,12 @@ describe('createExportService: exportDirect (issue #87)', () => { sink: { pickFile: vi.fn(async (opts) => { pickerOpts = opts; return asFileHandleLike(handle); }) }, params: { execStatementSql: vi.fn((s: string) => s) }, }); - h.ch.exportQuery.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody(['[]']) }))); + h.ch.exportResponse.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody(['[]']) }))); await createExportService(h.deps).exportDirect('SELECT 1 FORMAT JSON', 0); expect(pickerOpts!.suggestedName).toMatch(/\.json$/); expect(pickerOpts!.types[0].accept).toEqual({ 'application/json': ['.json'] }); - const call = h.ch.exportQuery.mock.calls[0]; - expect(call[2].format).toBe('JSON'); + const call = exportCall(h); + expect(call.defaultFormat).toBe('JSON'); }); it('query variables (#134/#173): sends the wave-captured params merged with sessionParamsFor', async () => { @@ -441,27 +457,53 @@ describe('createExportService: exportDirect (issue #87)', () => { }, sessionParamsFor: vi.fn(() => ({ session_id: 'sess-1' })), }); - h.ch.exportQuery.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody(['x']) }))); + h.ch.exportResponse.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody(['x']) }))); await createExportService(h.deps).exportDirect('SELECT {database:String}', 42); expect(h.params.prepareTabSource).toHaveBeenCalledWith('SELECT {database:String}\nFORMAT TabSeparatedWithNames', 42); - const call = h.ch.exportQuery.mock.calls[0]; - expect(call[2].params).toEqual({ session_id: 'sess-1', param_database: 'default' }); + const call = exportCall(h); + // `params` now also carries the wave's own `query_id` (#630 Phase 7 — + // this service builds the whole request object itself); `toMatchObject` + // ignores that extra key rather than pinning its exact generated value. + expect(call.params).toMatchObject({ session_id: 'sess-1', param_database: 'default' }); }); - it('a pre-header (non-OK) export failure toasts "Export failed" without ever opening the writable', async () => { + // #630 Phase 7 §23 — "non-2xx never starts streaming": `exportResponse` + // mirrors `authenticatedResponse`'s package classification, so a non-2xx + // status is a REJECTION this service receives before it ever holds a + // `Response` to stream from — `streamToFile`/the writable/the reader are + // never reached. + it('a pre-header (non-OK) export failure toasts "Export failed" without ever opening the writable — non-2xx never starts streaming', async () => { const { handle } = fakeFileHandle(); const h = makeHarness({ sink: { pickFile: vi.fn(async () => asFileHandleLike(handle)) } }); - h.ch.exportQuery.mockRejectedValue(new Error('DB::Exception: nope')); + h.ch.exportResponse.mockRejectedValue(new Error('DB::Exception: nope')); await createExportService(h.deps).exportDirect('SELECT 1', 0); expect(h.hooks.toast).toHaveBeenCalledWith('Export failed: DB::Exception: nope'); expect(handle.createWritable).not.toHaveBeenCalled(); expect(h.state.exporting.value).toBe(false); }); + // #630 Phase 7 §12.3/§23 — the successful raw-export path must never call + // `.text()`/`.json()` on the successful `Response`: it stays untouched + // until `streamToFile`'s own `body.getReader()`. A `.text()` that would + // throw if ever invoked proves the byte-stream path really does bypass it. + it('a successful Response whose .text() throws still succeeds — the successful body is never read for classification', async () => { + const { handle, writable, chunks } = fakeFileHandle(); + const h = makeHarness({ sink: { pickFile: vi.fn(async () => asFileHandleLike(handle)) } }); + const resp = { + ...fakeExportResponse({ body: streamBody(['clean data']) }), + text: () => { throw new Error('must not be called on a successful export response'); }, + }; + h.ch.exportResponse.mockResolvedValue(asResponse(resp)); + await createExportService(h.deps).exportDirect('SELECT 1', 0); + expect(writtenText(chunks)).toBe('clean data'); + expect(writable.close).toHaveBeenCalledTimes(1); + expect(h.hooks.toast).toHaveBeenCalledWith('Export complete'); + }); + it('reports a non-Error export rejection', async () => { const { handle } = fakeFileHandle(); const h = makeHarness({ sink: { pickFile: vi.fn(async () => asFileHandleLike(handle)) } }); - h.ch.exportQuery.mockRejectedValue('transport unavailable'); + h.ch.exportResponse.mockRejectedValue('transport unavailable'); await createExportService(h.deps).exportDirect('SELECT 1', 0); expect(h.hooks.toast).toHaveBeenCalledWith('Export failed: transport unavailable'); }); @@ -469,7 +511,7 @@ describe('createExportService: exportDirect (issue #87)', () => { it('suppresses the "Export failed" toast when the underlying error is "signed out"', async () => { const { handle } = fakeFileHandle(); const h = makeHarness({ sink: { pickFile: vi.fn(async () => asFileHandleLike(handle)) } }); - h.ch.exportQuery.mockRejectedValue(new Error('signed out')); + h.ch.exportResponse.mockRejectedValue(new Error('signed out')); await createExportService(h.deps).exportDirect('SELECT 1', 0); expect(h.hooks.toast).not.toHaveBeenCalled(); expect(h.state.exporting.value).toBe(false); @@ -479,7 +521,7 @@ describe('createExportService: exportDirect (issue #87)', () => { const { handle, writable, chunks } = fakeFileHandle(); const big = 'a'.repeat(40960); // > HOLDBACK (32 KiB) in a single chunk const h = makeHarness({ sink: { pickFile: vi.fn(async () => asFileHandleLike(handle)) } }); - h.ch.exportQuery.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody([big]) }))); + h.ch.exportResponse.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody([big]) }))); await createExportService(h.deps).exportDirect('SELECT 1', 0); // mid-loop commit (8192 = 40960 - 32768 HOLDBACK) then the EOF flush of the held-back tail. expect((writable.write as Mock).mock.calls.map((c) => (c[0] as Uint8Array).length)).toEqual([8192, 32768]); @@ -493,7 +535,7 @@ describe('createExportService: exportDirect (issue #87)', () => { const clean = 'x'.repeat(40); const frame = exceptionFrame(TAG, 'DB::Exception: Memory limit (total) exceeded'); const h = makeHarness({ sink: { pickFile: vi.fn(async () => asFileHandleLike(handle)) } }); - h.ch.exportQuery.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody([clean, frame]), headers: { 'X-ClickHouse-Exception-Tag': TAG } }))); + h.ch.exportResponse.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody([clean, frame]), headers: { 'X-ClickHouse-Exception-Tag': TAG } }))); await createExportService(h.deps).exportDirect('SELECT 1', 0); expect(writtenText(chunks)).toBe(clean); expect(writable.close).toHaveBeenCalledTimes(1); @@ -517,7 +559,7 @@ describe('createExportService: exportDirect (issue #87)', () => { ); const frameBytes = exceptionFrameBytes(TAG, 'DB::Exception: Memory limit (total) exceeded'); const h = makeHarness({ sink: { pickFile: vi.fn(async () => asFileHandleLike(handle)) } }); - h.ch.exportQuery.mockResolvedValue(asResponse(fakeExportResponse({ + h.ch.exportResponse.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBodyBytes([cleanBytes, frameBytes]), headers: { 'X-ClickHouse-Exception-Tag': TAG }, }))); @@ -539,7 +581,7 @@ describe('createExportService: exportDirect (issue #87)', () => { const { handle, writable, chunks } = fakeFileHandle(); const data = 'note\t__exception__ mentioned in this row, not a real frame\n'; const h = makeHarness({ sink: { pickFile: vi.fn(async () => asFileHandleLike(handle)) } }); - h.ch.exportQuery.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody([data]) }))); + h.ch.exportResponse.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody([data]) }))); await createExportService(h.deps).exportDirect('SELECT 1', 0); expect(writtenText(chunks)).toBe(data); expect(writable.close).toHaveBeenCalledTimes(1); @@ -560,7 +602,7 @@ describe('createExportService: exportDirect (issue #87)', () => { }), }; const h = makeHarness({ sink: { pickFile: vi.fn(async () => asFileHandleLike(handle)) } }); - h.ch.exportQuery.mockResolvedValue(asResponse(fakeExportResponse({ body }))); + h.ch.exportResponse.mockResolvedValue(asResponse(fakeExportResponse({ body }))); await createExportService(h.deps).exportDirect('SELECT 1', 0); expect(writable.abort).not.toHaveBeenCalled(); expect(writable.close).toHaveBeenCalledTimes(1); @@ -583,7 +625,7 @@ describe('createExportService: exportDirect (issue #87)', () => { }), }; const h = makeHarness({ sink: { pickFile: vi.fn(async () => asFileHandleLike(handle)) } }); - h.ch.exportQuery.mockResolvedValue(asResponse(fakeExportResponse({ body }))); + h.ch.exportResponse.mockResolvedValue(asResponse(fakeExportResponse({ body }))); service = createExportService(h.deps); await service.exportDirect('SELECT 1', 0); expect(writable.close).toHaveBeenCalled(); @@ -595,7 +637,7 @@ describe('createExportService: exportDirect (issue #87)', () => { const { handle, writable } = fakeFileHandle(); delete handle.move; const h = makeHarness({ sink: { pickFile: vi.fn(async () => asFileHandleLike(handle)) } }); - h.ch.exportQuery.mockResolvedValue(asResponse(fakeExportResponse({ body: throwingBody('network drop') }))); + h.ch.exportResponse.mockResolvedValue(asResponse(fakeExportResponse({ body: throwingBody('network drop') }))); await createExportService(h.deps).exportDirect('SELECT 1', 0); expect(writable.abort).not.toHaveBeenCalled(); expect(writable.close).toHaveBeenCalledTimes(1); @@ -606,23 +648,23 @@ describe('createExportService: exportDirect (issue #87)', () => { const { handle, writable } = fakeFileHandle(); handle.move = vi.fn(async () => { throw new Error('collision'); }); const h = makeHarness({ sink: { pickFile: vi.fn(async () => asFileHandleLike(handle)) } }); - h.ch.exportQuery.mockResolvedValue(asResponse(fakeExportResponse({ body: throwingBody('network drop') }))); + h.ch.exportResponse.mockResolvedValue(asResponse(fakeExportResponse({ body: throwingBody('network drop') }))); await createExportService(h.deps).exportDirect('SELECT 1', 0); expect(writable.abort).not.toHaveBeenCalled(); expect(handle.move).toHaveBeenCalledTimes(1); expect(h.hooks.toast).toHaveBeenCalledWith('Export failed: network drop'); }); - it('exporting.value is true for the duration of the run; cancelExport aborts the signal + issues its own KILL QUERY', async () => { + it('exporting.value is true for the duration of the run; cancelExport aborts the signal + issues its own owner-scoped KILL QUERY', async () => { const { handle } = fakeFileHandle(); const pending = deferred(); const h = makeHarness({ sink: { pickFile: vi.fn(async () => asFileHandleLike(handle)) } }); - h.ch.exportQuery.mockImplementation(async () => pending.promise); + h.ch.exportResponse.mockImplementation(async () => pending.promise); const service = createExportService(h.deps); const run = service.exportDirect('SELECT 1', 0); await flush(); expect(h.state.exporting.value).toBe(true); - const signalArg = h.ch.exportQuery.mock.calls[0][2].signal as AbortSignal; + const signalArg = exportCall(h).signal as AbortSignal; expect(signalArg.aborted).toBe(false); service.cancelExport(); @@ -632,7 +674,36 @@ describe('createExportService: exportDirect (issue #87)', () => { expect(h.state.exporting.value).toBe(false); expect(h.hooks.toast).not.toHaveBeenCalled(); // AbortError → silent - expect(h.ch.killQuery).toHaveBeenCalledWith(h.ctx, expect.stringMatching(/^export-/), sqlString); + // No executionScope supplied by this harness (defaults to `() => null`), + // so the owner epoch captured at wave start is null. + expect(h.ch.cancel).toHaveBeenCalledWith(null, expect.stringMatching(/^export-/)); + }); + + // #630 Phase 7 §9.3/9.5/§23 "owner-epoch cancel matrix" — cancelExport + // must pass the operation-owner epoch (the scope's `.epoch` at wave + // start), never a hardcoded/omitted value, and local abort must happen + // BEFORE the remote cancel call. + it('cancelExport passes the wave-start execution scope epoch to deps.cancel, local abort before remote kill', async () => { + const { handle } = fakeFileHandle(); + const pending = deferred(); + const order: string[] = []; + const h = makeHarness({ + executionScope: () => scopeWithChecks(Array(20).fill(true)), + sink: { pickFile: vi.fn(async () => asFileHandleLike(handle)) }, + }); + (h.ch.cancel as Mock).mockImplementation(async () => { order.push('remote'); }); + h.ch.exportResponse.mockImplementation(async () => pending.promise); + const service = createExportService(h.deps); + const run = service.exportDirect('SELECT 1', 0); + await flush(); + const signalArg = exportCall(h).signal as AbortSignal; + signalArg.addEventListener('abort', () => order.push('local-abort')); + service.cancelExport(); + pending.reject(abortError()); + await run; + // `scopeWithChecks`'s fixed epoch is 1 (see its own definition below). + expect(h.ch.cancel).toHaveBeenCalledWith(1, expect.stringMatching(/^export-/)); + expect(order).toEqual(['local-abort', 'remote']); }); it('a second click while the picker is still open is blocked (exporting flips true before the picker await)', async () => { @@ -656,7 +727,7 @@ describe('createExportService: exportDirect (issue #87)', () => { sink: { pickFile: vi.fn(async () => asFileHandleLike(handle)) }, hooks: { showExportProgress: vi.fn(() => progress) }, }); - h.ch.exportQuery.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody(['a'.repeat(50)]) }))); + h.ch.exportResponse.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody(['a'.repeat(50)]) }))); await createExportService(h.deps).exportDirect('SELECT 1', 0); expect(h.hooks.showExportProgress).toHaveBeenCalledTimes(1); expect(progress.update).toHaveBeenCalled(); @@ -725,12 +796,12 @@ describe('createExportService: authenticated execution scope', () => { it('fences stale direct-export progress and final completion independently', async () => { const many = 'x'.repeat(33 * 1024); const progress = makeHarness({ executionScope: () => scopeWithChecks([true, true, true, true, true, true, false, true]) }); - progress.ch.exportQuery.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody([many]) }))); + progress.ch.exportResponse.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody([many]) }))); await createExportService(progress.deps).exportDirect('SELECT 1', 0); expect(progress.hooks.toast).not.toHaveBeenCalled(); const final = makeHarness({ executionScope: () => scopeWithChecks([true, true, true, true, true, true, true, false]) }); - final.ch.exportQuery.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody([many]) }))); + final.ch.exportResponse.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody([many]) }))); await createExportService(final.deps).exportDirect('SELECT 1', 0); expect(final.hooks.toast).not.toHaveBeenCalled(); }); @@ -755,33 +826,33 @@ describe('createExportService: authenticated execution scope', () => { executionScope: () => scopeWithChecks([true, true, true, true, true, true, true, true, false]), tab: { sqlDraft: 'CREATE TABLE t (x Int8); SELECT 1' }, }); - failedEffect.ch.runQuery.mockRejectedValue(new Error('late failure')); + failedEffect.ch.runEffectText.mockRejectedValue(new Error('late failure')); await createExportService(failedEffect.deps).exportEntry(); expect(failedEffect.hooks.renderResults).toHaveBeenCalled(); }); it('stops effect-script settlement and final bookkeeping when its owning scope closes', async () => { - const afterTransport = deferred(); + const afterTransport = deferred(); const transportScope = executionScope(); const transport = makeHarness({ executionScope: () => transportScope, tab: { sqlDraft: 'CREATE TABLE t (x Int8); SELECT 1' }, }); - transport.ch.runQuery.mockImplementation(() => afterTransport.promise); + transport.ch.runEffectText.mockImplementation(() => afterTransport.promise); const pending = createExportService(transport.deps).exportEntry(); await flush(); transportScope.close(); - afterTransport.resolve({}); + afterTransport.resolve(''); await pending; expect(transport.hooks.loadSchema).not.toHaveBeenCalled(); - const failedTransport = deferred(); + const failedTransport = deferred(); const failedScope = executionScope(); const failed = makeHarness({ executionScope: () => failedScope, tab: { sqlDraft: 'CREATE TABLE t (x Int8); SELECT 1' }, }); - failed.ch.runQuery.mockImplementation(() => failedTransport.promise); + failed.ch.runEffectText.mockImplementation(() => failedTransport.promise); const failedPending = createExportService(failed.deps).exportEntry(); await flush(); failedScope.close(); @@ -818,7 +889,7 @@ describe('createExportService: authenticated execution scope', () => { executionScope: () => scope, sink: { pickFile: vi.fn(async () => asFileHandleLike(handle)) }, }); - h.ch.exportQuery.mockResolvedValue(asResponse(fakeExportResponse({ body }))); + h.ch.exportResponse.mockResolvedValue(asResponse(fakeExportResponse({ body }))); await createExportService(h.deps).exportDirect('SELECT 1', 0); expect(writable.close).toHaveBeenCalled(); }); @@ -838,7 +909,7 @@ describe('createExportService: authenticated execution scope', () => { expect(h.deps.ensureConfig).not.toHaveBeenCalled(); expect(h.deps.getToken).not.toHaveBeenCalled(); - expect(h.ch.exportQuery).not.toHaveBeenCalled(); + expect(h.ch.exportResponse).not.toHaveBeenCalled(); expect(h.hooks.toast).not.toHaveBeenCalled(); }); @@ -853,10 +924,10 @@ describe('createExportService: authenticated execution scope', () => { sink: { pickFile: vi.fn(async () => asFileHandleLike(handle)) }, hooks: { showExportProgress: vi.fn(() => progress) }, }); - h.ch.exportQuery.mockImplementation(async () => pending.promise); + h.ch.exportResponse.mockImplementation(async () => pending.promise); const run = createExportService(h.deps).exportDirect('SELECT 1', 0); await flush(); - const queryId = h.ch.exportQuery.mock.calls[0][2].queryId as string; + const queryId = exportCall(h).params!.query_id as string; scope.close({ epoch: 1, origin: 'https://ch.example', authorization: 'Bearer old', fetch: h.ctx.fetch }); expect(h.state.exporting.value).toBe(false); @@ -881,7 +952,7 @@ describe('createExportService: authenticated execution scope', () => { executionScope: () => scope, sink: { pickFile: vi.fn(async () => asFileHandleLike(handle)) }, }); - h.ch.exportQuery.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody(['late']) }))); + h.ch.exportResponse.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody(['late']) }))); const run = createExportService(h.deps).exportDirect('SELECT 1', 0); await flush(); scope.close(); @@ -908,8 +979,8 @@ describe('createExportService: authenticated execution scope', () => { const { handle } = fakeFileHandle(); (h.sink.pickFile as Mock).mockResolvedValueOnce(asFileHandleLike(handle)); await service.exportDirect('SELECT 2', 0); - expect(h.ch.exportQuery).toHaveBeenCalledTimes(1); - expect(h.ch.exportQuery.mock.calls[0][1]).toContain('SELECT 2'); + expect(h.ch.exportResponse).toHaveBeenCalledTimes(1); + expect(exportCall(h).sql).toContain('SELECT 2'); expect(h.state.exporting.value).toBe(false); }); @@ -923,10 +994,10 @@ describe('createExportService: authenticated execution scope', () => { tab: { sqlDraft: 'SELECT 1; SELECT 2' }, sink: { pickDirectory: vi.fn(async () => dir) }, }); - h.ch.exportQuery.mockImplementation(async () => pending.promise); + h.ch.exportResponse.mockImplementation(async () => pending.promise); const run = createExportService(h.deps).exportEntry(); await flush(); - const queryId = h.ch.exportQuery.mock.calls[0][2].queryId as string; + const queryId = exportCall(h).params!.query_id as string; const rendersBeforeClose = (h.hooks.renderResults as Mock).mock.calls.length; scope.close({ epoch: 1, origin: 'https://ch.example', authorization: 'Bearer old', fetch: h.ctx.fetch }); @@ -936,7 +1007,7 @@ describe('createExportService: authenticated execution scope', () => { await run; expect((h.hooks.renderResults as Mock).mock.calls.length).toBe(rendersBeforeClose); - expect(h.ch.exportQuery).toHaveBeenCalledTimes(1); + expect(h.ch.exportResponse).toHaveBeenCalledTimes(1); }); it('stops a script export in preflight and never lets the late directory picker start transport', async () => { @@ -955,8 +1026,8 @@ describe('createExportService: authenticated execution scope', () => { await run; expect(h.deps.ensureConfig).not.toHaveBeenCalled(); - expect(h.ch.exportQuery).not.toHaveBeenCalled(); - expect(h.ch.runQuery).not.toHaveBeenCalled(); + expect(h.ch.exportResponse).not.toHaveBeenCalled(); + expect(h.ch.runEffectText).not.toHaveBeenCalled(); expect(h.hooks.renderResults).not.toHaveBeenCalled(); }); }); @@ -1038,7 +1109,7 @@ describe('createExportService: exportScriptEntry / exportScript (issue #99)', () sink: { pickDirectory: vi.fn(async () => dir) }, tab: { sqlDraft: 'SELECT 1; SELECT 2' }, }); - h.ch.exportQuery.mockImplementationOnce(async () => pending.promise); + h.ch.exportResponse.mockImplementationOnce(async () => pending.promise); const run = createExportService(h.deps).exportEntry(); await vi.advanceTimersByTimeAsync(200); expect(h.hooks.renderResults).toHaveBeenCalled(); @@ -1067,17 +1138,20 @@ describe('createExportService: exportScriptEntry / exportScript (issue #99)', () }, sessionParamsFor: vi.fn(() => ({ session_id: 'sess-xyz' })), }); - h.ch.exportQuery.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody(['1\n']) }))); + h.ch.exportResponse.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody(['1\n']) }))); await createExportService(h.deps).exportEntry(); - // Effect statements (non-'rows') go through runQuery with format TSV. - expect(h.ch.runQuery).toHaveBeenCalledTimes(2); - const runCalls = h.ch.runQuery.mock.calls; - expect(runCalls[0][1]).toBe('CREATE TEMPORARY TABLE t (a Int8)'); - expect(runCalls[1][1]).toBe('INSERT INTO t VALUES (1)'); - runCalls.forEach((c) => expect(c[2].params).toMatchObject({ session_id: 'sess-xyz' })); - // Row-returning statement streams via exportQuery, one file. - expect(h.ch.exportQuery).toHaveBeenCalledTimes(1); + // Effect statements (non-'rows') go through runEffectText, whole-body + // TabSeparatedWithNamesAndTypes text, wait_end_of_query=1 + CORS (#630 + // Phase 7 §13). + expect(h.ch.runEffectText).toHaveBeenCalledTimes(2); + expect(effectCall(h, 0).sql).toBe('CREATE TEMPORARY TABLE t (a Int8)'); + expect(effectCall(h, 0).defaultFormat).toBe('TabSeparatedWithNamesAndTypes'); + expect(effectCall(h, 0).settings).toEqual({ wait_end_of_query: 1, add_http_cors_header: 1 }); + expect(effectCall(h, 1).sql).toBe('INSERT INTO t VALUES (1)'); + [effectCall(h, 0), effectCall(h, 1)].forEach((c) => expect(c.params).toMatchObject({ session_id: 'sess-xyz' })); + // Row-returning statement streams via exportResponse, one file. + expect(h.ch.exportResponse).toHaveBeenCalledTimes(1); expect((dir.getFileHandle as Mock)).toHaveBeenCalledTimes(1); const [name] = (dir.getFileHandle as Mock).mock.calls[0]; expect(name).toBe('003-t.tsv'); @@ -1091,7 +1165,7 @@ describe('createExportService: exportScriptEntry / exportScript (issue #99)', () sink: { pickDirectory: vi.fn(async () => dir) }, tab: { sqlDraft: 'SELECT 1;\nSELECT 2;' }, }); - h.ch.exportQuery + h.ch.exportResponse .mockResolvedValueOnce(asResponse(fakeExportResponse({ body: streamBody(['a']) }))) .mockResolvedValueOnce(asResponse(fakeExportResponse({ body: streamBody(['b']) }))); await createExportService(h.deps).exportEntry(); @@ -1106,7 +1180,7 @@ describe('createExportService: exportScriptEntry / exportScript (issue #99)', () tab: { sqlDraft: 'SELECT 1 FORMAT JSON;\nSELECT 2;' }, params: { execStatementSql: vi.fn((s: string) => s) }, }); - h.ch.exportQuery + h.ch.exportResponse .mockResolvedValueOnce(asResponse(fakeExportResponse({ body: streamBody(['[]']) }))) .mockResolvedValueOnce(asResponse(fakeExportResponse({ body: streamBody(['x']) }))); await createExportService(h.deps).exportEntry(); @@ -1120,7 +1194,7 @@ describe('createExportService: exportScriptEntry / exportScript (issue #99)', () sink: { pickDirectory: vi.fn(async () => dir) }, tab: { sqlDraft: 'CREATE TABLE bad;\nSELECT 1;' }, }); - h.ch.runQuery.mockResolvedValue({ error: 'DB::Exception: table exists' }); + h.ch.runEffectText.mockRejectedValue(new Error('DB::Exception: table exists')); await createExportService(h.deps).exportEntry(); expect((dir.getFileHandle as Mock)).not.toHaveBeenCalled(); expect(h.hooks.loadSchema).not.toHaveBeenCalled(); @@ -1132,9 +1206,9 @@ describe('createExportService: exportScriptEntry / exportScript (issue #99)', () sink: { pickDirectory: vi.fn(async () => dir) }, tab: { sqlDraft: 'SELECT 1;\nSELECT 2;' }, }); - h.ch.exportQuery.mockRejectedValue(new Error('DB::Exception: nope')); + h.ch.exportResponse.mockRejectedValue(new Error('DB::Exception: nope')); await createExportService(h.deps).exportEntry(); - expect(h.ch.exportQuery).toHaveBeenCalledTimes(1); // stopped before statement 2 + expect(h.ch.exportResponse).toHaveBeenCalledTimes(1); // stopped before statement 2 }); it('a mid-stream exception marks the row failed/incomplete and stops the script', async () => { @@ -1146,9 +1220,9 @@ describe('createExportService: exportScriptEntry / exportScript (issue #99)', () sink: { pickDirectory: vi.fn(async () => dir) }, tab: { sqlDraft: 'SELECT 1;\nSELECT 2;' }, }); - h.ch.exportQuery.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody([clean, frame]), headers: { 'X-ClickHouse-Exception-Tag': TAG } }))); + h.ch.exportResponse.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody([clean, frame]), headers: { 'X-ClickHouse-Exception-Tag': TAG } }))); await createExportService(h.deps).exportEntry(); - expect(h.ch.exportQuery).toHaveBeenCalledTimes(1); // stopped before statement 2 + expect(h.ch.exportResponse).toHaveBeenCalledTimes(1); // stopped before statement 2 }); it('never retries — a transient SESSION_IS_LOCKED failure is reported like any other error', async () => { @@ -1157,10 +1231,10 @@ describe('createExportService: exportScriptEntry / exportScript (issue #99)', () sink: { pickDirectory: vi.fn(async () => dir) }, tab: { sqlDraft: 'INSERT INTO t VALUES (1);\nSELECT 1;' }, }); - h.ch.runQuery.mockResolvedValue({ error: 'Code: 373. DB::Exception: SESSION_IS_LOCKED' }); + h.ch.runEffectText.mockRejectedValue(new Error('Code: 373. DB::Exception: SESSION_IS_LOCKED')); await createExportService(h.deps).exportEntry(); - expect(h.ch.runQuery).toHaveBeenCalledTimes(1); // no retry - expect(h.ch.exportQuery).not.toHaveBeenCalled(); // stopped before the SELECT + expect(h.ch.runEffectText).toHaveBeenCalledTimes(1); // no retry + expect(h.ch.exportResponse).not.toHaveBeenCalled(); // stopped before the SELECT }); it('cancelExportScript aborts the active row, marks it cancelled, skips the rest, kills the active query, keeps completed files', async () => { @@ -1170,7 +1244,7 @@ describe('createExportService: exportScriptEntry / exportScript (issue #99)', () sink: { pickDirectory: vi.fn(async () => dir) }, tab: { sqlDraft: 'SELECT 1;\nSELECT 2;\nSELECT 3;' }, }); - h.ch.exportQuery + h.ch.exportResponse .mockResolvedValueOnce(asResponse(fakeExportResponse({ body: streamBody(['a']) }))) .mockImplementationOnce(async () => pending.promise); const service = createExportService(h.deps); @@ -1183,25 +1257,47 @@ describe('createExportService: exportScriptEntry / exportScript (issue #99)', () await run; expect(written.get('001-select-1.tsv')!.writable.close).toHaveBeenCalledTimes(1); // completed file kept - expect(h.ch.killQuery).toHaveBeenCalledWith(h.ctx, expect.stringMatching(/^export-/), sqlString); + expect(h.ch.cancel).toHaveBeenCalledWith(null, expect.stringMatching(/^export-/)); expect(h.state.exporting.value).toBe(false); }); + // #630 Phase 7 §9.3/9.5/§23 "owner-epoch cancel matrix" — the script-export + // path's own owner epoch (captured once, at wave start) reaches + // cancelExportScript's remote kill, exactly like cancelExport's. + it('cancelExportScript passes the wave-start execution scope epoch to deps.cancel', async () => { + const { dir } = fakeDirHandle(); + const pending = deferred(); + const h = makeHarness({ + executionScope: () => scopeWithChecks(Array(30).fill(true)), + sink: { pickDirectory: vi.fn(async () => dir) }, + tab: { sqlDraft: 'SELECT 1;\nSELECT 2;' }, + }); + h.ch.exportResponse.mockImplementation(async () => pending.promise); + const service = createExportService(h.deps); + const run = service.exportEntry(); + await flush(); + service.cancelExportScript(); + pending.reject(abortError()); + await run; + // `scopeWithChecks`'s fixed epoch is 1 (see its own definition above). + expect(h.ch.cancel).toHaveBeenCalledWith(1, expect.stringMatching(/^export-/)); + }); + it('a cancel that arrives just after a statement completed cleanly still skips the remaining statements', async () => { const { dir } = fakeDirHandle(); - const pending = deferred(); + const pending = deferred(); const h = makeHarness({ sink: { pickDirectory: vi.fn(async () => dir) }, tab: { sqlDraft: 'CREATE TABLE t (a Int8);\nSELECT 1;' }, }); - h.ch.runQuery.mockImplementationOnce(async () => pending.promise); + h.ch.runEffectText.mockImplementationOnce(async () => pending.promise); const service = createExportService(h.deps); const run = service.exportEntry(); await flush(); service.cancelExportScript(); // cancel arrives while stmt1 is still in flight... - pending.resolve({}); // ...but the request completes cleanly anyway + pending.resolve(''); // ...but the request completes cleanly anyway await run; - expect(h.ch.exportQuery).not.toHaveBeenCalled(); // stmt2 was skipped, not run + expect(h.ch.exportResponse).not.toHaveBeenCalled(); // stmt2 was skipped, not run }); it('refreshes the schema when an effect statement that actually ran is schema-mutating', async () => { @@ -1210,7 +1306,7 @@ describe('createExportService: exportScriptEntry / exportScript (issue #99)', () sink: { pickDirectory: vi.fn(async () => dir) }, tab: { sqlDraft: 'CREATE TABLE t (a Int8);\nSELECT 1;' }, }); - h.ch.exportQuery.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody(['x']) }))); + h.ch.exportResponse.mockResolvedValue(asResponse(fakeExportResponse({ body: streamBody(['x']) }))); await createExportService(h.deps).exportEntry(); expect(h.hooks.loadSchema).toHaveBeenCalledTimes(1); }); @@ -1221,7 +1317,7 @@ describe('createExportService: exportScriptEntry / exportScript (issue #99)', () sink: { pickDirectory: vi.fn(async () => dir) }, tab: { sqlDraft: 'SELECT 1;\nSELECT 2;' }, }); - h.ch.exportQuery + h.ch.exportResponse .mockResolvedValueOnce(asResponse(fakeExportResponse({ body: streamBody(['x']) }))) .mockResolvedValueOnce(asResponse(fakeExportResponse({ body: streamBody(['y']) }))); await createExportService(h.deps).exportEntry(); @@ -1234,7 +1330,7 @@ describe('createExportService: exportScriptEntry / exportScript (issue #99)', () sink: { pickDirectory: vi.fn(async () => dir) }, tab: { sqlDraft: 'SELECT 1;\nSELECT 2;' }, }); - h.ch.exportQuery + h.ch.exportResponse .mockResolvedValueOnce(asResponse(fakeExportResponse({ body: streamBody(['x']) }))) .mockResolvedValueOnce(asResponse(fakeExportResponse({ body: streamBody(['y']) }))); await createExportService(h.deps).exportEntry(); diff --git a/tests/unit/query-execution-service.test.ts b/tests/unit/query-execution-service.test.ts index f6c99423..17974f11 100644 --- a/tests/unit/query-execution-service.test.ts +++ b/tests/unit/query-execution-service.test.ts @@ -3,25 +3,29 @@ import { createQueryExecutionService, } from '../../src/application/query-execution-service.js'; import type { - QueryExecutionDeps, ScriptStatement, + QueryExecutionDeps, QueryExecutionRequest, QueryProgressCallbacks, ScriptStatement, } from '../../src/application/query-execution-service.js'; -import type { ChCtx, RunQueryOptions, RunQueryResult, runQuery, killQuery } from '../../src/net/ch-client.js'; import { newResult } from '../../src/core/stream.js'; import { SELECT_ROW_CAP } from '../../src/core/script-result.js'; import type { ScriptEntry } from '../../src/core/script-result.js'; -// Issue #630 Phase 5 — sqlString now has one implementation, owned by the -// package; format.js no longer declares it. -import { sqlString } from '@altinity/clickhouse-http'; // ── Fakes ──────────────────────────────────────────────────────────────────── -/** One recorded `runQuery` call. */ -interface RunQueryCall { ctx: ChCtx; sql: string; opts: RunQueryOptions } - -/** A scripted behavior for one queued `runQuery` call: resolves/rejects, and - * may pulse `opts.onLine`/`opts.onChunk` first (simulating a stream) — the - * same shape the real `net/ch-client.js::runQuery` drives its callers with. */ -type Behavior = (opts: RunQueryOptions) => RunQueryResult | Promise; +/** One recorded `runProgress` call. */ +interface ProgressCall { request: QueryExecutionRequest; callbacks: QueryProgressCallbacks } +/** One recorded `runText` call. */ +interface TextCall { request: QueryExecutionRequest } + +/** A scripted behavior for one queued `runProgress` call: may pulse + * `callbacks.onLine`/`callbacks.onChunk` first (simulating a stream), then + * either resolves (clean stream completion) or throws — the same shape the + * real production `runProgress` (backed by `authenticatedProgress`) drives + * its callers with. */ +type ProgressBehavior = (callbacks: QueryProgressCallbacks) => void | Promise; +/** A scripted behavior for one queued `runText` call: resolves with the raw + * text body, or throws (matching the new "package consumers throw" contract + * — #630 Phase 7 §6.5). */ +type TextBehavior = () => string | Promise; function abortError(): Error { const e = new Error('aborted'); @@ -29,37 +33,43 @@ function abortError(): Error { return e; } -/** A queued fake matching `typeof runQuery` exactly: each call consumes the - * next queued behavior (throwing if the queue runs dry, so an unscripted call - * fails loudly rather than hanging). Records every call for assertions. */ -function fakeRunQuery(behaviors: Behavior[]): { fn: typeof runQuery; calls: RunQueryCall[] } { - const calls: RunQueryCall[] = []; +/** A queued fake matching `QueryExecutionDeps['runProgress']` exactly: each + * call consumes the next queued behavior (throwing if the queue runs dry). + * Records every call for assertions. */ +function fakeRunProgress(behaviors: ProgressBehavior[]): { fn: QueryExecutionDeps['runProgress']; calls: ProgressCall[] } { + const calls: ProgressCall[] = []; let i = 0; - const fn = vi.fn(async (ctx: ChCtx, sql: string, opts: RunQueryOptions = {}): Promise => { - calls.push({ ctx, sql, opts }); + const fn = vi.fn(async (request: QueryExecutionRequest, callbacks: QueryProgressCallbacks): Promise => { + calls.push({ request, callbacks }); const behavior = behaviors[i]; i += 1; - if (!behavior) throw new Error('unscripted runQuery call: ' + sql); - return behavior(opts); + if (!behavior) throw new Error('unscripted runProgress call: ' + request.sql); + await behavior(callbacks); }); return { fn, calls }; } -function fakeKillQuery(): { fn: typeof killQuery; calls: { ctx: ChCtx; queryId: string | null | undefined; sqlString: (s: unknown) => string }[] } { - const calls: { ctx: ChCtx; queryId: string | null | undefined; sqlString: (s: unknown) => string }[] = []; - const fn = vi.fn(async (ctx: ChCtx, queryId: string | null | undefined, sqlStringFn: (s: unknown) => string): Promise => { - calls.push({ ctx, queryId, sqlString: sqlStringFn }); +/** A queued fake matching `QueryExecutionDeps['runText']` exactly. */ +function fakeRunText(behaviors: TextBehavior[]): { fn: QueryExecutionDeps['runText']; calls: TextCall[] } { + const calls: TextCall[] = []; + let i = 0; + const fn = vi.fn(async (request: QueryExecutionRequest): Promise => { + calls.push({ request }); + const behavior = behaviors[i]; + i += 1; + if (!behavior) throw new Error('unscripted runText call: ' + request.sql); + return behavior(); }); return { fn, calls }; } -const fakeCtx: ChCtx = { - fetch: (() => Promise.reject(new Error('not used'))) as unknown as typeof fetch, - origin: 'https://ch.local', - getToken: async () => 'tok', - refresh: async () => false, - onSignedOut: () => {}, -}; +function fakeCancel(): { fn: QueryExecutionDeps['cancel']; calls: { ownerEpoch: number | null | undefined; queryId: string | null | undefined }[] } { + const calls: { ownerEpoch: number | null | undefined; queryId: string | null | undefined }[] = []; + const fn = vi.fn(async (ownerEpoch: number | null | undefined, queryId: string | null | undefined): Promise => { + calls.push({ ownerEpoch, queryId }); + }); + return { fn, calls }; +} /** A deterministic uid sequence: 'q-1', 'q-2', … — matches the shape of * app.ts's real `uid('q')` (prefix + a counter) closely enough for assertions @@ -79,14 +89,13 @@ function makeNow(): () => number { function makeDeps(over: Partial = {}): QueryExecutionDeps { return { - runQuery: fakeRunQuery([]).fn, - killQuery: fakeKillQuery().fn, - ctx: () => fakeCtx, + runProgress: fakeRunProgress([]).fn, + runText: fakeRunText([]).fn, + cancel: fakeCancel().fn, now: makeNow(), uid: makeUid(), retryMs: 7, sleep: vi.fn(async () => {}), - sqlString, ...over, }; } @@ -94,78 +103,114 @@ function makeDeps(over: Partial = {}): QueryExecutionDeps { // ── executeRead ────────────────────────────────────────────────────────────── describe('executeRead', () => { - it('folds streamed lines into the result via applyStreamLine', async () => { - const { fn, calls } = fakeRunQuery([ - (opts) => { - opts.onLine!({ meta: [{ name: 'x', type: 'Int32' }] }); - opts.onLine!({ row: { x: 1 } }); - return { streamed: true }; + it('folds streamed lines into the result via applyStreamLine (Table -> progress)', async () => { + const { fn, calls } = fakeRunProgress([ + (cbs) => { + cbs.onLine!({ meta: [{ name: 'x', type: 'Int32' }] }); + cbs.onLine!({ row: { x: 1 } }); }, ]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); + const svc = createQueryExecutionService(makeDeps({ runProgress: fn })); const result = newResult('Table'); const out = await svc.executeRead(result, { sql: 'SELECT 1' }); expect(out.columns).toEqual([{ name: 'x', type: 'Int32' }]); expect(out.rows).toEqual([[1]]); - expect(calls[0].sql).toBe('SELECT 1'); + expect(calls[0].request.sql).toBe('SELECT 1'); }); - // Issue #630 Phase 3 §11.8 — proves the package callback-order contract - // (every onLine for a chunk, THEN that chunk's onChunk) still produces the - // same visible row/progress state before the caller's own repaint hook - // fires — the real-time UI/result compatibility invariant this move must - // not disturb. `fakeRunQuery`'s behavior pulses onLine/onChunk synchronously - // in exactly the order the real production `runQuery` (via the package's - // `streamLines`) drives them. - it('reflects every line mutation from a chunk in the caller-owned result BEFORE that chunk\'s onChunk repaint fires', async () => { - const result = newResult('Table'); - const onChunk = vi.fn(() => { - // At the moment onChunk fires, the result must already carry both the - // meta and the row line dispatched earlier in this same chunk. - expect(result.columns).toEqual([{ name: 'x', type: 'Int32' }]); - expect(result.rows).toEqual([[1]]); - }); - const { fn } = fakeRunQuery([ - (opts) => { - opts.onLine!({ meta: [{ name: 'x', type: 'Int32' }] }); - opts.onLine!({ row: { x: 1 } }); - opts.onChunk!(); - return { streamed: true }; - }, - ]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); - await svc.executeRead(result, { sql: 'SELECT 1', onChunk }); - expect(onChunk).toHaveBeenCalledTimes(1); + it('maps Table to JSONStringsEachRowWithProgress with CORS, no wait_end_of_query, no cap at rowLimit 0', async () => { + const { fn, calls } = fakeRunProgress([() => {}]); + const svc = createQueryExecutionService(makeDeps({ runProgress: fn })); + await svc.executeRead(newResult('Table'), { sql: 'SELECT 1' }); + expect(calls[0].request.defaultFormat).toBe('JSONStringsEachRowWithProgress'); + expect(calls[0].request.settings).toEqual({ add_http_cors_header: 1 }); }); - it('sets result.error from out.error', async () => { - const { fn } = fakeRunQuery([() => ({ error: 'boom' })]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); - const result = newResult('Table'); - const out = await svc.executeRead(result, { sql: 'SELECT 1' }); - expect(out.error).toBe('boom'); + it('maps KPI to JSONEachRowWithProgress with CORS, no wait_end_of_query', async () => { + const { fn, calls } = fakeRunProgress([() => {}]); + const svc = createQueryExecutionService(makeDeps({ runProgress: fn })); + await svc.executeRead(newResult('KPI'), { sql: 'SELECT 1', format: 'KPI' }); + expect(calls[0].request.defaultFormat).toBe('JSONEachRowWithProgress'); + expect(calls[0].request.settings).toEqual({ add_http_cors_header: 1 }); + }); + + it('a positive rowLimit adds max_result_rows/result_overflow_mode to Table settings', async () => { + const { fn, calls } = fakeRunProgress([() => {}]); + const svc = createQueryExecutionService(makeDeps({ runProgress: fn })); + await svc.executeRead(newResult('Table'), { sql: 'SELECT 1', rowLimit: 100 }); + expect(calls[0].request.settings).toEqual({ add_http_cors_header: 1, max_result_rows: 100, result_overflow_mode: 'break' }); + }); + + it('a positive rowLimit adds the SAME cap to KPI settings', async () => { + const { fn, calls } = fakeRunProgress([() => {}]); + const svc = createQueryExecutionService(makeDeps({ runProgress: fn })); + await svc.executeRead(newResult('KPI'), { sql: 'SELECT 1', format: 'KPI', rowLimit: 100 }); + expect(calls[0].request.settings).toEqual({ add_http_cors_header: 1, max_result_rows: 100, result_overflow_mode: 'break' }); }); - it('sets rawText + progress.bytes from out.raw', async () => { - const { fn } = fakeRunQuery([() => ({ raw: 'abcde' })]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); + it('maps TSV to TabSeparatedWithNamesAndTypes via runText, with wait_end_of_query=1 + CORS', async () => { + const { fn, calls } = fakeRunText([() => 'abcde']); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); const result = newResult('TSV'); - const out = await svc.executeRead(result, { sql: 'SHOW TABLES' }); + const out = await svc.executeRead(result, { sql: 'SHOW TABLES', format: 'TSV' }); + expect(calls[0].request.defaultFormat).toBe('TabSeparatedWithNamesAndTypes'); + expect(calls[0].request.settings).toEqual({ wait_end_of_query: 1, add_http_cors_header: 1 }); expect(out.rawText).toBe('abcde'); expect(out.progress.bytes).toBe(5); }); - it('defaults format to Table and rowLimit to 0 in the runQuery opts', async () => { - const { fn, calls } = fakeRunQuery([() => ({ raw: '' })]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); - await svc.executeRead(newResult('Table'), { sql: 'SELECT 1' }); - expect(calls[0].opts.format).toBe('Table'); - expect(calls[0].opts.resultRowLimit).toBe(0); + it('a positive rowLimit on TSV keeps the cap in the SAME settings object as wait_end_of_query/CORS', async () => { + const { fn, calls } = fakeRunText([() => '']); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); + await svc.executeRead(newResult('TSV'), { sql: 'SHOW TABLES', format: 'TSV', rowLimit: 250 }); + expect(calls[0].request.settings).toEqual({ + wait_end_of_query: 1, add_http_cors_header: 1, max_result_rows: 250, result_overflow_mode: 'break', + }); + }); + + // #630 Phase 7 §23 — the dedicated explicit-FORMAT regression: a + // regression that retains the row cap ONLY in the Table/KPI branches must + // fail this exact case. + it('an explicit-FORMAT CSV SELECT with a positive row limit: exact caller format, wait_end_of_query, CORS, and the cap — all in settings', async () => { + const { fn, calls } = fakeRunText([() => '']); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); + await svc.executeRead(newResult('CSV'), { sql: 'SELECT 1 FORMAT CSV', format: 'CSV', rowLimit: 500 }); + expect(calls[0].request.defaultFormat).toBe('CSV'); + expect(calls[0].request.settings).toEqual({ + wait_end_of_query: 1, + add_http_cors_header: 1, + max_result_rows: 500, + result_overflow_mode: 'break', + }); + }); + + it('an explicit/raw format with rowLimit 0 (EXPLAIN/PIPELINE/ESTIMATE) stays uncapped', async () => { + const { fn, calls } = fakeRunText([() => '']); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); + await svc.executeRead(newResult('Table'), { sql: 'EXPLAIN SELECT 1', format: 'Table exempted via rowLimit', rowLimit: 0 }); + expect(calls[0].request.settings).toEqual({ wait_end_of_query: 1, add_http_cors_header: 1 }); + }); + + it('sets result.error from a thrown error (package consumers throw, not {error})', async () => { + const { fn } = fakeRunText([() => { throw new Error('boom'); }]); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); + const result = newResult('TSV'); + const out = await svc.executeRead(result, { sql: 'SELECT 1', format: 'TSV' }); + expect(out.error).toBe('boom'); + }); + + it('sets rawText + progress.bytes from the resolved raw text', async () => { + const { fn } = fakeRunText([() => 'abcde']); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); + const result = newResult('TSV'); + const out = await svc.executeRead(result, { sql: 'SHOW TABLES', format: 'TSV' }); + expect(out.rawText).toBe('abcde'); + expect(out.progress.bytes).toBe(5); }); it('passes explicit format/rowLimit/params/queryId/signal through', async () => { - const { fn, calls } = fakeRunQuery([() => ({ raw: '' })]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); + const { fn, calls } = fakeRunText([() => '']); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); const controller = new AbortController(); await svc.executeRead(newResult('JSON'), { sql: 'SELECT 1', @@ -175,39 +220,36 @@ describe('executeRead', () => { queryId: 'q-explicit', signal: controller.signal, }); - expect(calls[0].opts.format).toBe('JSON'); - expect(calls[0].opts.resultRowLimit).toBe(50); - expect(calls[0].opts.params).toEqual({ param_x: 'y' }); - expect(calls[0].opts.queryId).toBe('q-explicit'); - expect(calls[0].opts.signal).toBe(controller.signal); + expect(calls[0].request.defaultFormat).toBe('JSON'); + expect(calls[0].request.settings).toMatchObject({ max_result_rows: 50, result_overflow_mode: 'break' }); + expect(calls[0].request.params).toEqual({ query_id: 'q-explicit', param_x: 'y' }); + expect(calls[0].request.signal).toBe(controller.signal); }); it('forwards an onChunk pulse with no arguments', async () => { - const { fn, calls } = fakeRunQuery([ - (opts) => { opts.onChunk!(); return { raw: '' }; }, + const { fn, calls } = fakeRunProgress([ + (cbs) => { cbs.onChunk!(); }, ]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); + const svc = createQueryExecutionService(makeDeps({ runProgress: fn })); const onChunk = vi.fn(); - await svc.executeRead(newResult('TSV'), { sql: 'SELECT 1', onChunk }); + await svc.executeRead(newResult('Table'), { sql: 'SELECT 1', onChunk }); expect(onChunk).toHaveBeenCalledTimes(1); expect(onChunk).toHaveBeenCalledWith(); - expect(typeof calls[0].opts.onChunk).toBe('function'); + expect(typeof calls[0].callbacks.onChunk).toBe('function'); }); it('passes no onChunk wrapper when the request omits one', async () => { - const { fn, calls } = fakeRunQuery([() => ({ raw: '' })]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); - await svc.executeRead(newResult('TSV'), { sql: 'SELECT 1' }); - expect(calls[0].opts.onChunk).toBeUndefined(); + const { fn, calls } = fakeRunProgress([() => {}]); + const svc = createQueryExecutionService(makeDeps({ runProgress: fn })); + await svc.executeRead(newResult('Table'), { sql: 'SELECT 1' }); + expect(calls[0].callbacks.onChunk).toBeUndefined(); }); - it('does not acquire auth or mutate a result when the caller epoch is already stale', async () => { - const { fn } = fakeRunQuery([() => ({ error: 'must not run' })]); - const ctx = vi.fn(() => fakeCtx); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn, ctx })); - const result = newResult('Table'); - await svc.executeRead(result, { sql: 'SELECT 1', isCurrent: () => false }); - expect(ctx).not.toHaveBeenCalled(); + it('does not call the transport or mutate a result when the caller epoch is already stale', async () => { + const { fn } = fakeRunText([() => { throw new Error('must not run'); }]); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); + const result = newResult('TSV'); + await svc.executeRead(result, { sql: 'SELECT 1', format: 'TSV', isCurrent: () => false }); expect(fn).not.toHaveBeenCalled(); expect(result.error).toBeNull(); }); @@ -215,17 +257,17 @@ describe('executeRead', () => { it('fences late stream chunks and settlement after the caller epoch closes', async () => { let current = true; const onChunk = vi.fn(); - const { fn } = fakeRunQuery([ - (opts) => { - opts.onLine!({ meta: [{ name: 'x', type: 'Int32' }] }); - opts.onLine!({ row: { x: 1 } }); + const { fn } = fakeRunProgress([ + (cbs) => { + cbs.onLine!({ meta: [{ name: 'x', type: 'Int32' }] }); + cbs.onLine!({ row: { x: 1 } }); current = false; - opts.onLine!({ row: { x: 2 } }); - opts.onChunk!(); - return { error: 'late error' }; + cbs.onLine!({ row: { x: 2 } }); + cbs.onChunk!(); + throw new Error('late error'); }, ]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); + const svc = createQueryExecutionService(makeDeps({ runProgress: fn })); const result = newResult('Table'); await svc.executeRead(result, { sql: 'SELECT 1', isCurrent: () => current, onChunk }); expect(result.rows).toEqual([[1]]); @@ -233,15 +275,25 @@ describe('executeRead', () => { expect(onChunk).not.toHaveBeenCalled(); }); + it('fences a successful raw/text settlement after the caller epoch closes — no rawText/bytes publication', async () => { + let current = true; + const { fn } = fakeRunText([() => { current = false; return 'late body'; }]); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); + const result = newResult('TSV'); + const out = await svc.executeRead(result, { sql: 'SHOW TABLES', format: 'TSV', isCurrent: () => current }); + expect(out.rawText).toBeNull(); + expect(out.progress.bytes).toBe(0); + }); + it('marks cancelled (not error) and keeps partial rows on AbortError', async () => { - const { fn } = fakeRunQuery([ - (opts) => { - opts.onLine!({ meta: [{ name: 'x', type: 'Int32' }] }); - opts.onLine!({ row: { x: 1 } }); + const { fn } = fakeRunProgress([ + (cbs) => { + cbs.onLine!({ meta: [{ name: 'x', type: 'Int32' }] }); + cbs.onLine!({ row: { x: 1 } }); throw abortError(); }, ]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); + const svc = createQueryExecutionService(makeDeps({ runProgress: fn })); const result = newResult('Table'); const out = await svc.executeRead(result, { sql: 'SELECT 1' }); expect(out.cancelled).toBe(true); @@ -250,31 +302,31 @@ describe('executeRead', () => { }); it("sets error to 'Network error' on a TypeError", async () => { - const { fn } = fakeRunQuery([() => { throw new TypeError('fetch failed'); }]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); + const { fn } = fakeRunProgress([() => { throw new TypeError('fetch failed'); }]); + const svc = createQueryExecutionService(makeDeps({ runProgress: fn })); const out = await svc.executeRead(newResult('Table'), { sql: 'SELECT 1' }); expect(out.error).toBe('Network error'); }); - it('sets error to the message string on a generic Error', async () => { - const { fn } = fakeRunQuery([() => { throw new Error('weird failure'); }]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); + it('sets error to the message string on a generic Error (including a package ClickHouseError-shaped one — its `.message` is already the safe text)', async () => { + const { fn } = fakeRunProgress([() => { const e = new Error('weird failure'); e.name = 'ClickHouseError'; throw e; }]); + const svc = createQueryExecutionService(makeDeps({ runProgress: fn })); const out = await svc.executeRead(newResult('Table'), { sql: 'SELECT 1' }); expect(out.error).toBe('weird failure'); }); it('sets error via String(e) on a non-Error throw', async () => { - const { fn } = fakeRunQuery([() => { throw 'boom'; }]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); + const { fn } = fakeRunProgress([() => { throw 'boom'; }]); + const svc = createQueryExecutionService(makeDeps({ runProgress: fn })); const out = await svc.executeRead(newResult('Table'), { sql: 'SELECT 1' }); expect(out.error).toBe('boom'); }); it('returns the same result reference it was given', async () => { - const { fn } = fakeRunQuery([() => ({ raw: '' })]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); + const { fn } = fakeRunText([() => '']); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); const result = newResult('TSV'); - const out = await svc.executeRead(result, { sql: 'SELECT 1' }); + const out = await svc.executeRead(result, { sql: 'SELECT 1', format: 'TSV' }); expect(out).toBe(result); }); }); @@ -291,18 +343,18 @@ const ddlStmt = (params: Record = {}): ScriptStatement describe('executeScript', () => { it('fences late errors and callback publication after an authenticated epoch closes', async () => { let current = true; - const rejected = fakeRunQuery([() => { current = false; throw new Error('late'); }]); - const service = createQueryExecutionService(makeDeps({ runQuery: rejected.fn })); + const rejected = fakeRunProgress([() => { current = false; throw new Error('late'); }]); + const service = createQueryExecutionService(makeDeps({ runProgress: rejected.fn })); await expect(service.executeRead(newResult('Table'), { sql: 'SELECT 1', isCurrent: () => current })) .resolves.toMatchObject({ error: null }); // The entry is local bookkeeping, but its callback is a UI publication and // must be fenced independently for both error and success entries. - for (const outcome of [{ error: 'bad' } as RunQueryResult, { raw: '' } as RunQueryResult]) { + for (const outcome of [() => { throw new Error('bad'); }, () => ''] as TextBehavior[]) { let checks = 0; - const transport = fakeRunQuery([() => outcome]); + const transport = fakeRunText([outcome]); const onStatementResult = vi.fn(); - const scoped = createQueryExecutionService(makeDeps({ runQuery: transport.fn })); + const scoped = createQueryExecutionService(makeDeps({ runText: transport.fn })); const result = await scoped.executeScript({ statements: [ddlStmt()], isCurrent: () => (++checks < 5), @@ -315,8 +367,8 @@ describe('executeScript', () => { }); it('stringifies a non-Error script transport failure', async () => { - const { fn } = fakeRunQuery([() => { throw 'opaque transport failure'; }]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); + const { fn } = fakeRunText([() => { throw 'opaque transport failure'; }]); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); const { entries } = await svc.executeScript({ statements: [ddlStmt()], onStatementStart: vi.fn(), onStatementResult: vi.fn() }); expect(entries).toEqual([expect.objectContaining({ status: 'error', error: 'opaque transport failure' })]); }); @@ -326,14 +378,14 @@ describe('executeScript', () => { // script attempt: before the loop, after minting its id, after transport, // and after a retry. They are separate auth-loss interleavings in the UI. const before = createQueryExecutionService(makeDeps({ - runQuery: fakeRunQuery([]).fn, + runText: fakeRunText([]).fn, })); await expect(before.executeScript({ statements: [ddlStmt()], isCurrent: () => false, onStatementStart: vi.fn(), onStatementResult: vi.fn() })) .resolves.toEqual({ entries: [], aborted: true }); let checks = 0; const afterId = createQueryExecutionService(makeDeps({ - runQuery: fakeRunQuery([]).fn, + runText: fakeRunText([]).fn, })); await expect(afterId.executeScript({ statements: [ddlStmt()], @@ -342,21 +394,21 @@ describe('executeScript', () => { })).resolves.toEqual({ entries: [], aborted: true }); let postTransport = true; - const transport = fakeRunQuery([() => { postTransport = false; return { raw: '' }; }]); - const afterTransport = createQueryExecutionService(makeDeps({ runQuery: transport.fn })); + const transport = fakeRunText([() => { postTransport = false; return ''; }]); + const afterTransport = createQueryExecutionService(makeDeps({ runText: transport.fn })); await expect(afterTransport.executeScript({ statements: [ddlStmt()], isCurrent: () => postTransport, onStatementStart: vi.fn(), onStatementResult: vi.fn(), })).resolves.toEqual({ entries: [], aborted: true }); let retryTransportCalls = 0; - const retryTransport = fakeRunQuery([ - () => ({ error: 'SESSION_IS_LOCKED' }), - () => { retryTransportCalls += 1; return { raw: '' }; }, + const retryTransport = fakeRunText([ + () => { throw new Error('SESSION_IS_LOCKED'); }, + () => { retryTransportCalls += 1; return ''; }, ]); let retryChecks = 0; const afterRetry = createQueryExecutionService(makeDeps({ - runQuery: retryTransport.fn, + runText: retryTransport.fn, sleep: async () => {}, })); await expect(afterRetry.executeScript({ @@ -371,8 +423,8 @@ describe('executeScript', () => { it('does not enter transport when the scope closes between publishing the id and the attempt', async () => { let checks = 0; - const run = fakeRunQuery([]); - const svc = createQueryExecutionService(makeDeps({ runQuery: run.fn })); + const run = fakeRunText([]); + const svc = createQueryExecutionService(makeDeps({ runText: run.fn })); await expect(svc.executeScript({ statements: [ddlStmt()], // loop and id fence pass; attemptStatement itself observes the close. @@ -382,12 +434,12 @@ describe('executeScript', () => { expect(run.calls).toHaveLength(0); }); - it('runs one runQuery per statement, wire text vs authored sql, in order', async () => { - const { fn, calls } = fakeRunQuery([ - () => ({ raw: JSON.stringify({ meta: [{ name: 'x', type: 'Int32' }], data: [[1]] }) }), - () => ({ raw: '' }), + it('runs one runText call per statement, wire text vs authored sql, in order', async () => { + const { fn, calls } = fakeRunText([ + () => JSON.stringify({ meta: [{ name: 'x', type: 'Int32' }], data: [[1]] }), + () => '', ]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); const onStatementStart = vi.fn(); const onStatementResult = vi.fn(); const { entries, aborted } = await svc.executeScript({ @@ -396,29 +448,31 @@ describe('executeScript', () => { onStatementResult, }); expect(aborted).toBe(false); - expect(calls[0].sql).toBe('SELECT 1 /* exec */'); - expect(calls[1].sql).toBe('CREATE TABLE t (x Int32) ENGINE=Memory /* exec */'); + expect(calls[0].request.sql).toBe('SELECT 1 /* exec */'); + expect(calls[1].request.sql).toBe('CREATE TABLE t (x Int32) ENGINE=Memory /* exec */'); expect(entries[0].sql).toBe('SELECT 1'); expect(entries[1].sql).toBe('CREATE TABLE t (x Int32) ENGINE=Memory'); }); - it('parses a rows entry via parseSelectResult, over-fetching the cap only for row-returning statements', async () => { - const { fn, calls } = fakeRunQuery([ - () => ({ raw: JSON.stringify({ meta: [{ name: 'x', type: 'Int32' }], data: [[1], [2]] }) }), - () => ({ raw: '' }), + it('parses a rows entry via parseSelectResult, over-fetching the cap only for row-returning statements; both settings stay the same regardless of row-returning-ness', async () => { + const { fn, calls } = fakeRunText([ + () => JSON.stringify({ meta: [{ name: 'x', type: 'Int32' }], data: [[1], [2]] }), + () => '', ]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); const { entries } = await svc.executeScript({ statements: [selectStmt({ session_id: 's1' }), ddlStmt({ session_id: 's1' })], onStatementStart: vi.fn(), onStatementResult: vi.fn(), }); - expect(calls[0].opts.format).toBe('JSONCompact'); - expect(calls[0].opts.params).toEqual({ - session_id: 's1', max_result_rows: SELECT_ROW_CAP + 1, result_overflow_mode: 'break', + expect(calls[0].request.defaultFormat).toBe('JSONCompact'); + expect(calls[0].request.params).toEqual({ + query_id: calls[0].request.params!.query_id, session_id: 's1', max_result_rows: SELECT_ROW_CAP + 1, result_overflow_mode: 'break', }); - expect(calls[1].opts.format).toBe('TSV'); - expect(calls[1].opts.params).toEqual({ session_id: 's1' }); + expect(calls[0].request.settings).toEqual({ wait_end_of_query: 1, add_http_cors_header: 1 }); + expect(calls[1].request.defaultFormat).toBe('TabSeparatedWithNamesAndTypes'); + expect(calls[1].request.params).toEqual({ query_id: calls[1].request.params!.query_id, session_id: 's1' }); + expect(calls[1].request.settings).toEqual({ wait_end_of_query: 1, add_http_cors_header: 1 }); const rowsEntry = entries[0]; expect(rowsEntry.status).toBe('rows'); if (rowsEntry.status === 'rows') { @@ -430,16 +484,47 @@ describe('executeScript', () => { expect(entries[1].status).toBe('ok'); }); + // #630 Phase 7 §2.3/§8/§23 — script over-fetch cap placement/precedence: + // a caller-supplied `max_result_rows`/`result_overflow_mode` in + // `stmt.params` must be OVERRIDDEN by the service's own cap, the cap must + // live in `params` (never `settings`), and it must never be duplicated + // into `settings` either. + it('script cap wins a params collision and never appears in settings (row-returning statement)', async () => { + const { fn, calls } = fakeRunText([() => JSON.stringify({ meta: [], data: [] })]); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); + await svc.executeScript({ + statements: [selectStmt({ max_result_rows: 5, result_overflow_mode: 'throw' })], + onStatementStart: vi.fn(), + onStatementResult: vi.fn(), + }); + expect(calls[0].request.params!.max_result_rows).toBe(SELECT_ROW_CAP + 1); + expect(calls[0].request.params!.result_overflow_mode).toBe('break'); + expect(calls[0].request.settings).not.toHaveProperty('max_result_rows'); + expect(calls[0].request.settings).not.toHaveProperty('result_overflow_mode'); + }); + + it('a non-row-returning statement never receives the script cap in params or settings, even with conflicting caller params', async () => { + const { fn, calls } = fakeRunText([() => '']); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); + await svc.executeScript({ + statements: [ddlStmt({ max_result_rows: 5, result_overflow_mode: 'throw' })], + onStatementStart: vi.fn(), + onStatementResult: vi.fn(), + }); + expect(calls[0].request.params).toEqual({ query_id: calls[0].request.params!.query_id, max_result_rows: 5, result_overflow_mode: 'throw' }); + expect(calls[0].request.settings).not.toHaveProperty('max_result_rows'); + }); + it('publishes a fresh query_id per attempt, synchronously before each await, on the retry path', async () => { const order: string[] = []; - const { fn } = fakeRunQuery([ - (opts) => { order.push('run:' + opts.queryId); return { error: 'SESSION_IS_LOCKED: locked' }; }, - (opts) => { order.push('run:' + opts.queryId); return { raw: '' }; }, + const { fn } = fakeRunText([ + () => { throw new Error('SESSION_IS_LOCKED: locked'); }, + () => '', ]); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); const onStatementStart = vi.fn((_i: number, info: { queryId: string; attempt: 1 | 2 }) => { order.push('start:' + info.attempt + ':' + info.queryId); }); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); await svc.executeScript({ statements: [ddlStmt()], onStatementStart, @@ -451,20 +536,14 @@ describe('executeScript', () => { expect(first.attempt).toBe(1); expect(second.attempt).toBe(2); expect(first.queryId).not.toBe(second.queryId); - expect(order).toEqual([ - 'start:1:' + first.queryId, - 'run:' + first.queryId, - 'start:2:' + second.queryId, - 'run:' + second.queryId, - ]); }); it('retries a SESSION_IS_LOCKED failure for ANY statement (including non-row-returning)', async () => { - const { fn, calls } = fakeRunQuery([ - () => ({ error: 'Code: 373. DB::Exception: SESSION_IS_LOCKED' }), - () => ({ raw: '' }), + const { fn, calls } = fakeRunText([ + () => { throw new Error('Code: 373. DB::Exception: SESSION_IS_LOCKED'); }, + () => '', ]); - const deps = makeDeps({ runQuery: fn }); + const deps = makeDeps({ runText: fn }); const svc = createQueryExecutionService(deps); const { entries } = await svc.executeScript({ statements: [ddlStmt()], @@ -478,14 +557,13 @@ describe('executeScript', () => { it('does not let a delayed retry acquire a replacement auth context', async () => { let current = true; - const { fn, calls } = fakeRunQuery([ - () => ({ error: 'SESSION_IS_LOCKED: locked' }), - () => ({ raw: '' }), + const { fn, calls } = fakeRunText([ + () => { throw new Error('SESSION_IS_LOCKED: locked'); }, + () => '', ]); - const ctx = vi.fn(() => fakeCtx); const sleep = vi.fn(async () => { current = false; }); const onStatementStart = vi.fn(); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn, ctx, sleep })); + const svc = createQueryExecutionService(makeDeps({ runText: fn, sleep })); const result = await svc.executeScript({ statements: [ddlStmt()], isCurrent: () => current, @@ -494,16 +572,15 @@ describe('executeScript', () => { }); expect(result).toEqual({ entries: [], aborted: true }); expect(calls).toHaveLength(1); - expect(ctx).toHaveBeenCalledTimes(1); expect(onStatementStart).toHaveBeenCalledTimes(1); }); it('retries a transient (TypeError) failure only for a row-returning statement', async () => { - const { fn, calls } = fakeRunQuery([ + const { fn, calls } = fakeRunText([ () => { throw new TypeError('reset'); }, - () => ({ raw: JSON.stringify({ meta: [], data: [] }) }), + () => JSON.stringify({ meta: [], data: [] }), ]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); const { entries } = await svc.executeScript({ statements: [selectStmt()], onStatementStart: vi.fn(), @@ -514,10 +591,10 @@ describe('executeScript', () => { }); it('does NOT retry a transient failure for a non-row-returning statement, and reports the exact message', async () => { - const { fn, calls } = fakeRunQuery([ + const { fn, calls } = fakeRunText([ () => { throw new TypeError('reset'); }, ]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); const { entries } = await svc.executeScript({ statements: [ddlStmt()], onStatementStart: vi.fn(), @@ -531,10 +608,10 @@ describe('executeScript', () => { }); it('classifies a thrown non-TypeError Error as a non-transient error (no retry)', async () => { - const { fn, calls } = fakeRunQuery([ + const { fn, calls } = fakeRunText([ () => { throw new Error('kaboom'); }, ]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); const { entries } = await svc.executeScript({ statements: [selectStmt()], onStatementStart: vi.fn(), @@ -546,10 +623,10 @@ describe('executeScript', () => { }); it('does not retry a genuine (non-transient, non-locked) query error', async () => { - const { fn, calls } = fakeRunQuery([ - () => ({ error: 'Code: 62. DB::Exception: Syntax error' }), + const { fn, calls } = fakeRunText([ + () => { throw new Error('Code: 62. DB::Exception: Syntax error'); }, ]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); const { entries } = await svc.executeScript({ statements: [selectStmt()], onStatementStart: vi.fn(), @@ -561,10 +638,10 @@ describe('executeScript', () => { }); it('stops on the first failure — later statements are never sent', async () => { - const { fn, calls } = fakeRunQuery([ - () => ({ error: 'Code: 62. DB::Exception: Syntax error' }), + const { fn, calls } = fakeRunText([ + () => { throw new Error('Code: 62. DB::Exception: Syntax error'); }, ]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); const { entries } = await svc.executeScript({ statements: [ddlStmt(), selectStmt()], onStatementStart: vi.fn(), @@ -576,11 +653,11 @@ describe('executeScript', () => { }); it('aborts mid-script: {aborted:true}, no entry for the aborted statement, earlier entries kept', async () => { - const { fn, calls } = fakeRunQuery([ - () => ({ raw: '' }), + const { fn, calls } = fakeRunText([ + () => '', () => { throw abortError(); }, ]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); const { entries, aborted } = await svc.executeScript({ statements: [ddlStmt(), selectStmt()], onStatementStart: vi.fn(), @@ -593,8 +670,8 @@ describe('executeScript', () => { }); it('computes ms from the injected clock', async () => { - const { fn } = fakeRunQuery([() => ({ raw: '' })]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); + const { fn } = fakeRunText([() => '']); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); const { entries } = await svc.executeScript({ statements: [ddlStmt()], onStatementStart: vi.fn(), @@ -604,11 +681,11 @@ describe('executeScript', () => { }); it('fires onStatementResult once per pushed entry, with the correct index', async () => { - const { fn } = fakeRunQuery([ - () => ({ raw: '' }), - () => ({ raw: JSON.stringify({ meta: [], data: [] }) }), + const { fn } = fakeRunText([ + () => '', + () => JSON.stringify({ meta: [], data: [] }), ]); - const svc = createQueryExecutionService(makeDeps({ runQuery: fn })); + const svc = createQueryExecutionService(makeDeps({ runText: fn })); const seen: { index: number; entry: ScriptEntry }[] = []; await svc.executeScript({ statements: [ddlStmt(), selectStmt()], @@ -626,14 +703,21 @@ describe('executeScript', () => { // ── kill ───────────────────────────────────────────────────────────────────── describe('kill', () => { - it('delegates to deps.killQuery with ctx(), the queryId, and sqlString', async () => { - const killed = fakeKillQuery(); - const deps = makeDeps({ killQuery: killed.fn }); + it('delegates to deps.cancel with the owner epoch and the queryId', async () => { + const cancelled = fakeCancel(); + const deps = makeDeps({ cancel: cancelled.fn }); + const svc = createQueryExecutionService(deps); + await svc.kill(3, 'q-123'); + expect(cancelled.calls).toHaveLength(1); + expect(cancelled.calls[0].ownerEpoch).toBe(3); + expect(cancelled.calls[0].queryId).toBe('q-123'); + }); + + it('passes a null/undefined owner epoch or query id straight through — the fence lives in deps.cancel', async () => { + const cancelled = fakeCancel(); + const deps = makeDeps({ cancel: cancelled.fn }); const svc = createQueryExecutionService(deps); - await svc.kill('q-123'); - expect(killed.calls).toHaveLength(1); - expect(killed.calls[0].ctx).toBe(fakeCtx); - expect(killed.calls[0].queryId).toBe('q-123'); - expect(killed.calls[0].sqlString).toBe(sqlString); + await svc.kill(null, null); + expect(cancelled.calls[0]).toEqual({ ownerEpoch: null, queryId: null }); }); }); diff --git a/tests/unit/workbench-session.test.ts b/tests/unit/workbench-session.test.ts index 4f2d03e8..2fe877ed 100644 --- a/tests/unit/workbench-session.test.ts +++ b/tests/unit/workbench-session.test.ts @@ -426,7 +426,9 @@ describe('createWorkbenchSession: run()', () => { expect(h.hooks.tickElapsed).toHaveBeenCalledTimes(ticksBefore + 1); session.cancel(); expect(freshReq.signal?.aborted).toBe(true); - expect(h.execFakes.kill).toHaveBeenLastCalledWith('q-2'); + // The fresh wave registered under `newScope` (epoch 2) — #630 Phase 7 + // §9.3/9.4: the owner epoch captured at that wave's registration time. + expect(h.execFakes.kill).toHaveBeenLastCalledWith(2, 'q-2'); newGate.resolve({} as StreamResult); await fresh; @@ -811,7 +813,9 @@ describe('createWorkbenchSession: runScript()', () => { const p = session.runScript(['SELECT 1'], 'SELECT 1'); await flush(); session.cancel(); - expect(h.execFakes.kill).toHaveBeenCalledWith('q-live'); + // No executionScope supplied by this harness (defaults to null) — the + // owner epoch captured at registration is null. + expect(h.execFakes.kill).toHaveBeenCalledWith(null, 'q-live'); expect(capturedSignal?.aborted).toBe(true); gate.resolve({ entries: [], aborted: true }); await p; @@ -1598,7 +1602,9 @@ describe('createWorkbenchSession: cancel()', () => { expect(h.state.running.value).toBe(true); session.cancel(); - expect(h.execFakes.kill).toHaveBeenCalledWith(null); + // No executionScope (owner epoch null) and no query_id minted yet + // (cancel fires before preflight resolves). + expect(h.execFakes.kill).toHaveBeenCalledWith(null, null); configGate.resolve(undefined); await pending; @@ -1637,7 +1643,7 @@ describe('createWorkbenchSession: cancel()', () => { const req = h.execFakes.executeRead.mock.calls[0][1] as ExecuteReadRequest; session.cancel(); expect(req.signal?.aborted).toBe(true); - expect(h.execFakes.kill).toHaveBeenCalledWith('q-1'); + expect(h.execFakes.kill).toHaveBeenCalledWith(null, 'q-1'); gate.resolve({ ...req } as unknown as StreamResult); await p; }); @@ -1797,7 +1803,7 @@ describe('createWorkbenchSession: destroy()', () => { session.destroy(); expect(clearSpy.mock.calls.length).toBeGreaterThan(callsBefore); expect(req.signal?.aborted).toBe(true); - expect(h.execFakes.kill).toHaveBeenCalledWith('q-1'); + expect(h.execFakes.kill).toHaveBeenCalledWith(null, 'q-1'); gate.resolve({} as StreamResult); await p; clearSpy.mockRestore();