fix: repair orphaned pending skill versions stuck after publish - #3401
fix: repair orphaned pending skill versions stuck after publish#3401trippyogi wants to merge 17 commits into
Conversation
|
@trippyogi is attempting to deploy a commit to the OpenClaw Foundation Team on Vercel. A member of the Team first needs to authorize it. |
…air flow - releaseFinalizationClaimPatch (publishAttempts.ts) now caps consecutive finalization failures only for kind==="skill". Package attempts have no repair path yet, so they retry unbounded instead of becoming a permanent orphan. - isActiveAttemptLive (publishAttempts.ts) no longer abandons a non-terminal attempt just because finalizationFailureCount > 0. It now checks staleness against updatedAt (falling back to createdAt) using ACTIVE_ATTEMPT_RETRYABLE_STALE_MS for attempts with retry activity, or ACTIVE_ATTEMPT_UNCLAIMED_GRACE_MS for zero-failure attempts, so a below-cap dispatcher retry (or a long pending_checks phase that just transitioned cleanly) is not raced by manual repair. Added findActiveSkillPublishAttemptByIdInternal so the repair pre-check can look up a known publishAttemptId directly instead of only scanning by slug/version. - repairOrphanedPendingSkillVersionHandler (maintenance.ts) now publishes the pending version and closes the orphaned attempt atomically via a new publishPendingVersionAndCloseAttemptInternal mutation (skills.ts), closing before follow-ups are scheduled. This removes the publish-before-close race where a crash between publish and close left the version published but the attempt reclaimable. Also handles the already-published-but-still-open-attempt retry case as an attempt-cleanup path. - skillsGetRouterV1Handler exact-version path (skillsV1.ts) falls back to an owner-visible pending-publication diagnostic via getSkillBySlugInternal + describeOwnerVisibleSkillVersionState when the public getBySlug lookup misses entirely, so a brand-new hidden skill (pending its first publish) no longer 404s for its owner. Tests added/updated in publishAttempts.test.ts, maintenance.test.ts, and httpApiV1.handlers.test.ts for all four findings.
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs real behavior proof before merge. Reviewed August 6, 2026, 7:37 PM ET / 23:37 UTC. ClawSweeper reviewWhat this changesThis PR bounds repeated skill-finalization failures, adds admin-only recovery for stranded pending versions, and gives authorized owners useful pending-publication diagnostics. Regression provenancePossible regression — probable (reproduction; reviewed change). No predecessor PR is attributed. Merge readiness⛔ Blocked until stronger real behavior proof is added - 4 items remain Keep open. The repair design is coherent and no blocking code defect was found, but the claimed deployment run is not attached as an inspectable trace, so real after-fix behavior proof is still required before merge. Priority: P1 Review scores
Verification
How this fits togetherSkill publishing stages a version, runs checks, then finalizes it into the public skill index and latest pointers. This change handles versions left pending when finalization repeatedly fails and controls how an operator can safely complete that projection. flowchart LR
A[Skill publish] --> B[Pending version]
B --> C[Pre-publication checks]
C --> D[Finalization worker]
D --> E{Finalized?}
E -->|Yes| F[Public skill index]
E -->|Repeated failure| G[Admin repair]
G --> F
F --> H[Version and install reads]
Before merge
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep the staged workflow, merge the bounded operator recovery only after a redacted real deployment trace proves terminal failure, repair, public visibility, and security follow-up scheduling. Do we have a high-confidence way to reproduce the issue? Yes, source reproducible: current main separates pending-version creation from deferred finalization, and the reported live record matches that boundary; I did not independently run the failure against a deployment. Is this the best way to solve the issue? Yes, as a bounded recovery solution: it reuses captured publish inputs, requires completed checks, and closes the repair race atomically, but it still needs directly inspectable runtime proof. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 34350cd16d17. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (22 earlier review cycles; latest 8 shown)
|
Patrick-Erichsen
left a comment
Patrick-Erichsen
left a comment
There was a problem hiding this comment.
This adds owner diagnostics and an operator repair for skill versions stranded in staged publication. The repair is not safe yet because a terminally failed publish attempt is not actually closed/cleared by the helper path.
LOC: +2087/-26 (9 files)
Finding: when the linked attempt is already failed, the close helper returns without clearing the persisted pendingPublication snapshot. The version can become public while its record remains internally staged. Add the cap-exhausted regression case and real Convex repair proof.
Best-fix verdict: too broad in its current form. Reusing the finalization contract is sound, but the atomic repair must explicitly handle terminal attempts before this operator path is safe.
Alternatives considered: a second cleanup mutation would restore the crash window; the better shape is one transaction that publishes and clears/closes every supported attempt state.
Code read: convex/maintenance.ts, convex/skills.ts, convex/lib/skillPublish.ts, publish-attempt helpers, HTTP owner diagnostics, and adjacent tests.
Remaining uncertainty: no after-fix staged publish/repair has been exercised on a real Convex deployment, and the branch is currently conflicted with main.
clawsweeper
Bot
added
rating: 🧂 unranked krab
clawsweeper
Bot
added
rating: 🧂 unranked krab
…claw#3349) A staged skill publish inserts the skillVersion as pending up front and only projects it onto the skill's latest/tags/version-index once the async publishAttempts finalize step succeeds. If that worker never runs again (crashed action, exhausted retries), the version was left orphaned forever: files exist, the duplicate-version guard blocks a retry, but every metadata read 404s. - Cap consecutive transient finalization failures (finalizationFailureCount, publishAttempts.ts) so a stuck attempt surfaces as failed instead of looping forever. - Add an admin-gated repair path (maintenance.ts) that re-runs finalization for one orphaned pending version, or sweeps the whole table in age-bounded batches, from the exact insert args captured on the version at publish time. Refuses to act while a live attempt still owns the version, and force-closes the original attempt row afterward so the normal dispatcher can't re-claim it and re-run followups. - Surface pending/blocked publication state to the authenticated owner on version reads instead of a bare 404, without leaking it to other callers (skillsV1.ts).
- Authorize owner-visible pending/blocked reads via publisher scope - Return 409 when linked finalize attempt already failed - Do not treat checkFailureCount as abandoned (scanner retry gaps)
…air flow - releaseFinalizationClaimPatch (publishAttempts.ts) now caps consecutive finalization failures only for kind==="skill". Package attempts have no repair path yet, so they retry unbounded instead of becoming a permanent orphan. - isActiveAttemptLive (publishAttempts.ts) no longer abandons a non-terminal attempt just because finalizationFailureCount > 0. It now checks staleness against updatedAt (falling back to createdAt) using ACTIVE_ATTEMPT_RETRYABLE_STALE_MS for attempts with retry activity, or ACTIVE_ATTEMPT_UNCLAIMED_GRACE_MS for zero-failure attempts, so a below-cap dispatcher retry (or a long pending_checks phase that just transitioned cleanly) is not raced by manual repair. Added findActiveSkillPublishAttemptByIdInternal so the repair pre-check can look up a known publishAttemptId directly instead of only scanning by slug/version. - repairOrphanedPendingSkillVersionHandler (maintenance.ts) now publishes the pending version and closes the orphaned attempt atomically via a new publishPendingVersionAndCloseAttemptInternal mutation (skills.ts), closing before follow-ups are scheduled. This removes the publish-before-close race where a crash between publish and close left the version published but the attempt reclaimable. Also handles the already-published-but-still-open-attempt retry case as an attempt-cleanup path. - skillsGetRouterV1Handler exact-version path (skillsV1.ts) falls back to an owner-visible pending-publication diagnostic via getSkillBySlugInternal + describeOwnerVisibleSkillVersionState when the public getBySlug lookup misses entirely, so a brand-new hidden skill (pending its first publish) no longer 404s for its owner. Tests added/updated in publishAttempts.test.ts, maintenance.test.ts, and httpApiV1.handlers.test.ts for all four findings.
91cd641 to
ad0b5f7
Compare
Cap-exhausted failed attempts keep a fresh updatedAt and failure count, so isActiveAttemptLive blocked orphan repair for ~20m even though the dispatcher cannot reclaim them. Gate the live check on active statuses only, matching the direct-ID precheck, and cover the failed+cap path.
|
Thanks @Patrick-Erichsen, agreeing with the external review. your atomic mutation rewrite correctly addresses the original P1. One narrow P2 remained and is fixed in b149533. P2 Fix is the status-guard mirror only; plus a regression that uses Convex cloud evidence (throwaway
|
2 participants
Closes #3349
Related: #3353 (scanner retry cap only — explicitly not this defect)
What Problem This Solves
Fixes an issue where
clawhub skill publishcould report success and reserve a version while that version never appeared inlatest, the version index, or tag pointers. Publishers then hit a dead end: metadata reads 404, files remain fetchable by explicit version, and republish is rejected as a duplicate.Why This Change Was Made
Staged skill publish inserts a pending
skillVersionfirst and only projects it onto the skill document during async finalization. If finalization never completes, the version stays orphaned forever.This change:
failedinstead of looping forevermaintenance:repairOrphanedPendingSkillVersion+ sweep) that re-runs finalization from the version's captured insert args, refuses while a live attempt still owns the version, and force-closes the original attempt afterwardNon-goals: does not change the normal happy-path publish projection; does not auto-run repair in production without an operator.
User Impact
Evidence
Focused tests (local):
bunx vitest run convex/httpApiV1.handlers.test.ts -t "3349"— 5 passedbunx vitest run convex/publishAttempts.test.ts -t "findActiveSkillPublishAttempt|3349|orphaned|finalizationFailure|scanner retries"— 11 passedBugbot (branch vs main): fixed findings before draft —
checkFailureCounttreated as abandoned → onlyfinalizationFailureCountabandons grace (scanner retries stay live)Note: #3353 merged earlier and bounds prepublication scanner retries; it does not close #3349 (called out in that PR body as a separate post-finalization consistency defect).