test(vnext): prove isolated parser worker placement#178
Conversation
Coverage Report
File CoverageNo changed files found. |
There was a problem hiding this comment.
Pull request overview
Adds an evidence-gating harness to validate that node-sql-parser grammars can be kept out of the core and page graphs while being lazily loaded inside a single dedicated module worker under a strict CSP, and records this decision as ADR 0004.
Changes:
- Adds a packed-consumer Vite/Chromium fixture that builds a core-only page and a worker-based page, then proves parser/grammar placement via module traces and runtime assertions.
- Introduces a CI script (
test:worker-placement) that packs the library tarball, installs an offline frozen fixture, enforces bundle ceilings, and runs a strict-CSP browser execution. - Documents the accepted “isolated browser parser execution” decision and links it from existing vNext docs/ADR 0003.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/worker-placement/workers.html | Fixture page that loads the worker-placement runtime module. |
| test/worker-placement/vite.workers.config.mjs | Vite build config for the worker-placement page/worker with emitted module-trace assets. |
| test/worker-placement/vite.core.config.mjs | Vite build config for the core-only page with emitted module-trace asset. |
| test/worker-placement/src/workers.js | Browser runtime that creates the worker, measures cold/warm grammar loads, and asserts global restoration. |
| test/worker-placement/src/parser-worker.js | Worker-side loader/guard that imports deep grammar builds and restores global/NodeSQLParser descriptors. |
| test/worker-placement/src/parser-worker-entry.js | Worker entry that wires grammar IDs to literal dynamic imports. |
| test/worker-placement/src/core.js | Core-only runtime that asserts vNext import + session revision identity without parser bytes. |
| test/worker-placement/README.md | Explains fixture purpose and records measured bundle baselines + enforced ceilings. |
| test/worker-placement/pnpm-workspace.yaml | Fixture-local workspace + overrides to support offline frozen installs. |
| test/worker-placement/pnpm-lock.yaml | Fixture lockfile pinning Vite/Playwright/node-sql-parser versions for offline installs. |
| test/worker-placement/package.json | Fixture package definition and build scripts. |
| test/worker-placement/core.html | Fixture page that loads the core-only runtime module. |
| scripts/worker-placement.mjs | Orchestration script: pack tarball, offline install fixture, build, verify traces/sizes, serve w/ CSP, run Chromium. |
| package.json | Adds pnpm run test:worker-placement script hook. |
| docs/vnext/node-sql-parser-adapter.md | Links to ADR 0004 as the accepted placement direction for the adapter. |
| docs/adr/0004-isolated-parser-execution.md | New ADR defining the browser-worker-first decision plus evidence gates. |
| docs/adr/0003-node-sql-parser-adapter.md | Updates ADR 0003 to reference ADR 0004 and its remaining gates. |
| .github/workflows/test.yml | Runs the worker-placement evidence step in CI. |
Files not reviewed (1)
- test/worker-placement/pnpm-lock.yaml: Generated file
Comments suppressed due to low confidence (1)
test/worker-placement/src/workers.js:47
- If the worker errors or the ready handshake times out, the "message" listener is never removed. This can leak listeners and allow a late "ready" message to resolve after the promise is already rejected. Remove both listeners on timeout/error.
const timeout = setTimeout(() => {
reject(new Error("Parser worker ready handshake timed out"));
}, REQUEST_TIMEOUT_MS);
const onError = (event) => {
clearTimeout(timeout);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/worker-placement/src/parser-worker.js">
<violation number="1" location="test/worker-placement/src/parser-worker.js:252">
P3: Concurrent parse requests can bypass the parser cache and duplicate dialect loading/initialization because the cache is populated only after the awaited operation completes. Deduplicating an in-flight initialization promise per grammar would preserve the service-owned lazy cache and avoid redundant work under concurrent messages.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const startedAt = performance.now(); | ||
| const loadModule = moduleLoaders[grammar]; | ||
| const { descriptorEquality, moduleValue } = | ||
| await guardedLoad(loadModule); |
There was a problem hiding this comment.
P3: Concurrent parse requests can bypass the parser cache and duplicate dialect loading/initialization because the cache is populated only after the awaited operation completes. Deduplicating an in-flight initialization promise per grammar would preserve the service-owned lazy cache and avoid redundant work under concurrent messages.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/worker-placement/src/parser-worker.js, line 252:
<comment>Concurrent parse requests can bypass the parser cache and duplicate dialect loading/initialization because the cache is populated only after the awaited operation completes. Deduplicating an in-flight initialization promise per grammar would preserve the service-owned lazy cache and avoid redundant work under concurrent messages.</comment>
<file context>
@@ -0,0 +1,360 @@
+ const startedAt = performance.now();
+ const loadModule = moduleLoaders[grammar];
+ const { descriptorEquality, moduleValue } =
+ await guardedLoad(loadModule);
+ const Parser = findParserConstructor(moduleValue);
+ const parser = Reflect.construct(Parser, []);
</file context>
Summary
/vnextremain parser/worker-freeScope
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
pnpm packarchive imported under hostile SSR globals/vnextcore graph contains no parser modules or orphan JavaScriptnode-sql-parser/build/postgresql.jsandnode-sql-parser/build/bigquery.jsworker-src 'self'NodeSQLParserandglobaldescriptors restored exactlyValidation
pnpm run test:worker-placementpnpm run typecheckpnpm exec oxlintpnpm run test:integritypnpm test— 971 passed, 1 expected failurepnpm run test:coverage— vNext 98.67% statements / 97.13% branches / 100% functions / 98.66% linespnpm run test:browserpnpm run test:packagepnpm run demobd5a08fwith zero findingsPart of #169.
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
scripts/worker-placement.mjstopnpm pack, build a nested fixture, serve withworker-src 'self', run Chromium viaplaywright, record timings/sizes, and verify: lazy worker creation, no parser in core/page graphs, separate lazynode-sql-parser/build/postgresql.jsand.../bigquery.jschunks, exact global restoration, and cleanup poisoning.pnpm run test:worker-placementstep.Dependencies
vite@8.0.13,playwright@1.61.1, andnode-sql-parser@5.4.0to match the packed transitive; runs offline with a frozen lockfile.Written for commit 8b1ea20. Summary will update on new commits.