From af4ab6734b9bc5b639e993abe9c6b62963198ee8 Mon Sep 17 00:00:00 2001 From: Matt Wilkinson Date: Tue, 28 Jul 2026 02:56:14 -0400 Subject: [PATCH 1/5] fix(server): gate the shutdown test on the boundary frame, not frame one TestServeShutdownWithLiveCommsSubscriberReturnsClean opens SubscribeComms(SinceSeq: 0) and read the FIRST frame as its seeded ChannelChanged. A since_seq=0 subscribe receives commsSnapshotBoundary before any event, so the gate was asserting on a control frame and reading its absent payload as an empty channel id. The test predates the boundary frame. It has been failing on main since that frame was added, and failing at the gate means it never reached the drain assertion it exists for - so the regression it names, that Serve must close commsBus or a live SubscribeComms handler wedges the drain, has been undefended while the test looked like it was doing its job. Consume the boundary and assert it IS the boundary rather than skipping one frame blindly: a bare skip re-breaks silently the next time the preamble grows. The frame is identifiable by construction - seq 0 with no payload, where the terminal resync is seq 0 WITH a CommsResyncRequired. Verified by two mutations. Removing commsBus.Close() from Serve's drain now reddens at the drain assertion (line 247, "want nil (drain must close commsBus)") - previously it died at the gate on line 213 and never got there. Dropping the boundary consume reproduces the original failure exactly, empty channel id and all. Closes SEA-1530 Co-Authored-By: seal --- go/server/serve_pgtest_test.go | 35 ++++++++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/go/server/serve_pgtest_test.go b/go/server/serve_pgtest_test.go index 0d4a8e02..0e54f7d8 100644 --- a/go/server/serve_pgtest_test.go +++ b/go/server/serve_pgtest_test.go @@ -139,6 +139,12 @@ func TestServeShutdownIsClean(t *testing.T) { // nil assertion below — or, if Shutdown blocks past the test deadline, the // timeout fires. Either way the pre-fix drain is caught; the fix returns nil // promptly. +// +// That claim was untrue for as long as this test failed at its subscriber gate: +// it never reached the drain, so the regression it names went undefended while +// the test reported red for an unrelated reason. A red test is not self-evidently +// doing its job - it has to fail at the assertion it exists for, and that is what +// the drain mutation below is checked against. func TestServeShutdownWithLiveCommsSubscriberReturnsClean(t *testing.T) { socketPath := filepath.Join(t.TempDir(), "compass.sock") @@ -183,14 +189,35 @@ func TestServeShutdownWithLiveCommsSubscriberReturnsClean(t *testing.T) { } defer func() { _ = stream.Close() }() - // Drive the stream on a goroutine: gate on the seeded event (registration - // proof), then keep receiving so the stream stays open on the comms bus - // until the server's shutdown drain closes it. The final Receive returning - // false (clean EOF) is what proves the server-side close released it. + // Drive the stream on a goroutine: consume the preamble, gate on the seeded + // event (registration proof), then keep receiving so the stream stays open + // on the comms bus until the server's shutdown drain closes it. The final + // Receive returning false (clean EOF) is what proves the server-side close + // released it. + // + // A since_seq=0 subscribe receives commsSnapshotBoundary FIRST, before any + // event (internal/comms/subscribe.go). Assert that frame rather than + // skipping one blindly: a bare skip silently re-breaks the moment the + // preamble grows a second frame, which is how this gate broke in the first + // place - it was written before the boundary existed and read frame one as + // the seeded event, failing here with an empty channel id and never + // reaching the shutdown drain that is its actual subject. + // + // The boundary is identifiable by construction: seq 0 and no payload. The + // terminal resync is also seq 0, so payload is what separates them - it + // carries a CommsResyncRequired and the boundary carries nothing. gate := make(chan error, 1) done := make(chan struct{}) go func() { defer close(done) + if !stream.Receive() { + gate <- fmt.Errorf("stream ended before the snapshot boundary: %w", stream.Err()) + return + } + if seq, payload := stream.Msg().GetSeq(), stream.Msg().GetPayload(); seq != 0 || payload != nil { + gate <- fmt.Errorf("first frame = seq %d payload %T, want the snapshot boundary (seq 0, no payload)", seq, payload) + return + } if !stream.Receive() { gate <- fmt.Errorf("stream ended before the seeded event: %w", stream.Err()) return From bc1b0d517e1cd1e55766d0b4668e0059fe18629d Mon Sep 17 00:00:00 2001 From: Matt Wilkinson Date: Tue, 28 Jul 2026 03:07:33 -0400 Subject: [PATCH 2/5] fix(server): make the shutdown test's own claims self-verifying Round-1 review found two self-referential defects in the comments the fix added, both against the standard those comments state. The header closed with "that is what the drain mutation below is checked against" - and there is no drain mutation below. The mutation was a transient check run by hand; nothing in the tree encodes it. A comment whose subject is that a red test must fail at the assertion it exists for ended by pointing the next maintainer at a check that does not exist to be found. It now states the verification as a reproducible recipe rather than a reference to something absent. The first Receive's error named a missing snapshot boundary, but all that branch observes is a stream that ended before any frame. The handler already has a clean early return on that path, so a future change there would be reported as a boundary defect. The message now describes the observable and leaves the cause to the reader. Also drops a paragraph restating commsSnapshotBoundary's own doc comment one file away, and switches three ASCII prose hyphens to the em-dash the rest of the package uses. Re-verified after the edits rather than relying on the earlier run: the line numbers moved, so the old proof no longer certified this code. Deleting the drain's commsBus.Close() still reddens at the drain assertion, now line 254. Full lane: skips 0, PASS 64, races 0. Refs SEA-1530 Co-Authored-By: seal --- go/server/serve_pgtest_test.go | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/go/server/serve_pgtest_test.go b/go/server/serve_pgtest_test.go index 0e54f7d8..34b843a0 100644 --- a/go/server/serve_pgtest_test.go +++ b/go/server/serve_pgtest_test.go @@ -142,9 +142,10 @@ func TestServeShutdownIsClean(t *testing.T) { // // That claim was untrue for as long as this test failed at its subscriber gate: // it never reached the drain, so the regression it names went undefended while -// the test reported red for an unrelated reason. A red test is not self-evidently -// doing its job - it has to fail at the assertion it exists for, and that is what -// the drain mutation below is checked against. +// the test reported red for an unrelated reason. A red test is not +// self-evidently doing its job — it has to fail at the assertion it exists for. +// Verified by deleting the drain's commsBus.Close() and confirming this test +// then fails at the Serve-returns-nil assertion rather than at the gate. func TestServeShutdownWithLiveCommsSubscriberReturnsClean(t *testing.T) { socketPath := filepath.Join(t.TempDir(), "compass.sock") @@ -196,22 +197,22 @@ func TestServeShutdownWithLiveCommsSubscriberReturnsClean(t *testing.T) { // released it. // // A since_seq=0 subscribe receives commsSnapshotBoundary FIRST, before any - // event (internal/comms/subscribe.go). Assert that frame rather than - // skipping one blindly: a bare skip silently re-breaks the moment the - // preamble grows a second frame, which is how this gate broke in the first - // place - it was written before the boundary existed and read frame one as - // the seeded event, failing here with an empty channel id and never - // reaching the shutdown drain that is its actual subject. - // - // The boundary is identifiable by construction: seq 0 and no payload. The - // terminal resync is also seq 0, so payload is what separates them - it - // carries a CommsResyncRequired and the boundary carries nothing. + // event (internal/comms/subscribe.go, which documents the frame's shape). + // Assert that frame rather than skipping one blindly: a bare skip silently + // re-breaks the moment the preamble grows a second frame, which is how this + // gate broke in the first place — it was written before the boundary + // existed and read frame one as the seeded event, failing here with an + // empty channel id and never reaching the shutdown drain that is its + // actual subject. gate := make(chan error, 1) done := make(chan struct{}) go func() { defer close(done) if !stream.Receive() { - gate <- fmt.Errorf("stream ended before the snapshot boundary: %w", stream.Err()) + // Describe the observable, not a cause: all this proves is that the + // stream ended before any frame arrived. Naming the boundary as the + // missing one would mis-attribute a stream that ended early. + gate <- fmt.Errorf("stream ended before any frame (expected the snapshot boundary first): %w", stream.Err()) return } if seq, payload := stream.Msg().GetSeq(), stream.Msg().GetPayload(); seq != 0 || payload != nil { From 525d0244e6649d289feefaf8c0394e5850eb20f8 Mon Sep 17 00:00:00 2001 From: Matt Wilkinson Date: Tue, 28 Jul 2026 20:34:17 -0400 Subject: [PATCH 3/5] test(server): stop the shutdown pgtest leaking Serve on its failure paths Every t.Fatalf ahead of the happy-path cancel() left Serve running against a live socket and pg pool for the rest of the binary. Adopted from the parallel sealed fix for the same test after comparing both: that lane caught a leak this one did not. Mutation-verified rather than inspected: deleting the drain's commsBus.Close() reds ONLY TestServeShutdownWithLiveCommsSubscriberReturnsClean, at the Serve-returns-nil assertion (line 261), while TestServeShutdownIsClean stays green. Full server + comms pgtest suites pass twice consecutively after restoring the mutated file. Refs SEA-1530, SEA-1423 --- go/server/serve_pgtest_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/go/server/serve_pgtest_test.go b/go/server/serve_pgtest_test.go index 34b843a0..aed609ee 100644 --- a/go/server/serve_pgtest_test.go +++ b/go/server/serve_pgtest_test.go @@ -150,6 +150,13 @@ func TestServeShutdownWithLiveCommsSubscriberReturnsClean(t *testing.T) { socketPath := filepath.Join(t.TempDir(), "compass.sock") serveCtx, cancel := context.WithCancel(context.Background()) + // Idempotent with the explicit cancel() on the happy path below, and the + // reason it is needed: every failure path here t.Fatalf before reaching + // that call, which would leave 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 the two; that lane caught a leak this + // one had. + defer cancel() errCh := make(chan error, 1) go func() { errCh <- Serve(serveCtx, ServeConfig{ From 4cc8b171c354c6ab1b78d77844de5ae803263a7d Mon Sep 17 00:00:00 2001 From: Matt Wilkinson Date: Tue, 28 Jul 2026 21:41:14 -0400 Subject: [PATCH 4/5] test(server): correct the boundary-assertion rationale and bound the final wait The comment justifying the boundary assertion claimed a bare skip re-breaks silently as the preamble grows. A reviewer ran both variants and falsified it: the seeded-event assertion is a hard backstop, so a skip is caught either way. Replace the claim with the measured 2x2 - the gain is attribution and 13s, not catch-vs-miss. Bound the final <-done on testTimeout, per helpers_test.go convention, so a tail loop that never ends fails by name instead of hanging to the package timeout. Proven live by mutation. Trim cross-repo provenance from the defer cancel() comment; it is unresolvable from inside this repo. Co-Authored-By: seal --- go/server/serve_pgtest_test.go | 42 ++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/go/server/serve_pgtest_test.go b/go/server/serve_pgtest_test.go index aed609ee..c35765e0 100644 --- a/go/server/serve_pgtest_test.go +++ b/go/server/serve_pgtest_test.go @@ -150,12 +150,10 @@ func TestServeShutdownWithLiveCommsSubscriberReturnsClean(t *testing.T) { socketPath := filepath.Join(t.TempDir(), "compass.sock") serveCtx, cancel := context.WithCancel(context.Background()) - // Idempotent with the explicit cancel() on the happy path below, and the - // reason it is needed: every failure path here t.Fatalf before reaching - // that call, which would leave 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 the two; that lane caught a leak this - // one had. + // Idempotent with the explicit cancel() on the happy path below. Needed + // because every failure path here t.Fatalf's before reaching that call, + // which would otherwise leave Serve running against a live socket and pg + // pool for the remainder of the binary. defer cancel() errCh := make(chan error, 1) go func() { @@ -204,13 +202,25 @@ func TestServeShutdownWithLiveCommsSubscriberReturnsClean(t *testing.T) { // released it. // // A since_seq=0 subscribe receives commsSnapshotBoundary FIRST, before any - // event (internal/comms/subscribe.go, which documents the frame's shape). - // Assert that frame rather than skipping one blindly: a bare skip silently - // re-breaks the moment the preamble grows a second frame, which is how this - // gate broke in the first place — it was written before the boundary - // existed and read frame one as the seeded event, failing here with an - // empty channel id and never reaching the shutdown drain that is its - // actual subject. + // event (see commsSnapshotBoundary in internal/comms/subscribe.go, whose + // doc comment specifies the frame's shape). + // + // Assert that frame rather than skipping one blindly. The gain is + // attribution, not catch-vs-miss: the seeded-event assertion below is a + // hard backstop either way, because no control frame can satisfy + // GetChannelChanged().GetChannel().GetId() == wantChannelID. Measured, by + // mutating subscribe.go with the skip and the assertion in turn: + // + // preamble grows a second boundary skip: caught, opaque empty channel id + // assert: caught, same + // boundary Send removed entirely skip: caught at 25.4s, gate deadline + // assert: caught at 11.8s, names the frame + // + // So a bare skip is slower and far less diagnostic, not silent. An earlier + // version of this comment claimed it re-breaks silently; a reviewer ran + // both variants and falsified that. The claim was the justification for + // the assertion, and it was never measured — the same defect this test's + // header records, in the prose arguing the fix. gate := make(chan error, 1) done := make(chan struct{}) go func() { @@ -265,5 +275,9 @@ func TestServeShutdownWithLiveCommsSubscriberReturnsClean(t *testing.T) { } // The subscriber's stream ended because the server closed the comms bus. - <-done + select { + case <-done: + case <-timeAfter(): + t.Fatal("subscriber stream never ended after the drain closed the comms bus") + } } From adc6bcb991ba66b52db54638daa0ca1d3a771aff Mon Sep 17 00:00:00 2001 From: Matt Wilkinson Date: Wed, 29 Jul 2026 11:24:38 -0400 Subject: [PATCH 5/5] test(server): trim review-thread archaeology from the shutdown test comment Keep the durable boundary-vs-skip rationale and the measured attribution table; drop the self-referential note about a prior comment revision, which is PR-thread history rather than test explanation. Co-Authored-By: seal --- go/server/serve_pgtest_test.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/go/server/serve_pgtest_test.go b/go/server/serve_pgtest_test.go index c35765e0..1dc60d4b 100644 --- a/go/server/serve_pgtest_test.go +++ b/go/server/serve_pgtest_test.go @@ -216,11 +216,7 @@ func TestServeShutdownWithLiveCommsSubscriberReturnsClean(t *testing.T) { // boundary Send removed entirely skip: caught at 25.4s, gate deadline // assert: caught at 11.8s, names the frame // - // So a bare skip is slower and far less diagnostic, not silent. An earlier - // version of this comment claimed it re-breaks silently; a reviewer ran - // both variants and falsified that. The claim was the justification for - // the assertion, and it was never measured — the same defect this test's - // header records, in the prose arguing the fix. + // So a bare skip is slower and far less diagnostic, not silent. gate := make(chan error, 1) done := make(chan struct{}) go func() {