Repository navigation
🧹 fix: Make Skill Deletion Durable and Retryable - #17007
dustinhealy wants to merge 20 commits into
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b56a0c6834
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa5e25c4b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80878707b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c20d74d996
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dce6e55773
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa48cbfe42
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f5e257529
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3507cfc886
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f537d53669
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a3e095009
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5648259269
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d8fd51b38
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
🟡 Changes recommended
A deleting Skill remains visible through one state lookup, and sweep concurrency can exceed its configured bound quadratically.
2 open findings
What changed in this PR
Adds durable, retryable Skill deletion using tombstones, upload intents, provider cleanup, and gated background finalization.
Changes:
- Persists deletion journals and upload intents before storage writes.
- Adds claimed background cleanup, recovery tooling, and rollout configuration.
- Integrates the lifecycle across uploads, imports, GitHub sync, management, and account deletion.
| File | Description |
|---|---|
packages/data-schemas/src/types/skillDeletion.ts |
Defines deletion and upload-intent records. |
packages/data-schemas/src/types/skill.ts |
Adds deletion lifecycle fields. |
packages/data-schemas/src/types/index.ts |
Exports lifecycle types. |
packages/data-schemas/src/types/app.ts |
Types deletion configuration. |
packages/data-schemas/src/schema/skillUploadIntent.ts |
Adds intent persistence schema. |
packages/data-schemas/src/schema/skillDeletion.ts |
Adds tombstone schema. |
packages/data-schemas/src/schema/skill.ts |
Persists deletion markers and index. |
packages/data-schemas/src/models/skillUploadIntent.ts |
Creates intent model. |
packages/data-schemas/src/models/skillDeletion.ts |
Creates tombstone model. |
packages/data-schemas/src/models/plugins/tenantIsolation.coverage.spec.ts |
Registers manual tenant scoping. |
packages/data-schemas/src/models/index.ts |
Registers new models. |
packages/data-schemas/src/methods/skillLifecycle.ts |
Implements lifecycle persistence and claims. |
packages/data-schemas/src/methods/skillDeletionAdapter.ts |
Projects persistence records for API use. |
packages/data-schemas/src/methods/skillDeletionAdapter.spec.ts |
Tests record projection. |
packages/data-schemas/src/methods/skill.ts |
Fences reads/writes and finalizes deletion. |
packages/data-schemas/src/methods/skill.spec.ts |
Tests fenced and deferred deletion. |
packages/data-schemas/src/methods/index.ts |
Exposes lifecycle methods. |
packages/data-schemas/src/index.ts |
Exports deletion adapter. |
packages/data-schemas/src/app/service.ts |
Exposes deletion configuration. |
packages/data-schemas/src/app/service.spec.ts |
Tests configuration projection. |
packages/data-provider/src/types/skills.ts |
Updates deletion response documentation. |
packages/data-provider/src/types/files.ts |
Types cleanup concurrency. |
packages/data-provider/src/file-config.ts |
Configures deletion concurrency. |
packages/data-provider/src/file-config.spec.ts |
Tests concurrency configuration. |
packages/data-provider/src/config.ts |
Adds rollout and sweep settings. |
packages/data-provider/src/config.spec.ts |
Tests deletion defaults and validation. |
packages/api/src/skills/upload.ts |
Wraps browser uploads in intents. |
packages/api/src/skills/upload.spec.ts |
Tests upload deletion races. |
packages/api/src/skills/sync/github.ts |
Makes sync uploads and rollback durable. |
packages/api/src/skills/sync/github.spec.ts |
Tests durable sync cleanup. |
packages/api/src/skills/save.ts |
Adds intents to authored file saves. |
packages/api/src/skills/save.spec.ts |
Tests durable save behavior. |
packages/api/src/skills/reservation.ts |
Reserves provider object locations. |
packages/api/src/skills/reservation.spec.ts |
Tests reservation strategies. |
packages/api/src/skills/recoveryCli.ts |
Implements operator recovery commands. |
packages/api/src/skills/recoveryCli.spec.ts |
Tests recovery safeguards. |
packages/api/src/skills/management.ts |
Maps deletion races to conflicts. |
packages/api/src/skills/management.spec.ts |
Tests management conflicts. |
packages/api/src/skills/intent.ts |
Orchestrates upload intent transitions. |
packages/api/src/skills/intent.spec.ts |
Tests intent ordering and recovery. |
packages/api/src/skills/index.ts |
Exports lifecycle modules. |
packages/api/src/skills/import.ts |
Makes imported blobs retryable. |
packages/api/src/skills/handlers.ts |
Delegates deletion to the lifecycle module. |
packages/api/src/skills/handlers.spec.ts |
Tests lifecycle-backed handlers. |
packages/api/src/skills/deletionService.ts |
Adds claims, sweeps, and provider cleanup. |
packages/api/src/skills/deletionService.spec.ts |
Tests claims and sweep concurrency. |
packages/api/src/skills/deletion.ts |
Implements deletion reconciliation. |
packages/api/src/skills/deletion.spec.ts |
Tests retry and late-upload behavior. |
packages/api/src/skills/cleanup.ts |
Handles upload-intent cleanup results. |
packages/api/src/skills/cleanup.spec.ts |
Tests result merging. |
packages/api/src/skills/__tests__/import.test.ts |
Tests durable import rollback. |
packages/api/src/agents/handlers.ts |
Reports deletion races safely. |
packages/api/src/agents/handlers.spec.ts |
Tests agent-facing race errors. |
package.json |
Adds root recovery command. |
api/server/services/Skills/sync.test.js |
Updates sync wiring mocks. |
api/server/services/Skills/sync.js |
Wires durable sync dependencies. |
api/server/services/Skills/handlers.js |
Wires lifecycle deletion handlers. |
api/server/services/Skills/deletion.js |
Adapts the deletion module to legacy wiring. |
api/server/services/Endpoints/agents/skillDeps.spec.js |
Updates Skill dependency mocks. |
api/server/services/Endpoints/agents/skillDeps.js |
Wires intent-aware agent saves. |
api/server/routes/skills.test.js |
Updates route integration coverage. |
api/server/routes/skills.tenant.test.js |
Adds deletion-capable storage mocks. |
api/server/routes/skills.js |
Wires reservation and lifecycle dependencies. |
api/server/routes/agents/__tests__/openapi.contract.spec.js |
Supplies configuration for contract tests. |
api/server/index.js |
Starts the deletion sweep. |
api/server/experimental.js |
Starts the sweep in experimental mode. |
api/recover-skill-upload.js |
Adds the recovery executable. |
api/package.json |
Registers the API recovery script. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
codegraph-librechat
Bot
added
the
🗺️ Skills UI
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 603628295f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Superseded by a reviewable four-PR stack: #17040 (persistence contracts) → #17041 (upload reservation helpers) → #17042 (bounded deletion engine) → #17043 (runtime integration). The combined tree at #17043 includes the fixes for the two latest Codex findings in 3d8e6fd: safe recovery-command error reporting and reuse of loaded cleanup config. All four replacement PRs are draft; closing this PR does not imply the stack is approved or ready to merge. |
Summary
Whole-Skill deletion must not discard its last database reference before storage cleanup completes. This PR adds a persistent, retryable lifecycle: mark the Skill as deleting, retain file references, delete provider objects, and remove the Skill and ACLs only after cleanup succeeds.
This is stacked on #17005, which introduces the management DELETE endpoint. The
skillDeletion.finalizationEnabledrollout gate defaults tofalse. After both stacked PRs are deployed and all older Skill-writing processes and in-flight uploads have drained, set it totrueto permit finalization and background cleanup.How it works
Skill uploads record their future blob location before writing bytes, then advance the intent through uploading, committing, and committed states. Deletion atomically moves the public name into an internal tombstone namespace and marks the Skill as deleting, so the name can be reused immediately under the existing unique index. It records a tombstone, backfills pre-existing file rows, fences ordinary Skill writes, and retries incomplete cleanup after restarts. Independent tombstones are processed with bounded concurrency, and a shared provider limiter keeps aggregate sweep deletions within the configured bound. Skill-state pruning uses the same active-Skill filter as other reads. Completed tombstones remain available for late uploads. A stopped writer is recovered only by an operator after draining writers; the recovery command checks the file row before choosing committed versus abandoned. GitHub-sync rollback queues its newly uploaded blobs before restoring old rows, so failed provider cleanup remains retryable.
Browser and management deletion keep their existing HTTP response shape, including
cleanupComplete. A clean response describes that pass, not a strict guarantee that a previously in-flight writer cannot finish later. Tombstones and intents are retained for reconciliation.Type of change
Testing
Tested environments/configuration: Local MongoDB memory server, Skill routes, import, GitHub sync, and mocked local/cloud storage strategies.
Automated tests: Earlier broad focused runs: packages/api 515 passed, data-schemas 144 passed, real server HTTP/route/sync 85 passed. On this head: deletion and management API 57 passed, Skill methods 127 passed, browser and OpenAPI route suites 66 passed, lifecycle DB 21 passed, configuration 488 passed; packages/api and data-schemas typechecks pass. Scoped formatting, import sorting, and static checks pass. Local Lighthouse could not start because port 3080 was already occupied; CI runs its own Lighthouse lane. CI remains subject to an unrelated DocumentDB compatibility test failure already present on the base branch.
Screenshots / recordings
No user-facing visual change.
Risk / compatibility
Adds persistent Skill deletion and upload-intent collections. A mixed-version rollout is unsafe for finalization because old upload writers do not create intents. The default-off gate leaves deletion marked and retryable but does not finalize it. Deploy both stacked PRs, drain old processes and in-flight writes, then enable
skillDeletion.finalizationEnabled: truein a subsequent config rollout. Until enabled, DELETE responses can havecleanupComplete: falseand the background worker does not finalize tombstones. A failed archive import during this rollout interval can report incomplete rollback with a retryable Skill ID; enabling the gate lets cleanup resume. In-flight or ambiguous upload commits are not abandoned solely because a lease expires. Standalone MongoDB remains supported; no multi-document transaction is required. This PR does not redesign the existing per-file replacement cleanup path: making replacement of an active Skill's file crash-durable is a separate cross-document lifecycle problem and remains out of scope.Checklist