Skip to content

Commit 873c926

Browse files
authored
feat(vnext): add bounded browser parser executor (#181)
## Summary Adds the private main-realm executor for the isolated parser worker selected in ADR 0004. - Lazily creates at most one authoritative worker generation. - Serializes requests through a bounded FIFO queue with independent request-count and retained UTF-16 text limits. - Enforces startup, queue-wait, and posted-execution safety deadlines. - Settles caller cancellation promptly while draining already-posted work. - Retires crashed, timed-out, malformed, or poisoned generations without replaying posted work. - Preserves never-posted queued work across eligible generation replacement. - Rejects reused worker identities and serializes retirement across hostile cleanup/settlement reentrancy. - Keeps the worker factory, executor, protocol, and implementation types private; there is no root or `/vnext` public API change. ## Risk classification Medium: concurrency, cancellation, worker lifecycle, resource accounting, and packaging boundaries. The principal risks are double settlement, stale-generation authority, physical worker overlap, posted-work replay, unbounded retention, and browser bundler regressions. The implementation mutates ownership before every external cleanup or settlement boundary, uses an iterative pump trampoline, and tests hostile synchronous callbacks at each boundary. ## Evidence - Full Node suite: 1,246 passed, 1 expected failure. - Focused deterministic executor suite: 84 passed. - Chromium: 4 files / 7 tests, including real worker, crash, silent-worker deadline, recovery, and default static Worker URL behavior. - Changed executor coverage: - Statements: 95.51% - Branches: 95.37% - Functions: 100% - Lines: 95.49% - All TypeScript project checks, oxlint, test-integrity, and diff checks pass. - Packed-package smoke test passes and asserts the executor artifact. - The post-rebase executor patch ID exactly matches the twice-reviewed pre-rebase patch. ## Adversarial review ledger The review loop found and fixed: - cancellation and FIFO reentrancy during promotion; - `preventDefault` running before generation retirement; - listener installation continuing after synchronous retirement; - recursive synchronous worker responses overflowing the stack; - startup-failure queue transfer and ready-failure ownership errors; - postMessage accessor disposal/forged-response races; - a P1 physical-worker overlap during cleanup-triggered nested submission; and - same-worker identity reuse after termination. Two independent reviewers approved the corrected pre-rebase patch. Two fresh independent audits also approved the exact rebased head after rerunning Node, Chromium, package, coverage, type, lint, integrity, placement, and demo gates. ## API and rollout This is implementation infrastructure only. It is not wired to sessions, completion, diagnostics, or other public features. No compatibility or migration work is required in this slice. The next consuming coordinator must preserve the same bounded ownership and no-replay rules. <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Adds a private, single-lane browser executor for the isolated SQL parser worker with bounded queueing, strict deadlines, and safe lifecycle handling. Internal only; no public or `/vnext` API changes. Hardened to fail closed on retirement errors with explicit terminal states. - **New Features** - Lazily creates at most one worker generation; posts one request at a time. - Bounded FIFO queue with request-count and UTF‑16 text limits; startup/queue/execution deadlines and prompt caller cancellation. - Retires crashed/timed-out/poisoned generations without replay; preserves never-posted queued work across replacement. - Rejects reused worker identities; retirement is serialized and atomic to avoid reentrancy races. - Executor, worker factory, protocol, and types remain private; docs updated and `scripts/package-smoke.mjs` asserts executor artifacts. - **Bug Fixes** - Fails closed if worker retirement throws to prevent accepting new work in an unsafe state. - Makes terminal states explicit to avoid resurrection and double settlement. <sup>Written for commit 1cb7105. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/marimo-team/codemirror-sql/pull/181?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
1 parent a5c4e86 commit 873c926

8 files changed

Lines changed: 4043 additions & 23 deletions

docs/adr/0004-isolated-parser-execution.md

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -55,21 +55,26 @@ Each `SqlLanguageService` will lazily own at most one dedicated parser worker.
5555
All sessions opened by that service share it. The worker is neither a
5656
`SharedWorker` nor a module-global singleton.
5757

58-
The first executor is single-lane:
58+
The private browser executor is single-lane:
5959

6060
- At most one request is posted at a time.
6161
- The host queue is bounded independently by request count and retained UTF-16
6262
text units.
63-
- A service owns construction, listeners, timers, termination, and disposal.
63+
- Worker construction is lazy and the executor owns listeners, timers,
64+
termination, and disposal.
6465
- No worker pool or idle shutdown is introduced without profile evidence.
65-
- Service disposal terminates the worker and settles every pending consumer.
66+
- Executor disposal terminates the worker and settles every pending consumer.
67+
68+
The executor and its worker factory remain package-private. No package export,
69+
`/vnext` export, language-service module, session ownership, or public
70+
configuration surface is introduced by this slice.
6671

6772
Ordinary caller cancellation and supersession settle the consumer promptly
6873
without relying on a worker message that cannot run during synchronous
6974
parsing. The executor may drain and discard that active result. A hard
7075
wall-clock deadline, worker crash, malformed protocol, or service disposal
71-
terminates the generation. The placement benchmark must compare drain versus
72-
restart under rapid edits before the executor policy is frozen.
76+
terminates the generation. Never-posted queued requests may continue on a
77+
fresh generation, but a posted request is never replayed.
7378

7479
The execution deadline belongs to the posted worker job, not to any attached
7580
consumer. Consumer cancellation never clears it. A draining operation retains
@@ -154,16 +159,20 @@ Messages do not contain:
154159

155160
The wire does not transport an independently supplied retryability boolean.
156161
The host treats only `module-load` as retryable; `backend` and
157-
`malformed-output` are terminal. A module-load failure closes the current
158-
worker generation so a retry cannot reuse a rejected dynamic-import realm.
162+
`malformed-output` are terminal. Every worker-reported failure retires the
163+
current generation. In particular, `backend` cannot distinguish an ordinary
164+
parse failure from descriptor-cleanup poisoning that closes the endpoint, and
165+
`module-load` must not reuse a rejected dynamic-import realm. Never-posted
166+
queued requests may continue on a fresh generation; the failed posted request
167+
is never replayed.
159168

160169
The host requires the current protocol version and correlation ID, validates
161170
all keys and closed values, and copies accepted data into new frozen objects.
162-
It then constructs an authentic `SqlParserAnalysis` with the exact pending
163-
request text and the host-owned authority. PostgreSQL and BigQuery rejection
164-
remain uncovered constructs; DuckDB rejection remains compatibility rejection.
165-
Worker isolation does not strengthen the compatibility-only evidence recorded
166-
by ADR 0003.
171+
A future statement coordinator will construct an authentic
172+
`SqlParserAnalysis` with the exact pending request text and the host-owned
173+
authority. PostgreSQL and BigQuery rejection remain uncovered constructs;
174+
DuckDB rejection remains compatibility rejection. Worker isolation does not
175+
strengthen the compatibility-only evidence recorded by ADR 0003.
167176

168177
Old-generation events are ignored by generation-owned listeners. A malformed,
169178
duplicate, unsolicited, or mismatched response kills the generation and
@@ -211,7 +220,7 @@ prove:
211220
recorded.
212221
- Raw and gzip worker sizes are recorded.
213222

214-
The executor and semantic slices additionally require:
223+
The semantic and session-integration slices additionally require:
215224

216225
- Main-thread long-task and event-loop responsiveness evidence.
217226
- Malformed message, crash, timeout, late-event, and restart tests.
@@ -280,7 +289,7 @@ optional-integration bundle budget.
280289

281290
1. Add this ADR and the packed-consumer browser placement harness.
282291
2. Extract a realm-neutral backend engine and add strict protocol codecs.
283-
3. Add the minimal browser worker and single-lane executor.
292+
3. Add the minimal browser worker and private bounded single-lane executor.
284293
4. Add in-worker normalized relation extraction.
285294
5. Add the pure statement coordinator, bounded cache, in-flight sharing, and
286295
atomic session ownership.

docs/vnext/node-sql-parser-adapter.md

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ parsing, and output normalization.
6969
The package contains a production-shaped but private module-worker endpoint.
7070
It is not exported from the package and is not reachable through `/vnext`.
7171
There is no public worker constructor, executor, queue, language-service
72-
module, or session integration yet.
72+
module, or session integration.
7373

7474
The endpoint:
7575

@@ -88,16 +88,27 @@ The endpoint:
8888
exact.
8989

9090
The endpoint accepts only one request at a time. Overlap and malformed messages
91-
fail closed instead of creating an implicit worker-side queue. The future
92-
service-owned executor is responsible for serialization, correlation,
93-
deadlines, cancellation, generation replacement, and disposal.
91+
fail closed instead of creating an implicit worker-side queue. A private
92+
main-realm executor now provides bounded FIFO admission, serialization,
93+
correlation, startup/queue/execution deadlines, prompt consumer cancellation,
94+
generation replacement, and disposal. It creates the production module worker
95+
lazily and keeps cancelled posted work in a draining lane until the worker
96+
responds or its safety deadline retires that generation. Posted work is never
97+
replayed. Every worker-reported failure retires the generation because a
98+
`backend` failure may be indistinguishable from endpoint cleanup poisoning;
99+
never-posted queued work retains its original deadline on the replacement.
100+
101+
The executor remains implementation infrastructure only. It is not exported
102+
from the root package or `/vnext`, is not owned by `SqlLanguageService`, and
103+
does not yet create authenticated syntax analyses or relation facts for a
104+
session.
94105

95106
Direct Chromium tests construct this source module worker and exercise both
96-
real grammar builds. The separate worker-placement fixture remains
97-
diagnostic packaging evidence: it records resource timing, emitted chunk
98-
reachability, and bundle sizes with a fixture-owned protocol. It is not the
99-
public integration boundary and must not be read as evidence that an executor
100-
or session API already exists.
107+
real grammar builds, including the private executor's production worker path.
108+
The separate worker-placement fixture remains diagnostic packaging evidence:
109+
it records resource timing, emitted chunk reachability, and bundle sizes with
110+
a fixture-owned protocol. It is not the public integration boundary and must
111+
not be read as evidence that a session API already exists.
101112

102113
Approximate local Node 24 arm64 measurements for the installed package were:
103114

scripts/package-smoke.mjs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,8 @@ try {
170170
throw new Error("Packed manifest does not declare node-sql-parser");
171171
}
172172
const privateWorkerArtifacts = [
173+
"dist/vnext/node-sql-parser-browser-executor.d.ts",
174+
"dist/vnext/node-sql-parser-browser-executor.js",
173175
"dist/vnext/node-sql-parser-browser-worker.d.ts",
174176
"dist/vnext/node-sql-parser-browser-worker.js",
175177
"dist/vnext/node-sql-parser-browser-worker-endpoint.d.ts",

0 commit comments

Comments
 (0)