max / makenotwork

Fix the flaky webhook-lock test that was reddening the gate try_lock_event_is_non_blocking_and_per_event failed about one run in three under load, asserting re-acquisition immediately after `drop(held)`. Dropping a sqlx Transaction does not run its ROLLBACK inline -- it queues it onto the connection, to be flushed when that connection is next used or returned to the pool -- so the advisory lock clears eventually, not by the time drop returns. The assertion raced that flush. Failing runs took 5.2s and printed the harness's "pool did not close in 5s; forcing"; passing runs took 0.2s. The test now releases with an explicit rollback().await, which is the deterministic guarantee a caller can actually rely on. 13 consecutive passes, 6 of them on the slow pool-contention path that used to fail. Production is unaffected: the losing delivery has already shed its connection and Stripe redelivers on its own backoff. The doc comment on try_lock_event claimed drop "releases the lock" without qualification, which is what the test was written against, so it now states that the release is prompt but not synchronous and points callers needing it sooner at rollback().await. This mattered because the test sits inside Sando's cargo_test gate, where an intermittent red blocks promotes for no real reason.

Author: Max Johnson <me@maxj.phd> · 2026-07-21 21:28 UTC

Signed with PGP, not checked

Commit: 3efc1fe4c5bdd74f123d8d09aec504f69cadfb7a

Parent: a64ec63

2 files changed, +23 insertions, -3 deletions

M server/src/db/webhook_events.rs +8 -1

			@@ -87,7 +87,14 @@
87	87		/// early return, `?`, or panic rolls the (write-free) transaction back and
88	88		/// releases the lock. It cannot leak the way a pooled session lock would. When
89	89		/// the lock is not acquired the returned `tx` is dropped here holding nothing,
90		-	/// so there is no lock to leak. The key is namespaced (`stripe_webhook:` prefix)
	90	+	/// so there is no lock to leak.
	91	+	///
	92	+	/// That release is prompt but not synchronous: dropping a sqlx `Transaction`
	93	+	/// queues the ROLLBACK onto the connection rather than awaiting it, so the lock
	94	+	/// clears when the connection is next used or returned to the pool. Nothing in
	95	+	/// the webhook path depends on the difference (the loser has already shed its
	96	+	/// connection and Stripe redelivers on its own backoff). Callers that do need
	97	+	/// the lock gone before their next acquire must `rollback().await` explicitly. The key is namespaced (`stripe_webhook:` prefix)
91	98		/// so it shares no space with the other `hashtextextended` advisory locks in the
92	99		/// codebase (reports, oauth).
93	100		#[tracing::instrument(skip_all)]

M server/tests/workflows/db_webhook_events.rs +15 -2

			@@ -157,13 +157,26 @@
157	157		);
158	158
159	159		// Releasing the first guard frees event A for the next delivery.
160		-	drop(held);
	160	+	//
	161	+	// Rolled back explicitly rather than dropped. Dropping a sqlx `Transaction`
	162	+	// does not run the ROLLBACK inline — it queues it onto the connection, to be
	163	+	// flushed when that connection is next used or returned to the pool. The
	164	+	// lock is therefore released eventually, not by the time `drop` returns,
	165	+	// and asserting re-acquisition straight after a bare `drop(held)` raced that
	166	+	// flush: this test failed roughly one run in three under load, and was the
	167	+	// only flake in the suite once Sando started gating on it. Production is
	168	+	// unaffected (the loser sheds its connection and Stripe redelivers later),
	169	+	// but the deterministic release is what a caller can actually rely on, so it
	170	+	// is what the test asserts.
	171	+	held.rollback()
	172	+	.await
	173	+	.expect("releasing the held lock must not error");
161	174		assert!(
162	175		webhook_events::try_lock_event(&db.pool, "evt_lock_A")
163	176		.await
164	177		.expect("post-release try_lock must not error")
165	178		.is_some(),
166		-	"the lock must be re-acquirable once the holder drops"
	179	+	"the lock must be re-acquirable once the holder releases it"
167	180		);
168	181		}
169	182