preflight: wait for the subscription's slot to be released before dropping it

[Kiran01bm]

Why

TestCheckCopySwapShapeRefusesSubscriptionTarget (added in #135) fails intermittently in CI on any PostgreSQL major — seen on 17/18 in the #135 merge run on main and on 15 in #137's run:

ERROR: replication slot "sub_applied" is active for PID 85 (SQLSTATE 55006)

The cleanup disables and drops the subscription, then immediately runs pg_drop_replication_slot('sub_applied') on the publisher. ALTER SUBSCRIPTION ... DISABLE only signals the apply worker to exit; the publisher's walsender holds the slot active until that worker's connection closes, so the drop races the worker.

What

The cleanup polls pg_replication_slots.active for the slot under a named deadline until it is released, then drops it. The subscription is still detached from its slot first, so the drop still needs no publisher connection. Test-only change; no engine behavior touched.

Verification

• scripts/test-flaky.sh TestCheckCopySwapShapeRefusesSubscriptionTarget 20 ./pkg/preflight/ — 20/20 with -race.
• make lint — 0 issues.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 1/2: adversarial correctness review of 7d5b15b. I read the whole cleanup of TestCheckCopySwapShapeRefusesSubscriptionTarget against the slot's lifecycle on the publisher and against RF-2 and ST-3. I ran it, its base, and probes on real PostgreSQL 16 (testcontainers), including a container throttled to 0.05 CPU to stand in for a slow CI runner.

0 blocking, 1 non-blocking.

The diagnosis is right, and the fix waits on the right signal. DROP SUBSCRIPTION waits for the subscriber's apply worker to exit, but the publisher's walsender only lets go of the slot once it sees that connection close. A drop issued in between fails with 55006. Polling pg_replication_slots.active waits for exactly that release, and nothing can take the slot back afterwards: the subscription is gone, so no worker reconnects. The wait is bounded by a named deadline, it runs on the cleanup's WithoutCancel context, and a slot that never releases fails the test instead of hanging it. The PR touches no engine code.

Non-blocking

1. The poll reports a failed read as "the walsender should release the slot". copy_swap_shape_integration_test.go:450-455

return err == nil && !active treats "the read failed" and "the slot is still held" as the same thing. If the slot is missing, or the publisher pool cannot be read, the cleanup waits the full ten seconds and then names the walsender. The real error is never shown. I pointed the poll at a slot name that does not exist to check this: the test fails after 11.4s with Condition never satisfied and the apply worker's walsender should release the slot. EventuallyWithT keeps the two causes apart and reports the last attempt's own error:

assert.EventuallyWithT(t, func(c *assert.CollectT) {
	var active bool
	err := publisher.QueryRow(ctx,
		`SELECT active FROM pg_replication_slots WHERE slot_name = 'sub_applied'`).Scan(&active)
	if !assert.NoError(c, err, "read the slot") {
		return
	}
	assert.False(c, active, "the apply worker's walsender still holds the slot")
}, slotReleaseDeadline, 50*time.Millisecond)

The test passes with this change, three of three runs. With the same wrong slot name, it fails with Received unexpected error: no rows in result set / read the slot.

Verified

• Reproduced the CI failure and the fix. I could not get the flake locally from the real test: base passed 15 of 15 runs, and 10 of 10 with the container throttled from the start, most likely because the worker had not started streaming by the time cleanup ran. A probe that waits until the slot is active, then runs this cleanup in a container throttled to 0.05 CPU, does reproduce it. Without the wait, 4 of 20 drops failed with replication slot "sub_applied" is active for PID … (SQLSTATE 55006), which is the CI error. With the PR's wait, 0 of 60 failed, and the poll read the slot as still active in 4 of them, so the wait was doing real work.
• Nothing takes the slot back after an inactive read. I ran the PR's cleanup 200 times at offsets of 0-39 ms after CREATE SUBSCRIPTION, so it also covers a worker that is still starting up. Every drop after an inactive read succeeded.
• Head and base pass. At 7d5b15b, the test passed 3 of 3 runs, and 10 of 10 with the container throttled. go vet ./pkg/preflight/ is clean.
• RF-2 is upheld. The test still asserts that the subscription target is refused and that the sibling table is accepted. The change only stops its cleanup from failing.
• ST-3 is not touched, because no engine slot code changes.
• No mutation testing of production code, because there is none in the diff. Against the cleanup itself:

Mutant	Caught by
Drop the wait (the base cleanup)	Throttled probe: 4/20 drops fail with 55006. The PR's own test does not catch it locally (15/15 and 10/10 pass), as you would expect for a flake.
Poll a slot name that does not exist	TestCheckCopySwapShapeRefusesSubscriptionTarget: fails after 11.4s, with the misleading message from finding 1

This review was generated by Claude Code (claude-opus-5-5).

[aparajon]

🤖 2/2: OSS adoption and integration ease, at 7d5b15b. These are lenses, not correctness findings. 0 blocking, 1 non-blocking.

This is a test-only change, so it adds no API for an importer to adopt and changes no behavior an importer depends on. What it does is make RF-2's subscription refusal test reliable on every major. That is the evidence an importer such as SchemaBot relies on when it trusts preflight to refuse a table that a subscription applies into.

1. Carry this race into the slot lifecycle manager when ST-3 lands. No code outside the tests calls pg_drop_replication_slot yet. When the engine drops its own decoding slot, it will meet the same window this test hit: the client connection has closed, but the walsender still holds the slot. There, a 55006 needs a bounded wait. It must never read as "dropped" and never be logged and skipped, because either way a slot keeps retaining WAL on the target. That is the failure ST-3 rules out. DROP_REPLICATION_SLOT <name> WAIT on a replication connection does the wait in the server. In the same throttled probe it dropped 20 of 20, including the 2 iterations where the slot was still active at the drop. WAIT has no timeout of its own, so it needs the caller's context deadline. Without one, a slot that never releases hangs the teardown instead of failing it.

This review was generated by Claude Code (claude-opus-5-5).

[aparajon]

🤖 Approving 7d5b15b with 0 blocking findings. The fix waits on the right signal, and I reproduced the 55006 race and its fix in a throttled container. The 1/2 comment has one non-blocking finding: the poll reports a failed read as a slot that is still held. The 2/2 comment has one integration note for ST-3's slot drop.

This stamp was left by Claude Code (claude-opus-5-5).

[Kiran01bm]

🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.6) — pull/140, follow-up commit

Verdict: no blocking findings; the one correctness note is fixed as proposed, and the integration-lens note is tracked as an internal follow-up on the slot-lifecycle row.

#	Finding	Status	Explanation
C1-F1	The poll reports a failed read as "the walsender should release the slot" — err == nil && !active conflates a read error with a slot still held	✅ Fixed	The wait is now assert.EventuallyWithT: a read error is asserted on its own (read the slot) and the active flag separately (the apply worker's walsender still holds the slot), so a missing slot or an unreadable publisher surfaces its own error instead of the deadline message. In the follow-up commit.
C2-F1	Carry this race into the engine's slot drop when ST-3 lands: a 55006 there needs a bounded wait, must never read as dropped, never logged-and-skipped; DROP_REPLICATION_SLOT … WAIT bounded by the caller's context	⏳ Deferred	No engine code drops a slot yet; this PR touches only the preflight test's cleanup. Tracked as an internal follow-up on the slot-lifecycle row (7a) with exactly this shape: server-side WAIT on the replication connection, bounded by the caller's context, 55006 fails closed, plus a test that drops while the slot is still held.

Source: block/pg-sprite#140, review comments 5927064869 and 5927066372 and review 5376391424 at head 7d5b15b; fix in the follow-up commit