Keep the two claim-loop tests written during the #119 CI investigation - #124
Conversation
Both were written as throwaway diagnostics while investigating a `two_concurrent_claimers_never_both_win_the_same_row` CI failure that turned out to be a bad assertion rather than a real double-claim. They are worth keeping as standing regression tests, so this renames them, drops the "diagnostic" framing, and states the invariant each one guards. - `a_second_claim_batch_call_picks_up_the_row_the_routing_hop_just_queued` pins `take_lease`'s `accepted` branch leaving no real lease. If a future change gives that hop a real lease, every `accepted` row would stall behind a lease it never needed, and this fails instead of it going unnoticed. It also stands as the answer to a misreading that has already cost real time once: one message id appearing in two claimers' results is not evidence of a double-claim, which is exactly why the neighbouring test now asserts on reaching `routed`. - `concurrent_if_match_updates_never_both_win` races N concurrent `if_match` updates against one row at one starting version, bypassing `claim_batch` entirely, and asserts exactly one wins. This is the guarantee the whole claim loop rests on, one layer below it. `cratestack` is pinned exactly and moves fast; if a future bump ever weakened `if_match`, every CAS claim in this system would begin double-claiming with nothing else noticing. Verified sound on the current pin (=0.6.7). Both take the per-binary `TEST_MUTEX` and the standard live-Postgres `#[ignore]` reason, so they run under `just test-live` and the CI live job like every other suite. Their original `#[ignore = "diagnostic — not for CI"]` would not have excluded them anyway: the live job runs `cargo test --workspace -- --ignored`, which runs every ignored test regardless of the reason string. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
![lightbridge-assistant[bot]](https://avatars.githubusercontent.com/u/128943481?s=60&v=4){kind=small}
There was a problem hiding this comment.
🅵 Fast automated pass — SAST + a quick, diff-scoped look (no repo-wide retrieval). For a deeper, repo-aware review, mention @lightbridge-assistant on this PR.
Test-only change (two #[ignore]d live-Postgres tests), no production code touched, SAST clean. The CAS regression test concurrent_if_match_updates_never_both_win is sound (8-racer if_match(version) race, wins==1 per round asserted). The one finding: a_second_claim_batch_call_picks_up_the_row_... doesn't actually assert its central claim — the second-call re-pickup and total_wins==2 are only printed, so it would pass even if take_lease regressed to leave a real lease, contradicting its stated purpose of failing loudly.
🤖 AI-generated review — treat it as untrusted, verify before acting; a human owns the final decision (AI governance).
| let total_wins = first | ||
| .iter() | ||
| .chain(second.iter()) | ||
| .filter(|m| m.id == seeded.id) |
There was a problem hiding this comment.
Central claim only printed, never asserted
The test's documented purpose ("Locks in take_lease's accepted branch ... so a future change to that hop fails loudly here instead of silently stalling") is not actually asserted. The only real assertion is after_first.state == queued. The test's central claim — that a second, separate claim_batch call genuinely picks up the same row (worker-b saw the same id again, total_wins == 2) — is computed and printed but never checked. If take_lease were changed to leave a real future lease (the exact regression this test exists to catch), the test would still pass.
Evidence: Lines 458–470: after_second and total_wins are derived but only consumed by println!; no assert!(after_second.is_some()) / assert_eq!(total_wins, 2). The only assert_eq! in this test is line 446 (after_first.state == queued).
| .filter(|m| m.id == seeded.id) | |
| assert!(after_second.is_some(), "second claim_batch must pick up the row the routing hop just queued"); | |
| assert_eq!(total_wins, 2); |
Was this useful? React 👍/👎 to give us feedback
CI's live job failed on reclaims_a_routed_row_abandoned_by_a_crashed_worker while both new tests (concurrent_if_match_updates_never_both_win, a_second_claim_batch_call_picks_up_the_row_the_routing_hop_just_queued) passed. Locally this suite passed 9/9 repeatedly, including a full just test-live sweep — the failure was CI-only, a slower runner widening a window that already existed. Confirmed mechanism: candidates() orders by `priority DESC, createdAt ASC LIMIT budget`, and every fixture in this file seeds priority: 1000 (the max), so ties break purely by age. This binary's database is never reset between tests or between cargo test invocations. concurrent_if_match_updates_never_both_win alone leaves 15 accepted rows behind every run (only the winning racer's stateReason changes; state stays accepted, since that field is deliberately not what the race updates), and a_second_claim_batch_call_picks_up_the_row_the_routing_hop_just_queued leaves one more. That older-createdAt residue can fill budget=10 before reclaims_a_routed_row_abandoned_by_a_crashed_worker's own freshly-abandoned (and therefore newest-createdAt) row is reached — on a slow enough runner, exactly the failure CI hit. Same defect shape dispatch_live_postgres.rs already found and fixed (#119). Reused its proven fix rather than inventing a second mechanism: clear_claimable_backlog drains every accepted/queued/routed row to cancelled (reachable from all three per §2.10) before a test seeds its own fixture, through CrateStack delegates only (R1); isolated_db() wraps db() with it so no new test can forget the call. The two tests that build their own Cratestack pool directly (bigger connection limits) call clear_claimable_backlog explicitly instead of going through isolated_db(). ca653a1's TEST_MUTEX is untouched and still required — it serializes execution within this binary; it was never meant to, and doesn't, clean up residue between tests. Verified: cargo test -p sms-worker --test claim_live_postgres -- --ignored 10 consecutive runs, 9/9 green every time. Two full just test-live sweeps green. just check, just lint, just parity, and ./ci/assert-no-raw-sqlx.sh all green. No production code touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Summary
Promotes two tests written as throwaway diagnostics during the #119 CI investigation into standing regression tests in
crates/sms-worker/tests/claim_live_postgres.rs. Renamed, reframed, and given the file's standard live-Postgres#[ignore]reason so they run like every other suite.Test-only. No production code touched (
crates/sms-worker/src/claim.rsis unchanged).Intent
While #119 was blocked on CI,
two_concurrent_claimers_never_both_win_the_same_rowfailed withwins: 2. That assertion's own doc comment says it exists to catch a CAS failure that would mean two workers submitting the same SMS — so it was investigated as a possible production double-send.It was not one.
take_lease'sacceptedbranch deliberately leaves no real lease (claim.rs:205-226), so a row is immediately re-claimable, anddispatch.rs::tick()submits only rows in staterouted. Twoclaim_batchcalls can therefore both return one id — one doing the freeaccepted -> queuedhop, one the real claim — with no CAS violation. The assertion was counting the wrong thing; #120 corrected it to assert on reachingrouted.The two tests written to establish that are worth more than the investigation that produced them. Source of truth: #119, #120, and
claim.rs's own documented design.Scope
a_second_claim_batch_call_picks_up_the_row_the_routing_hop_just_queued(wasdiagnostic_second_claim_batch_call_...) — pins the no-real-lease routing hop. Deterministic and not concurrent at all; it reproduces the "two wins" observation with two sequential calls. If a future change gives that hop a real lease, everyacceptedrow would stall behind a lease it never needed and this fails loudly. It also stands as the written answer to a misreading that cost real time once.concurrent_if_match_updates_never_both_win(wasdiagnostic_raw_cas_never_double_wins) — races 8 concurrentif_matchupdates against one row at one starting version, over 15 rounds, bypassingclaim_batch/Claimableentirely. Asserts exactly one winner and that the rest getPreconditionFailed. This is the guarantee the whole claim loop rests on, one layer below it.Why keep the second one specifically:
cratestackis pinned exactly and moves fast (=0.6.7today, and the family has been bumped repeatedly). If a future bump ever weakenedif_match, every CAS claim in this system would begin double-claiming and no other test would notice —#87is this repo's standing example of exactly that shape of silent regression.A real defect in the tests as originally written: both carried
#[ignore = "diagnostic — not for CI"], which does not exclude anything. The live job runscargo test --workspace -- --ignored, which runs every ignored test regardless of the reason string — so they would have run in CI while claiming not to. Both now carry the file's standard reason, and are intended to run.Verification
just lintgreen. Notably, the tests passed undercargo testwhile failingclippy -D warningswith 3 errors (adoc_markdownlint and twoitems_after_statements), since warnings are not errors undercargo test. Fixed; worth flagging as a reminder that a green test run does not imply a green lint job.cargo test -p sms-worker --test claim_live_postgres -- --ignored— 9/9 passed, three consecutive runs (1.57s / 1.43s / 1.50s).just test-livesweep green, all suites.Screenshots/Evidence
N/A — test-only change.
Risk Assessment
Low. Test-only, no production code. Adds ~1.5s to the claim suite. The concurrency test is bounded (15 rounds x 8 racers) and was stable across every run; if it ever proves flaky on a slower CI runner it should be investigated as a real finding rather than muted, since a flake there would mean
if_matchgenuinely is not mutually exclusive.AI Usage Declaration
Written by Claude Code (Claude Opus 5) under my direction. The underlying tests were authored by a subagent during the #119 investigation; I reviewed the diagnosis against
claim.rsanddispatch.rsdirectly before accepting it, then rebased, renamed, and re-verified them here.routed-only submit guard by reading the source, not by trusting the investigation's summary.#[ignore]reason string would not have excluded these from CI.Reviewer Focus
Whether
concurrent_if_match_updates_never_both_winbelongs in the per-PR live job at all, versus a slower periodic run. It is cheap today, but it is a concurrency stress test and those tend to grow teeth on slower hardware.