fix: make addBadgeScan idempotent against a client retry of the same scan - #602
Open
romanetar wants to merge 2 commits into
Open
fix: make addBadgeScan idempotent against a client retry of the same scan#602romanetar wants to merge 2 commits into
romanetar wants to merge 2 commits into
Conversation
…scan SUP-86b9fp53j: scanbadgeapp's SyncService retries an upload whenever its own 4s client-side timeout elapses, with no guarantee the original request didn't already reach the server and commit. Two such requests racing addBadgeScan could both find no existing SponsorBadgeScan for the same (sponsor, badge, scan_date) and both INSERT, producing two server-side rows for one physical scan - the duplicate reported in the ticket. This closes the server side of that bug; the client-side races (periodic sync vs. the upload button, vs. individual retry) were already fixed in scanbadgeapp. A retry always carries the exact same scan_date as the original attempt (the client never changes a scan's captured timestamp between attempts), so exact (sponsor, badge, scan_date) equality is enough to recognize a retry without a fuzzy time-window heuristic - a genuinely later re-scan of the same badge gets a new scan_date and is never collapsed. addBadgeScan now resolves the ticket/badge/sponsor first (read-only, so it runs outside any transaction), then creates the scan under ILockManagerService (Redis-backed; already used elsewhere in this codebase, e.g. SummitOrderService) keyed by that same tuple. The lock wraps the whole transaction, held past its COMMIT rather than released as soon as the row is attached in-memory - releasing any earlier would let a concurrent request's existence check run, and find nothing, before the first request's INSERT is actually durable. A plain check-then-insert alone doesn't close this under READ_COMMITTED, the isolation level ITransactionService::transaction defaults to. ISponsorUserInfoGrantRepository::findExistingBadgeScan queries SponsorBadgeScan directly for the exact match. UnacquiredLockException is left to propagate rather than wrapped in a ValidationException: the scanning app treats a non-4xx failure as transient and retries on its own, which is the right outcome for lock contention - a ValidationException would mark it a permanent client error instead. Tests added to OAuth2SummitBadgeScanApiControllerTest: two identical POSTs produce one row and the same response id; a different scan_date is not deduplicated; and - since PHPUnit calls are sequential and a plain existence check alone would pass the first two - a dedicated test that holds the exact lock name externally and confirms addBadgeScan fails to acquire it, proving the lock itself is what's being exercised, not just the end result of the happy path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-602/ This page is automatically updated on each push to this PR. |
…e lock TTL The Redis dedup lock added in fccbfbb cannot guarantee one row per scan: it has a TTL, no renewal and no fencing token, so it can lapse while the transaction it wraps is still running -- DoctrineTransactionService retries a root transaction up to MaxRetries = 10 on reconnectable errors with no backoff bounding the wall clock, and LockManagerService::releaseLock only logs 'lock was not held by this token at release time' when that happens. A concurrent retry can then acquire the same lock name, run findExistingBadgeScan, see nothing committed yet and INSERT a duplicate -- exactly the race the fix exists to close. SponsorUserSyncService hit this same wall with the structurally identical pattern and raised its lifetime from 30 to 120 in 290357f; raising a TTL only moves the boundary. Moves the invariant to where it can actually hold: a UNIQUE index over the new SponsorBadgeScan.ScanDedupKey column ("<sponsor>:<badge>:<epoch>"), with UniqueConstraintViolationException resolved to the row that won the race. Caught outside the transaction on purpose -- a failed flush leaves the EntityManager closed and the connection rollback-only, so the winning row can only be re-read in a fresh transaction (same placement as SummitService::addEventToMemberSchedule). A violation that does not resolve to our tuple is rethrown rather than swallowed. The lock stays as an optimization that keeps the common case from doing wasted work, and its 30s TTL stays short on purpose: acquireLock only waits ~0.7s before giving up, so a long-lived orphan would turn every retry of that one scan into a failure. The column has to be denormalized onto SponsorBadgeScan rather than indexed over the tuple itself: SponsorUserInfoGrant/SponsorBadgeScan is a JOINED pair with SponsorID on the parent table and BadgeID/ScanDate on the child, and a UNIQUE index cannot span both. The migration deliberately does not backfill and does not delete anything. The column is nullable and existing rows keep NULL; MySQL permits unlimited NULLs in a UNIQUE index, so the duplicates this bug already produced neither block the CREATE nor have to be reconciled here (deleting scan rows would cascade into SponsorBadgeScanExtraQuestionAnswer -- real lead-gen data). Those rows stay covered by the explicit findExistingBadgeScan check; only rows created from now on carry a key, and those are exactly the ones a retry can race against. Tests: the UNIQUE index actually rejects a second row carrying the same key, and addBadgeScan stamps the key -- without the latter every new row would go in NULL and the protection would silently be gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-602/ This page is automatically updated on each push to this PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref https://app.clickup.com/t/9014802374/86b9fp53j
SUP-86b9fp53j: scanbadgeapp's SyncService retries an upload whenever its own 4s client-side timeout elapses, with no guarantee the original request didn't already reach the server and commit. Two such requests racing addBadgeScan could both find no existing SponsorBadgeScan for the same (sponsor, badge, scan_date) and both INSERT, producing two server-side rows for one physical scan - the duplicate reported in the ticket. This closes the server side of that bug; the client-side races (periodic sync vs. the upload button, vs. individual retry) were already fixed in scanbadgeapp.
A retry always carries the exact same scan_date as the original attempt (the client never changes a scan's captured timestamp between attempts), so exact (sponsor, badge, scan_date) equality is enough to recognize a retry without a fuzzy time-window heuristic - a genuinely later re-scan of the same badge gets a new scan_date and is never collapsed.
addBadgeScan now resolves the ticket/badge/sponsor first (read-only, so it runs outside any transaction), then creates the scan under ILockManagerService (Redis-backed; already used elsewhere in this codebase, e.g. SummitOrderService) keyed by that same tuple. The lock wraps the whole transaction, held past its COMMIT rather than released as soon as the row is attached in-memory - releasing any earlier would let a concurrent request's existence check run, and find nothing, before the first request's INSERT is actually durable. A plain check-then-insert alone doesn't close this under READ_COMMITTED, the isolation level ITransactionService::transaction defaults to.
ISponsorUserInfoGrantRepository::findExistingBadgeScan queries SponsorBadgeScan directly for the exact match. UnacquiredLockException is left to propagate rather than wrapped in a ValidationException: the scanning app treats a non-4xx failure as transient and retries on its own, which is the right outcome for lock contention - a ValidationException would mark it a permanent client error instead.
Tests added to OAuth2SummitBadgeScanApiControllerTest: two identical POSTs produce one row and the same response id; a different scan_date is not deduplicated; and - since PHPUnit calls are sequential and a plain existence check alone would pass the first two - a dedicated test that holds the exact lock name externally and confirms addBadgeScan fails to acquire it, proving the lock itself is what's being exercised, not just the end result of the happy path.