fix(server): gate the shutdown test on the boundary frame, not frame one

[mattwilkinsonn]

Fixes SEA-1530. TestServeShutdownWithLiveCommsSubscriberReturnsClean has been failing on clean main, and — the part that matters more than the red — it was failing before it reached the assertion it exists for.

Retargeted onto main (was stacked on #15). The stack was historical, not a dependency — verified rather than assumed:

comm -12 of the two file lists            -> empty (disjoint)
git merge-tree --write-tree main HEAD     -> exit 0
  instrument calibrated: same command on an induced
  same-file conflict returns exit 1, so the 0 is a real
  clean merge and not a vacuous one

#15 is 8 TypeScript/harness files; this is one Go server test file. #15 carries a deliberately-open P1 that waits on another lane's entrypoint, and this fix is one of the two reds blocking #19's pgtest job — so gating a CI fix behind an unrelated open finding bought nothing.

The bug

The test opens SubscribeComms(SinceSeq: 0) and reads the first frame as its seeded ChannelChanged. A since_seq=0 subscribe receives commsSnapshotBoundary before any event (go/internal/comms/subscribe.go:226), so the gate was asserting on a control frame and reading its absent payload as an empty channel id.

I probed the frame rather than inferring it:

PROBE first event payload = <nil> seq=0; channel id = "", want "d0c7cd14…"

payload = <nil>, seq = 0 is the boundary frame exactly.

Why this is worse than a red

The test's subject is the shutdown drain: connect's http.Server.Shutdown does not cancel an in-flight streaming handler's context, so the only thing that releases a live SubscribeComms handler is Serve closing the comms bus. The gate failure meant that assertion was never reached — so the regression the test names has been undefended for as long as the test has been red, while looking like it was doing its job.

Its header comment claimed "either way the pre-fix drain is caught." That was false the whole time. The comment now records why, because the lesson generalizes:

A red test is not self-evidently doing its job — it has to fail at the assertion it exists for. Nobody audits a failing test for whether it is failing in the right place.

The fix

Consume the boundary frame and assert it is the boundary, rather than skipping one frame blindly. The frame is identifiable by construction (subscribe.go:218-225): seq 0 with no payload, where the terminal resync is also seq 0 but carries a CommsResyncRequired.

Verification

Re-run in full after the rebase onto main. The rebase minted new shas (3e054b2 → 525d024; the review round then minted 4cc8b17, whose diff is comment-and-test-wait only, re-verified below), so every earlier measurement was bound to a tree that no longer exists — a green does not travel with a restack.

Two mutations, because a test that goes green after a fix has proven nothing about whether it can still fail.

remove commsBus.Close() from Serve's drain
  -> FAIL at the Serve-returns-nil assertion: "draining compass.v1 servers
     on shutdown: context deadline exceeded, want nil (drain must close
     commsBus)"
  -> ONLY this test reddens; TestServeShutdownIsClean stays green
  -> previously it died at the subscriber gate and never reached the drain

drop the boundary consume (simulate the original bug)
  -> FAIL: "first event channel id = "", want the seeded ..." — the original
     failure reproduced exactly

The only is the load-bearing part: a mutation that reddens several tests proves coverage exists somewhere, not that this test works.

The reviewer added two more the fix had not been proven against — a payload on commsSnapshotBoundary with seq = 0 (fails on the payload clause alone, so GetPayload() != nil is live), and removing the boundary Send entirely (fails at :248 in 11.8s naming the frame).

The review falsified a claim this body made

An earlier version of both this section and the code comment claimed a bare skip "re-breaks silently" as the preamble grows. It does not. The reviewer ran both variants against both mutations and measured:

                                  bare skip              the assertion
preamble grows a 2nd boundary     caught, opaque         caught, opaque
boundary Send removed entirely    caught at 25.4s        caught at 11.8s, by name
                                  (gate deadline)

The seeded-event assertion below is a hard backstop either way: no control frame can satisfy GetChannelChanged().GetChannel().GetId() == wantChannelID. So the assertion's gain is attribution and 13s, not catch-vs-miss — worth keeping, but not for the reason claimed. The claim was the justification for the change and was never measured, which is the same defect this PR exists to fix, reproduced in the prose arguing the fix. Corrected in 4cc8b17; the comment now carries the table above instead.

serve.go was restored after each mutation and re-verified green — the cleanup step is itself a write to the artifact under test.

Run at the scope #19's pipeline uses

#19's pgtest job is go test -tags pgtest -race ./...; its gate job is repo-wide moon run :ci. Run at both packages rather than ./server/ alone, because the fix's subject is a comms-bus interaction and a green scoped to one package is not a green for the job.

go test -tags pgtest -race -v ./internal/comms/... ./server/   (live postgres:17)
  ok    github.com/sealedsecurity/compass/go/internal/comms   371.077s
  FAIL  github.com/sealedsecurity/compass/go/server           335.748s
  PASS: 93    FAIL: 1    skips: 0    DATA RACE: 0

moon run :ci --force
  Tasks: 35 completed    0 failed    2m 0.7s

The two tests this PR turns on:

--- PASS: TestServeShutdownWithLiveCommsSubscriberReturnsClean   (7.62s)
--- PASS: TestSubscribeCommsSnapshotBoundaryFirstFrame          (29.46s)

skips: 0 and the PASS count are #19's own assertions (ci.yml:221), not mine — go test prints ok for a fully-skipped tagged suite, so ok alone is a vacuous green.

Correction: the SEA-1541 red was MY ENVIRONMENT, not this PR and not a defect

Earlier revisions of this body reported a waitListening failure and attributed it to cross-package Postgres contention. Both the failure and the attribution were artifacts of a broken DSN on my side. Recording it rather than deleting it, since the wrong numbers were published:

/tmp/pgdsn.env:  DSN=postgres://…?host=/tmp/pgcs-32431&sslmode=disable
                                                      ^ unquoted &

Sourcing that parses DSN=…pgcs-32431 as a background job and sslmode=disable as a separate assignment, so DSN was never set. Every run passed an empty COMPASS_TEST_DATABASE_DSN, pgtest.RequireDSN took the no-DSN branch, and each test spawned its own podman Postgres.

RequireDSN, real DSN     16.108367ms   dsn len 117
RequireDSN, no DSN        9.066398s    dsn len 72     <- what every earlier run did

TestServeShutdownIsClean, real DSN     ok  1.172s     (15s budget: 13x margin)
TestServeShutdownIsClean, no DSN       FAIL 25.53s

dsn len 72 was in my own log output from the first probe and distinguishes the two paths in one line. The 9.07s container spawn is the entire "fixed interval" I had described as the key insight — it is startContainer's cost, not a schema cost and not contention.

Every measurement in this section's earlier form is withdrawn: the 25.4-25.7s cluster, the contention mechanism, and a proposed RequireDSN hoist that "fixed" a 10.9s self-inflicted delay. SEA-1541 carries the full retraction.

The check that would have caught it before the first measurement: [ -n "$DSN" ] || exit 1. A DSN contains & and ?, so it is unsafe as an unquoted shell value, and the failure is silent — no error, an empty variable, and a suite that transparently falls back to a slower path which still mostly passes.

Also in this branch: a Serve leak on every failure path

defer cancel() added to the shutdown test. Every t.Fatalf ahead of the happy-path cancel() left Serve running against a live socket and pg pool for the remainder of the binary. Adopted from the parallel sealed fix (sealed#973) after comparing both — that lane caught a leak this one had, and mine was the smaller diff without being the better one. That provenance lived in the code comment until the review; sealed#973 is unresolvable from inside this repo, so it belongs here.

Also in this branch: the final wait is now bounded

<-done was the only unguarded blocking wait in the test, against helpers_test.go:23-27's convention that testTimeout bounds every stream wait so a broken handler fails fast. Unreachable today — Serve returning nil implies the drain ran commsBus.Close() — but it converts a future hang-to-package-timeout into a named failure, which is this PR's own defect class. Proven live rather than assumed:

mutate `case <-done:` -> a channel that never closes
  -> FAIL serve_pgtest_test.go:281: subscriber stream never ended
     after the drain closed the comms bus

Re-verified at 4cc8b17

moon run :ci --force            -> 35 tasks, 0 failed
go test -tags pgtest -race ./internal/comms/... ./server/
  -> EXIT=0, 94 PASS / 0 FAIL, in 12.5s   (real DSN, asserted non-empty)
  incl. TestServeShutdownWithLiveCommsSubscriberReturnsClean (12.13s)
  incl. TestSubscribeCommsSnapshotBoundaryFirstFrame (24.13s, in ./internal/comms)

No failures. An earlier revision reported one here; see the correction above — it was a podman-path artifact of an unset DSN, and with a real DSN the same two packages complete in 12.5s.

This repo has no CI today. git ls-tree origin/main .github/workflows/ is empty and #19 is still OPEN, so the above is locally-verified, never CI-verified. The distinction is the point: it is evidence I produced about my own work, and #19's own pgtest job is the thing that would make it independent.

Closes SEA-1530

[linear-code]

SEA-1530

[mattwilkinsonn]

• fix(server): gate the shutdown test on the boundary frame, not frame one #21 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[greptile-apps]

Greptile Summary

The PR corrects and hardens the live-subscriber shutdown regression test.

• Consumes and validates the snapshot-boundary frame before checking the seeded comms event.
• Ensures early test failures cancel the running server.
• Bounds the final wait for subscriber-stream termination.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
go/server/serve_pgtest_test.go	The updated test now follows the documented boundary-first stream protocol, reliably reaches the shutdown-drain assertion, and bounds cleanup waits.

_{Reviews (5): Last reviewed commit: "test(server): trim review-thread archaeo..." | Re-trigger Greptile}