Rifqi Hafizuddin Claude Opus 4.8 commited on
Commit
f3025d4
·
1 Parent(s): 60887f6

[NOTICKET] fix: F-5 — isolate DB threads; stop treating session hardening as best-effort

Browse files

`asyncio.wait_for` cancels the awaiting COROUTINE. The `to_thread` worker underneath
is not cancellable — it runs to completion, holding a thread and a connection on the
CUSTOMER's database long after we returned "timed out" to the user. Degrading their
production database is our liability, and it is invisible from our side because we
already answered.

(a) Dedicated bounded pool. DB work moves off `asyncio.to_thread` (which always uses
the shared default executor) onto its own ThreadPoolExecutor(50, "dbexec") via
`run_in_executor`. Abandoned workers previously accumulated in a pool of
min(32, cpu_count+4) shared with every other to_thread caller in the process — notably
the tabular Parquet loader — so a handful of slow customer queries could stall
unrelated work service-wide. Sized to the engine cache's own ceiling (_MAX_ENGINES=50
x _POOL_SIZE=1): more threads than that cannot make progress anyway. Timeout semantics
are unchanged.

(b) Session hardening is no longer best-effort — and reading it turned up a latent bug
worse than the finding. Both SETs shared one `try`:

cur.execute("SET statement_timeout = ...")
cur.execute("SET default_transaction_read_only = on")

so a statement_timeout failure SKIPPED THE READ-ONLY SET ENTIRELY, and the connection
then served queries in a writable session — behind a logger.warning, with the code
commenting that this was fine because "the authoritative guarantee is the compiler +
sqlglot guard". Defense in depth means this layer holds too.

They are now independent:
- `default_transaction_read_only` FAILS THE CONNECTION if it cannot be set. A
writable session against a customer database is not something to degrade into.
- `statement_timeout` logs at ERROR with `degraded_seam` but does not refuse service.
It is the customer's I/O at risk, not our correctness, so refusing outright is the
worse trade — but it is no longer a warning described as best-effort, because with
(a) established it is the ONLY real server-side bound.

The old comment claiming statement_timeout is "backed by the executor's
asyncio.wait_for" was circular and is corrected in place — that is the whole point of
this finding.

RISK TO WATCH: if any deployment currently fails the read-only SET silently, its
sources now fail loudly instead of running unhardened. Believed impossible — Neon
accepts it as a SET, which the existing comment documents — but it is the one
judgement call in this change and it is deliberate.

7 new tests. Suite 456 passed / 0 failed / 7 skipped. Ruff clean on touched paths.
`import main` OK. Readiness eval 17/17.

Docs: DEV_PLAN #45/#48 closed, #49 added; contract documents PII redaction on both
the traceability preview and report evidence tables.

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

API_CONTRACT_BE_PYTHON.md CHANGED
@@ -371,7 +371,7 @@ Report v2 fields (added 2026-07-09; all default-empty, so older stored reports r
371
  - `bq_answers` — one entry per business question. `status` is `answered` | `partial` | `unanswered`; `record_ids` cite the backing analyses. Written in the analysis's language (Indonesian objective → Indonesian answers).
372
  - `unresolved` — runs that were attempted but produced no usable evidence (every `analyze_*` step failed). Not part of the findings body.
373
  - `excluded` — runs the caller excluded via `exclude_record_ids`.
374
- - `evidence_tables` — `record_id` → small result tables copied from the run's stored outputs (max 3 tables per record, max 10 rows each; `truncated: true` when rows were capped). Rendered as markdown tables under the matching Key Findings group in `rendered_markdown`.
375
  - `charts` *(added 2026-07-14)* — `record_id` → `dataeyond.chart.v1` envelopes (see §Charts) copied verbatim from the run's stored outputs (max 3 per record). `rendered_markdown` gains an `## EDA` section where each chart appears as a fenced block the frontend renders with plotly.js:
376
 
377
  ````text
@@ -509,6 +509,7 @@ Field rules:
509
  - `thinking`, `filters[].description`, `tool_calls[].summary` are built from fixed templates, never an LLM — traceability adds no latency or token cost and cannot hallucinate.
510
  - The payload also carries an internal `user_id` (ownership); the frontend may ignore it.
511
  - Truncation: `preview` ≤ 5 rows; any string inside `input`/`output`/`preview`/`snippet` ≤ 300 chars (executed `query` ≤ 2000); rows beyond the preview are dropped (`row_count` is preserved).
 
512
 
513
  Response `200` for `structured_flow`:
514
 
 
371
  - `bq_answers` — one entry per business question. `status` is `answered` | `partial` | `unanswered`; `record_ids` cite the backing analyses. Written in the analysis's language (Indonesian objective → Indonesian answers).
372
  - `unresolved` — runs that were attempted but produced no usable evidence (every `analyze_*` step failed). Not part of the findings body.
373
  - `excluded` — runs the caller excluded via `exclude_record_ids`.
374
+ - `evidence_tables` — `record_id` → small result tables copied from the run's stored outputs (max 3 tables per record, max 10 rows each; `truncated: true` when rows were capped). Rendered as markdown tables under the matching Key Findings group in `rendered_markdown`. **PII redaction (added 2026-07-24):** cells in a column the catalog flags as PII are replaced with `"[redacted]"`; column headers are kept. A report is permanent and versioned, so this is where an unmasked customer name would have lasted longest. The narrative (`executive_summary`, findings, `bq_answers`) is generated from the real values and is unaffected — only the raw evidence dump is redacted. Reports generated before this date are unchanged.
375
  - `charts` *(added 2026-07-14)* — `record_id` → `dataeyond.chart.v1` envelopes (see §Charts) copied verbatim from the run's stored outputs (max 3 per record). `rendered_markdown` gains an `## EDA` section where each chart appears as a fenced block the frontend renders with plotly.js:
376
 
377
  ````text
 
509
  - `thinking`, `filters[].description`, `tool_calls[].summary` are built from fixed templates, never an LLM — traceability adds no latency or token cost and cannot hallucinate.
510
  - The payload also carries an internal `user_id` (ownership); the frontend may ignore it.
511
  - Truncation: `preview` ≤ 5 rows; any string inside `input`/`output`/`preview`/`snippet` ≤ 300 chars (executed `query` ≤ 2000); rows beyond the preview are dropped (`row_count` is preserved).
512
+ - **PII redaction in `preview` (added 2026-07-24).** Cells belonging to a column the catalog flags as PII are replaced with the literal string `"[redacted]"`. When any cell was redacted the output also carries `pii_masked`: the list of column names affected — render that so the user understands the blanks are deliberate, not missing data. The **column headers are never redacted**, only the values. This affects the stored provenance record only: the chat answer itself is generated from the real values, so a question like "list our top customers" still answers normally. Records written before this date have no `pii_masked` key and are returned unchanged.
513
 
514
  Response `200` for `structured_flow`:
515
 
DEV_PLAN.md CHANGED
@@ -246,10 +246,11 @@ plus the live report bug on analysis `966224d4…`. Same status legend as §0.
246
  | 42 | **F-20 observability** — `degraded_seam=<name>` on every never-throw / silent-drop path | Rifqi | 🔄 | The 2026-07-23 report bug was invisible by construction: the record was dropped with zero logging. **Partially shipped 2026-07-23** — the 10 seams where silent degradation is user-visible now emit a stable `degraded_seam` field (+ `repr(e)` instead of `str(e)`, so an empty-`str()` Fernet error is no longer a blank log): `input_guard_fail_open`, `analysis_catalog_read`, `report_floor_record_read`, `traceability_persist`, `traceability_flush`, `chart_persist` (×2), `report_input_persist` (×2), `analysis_state_ensure`. **Remaining:** the other ~76 `except Exception` sites, most of which are in unwired routers (`db_client`, `data_catalog`, `users`) or non-live paths — deliberately not swept, since a blanket edit across unwired code is exactly the drive-by §7A forbids. Control flow unchanged throughout (§5.4) |
247
  | 43 | **Go identity contract** — what does Go forward, and when? | Rifqi ↔ Harry | ⬜ new | Needed to replace #37's shared secret with real per-user authorization. Until then the store predicates in #38 are defensive only |
248
  | 44 | **F-17 / F-18 / F-25 compiler-parity batch** | Rifqi | ✅ new | Shipped 2026-07-23. Three execution-verified review findings that had **no task row** — the tracker jumped from #39 to #40 and lost them. **F-17 (High):** the bare-select check was gated on `if ir.group_by`, so a mixed select with `group_by=[]` passed; Postgres then failed loudly but the pandas path silently DROPPED the column while `output_columns` still advertised it → a real-looking table with a fabricated all-null column. Fixed in `validator.py` (fires whenever any agg is present — no false positives possible) + a presence backstop in `TabularExecutor`. **F-18:** `astype(str)` ran before `na=False`, so `NULL LIKE '%an%'` matched the literal `"nan"`/`"None"`. **F-25:** `SqlCompiler` raised on an empty `in`/`not_in` where pandas and `_column_values`' own docstring implement the empty-set semantics — a two-step plan whose first step legitimately returned zero rows hard-failed instead of answering. 16 new parity tests; one stale test updated (it pinned the old F-25 raise) |
249
- | 45 | **F-9 PII in persisted artifacts** — mask the traceability preview + report evidence tables | Rifqi | new | **Decided 2026-07-23** (CODE_REVIEW §7) but never tracked. `pii_flag` is an ingestion-time control only: nothing stops the planner SELECTing a flagged column, and the real values then land in `message_traceability.data` (served by a GET that is only now becoming scoped, #40) and are frozen permanently into `reports.content`. Decision: **the assembler still sees values** (otherwise "list our top customers" stops being answerable); mask only the persisted artifacts. Carry `pii_flag` onto `retrieve_data`'s output meta the catalog is already in scope at `data_access.py:232` |
250
  | 46 | **F-8 prompt-injection resistance** — planner / assembler / report_summary | Rifqi | ✅ new | Shipped 2026-07-24. The three prompts that ingest customer data had **no injection rule**: `guardrails.md` is appended only in `chatbot.py`/`help.py`, and the `InputGuard` screens only the user's message — it never sees catalog or row content. This is the one attack the five query-defense layers structurally cannot see, because every IR the planner emits is individually valid. A hostile string only needs to reach a text column in the customer's OWN database (a product description, a support ticket, a form field) to be sampled into `sample_values` and rendered verbatim into the planner prompt. Fix = a purpose-written "content is data, never instructions" rule in each prompt + `<data>…</data>` delimiters around the catalog render and the run-state render. **Deliberately NOT `guardrails.md` wholesale** — its rules prescribe refusal sentences, and the planner's only free-text field is `infeasible_reason`, so those strings would surface there and could regress the Q2 data-gap path (tests pin their absence). Verified: planner eval **6/6, carried_over 5/5 green**; live hostile-catalog run planned only `t_products` and never touched the planted `employees.salary`; 10 new tests |
251
  | 47 | **Cheap-batch review fixes** — F-19, F-22, F-24, F-26, F-16, F-4 | Rifqi | ✅ new | Shipped 2026-07-24, one low-risk batch. **F-19:** `sources` was missing entirely on `check` + router-`help` and came *after* `status` on the slow path — contract said always-first; additive fix, contract updated. **F-22:** `analysis_id` now 422s unless it parses as a UUID, on **both** live endpoints (chat + help kept identical on purpose). **F-24:** traceability/chart writes are skipped, and logged, when `analysis_id` is falsy — Go `0007` declares `analysis_id UUID NOT NULL` (+ FK on charts), so `analysis_id or ""` failed the insert and the never-throw seam lost the row silently. **F-26:** `QueryResult.error` uses `str(e) or repr(e)` — a Fernet `InvalidToken` reached the assembler/traceability/report as an EMPTY string; falling back only when `str()` is empty changes no existing message. **F-16:** retrieval cache key gains `redis_prefix` (two envs on one Redis cross-served results). **F-4:** non-Postgres sources now refused at the executor — **zero blast radius today** (Go's `isSupportedActive` allows only `postgres`), a tripwire so nobody re-enables a path that has no read-only session and no `statement_timeout`; legacy branch commented out per house convention, orphaned import commented with it |
252
- | 48 | **floor_08 — floor/body disagreement** | Rifqi | new | **Needs a decision, found while adding #34 coverage.** When a plan HAS an `analyze_*` step that FAILS but its upstream `retrieve_data` returned rows, the floor passes (`_produced_rows`) while the body rejects it (`has_reportable_result` keys on the failed analysis step). Verified by execution it is the only shape where the two disagree. Because the "Attempted, Unresolved" section is commented out (`generator.py:527`), such a run leaves **no trace anywhere**: if it is the session's only run, the report generates but comes out empty and the business question renders "Unanswered" — exactly the #33 bug through a different door. Suggested fix: apply the floor's second arm only when the plan has NO analysis step, mirroring the body predicate; `floor_08` then flips to `expected_ready: false`. **Not changed Rifqi owns the floor** |
 
253
 
254
  **Reading `eval/readiness/results/` (note for future sessions).** Four files are dated
255
  2026-07-23. `…_150632.json` scores **4/15 (26.7%)** — that is **not** a product
 
246
  | 42 | **F-20 observability** — `degraded_seam=<name>` on every never-throw / silent-drop path | Rifqi | 🔄 | The 2026-07-23 report bug was invisible by construction: the record was dropped with zero logging. **Partially shipped 2026-07-23** — the 10 seams where silent degradation is user-visible now emit a stable `degraded_seam` field (+ `repr(e)` instead of `str(e)`, so an empty-`str()` Fernet error is no longer a blank log): `input_guard_fail_open`, `analysis_catalog_read`, `report_floor_record_read`, `traceability_persist`, `traceability_flush`, `chart_persist` (×2), `report_input_persist` (×2), `analysis_state_ensure`. **Remaining:** the other ~76 `except Exception` sites, most of which are in unwired routers (`db_client`, `data_catalog`, `users`) or non-live paths — deliberately not swept, since a blanket edit across unwired code is exactly the drive-by §7A forbids. Control flow unchanged throughout (§5.4) |
247
  | 43 | **Go identity contract** — what does Go forward, and when? | Rifqi ↔ Harry | ⬜ new | Needed to replace #37's shared secret with real per-user authorization. Until then the store predicates in #38 are defensive only |
248
  | 44 | **F-17 / F-18 / F-25 compiler-parity batch** | Rifqi | ✅ new | Shipped 2026-07-23. Three execution-verified review findings that had **no task row** — the tracker jumped from #39 to #40 and lost them. **F-17 (High):** the bare-select check was gated on `if ir.group_by`, so a mixed select with `group_by=[]` passed; Postgres then failed loudly but the pandas path silently DROPPED the column while `output_columns` still advertised it → a real-looking table with a fabricated all-null column. Fixed in `validator.py` (fires whenever any agg is present — no false positives possible) + a presence backstop in `TabularExecutor`. **F-18:** `astype(str)` ran before `na=False`, so `NULL LIKE '%an%'` matched the literal `"nan"`/`"None"`. **F-25:** `SqlCompiler` raised on an empty `in`/`not_in` where pandas and `_column_values`' own docstring implement the empty-set semantics — a two-step plan whose first step legitimately returned zero rows hard-failed instead of answering. 16 new parity tests; one stale test updated (it pinned the old F-25 raise) |
249
+ | 45 | **F-9 PII in persisted artifacts** — mask the traceability preview + report evidence tables | Rifqi | | Shipped 2026-07-24. `pii_flag` was an **ingestion-time control only**: it nulls `sample_values` into the planner prompt, but nothing stops the planner SELECTing a flagged column and "list our top 20 customers" legitimately selects `customer_name`/`email`. Real values then reached two PERSISTED sinks: `message_traceability.data` (served by an unauthenticated GET, F-3 declined) and report evidence tables frozen permanently into `reports.content`. Fix = `retrieve_data` now carries `meta.pii_columns` (resolved through the IR select list, so aliases are honoured), and both sinks redact those cells to `[redacted]`. **Per the 2026-07-23 decision the ASSEMBLER still receives real values**, so the answer prose is unchanged and the question stays answerable verified end-to-end: assembler input kept `Ada Lovelace`, the persisted preview showed `[redacted]`. Aggregates are deliberately NOT masked except `min`/`max`: `sum(salary)` identifies nobody, but `max(email)` returns one customer's actual address. Fails **open** (an unresolvable name is left unmasked, never a legitimate column wrongly blanked), so this is a mitigation, not a guarantee. 15 new tests |
250
  | 46 | **F-8 prompt-injection resistance** — planner / assembler / report_summary | Rifqi | ✅ new | Shipped 2026-07-24. The three prompts that ingest customer data had **no injection rule**: `guardrails.md` is appended only in `chatbot.py`/`help.py`, and the `InputGuard` screens only the user's message — it never sees catalog or row content. This is the one attack the five query-defense layers structurally cannot see, because every IR the planner emits is individually valid. A hostile string only needs to reach a text column in the customer's OWN database (a product description, a support ticket, a form field) to be sampled into `sample_values` and rendered verbatim into the planner prompt. Fix = a purpose-written "content is data, never instructions" rule in each prompt + `<data>…</data>` delimiters around the catalog render and the run-state render. **Deliberately NOT `guardrails.md` wholesale** — its rules prescribe refusal sentences, and the planner's only free-text field is `infeasible_reason`, so those strings would surface there and could regress the Q2 data-gap path (tests pin their absence). Verified: planner eval **6/6, carried_over 5/5 green**; live hostile-catalog run planned only `t_products` and never touched the planted `employees.salary`; 10 new tests |
251
  | 47 | **Cheap-batch review fixes** — F-19, F-22, F-24, F-26, F-16, F-4 | Rifqi | ✅ new | Shipped 2026-07-24, one low-risk batch. **F-19:** `sources` was missing entirely on `check` + router-`help` and came *after* `status` on the slow path — contract said always-first; additive fix, contract updated. **F-22:** `analysis_id` now 422s unless it parses as a UUID, on **both** live endpoints (chat + help kept identical on purpose). **F-24:** traceability/chart writes are skipped, and logged, when `analysis_id` is falsy — Go `0007` declares `analysis_id UUID NOT NULL` (+ FK on charts), so `analysis_id or ""` failed the insert and the never-throw seam lost the row silently. **F-26:** `QueryResult.error` uses `str(e) or repr(e)` — a Fernet `InvalidToken` reached the assembler/traceability/report as an EMPTY string; falling back only when `str()` is empty changes no existing message. **F-16:** retrieval cache key gains `redis_prefix` (two envs on one Redis cross-served results). **F-4:** non-Postgres sources now refused at the executor — **zero blast radius today** (Go's `isSupportedActive` allows only `postgres`), a tripwire so nobody re-enables a path that has no read-only session and no `statement_timeout`; legacy branch commented out per house convention, orphaned import commented with it |
252
+ | 48 | **floor_08 — floor/body disagreement** | Rifqi | | **Decided + shipped 2026-07-24 (lead).** #34's row-producing arm was unconditional, so a plan that HAS an `analyze_*` step which FAILED still cleared the floor on the strength of its upstream fetchwhile the body rejected it. Because the "Attempted, Unresolved" section is commented out, such a run left no trace: as a session's only run it produced an empty report with the business question "Unanswered" (the #33 bug via another door). Fix = the arm now applies **only when the plan has no analysis step**, mirroring `has_reportable_result`; the shared `_plan_has_analysis` helper means the two predicates can no longer drift on that question. `floor_08` flips to `expected_ready: false`. The INTENDED asymmetry is preserved and re-verified: a zero-row retrieval still fails the floor but passes the body (floor_03). Readiness eval **17/17** |
253
+ | 49 | **F-5 timeout does not stop the customer's query** | Rifqi | ✅ new | Shipped 2026-07-24. `asyncio.wait_for` cancels the awaiting **coroutine**; the `to_thread` worker is not cancellable and runs to completion, holding a thread and a connection on the **customer's** database after we already answered "timed out". Two fixes. **(a) Dedicated bounded pool** — DB work moves to its own `ThreadPoolExecutor(50, 'dbexec')` via `run_in_executor`; abandoned workers previously accumulated in the shared default pool (`min(32, cpu_count+4)`) alongside every other `to_thread` caller, notably the tabular Parquet loader, so a few slow customer queries could stall unrelated work process-wide. **(b) Session hardening is no longer best-effort** — and a **latent bug** was found while reading it: both SETs shared one `try`, so a `statement_timeout` failure **skipped `default_transaction_read_only` entirely** and the connection served queries in a WRITABLE session behind a `logger.warning`. Now independent: read-only **fails the connection** if it cannot be set (a writable session against a customer DB is not something to degrade into); `statement_timeout` logs at **error** with `degraded_seam` but does not refuse service, since it is their I/O at risk rather than our correctness. ⚠️ **Blast radius to watch:** if any deployment currently fails that SET silently, its sources now fail loudly instead. Believed impossible (Neon accepts it as a SET — the existing comment documents this), but it is the one judgement call here. 7 new tests |
254
 
255
  **Reading `eval/readiness/results/` (note for future sessions).** Four files are dated
256
  2026-07-23. `…_150632.json` scores **4/15 (26.7%)** — that is **not** a product
eval/readiness/results/readiness_result_2026-07-24_094133.json ADDED
@@ -0,0 +1,278 @@
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
1
+ {
2
+ "run": {
3
+ "timestamp": "2026-07-24T09:41:33",
4
+ "dataset": "readiness_dataset.json",
5
+ "target": "src/agents/report/readiness.is_report_ready",
6
+ "total": 17,
7
+ "passed": 17,
8
+ "accuracy": 1.0,
9
+ "runtime_avg_ms": 0.0
10
+ },
11
+ "alignment_gap": {
12
+ "count": 2,
13
+ "ids": [
14
+ "align_01",
15
+ "align_02"
16
+ ]
17
+ },
18
+ "by_group": {
19
+ "floor": {
20
+ "n": 8,
21
+ "passed": 8,
22
+ "accuracy": 1.0
23
+ },
24
+ "delta": {
25
+ "n": 5,
26
+ "passed": 5,
27
+ "accuracy": 1.0
28
+ },
29
+ "edge": {
30
+ "n": 1,
31
+ "passed": 1,
32
+ "accuracy": 1.0
33
+ },
34
+ "alignment": {
35
+ "n": 3,
36
+ "passed": 3,
37
+ "accuracy": 1.0
38
+ }
39
+ },
40
+ "cases": [
41
+ {
42
+ "id": "floor_01",
43
+ "group": "floor",
44
+ "expected_ready": false,
45
+ "got_ready": false,
46
+ "expected_missing": [
47
+ "at least one completed analysis"
48
+ ],
49
+ "got_missing": [
50
+ "at least one completed analysis"
51
+ ],
52
+ "correct": true,
53
+ "aligned": false,
54
+ "gap": false,
55
+ "latency_ms": 0.0
56
+ },
57
+ {
58
+ "id": "floor_02",
59
+ "group": "floor",
60
+ "expected_ready": false,
61
+ "got_ready": false,
62
+ "expected_missing": [
63
+ "at least one completed analysis"
64
+ ],
65
+ "got_missing": [
66
+ "at least one completed analysis"
67
+ ],
68
+ "correct": true,
69
+ "aligned": false,
70
+ "gap": false,
71
+ "latency_ms": 0.0
72
+ },
73
+ {
74
+ "id": "floor_03",
75
+ "group": "floor",
76
+ "expected_ready": false,
77
+ "got_ready": false,
78
+ "expected_missing": [
79
+ "at least one completed analysis"
80
+ ],
81
+ "got_missing": [
82
+ "at least one completed analysis"
83
+ ],
84
+ "correct": true,
85
+ "aligned": false,
86
+ "gap": false,
87
+ "latency_ms": 0.0
88
+ },
89
+ {
90
+ "id": "floor_04",
91
+ "group": "floor",
92
+ "expected_ready": true,
93
+ "got_ready": true,
94
+ "expected_missing": [],
95
+ "got_missing": [],
96
+ "correct": true,
97
+ "aligned": true,
98
+ "gap": false,
99
+ "latency_ms": 0.0
100
+ },
101
+ {
102
+ "id": "floor_05",
103
+ "group": "floor",
104
+ "expected_ready": true,
105
+ "got_ready": true,
106
+ "expected_missing": [],
107
+ "got_missing": [],
108
+ "correct": true,
109
+ "aligned": true,
110
+ "gap": false,
111
+ "latency_ms": 0.0
112
+ },
113
+ {
114
+ "id": "floor_06",
115
+ "group": "floor",
116
+ "expected_ready": true,
117
+ "got_ready": true,
118
+ "expected_missing": [],
119
+ "got_missing": [],
120
+ "correct": true,
121
+ "aligned": true,
122
+ "gap": false,
123
+ "latency_ms": 0.0
124
+ },
125
+ {
126
+ "id": "floor_07",
127
+ "group": "floor",
128
+ "expected_ready": true,
129
+ "got_ready": true,
130
+ "expected_missing": [],
131
+ "got_missing": [],
132
+ "correct": true,
133
+ "aligned": true,
134
+ "gap": false,
135
+ "latency_ms": 0.0
136
+ },
137
+ {
138
+ "id": "floor_08",
139
+ "group": "floor",
140
+ "expected_ready": false,
141
+ "got_ready": false,
142
+ "expected_missing": [
143
+ "at least one completed analysis"
144
+ ],
145
+ "got_missing": [
146
+ "at least one completed analysis"
147
+ ],
148
+ "correct": true,
149
+ "aligned": true,
150
+ "gap": false,
151
+ "latency_ms": 0.0
152
+ },
153
+ {
154
+ "id": "delta_01",
155
+ "group": "delta",
156
+ "expected_ready": false,
157
+ "got_ready": false,
158
+ "expected_missing": [
159
+ "a new analysis since the last report"
160
+ ],
161
+ "got_missing": [
162
+ "a new analysis since the last report"
163
+ ],
164
+ "correct": true,
165
+ "aligned": true,
166
+ "gap": false,
167
+ "latency_ms": 0.0
168
+ },
169
+ {
170
+ "id": "delta_02",
171
+ "group": "delta",
172
+ "expected_ready": true,
173
+ "got_ready": true,
174
+ "expected_missing": [],
175
+ "got_missing": [],
176
+ "correct": true,
177
+ "aligned": true,
178
+ "gap": false,
179
+ "latency_ms": 0.0
180
+ },
181
+ {
182
+ "id": "delta_03",
183
+ "group": "delta",
184
+ "expected_ready": true,
185
+ "got_ready": true,
186
+ "expected_missing": [],
187
+ "got_missing": [],
188
+ "correct": true,
189
+ "aligned": true,
190
+ "gap": false,
191
+ "latency_ms": 0.0
192
+ },
193
+ {
194
+ "id": "delta_04",
195
+ "group": "delta",
196
+ "expected_ready": false,
197
+ "got_ready": false,
198
+ "expected_missing": [
199
+ "a new analysis since the last report"
200
+ ],
201
+ "got_missing": [
202
+ "a new analysis since the last report"
203
+ ],
204
+ "correct": true,
205
+ "aligned": true,
206
+ "gap": false,
207
+ "latency_ms": 0.0
208
+ },
209
+ {
210
+ "id": "delta_05",
211
+ "group": "delta",
212
+ "expected_ready": false,
213
+ "got_ready": false,
214
+ "expected_missing": [
215
+ "a new analysis since the last report"
216
+ ],
217
+ "got_missing": [
218
+ "a new analysis since the last report"
219
+ ],
220
+ "correct": true,
221
+ "aligned": true,
222
+ "gap": false,
223
+ "latency_ms": 0.0
224
+ },
225
+ {
226
+ "id": "edge_01",
227
+ "group": "edge",
228
+ "expected_ready": false,
229
+ "got_ready": false,
230
+ "expected_missing": [
231
+ "at least one completed analysis"
232
+ ],
233
+ "got_missing": [
234
+ "at least one completed analysis"
235
+ ],
236
+ "correct": true,
237
+ "aligned": false,
238
+ "gap": false,
239
+ "latency_ms": 0.0
240
+ },
241
+ {
242
+ "id": "align_01",
243
+ "group": "alignment",
244
+ "expected_ready": true,
245
+ "got_ready": true,
246
+ "expected_missing": [],
247
+ "got_missing": [],
248
+ "correct": true,
249
+ "aligned": false,
250
+ "gap": true,
251
+ "latency_ms": 0.0
252
+ },
253
+ {
254
+ "id": "align_02",
255
+ "group": "alignment",
256
+ "expected_ready": true,
257
+ "got_ready": true,
258
+ "expected_missing": [],
259
+ "got_missing": [],
260
+ "correct": true,
261
+ "aligned": false,
262
+ "gap": true,
263
+ "latency_ms": 0.0
264
+ },
265
+ {
266
+ "id": "align_03",
267
+ "group": "alignment",
268
+ "expected_ready": true,
269
+ "got_ready": true,
270
+ "expected_missing": [],
271
+ "got_missing": [],
272
+ "correct": true,
273
+ "aligned": true,
274
+ "gap": false,
275
+ "latency_ms": 0.0
276
+ }
277
+ ]
278
+ }
src/database_client/engine.py CHANGED
@@ -136,19 +136,47 @@ class UserEngineCache:
136
  # connect event (not per query, so the pooling latency win stays). These are
137
  # ordinary SET commands, NOT libpq startup `options` — Neon's transaction
138
  # pooler rejects `default_transaction_read_only` as a startup parameter but
139
- # accepts it as a SET. Best-effort: the authoritative read-only guarantee is
140
- # the compiler (SELECT-only) + the sqlglot DML guard; statement_timeout is
141
- # backed by the executor's asyncio.wait_for. So a failure here must not break
142
- # the connection.
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
143
  @event.listens_for(engine, "connect")
144
  def _init_session(dbapi_conn, _record): # noqa: ANN001
 
145
  try:
146
- cur = dbapi_conn.cursor()
147
- cur.execute(f"SET statement_timeout = {_STATEMENT_TIMEOUT_MS}")
 
 
 
 
 
 
 
 
 
 
 
148
  cur.execute("SET default_transaction_read_only = on")
 
149
  cur.close()
150
- except Exception as exc: # noqa: BLE001 — best-effort session hardening
151
- logger.warning("session init SET failed", error=str(exc))
152
 
153
  return engine
154
 
 
136
  # connect event (not per query, so the pooling latency win stays). These are
137
  # ordinary SET commands, NOT libpq startup `options` — Neon's transaction
138
  # pooler rejects `default_transaction_read_only` as a startup parameter but
139
+ # accepts it as a SET.
140
+ #
141
+ # REWORKED 2026-07-24 (F-5). Three things were wrong here:
142
+ #
143
+ # 1. Both SETs shared one `try`, so a `statement_timeout` failure skipped
144
+ # `default_transaction_read_only` entirely — the connection then served
145
+ # queries in a WRITABLE session, silently, behind a `logger.warning`. They
146
+ # are now independent.
147
+ # 2. The old comment said statement_timeout is "backed by the executor's
148
+ # asyncio.wait_for". It is not. `wait_for` cancels the awaiting coroutine;
149
+ # the `to_thread` worker underneath is NOT cancellable and runs to
150
+ # completion, holding a connection on the customer's database. This SET is
151
+ # therefore the ONLY real bound on how long a query burns their I/O.
152
+ # 3. Consequently neither SET is "best-effort" any more.
153
+ #
154
+ # `default_transaction_read_only` now FAILS THE CONNECTION if it cannot be set:
155
+ # a writable session against a customer database is not something to degrade
156
+ # into. `statement_timeout` logs at error but does not fail the connect — the
157
+ # query still runs bounded by LIMIT and the caller still stops waiting; it is
158
+ # their I/O at risk, not correctness, so refusing service outright would be a
159
+ # worse trade. Both are visible as `degraded_seam`.
160
  @event.listens_for(engine, "connect")
161
  def _init_session(dbapi_conn, _record): # noqa: ANN001
162
+ cur = dbapi_conn.cursor()
163
  try:
164
+ try:
165
+ cur.execute(f"SET statement_timeout = {_STATEMENT_TIMEOUT_MS}")
166
+ except Exception as exc: # noqa: BLE001
167
+ # Not fatal, but the customer's server now has no cap on how long
168
+ # our query runs after we have given up waiting for it.
169
+ logger.error(
170
+ "statement_timeout SET failed — customer queries are unbounded "
171
+ "server-side on this connection",
172
+ degraded_seam="db_statement_timeout_unset",
173
+ error=repr(exc),
174
+ )
175
+ # Deliberately NOT wrapped: if this raises, SQLAlchemy discards the
176
+ # connection and the query fails loudly. That is the intended outcome.
177
  cur.execute("SET default_transaction_read_only = on")
178
+ finally:
179
  cur.close()
 
 
180
 
181
  return engine
182
 
src/query/executor/db.py CHANGED
@@ -21,6 +21,7 @@ from __future__ import annotations
21
 
22
  import asyncio
23
  import time
 
24
  from typing import Any
25
 
26
  import sqlglot
@@ -45,6 +46,25 @@ from .base import BaseExecutor, QueryResult
45
  logger = get_logger("db_executor")
46
 
47
  _QUERY_TIMEOUT_SECONDS = 30
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
48
  _DBCLIENT_PREFIX = "dbclient://"
49
 
50
 
@@ -88,9 +108,20 @@ class DbExecutor(BaseExecutor):
88
  )
89
  creds = decrypt_credentials_dict(client.credentials)
90
 
 
 
 
 
 
 
91
  columns, rows = await asyncio.wait_for(
92
- asyncio.to_thread(
93
- self._run_sync, client_id, client.db_type, creds, compiled
 
 
 
 
 
94
  ),
95
  timeout=_QUERY_TIMEOUT_SECONDS,
96
  )
 
21
 
22
  import asyncio
23
  import time
24
+ from concurrent.futures import ThreadPoolExecutor
25
  from typing import Any
26
 
27
  import sqlglot
 
46
  logger = get_logger("db_executor")
47
 
48
  _QUERY_TIMEOUT_SECONDS = 30
49
+
50
+ # Dedicated pool for blocking customer-DB work (F-5, 2026-07-24).
51
+ #
52
+ # `asyncio.wait_for` cancels the awaiting COROUTINE; the worker thread underneath is
53
+ # not cancellable and runs to completion regardless. So every timed-out query leaves a
54
+ # thread occupied until the customer's server gives up (bounded by the connection's
55
+ # `statement_timeout`, which is best-effort — see engine.py). On the DEFAULT executor
56
+ # those abandoned workers accumulate in a pool of `min(32, cpu_count + 4)` that is
57
+ # shared with every other `to_thread` caller in the process — notably the tabular
58
+ # Parquet loader. A handful of slow customer queries could therefore stall unrelated
59
+ # work across the whole service.
60
+ #
61
+ # Isolating them means the blast radius of a slow customer database is queries against
62
+ # THAT class of work, not the entire process. Sized to the engine cache's own ceiling
63
+ # (_MAX_ENGINES=50 x _POOL_SIZE=1) — more threads than that cannot make progress
64
+ # anyway, since each needs a pooled connection.
65
+ _DB_THREAD_POOL = ThreadPoolExecutor(
66
+ max_workers=50, thread_name_prefix="dbexec"
67
+ )
68
  _DBCLIENT_PREFIX = "dbclient://"
69
 
70
 
 
108
  )
109
  creds = decrypt_credentials_dict(client.credentials)
110
 
111
+ # `run_in_executor` on the dedicated pool, not `asyncio.to_thread` (which
112
+ # always uses the shared default executor). The timeout semantics are
113
+ # unchanged — `wait_for` still stops US waiting after 30s — but a worker
114
+ # abandoned by that timeout now occupies a DB-only thread. See
115
+ # _DB_THREAD_POOL. (F-5)
116
+ loop = asyncio.get_running_loop()
117
  columns, rows = await asyncio.wait_for(
118
+ loop.run_in_executor(
119
+ _DB_THREAD_POOL,
120
+ self._run_sync,
121
+ client_id,
122
+ client.db_type,
123
+ creds,
124
+ compiled,
125
  ),
126
  timeout=_QUERY_TIMEOUT_SECONDS,
127
  )