mirror of
https://github.com/block/buzz.git
synced 2026-10-31 22:41:12 +00:00
c05cdb15d1
Review (Wren, ratified by Eva) found a renewal-vs-withdrawal race in the reconciler. `reconcile()` snapshotted `desired()` and `established` once, then ran both loops against those snapshots with awaited Redis round-trips in between. Its own doc comment claimed the opposite — "read immediately before each command" — describing a contract the code did not implement. I wrote that comment, and it is exactly how the next reader would have re-introduced this. The lost interleaving: desire drops community C, the pass records `have` containing C, and before the unsubscribe loop reaches C a new authz resolution calls `admit(C)`. `admit` sees the still-present establishment bit, permits a cache insert, and the reconciler then withdraws establishment and unsubscribes from its stale view. The entry stays readable for its whole 10s TTL with no invalidation channel behind it — the silent stale-authz hole this lane exists to close, reached through our own unsubscribe path instead of AWS's restriction. A per-command re-read does not fix it: renewal can still land between the check and the removal. `admit()`'s wake shortens the gap to milliseconds but cannot close it, because pub/sub has no replay — the tick restores state, nothing restores a dropped message. Fix: `CommunityTopics::withdraw_undesired` re-reads `desired()` *inside* the establishment write lock and returns only the communities to unsubscribe. `admit()` already recorded residency before reading establishment, so the two critical sections are mutually exclusive and only two orders exist: either the renewal's residency write precedes the withdrawal's re-read (interest is seen, the subscription survives) or it follows the withdrawal (establishment reads false, nothing is admitted). An admitted entry can no longer outlive its subscription. `remove_established` is gone, so no caller can withdraw outside the lock; the ordering requirement is documented at both call sites, since each is correct alone and only the pairing carries the property. Subscribe stays on the pass's initial read: holding a subscription nobody wants costs one idle channel until the next pass and can never make an entry readable without coverage. Both families inherit the invariant because it lives in the shared reconciler; conn-control's milder exposure (a lost disconnect leaves a socket the DB ban row still refuses at next authz) is now stated in code rather than implied. Tests, all mutation-verified to bite: - `a_renewal_racing_withdrawal_never_sees_establishment_it_loses` parks the `desired` closure inside the withdrawal critical section and releases a renewal thread from there, forcing the interleaving rather than sleeping for it. Moving the `desired()` read back outside the lock fails it. - `interest_renewed_mid_reconcile_keeps_its_exact_subscription` proves it end-to-end through the real path against live Redis, asserting on `PUBSUB NUMSUB`. Its first draft did NOT bite the reinstated bug twice over: renewing on the pass's first read meant no withdrawal was ever attempted, and a coarse end-state assertion passed vacuously because the next tick re-subscribes. It now discriminates on read count within a window well under `RECONCILE_INTERVAL` and asserts establishment never transiently drops — the transient loss is the bug, the repaired end state is not proof. - `withdrawal_reads_live_desire_not_the_passs_stale_view`, `withdrawal_still_removes_communities_nobody_wants`, and relay-side `admitted_residency_is_visible_to_a_concurrent_withdrawal` cover both directions and the cross-crate pairing. Also corrects two stale comments Eva flagged: `state.rs` named the renamed `may_cache` in an intra-doc link (a real rustdoc `broken_intra_doc_links` warning I introduced), and `event.rs` test prose said "PSUBSCRIBEs" for a path that has always used exact `SUBSCRIBE`. Verified at this tree: buzz-pubsub 52/52 `--include-ignored` against standalone Redis 8.6.2 and against a cluster-mode rig (`cluster_state:ok`, 16384 slots); dead-port control fails all 6 Redis-backed reconciliation tests by name while the 7 pure-logic tests still pass; buzz-relay --lib 807 passed / 0 failed; workspace `clippy --all-targets -D warnings` rc=0; `cargo fmt --all --check` clean. Co-authored-by: npub17jjz49l9jjmhhk7cac63j8yt9z555n9cw8vk7v5jz4vzw4ppld5qgj57cc <f4a42a97e594b77bdbd8ee35191c8b28a94a4cb871d96f32921558275421fb68@buzz.block.builderlab.xyz> Signed-off-by: npub17jjz49l9jjmhhk7cac63j8yt9z555n9cw8vk7v5jz4vzw4ppld5qgj57cc <f4a42a97e594b77bdbd8ee35191c8b28a94a4cb871d96f32921558275421fb68@buzz.block.builderlab.xyz>