Skip to content

Commit a775bf4

Browse files
authored
test(vnext): prove isolated parser worker placement (#178)
## Summary - accept ADR 0004 for browser-first isolated parser execution - add a production-shaped Vite 8 / Chromium placement harness for one lazy, service-owned parser worker - prove exact PostgreSQL and BigQuery deep builds remain separate lazy chunks while core and `/vnext` remain parser/worker-free - record cold/warm phase timings, bundle ceilings, strict-CSP execution, exact global restoration, cleanup poisoning, and frozen offline fixture installation ## Scope This is an evidence gate, not production session wiring. The worker implementation remains fixture-owned in this PR. The following protocol PR must move the worker behind a packed optional integration, remove the fixture's direct parser dependency, and preserve the accepted packaging/realm boundaries. ## Evidence - exact `pnpm pack` archive imported under hostile SSR globals - root and `/vnext` core graph contains no parser modules or orphan JavaScript - one static module worker handles PostgreSQL then BigQuery - page graph contains no parser grammar - worker graph contains only `node-sql-parser/build/postgresql.js` and `node-sql-parser/build/bigquery.js` - grammar chunks are separate, lazy, and transitively budgeted - real Chromium execution under `worker-src 'self'` - worker-local `NodeSQLParser` and `global` descriptors restored exactly - reversible cleanup keeps the loader reusable; failed cleanup poisons it - nested fixture install is frozen and offline - clean temporary root install plus nested offline harness passed against a brand-new pnpm store ## Validation - `pnpm run test:worker-placement` - `pnpm run typecheck` - `pnpm exec oxlint` - `pnpm run test:integrity` - `pnpm test` — 971 passed, 1 expected failure - `pnpm run test:coverage` — vNext 98.67% statements / 97.13% branches / 100% functions / 98.66% lines - `pnpm run test:browser` - `pnpm run test:package` - `pnpm run demo` - two independent exact-head adversarial reviewers approved `bd5a08f` with zero findings Part of #169. <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Accepts ADR 0004 for isolated browser parser execution and adds a Vite 8/Chromium harness that proves a single, lazy, service-owned module worker with PostgreSQL/BigQuery loaded as separate chunks under a strict CSP; also hardens the fixture to clean up worker listeners/timers to avoid leaks. CI now fails closed on size, graph, SSR-safety, and readiness regressions. Part of #169. - **New Features** - Added ADR 0004; updated ADR 0003 and vNext docs to reference it. - Added `scripts/worker-placement.mjs` to `pnpm pack`, build a nested fixture, serve with `worker-src 'self'`, run Chromium via `playwright`, record timings/sizes, and verify: lazy worker creation, no parser in core/page graphs, separate lazy `node-sql-parser/build/postgresql.js` and `.../bigquery.js` chunks, exact global restoration, and cleanup poisoning. - Enforces ceilings: PostgreSQL 68 KiB gzip, BigQuery 50 KiB gzip, worker total 120 KiB gzip / 570 KiB raw. - CI: added `pnpm run test:worker-placement` step. - **Dependencies** - Fixture pins `vite@8.0.13`, `playwright@1.61.1`, and `node-sql-parser@5.4.0` to match the packed transitive; runs offline with a frozen lockfile. - No runtime changes for consumers; adds the test harness and script only. <sup>Written for commit 8b1ea20. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/marimo-team/codemirror-sql/pull/178?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 79dc271 commit a775bf4

18 files changed

Lines changed: 2565 additions & 0 deletions

.github/workflows/test.yml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,9 @@ jobs:
9999
- name: 🧪 Browser Tests
100100
run: pnpm run test:browser
101101

102+
- name: 🧵 Worker Placement Evidence
103+
run: pnpm run test:worker-placement
104+
102105
package:
103106
env:
104107
EXPECTED_PNPM_VERSION: ${{ matrix.pnpm-version }}

docs/adr/0003-node-sql-parser-adapter.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,11 @@ follow-up decision records one of:
167167
That decision must include browser measurements, cancellation behavior,
168168
worker/module failure recovery, and the effect of many mounted editors.
169169

170+
[ADR 0004](./0004-isolated-parser-execution.md) selects a dedicated,
171+
service-owned browser worker and defines the remaining evidence gates. It does
172+
not authorize session wiring until those gates and in-worker semantic
173+
normalization pass.
174+
170175
The current production loader supports pure Node only. A future browser
171176
integration must invoke parsing from a dedicated worker whose global object is
172177
not shared with application code, the legacy parser, or another installed copy.
Lines changed: 335 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,335 @@
1+
# ADR 0004: Isolated Browser Parser Execution
2+
3+
Status: accepted for implementation, session wiring gated by evidence
4+
5+
Date: 2026-07-25
6+
7+
## Context
8+
9+
ADR 0003 keeps the `node-sql-parser` adapter internal and unwired. Its parser
10+
is synchronous, so an `AbortSignal` cannot interrupt it while it occupies the
11+
JavaScript thread. Even a late result that is correctly discarded can make an
12+
editor unresponsive.
13+
14+
Moving the current adapter object into a worker is not valid. Parser requests,
15+
authorities, ranges, artifacts, and analyses are authenticated by
16+
package-owned, realm-local `WeakSet` and `WeakMap` state. Structured cloning
17+
would produce unauthenticated copies. The backend AST is also retained in a
18+
realm-local weak map and cannot become a cross-realm semantic API.
19+
20+
The distributed dialect builds introduce separate constraints:
21+
22+
- The Node loader uses `node:module` and intentionally rejects any realm with
23+
`self` or `window`.
24+
- The browser builds are CommonJS/UMD files which a consumer bundler must
25+
transform.
26+
- Loading a build may write `NodeSQLParser` or `global` on its realm.
27+
- A browser worker can be terminated for a wall-clock deadline, but browsers
28+
do not expose an enforceable per-worker heap limit.
29+
- One worker per editor would multiply parser memory across marimo's many
30+
mounted editors.
31+
32+
This decision concerns local browser placement. Node `worker_threads`, native
33+
providers, remote providers, and public packaging are separate decisions.
34+
35+
## Decision
36+
37+
### Browser-first placement
38+
39+
Interactive browser parsing will use a dedicated module worker. The existing
40+
pure-Node inline adapter remains internal evidence and batch-test
41+
infrastructure. It is not a fallback when browser worker construction,
42+
loading, or execution fails.
43+
44+
Browser placement is accepted with an explicit residual risk: input, queue,
45+
response, cache, and lifetime can be bounded, but transient parser allocation
46+
cannot be capped before the browser itself terminates an over-consuming
47+
worker. The current 16 KiB input ceiling remains an upper safety bound, not an
48+
interactive performance claim. Production session wiring remains blocked
49+
until adversarial memory, latency, failure-recovery, and many-editor gates
50+
pass.
51+
52+
### Ownership and scheduling
53+
54+
Each `SqlLanguageService` will lazily own at most one dedicated parser worker.
55+
All sessions opened by that service share it. The worker is neither a
56+
`SharedWorker` nor a module-global singleton.
57+
58+
The first executor is single-lane:
59+
60+
- At most one request is posted at a time.
61+
- The host queue is bounded independently by request count and retained UTF-16
62+
text units.
63+
- A service owns construction, listeners, timers, termination, and disposal.
64+
- No worker pool or idle shutdown is introduced without profile evidence.
65+
- Service disposal terminates the worker and settles every pending consumer.
66+
67+
Ordinary caller cancellation and supersession settle the consumer promptly
68+
without relying on a worker message that cannot run during synchronous
69+
parsing. The executor may drain and discard that active result. A hard
70+
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.
73+
74+
The execution deadline belongs to the posted worker job, not to any attached
75+
consumer. Consumer cancellation never clears it. A draining operation retains
76+
the active lane until it returns or its generation is terminated; queued work
77+
has a separate wait deadline. Service disposal always terminates immediately.
78+
These rules prevent a cancelled hostile parse from occupying the only worker
79+
indefinitely.
80+
81+
The safety deadline is separate from product latency targets. A deadline
82+
failure never upgrades parser authority and an active request is not
83+
automatically replayed after a crash or timeout.
84+
85+
### Realm and loading boundary
86+
87+
The worker is created with a same-origin URL:
88+
89+
```ts
90+
new Worker(
91+
new URL("./node-sql-parser-browser-worker.js", import.meta.url),
92+
{ name: "codemirror-sql-parser", type: "module" },
93+
);
94+
```
95+
96+
Blob, data, and evaluated workers are not used. Hosts must allow the emitted
97+
worker URL in their Content Security Policy, normally through
98+
`worker-src 'self'`. The worker asset response also receives a restrictive
99+
policy because a worker has its own execution context.
100+
101+
The constructor shape and its literal options stay static. Current
102+
[Vite worker handling](https://vite.dev/guide/features#web-workers) recognizes
103+
the URL only when `new URL(..., import.meta.url)` appears directly inside the
104+
worker constructor. The
105+
[platform worker contract](https://developer.mozilla.org/en-US/docs/Web/API/Worker/Worker)
106+
also requires a same-origin entry URL and JavaScript response media type.
107+
108+
The browser worker has a browser-specific loader. It does not weaken or reuse
109+
the pure-Node realm gate. It dynamically imports literal, dialect-specific
110+
paths only:
111+
112+
```text
113+
node-sql-parser/build/postgresql.js
114+
node-sql-parser/build/bigquery.js
115+
```
116+
117+
The worker verifies that `self === globalThis` and that no DOM window exists.
118+
It snapshots and restores the exact `NodeSQLParser` and `global` descriptors
119+
around dialect loading. Cleanup failure poisons that worker generation.
120+
121+
### Private wire protocol
122+
123+
The worker protocol is package-private, versioned, closed, and decoded from
124+
`unknown` on both sides. It transports plain evidence, never authenticated
125+
syntax objects.
126+
127+
The initial request contains only:
128+
129+
- Protocol version
130+
- Host correlation ID
131+
- Grammar ID: PostgreSQL or BigQuery
132+
- Exact untrimmed statement text
133+
134+
DuckDB uses the PostgreSQL grammar. Target-dialect policy stays in the main
135+
realm.
136+
137+
The initial response contains only one closed outcome:
138+
139+
- Parsed normalized statement kind
140+
- Syntax rejection
141+
- Bounded unsupported reason
142+
- Bounded failure code plus retryability
143+
144+
Messages do not contain:
145+
146+
- Public document revisions or session identities
147+
- Parser authorities or dialect handles
148+
- `AbortSignal`, `Error`, stack, or raw backend message values
149+
- Source text echoed in a response
150+
- Absolute document ranges
151+
- Raw ASTs or generic payload bags
152+
153+
The host requires the current protocol version and correlation ID, validates
154+
all keys and closed values, and copies accepted data into new frozen objects.
155+
It then constructs an authentic `SqlParserAnalysis` with the exact pending
156+
request text and the host-owned authority. PostgreSQL and BigQuery rejection
157+
remain uncovered constructs; DuckDB rejection remains compatibility rejection.
158+
Worker isolation does not strengthen the compatibility-only evidence recorded
159+
by ADR 0003.
160+
161+
Old-generation events are ignored by generation-owned listeners. A malformed,
162+
duplicate, unsolicited, or mismatched response kills the generation and
163+
settles the active operation exactly once without exposing raw event data.
164+
165+
### Semantic reuse
166+
167+
Raw backend ASTs will not cross the worker boundary and the first protocol will
168+
not introduce remote AST handles or worker-local AST leases.
169+
170+
Before production session wiring, the worker request will parse once and run
171+
adapter-owned semantic decoders in the same realm. It will return only the
172+
bounded, validated relation facts required by the first completion slice.
173+
This keeps backend shapes private, avoids reparsing once for syntax and again
174+
for relations, and makes cached main-realm evidence measurable.
175+
176+
Worker-local AST caching is deferred until profiling demonstrates that
177+
reparsing is material enough to justify leases, byte accounting, generation
178+
invalidation, and release semantics.
179+
180+
### Packaging boundary
181+
182+
Core and `/vnext` imports must remain SSR-safe and contain no parser grammar or
183+
worker asset. A future optional integration entry may create the worker lazily,
184+
but it will expose an opaque language-service module factory rather than the
185+
protocol, worker URL, transport, pool, or backend AST.
186+
187+
The initial supported bundler claim is limited to packed-consumer fixtures that
188+
run in CI. Source-workspace success is not packaging evidence.
189+
190+
## Evidence required before session wiring
191+
192+
A production-shaped fixture built alongside the exact `npm pack` archive must
193+
prove:
194+
195+
- Core-only import emits no parser or worker bytes.
196+
- PostgreSQL and BigQuery emit separate worker chunks.
197+
- The all-dialect build is absent.
198+
- Worker creation is lazy.
199+
- Both grammars execute in a real browser.
200+
- Main-window parser globals remain unchanged.
201+
- Core import remains SSR-safe.
202+
- A same-origin module worker runs under a strict CSP.
203+
- Worker startup, cold import, warm parse, and message round-trip samples are
204+
recorded.
205+
- Raw and gzip worker sizes are recorded.
206+
207+
The executor and semantic slices additionally require:
208+
209+
- Main-thread long-task and event-loop responsiveness evidence.
210+
- Malformed message, crash, timeout, late-event, and restart tests.
211+
- Rapid-edit drain-versus-restart measurements.
212+
- One, ten, and fifty editor scenarios.
213+
- Retained worker, listener, timer, and memory checks after disposal.
214+
- Adversarial statements at the accepted input ceiling.
215+
216+
The current product envelopes remain:
217+
218+
- No routine main-thread task over 50 ms.
219+
- Warm active-statement analysis p95 under 16 ms.
220+
- Local completion p95 under 50 ms.
221+
222+
Safety timeouts are not evidence that these product targets are met.
223+
224+
### Initial packed-consumer baseline
225+
226+
The placement harness introduced with this decision builds the exact packed
227+
archive, verifies its core import in an isolated fixture, and separately uses
228+
fixture-owned worker code with the pinned `node-sql-parser` dependency to prove
229+
consumer-side Vite 8 placement feasibility. It serves the production output
230+
with a same-origin CSP and runs it in Chromium.
231+
232+
The worker portion does not yet prove a packed optional parser integration;
233+
that entry does not exist. The protocol PR must move the worker implementation
234+
behind the packed package boundary and remove the fixture's direct parser
235+
dependency before making a public packaging claim.
236+
237+
A representative local Node 24 / Chromium 149 / arm64 macOS sample recorded:
238+
239+
| Output | Raw | gzip |
240+
| --- | ---: | ---: |
241+
| Core-only fixture | 24,056 B | 7,497 B |
242+
| PostgreSQL transitive worker graph | 321,156 B | 67,396 B |
243+
| BigQuery transitive worker graph | 225,309 B | 50,389 B |
244+
| Complete worker fixture | 549,893 B | 118,170 B |
245+
246+
The core module trace contained no `node-sql-parser` module. No dialect
247+
resource loaded before explicit construction. A single static module worker
248+
loaded PostgreSQL and then BigQuery through separate literal lazy imports;
249+
both parsed successfully without changing the main-window parser sentinel.
250+
The per-dialect figures conservatively include their complete static
251+
transitive closures, with shared chunks also reported separately.
252+
253+
One sequential cold/warm run on the shared worker measured:
254+
255+
| Dialect | Grammar load and initialization | First parse | First round trip | Warm parse | Warm round trip |
256+
| --- | ---: | ---: | ---: | ---: | ---: |
257+
| PostgreSQL | 8.0 ms | 2.4 ms | 10.6 ms | 0.1 ms | 0.3 ms |
258+
| BigQuery | 4.2 ms | 2.5 ms | 6.7 ms | 0.2 ms | 0.4 ms |
259+
260+
The worker ready handshake took 10.3 ms in that run.
261+
262+
These numbers establish packaging feasibility and initial size guards. They
263+
are not percentile claims. Stable latency decisions require repeated,
264+
cross-platform samples over the representative and adversarial corpus.
265+
266+
The checked-in harness fails above 68 KiB gzip for the PostgreSQL transitive
267+
graph, 50 KiB for the BigQuery transitive graph, or 120 KiB / 570 KiB for the
268+
complete worker fixture in gzip/raw form. These ceilings include small
269+
measurement headroom and are placement-spike guards, not the final
270+
optional-integration bundle budget.
271+
272+
## Implementation sequence
273+
274+
1. Add this ADR and the packed-consumer browser placement harness.
275+
2. Extract a realm-neutral backend engine and add strict protocol codecs.
276+
3. Add the minimal browser worker and single-lane executor.
277+
4. Add in-worker normalized relation extraction.
278+
5. Add the pure statement coordinator, bounded cache, in-flight sharing, and
279+
atomic session ownership.
280+
6. Ship relation completion as the first public consuming vertical slice.
281+
282+
Every production step is a medium change and receives two independent,
283+
commit-bound adversarial reviews.
284+
285+
## Consequences
286+
287+
- Synchronous parser CPU work cannot block the editor main thread.
288+
- Fifty editors on one service do not imply fifty parser workers.
289+
- Realm-local authenticity remains an internal safety boundary.
290+
- Worker failure is explicit and never falls back to unsafe inline parsing.
291+
- The raw AST remains replaceable and private.
292+
- A serial worker may create head-of-line blocking; measurement, queue bounds,
293+
and hard deadlines make that tradeoff visible before considering a pool.
294+
- Browser heap exhaustion cannot be fully contained and remains a documented
295+
residual risk.
296+
- Browser and Node interactive execution can evolve independently.
297+
298+
## Rejected alternatives
299+
300+
### Run the parser on the browser main thread
301+
302+
Late-result rejection preserves correctness but cannot restore responsiveness
303+
while synchronous parsing runs.
304+
305+
### Clone normalized syntax objects from the worker
306+
307+
Structured cloning loses the package-owned realm authentication required by
308+
the syntax contract.
309+
310+
### Send raw ASTs or AST handles
311+
312+
Raw ASTs expose backend coupling and can be very large. Remote handles add
313+
leases, eviction, crash invalidation, and release semantics before a semantic
314+
consumer exists.
315+
316+
### One worker per editor
317+
318+
This multiplies grammar and runtime memory and conflicts with the many-editor
319+
release target.
320+
321+
### `SharedWorker` or a module-global singleton
322+
323+
Both weaken service ownership and disposal isolation. `SharedWorker` also
324+
narrows runtime and CSP compatibility.
325+
326+
### A generic worker or provider transport
327+
328+
The first need is one parser with a small closed protocol. A general framework
329+
would stabilize abstractions before there is evidence from a second workload.
330+
331+
### A worker pool
332+
333+
A pool increases grammar duplication, memory, scheduling, and cancellation
334+
complexity. It can be reconsidered only if a measured serial bottleneck
335+
outweighs those costs.

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,3 +114,7 @@ without importing a backend.
114114
For that reason, this adapter remains unwired. A worker-versus-main-thread ADR,
115115
with browser latency, memory, hostile-input, timeout, and recovery evidence, is
116116
required before interactive sessions may call it.
117+
118+
[ADR 0004](../adr/0004-isolated-parser-execution.md) chooses isolated
119+
browser-worker execution and records the packaging, performance, memory, and
120+
semantic-reuse gates that still block session wiring.

package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
"test:coverage": "vitest run --config vitest.config.ts --coverage",
2424
"test:coverage:changed": "node ./scripts/changed-coverage.mjs",
2525
"test:browser": "vitest run --config vitest.browser.config.ts",
26+
"test:worker-placement": "node ./scripts/worker-placement.mjs",
2627
"test:integrity": "node ./scripts/check-test-integrity.mjs",
2728
"bench:parser-adapter": "vitest bench --run src/vnext/__tests__/node-sql-parser-adapter.bench.ts",
2829
"bench:statement-index": "vitest bench --run src/vnext/__tests__/statement-index.bench.ts",

0 commit comments

Comments
 (0)