Execution budget + QueryBudgetExceededError for buffering operators

[philcunliffe]

What

Adds a per-run execution budget to the public executeSql surface and makes the buffering operators refuse over it with a new exported typed error, QueryBudgetExceededError.

ExecuteSqlOptions gains budget?: ExecutionBudget:

interface ExecutionBudget {
  maxBufferedRows?: number   // ceiling on rows a single buffering operator may hold
  maxBufferedBytes?: number  // ceiling on estimated bytes of buffered cell values
}

It threads onto the execution context exactly like signal (context.budget), so every operator sees it. An undefined budget — or an undefined ceiling within it — is unbounded, so existing callers are unaffected (all 1697 existing tests pass unchanged).

Why — refuse, don't spill or truncate

The buffering operators (ORDER BY, high-cardinality GROUP BY, DISTINCT, and the scalar-aggregate slow path) accumulate the whole scanned input in memory before they can emit a row. Over a large dataset that exhausts the heap and OOM-kills the host process for every caller. V1 of the fix refuses over a ceiling rather than spill to disk or truncate: a partial COUNT(DISTINCT …) / GROUP BY would undercount silently, and a sorted prefix is only correct after the full input is already buffered — the exact memory the budget exists to avoid. When a ceiling is crossed the operator throws QueryBudgetExceededError (carrying operator, limitKind, limit, observed) and returns no rows. No spill, no truncation.

Enforced at (the buffering sites)

• src/execute/sort.js — executeSort row buffer (ORDER BY)
• src/execute/aggregates.js — executeScalarAggregate slow-path group collection (aggregate) and executeHashAggregate row collection (GROUP BY)
• src/execute/execute.js — executeDistinct dedup-key set (DISTINCT)

A shared BufferBudget accountant (src/execute/budget.js) charges each buffered row/key and throws when over; the byte estimate is a cheap O(columns) bound that never forces a lazy cell.

Streaming paths are unaffected

The tryColumnScanAggregate / scanColumnAggregate fast path holds an O(1) accumulator (or an O(cardinality) distinct set) and never constructs a budget, so a low ceiling does not bite COUNT/MIN/MAX/SUM/AVG (or low-card COUNT(DISTINCT …)) over a streamed column. Proven by the test a streaming column-scan aggregate is NOT affected by a low ceiling, which runs COUNT(id) with { maxBufferedRows: 1, maxBufferedBytes: 1 } over a scanColumn source whose buffering scan throws if ever called — and still returns the full count.

API added

• QueryBudgetExceededError — exported from src/index.js, declared in src/index.d.ts, defined in src/execute/budget.js.
• ExecutionBudget interface + ExecuteSqlOptions.budget + ExecuteContext.budget in src/types.d.ts.

Tests

test/execute/budget.test.js (9 tests): row-ceiling refusal for ORDER BY, the scalar-aggregate slow path, GROUP BY, and DISTINCT (asserting error type + operator/limit); byte-ceiling refusal (limitKind: 'bytes'); under-ceiling success; no-budget backward compatibility; and the streaming-bypass proof. Gates: npm test 1697 passed, npm run lint clean, npx tsc --noEmit clean.

---

Part of the hypaware bounded-query-execution effort (design: hypaware LLP 0058, decision LLP 0056; task T6).

[philcunliffe]

Dual-agent review — request_changes

• Verdict: request_changes
• Risk class: medium
• Auto-merge advisory: 👎 thumbs down — verdict is request_changes; needs human-gated follow-up

Advisory only: no merge was attempted.

Risk capstone

Cross-reference (reviewer findings vs blast radius)

#	Finding	Reviewer(s)	Severity	On-diff?	Notes
1	DISTINCT yields rows before the budget refuses → contradicts the "refused with no rows / no truncation" contract this PR adds	Codex (cat 2) + Claude	major	yes	The diff adds both the contract doc and the DISTINCT guard; the contradiction is owned by this diff. collect() masks it; a streaming consumer would see truncation+error.
2	COUNT(DISTINCT col) on a scanColumn source builds an unbounded Set and bypasses the budget	Codex (cat 3) + Claude	major	adjacent	Pre-existing Set in scanColumnAggregate (aggregates.js:343), reached via the fast path that never receives budget. Same memory profile as the bounded DISTINCT operator — inconsistent and a real OOM hole for budgeted callers.
3	ExecutionBudget doc implies general coverage; joins / window / set-ops stay unbounded with no disclosure	Claude	minor	docs-on-diff	Buffers themselves are pre-existing (join.js, window.js, set-ops in execute.js); the disclosure gap is in the doc this PR introduces. False-safety risk for hypaware.
4	Tests assert only that collect() rejects; don't prove DISTINCT emits no rows pre-refusal; streaming-bypass test covers COUNT(id) not COUNT(DISTINCT id)	Codex (cat 9)	minor	yes	Test-evidence gap aligned with findings 1 and 2.
5	Streaming column-scan aggregate immunity	Codex + Claude	(confirmed good)	yes	Fast path destructures only { tables, signal }; holds O(1) state for COUNT/SUM/AVG/MIN/MAX; test is a real proof (a buffering slow path would have thrown the source's scan() sentinel; scanColumnCalls===1 rules out the numRows shortcut).

Codex review

Fix Validations

Public budget plumbing and typed error

• Status: correct
• Evidence: src/execute/execute.js:30, src/execute/execute.js:53, src/index.js:2, src/index.d.ts:44, src/types.d.ts:32, src/types.d.ts:47, src/types.d.ts:68
• Assessment: budget is accepted on executeSql, threaded into ExecuteContext, and the new QueryBudgetExceededError is exported in JS and declarations.

Budget checks in the changed buffering operators

• Status: incomplete
• Evidence: src/execute/sort.js:136, src/execute/sort.js:141, src/execute/aggregates.js:91, src/execute/aggregates.js:101, src/execute/aggregates.js:212, src/execute/aggregates.js:222, src/execute/execute.js:610, src/execute/execute.js:630
• Assessment: ORDER BY, GROUP BY, and scalar aggregate slow path charge before retaining rows. DISTINCT charges retained keys, but still streams partial rows before a later budget error, and the column-scan COUNT(DISTINCT ...) fast path is not budgeted.

Findings

2) Contract & Interface Fidelity

• Severity: major
• Confidence: high
• Evidence: src/execute/execute.js:610, src/execute/execute.js:630, src/execute/execute.js:632, src/execute/execute.js:645, src/execute/execute.js:647
• Why it matters: DISTINCT can yield rows 1..N before row N+1 trips QueryBudgetExceededError, violating the stated “refused with no rows / no truncation” contract for streaming consumers.
• Suggested fix: When a finite budget is present, make executeDistinct complete the scan and budget checks before yielding any rows, or explicitly change the public contract and tests to allow partial rows before an error.

3) Change Impact / Blast Radius

• Severity: major
• Confidence: high
• Evidence: src/execute/aggregates.js:193, src/execute/aggregates.js:267, src/execute/aggregates.js:343, src/execute/aggregates.js:350
• Why it matters: COUNT(DISTINCT col) on a scanColumn source still builds an unbounded Set, so a high-cardinality distinct aggregate can bypass the new execution budget entirely.
• Suggested fix: Thread context.budget into tryColumnScanAggregate / scanColumnAggregate and charge each newly retained distinct key, or route COUNT(DISTINCT ...) away from the fast path when a budget is configured.

9) Test Evidence Quality

• Severity: minor
• Confidence: high
• Evidence: test/execute/budget.test.js:20, test/execute/budget.test.js:23, test/execute/budget.test.js:69, test/execute/budget.test.js:151
• Why it matters: The tests only assert that collect() rejects; they do not prove DISTINCT emits no rows before rejection, and the streaming bypass test covers COUNT(id) rather than COUNT(DISTINCT id).
• Suggested fix: Add a manual async-iterator test asserting zero yielded rows before a DISTINCT budget error, plus a COUNT(DISTINCT id) scanColumn test with a low budget.

No Finding

• 4. Concurrency, Ordering & State Safety
• 5. Error Handling & Resilience
• 6. Security Surface
• 7. Resource Lifecycle & Cleanup
• 8. Release Safety
• 10. Architectural Consistency
• 11. Debuggability & Operability

Evidence Bundle

• Changed hot paths: executeSql context setup at src/execute/execute.js:30 and src/execute/execute.js:53; sort budget at src/execute/sort.js:136; aggregate budgets at src/execute/aggregates.js:91 and src/execute/aggregates.js:212; DISTINCT budget at src/execute/execute.js:610; accountant/error at src/execute/budget.js:56 and src/execute/budget.js:111.
• Impacted callers: public executeSql to executePlan at src/execute/execute.js:55; recursive/context-preserving plan calls at src/execute/execute.js:77, src/execute/execute.js:118, src/execute/execute.js:489, src/execute/execute.js:505, src/execute/execute.js:599, src/execute/execute.js:684, src/execute/execute.js:697, src/execute/execute.js:734, src/execute/execute.js:789; sort/aggregate child calls at src/execute/sort.js:127, src/execute/aggregates.js:83, src/execute/aggregates.js:203.
• Impacted tests: budget row-ceiling tests at test/execute/budget.test.js:35, test/execute/budget.test.js:49, test/execute/budget.test.js:60, test/execute/budget.test.js:69; byte ceiling at test/execute/budget.test.js:82; under/no-budget compatibility at test/execute/budget.test.js:97, test/execute/budget.test.js:108, test/execute/budget.test.js:117; streaming bypass at test/execute/budget.test.js:149.
• Unresolved uncertainty: I did not run the test suite; this review is static against the supplied diff and targeted call-site traces.

Claude review

Claude review

DISTINCT emits rows before the budget trips — violates the documented "refused with no rows" contract

• Severity: major
• Confidence: 85
• Evidence: src/execute/execute.js:630-632 (and the final flush at :645-647); contract text at src/execute/budget.js:52-53 and src/index.d.ts:41-42
• Why it matters: Unlike ORDER BY / GROUP BY / scalar-aggregate — which buffer the whole input before emitting (so a refusal yields zero rows) — executeDistinct yields each new distinct row immediately (yield buffer[i] right after budget.addKey). A later key trips QueryBudgetExceededError only after rows 1..N have already been handed to a streaming consumer, so the public guarantee "refused with no rows; the engine does not spill to disk or truncate the result" is false for DISTINCT. collect() masks this (it discards on throw), which is why the tests pass, but a true async-generator consumer gets a truncated result followed by an error.
• Suggested fix: Either make executeDistinct charge the key into the budget before yielding the row for that key (so the (ceiling+1)th distinct row throws instead of being emitted), or relax the public contract wording to "no complete result" and add a test asserting DISTINCT's actual mid-stream behavior. Charging-before-yield is the smaller change and keeps the all-or-nothing story honest.

COUNT(DISTINCT col) on a scanColumn source bypasses the budget entirely

• Severity: major
• Confidence: 88
• Evidence: src/execute/aggregates.js:343-353 (unbounded seen Set in scanColumnAggregate), reached via the fast path tryColumnScanAggregate at src/execute/aggregates.js:267 which destructures only { tables, signal } and never sees context.budget
• Why it matters: SELECT COUNT(DISTINCT id) FROM t (no WHERE, source exposes scanColumn) takes the streaming fast path, which builds a Set of every distinct value — O(distinct rows), i.e. unbounded for a high-cardinality column — with no budget guard. The DISTINCT operator slow path is bounded, but this aggregate fast path with the same memory profile is not, so a budgeted caller can still OOM. It is also an inconsistency a caller cannot predict (one DISTINCT path bounded, another not).
• Suggested fix: Thread context.budget into tryColumnScanAggregate/scanColumnAggregate and charge each newly retained distinct key (mirror executeDistinct), or route COUNT(DISTINCT …) off the fast path when a budget is configured. At minimum, document that this aggregate is exempt.

ExecutionBudget doc implies general buffering coverage; joins / window / set-ops are silently unbounded

• Severity: minor
• Confidence: 80
• Evidence: doc at src/types.d.ts:25-38 ("Max rows a single buffering operator may hold"); unguarded buffers at src/execute/join.js:35 (nested-loop right side), :170/:177 (positional both sides), :216/:223 (hash-join build side + hashMap), src/execute/window.js:67 (window buffer), src/execute/execute.js:704 (UNION DISTINCT), :742 (INTERSECT rightKeys), :797 (EXCEPT rightKeys)
• Why it matters: The interface-level doc parenthetically lists four operators, but the per-field doc reads generically ("a single buffering operator"), and joins — the single most memory-hungry SQL operator and the most likely to OOM — get zero protection with no disclosure. A hypaware caller that sets a budget to prevent OOM gets false safety on JOIN/window/set-op/COUNT(DISTINCT) queries. These buffers are pre-existing (outside this diff), so this is a scoping/disclosure issue, not a defect in the added code.
• Suggested fix: Make the non-coverage explicit in the ExecutionBudget doc (name the operators that are not yet bounded), and track the remaining operators as follow-up so the gap is a conscious decision rather than a silent hole.

---

Reports: /Users/phil/workspace/squirreling/.git/worktrees/sq-review.mFakGZ/dual-review/pr-42

[philcunliffe]

🤖 neutral: reviewed (2 rounds), green, mergeable — held for your merge

Part of the hypaware bounded-query-execution effort (design hypaware LLP 0058, decision LLP 0056, task T6). Driven to held:

• Dual-review round 1 (Codex + Claude) found two real majors, now fixed in round 2:
  1. COUNT(DISTINCT) fast-path was unbounded — scanColumnAggregate built an O(distinct) seen Set with no budget guard. Round 2 threads context.budget into tryColumnScanAggregate → scanColumnAggregate and charges each newly-retained distinct key (refuses past the ceiling). Mutation-verified. Plain COUNT/SUM/MIN/MAX/AVG construct no budget and stay O(1)/immune.
  2. DISTINCT contract accuracy — the memory was already bounded (the dedup set is charged before yield); the doc now honestly distinguishes buffering operators (all-or-nothing: error before any row) from streaming operators (DISTINCT/COUNT(DISTINCT): may have emitted rows before refusal → consumer must treat the error as invalidating the whole result).
• Honest coverage doc — ExecutionBudget now enumerates what V1 bounds (ORDER BY, GROUP BY, scalar-aggregate slow path, DISTINCT, COUNT(DISTINCT)) and what it does NOT (joins, window functions, set operations) — filed as a follow-up.
• State: head 60106e2, MERGEABLE / CLEAN, CI green (lint + test + typecheck; vitest 1700 pass), tsc clean.

Merge is yours — neutral never merges. When you merge + publish a new squirreling version, neutral bumps the hypaware pin. Held for your call.