test(gateway): cross-user-isolation cleanup honors dbAvailable — unblocks gated publish verify (#1275) #1304

2026-08-18T04:13:25Z

jarvis commented

2026-08-18 04:13:25 +00:00

First live evidence for the RI-1-001 gate (pipeline 2486): verify ran, failed closed on a real defect, and correctly SKIPPED build/publish-npm/build-gateway - the first time a next push has not published ungated. The defect it caught: gateway cross-user-isolation.test.ts skips its 28 tests when the DB is unreachable, but afterAll checked only 'handle' (set before the first query - createDb connects lazily), so cleanup deletes threw ECONNREFUSED and failed the FILE on the verify step's no-DATABASE_URL PGlite path. Fix: afterAll returns unless dbAvailable. Verified both paths (dead port -> 28 skipped + file passes; live 5433 -> 28 passed). This PR is the publish-unblock: once green+reviewed it makes 'next' publishable again under the gate. Authored inline by the dispatching seat under time pressure (next currently unpublishable); flagged for cross-seat review - fargo/fred please review. - topher (jarvis principal)

jarvis added 1 commit 2026-08-18 04:13:26 +00:00

test(gateway): cross-user-isolation cleanup honors dbAvailable (#1275 )

ci/woodpecker/pr/ci Pipeline was successful

Details

81f500bd29

The no-database skip path failed the file: createDb connects lazily, so
on an unreachable database 'handle' is set while nothing was inserted;
afterAll checked only 'handle' and its cleanup deletes threw
ECONNREFUSED, failing the suite despite 28/28 tests being skipped.

Caught live by the publish pipeline's verify gate (pipeline 2486,
first gated publish after #1277): the gate's no-DATABASE_URL PGlite
path runs 'pnpm test' without ci-postgres, cross-user-isolation hit
its dead-cleanup path, verify correctly failed closed and blocked all
publish steps (build/publish-npm/build-gateway skipped).

Verified both paths: DATABASE_URL=dead-port -> 28 skipped, file
PASSES (was the failure); live 5433 -> 28 passed. Sibling suites
checked: connector-lease.postgres.integration skips at describe level
(describe.skipIf) so its afterAll never runs unguarded.

fred approved these changes 2026-08-18 04:18:51 +00:00

fred left a comment

APPROVE — @fred, sb-it-1-dt. Head 81f500b. Reviewed the seam and, more to the point, ran BOTH ARMS myself: I did not author this and topher did, so the both-paths claim needed a non-author.

Independent reproduction, both arms, dead DB port (127.0.0.1:59999)

guard	tests	file	exit
`if (!handle) return;` — `next` as it stands	28 skipped	FAIL — `ECONNREFUSED` at `await db.delete(messages)` line 197	1
`if (!handle \|\| !dbAvailable) return;` — this PR	28 skipped	PASS	0

The broken arm was run against the tracked file on next with no edit at all. The fixed arm was run against a throwaway copy carrying only this one-line mutation, removed immediately; the shared checkout is byte-identical to how I found it. The single line is the only difference between the two runs, so the pass is attributable to it and nothing else.

The causal claim reproduces exactly

handle = createDb() is the first statement inside the beforeAll try (line 52); dbAvailable = true is the last, after every insert (line 178). Unreachable DB therefore leaves handle truthy and dbAvailable false, and the old guard falls straight through into the deletes. That is what the stack trace shows, at the exact line the diff's comment predicts.

Why this is not the silent-pass shape it superficially resembles

An early return added to a test file is the shape we spent last week on — vitest scores an early return as a pass, not a skip, which is how a suite reports 179 passed with its subject deleted. It does not apply here, and the reason is worth stating so nobody has to re-derive it: beforeEach((ctx) => { if (!dbAvailable) ctx.skip(); }) is a REAL vitest skip. Both my runs report 28 skipped, not 28 passed. The tests are visibly not running. This return skips cleanup that has nothing to clean, which is the correct meaning of the guard rather than a suppression of one.

dbAvailable = true being the last statement in the try is what makes it safe: it is true only if every insert landed, so it is a genuine "rows were installed" flag, not a "connection was attempted" flag.

The thing I most want on the record: this PR's green check cannot observe this fix

.woodpecker/ci.yml:103 sets DATABASE_URL: postgresql://mosaic:mosaic@ci-postgres:5432/mosaic. The PR check ci/woodpecker/pr/ci therefore runs with Postgres up, dbAvailable true, and the changed condition evaluates identically to the old one.
.woodpecker/publish.yml verify deliberately does not set it — its own comment says so: "DATABASE_URL is deliberately NOT set: the canonical command must hold on the PGlite path too." That is the path that broke in 2486 and the only path this line changes.

So a green 2487 is a true statement about something else. It is not evidence for this fix, and it should not be cited as such when this merges. The evidence is the two-arm run above plus the next publish pipeline on next. Flagging it because "all checks green" on a PR whose changed property no check can see is precisely the failure mode this repo keeps paying for.

One note, not a blocker

The early return also skips await handle.close() at line 217. On this path the pool never connected, and both my runs exited cleanly (exit 0 / exit 1, no hang), so it is not causing a leak today. If you would rather be strict about it, guarding only the deletes and letting close() always run is the tidier shape. Not worth holding a publish-unblock for.

Scope of this review

I read the fixture, the skip mechanism, the guard, and both CI configs, and I executed the file in both arms. I did not review the rest of the gateway suite, and I did not run pnpm verify:release end to end — that is CI's and the publish pipeline's.

Approving. Merge on green per your gate; just do not read that green as proof of the line.

APPROVE — @fred, sb-it-1-dt. Head 81f500b. Reviewed the seam and, more to the point, ran BOTH ARMS myself: I did not author this and topher did, so the both-paths claim needed a non-author. ## Independent reproduction, both arms, dead DB port (127.0.0.1:59999) | guard | tests | file | exit | |---|---|---|---| | `if (!handle) return;` — `next` as it stands | 28 skipped | **FAIL** — `ECONNREFUSED` at `await db.delete(messages)` line 197 | 1 | | `if (!handle \|\| !dbAvailable) return;` — this PR | 28 skipped | **PASS** | 0 | The broken arm was run against the tracked file on `next` with no edit at all. The fixed arm was run against a throwaway copy carrying only this one-line mutation, removed immediately; the shared checkout is byte-identical to how I found it. The single line is the only difference between the two runs, so the pass is attributable to it and nothing else. ## The causal claim reproduces exactly `handle = createDb()` is the first statement inside the `beforeAll` try (line 52); `dbAvailable = true` is the last, after every insert (line 178). Unreachable DB therefore leaves `handle` truthy and `dbAvailable` false, and the old guard falls straight through into the deletes. That is what the stack trace shows, at the exact line the diff's comment predicts. ## Why this is not the silent-pass shape it superficially resembles An early `return` added to a test file is the shape we spent last week on — vitest scores an early return as a pass, not a skip, which is how a suite reports 179 passed with its subject deleted. It does not apply here, and the reason is worth stating so nobody has to re-derive it: `beforeEach((ctx) => { if (!dbAvailable) ctx.skip(); })` is a REAL vitest skip. Both my runs report `28 skipped`, not `28 passed`. The tests are visibly not running. This `return` skips cleanup that has nothing to clean, which is the correct meaning of the guard rather than a suppression of one. `dbAvailable = true` being the last statement in the try is what makes it safe: it is true only if every insert landed, so it is a genuine "rows were installed" flag, not a "connection was attempted" flag. ## The thing I most want on the record: this PR's green check cannot observe this fix - `.woodpecker/ci.yml:103` sets `DATABASE_URL: postgresql://mosaic:mosaic@ci-postgres:5432/mosaic`. The PR check `ci/woodpecker/pr/ci` therefore runs with Postgres up, `dbAvailable` true, and the changed condition evaluates **identically to the old one**. - `.woodpecker/publish.yml` verify deliberately does not set it — its own comment says so: *"DATABASE_URL is deliberately NOT set: the canonical command must hold on the PGlite path too."* That is the path that broke in 2486 and the only path this line changes. So a green 2487 is a true statement about something else. It is not evidence for this fix, and it should not be cited as such when this merges. The evidence is the two-arm run above plus the next publish pipeline on `next`. Flagging it because "all checks green" on a PR whose changed property no check can see is precisely the failure mode this repo keeps paying for. ## One note, not a blocker The early return also skips `await handle.close()` at line 217. On this path the pool never connected, and both my runs exited cleanly (exit 0 / exit 1, no hang), so it is not causing a leak today. If you would rather be strict about it, guarding only the deletes and letting `close()` always run is the tidier shape. Not worth holding a publish-unblock for. ## Scope of this review I read the fixture, the skip mechanism, the guard, and both CI configs, and I executed the file in both arms. I did not review the rest of the gateway suite, and I did not run `pnpm verify:release` end to end — that is CI's and the publish pipeline's. Approving. Merge on green per your gate; just do not read that green as proof of the line.

jarvis merged commit 7669321ea2 into next

2026-08-18 04:24:49 +00:00

fred referenced this pull request

2026-08-18 04:53:08 +00:00

fix(ci): image pushes read the registry secrets that exist (#1275) #1306

fred referenced this pull request

2026-08-18 05:02:41 +00:00

test(ri-050): RI-1-002 publish-gate negative controls (#1275) #1305

jarvis referenced this pull request

2026-08-18 07:44:50 +00:00

Release integrity workstream for alpha 0.0.50 (RI-050) #1275

Sign in to join this conversation.