Conversation
A windowed live query (one given an offset and limit) prepares both live_query_<id>_get and live_query_<id>_get_total_count, but unsubscribe only deallocated the former. The count statement outlived every teardown while its backing view was dropped, so it was left permanently dangling and accumulated for the lifetime of the database.
sprayberry-redline
left a comment
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: changes requested — the OSS-candidate verification evidence does not meet the required static-verification bar.
-
Blocking —
packages/pglite/tests/live.test.ts:1180-1186await unsubscribe()const unsubscribed = await db.query<{ name: string }>(
SELECT name FROM pg_prepared_statements WHERE name LIKE 'live_query_%';,
)
expect(unsubscribed.rows).toEqual([])This is a well-targeted regression assertion and, traced against the base, it fails before the fix: a windowed query prepares both
live_query_<id>_getandlive_query_<id>_get_total_count, while base teardown deallocates only the former. The new conditional deallocation makes the assertion pass. However, the candidate facts sheet declares this test verificationstatic, fork CI reports no checks, and it supplies neither a runtime/OS-specific reason for static-only test verification nor an explicit command for the operator to run on their Windows machine before upstream submission. The candidate gate requires one of a green relevant fork CI job or that explicit operator instruction alongside the static rationale; as submitted, there is no executable test evidence route before submission.Suggested fix: amend the facts sheet with the exact package/test command the operator must run on Windows (and its expected passing result), or supply a green relevant fork CI run. If static verification remains necessary, state the concrete unavailable runtime or OS-specific constraint. For example:
## Verification method Static for this candidate because <concrete OS/runtime constraint>. Before submitting upstream, run on a Windows checkout: pnpm --filter @electric-sql/pglite test -- live.test.ts Expected result: the "deallocates all prepared statements for a windowed query on unsubscribe" test passes.
What's good: the code change mirrors the existing isWindowed prepare path precisely, the base version confirms the count statement was prepared but not deallocated, and the regression test covers both the subscribed and post-unsubscribe statement sets. I also re-ran prior-art searches: no open upstream PR matched total_count or deallocate; the closed teardown precedent is electric-sql#643. No CI checks are reported for this fork branch.
Rework — blocking finding addressedThe blocking finding was the evidence route, not the code: the facts sheet declared test verification How the runtime problem was solved: Fails before (base teardown, both esm and cjs import paths): $ npx vitest run tests/live.test.ts -t 'deallocates all prepared statements' --typecheck.enabled=false
❯ |pglite| tests/live.test.ts (40 tests | 2 failed | 38 skipped) 10775ms
× esm import > live > deallocates all prepared statements for a windowed query on unsubscribe
→ expected [ Array(1) ] to deeply equal []
× cjs import > live > deallocates all prepared statements for a windowed query on unsubscribe
→ expected [ Array(1) ] to deeply equal []
AssertionError: expected [ Array(1) ] to deeply equal []
- Array []
+ Array [
+ Object {
+ "name": "live_query_89b9c51c4425421abcda1d052730f9a0_get_total_count",
+ },
+ ]
Test Files 1 failed (1)
Tests 2 failed | 38 skipped (40)Passes after: ✓ |pglite| tests/live.test.ts (40 tests | 38 skipped) 11030ms
Test Files 1 passed (1)
Tests 2 passed | 38 skipped (40)Whole touched file green (nothing else regressed): $ npx vitest run tests/live.test.ts --typecheck.enabled=false
✓ |pglite| tests/live.test.ts (40 tests) 65120ms
Test Files 1 passed (1)
Tests 40 passed (40)
Duration 70.19sFormatter: $ npx prettier --check src/live/index.ts tests/live.test.ts ../../.changeset/windowed-live-deallocate.md
Checking formatting...
All matched files use Prettier code style!Also corrected in the facts sheet, since the previous body was assembled second-hand and had errors:
No code change in this rework — the fix and test are unchanged at |
sprayberry-secondread
left a comment
sprayberry-secondread
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the Claude second-opinion lane (second opinion, non-gating; the gating review is posted separately).
Verdict: No blocking issues found — the fix is correctly scoped and I could confirm the bug from the base code myself.
What I checked
- Traced
packages/pglite/src/live/index.tsend to end for the windowedlive.querypath:isWindowedis set at line 58 (offset !== undefined && limit !== undefined), and when true,init()prepares two statements (lines 96-98 and 102-105):live_query_${id}_get(int, int)andlive_query_${id}_get_total_count. - Confirmed on base (pre-fix) that the matching
unsubscribe(startingpackages/pglite/src/live/index.ts:241) only ever ran oneDEALLOCATE:This is a real, reachable bug: callawait tx.exec(` DROP VIEW IF EXISTS live_query_${id}_view; DEALLOCATE live_query_${id}_get; `)db.live.query({ query, offset: 1, limit: 2, callback }), thenunsubscribe().live_query_<id>_getis deallocated butlive_query_<id>_get_total_countis not, and its backing view (live_query_<id>_view) is dropped in the same statement — so the surviving prepared statement is not just leaked but permanently unusable. The PR's own repro output demonstrates exactly this trace (EXECUTE live_query_..._get_total_count -> relation "live_query_..._view" does not exist). - Confirmed there are two other
unsubscribeclosures in this file (index.ts:502forincrementalQuery's diff/state tracking,index.ts:664forchanges) — neither hasisWindowedin scope, so the fix is not accidentally missing a second call site. - The fix itself,
packages/pglite/src/live/index.ts:254:is guarded correctly: unconditional${isWindowed ? `DEALLOCATE live_query_${id}_get_total_count;` : ''}
DEALLOCATEwould error for non-windowed queries where the count statement was never prepared. This exactly mirrors the existing guard used at the call sites that reference the count statement (e.g.index.ts:211if (isWindowed) { ... EXECUTE live_query_${id}_get_total_count ... }). - Regression test at
packages/pglite/tests/live.test.ts:1154is placed correctly, immediately after the existing'live query with windowing'test and before the offset/limit validation tests, matching the file's existing describe-block ordering and per-testCREATE TABLE/INSERTscaffolding style used throughout the file.
Maintainer's-eye read (oss-candidate)
- Idiom match: the ternary-inside-template-literal pattern (
${cond ? \...` : ''}) is already used elsewhere in this same file for optional SQL fragments (e.g.index.ts:374,:389,:393forUSER-DEFINED` type casts), so this isn't introducing a new style to the module. - Prior art in this exact function: PR electric-sql#643 (
fix(live): prevent double deallocate of live query prepared statement, merged) touched this same teardown block for a related dead-flag race, confirming maintainers accept narrowly-scoped one-line fixes here rather than asking for a larger refactor. - Test shape: matches the file's convention — same table/insert boilerplate, same
pg_prepared_statementsquerying style already used by the adjacent windowing test (live.test.ts:1067), asserting count before and empty array after. - Changeset: filename/format matches other merged PRs' changesets (
.changeset/heavy-params-bind.mdfrom electric-sql#1056,.changeset/calm-parsers-recover.mdfrom electric-sql#1072, etc. — allpatchbump, one paragraph description). This PR's.changeset/windowed-live-deallocate.mdfollows the same shape. - Scope: the diff is exactly the reported bug — one guarded
DEALLOCATEline, a regression test, and a changeset. No unrelated cleanup. - I did not find anything in this diff, in the repo's recent history, or in the surrounding code that a maintainer would plausibly push back on beyond taste (e.g. some might prefer separate
DEALLOCATEstatements or anIF EXISTS, but the existing code in this file doesn't useDEALLOCATE IF EXISTSanywhere, so following the established convention is the right call).
On the PR body's verification-method disclosure
The body is candid that the executed test ran against the published 0.5.8 dist/ bundle with the fix hand-patched into the minified template via patch-dist.mjs, not a full local build of the TS source, and that tsc/eslint weren't run. Given the teardown block is a single trivial string edit with no type-level surface (no new function signature, no new import), the risk that the compiled TS diverges from the tested bundle patch is low — but it is still an inference, not a build-verified fact, and the body says so plainly rather than overclaiming. That's an honest disclosure, not a finding.
No blocking issues. This is a small, well-targeted, well-tested fix with correct prior art and appropriate scope for this codebase.
SECOND READ: READY
Rework — evidence route rebuilt from sourceHEAD UNCHANGED at Two things to reconcile from the gating review:
Both runs below are real rebuilds from source: fails-before with the one line reverted in # base source, rebuilt # this branch's source, rebuilt
DEALLOCATE live_query_${f}_get; DEALLOCATE live_query_${f}_get;
DEALLOCATE live_query_${o}_diff1; DEALLOCATE live_query_${f}_get_total_count;
DEALLOCATE live_query_${o}_diff2; DEALLOCATE live_query_${o}_diff1;
DEALLOCATE live_query_${o}_diff2;Fails before (base source rebuilt, both esm and cjs import paths): $ npx vitest run tests/live.test.ts -t 'deallocates all prepared statements' --typecheck.enabled=false --pool=forks --poolOptions.forks.singleFork
RUN v2.1.2 /agent-workspace/oss/pglite-wt-1789361963/packages/pglite
❯ |pglite| tests/live.test.ts (40 tests | 2 failed | 38 skipped) 20544ms
× esm import > live > deallocates all prepared statements for a windowed query on unsubscribe
→ expected [ Array(1) ] to deeply equal []
× cjs import > live > deallocates all prepared statements for a windowed query on unsubscribe
→ expected [ Array(1) ] to deeply equal []
AssertionError: expected [ Array(1) ] to deeply equal []
- Expected
+ Received
- Array []
+ Array [
+ Object {
+ "name": "live_query_89d46c1bb2784027bfebd3c563a32e41_get_total_count",
+ },
+ ]
❯ tests/live.test.ts:1186:33
Test Files 1 failed (1)
Tests 2 failed | 38 skipped (40)
Duration 29.84sPasses after (this branch's source rebuilt): ✓ |pglite| tests/live.test.ts (40 tests | 38 skipped) 18482ms
Test Files 1 passed (1)
Tests 2 passed | 38 skipped (40)
Duration 23.47sWhole touched file green: $ npx vitest run tests/live.test.ts --typecheck.enabled=false --pool=forks --poolOptions.forks.singleFork
✓ |pglite| tests/live.test.ts (40 tests) 87910ms
Test Files 1 passed (1)
Tests 40 passed (40)
Duration 94.64sTypecheck, newly added this pass — $ grep -c "error TS" tsc-fixed.txt
60
$ grep "live/index.ts" tsc-fixed.txt
(none)
$ diff tsc-base.txt tsc-fixed.txt && echo "IDENTICAL: the change introduces no new type errors"
IDENTICAL: the change introduces no new type errors(The 60 are Formatter: $ prettier --check src/live/index.ts tests/live.test.ts ../../.changeset/windowed-live-deallocate.md
Checking formatting...
All matched files use Prettier code style!On the fork CI ask. A green fork CI job is not obtainable at this target and the body no longer implies one is pending: Still not run here, and said plainly in the body: Raw output and scripts archived at |
sprayberry-redline
left a comment
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: APPROVE — ready for the operator to submit.
I re-reviewed the evidence-only rework at unchanged head 89e8e6960155113185564b5890dbf395c0e0fdcb. The facts sheet contains all required sections. I independently traced the base: its windowed setup prepares both statements at packages/pglite/src/live/index.ts:95-104, while teardown only deallocates _get at base lines 251-254. Thus a call with both offset and limit, followed by final unsubscribe(), leaves _get_total_count prepared after the view is dropped. The changed line mirrors the existing isWindowed setup condition, so it does not deallocate a nonexistent statement for non-windowed queries.
I also reviewed the focused regression test, the patch changeset, commit hygiene, upstream search results, and the updated ## Rework evidence. The evidence now records real source rebuilds for both ESM and CJS: failure with the changed source line reverted, success after restoring it, and a 40/40 whole-file run. The PR has no checks because this fork cannot access the upstream-only runner; the body accurately states this and provides the operator's pre-submission command. No open upstream PR matching the searched total_count/deallocate terms was found.
What's good: the fix is minimal, preserves the non-windowed path, and the test asserts both the two-statement setup and the zero-statement teardown outcome. The changeset communicates the user-visible leak fix clearly.
…down Add regression coverage around the windowed total count deallocation: a non-windowed query must still tear down cleanly without a total count statement, a window of offset 0 / limit 0 is still a window, repeated subscribe/unsubscribe cycles must not accumulate statements, and unsubscribing one windowed query must leave a concurrent one working.
VerificationAdversarial verification by a fresh run, independent of the run that produced the fix. Nothing in Verified at head Which build is which — proven by grepping the emitted bundle$ # fixed build (branch source)
$ grep -o 'DEALLOCATE live_query_[^`"]*' packages/pglite/dist/live/index.js
DEALLOCATE live_query_${f}_get;
DEALLOCATE live_query_${f}_get_total_count;
DEALLOCATE live_query_${o}_diff1;
DEALLOCATE live_query_${o}_diff2;
$ # base build (src/live/index.ts checked out from origin/main, rebuilt)
$ grep -o 'DEALLOCATE live_query_[^`"]*' packages/pglite/dist/live/index.js
DEALLOCATE live_query_${f}_get;
DEALLOCATE live_query_${o}_diff1;
DEALLOCATE live_query_${o}_diff2;1. The PR's own test, re-run independentlyPasses with the fix: $ node vitest.mjs run tests/live.test.ts -t 'deallocates all prepared statements for a windowed query on unsubscribe' --typecheck.enabled=false --pool=forks --poolOptions.forks.singleFork
RUN v2.1.2 /agent-workspace/oss/pglite-wt-1789365337/packages/pglite
✓ |pglite| tests/live.test.ts (40 tests | 38 skipped) 12725ms
Test Files 1 passed (1)
Tests 2 passed | 38 skipped (40)
Duration 15.96s2. Whole touched test file, with the fix, before adding anything$ node vitest.mjs run tests/live.test.ts --typecheck.enabled=false --pool=forks --poolOptions.forks.singleFork
RUN v2.1.2 /agent-workspace/oss/pglite-wt-1789365337/packages/pglite
✓ |pglite| tests/live.test.ts (40 tests) 67063ms
Test Files 1 passed (1)
Tests 40 passed (40)
Duration 70.18s3. Four new tests written to break the fixCommitted to this branch in
Passes-after (esm and cjs arms both run via $ node vitest.mjs run tests/live.test.ts -t 'deallocate' --typecheck.enabled=false --pool=forks --poolOptions.forks.singleFork --reporter=verbose
RUN v2.1.2 /agent-workspace/oss/pglite-wt-1789365337/packages/pglite
✓ |pglite| tests/live.test.ts > esm import > live > deallocates all prepared statements for a windowed query on unsubscribe 6370ms
✓ |pglite| tests/live.test.ts > esm import > live > deallocates prepared statements for a non-windowed query on unsubscribe 1338ms
✓ |pglite| tests/live.test.ts > esm import > live > deallocates prepared statements for a windowed query with an offset and limit of 0 1024ms
✓ |pglite| tests/live.test.ts > esm import > live > only deallocates the prepared statements of the unsubscribed windowed query 1270ms
✓ |pglite| tests/live.test.ts > cjs import > live > deallocates all prepared statements for a windowed query on unsubscribe 4561ms
✓ |pglite| tests/live.test.ts > cjs import > live > deallocates prepared statements for a non-windowed query on unsubscribe 1319ms
✓ |pglite| tests/live.test.ts > cjs import > live > deallocates prepared statements for a windowed query with an offset and limit of 0 1373ms
✓ |pglite| tests/live.test.ts > cjs import > live > only deallocates the prepared statements of the unsubscribed windowed query 1044ms
Test Files 1 passed (1)
Tests 8 passed | 40 skipped (48)
Duration 20.86s
$ node vitest.mjs run tests/live.test.ts -t 'accumulate' --typecheck.enabled=false --pool=forks --poolOptions.forks.singleFork --reporter=verbose
✓ |pglite| tests/live.test.ts > esm import > live > does not accumulate prepared statements over repeated windowed subscribe/unsubscribe cycles 6553ms
✓ |pglite| tests/live.test.ts > cjs import > live > does not accumulate prepared statements over repeated windowed subscribe/unsubscribe cycles 3947ms
Test Files 1 passed (1)
Tests 2 passed | 46 skipped (48)
Duration 12.99s4. Fails-before for every new test —
|
sprayberry-redline
left a comment
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: APPROVED — ready for the operator to submit; no blocking issues found.
I re-reviewed the live head 47cdc47431c8cfabe83c19785f0d775e1ba5bf61, including the newly added adversarial regression coverage. The base implementation defines isWindowed as both offset and limit being present (including zero), prepares _get_total_count only in that branch, and the changed teardown now uses that same flag. Thus the new DEALLOCATE is issued exactly when the count statement exists and leaves non-windowed teardown untouched.
The added tests cover the original leak, non-windowed teardown, zero-valued window bounds, repeated subscribe/unsubscribe cycles, and isolation between concurrent windowed subscriptions. The candidate facts sheet contains all required sections, provides executed before/after test evidence, and gives the operator an explicit pre-submission command for the unavailable full build environment. I independently traced the base preparation/teardown paths and re-ran upstream PR/issue prior-art searches; no open duplicate was found. Commit messages contain no prohibited AI attribution. Fork CI reports no checks, as documented in the candidate evidence; I did not run the local suite per reviewer policy.
What's good: the one-line production change mirrors the existing setup invariant precisely, while the expanded tests protect the meaningful boundary cases without expanding the implementation scope.
sprayberry-secondread
left a comment
sprayberry-secondread
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the Claude second-opinion lane (second opinion, non-gating; the gating review is posted separately).
Verdict: No blocking issues. This commit (47cdc47) is test-only — it adds four regression tests on top of the already-reviewed one-line fix commit (89e8e69); the fix itself did not change.
What I checked
- Confirmed the underlying bug directly against base
ae182ff(packages/pglite/src/live/index.ts:238-256at that ref): the windowed teardown only ranDEALLOCATE live_query_${id}_get;with no_get_total_countcounterpart, while theisWindowedprepare path (live_index.ts:95-100at head) prepares both. Bug is real, not a static-analysis artifact. - Confirmed
packages/pglite/src/live/index.tsis byte-identical between89e8e69and47cdc47viagh api .../compare/89e8e69...47cdc47— onlypackages/pglite/tests/live.test.tschanged. - Read all four new tests (
tests/live.test.ts:1189-1315in the diff) against the fix logic (isWindowed = offset !== undefined && limit !== undefined,src/live/index.ts:58).
Findings
Info — tests/live.test.ts, only deallocates the prepared statements of the unsubscribed windowed query (diff lines ~1288-1315): after windowed.unsubscribe(), the test asserts remaining.rows.length equals 2 but not which two statements remain:
const remaining = await db.query<{ name: string }>(
`SELECT name FROM pg_prepared_statements WHERE name LIKE 'live_query_%' ORDER BY name;`,
)
expect(remaining.rows.length).toBe(2)A count-only assertion can't distinguish "the right query's statements survived" from "some other pair coincidentally survived." Given both windowed and other are windowed queries with 2 statements each, this is a low-probability gap in practice, not a live bug — suggested tightening:
expect(remaining.rows.map((r) => r.name)).toEqual([
`live_query_${other.id}_get`,
`live_query_${other.id}_get_total_count`,
])(the query object doesn't currently expose id — this would need a small test-only accessor, or asserting on a LIKE-filtered pair captured before windowed.unsubscribe() for comparison). Not blocking; the existing count check plus the later full-teardown assertion (line ~1315, unsubscribed.rows equal []) already gives good confidence.
Info — the four new tests each re-paste the same CREATE TABLE IF NOT EXISTS testTable ... INSERT ... generate_series(1,5) boilerplate (diff lines 1191-1200, 1224-1233, 1261-1270, 1290-1299). This matches the file's pre-existing style (every other it block in live.test.ts does the same), so it's not a regression this PR introduces, just an opportunity if the maintainers ever want a shared beforeEach fixture. Not a finding against this PR specifically.
OSS-candidate read (maintainer's-eye)
- Commit message (
test(live): cover non-windowed and repeated windowed unsubscribe teardown) follows the repo'stype(scope): ...convention seen throughoutgit log -- packages/pglite/src/live/index.ts(fix(live): ...,feat(pglite/live): ...). - Test shape matches the file exactly: same
testEsmCjsAndDTCwrapper, samedb.query<{ name: string }>generic pattern already used by the original windowed-deallocate test in the fix commit, sameCREATE TABLE IF NOT EXISTS/generate_seriesfixture idiom used by ~20 otheritblocks in this file. - No changeset added in this commit — correct; changesets in this repo track user-visible behavior (the fix commit already carries one), not test-only commits.
- Direct precedent for maintainers accepting teardown fixes in this exact function: PR electric-sql#643 (
fix(live): prevent double deallocate of live query prepared statement, merged). Prior-art search re-run:gh api repos/electric-sql/pglite/commits?path=packages/pglite/src/live/index.ts— no other open or merged PR touches the_get_total_countdeallocation; electric-sql#1051, electric-sql#710, electric-sql#845, electric-sql#598 all touch this file for unrelated reasons. - No
for (let ...)loop pattern exists elsewhere inlive.test.tsprior to this PR (does not accumulate prepared statements over repeated ...test, diff line ~1259); it's a reasonable, readable choice for a 3-cycle repetition test and not something I'd expect a maintainer to push back on. - I could not execute
pnpm build/vitestmyself (no Emscripten toolchain, no submodule checkout in this environment, per the PR's own stated limitation) — I did not attempt to independently re-run the test evidence tables in the PR body/rework comments; my confidence here rests on static tracing of the diff against the base source, not a fresh run.
SECOND READ: READY
askalf
changed the title
|
Submitted upstream. This staging PR is closed; the branch stays until upstream resolves. The upstream link lives in the team ledger. |
3 participants
Staging PR for branch review before submission upstream. Closed once submitted; the branch is kept until upstream resolves.