mirror of
https://github.com/block/buzz.git
synced 2026-08-18 06:50:31 +02:00
fix(db): use microsecond stamps in claim_due_reminder to close same-second ABA
The rollback guard `delivered_at = $stamp` was sound only at second granularity because `claim_due_reminder` wrote `Utc::now().timestamp()`. Two claims in the same wall-clock second produced the same i64 stamp, so a stale rollback from a slow pod could match (and silently clobber) a live claim freshly written by a fast pod. That's the very ABA hazard the guard exists to prevent. Switch the claim write to `Utc::now().timestamp_micros()`. The column stays `BIGINT` — microsecond-epoch values fit, and no reader interprets `delivered_at` as a whole-seconds count (every read is `IS NULL` / `IS NOT NULL`; verified across schema.sql:138, migrations/0003:17, event.rs:1049). No migration needed. Tests: - Add `rollback_under_same_wall_clock_second_does_not_clobber`: two back-to-back claims (no sleep) with a rollback between, assert the stamps are distinct, then prove the stale stamp_a rollback against the live stamp_b row returns false and the row is unchanged. Pins the actual race Eva [4] flagged — under seconds-granularity stamp_a == stamp_b on any modern host, the assertion fails, and the stale rollback silently clobbers. - Update `rollback_with_matching_stamp_reverts_then_reclaim_wins` comment that previously cited 1-second resolution as the reason for not comparing stamps. No longer true. - Update `rollback_with_stale_stamp_does_not_clobber_live_claim` comment: stamp_old+1 is now a 1-microsecond delta, not 1 second. Adversarial: reverted `claim_due_reminder` to `Utc::now().timestamp()`, ran the new test → loud fail at `assert_ne!(stamp_a, stamp_b)` with `stamp_a=1782498896, stamp_b=1782498896`. Restored. Validation: - cargo fmt --check (buzz-db, buzz-relay): OK - cargo clippy --tests --no-deps -- -D warnings (buzz-db, buzz-relay): OK - cargo test -p buzz-db --lib --include-ignored --test-threads=1: 88 pass - cargo test -p buzz-relay --lib: 369 pass - cargo check -p buzz-relay: OK Co-authored-by: Tyler Longwell <tlongwell@block.xyz> Signed-off-by: Tyler Longwell <tlongwell@block.xyz>
This commit is contained in:
co-authored by
Tyler Longwell
parent
a88c18f029
commit
f4dbd208f5
+107
-8
@@ -1083,12 +1083,19 @@ pub async fn query_due_reminders(
|
||||
/// [`rollback_reminder_claim`] if a subsequent side effect (e.g. Redis
|
||||
/// publish) fails — the rollback guards on `delivered_at = stamp` so a
|
||||
/// stale rollback can never clobber a fresh winner's claim.
|
||||
///
|
||||
/// The stamp is `Utc::now().timestamp_micros()` (microsecond epoch). Using
|
||||
/// whole seconds would let two claims in the same wall-clock second share
|
||||
/// a stamp and defeat the rollback guard — the very ABA bug the guard
|
||||
/// exists to prevent. Microseconds fit in `BIGINT` and no other reader
|
||||
/// interprets `delivered_at` as a whole-seconds count (it's only ever
|
||||
/// checked `IS NULL`/`IS NOT NULL`).
|
||||
pub async fn claim_due_reminder(
|
||||
pool: &PgPool,
|
||||
event_id: &[u8],
|
||||
event_created_at: DateTime<Utc>,
|
||||
) -> Result<Option<i64>> {
|
||||
let now_epoch = Utc::now().timestamp();
|
||||
let stamp = Utc::now().timestamp_micros();
|
||||
let result = sqlx::query(
|
||||
r#"
|
||||
UPDATE events
|
||||
@@ -1096,14 +1103,14 @@ pub async fn claim_due_reminder(
|
||||
WHERE created_at = $2 AND id = $3 AND delivered_at IS NULL
|
||||
"#,
|
||||
)
|
||||
.bind(now_epoch)
|
||||
.bind(stamp)
|
||||
.bind(event_created_at)
|
||||
.bind(event_id)
|
||||
.execute(pool)
|
||||
.await?;
|
||||
|
||||
if result.rows_affected() > 0 {
|
||||
Ok(Some(now_epoch))
|
||||
Ok(Some(stamp))
|
||||
} else {
|
||||
Ok(None)
|
||||
}
|
||||
@@ -1413,9 +1420,11 @@ mod claim_dedup_tests {
|
||||
assert!(reverted, "rollback with matching stamp must revert the row");
|
||||
|
||||
// After rollback `delivered_at` is NULL again — a second claim wins.
|
||||
// (We don't compare the two stamps: `Utc::now().timestamp()` has
|
||||
// 1-second resolution, so back-to-back claims often share a value.
|
||||
// What matters is that the second claim returns Some, not None.)
|
||||
// The second claim's stamp differs from the first's because
|
||||
// `timestamp_micros()` distinguishes back-to-back calls; we don't
|
||||
// assert that here (the dedicated same-wall-clock-second test below
|
||||
// pins that invariant). What matters here is that the second claim
|
||||
// returns Some, not None.
|
||||
let _stamp_2 = claim_due_reminder(&pool, &id, created_at)
|
||||
.await
|
||||
.expect("second claim runs")
|
||||
@@ -1447,8 +1456,10 @@ mod claim_dedup_tests {
|
||||
.expect("first claim wins");
|
||||
|
||||
// Simulate the row's `delivered_at` advancing (operator manual
|
||||
// clear-then-reclaim, or any post-claim mutation). A stamp 1
|
||||
// second later is sufficient — the guard compares exact equality.
|
||||
// clear-then-reclaim, or any post-claim mutation). Any distinct
|
||||
// value triggers the guard; we use `stamp_old + 1` (1 microsecond
|
||||
// later) because the guard compares exact equality and a 1-µs
|
||||
// delta is the tightest distinct-stamp shape we can construct.
|
||||
let stamp_new = stamp_old + 1;
|
||||
sqlx::query(
|
||||
r#"
|
||||
@@ -1485,4 +1496,92 @@ mod claim_dedup_tests {
|
||||
"delivered_at must remain stamp_new — the stale rollback was correctly a no-op"
|
||||
);
|
||||
}
|
||||
|
||||
/// (§5c acceptance — closes Eva [4]) The realistic ABA window: two
|
||||
/// claims that happen in the same wall-clock second must produce
|
||||
/// distinct stamps, so a stale rollback from the first cannot match
|
||||
/// (and clobber) the live second claim. This pins the actual race —
|
||||
/// without microsecond precision, `Utc::now().timestamp()` would
|
||||
/// return the same `i64` for both back-to-back claims, the rollback
|
||||
/// guard `delivered_at = $stamp_a` would falsely match the live
|
||||
/// `delivered_at = stamp_b` row, and the rollback would revert the
|
||||
/// live claim. A subsequent publish would then re-claim and re-fire,
|
||||
/// producing the silent duplicate-delivery this whole guard exists
|
||||
/// to prevent.
|
||||
///
|
||||
/// Shape: claim → rollback (round-trip ok) → reclaim. The two real
|
||||
/// claims happen at machine speed with no `sleep` — they land in the
|
||||
/// same wall-clock second on any modern host. With microsecond
|
||||
/// stamps they differ by tens of µs; we assert `stamp_a != stamp_b`.
|
||||
/// Then a stale rollback with `stamp_a` against the live `stamp_b`
|
||||
/// row must return false.
|
||||
///
|
||||
/// Adversarial: revert `claim_due_reminder` to `Utc::now().timestamp()`
|
||||
/// (whole seconds). On the same wall-clock second, `stamp_a == stamp_b`,
|
||||
/// the `stamp_a != stamp_b` assertion fails, AND the stale rollback
|
||||
/// returns true (silently clobbering the live claim). Verified by
|
||||
/// hand before commit.
|
||||
#[tokio::test]
|
||||
#[ignore = "requires Postgres"]
|
||||
async fn rollback_under_same_wall_clock_second_does_not_clobber() {
|
||||
let pool = setup_pool().await;
|
||||
let (id, created_at) = insert_due_reminder(&pool).await;
|
||||
|
||||
// First claim — the "slow pod" whose publish will (notionally) fail.
|
||||
let stamp_a = claim_due_reminder(&pool, &id, created_at)
|
||||
.await
|
||||
.expect("first claim runs")
|
||||
.expect("first claim wins");
|
||||
|
||||
// Round-trip the rollback (this is the slow pod's publish-failure
|
||||
// path completing successfully — but imagine its `rollback` call
|
||||
// hasn't reached the DB yet in the racy version).
|
||||
let reverted_a = rollback_reminder_claim(&pool, &id, created_at, stamp_a)
|
||||
.await
|
||||
.expect("rollback A runs");
|
||||
assert!(reverted_a, "first rollback must revert (stamps match)");
|
||||
|
||||
// Second claim immediately after — the "fast pod" that wins next.
|
||||
// On the same wall-clock second as the first claim — no sleep.
|
||||
let stamp_b = claim_due_reminder(&pool, &id, created_at)
|
||||
.await
|
||||
.expect("second claim runs")
|
||||
.expect("second claim wins after rollback");
|
||||
|
||||
// The forcing assertion: under microsecond stamps these MUST
|
||||
// differ. Under the old seconds-granularity bug, `stamp_a ==
|
||||
// stamp_b` (machine speed → same second), and this assertion
|
||||
// is what fails first when the fix is reverted.
|
||||
assert_ne!(
|
||||
stamp_a, stamp_b,
|
||||
"back-to-back claims must produce distinct stamps — \
|
||||
same-stamp collision is the ABA bug Eva [4] flagged. \
|
||||
stamp_a={stamp_a}, stamp_b={stamp_b}"
|
||||
);
|
||||
|
||||
// Now the racy late rollback: slow pod fires its stale rollback
|
||||
// with `stamp_a` against the live `stamp_b` row. Must return
|
||||
// false — guard rejects the stale stamp.
|
||||
let reverted_stale = rollback_reminder_claim(&pool, &id, created_at, stamp_a)
|
||||
.await
|
||||
.expect("stale rollback runs");
|
||||
assert!(
|
||||
!reverted_stale,
|
||||
"stale rollback with stamp_a must NOT revert the live stamp_b row"
|
||||
);
|
||||
|
||||
// Confirm the row's `delivered_at` is still `stamp_b`, not NULL.
|
||||
let current: Option<i64> =
|
||||
sqlx::query_scalar("SELECT delivered_at FROM events WHERE created_at = $1 AND id = $2")
|
||||
.bind(created_at)
|
||||
.bind(&id)
|
||||
.fetch_one(&pool)
|
||||
.await
|
||||
.expect("re-read delivered_at");
|
||||
assert_eq!(
|
||||
current,
|
||||
Some(stamp_b),
|
||||
"delivered_at must remain stamp_b — the stale rollback was correctly a no-op"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user