fix(publish): return deterministic conflict on concurrent coordinate race - #784
Merged
Conversation
…race Concurrent publishes for the same (namespace_id, slug, owner_id) or (skill_id, version) coordinate both pass the check-then-create reads and race on the database unique constraints. The losing request surfaced an unhandled DataIntegrityViolationException as HTTP 500. Translate the constraint violation at both insert points into a deterministic DomainBadRequestException (error.skill.publish.concurrentConflict), matching the existing idiom in LabelDefinitionService/ReviewService/ PromotionService. No same-transaction re-read is attempted, so the losing publish rolls back cleanly and returns a retryable conflict instead of a 500. Add the i18n key (en/zh) and two unit tests covering the skill-insert and version-insert races. Closes #617 Signed-off-by: FenjuFu <92919259+FenjuFu@users.noreply.github.com>
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
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.
What
Closes #617.
Concurrent publishes for the same final
(namespace_id, slug, owner_id)/(skill_id, version)coordinate could both pass the check-then-create reads and then race on the database unique constraints. The losing request surfaced an unhandledDataIntegrityViolationExceptionas HTTP 500.Fix
Follow the codebase's existing concurrency idiom —
LabelDefinitionService,ReviewService, andPromotionServiceallcatch (DataIntegrityViolationException)and translate the unique-constraint violation into a deterministic business error.SkillPublishService.publishFromEntriesnow does the same at its two check-then-create insert points:skill(namespace_id, slug, owner_id)unique constraintskill_version(skill_id, version)unique constraintBoth translate a violation into a deterministic
DomainBadRequestException("error.skill.publish.concurrentConflict")(HTTP 4xx) instead of a 500. No same-transaction re-read is attempted (that would hit the aborted-transaction trap); the losing publish rolls back cleanly and returns a retryable conflict, matching the issue's "one success and one deterministic business conflict" contract. No response-shape change.i18n
New key
error.skill.publish.concurrentConflictadded tomessages.properties(en) andmessages_zh.properties(zh); other locales fall back to the default bundle.Tests
Two unit tests in
SkillPublishServiceTestassert that aDataIntegrityViolationExceptionfrom the skill insert and from the version insert each surface aserror.skill.publish.concurrentConflictrather than propagating as a 500, and that no version save / object upload happens once the coordinate race is lost.Scope
This eliminates the reported 500 — the core of "they should not produce an internal server error". The issue also raises a broader follow-up: deterministic serialization of interleaved publish/review lifecycle mutations (pending-review withdrawal vs
latest_version_id). That is a larger lifecycle change; happy to address it in a separate PR if maintainers want it in scope here.