Repository navigation
🗑️ fix: Route Delete-User CLI Through the Shared Account Erasure Cascade - #17033
Conversation
berry-13
changed the title
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: efa6b608bf
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3432c72cb6
ℹ️ 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".
561347e to
ff6cf7e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff6cf7e05c
ℹ️ 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".
| await deps.deleteConversationTags({ user: userId }); | ||
| await deps.deleteUserToolFavorites(userId); | ||
| await deps.deleteAllUserMemories(userId); | ||
| await deps.deleteUserPrompts(userId); |
There was a problem hiding this comment.
When the operator CLI hits a transient failure in Prompt.deleteMany after PromptGroup.deleteMany has succeeded (packages/data-schemas/src/methods/prompt.ts:1059-1060), the added cascade rejects here and keeps the account, but the retry discovers work only from remaining prompt groups and therefore returns early, allowing the final user deletion to commit while orphaned prompt rows survive. Fresh evidence beyond the earlier review thread is this failure after group deletion, rather than the covered prompt-group lookup failure; delete prompts before their groups or make retries discover residual prompts independently.
AGENTS.md reference: AGENTS.md:L46-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4e3b0e3: prompts, then groups, then ACL entries are deleted, so a failed Prompt.deleteMany leaves the group discoverable. prompt.spec.ts covers the retry.
ff6cf7e to
1030e10
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1030e10b68
ℹ️ 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".
| await Agent.deleteMany({ _id: { $in: agentObjectIds } }); | ||
| } catch (error) { | ||
| logger.error('[deleteUserAgents] General error:', error); | ||
| throw error; |
There was a problem hiding this comment.
When the account is the sole ACL owner of an agent whose author is another user, a failure in Agent.deleteMany occurs after the method has removed the ownership ACL. This new rethrow keeps the account for retry, but the retry cannot rediscover the agent through either getSoleOwnedResourceIds or the author query, so the cascade can subsequently delete the account while leaving the agent and its remaining ACLs orphaned. Fresh evidence beyond the earlier failure-propagation thread is this post-ACL Agent.deleteMany failure; make the resource/ACL deletion atomic or preserve a durable way to rediscover the resource until deletion succeeds.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4e3b0e3: Agent.deleteMany now runs before the ACL delete, and sole-owned ids whose agent is already gone still get their ACLs removed on retry. agent.spec.ts covers a failed delete followed by a successful retry.
| await mongoose.models.AclEntry.deleteMany({ | ||
| resourceType: ResourceType.MCPSERVER, | ||
| resourceId: { $in: objectIds }, | ||
| }); | ||
| const result = await MCPServer.deleteMany({ _id: { $in: objectIds } }); |
There was a problem hiding this comment.
When a deleting user is the sole ACL owner of an MCP server authored by another user, AclEntry.deleteMany can succeed and MCPServer.deleteMany can then fail during a transient database outage. On retry, the server is neither returned as sole-owned nor selected as a legacy authored server, so account deletion can complete while the MCP server and any non-owner ACLs remain orphaned. Delete these records atomically or retain the deleting owner's discovery record until the server deletion has committed.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4e3b0e3: MCPServer.deleteMany runs before the ACL delete, and findUserErasableMCPServers returns sole-owned ids whose server is gone so a retry cleans their ACLs. mcpServer.spec.ts covers it.
4e3b0e3 to
8c9fc0a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c9fc0aa94
ℹ️ 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".
| await deps.deleteUserToolFavorites(userId); | ||
| await deps.deleteAllUserMemories(userId); | ||
| await deps.deleteUserPrompts(userId); | ||
| await deps.deleteUserSkills(userId); |
There was a problem hiding this comment.
When the newly shared CLI cascade reaches this call and either agent allowlist cleanup or permission cleanup fails, deleteUserSkills suppresses the failure (skill.ts:1695-1721 and 1775-1786) after deleting the skill document, so this cascade continues to delete the account. Surviving agents can then retain a dangling skill ID—which the helper's own contract says silently causes the agent to lose all skills—and failed ACL cleanup is no longer naturally retryable after the account is gone. Treat either incomplete cleanup as an operational failure so the account remains available for retry.
AGENTS.md reference: AGENTS.md:L117-L122
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8be7a84: deleteUserSkills now throws when allowlist or permission cleanup fails and keeps the ACL retry anchor; skill.spec.ts covers failure then retry.
| @@ -1974,8 +1971,14 @@ export function createAgentMethods( | |||
| } | |||
|
|
|||
| await Agent.deleteMany({ _id: { $in: agentObjectIds } }); | |||
There was a problem hiding this comment.
When pruning handoff edges or embedded favorites fails, the nested catches at agent.ts:1958-1971 log and continue into this irreversible agent deletion and the subsequent ACL deletion. removeAgentIdsFromEdges can fail not only on a database outage but after its bounded sweeps are exhausted by concurrent graph edits; once the agent and its owner ACL are gone, a retry cannot rediscover it, leaving surviving agents with handoff edges targeting a nonexistent agent. Fresh evidence beyond the earlier agent-erasure thread is these still-suppressed pre-delete reference-cleanup failures; propagate them before removing the agent.
AGENTS.md reference: AGENTS.md:L117-L122
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8be7a84: handoff edge and favorites cleanup now propagate before Agent.deleteMany, so the agent stays discoverable; agent.spec.ts covers failure then retry. A sweep of the remaining cascade methods found no other swallowed failure.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8be7a847a8
ℹ️ 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".
| await removeAgentIdsFromEdges(Agent, agentIds); | ||
| await User.updateMany( | ||
| { 'favorites.agentId': { $in: agentIds } }, | ||
| { $pull: { favorites: { agentId: { $in: agentIds } } } }, |
There was a problem hiding this comment.
When the operator CLI deletes an account whose agent id is also used in another tenant—a supported case because agentSchema only makes { id, tenantId } unique—the cascade runs under runAsSystem, so these unscoped operations bypass tenant filtering. removeAgentIdsFromEdges then prunes matching handoff edges in every tenant, and User.updateMany removes other tenants' favorites for the colliding ID. Include tenantId in the agent projection and scope both cleanup operations to each deleted agent's tenant.
AGENTS.md reference: AGENTS.md:L187-L190
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6bb9806: the CLI cascade now runs in the deleted user's tenant scope (as account self-deletion does), so agent edge and favorite cleanup is tenant-filtered; delete-user.spec.js asserts the scope.
| ...methods, | ||
| openCheckpointDeletion, | ||
| deleteSharedLinks: deleteAllSharedLinksWithCleanup, | ||
| deleteStoredFiles: (files) => processDeleteRequest({ req: fileRequest, files }), |
There was a problem hiding this comment.
When two users have the same provider-issued file_id, the operator CLI executes this callback inside runAsSystem, while processDeleteRequest deletes metadata by file_id alone (api/server/services/Files/process.js:313) and then removes that ID from every matching agent. Because the File schema neither makes file_id globally unique nor includes the owner in that deletion, deleting one account can erase another tenant's file record and agent references. Run the storage callback in the deleted user's tenant context or provide an owner-scoped metadata/reference cleanup operation.
AGENTS.md reference: AGENTS.md:L187-L190
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6bb9806: processDeleteRequest runs inside the same tenant scope, so file metadata and agent reference removal by file_id stay in the deleted user's tenant.
| await deps.deleteFiles([], userId); | ||
| await deps.deleteAllUserSessions({ userId }); | ||
| deps.onSessionsDeleted?.(); | ||
| await deps.deleteRefreshTokenBridges({ userId, tenantId }); |
There was a problem hiding this comment.
When a tenant-scoped user still has a bridge written without tenantId—a valid legacy/non-tenant shape allowed by the schema—passing the current tenant here makes deleteRefreshTokenBridges filter only that exact tenant (refreshTokenBridge.ts:110-111). The encrypted refresh-token bridge without the field therefore survives account deletion. Since the Mongo user ID is globally unique, account erasure should delete every bridge for userId, or explicitly include both the current tenant and missing-tenant records.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6bb9806: eraseUserData also deletes bridges for the user id that carry no tenantId; erasure.spec.ts covers both deletes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bb9806265
ℹ️ 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".
| user.tenantId | ||
| ? tenantStorage.run({ tenantId: user.tenantId, userId: uid }, operation) | ||
| : runAsSystem(operation); |
There was a problem hiding this comment.
When deleting a legacy user whose document has no tenantId, this fallback runs the new shared cascade under runAsSystem. Fresh evidence after the earlier tenant-scope comments is that this branch scopes only users with a tenant: in system context, deleteUserAgents performs ID-only edge/favorite cleanup even though agent IDs are unique only by { id, tenantId } (packages/data-schemas/src/schema/agent.ts:196), and processDeleteRequest deletes metadata and agent references by non-unique file_id (api/server/services/Files/process.js:313). A colliding record in a real tenant can therefore be modified or deleted; use explicit no-tenant predicates for these ID-keyed operations rather than a system-wide context.
AGENTS.md reference: AGENTS.md:L187-L190
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid but pre-existing and narrow: it needs a deliberate id collision across tenants with a legacy tenantless user, and self-service deletion of such a user is equally unscoped on dev. Tracked at berry-13#357.
The operator CLI ran raw deleteMany calls that dropped File metadata while leaving storage objects, skipped skills, MCP servers and agent checkpoints, and used the wrong field for agent API keys. Account self-deletion and the CLI now share eraseUserData in packages/api, so both remove stored file objects before metadata, open checkpoint deletion around conversations, and clear skills, MCP servers and shared links with ACL cleanup.
… the Account Account erasure left tool favorites, chat projects, queued agent turns and their sequences, and OpenID refresh token bridges behind. The shared eraseUserData cascade now clears them, adding deleteUserToolFavorites and deleteUserChatProjects where no delete-by-user method existed.
…y Refresh Bridges
01bada2 to
9fad81f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fad81fc4e
ℹ️ 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".
| snapshot: await prepareMCPServerOAuthDeletion({ | ||
| getTokenUserIds: () => deps.findMCPServerOAuthTokenUserIds(serverName), | ||
| getAclEntries: () => deps.findMCPServerViewerEntries(id), | ||
| }), |
There was a problem hiding this comment.
When the CLI deletes a shared database MCP server that uses server-level credentials, viewers with no personal OAuth token are absent from findMCPServerOAuthTokenUserIds; the ACL snapshot passed here is only used to test token-holding candidates, not to enumerate direct user principals. Their cache generation and live MCP connection are therefore never fenced even though the server and ACL are removed, allowing an already-loaded tool to retain access after deletion. Fresh evidence beyond the earlier thread is that the new account-erasure path seeds cleanup candidates solely from token user IDs despite also snapshotting viewer ACLs; enumerate direct user ACL principals (and affected group members) for fencing.
AGENTS.md reference: AGENTS.md:L187-L190
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid but pre-existing: the server-delete route on dev takes the same token-holder candidates, and erasure shares that protocol. Tracked at berry-13#364.
| getServerConfig: async () => undefined, | ||
| isRegisteredOAuthServer: async () => false, |
There was a problem hiding this comment.
When the operator CLI removes an OAuth-enabled database MCP server, these hard-coded dependency results make cleanupMCPServerOAuth treat every server as non-OAuth: with no serverConfigOverride, it follows the local-state-only branch in cleanup.ts:460-468. The encrypted local records are then deleted, but the access and refresh grants remain valid at the provider and cannot be retried after the server/config is gone. Fresh evidence beyond the earlier cleanup thread is this CLI-specific stub; snapshot the stored MCP config before deletion and pass it into cleanup, as the HTTP deletion path does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b03de68: the CLI reads the stored server config before deletion and passes the user's MCP allowlists, so OAuth grants are revoked at the provider.
| const runInUserScope = (operation) => | ||
| user.tenantId | ||
| ? tenantStorage.run({ tenantId: user.tenantId, userId: uid }, operation) | ||
| : runAsSystem(operation); |
There was a problem hiding this comment.
When the CLI deletes a tenant-scoped user who still has records written before tenant stamping, this wrapper injects the current tenantId into every tenant-isolated model query, so the cascade's user-ID deletes cannot see tenantless Keys, PluginAuths, Tokens, Files, sessions, memories, and similar rows. This regresses the previous CLI's system-scoped user-ID deletes and leaves credentials or stored-file metadata behind after the account is irreversibly removed. Fresh evidence beyond the earlier refresh-bridge fix is that only bridges receive a second missing-tenant delete; add equivalent tenantless cleanup for the other globally user-keyed resources without making ID-only agent/file cleanup cross-tenant.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b03de68: the CLI cascade is system-scoped again (as before this PR); only deleteUserAgents and deleteStoredFiles run in the deleted user's tenant, so tenantless user-keyed rows are removed without making ID-keyed cleanup cross-tenant.
| await deps.deleteBalances({ user: userObjectId }); | ||
| await deps.deletePresets(userId); | ||
| await deps.deleteAllUserPluginAuths(userId); | ||
| await deps.deleteSharedLinks(userId); |
There was a problem hiding this comment.
When shared-link ACL deletion fails—or the CLI begins shutting down before it completes—this awaited dependency still reports success because deleteAllSharedLinksWithCleanup launches cleanupBulkSharedLinkPermissions(ids).catch(...) without awaiting it. The cascade then removes the account after the SharedLink rows that identified those resources are already gone; the later principal cleanup removes only the deleted user's entries, so PUBLIC, GROUP, or other-user ACL entries remain permanently orphaned. Use an erasure-specific operation that awaits and propagates the ACL delete before returning.
AGENTS.md reference: AGENTS.md:L117-L122
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b03de68: erasure awaits shared-link ACL cleanup, and removes the ACLs before the links so a failure leaves the account retryable; service.test.ts covers failure then retry.
… Shared-Link ACL Cleanup
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b03de68a79
ℹ️ 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".
| await runAsSystem(() => | ||
| eraseUserData( |
There was a problem hiding this comment.
When the operator CLI deletes a tenant account and another tenant has the same serverName—explicitly supported by the { serverName, tenantId } unique index—this system-scoped cascade makes both findMCPServerByServerName and findMCPServerOAuthTokenUserIds search across all tenants. The former can supply the wrong server configuration, while the latter includes other tenants' token holders; because every user's principals include role/public access, deleting a public or role-shared server can then revoke and delete those unrelated users' OAuth credentials. Fresh evidence beyond the earlier ID-keyed tenant-scope threads is these newly added server-name-keyed OAuth lookups; run them in the deleted user's tenant scope.
AGENTS.md reference: AGENTS.md:L187-L190
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1e7bbdf: the token, viewer ACL and server config lookups run in the deleted user's tenant.
| for (const result of results) { | ||
| if (result.status === 'rejected') { | ||
| logger.error(`[eraseUserData] MCP OAuth cleanup failed for user ${userId}`, result.reason); | ||
| } |
There was a problem hiding this comment.
When token discovery, principal resolution, connection fencing, allowlist resolution, or OAuth uninstall rejects, this loop only logs the rejected result and reports success after the MCP server and its ACL retry anchor have already been deleted. The cascade then deletes the owner's remaining tokens and user document, so external grants, shared-user credentials, or live connections can survive with no discoverable server from which to retry cleanup. Fresh evidence beyond the earlier MCP fencing/config threads is this allSettled rejection suppression; propagate the failure before committing account deletion or persist a durable cleanup retry record.
AGENTS.md reference: AGENTS.md:L117-L122
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1e7bbdf: erasure throws when any shared-user OAuth cleanup rejects, so the failure surfaces.
| const entries = await mongoose.models.AclEntry.find({ | ||
| resourceType: ResourceType.MCPSERVER, | ||
| resourceId: new mongoose.Types.ObjectId(serverId), | ||
| permBits: { $bitsAnySet: PermissionBits.VIEW }, |
There was a problem hiding this comment.
On Azure Cosmos DB for MongoDB, this newly added account-erasure query always fails because $bitsAnySet is unsupported. Both self-service and CLI deletion call this lookup for every named MCP server before deleting it, so affected users can never complete account deletion. The repository's ACL layer already uses permBits: { $in: permissionBitSupersets(...) } specifically for Cosmos compatibility; use that predicate here as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1e7bbdf: the query uses permissionBitSupersets like the ACL layer.
…Shared-User Cleanup Fails
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e7bbdf182
ℹ️ 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".
| uninstallOAuthMCP: (candidateId, pluginKey, cleanupConfig, serverConfig) => | ||
| cleanupMCPServerOAuth({ |
There was a problem hiding this comment.
When the CLI deletes a tenant-scoped MCP server while a shared user is refreshing OAuth credentials, this callback still executes inside the enclosing runAsSystem. cleanupMCPServerOAuth derives its teardown lease and flow IDs from getTenantId(), so it fences the __SYSTEM__ scope instead of the real tenant; the shared user's live refresh can therefore repersist credentials after cleanup, and tenant-scoped flow state is not removed. Fresh evidence beyond the earlier tenant-scoped lookup thread is that the teardown callback itself remains outside runInUserTenant; wrap this call just like the server/token lookups.
AGENTS.md reference: AGENTS.md:L187-L190
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f768334: fencing and OAuth teardown run in the deleted user's tenant.
| resolveAllowlists: async () => ({ | ||
| allowedDomains: userAppConfig?.mcpSettings?.allowedDomains, | ||
| allowedAddresses: userAppConfig?.mcpSettings?.allowedAddresses, | ||
| }), |
There was a problem hiding this comment.
When a removed server has an OAuth token holder whose role/principal config has a different MCP allowlist, this callback ignores candidateId and applies the deleting owner's userAppConfig to every revocation. That can either permit an outbound revocation the affected user's policy forbids or reject one their policy allows, leaving external credentials behind after the server is gone; the HTTP deletion path deliberately calls registry.resolveAllowlists({ userId: candidateUserId }). Resolve each candidate's effective config within the deleted tenant instead of reusing the owner's values.
AGENTS.md reference: AGENTS.md:L187-L190
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f768334: each affected user's role and effective MCP allowlists are resolved inside the tenant.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f768334d34
ℹ️ 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".
| if ((await cleanupSharedUsers(prepared)) > 0) { | ||
| throw new Error('MCP OAuth cleanup failed for users of a removed server'); |
There was a problem hiding this comment.
When shared-user OAuth cleanup fails after the MCP server deletion (for example, during a Redis or provider outage), this throws only after deleteMCPServersWithAcl has removed both the server and its ownership ACL. Fresh evidence beyond the earlier cleanup-failure thread is that the newly propagated error still destroys every discovery record first: on the next account-deletion attempt, findUserErasableMCPServers returns nothing, so the account can be deleted while the external grant or live connection remains. Perform teardown before removing the retry anchors or persist a durable cleanup record.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7969c8b: shared-user OAuth teardown now runs before the server and its ACLs are deleted and a failure aborts erasure, so a retry rediscovers the server; erasure.spec.ts covers failure then retry.
| if (tenantId) { | ||
| /** Bridges written before tenancy carry no tenantId, and the user id is globally unique. */ | ||
| await deps.deleteRefreshTokenBridges({ userId }); |
There was a problem hiding this comment.
When a tenant user self-deletes through the authenticated HTTP route, this second call still runs inside the tenant ALS context established by requireJwtAuth. Fresh evidence beyond the earlier bridge-cleanup thread is that the tenant-isolation pre-hook appends tenantId: <current tenant> via Query.where, which overwrites deleteRefreshTokenBridges' explicit tenantId: { $exists: false } predicate; therefore the legacy tenantless bridge is not deleted, despite the test only verifying dependency arguments outside tenant context. Run this narrowly user-scoped legacy deletion under system context or expose a repository method that safely includes both tenant and missing-tenant rows.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not applicable on 7969c8b: RefreshTokenBridge is excluded from automatic tenant isolation (models/refreshTokenBridge.ts), so the explicit missing-tenant predicate is not overwritten; erasure.spec.ts deletes a tenantless bridge inside tenantStorage.run.
| /** Runtime hooks for MCP servers removed with a deleted account. */ | ||
| const createMCPServerCleanup = (userId) => ({ | ||
| findUserErasableMCPServers: db.findUserErasableMCPServers, | ||
| deleteMCPServersWithAcl: db.deleteMCPServersWithAcl, | ||
| invalidateMCPServerTools: (serverName) => invalidateCachedTools({ userId, serverName }), |
There was a problem hiding this comment.
/api
This new CJS helper contains account-erasure behavior rather than legacy Express wiring: it resolves server configuration and user principals/allowlists, invalidates caches, disconnects runtime clients, and selects the OAuth uninstaller. Keeping this policy in UserController.js makes the shared TypeScript erasure service depend on behavior assembled in the legacy layer and creates another implementation point for non-Express callers; move the cleanup adapter into packages/api and leave this file with imports and a call into that module.
AGENTS.md reference: AGENTS.md:L78-L82
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7969c8b: the controller now only supplies runtime functions; the invalidate-then-disconnect sequencing lives in packages/api.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7969c8b795
ℹ️ 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".
| /** The server and its ACLs are the retry anchors, so they outlive a failed teardown. */ | ||
| if ((await cleanupSharedUsers(prepared)) > 0) { | ||
| throw new Error('MCP OAuth cleanup failed for users of a server being removed'); | ||
| } | ||
| await deps.deleteMCPServersWithAcl(servers.map((s) => s.id)); |
There was a problem hiding this comment.
When a shared user starts a new OAuth authorization or refresh after cleanupSharedUsers returns but before deleteMCPServersWithAcl commits, cleanupMCPServerOAuth has already released its teardown lease and process-local refresh gate, while the server and its ACL still permit the operation. The new credentials can therefore be persisted after the cleanup snapshot, and deleting the server then removes the retry anchor while the shared user's token and external grant survive. Fresh evidence beyond the earlier retry-anchor thread is this post-cleanup/pre-delete race; hold the teardown fence through the server deletion or retain a durable cleanup record.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid but narrow and not a regression: the window is milliseconds on an account already mid-deletion, and erasure did no shared-user cleanup before this PR. Tracked at berry-13#365.
| if (options.awaitCleanup && ids.length > 0) { | ||
| /** The links identify the ACL resources, so they must outlive a failed cleanup for a retry. */ | ||
| await cleanupBulkSharedLinkPermissions(ids); | ||
| } | ||
| const result = await SharedLink.deleteMany({ user }); |
There was a problem hiding this comment.
When an already-authenticated GET /share/link/:conversationId for a legacy link is in flight during account deletion, this pre-delete ACL cleanup can complete and then ensureLinkPermissions can run while the SharedLink still exists; autoMigrateLegacyLink recreates both owner and PUBLIC ACL entries before deleteMany removes the link. The later principal cleanup removes only the owner's entry, leaving the PUBLIC ACL orphaned with no link available for a retry to discover it. Fresh evidence beyond the earlier awaited-cleanup thread is this mixed-version migration race; serialize migration with deletion or retain the resource IDs through a post-delete cleanup retry.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid but pre-existing: the same window exists on dev for self-service and needs an in-flight legacy-link request during the user's own deletion. Tracked at berry-13#366.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7fce3370a
ℹ️ 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".
| invalidateCachedTools({ userId: candidateId, serverName }), | ||
| disconnectUser: (candidateId, serverName) => | ||
| getMCPManager()?.disconnectUserConnection(candidateId, serverName), | ||
| uninstallOAuthMCP: maybeUninstallOAuthMCP, |
There was a problem hiding this comment.
When the provider rejects access- or refresh-token revocation, this account-erasure adapter still reports success because cleanupMCPServerOAuth catches revokeOAuthToken failures at packages/api/src/mcp/oauth/cleanup.ts:540-542 and returns normally. The cascade consequently deletes the MCP server, its ACL retry anchor, and ultimately the account while an external grant remains valid. Fresh evidence beyond the earlier cleanup-failure thread is that the supplied uninstaller itself suppresses provider failures, so the new Promise.allSettled rejection handling never sees them; account erasure needs a strict teardown mode that clears local state but then rejects so the server remains discoverable for retry.
AGENTS.md reference: AGENTS.md:L117-L122
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid but pre-existing: cleanupMCPServerOAuth hides provider revocation failures for the server-delete route on dev too. Tracked at berry-13#369.
| deleteSharedLinks: (id) => deleteAllSharedLinksWithCleanup(id, { awaitCleanup: true }), | ||
| deleteStoredFiles: (files) => | ||
| runInUserTenant(async () => processDeleteRequest({ req: fileRequest, files })), | ||
| deleteUserAgents: (id) => runInUserTenant(async () => methods.deleteUserAgents(id)), |
There was a problem hiding this comment.
When the operator deletes a tenant user who still owns an agent written before tenant stamping, wrapping the entire deleteUserAgents call in runInUserTenant makes its initial Agent.find({ author }) receive tenantId: <current tenant> from the isolation plugin, so the valid missing-tenantId row is never discovered and survives after its author is deleted. Fresh evidence beyond the earlier tenantless-row thread is that agentSchema.tenantId remains optional, while the previous CLI's author-based Agent.deleteMany was unscoped; keep ID-based reference pruning tenant-scoped, but separately discover and delete the user's tenantless authored agents.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e382a5e: the CLI no longer wraps deleteUserAgents in the tenant; reference cleanup is pinned per agent tenant inside the method, so tenantless authored agents are still discovered and deleted.
e7fce33 to
e382a5e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e382a5e859
ℹ️ 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".
| try { | ||
| await deps.deleteStoredFiles(files ?? []); | ||
| } catch (error) { | ||
| logger.error(`[eraseUserData] Failed to delete stored files for user ${userId}`, error); | ||
| } |
There was a problem hiding this comment.
When a storage provider is unavailable, processDeleteRequest normally resolves with nonempty failedFileIds because it awaits deletions with Promise.allSettled; if setup or metadata cleanup throws instead, this catch suppresses that too. The cascade then deletes every File record, the user's provider keys, and eventually the account, leaving remote objects with no metadata or credentials from which to retry deletion. Treat either reported failures or a rejection as an operational failure and retain the account/metadata for retry.
AGENTS.md reference: AGENTS.md:L117-L122
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid but pre-existing and a recorded non-goal: the controller dropped File rows after a failed storage delete before this PR. Tracked at berry-13#328.
| deleteStoredFiles: (files) => | ||
| runInUserTenant(async () => processDeleteRequest({ req: fileRequest, files })), |
There was a problem hiding this comment.
Fresh evidence beyond the earlier tenant-scope thread is that this wrapper narrows processDeleteRequest only to a tenant, not to the file owner. If two users in the same tenant have the same provider-issued file_id—which the File schema permits—processDeleteRequest calls deleteFiles and removeAgentResourceFilesFromAllAgents by file_id alone, deleting the bystander's metadata and agent references as well. Use an owner-scoped metadata/reference cleanup operation rather than relying solely on tenant context.
AGENTS.md reference: AGENTS.md:L187-L190
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid but pre-existing: every file-deletion flow on dev deletes metadata and agent references by file_id alone, and erasure inherits it. Tracked at berry-13#371.
| getServerConfig: (_serverName, serverId) => | ||
| runInUserTenant( | ||
| async () => (await methods.findMCPServerByObjectId(serverId))?.config, | ||
| ), |
There was a problem hiding this comment.
Fresh evidence beyond the earlier cross-tenant OAuth thread is the mixed-version case: when a tenant user owns an MCP server created before tenant stamping, this ID lookup runs under the user's current tenant and the isolation plugin hides the tenantless server, so getServerConfig returns undefined. cleanupMCPServerOAuth consequently takes its local-state-only path instead of revoking the provider grant, after which the cascade deletes the server and its retry anchor; retrieve the specific legacy server safely outside the active tenant predicate or explicitly include missing-tenant records.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 141547a: the CLI reads the server config by its unique object id outside the tenant wrapper, so a server stamped before tenancy still yields its config for revocation.
e382a5e to
141547a
Compare
Summary
The
delete-useroperator CLI said it removed a user "and ALL their data", but it only ran rawdeleteManycalls, so it erased less than self-service account deletion. Uploaded files stayed in storage after their File rows were dropped. Skills, MCP servers, agent checkpoints and shared-link permissions were left behind. Agent API keys were never deleted either, because the CLI filtered them by the wrong field.The account erasure cascade now lives in one
eraseUserDataservice inpackages/api, and both the controller and the CLI call it, so the two paths cannot drift apart. The cascade also erases tool favorites, chat projects, queued agent turns and refresh-token bridges, which neither path removed before. Fences, schedule suspension, generation aborts, the transaction prompt and deleting the user document last stay with each caller.Self-service deletion also never removed stored files when 2FA was off: the DELETE carries no body,
processDeleteRequestthrew onreq.body.assistant_id, and the error was logged and skipped. The controller now passes an empty body. Database failures while deleting plugin credentials, agents, prompts or MCP server records, or listing the user's files, now abort the deletion and restore the fences instead of being skipped, so the account stays retryable. Stored files are deleted before user keys, so user-provided assistants credentials still exist at that point; a storage or provider failure on one file is still logged and skipped. The MCP ownership queries moved into data-schemas methods. Removing a server on account deletion now also fences and uninstalls the OAuth state of the users it was shared with, the way the server-delete route does, for both self-service and the CLI (berry-13#355). The CLI runs ID-keyed cleanup for agents, files and MCP lookups in the deleted user's tenant, and agent and token cleanup pins a tenantless user to tenantless records (berry-13#357).Type of change
Testing
Tested environments/configuration:
Automated tests:
delete-user-cli.spec.tsrun the CLI (deleting and keeping the ledger) andDELETE /api/user/deleteagainst a seeded account and a bystander. They check that the upload bytes, files, agent API keys, legacy MCP servers, tool favorites and chat projects are gone and that the bystander keeps everything. The self-service scenario fails on the previous head, with the upload left on disk.packages/api/src/user/erasure.spec.tscovers every store in the cascade, the retained ledger, file deletion before user keys, checkpoint, MCP, agent and prompt lookup failures, and shared-user MCP OAuth cleanup.config/__tests__/delete-user.spec.js, the controller specs (deleteUser, resource coverage, MCP server deletion, UserController) and the data-schemas mcpServer, favorite and chatProject specs pass.reviewctl precheck(eslint, prettier, import order, typecheck, related jest in api, packages/api and data-schemas): pass.Checklist
Known gaps, tracked separately
Deferred and not part of this change: berry-13#328, berry-13#364, berry-13#365, berry-13#366, berry-13#369, berry-13#371.