fix: claim quest before granting reward - #77
Draft
dev-yunseong wants to merge 1 commit into
Draft
Conversation
Rewards were granted before the completion UPDATE, and that UPDATE had no state condition and discarded its row count, so two concurrent check requests both granted the same quest reward. The card reward then hit the user_cards (card_id, user_id) unique constraint and the class-level transaction rolled back every reward in the batch. The UPDATE now requires state = 'IN_PROGRESS' and returns the affected row count so a reward is only given when this request actually claimed the quest, and the card grant is an upsert so rewarding an already-owned card increments the count instead of failing. Closes #68 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 20, 2026
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
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.
요약
보상 지급 전에 퀘스트를 선점하도록 순서를 뒤집었습니다.
setCompletedByUserIdAndQuestId에AND state = 'IN_PROGRESS'조건을 추가하고 반환 타입을Mono<Long>(영향 행 수)으로 바꿔, 실제로 1행 이상을 선점한 요청만rewardGiver.giveWithReward를 호출합니다. 또한CardRewardGiver의 INSERT를ON CONFLICT (card_id, user_id) DO UPDATE SET count = user_cards.count + :countupsert로 바꿔, 이미 보유한 카드를 보상으로 줄 때 유니크 제약 위반으로 배치 전체가 롤백되지 않게 했습니다.대상 이슈
Closes #68
검증
Ran
./gradlew buildin the worktree: compiled clean,:testreported "26 tests completed, 3 failed" — the 3 failures are DataControllerTest (x2) and CardControllerTest (x1), all Spring context load failures (ConfigurationPropertiesBindException -> NumberFormatException from missing env config, e.g. PORT/DATABASE_URL). Confirmed pre-existing bygit stash+./gradlew teston the untouched origin/main tree: "25 tests completed, 3 failed" with exactly the same 3 test names. Thengit stash pop. Ran./gradlew test --tests com.wordonline.matching.quest.service.QuestServiceTeston my change: BUILD SUCCESSFUL (all 4 quest service tests including the new one pass). The Postgres upsert SQL itself was not executed against a live DB — no DB is available in this environment.테스트
— added
checkQuestsWithRewards_SkipsRewardWhenAlreadyClaimed(@DisplayName "퀘스트_선점_실패시_보상_미지급"): the claim UPDATE returns 0 rows, the result list must be empty, andverify(rewardGiver, never()).giveWithReward(...). Matches the existing Mockito + StepVerifier style in that file. Also updated the existingcheckQuestsWithRewards_Successstubs fromMono.empty()toMono.just(1L)for the new return type.리뷰어 참고
Both defects in the issue were verified in code before editing. (1)
setCompletedByUserIdAndQuestIdreally had no state predicate and returnedMono<Void>, and rewards really ran first. (2)database/migration/V000_20260406__init_tables.sql:363confirmsuser_cards ... unique (card_id, user_id), andCardRewardGiverreally saved a null-idUserCard(always an INSERT). The class-level@Transactionalon QuestService is unchanged, so batch-wide rollback semantics stay as-is — the fix removes the cause of the constraint violation rather than narrowing the transaction.Implementation notes: the new
UserCardRepository.addCountupsert follows the existingON CONFLICT ... DO UPDATEpattern already used inUserScenarioRepository, and theMono<Long>row-count return type matches the existing@QueryUPDATE methods inUserRepository.ON CONFLICT (card_id, user_id)is written in the constraint's declared column order. The claim-then-give ordering relies on Postgres READ COMMITTED re-evaluating the WHERE clause after a blocking row lock is released, which is what makes the second concurrent transaction see 0 affected rows.Out of scope / not changed:
DecorationRewardGiverandMagicRewardGiverstill insert directly.user_decorationshas no unique constraint in the migration so it duplicates silently rather than erroring, anduser_magicsdoes haveuq_user_magics_user_id_magic_id— the claim guard now prevents the concurrent double-grant for both, but a quest whose reward is an already-owned magic would still raise a duplicate key. Issue 68 named only the card path, so I left those alone; worth a separate issue if magic rewards can overlap theinitUserMagicDEFAULT grant.Residual risk: the upsert SQL was never run against a real Postgres instance here, so a typo-level SQL error would only surface at runtime. No migration is required — the
unique (card_id, user_id)index the upsert infers on already exists.멀티 에이전트 코드 리뷰에서 검증된 결함을 수정한 것. 격리된 워크트리에서 작업하고 모듈 빌드로 확인함.