fix(hub): make skill version publish idempotent for draft versions#451
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the publishSkillVersion handler to support idempotent retries by reusing existing draft versions of a skill, while rejecting conflicts for other statuses. The review feedback correctly identifies that database errors from GetSkillVersionByNumber are currently ignored instead of being handled, which could mask critical issues. A fix is suggested to explicitly check for and handle non-not-found errors.
| existing, err := s.store.GetSkillVersionByNumber(ctx, skillID, req.Version) | ||
| if err == nil && existing.Status == store.SkillVersionStatusPublished { | ||
| writeError(w, http.StatusConflict, "conflict", | ||
| fmt.Sprintf("version %s is already published and immutable; publish a new version instead", req.Version), nil) | ||
| return | ||
| if err == nil { | ||
| switch existing.Status { | ||
| case store.SkillVersionStatusDraft: | ||
| sv = existing | ||
| case store.SkillVersionStatusPublished: | ||
| writeError(w, http.StatusConflict, "conflict", | ||
| fmt.Sprintf("version %s is already published and immutable; publish a new version instead", req.Version), nil) | ||
| return | ||
| default: | ||
| writeError(w, http.StatusConflict, "conflict", | ||
| fmt.Sprintf("version %s already exists with status %q", req.Version, existing.Status), nil) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
The code currently ignores any error returned by GetSkillVersionByNumber and proceeds to create a new draft version. If the error is a legitimate database error (such as a connection failure, timeout, or query error) rather than a 'not found' error, ignoring it and attempting to write to the database is unsafe and masks the actual root cause.
We should explicitly check if the error is store.ErrNotFound and return any other error. Note that you will need to import the standard `
existing, err := s.store.GetSkillVersionByNumber(ctx, skillID, req.Version)
if err != nil && !errors.Is(err, store.ErrNotFound) {
writeErrorFromErr(w, err, "")
return
}
if err == nil {
switch existing.Status {
case store.SkillVersionStatusDraft:
sv = existing
case store.SkillVersionStatusPublished:
writeError(w, http.StatusConflict, "conflict",
fmt.Sprintf("version %s is already published and immutable; publish a new version instead", req.Version), nil)
return
default:
writeError(w, http.StatusConflict, "conflict",
fmt.Sprintf("version %s already exists with status %q", req.Version, existing.Status), nil)
return
}
}When publishing a skill version, if the draft creation (step 1) succeeds but upload/finalize (steps 2-3) fail or are interrupted, retrying from step 1 would hit a UNIQUE constraint and return 409 Conflict, leaving the user stuck. Now when a version with the same number already exists as a draft, the endpoint returns the existing draft with fresh upload URLs instead of rejecting. Published/deprecated/archived versions still return 409.
ptone
force-pushed
the
scion/skill-bank-publish-fix
branch
from
55ee94c to
ca1ffcf
Compare
1 participant
Summary
Fixes the issue where interrupted publish attempts left orphan drafts that blocked retries.
Test plan
go test ./pkg/hub -run Skill -count=1passes