Repository navigation
Clean up Artifacts repos on package delete and account deletion - #511
Merged
Merged
Conversation
Add DELETE support to the Artifacts REST binding and best-effort cleanup orchestration for entity source repos and session fork repos. Wire cleanup into deleteSavedPackageProjection and deleteUserAccount while preserving existing D1 and KV cascade behavior. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughThis PR introduces artifact repository cleanup infrastructure. It adds a delete method to the artifacts REST binding, creates a dedicated cleanup module with three entry points (single-repo, package-scoped, and user-scoped), and integrates cleanup calls into account deletion and package deletion workflows. All cleanup operations are best-effort with appropriate access control and warning logging. ChangesArtifact Repository Cleanup
Sequence DiagramsequenceDiagram
participant User
participant CleanupAPI
participant ArtifactBinding
participant Database
User->>CleanupAPI: cleanupAllUserArtifactRepos(userId)
CleanupAPI->>Database: list all sources and sessions
CleanupAPI->>CleanupAPI: deduplicate repo names
CleanupAPI->>ArtifactBinding: delete each repo
ArtifactBinding-->>CleanupAPI: deletion status
CleanupAPI-->>User: total deleted count
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
🔎 Preview deployed: https://kody-pr-511.kentcdodds.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/worker/src/package-registry/service.node.test.ts (1)
362-366: ⚡ Quick winAdd a rejection-path test for artifact cleanup.
Please add one test where
cleanupArtifactReposForPackagerejects and verifydeleteSavedPackage,deleteSavedPackageVector, andsyncJobManagerAlarmstill run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/package-registry/service.node.test.ts` around lines 362 - 366, Add a new unit test that stubs mockModule.cleanupArtifactReposForPackage to return a rejected promise and then invokes the same code path used in the existing test; verify that despite the rejection cleanupArtifactReposForPackage was called with { env, userId: 'user-1', sourceId: 'source-1' } and that deleteSavedPackage, deleteSavedPackageVector, and syncJobManagerAlarm were still invoked (use toHaveBeenCalled / toHaveBeenCalledWith as in the existing test), and ensure the test awaits the promise and handles the rejection so it does not fail the test (e.g., return or await the call and catch the error).packages/worker/src/package-registry/service.ts (1)
274-288: ⚡ Quick winSurface non-throw cleanup warnings for manual follow-up.
cleanupArtifactReposForPackagecan report skipped/manual-cleanup scenarios throughwarningswithout throwing. Right now, only thrown errors get logged, so those cases are silent.💡 Proposed change
if (savedPackage) { + const cleanupWarnings: Array<string> = [] await cleanupArtifactReposForPackage({ env: input.env, userId: input.userId, sourceId: savedPackage.sourceId, + warnings: cleanupWarnings, }).catch((error) => { console.warn( JSON.stringify({ message: 'package artifact repo cleanup failed', userId: input.userId, packageId: input.packageId, sourceId: savedPackage.sourceId, error: error instanceof Error ? error.message : String(error), }), ) }) + for (const warning of cleanupWarnings) { + console.warn( + JSON.stringify({ + message: 'package artifact repo cleanup warning', + userId: input.userId, + packageId: input.packageId, + sourceId: savedPackage.sourceId, + warning, + }), + ) + } const listedServices = await listSavedPackageServices({🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/package-registry/service.ts` around lines 274 - 288, The cleanup call currently only logs thrown errors; update the call to capture and inspect the result of cleanupArtifactReposForPackage (e.g., const result = await cleanupArtifactReposForPackage({...})) and if result contains a non-empty warnings array (result.warnings) log those warnings for manual follow-up including context (userId: input.userId, packageId: input.packageId, sourceId: savedPackage.sourceId); keep the existing .catch to log thrown errors as before but add the warnings log path so skipped/manual-cleanup scenarios are not silent.packages/worker/src/repo/artifact-repo-cleanup.ts (1)
39-39: ⚡ Quick winVariable shadowing: use a different name.
The local
const repoNameshadowsinput.repoName. While this works, it's a code smell that can lead to confusion. Consider usingtrimmedRepoNameor similar.♻️ Proposed fix
- const repoName = input.repoName.trim() - if (!repoName) return false + const trimmedRepoName = input.repoName.trim() + if (!trimmedRepoName) return false if (!hasArtifactsAccess(input.env)) { return false } try { const binding = getArtifactsBinding(input.env) - const result = await binding.delete(repoName) + const result = await binding.delete(trimmedRepoName) logArtifactRepoDeleted({ userId: input.userId, - repoName, + repoName: trimmedRepoName, result, }) return true } catch (error) { const message = error instanceof Error ? error.message : String(error) input.warnings?.push( - `Artifact repo delete failed for ${repoName}: ${message}`, + `Artifact repo delete failed for ${trimmedRepoName}: ${message}`, ) console.warn( JSON.stringify({ message: 'artifact repo delete failed', userId: input.userId, - repoName, + repoName: trimmedRepoName, error: message, }), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/repo/artifact-repo-cleanup.ts` at line 39, The local constant repoName shadows input.repoName; rename the trimmed local variable to something non-conflicting (e.g., trimmedRepoName) in artifact-repo-cleanup.ts so you use trimmedRepoName = input.repoName.trim() wherever repoName is currently used in this function (adjust any occurrences in functions/methods like the cleanup logic that reference repoName).packages/worker/src/repo/artifact-repo-cleanup.node.test.ts (1)
74-106: 💤 Low valueTest name implies ordering guarantee that isn't enforced.
The test name suggests "deletes session fork repos before entity repo," but the implementation only happens to preserve insertion order via Set iteration. While this works in practice (JavaScript Sets maintain insertion order), the test only verifies both deletions occurred, not their sequence.
💡 Consider clarifying the test name
-test('cleanupArtifactReposForPackage deletes session fork repos before entity repo', async () => { +test('cleanupArtifactReposForPackage deletes both session fork repos and entity repo', async () => {Or, if ordering is important, add explicit order verification:
expect(mockModule.deleteArtifactRepo.mock.calls[0][0]).toBe('package-pkg-1-abc123') expect(mockModule.deleteArtifactRepo.mock.calls[1][0]).toBe('package-pkg-1')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/repo/artifact-repo-cleanup.node.test.ts` around lines 74 - 106, The test name claims an ordering guarantee for cleanupArtifactReposForPackage but doesn't assert call order; update the test to explicitly verify deletion order by asserting mockModule.deleteArtifactRepo mock.calls sequence (e.g., check mockModule.deleteArtifactRepo.mock.calls[0][0] is the session repo name and mockModule.deleteArtifactRepo.mock.calls[1][0] is the entity repo), or alternatively rename the test to remove the "before" wording if order is not required; reference cleanupArtifactReposForPackage and mockModule.deleteArtifactRepo when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/worker/src/app/account-deletion.ts`:
- Around line 413-417: The call to cleanupAllUserArtifactRepos can throw and
currently aborts the entire account deletion; wrap the call in a try/catch so
deletion remains best-effort: call cleanupAllUserArtifactRepos inside try,
assign its return to result.deletedArtifactRepos on success, and in catch push a
warning with the error message into the existing warnings array (and set
result.deletedArtifactRepos to [] or a sensible default) so the flow continues;
refer to the same pattern used elsewhere in this file for guidance (use the
warnings array and not throwing the error).
In `@packages/worker/src/mcp/capabilities/packages/delete-package.ts`:
- Line 13: Update the capability description in delete-package.ts (the string
describing the "Delete a saved package projection..." capability) to state that
repository cleanup is best-effort rather than guaranteed; reword the sentence to
something like "This removes the saved package from discovery and attempts to
clean up associated Artifacts repos on a best-effort basis, which may not
succeed in all cases." Ensure the updated description replaces the existing line
so it no longer overpromises guaranteed cleanup.
In `@packages/worker/src/repo/artifacts.ts`:
- Line 84: The delete method signature references a missing exported type
ArtifactDeleteRepoResult which causes a compile error; add and export a correct
ArtifactDeleteRepoResult type (e.g., a union/object describing deleted vs
not-found outcome) immediately after the existing ArtifactGetRepoResult type in
the same file so the import in artifact-repo-cleanup.ts resolves; ensure the
exported type name exactly matches ArtifactDeleteRepoResult and its shape
matches usages of the delete(name: string): Promise<ArtifactDeleteRepoResult>
method.
---
Nitpick comments:
In `@packages/worker/src/package-registry/service.node.test.ts`:
- Around line 362-366: Add a new unit test that stubs
mockModule.cleanupArtifactReposForPackage to return a rejected promise and then
invokes the same code path used in the existing test; verify that despite the
rejection cleanupArtifactReposForPackage was called with { env, userId:
'user-1', sourceId: 'source-1' } and that deleteSavedPackage,
deleteSavedPackageVector, and syncJobManagerAlarm were still invoked (use
toHaveBeenCalled / toHaveBeenCalledWith as in the existing test), and ensure the
test awaits the promise and handles the rejection so it does not fail the test
(e.g., return or await the call and catch the error).
In `@packages/worker/src/package-registry/service.ts`:
- Around line 274-288: The cleanup call currently only logs thrown errors;
update the call to capture and inspect the result of
cleanupArtifactReposForPackage (e.g., const result = await
cleanupArtifactReposForPackage({...})) and if result contains a non-empty
warnings array (result.warnings) log those warnings for manual follow-up
including context (userId: input.userId, packageId: input.packageId, sourceId:
savedPackage.sourceId); keep the existing .catch to log thrown errors as before
but add the warnings log path so skipped/manual-cleanup scenarios are not
silent.
In `@packages/worker/src/repo/artifact-repo-cleanup.node.test.ts`:
- Around line 74-106: The test name claims an ordering guarantee for
cleanupArtifactReposForPackage but doesn't assert call order; update the test to
explicitly verify deletion order by asserting mockModule.deleteArtifactRepo
mock.calls sequence (e.g., check mockModule.deleteArtifactRepo.mock.calls[0][0]
is the session repo name and mockModule.deleteArtifactRepo.mock.calls[1][0] is
the entity repo), or alternatively rename the test to remove the "before"
wording if order is not required; reference cleanupArtifactReposForPackage and
mockModule.deleteArtifactRepo when making the change.
In `@packages/worker/src/repo/artifact-repo-cleanup.ts`:
- Line 39: The local constant repoName shadows input.repoName; rename the
trimmed local variable to something non-conflicting (e.g., trimmedRepoName) in
artifact-repo-cleanup.ts so you use trimmedRepoName = input.repoName.trim()
wherever repoName is currently used in this function (adjust any occurrences in
functions/methods like the cleanup logic that reference repoName).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5cb97534-a435-4b35-800d-4bc211df027f
📒 Files selected for processing (9)
packages/worker/src/app/account-deletion.tspackages/worker/src/mcp/capabilities/packages/delete-package.tspackages/worker/src/package-registry/service.node.test.tspackages/worker/src/package-registry/service.tspackages/worker/src/repo/artifact-repo-cleanup.node.test.tspackages/worker/src/repo/artifact-repo-cleanup.tspackages/worker/src/repo/artifacts.node.test.tspackages/worker/src/repo/artifacts.tspackages/worker/src/repo/repo-sessions.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a3a9ab3. Configure here.
Export the delete result type that CI typecheck requires. Wrap account deletion artifact cleanup in a catch so D1 enumeration failures cannot abort the cascade, and clarify package_delete is best-effort. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
This was referenced Jun 24, 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
2 participants
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.
Add DELETE support to the Artifacts REST binding and best-effort cleanup orchestration for entity source repos and session fork repos. Wire cleanup into deleteSavedPackageProjection and deleteUserAccount while preserving existing D1 and KV cascade behavior.
Note
Medium Risk
Destructive, best-effort remote deletes tied to user data removal; failures are warned but D1 cleanup still proceeds, so orphaned Artifacts repos are possible if the API is down or mis-scoped.
Overview
Adds DELETE to the Cloudflare Artifacts REST binding (404 treated as already gone) and a new best-effort cleanup layer that removes entity-source repos and session fork repos for the signed-in user, with user-scope checks and warnings when Artifacts is unavailable.
Package delete now runs
cleanupArtifactReposForPackagebefore stopping services and removing D1/KV/vector projection data; thepackage_deletecapability text reflects that behavior.Account deletion runs
cleanupAllUserArtifactReposafter vector cleanup and before storage/KV/D1 cascades, and reportsdeletedArtifactReposon the deletion result.Supporting changes:
listRepoSessionsByUserfor enumerating all session repos on full account cleanup, plus unit tests for the binding delete path and cleanup orchestration.Reviewed by Cursor Bugbot for commit 7da4d5c. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit