ktx/spider2-specs/specs/16-bounded-query-execution-timeout.md
Andrey Avtomonov f65a5b0e2e
feat: ktx batch — scan resilience, analytics SQL craft, connector hardening (#312)
* docs: add spider2-specs handoff directory for benchmark-driven feature specs

* feat(cli): connection-scoped wiki pages

Add an optional `connections` frontmatter field so database-specific wiki
knowledge can be scoped to a connection without polluting searches about other
databases, while page keys stay a flat, globally-unique namespace.

- connections: single string or list; absent/empty ⇒ unscoped (applies to all)
- wiki_search (MCP) and `ktx wiki --connection` return unscoped ∪ matching
  pages, filtered at the disk-load seam so all three search lanes draw their
  candidate pool from the already-scoped set (not a post-filter)
- wiki_write accepts connections with REPLACE semantics and rejects a
  connection-scoped write whose key collides with a disjoint-connection page
  (data-loss guard; hard error, no silent clobber)
- explicit connection-id args (wiki_search, memory_ingest, ktx wiki) are
  validated against ktx.yaml via a shared assertConfiguredConnectionId, which
  also closes the prior gap where memory_ingest's connectionId was unvalidated;
  persisted ids absent from config warn (not fail) in `ktx status`
- prompt guidance in the wiki_capture skill and external-ingest prompt; the
  session connectionId is surfaced to the memory agent and ingest work units

Implements spider2-specs/specs/01-connection-scoped-wiki.md; intake draft moved
to spider2-specs/done/.

* docs(spider2-specs): add specs/ refinement stage and composite-key join spec

Describe the todo/ → specs/ → done/ pipeline in the README (refined specs are
the durable artifact; intake drafts move to done/ on ship) and add a
MEDIUM-priority spec for multi-column composite-key join detection found during
the first sqlite smoke test.

* feat(cli): add --verbatim ingest mode for authoritative documents

Store each --text/--file document body unchanged as a GLOBAL wiki page
instead of routing it through the memory agent, which may rewrite,
condense, or re-title it. The LLM derives only metadata (summary, tags,
sl_refs) and only for frontmatter fields the document does not already
set; the stored body is written by code and never edited.

- Deterministic page key: files derive it from the filename, inline
  text from its leading Markdown heading (headless inline text is
  rejected — pass it as --file instead).
- Idempotent: re-running the same body is a no-op; a different body at
  the same key fails loudly rather than overwriting.
- Works with llm.provider.backend: none, deriving a degraded summary
  from the heading or first sentence.
- Existing frontmatter (including unmodeled fields like effective_date)
  passes through untouched; --connection-id scopes the page.

* feat(cli): SQL-authoring craft and per-dialect notes tool for the analytics skill

Spec 07: add a dialect-agnostic <sql_craft> block to the ktx-analytics skill (schema discovery, composition, window-function correctness, numeric precision, answer completeness) with one worked window-then-filter example. Workflow steps gain pointers into it; existing guidance is unchanged.

Spec 08: add a read-only sql_dialect_notes MCP tool returning a connection's engine SQL conventions (FQTN form, identifier quoting/case, date/time, top-N idiom, JSON access), resolved through the existing sqlAnalysisDialectForDriver path. Notes are per-dialect markdown files under context/sql-analysis/dialects, served by the tool and copied to dist (package-internal, never installed). Non-SQL connections return a clear KtxExpectedError. The flat skill gains a one-line pointer to the tool.

Both spider2-specs intake drafts move to done/ with implementation notes.

* feat(cli): tolerate objects that fail introspection during scan

Isolate per-object introspection failures so one broken or inaccessible object no longer zeroes out a connection's whole semantic layer: the sqlite and bigquery connectors introspect each object defensively (tryIntrospectObject), the live-database adapter records a scan outcome and fetch report, and enabled_tables accepts catalog.db.name, db.name, or bare names with a clear no-match error. Includes matching ktx-daemon introspection changes, docs, and tests.

* docs(spider2-specs): add 06-scan-tolerate-broken-objects spec

* feat(cli): generalize analytics fan-out rule to multi-hop join chains

The ktx-analytics skill's fan-out rule only reliably caught single-hop
inflation; agents still silently fanned out on multi-hop chains where the
offending one-to-many join sits several hops below the SUM/COUNT and is easy
to miss.

Rewrite the Composition rule so the danger reads as cumulative across the whole
chain (pre-aggregate per measure-owning table), add an affirmative
grain-verification habit (default: pre-aggregate to grain; escape hatch:
COUNT(DISTINCT key) for pure counts only; SUM/AVG of a fanned-out measure must
pre-aggregate), and add one generic wrong-vs-right worked example. Content-only
and dialect-agnostic; no new tool, flag, or config.

Implements spider2-specs/specs/09 and annotates spec 07's one-example
constraint as superseded.

* feat(cli): add panel-completeness, time-series window, and text-encoded numeric SQL craft

Extend the analytics skill's <sql_craft> with three correctness habits and
route the dialect-specific halves through sql_dialect_notes:

- Panel completeness (spec 10): full-domain spine -> LEFT JOIN -> COALESCE for
  "each/every/all/per" questions, defaulted by measure additivity.
- Time-series windows (spec 11): explicit cumulative frames, calendar-range
  rolling windows with minimum-periods guards, and period-over-period via LAG.
- Text-encoded numerics (spec 12): sample distinct values, strip/scale/cast in
  one early CTE, and confirm coverage with a failure-detecting cast.

Add per-dialect Series, Rolling window, and Safe cast notes to all seven
dialect files so the skill stays dialect-agnostic while the engine-specific
syntax lives in sql_dialect_notes. Tests updated and passing (19).

* docs(spider2-specs): add specs 10-12 for analytics SQL-craft additions

Refined specs and completion records for the panel-completeness spine (10),
time-series window recipes (11), and text-encoded numeric parsing (12)
implemented in the preceding commit.

* docs(spider2-specs): add backlog intake drafts 13-14

- 13: canonical authoritative-source measures
- 14: output-completeness final check

* skill(analytics): spec 14 output-completeness + iter1 (active column planning)

Bundles two changes (entangled in SKILL.md; future spider2 iterations land as
separate commits):

- spec 14 (output-completeness): multi-part "answer every requested output" rule
  + a "Final completeness check" in workflow Step 6 and <sql_craft>; analytics
  skill-content test updated; intake draft -> done/, refined spec added.
- iter1 experiment: spec 14's passive end-check did not change behavior on the
  benchmark's output-completeness failures, so (a) the Plan step now writes the
  exact output-column list UP FRONT as a contract the final SELECT must match,
  and (b) "expose identity" -> "project BOTH the entity id and its name" (covers
  both omission directions). All generic craft.

Driven by the Spider 2.0-Lite failure analysis (incomplete output was the
largest failure bucket); benchmark only as motivation.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* skill(analytics): iter2 — deterministic order in string/array aggregation

GROUP_CONCAT/string_agg/array_agg element order is undefined without an explicit
ORDER BY; also note SQLite's default text sort is binary/case-sensitive (uppercase
before lowercase) vs case-insensitive (COLLATE NOCASE). Generic SQLite craft.

Spider 2.0-Lite motivation: an ordered-ingredient-list question failed only on the
within-string element order (right elements, wrong order); benchmark as motivation only.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* feat(mcp): structured, leveled logging for the MCP server

Add one synchronous pino logger per MCP server process, written through the
io.stderr sink: plain JSON when stderr is not a TTY, colorized pino-pretty
(sync, in-process) when it is. Every tool call logs tool.start with its raw
params BEFORE the handler runs and tool.end after (info / warn past
KTX_MCP_SLOW_TOOL_MS / error), correlated by callId plus sessionId, so a
runaway sql_execution leaves a recoverable start line with its exact SQL and
no matching end. HTTP logs session.open/close and wires the previously-dead
transport.onerror to transport.error; stdio routes its transport error
through the logger. Level via KTX_MCP_LOG_LEVEL (default info). Existing
mcp_request_completed telemetry and registerParsedTool are unchanged; no
worker/async transport and no redaction in v1 (logs are local-only).

Implements spider2-specs/specs/15-mcp-server-structured-logging.md and moves
the intake draft to done/.

* feat(mcp): report uptimeMs in MCP server /health

The /health endpoint now includes uptimeMs (monotonic elapsed time since
the server started), mirroring the Python daemon's uptime_ms telemetry
field.

* feat(cli): bound read-query execution with a per-connection deadline

Enforce one shared query deadline (default 30s, overridable per connection via
query_timeout_ms) on every executeReadOnly path, so an accidentally-expensive
LLM-authored query returns a fast "query exceeded Ns" KtxQueryError instead of
hanging the MCP server.

- New shared contract context/connections/query-deadline.ts
  (resolveQueryDeadlineMs, queryDeadlineExceededError); query_timeout_ms added to
  the shared warehouse schema; BigQuery's job_timeout_ms removed.
- SQLite runs the read query in a short-lived forked child process and enforces
  the deadline with SIGKILL. worker_threads + terminate() was tried first but
  cannot interrupt a synchronous better-sqlite3 scan (the native loop never
  yields); SIGKILL reclaims the process in ~2ms and keeps the event loop free.
- Remote connectors apply a real server-side statement timeout and re-wrap their
  own timeout signal as KtxQueryError: Postgres statement_timeout/57014, MySQL
  max_execution_time/3024, Snowflake STATEMENT_TIMEOUT_IN_SECONDS/604, ClickHouse
  max_execution_time + aligned request_timeout/159, SQL Server requestTimeout/
  ETIMEOUT, BigQuery jobTimeoutMs.
- Relationship validation skips a candidate to review on a deadline timeout
  instead of aborting the pass; the deadline surfaces through the existing MCP
  pino logger as a matched tool.start/tool.end(error) pair (no new logging code).

Also fixes a pre-existing, unrelated invalid cast in mcp-server-factory.test.ts
that was breaking tsc -p tsconfig.test.json.

* docs(spider2-specs): mark spec 16 (bounded query execution) done

Append Implementation notes to the refined spec (what shipped, where, and the
worker-thread -> child-process+SIGKILL deviation with its evidence) and move the
intake draft from todo/ to done/.

* skill(analytics): iter3 — measure-as-amount, inter-event gap, top-per-metric career

Three generic interpretation rules: a named business measure (sales/revenue/spend)
means its amount not a row count; "inter-event duration/gap" is LAG/LEAD time-between
events not a magnitude column; "highest across several achievements" aggregates per
metric over the whole history. All three demonstrably FIRE (verified on local008/003/152
SQL). local008 flips to correct (mechanism-aligned). 003/152 still fail on a different
axis (source-column / grouping). Generic craft; benchmark only as motivation.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* skill(analytics): spine-for-extreme-selection + aggregate-over-selected-set

Two generic answer-completeness refinements:
- Selecting the extreme group (lowest/highest count over a period/category
  domain) must rank over the COMPLETE spine, not only groups with fact rows —
  an empty period is a genuine 0 and often the true minimum.
- An aggregate scoped to a per-entity selected set ('avg revenue per actor in
  those top-3 films') is computed ACROSS that set, distinct from the per-item
  value; project both.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* skill(analytics): iter2 — sharpen extreme-selection spine + top-N ranking-measure

- spine-for-extreme: concrete cue that a zero-row period never appears in a
  GROUP BY of the facts; generate the full calendar, LEFT JOIN, COALESCE, then rank.
- aggregate-over-selected-set: top-N selection ranks by the named ranking measure
  (the item's own revenue), independent of the per-item share that feeds the aggregate.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* skill(analytics): iter3 — comparison-between-two-extremes is one wide row

Distinguishes a cross-item comparison ('the difference between the highest and
lowest month' -> single wide row, both extremes side by side + the comparison
column) from 'report a metric for each group' (-> stays long). Generic, question-
derived; targets the wide-vs-long shape gap without affecting per-group long output.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* skill(analytics): iter4 — anchor a period bucket to the named lifecycle event

When a record carries multiple lifecycle timestamps (created/placed, approved,
shipped, delivered, completed, settled) and the question counts/measures records
in a named *completed state* by period ("delivered orders by month", "shipped
items per week"), bucket the period by that named event's own timestamp, not the
record-creation timestamp; the state value is the qualifying filter, the matching
timestamp is the time anchor. Wording priority is explicit — purchased/placed/
created/submitted/ordered keep the start-event timestamp — and a non-temporal
state filter (counts by customer/city/seller with no period) introduces no anchor.

Generic analytics craft: counting completed-state records by their creation date
silently answers "records that later reached that state, grouped by when they
started" instead of the question asked. Surfaced via the spider2-autofix loop;
FAIR_PRODUCT (adversary-screened, restatable from question wording + schema/
semantic-layer lifecycle descriptions, no gold dependency).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* skill(analytics): iter5 — canonicalize observed URL-path variants before page-level analysis

When a question groups/filters/sequences web pages by a path/url column, sample
its distinct values; if the data itself shows /route and /route/ variants for the
same page context, canonicalize in an early CTE (preserve / as root, strip trailing
slashes from non-root paths, map an observed empty path to / only when the column is
a URL path with blank root-page events) and use the canonical path everywhere above.
Explicitly forbids inventing aliases the data doesn't show: no merging different
route names, no stripping query/fragment/host/scheme, no lowercasing, and no
canonicalization when the question asks for raw URL/path or slash-vs-no-slash diffs.

Generic web-analytics craft: raw request logs routinely store the same user-visible
page with and without a trailing slash, so grouping raw labels silently splits one
page into several. Surfaced via the spider2-autofix loop (Codex runner, round r2);
FAIR_PRODUCT (adversary-screened, restatable from URL-path semantics + page-grain
question wording + solver-observed distinct values, no gold dependency). The rule
fired mechanism-aligned on both targets; flipped local330 (landing/exit page counts),
local331 residual is a separate sequence-semantics axis beyond canonicalization.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* skill(analytics): iter6 — coverage over a selected group is a set-membership aggregate

When a question first selects a group of entities ("the top 5 actors", "these
products") and then asks what count/share/percentage of a DIFFERENT subject domain
relates to *these* selected entities ("what % of customers rented films featuring
these actors"), the subject set is the UNION across the whole group: count DISTINCT
subject ids once across the selected entities and return one collective value at the
subject-domain grain — not one row per selected entity (which double-counts subjects
related to more than one entity and answers a different question). Narrowly guarded:
emit one row per entity only when the wording says "for each / per / by / list" or
asks for each entity's own metric ("top 5 players and their batting averages").

The collective-coverage cousin of the existing per-entity selected-set rule. Generic
analytics craft (per-entity metric vs set-level coverage). Surfaced via the
spider2-autofix loop (Codex runner, round r3); FAIR_PRODUCT (adversary-screened,
restatable from wording alone, no gold dependency). Flipped local195 mechanism-aligned
(union COUNT(DISTINCT customer)/total, one scalar); 0 regression across 5 passing
per-entity top-N guards (local023/024/029/212/221 stayed long).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* skill(analytics): label-only joins must LEFT JOIN — incomplete dims silently drop fact rows

Mirror of the existing fan-out rule for the DROP direction: an inner JOIN to a
dimension table used only to attach a display attribute silently discards every
fact row whose key has no parent when the dimension is incomplete (trimmed
catalogs, late-arriving / SCD-gap rows), shrinking counts/sums and the universe
over which shares/averages/medians are computed. Guidance: LEFT JOIN pure
enrichment; inner-join a dimension only when intended as a filter; key the
aggregate/GROUP BY on the fact column, not the dimension column.

Spider2 autofix round 'joindim': flips complex_oracle local050 (FAIL->PASS,
official scorer) — solver dropped the gratuitous products inner-join and
recovered the exact gold. local060/063 also adopt LEFT JOIN (rule fires) but
remain gold-convention-blocked. Guards local061/067 held.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* docs(spider2-specs): add todo/17 — lifecycle-event metrics (semantic-layer)

Draft intake spec surfaced by the spider2-autofix loop (round r1): the model-layer
form of the shipped iter4 lifecycle-date-anchoring skill rule — infer per-state
lifecycle-event metrics (e.g. delivered_orders with defaultTimeDimension = the
delivery timestamp) during enrichment so the correct time anchor is the default for
any consumer, not only an agent that loaded the skill. Generic; FAIR_PRODUCT.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(connectors): accept leading underscore in connection/identifier ids

The safe-identifier validator regex /^[a-zA-Z0-9][a-zA-Z0-9_-]*$/ allowed an
underscore everywhere except the first character, so a connection id / database
name that legitimately starts with '_' (valid in Snowflake, e.g. _1000_GENOMES)
could never be ingested or queried. Allow a leading underscore across all 16
duplicated validators (connection ids, source ids, page/wiki keys, warehouse-
verification tool schemas). Path-safety is unaffected — '.' and '/' remain
excluded, and assertSafePathToken still blocks traversal.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* feat(analytics): generic geospatial query guidance

Add a Snowflake ST_* dialect note (ST_MAKEPOINT lon-first, ST_DWITHIN/ST_CONTAINS/
ST_WITHIN/ST_INTERSECTS, bbox->polygon via ST_MAKEPOLYGON/ST_MAKELINE) and a
dialect-agnostic 'Spatial predicates' recipe in the analytics skill (resolve the
entity geometry, build an area-of-interest polygon, test with the engine's
containment/proximity/overlap predicate; mind lon/lat argument order). Steers the
solver off hand-rolled lat/lon BETWEEN boxes toward correct, index-assisted
geospatial predicates.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* feat(analytics): parse code/dependency text by language grammar

Add two generic <sql_craft> rules: (1) parse imported/required/loaded packages by
the language or manifest format (Java import keep-package-path allowing underscores/
mixed-case; Python import/from + alias stripping; R library/require; .ipynb parse
JSON cell source before language rules; JSON manifests flatten the dependency object
keys), stripping comments/prose and splitting multi-import lines; (2) on a
de-duplicated table with a documented copy/occurrence count, choose COUNT(*) vs the
weight column from the population the question names, not silently. Steers off one
broad regex that drops valid identifiers and matches prose.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* feat(analytics): source filters/dates/measures from the owning fact grain

Add a <sql_craft> rule for joined fact tables at different grains (parent order
vs child line item): read each predicate, calendar bucket, and measure from the
table whose grain the question names, not whichever is in scope post-join. An
order-grain filter ("orders that are Complete", "the order's creation date")
must come from the parent even though the child carries its own status/created_at;
line price/cost come from the child. Mirror at metric grain: don't combine a
parent-grain count with child rows (num_of_item * SUM(line_price) per line) —
aggregate each measure at its own grain before combining.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* feat(analytics): collapse multi-valued classes to one representative per entity before counting/concentration

When an entity carries a multi-valued classification array (IPC/CPC codes, tags)
and the methodology counts entities-per-class or a concentration/diversity metric
(HHI, originality, share), pick ONE representative per entity first (the array's
main/primary/first flag, else a defined fallback like most-frequent), then
aggregate; and use COUNT(DISTINCT entity) when the denominator is defined as a
count of entities. Unnesting the array otherwise multiplies an entity's weight by
its code count, inflating per-class frequencies and skewing the ranking/score.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* feat(connectors): introspect BigQuery datasets hosted in foreign projects

A dataset_ids/dataset_id entry may now be written `project.dataset` to
introspect a dataset hosted in another project while query jobs still bill to
credentials.project_id. Entries are parsed once at the config boundary into
canonical {project, dataset} pairs; introspection, primary-key discovery,
testConnection, getTableRowCount, and listTables (grouped per project) all
resolve in the dataset's own project, and scanned tables are labeled with that
project so sampling, distinct-value, and read queries resolve. Bare entries are
unchanged.

Implements spider2-specs/specs/18-bigquery-cross-project-datasets.md.

* feat(scan): durable, resumable, bounded relationship detection during enrichment

Move the enrichment persistence boundary to the cost boundary and bound the
open-ended relationship stage (spec 19).

- Checkpoint descriptions + embeddings into the queryable `_schema` manifest
  (and the raw enrichment artifacts) before relationship detection runs, via a
  new `onCheckpoint` hook + `writeLocalScanEnrichmentCheckpoint`. An interrupted,
  budget-truncated, or failed relationship stage now degrades to "no joins",
  never "no descriptions".
- Resume the enrichment cache by content identity: re-key the SQLite stage store
  on `(connection_id, stage, input_hash)` so a re-run with a fresh runId resumes
  finished descriptions/embeddings instead of re-paying for LLM work. The
  disposable cache recreates its table if the on-disk key shape differs.
- Make the relationship stage observable and bounded: a sticky wall-clock budget
  (`scan.relationships.detectionBudgetMs`, default 600000 ms) + per-unit progress
  + honored `ctx.signal`, threaded through profiling, validation, and composite
  detection. On exhaustion/abort it stops scheduling, finalizes, and returns a
  partial result instead of throwing or hanging.
- Mark a budget/abort-truncated result partial (diagnostics `partial`/`partialReason`
  + recoverable `relationship_detection_partial` warning). A graceful partial saves
  as a completed stage and resumes cheaply; raising the budget changes inputHash
  and forces a fresh, fuller run. A process killed mid-stage saves nothing.

Document `detectionBudgetMs` in the ktx.yaml reference. Append implementation
notes to specs/19 and move the intake draft to done/.

Also carries the in-tree per-table enrichment LLM timeout work it builds on
(`description-generation.ts` + the `enrichment_timeout` warning code), which is
intertwined in `local-enrichment.ts`/`types.ts` and cannot be split into a
separately-building commit.

* feat(scan): bound + retry the per-table enrichment LLM call

The batched table-description call had no retry (sampleTable retried 3x, this did
not), so a single transient backend error (e.g. an overloaded/burst rejection when
many tables enrich concurrently) silently nulled a whole table's descriptions —
observed dropping ~70% of a db's tables during a bad window despite ample quota.

- Wrap generateObject in retryAsync (3 attempts + backoff; KTX_ENRICH_LLM_ATTEMPTS).
- Fresh per-attempt timeout (KTX_ENRICH_LLM_TIMEOUT_MS, default 120s) still bounds a
  wedged wide table; a timeout is surfaced as KtxAbortedError so it is NOT retried
  (one wedge stays one timeout, not 3x).
- Granular per-table progress + start/done/retry/timeout logging.

Composes with spec 19 (its non-goal #1): spec 19 makes completed descriptions durable;
this makes more of them complete.

* feat(scan): survive a hung LLM enrichment backend and resume descriptions

Two compounding failure modes on the per-table description-enrichment path (spec 20):

Enforced per-table timeout for subprocess backends. The runtime declares whether it owns an SDK subprocess (subprocessForkSpec on KtxLlmRuntimePort); codex/claude-code calls run behind a ktx-owned detached child that is tree-killed (SIGKILL of the process group on POSIX, taskkill /T on Windows) on the deadline or ctx.signal, reaping the wedged model grandchild. HTTP backends keep native fetch abort. Default stays 120s, one-wedge-one-timeout.

Incremental, resumable descriptions persistence. generateDescriptions flushes enriched tables per batch to an inputHash-tagged durable record (at a stable, non-syncId path) plus only the changed manifest shards, skips already-enriched tables on resume, and never lets one table's failure discard the stage (a skipped table costs one missing description, not the whole stage's output).

Spec 20 refined + intake draft moved to done/.

* feat(scan): selective enrichment stages (--stages) + per-stage cache keys

Split the single coarse enrichment cache key into per-stage hashes
(descriptions <- snapshot + LLM identity; embeddings <- snapshot + embedding
identity + description digest; relationships <- snapshot + relationship settings
+ LLM identity), so changing one stage's inputs invalidates only that stage and
never throws away the expensive per-table descriptions on an unrelated edit.

Add `ktx ingest --stages <list>` to force-re-run a chosen subset on an
already-ingested connection: a named stage bypasses the completed-stage
short-circuit while the per-table descriptions resume record still skips
already-enriched tables, and unselected stages are left untouched on disk. Feed
embeddings + relationships their description context from the on-disk _schema
when descriptions do not run this invocation, and carry descriptions into the
llmProposals evidence packet (closing a latent gap on the full-run path too).
Surface an enrichment_stage_stale warning when an unselected stage's inputs have
drifted, rather than silently cascading the work.

Implements spider2-specs/specs/21-selective-enrichment-stages.md.

* test(analytics): realign SKILL.md acceptance test with the evolved skill

Three assertions in analytics-skill-content.test.ts drifted from the analytics
SKILL.md as later iterations edited the skill without updating the test:

- the sub-heading was renamed Window functions -> Ordering & aggregation
  determinism (iter2), so follow the source name;
- the rule "Expose identity, not just the label" was renamed to "Project BOTH
  identity and label" (spec 14), so match the new wording;
- the dialect-FQTN guard false-positived on the Java package example
  com.planet_ink.coffee_mud, whose backticks made a 3-segment package path read
  as a BigQuery/Snowflake `a.b.c` table reference. Drop the backticks so the
  guard stays at full strength without weakening it.

* fix(scan): --stages subset must not delete unselected stages' on-disk artifacts

A --stages subset that omitted descriptions wiped all on-disk ai/db descriptions
from the written _schema. runLocalScan writes the structural manifest shard from
the bare snapshot BEFORE enrichment runs, and the shard merge treats ai/db as
scan-managed and overwrites them with whatever the run emits — none, on a subset
that skips descriptions. Enrichment then read the already-wiped shard via
loadPriorDescriptions and had nothing to restore.

runLocalScanEnrichment now returns the best-available descriptions (fresh-this-run
if descriptions ran, else loaded from the on-disk _schema) instead of [], and
runLocalScan captures the prior descriptions before the structural write and feeds
them to both the structural write and enrichment, so an unselected stage's
artifacts survive. Joins were already preserved for --stages descriptions via the
manual/inferred preservedJoins path.

Tests: a full runLocalScan --stages relationships path test (RED without the fix,
GREEN with it — the earlier unit test missed the structural-pre-write ordering),
plus enrichment-layer contract tests for both directions. Validated live on
northwind: --stages relationships keeps all 110 descriptions + 22 joins (was
wiping to 0); --stages descriptions restores descriptions from the spec-20 resume
record (no LLM calls) while keeping joins.

* feat(dialects): bigquery nested-data (ARRAY/STRUCT/UNNEST), geospatial (GEOGRAPHY), SAFE_DIVIDE

bigquery.md lacked the two sections that define BigQuery analytics (present in snowflake.md):
- Nested & repeated data: UNNEST to flatten arrays of STRUCTs (GA360 hits, GA4 event_params),
  dot-notation field access, key-value param scalar-subquery extraction, fan-out/COUNT(DISTINCT) guard.
- Geospatial (GEOGRAPHY): ST_GEOGPOINT (lon-first), containment/proximity/distance/intersection
  predicates, areal allocation via ST_AREA(ST_INTERSECTION()).
- SAFE_DIVIDE for zero-denominator-safe rates; sharded-table shard-presence note.
Generic BigQuery craft surfaced by sql_dialect_notes; product-completeness (any BQ analyst benefits).

* feat(dialects): sqlite ROUND half-up FP-underflow note (+1e-9 before ROUND)

SQLite ROUND(x,n) rounds half-away-from-zero, but binary FP stores an exact
half-way value just below it, so ROUND(6.475,2) returns 6.47 not 6.48. Add a
dialect note: nudge by a tiny epsilon (1e-9) below display precision before
rounding for deterministic half-up, leaving non-boundary values unchanged.
Generic SQLite craft surfaced by sql_dialect_notes (any analyst rounding a
displayed average/rate/price benefits).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* docs(analytics): list-as-delimited-string, answer-literally, drop free-text columns

Add SKILL.md guidance to emit list-valued answer cells as delimited
STRING (not ARRAY/repeated column), answer the literal ask without
unrequested transformations (HAVING for aggregate bounds), and avoid
projecting unrequested free-text columns that corrupt row-delimited output.

* fix(scan,mcp): gitignore runtime logs, budget-guard LLM proposal, validate enrich timeout

- gitignore `.ktx/logs/` in both scaffold + setup-merge lists: the managed MCP
  daemon writes raw tool params (SQL, memory_ingest content) to mcp.log under a
  version-controlled `.ktx/`, and snowflake.log already sat there unprotected.
- gate the LLM relationship proposal on the detection budget/abort signal so an
  exhausted or aborted stage cannot start a fresh LLM call; document the boundary.
- validate KTX_ENRICH_LLM_TIMEOUT_MS (NaN/0 → 120s default) like enrichAttempts,
  so a bad value no longer times out every table immediately.
- daemon introspection now warns on malformed column/FK rows instead of dropping
  them silently, matching the table-row path and the "surface broken objects" goal.
- docs: document `ktx wiki -c/--connection`; fix the SQLite query-deadline schema
  doc (forked-subprocess SIGKILL, not worker-thread termination).

* fix(scan,wiki,mcp): address PR #312 review findings

- scan: key the description pipeline (resume map, enriched-schema and
  embedding-text lookups, manifest write/read) by full table identity via
  tableRefKey/buildTableRef, so two same-named tables in different schemas no
  longer cross-assign descriptions or skip a sibling on resume
- scan: re-throw a genuine context cancel during the batched description LLM
  call so Ctrl-C resumes the stage instead of nulling tables and recording it
  completed; per-table timeouts still degrade (context.signal not aborted)
- scan: report statisticalValidation 'skipped' (not 'completed') when a
  budget/abort stop leaves relationship profiling partial
- wiki: sync the full page corpus into the sqlite index and filter only the
  candidate/result set, so a connection-scoped search no longer prunes other
  connections' pages and cached embeddings from the shared index
- wiki: route verbatim ingest through the canonical writePageAndSync so
  contentHash is set and later syncs can short-circuit
- mcp: drop the as-unknown-as cast in serializeMcpError
- dialects/analytics: document the integer-division trap on postgres/sqlite/tsql

Adds regression tests for each behavior change.

* fix(wiki): scope connection filter before SQLite lane limit

Connection-scoped wiki search applied the connectionId allowlist after
the lexical/semantic lanes had already truncated to laneCandidatePoolLimit
over the full (connection-agnostic) corpus. When the requested connection
was a minority of a large corpus, its pages were crowded out of the
candidate pool before filtering, so a semantic-only match could be missed
outright and lexical hits under-ranked.

Push the path allowlist into searchLexicalCandidates/searchSemanticCandidates
so LIMIT applies to in-scope rows, matching what the token lane already did,
and drop the now-redundant post-limit JS filters.

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
2026-06-29 16:35:57 +00:00

493 lines
30 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# Bounded query execution (deadline + non-blocking) for read SQL
> Refined spec. Intake draft: `todo/16-bounded-query-execution-timeout.md`.
>
> **Scope: bound and cancel a read query that runs too long.** This is the
> execution-model companion to spec 15 (MCP structured logging). Spec 15
> *surfaces* a runaway query in the log; it explicitly defers *preventing* one —
> "off-event-loop execution, query timeouts, worker-thread isolation … is
> execution-model work in a separate spec." This is that spec.
## Problem
Two compounding gaps on the read-query path (`executeReadOnly`), confirmed in the
current code:
1. **No execution deadline, handled divergently per connector.** A single
expensive query runs unbounded, and whether it is bounded at all depends
entirely on which driver the caller hit:
- **BigQuery** is the only connector with a real statement timeout — it sets
`jobTimeoutMs` on the query job from a per-connection config field
`job_timeout_ms` (`connectors/bigquery/connector.ts`, `query(...)` ~491512).
- **ClickHouse** sets a hardcoded 30s *HTTP* `request_timeout` at client
creation (`connectors/clickhouse/connector.ts:602`) — a client-side give-up,
not a server-side `max_execution_time`; the server keeps working.
- **Snowflake, Postgres, MySQL, SQL Server** bound only pool/connection
*acquisition* (Snowflake `acquireTimeoutMillis: 60_000`; Postgres
`connectionTimeoutMillis: 10_000`; SQL Server `idleTimeoutMillis: 30000`;
MySQL pool size only) — nothing bounds statement *execution*.
- **SQLite** has nothing.
2. **In-process SQLite blocks the event loop and cannot be cancelled.** The
SQLite connector executes on the main thread via synchronous
`better-sqlite3 .prepare().all()` (`connectors/sqlite/connector.ts`,
`query(...)` 311318, used by `executeReadOnly` 247251). A slow query freezes
the whole MCP server — it cannot serve other requests, send progress, or write
`tool.end` — and there is no in-thread way to interrupt it: better-sqlite3 (v12)
exposes no interrupt/cancel API. Its documented mechanism for slow queries is a
**worker thread**, and the only way to stop a runaway synchronous query is to
**terminate the thread** executing it (context7 `/wiselibs/better-sqlite3`,
`docs/threads.md`).
The observed failure (Spider2-lite sqlite run, 2026-06-18): a single
`sql_execution` MCP call —
`SELECT MIN(time_id), MAX(time_id), COUNT(*) FROM profits` on `complex_oracle`,
where `profits` is a VIEW (`costs ⋈ sales`, 918,843 × 82,112 rows, joined on a
4-column key with no composite index) — degraded to an O(N×M) nested-loop scan,
pegged a worker at 100% CPU for 13+ minutes, never returned, produced a
`tool.start` with no matching `tool.end`, and stalled an eval shard until the
worker was killed by hand. A row cap (`maxRows`) does not help: it bounds returned
rows, not scan work, and the failing query returned a single aggregate row.
## Generic use case (independent of any benchmark)
Any data agent that lets an LLM author SQL will eventually issue an
accidentally-expensive query — an unindexed or cartesian join, an expensive VIEW,
a wide aggregate over a large fact table. A general-purpose context layer must
bound that and return a clean, fast "query exceeded Ns" error so the agent can
revise (add filters, query base tables, narrow the range) instead of hanging the
tool and the server. This matters for embedded/local warehouses (SQLite, and any
future DuckDB-style in-process driver) and remote ones alike, and is wholly
independent of any benchmark.
## Design decisions (resolved during refinement)
These resolve ambiguities the intake draft left open. They constrain the
implementer; the exact code is theirs.
### One canonical deadline, applied uniformly at the contract
The deadline is enforced for **every** `executeReadOnly` caller, not only the MCP
`sql_execution` path. `executeReadOnly` has 13 call sites beyond MCP (ingest query
executor, relationship profiling and composite-candidate probes, relationship
validation, historic-SQL probes, `ktx sql`); the contract is the single place to
bound all of them. A heavy ingest profiling probe over a giant unindexed join is
exactly as worth abandoning as an interactive one — those call sites are
best-effort and degrade gracefully, so a deadline `KtxQueryError` becomes "skip
this probe / mark unprofiled," not "fail the source." (Requirement 8 covers the
call sites that must treat the timeout as recoverable.)
> Rejected alternative: a caller-resolved deadline (short on the interactive path,
> longer/none for ingest). That introduces a second value source and the open
> question "what is the ingest budget," for no real gain — the 30s default already
> clears any normal profiling probe, and a probe that exceeds it is one to drop.
### Default 30s, configurable per-connection via one shared field
- **Default `30_000` ms.** Fast enough that an LLM agent gets a clean
"exceeded 30s" and revises within the same turn; generous headroom over any
indexed aggregate or normal profiling probe; a genuine pathological nested-loop
scan blows past it immediately.
- **One shared per-connection override**, honored by every connector:
`query_timeout_ms` in `ktx.yaml` (`queryTimeoutMs` in TS), a positive integer
in **milliseconds**. Milliseconds matches the BigQuery SDK and the field it
replaces; the user-facing error still reads in seconds.
- **BigQuery's `job_timeout_ms` config key is removed**, not kept alongside the
new field. BigQuery reads the shared `query_timeout_ms` and maps the resolved
value onto its SDK's `jobTimeoutMs`. ktx keeps no backward compatibility, so
there is exactly one way to set a query timeout — no parallel knob (intake
requirement 1).
- **Granularity is per-connection only.** No global all-connections override —
different warehouses have different performance envelopes, and a second
(global) knob would double the configuration surface for no stated need.
### The shared contract is a value + an error, not a base class
There is **no shared connector base class or factory** — each connector is
constructed independently; the only shared registry is the *dialect* factory
(`context/connections/dialects.ts:4755`). So "defined once" (intake requirement
3) means a single shared module that owns:
- `DEFAULT_QUERY_TIMEOUT_MS = 30_000`;
- `resolveQueryDeadlineMs(connectionConfig)` → the validated `query_timeout_ms`
override, else the default — so the default and the override precedence live in
exactly one place;
- `queryDeadlineExceededError(deadlineMs)` → a `KtxQueryError` with the canonical
message `query exceeded ${Math.round(deadlineMs / 1000)}s`.
Each connector calls the resolver once (at construction; connectors already
receive their connection config) and stores `this.deadlineMs`. **Enforcement is
necessarily per-connector** — different engines cancel differently — but the
*value* and the *error message* are shared, so the agent sees one consistent,
actionable error regardless of driver.
### Real cancellation, not client-side give-up
Per intake requirement 5, the deadline must *stop the work*, not merely abandon
the promise while the query keeps running (which on a pooled driver also risks
returning a still-busy connection to the pool). So:
- **In-process (SQLite, and any future embedded driver):** run the query off the
main thread and enforce the deadline by **terminating the worker thread**. There
is no generic `Promise.race` outer wrapper — a `Promise.race` against a
synchronous in-thread `.all()` can never fire (the loop is blocked), and against
a pooled remote query it would poison the pool. Thread termination *is* the
cancellation.
- **Remote engines:** set the engine's **server-side statement timeout** so the
server itself aborts the query and frees the connection cleanly.
### Logging routes through spec 15's pino path — no second logger
The deadline cases are logged through the **existing** MCP tool-call logger
(spec 15's `instrumentMcpServer`, `context/mcp/context-tools.ts:644730`), not a
new logging path threaded into the connector. Verified flow for a timeout:
`executeReadOnly` throws `queryDeadlineExceededError` (a `KtxQueryError`) →
`local-project-ports.ts` preserves it → `registerParsedTool` (:552) reports it
(`reportException` skips `$exception` for `KtxExpectedError`) and returns an
in-band `isError` result → `instrumentMcpServer` writes `tool.end` at **`error`**
with `outcome:"error"`, `err.message = "query exceeded {N}s"`, and the **same
`callId`** as the `tool.start`.
This is the central observability win and it requires **no new MCP logging code**:
spec 15 made a hang show up as a `tool.start` with *no* matching `tool.end`; this
spec turns it into a **matched `tool.start` → `tool.end(error)` pair** whose
`tool.end` names the deadline. The worker-termination (SQLite) and server-side
abort (remote) are internal enforcement mechanisms; their single observable signal
is that `tool.end`, so the connector does **not** get its own logger threaded
through `KtxScanContext` — that would fork a second path for one capability. The
"worker was actually reaped, not left spinning" guarantee is asserted by the
worker's `exit` event in tests (Requirement 3), not by a log line.
## Requirements
### 1. Shared deadline contract, defined once
A single new module (e.g. `packages/cli/src/context/connections/query-deadline.ts`)
exports `DEFAULT_QUERY_TIMEOUT_MS` (30_000), `resolveQueryDeadlineMs(connectionConfig)`,
and `queryDeadlineExceededError(deadlineMs)`. Every connector resolves its
deadline through this resolver; no connector hardcodes its own default or
duplicates the override-precedence logic.
### 2. Shared per-connection config field; BigQuery's removed
`query_timeout_ms` is added to the **shared** connection config schema (validated
as an optional positive integer, milliseconds) so every driver accepts it. The
BigQuery-specific `job_timeout_ms` config field and its dedicated reader
(`bigQueryJobTimeoutMsFromConnection`) are removed; BigQuery sources its timeout
from the shared field and applies it as `jobTimeoutMs`. A bad `query_timeout_ms`
(zero, negative, non-integer) is a clear config validation error, consistent with
how ktx validates `ktx.yaml`.
### 3. SQLite executes off the main thread, terminated on deadline
`executeReadOnly` on the SQLite connector MUST NOT block the MCP server event
loop:
- Read-only validation and the row-limit wrapper (`assertReadOnlySql` +
`limitSqlForExecution`) run **on the main thread** before dispatch — invalid SQL
fails instantly without spawning a worker, and read-only enforcement stays at
the boundary (Requirement 7).
- The validated, row-limited SQL (and any params) is dispatched to a **worker
thread** that opens the database `{ readonly: true, fileMustExist: true }`, runs
the query, and posts back `{ headers, rows, totalRows }` (all values are
structured-cloneable — primitives, `Buffer`, `BigInt`).
- The main thread arms a timer for `this.deadlineMs`; on expiry it calls
`worker.terminate()` and rejects with `queryDeadlineExceededError`. On a normal
message it clears the timer and resolves. On a worker error (SQLite rejected the
SQL) it rejects with that error, message preserved. A provided
`ctx.signal` (`KtxScanContext.signal`, already on the contract) also terminates
the worker, for external cancellation.
- **One short-lived worker per call**, terminated on completion or deadline — not
a persistent worker or pool. Terminate-on-deadline destroys the worker, so a
pool would need respawn/job-tracking for no benefit: `executeReadOnly` is
low-frequency (LLM-issued, serial per agent turn) and worker spawn cost is
negligible against query latency. The other SQLite paths (introspect, sample,
stats, distinct-values, row-count) stay on the main thread — they are
ktx-authored, bounded, and not on the `executeReadOnly` contract.
- The event loop stays responsive throughout, so `tool.end` is always written and
concurrent requests on the same port are served.
### 4. Remote engines set a real server-side statement timeout
Each remote connector applies `this.deadlineMs` as its engine's server-side
statement timeout, so the deadline stops server work rather than abandoning the
promise:
| Connector | Mechanism | Unit |
|------------|--------------------------------------------------------|---------------|
| BigQuery | `jobTimeoutMs` on the query job (replaces `job_timeout_ms`) | ms |
| Postgres | `statement_timeout` | ms |
| MySQL | session `max_execution_time` (applies to read-only SELECT — the only kind on this path) | ms |
| Snowflake | `STATEMENT_TIMEOUT_IN_SECONDS` (ALTER SESSION) | s (ceil) |
| ClickHouse | `max_execution_time` setting, with `request_timeout` aligned to the deadline so the HTTP client does not give up before the server aborts | s (ceil) |
| SQL Server | `mssql` `requestTimeout` (TDS attention cancels server-side) | ms |
ClickHouse's existing hardcoded 30s `request_timeout` is brought under this
contract (derived from the resolved deadline), not left as a parallel mechanism.
### 5. Timeout resolves as a `KtxQueryError` with the canonical message
On exceeding the deadline, the path resolves with a `KtxQueryError`
(`query exceeded {N}s`) — a finite, decision-reaching outcome, never an unbounded
hang. For SQLite the worker-termination path throws `queryDeadlineExceededError`
directly. For remote engines, each connector recognizes **its own** engine's
timeout signal (Postgres `57014`; MySQL errno `3024`; ClickHouse code `159`;
SQL Server `ETIMEOUT`; Snowflake and BigQuery timeout errors) and re-wraps it as
`queryDeadlineExceededError`, keeping the driver error as `cause`. Each connector
owns its driver's signal — there is no central denylist of error codes to
maintain.
### 6. MCP surfacing and logging via the existing pino path
The MCP `sql_execution` path already (a) maps any non-native driver error to
`KtxQueryError` (`context/mcp/local-project-ports.ts:7888`, guarded by
`isNativeProgrammingFault`), (b) reports it through `reportException`, which skips
`$exception` Error Tracking for `KtxExpectedError`, and (c) writes `tool.start`
synchronously before the handler and `tool.end` in `instrumentMcpServer`
(`context/mcp/context-tools.ts:644730`). The deadline cases MUST surface through
this path — the implementer verifies and tests them, but adds **no parallel
classification or logging path**:
- **Query exceeds the deadline (any driver):** a `tool.end` at **`error`** with
`outcome:"error"` and `err.message = "query exceeded {N}s"`, carrying the same
`callId` as the `tool.start`. Classified as an expected error, so it is absent
from `$exception` Error Tracking. The reason `tool.end` was previously missing
is solely the blocked event loop (Requirement 3); once the loop stays free and
the deadline throws, the existing instrumentation logs the matched pair — closing
spec 15's "`tool.start` with no `tool.end` = hang" gap for this case.
- **Completed-but-slow query (under the deadline, over `KTX_MCP_SLOW_TOOL_MS`):**
unchanged from spec 15 — its `tool.end` is emitted at **`warn`**. The deadline
(default 30s) and the slow threshold (default 10s) are independent knobs; a query
between 10s and 30s completes with a slow `warn`, one past 30s is killed with the
`error` above.
### 7. Read-only enforcement and `maxRows` unchanged
`assertReadOnlySql` and the `maxRows` row cap (`limitSqlForExecution`) behave
exactly as today. The deadline is additive. `maxRows` is not a substitute for it
(it bounds returned rows, not scan work).
### 8. Best-effort callers treat a deadline timeout as recoverable
The non-interactive `executeReadOnly` call sites that are best-effort —
relationship profiling, composite-candidate probes, relationship validation,
historic-SQL probes — MUST treat a deadline `KtxQueryError` as "skip this
probe / mark unprofiled" and continue, never as a source-fatal error. The
implementer confirms each such site already swallows query errors into a
graceful-skip and adds that handling where it does not, so the uniform deadline
(Requirement 1, applied to all callers) cannot abort an ingest run. A skipped
probe is logged at the skip site through that path's existing scan/ingest logger
(`KtxScanContext.logger`, `warn`/`debug`), never silently dropped — these callers
are off the MCP tool-call path, so their visibility comes from the logger they
already use.
## Acceptance criteria
- A read query that exceeds the deadline returns a `KtxQueryError`
(`query exceeded {N}s`) within roughly the deadline; the MCP worker stays
responsive (a concurrent tool call on the same server completes while the slow
query is still pending) and writes a matching `tool.end` with a non-ok outcome.
- **Logging:** a timed-out `sql_execution` produces a `tool.start` and a matching
`tool.end` (same `callId`) at `error` with `outcome:"error"` and
`err.message = "query exceeded {N}s"` — no unmatched `tool.start` remains. The
timeout does not raise a `$exception` Error Tracking event (it is a
`KtxExpectedError`). A completed query slower than `KTX_MCP_SLOW_TOOL_MS` but
under the deadline still emits its `tool.end` at `warn`. No new logger is
introduced — the lines come from the existing `instrumentMcpServer`.
- **SQLite specifically:** executing a deliberately pathological query (an
expensive VIEW or an unindexed cross join) on a fixture does not block the event
loop, is terminated at the deadline, and the worker exits (the off-main-thread
executor is killed, not left spinning) so CPU returns to idle.
- **One server-side-timeout driver (Postgres):** the connector applies
`statement_timeout` equal to the resolved deadline, and a `57014` cancellation
is mapped to the canonical `KtxQueryError`.
- `resolveQueryDeadlineMs` returns 30_000 by default, honors a `query_timeout_ms`
override, and rejects an invalid value (zero / negative / non-integer).
- **No regression:** normal fast queries return identical results; read-only
rejection still works; `maxRows` still bounds returned rows.
- The shared `query_timeout_ms` field is accepted by every connector; BigQuery's
former `job_timeout_ms` key is gone and BigQuery's timeout is driven by the
shared field.
## Non-goals
- **A row/byte/cost budget on returned data.** This spec bounds *time*, not result
size — `maxRows` already bounds rows, and BigQuery's `maximumBytesBilled` is a
separate, retained concern.
- **A global `KTX_QUERY_TIMEOUT_MS` or per-call user flag.** One opinionated
default plus a per-connection override; no per-call knob, no global knob.
- **A server watchdog that recycles the process on an unmatched `tool.start`.**
Spec 15 names this as a possible future mitigation; this spec prevents the hang
at the source, so the watchdog is out of scope here.
- **Moving SQLite introspection / sampling / stats off the main thread.** Only the
`executeReadOnly` (LLM-SQL) path needs worker isolation; the rest are bounded
ktx-authored queries.
- **Per-connection retry / backoff on timeout.** A timeout returns a clean error
for the agent to revise; ktx does not auto-retry.
- **A second logger threaded into the connector.** The deadline cases are logged
through spec 15's existing MCP tool-call logger; the connector gets no separate
pino instance and `KtxScanContext` gets no MCP-logger thread (see "Logging routes
through spec 15's pino path").
## Implementation orientation
Line numbers drift; treat these as anchors, not addresses. The implementer owns
the design.
- **Shared contract** — new `packages/cli/src/context/connections/query-deadline.ts`:
`DEFAULT_QUERY_TIMEOUT_MS`, `resolveQueryDeadlineMs`, `queryDeadlineExceededError`.
Error class is `KtxQueryError` (`packages/cli/src/errors.ts:25`).
- **Contract anchor** — `KtxScanConnector.executeReadOnly`
(`context/scan/types.ts:343`), `KtxReadOnlyQueryInput` (`types.ts:285`),
`KtxScanContext.signal` (`types.ts:176`, already present, currently unused on the
MCP path).
- **Config schema** — add `query_timeout_ms` to the shared connection config
(`context/project/config.ts`, `KtxProjectConnectionConfig` and its zod schema);
remove BigQuery's `job_timeout_ms` reader.
- **SQLite worker** — new `packages/cli/src/connectors/sqlite/read-query-worker.ts`
(constructed by path via `new URL('./read-query-worker.js', import.meta.url)`);
rework `connectors/sqlite/connector.ts` `executeReadOnly` (247251) to validate
on the main thread then dispatch to the worker with a terminate-on-deadline
timer. Reuse `normalizeQueryRows` (`context/connections/query-executor.ts`) in
the worker. Register the worker as a dynamic entry in `knip.json` (it is
referenced by path, not import) and confirm the build copies it into `dist`.
- **Remote connectors** — apply the resolved deadline and recognize the engine's
timeout signal in each `executeReadOnly` / `query(...)`:
`connectors/bigquery/connector.ts` (~491512, `jobTimeoutMs`),
`connectors/clickhouse/connector.ts` (~602/629644, `max_execution_time` +
`request_timeout`), `connectors/snowflake/connector.ts` (~354371/510534,
`STATEMENT_TIMEOUT_IN_SECONDS`), `connectors/postgres/connector.ts` (~822838,
`statement_timeout`), `connectors/mysql/connector.ts` (~774793,
`max_execution_time`), `connectors/sqlserver/connector.ts` (~812832,
`requestTimeout`).
- **MCP path + logging (verify only)** — `context/mcp/local-project-ports.ts:6988`
(error mapping), the `sql_execution` registration (~915943), and the logging in
`instrumentMcpServer` (`context/mcp/context-tools.ts:644730`, which writes
`tool.start`/`tool.end` via the spec-15 pino logger `context/mcp/logger.ts`). No
new classification or logging code; confirm the timeout flows through as an
expected error producing a matching `tool.end(error)` with the canonical message.
- **Best-effort callers** — `context/scan/relationship-profiling.ts` (~227, 275),
`context/scan/relationship-composite-candidates.ts` (~365, 440),
`context/scan/relationship-validation.ts` (~259),
`context/ingest/historic-sql-probes/bigquery-runner.ts` (~97), and the
historic-sql clients: confirm a deadline `KtxQueryError` is swallowed into a
graceful skip.
- **Tests** — a SQLite fixture with a pathological query (tiny `query_timeout_ms`
as the test seam) asserting terminate-on-deadline, event-loop responsiveness
(a concurrent promise resolves while the query is pending), and worker exit; a
Postgres test asserting `statement_timeout` is set to the resolved deadline and
a `57014` error maps to `KtxQueryError`; resolver unit tests (default /
override / invalid); regression tests for normal results, read-only rejection,
and `maxRows`. Extend the MCP logging tests (alongside spec 15's, e.g.
`test/context/mcp/server.test.ts`) to assert a timed-out `sql_execution` yields a
matched `tool.start`/`tool.end(error)` pair carrying `query exceeded {N}s`.
- After implementing, rebuild and re-link so the playground picks it up:
`pnpm run build && pnpm run link:dev`.
## Benchmark context (motivation, not a requirement)
The Spider2-lite local set loads several warehouses into SQLite, some with
expensive VIEWs over large fact tables — e.g. `complex_oracle.profits` =
`costs ⋈ sales` on `(prod_id, time_id, channel_id, promo_id)`, 918,843 × 82,112
rows, no composite index, with `promo_id` (the index the optimizer picks) being
95.5% a single value. LLM-authored profiling queries (MIN/MAX/COUNT over such a
view) trigger O(N×M) nested-loop scans. Without a deadline these hang an eval
shard for 10+ minutes; with one, the agent gets a fast error and can scope the
query instead. Improving the benchmark is a side effect; the deadline is generic
production hygiene for any agent that lets an LLM author SQL.
## Implementation notes
Implemented on branch `write-feature-spec-wiki` (ktx worktree `tallinn-v2`). All
acceptance criteria are met; tests, type-check, dead-code, and build are green
for the changed surface.
### What was built, and where
- **Shared contract** — new `packages/cli/src/context/connections/query-deadline.ts`:
`DEFAULT_QUERY_TIMEOUT_MS = 30_000`, `resolveQueryDeadlineMs(connection)` (returns
the validated `query_timeout_ms` override else the default; throws on
zero/negative/non-integer), and `queryDeadlineExceededError(deadlineMs, options?)`
(a `KtxQueryError` reading `query exceeded ${round(ms/1000)}s`, carrying the
driver error as `cause`). Unit-tested in `test/context/connections/query-deadline.test.ts`.
- **Config field** — `query_timeout_ms` (optional positive integer, ms) added to
the **shared warehouse** schema. NOTE (spec drift): that schema lives in
`context/project/driver-schemas.ts` (`warehouseConnectionSchema`), not
`config.ts`. The warehouse schemas use `z.looseObject`, so the field had to be
declared explicitly to be *validated* (otherwise it would pass through
unvalidated). BigQuery's `job_timeout_ms` field and `bigQueryJobTimeoutMsFromConnection`
reader were removed; BigQuery now resolves the shared field. Every connector
resolves its deadline once at construction via `resolveQueryDeadlineMs`.
### Deviation from the spec's SQLite mechanism (worker thread → child process)
The spec mandated running SQLite read queries on a **worker thread** and enforcing
the deadline by `worker.terminate()`. This was **empirically disproven**:
`Worker.terminate()` cannot interrupt a CPU-bound synchronous `better-sqlite3`
scan — the native `sqlite3_step` loop never yields to V8, so terminate's promise
never even resolves (an 8s probe of the exact failing query shape confirmed the
thread keeps spinning). better-sqlite3 v12 exposes no `interrupt`/progress-handler
API, and `.iterate()` does not help because the failing query is a single
aggregate row produced only *after* the full scan.
The implemented mechanism is therefore **`child_process.fork` + `SIGKILL`**
(`packages/cli/src/connectors/sqlite/read-query-child.ts`, spawned from
`connector.ts`). SIGKILL lets the OS reclaim the whole process — a probe confirmed
the scan is interrupted in ~2 ms and CPU returns to idle. This satisfies *both*
SQLite requirements better than a thread (event loop stays free **and** the query
is genuinely cancellable). The child is self-contained (imports only
`better-sqlite3` + node builtins); validation/row-limiting (`limitSqlForExecution`)
and `normalizeQueryRows` stay on the main thread. One short-lived child per call,
killed on completion, deadline, or `ctx.signal` abort. Node v24's native
TS type-stripping lets the `.ts` child load under vitest; a `.js`-if-exists-else-`.ts`
URL resolver picks the compiled child in `dist`. Registered as a dynamic entry in
`knip.json`; `tsc` emits it to `dist` (verified, plus a dist-level end-to-end smoke).
### Remote connectors (server-side timeouts + own-signal mapping)
Each applies the resolved deadline server-side and re-wraps its own timeout signal
as `queryDeadlineExceededError(deadlineMs, { cause })`:
- **BigQuery** — `jobTimeoutMs` on the query job; maps a "Job timed out" / timeout-reason error.
- **Postgres** — `statement_timeout` via pool `options` (`-c statement_timeout=<ms>`); maps `57014`.
- **MySQL** — `SET SESSION max_execution_time = <ms>` before the read; maps errno `3024`.
- **Snowflake** — `ALTER SESSION SET STATEMENT_TIMEOUT_IN_SECONDS = <ceil(s)>` in the pooled connection; maps code `604` / "reached its … timeout".
- **ClickHouse** — `max_execution_time` (ceil seconds) setting, with `request_timeout` set to `deadline + 5s` so the HTTP client outlasts the server abort (replaces the old hardcoded 30s); maps code `159`.
- **SQL Server** — `requestTimeout` on the `mssql` pool config (TDS attention cancels server-side); maps `ETIMEOUT`.
Each connector has a focused test asserting the timeout is applied and its signal
maps to `KtxQueryError` (Postgres is the spec's required acceptance test).
### Best-effort callers (Requirement 8)
Confirmed already graceful: relationship **profiling** (outer try/catch →
`profile_failed` warning) and **composite-candidate** detection
(`detectCompositeRelationships` → recoverable warning, returns `[]`). Historic-SQL
**probes** flow through `runHistoricSqlReadinessProbe`, which catches *any* error
into `{ ok: false }`. **Added** handling to relationship **validation**: a
`KtxQueryError` on the per-candidate coverage probe now sends that one candidate to
`review` (`validation_query_failed`, logged via `ctx.logger.warn`) instead of
aborting the whole validation pass. `ingest-query-executor.ts` is a generic
executor port whose callers own recoverability — left unchanged.
### MCP surfacing/logging
No new MCP classification or logging code. The deadline `KtxQueryError` flows
through the existing `local-project-ports` mapping → `reportException` (skips
`$exception` for `KtxExpectedError`; existing test `telemetry/exception.test.ts`
covers the skip for `KtxQueryError`) → `instrumentMcpServer`, which logs a matched
`tool.start``tool.end(error, level 50)` pair carrying `err.message = "query
exceeded {N}s"`. A test in `test/context/mcp/server.test.ts` asserts the matched
pair, closing spec 15's "`tool.start` with no `tool.end` = hang" gap for this case.
### Pre-existing branch issues encountered (not part of this feature)
- `test/mcp-server-factory.test.ts` had a type error (an `as` cast to a shape with
a fake `context_tool` key, introduced by branch commit `2677b3ef`) that broke
`tsc -p tsconfig.test.json`. Fixed with a clean single cast to keep the
type-check gate green; behavior unchanged.
- `test/skills/analytics-skill-content.test.ts` fails (2 cases: missing
`**Window functions**` heading and `Expose identity, not just the label` prose
in `src/skills/analytics/SKILL.md`). This is unrelated analytics-skill (spec
13/14) content drift committed earlier on the branch; **left untouched** — no
skill files were modified by this feature.