Repository navigation
🧹 fix: Run the Full Erasure Cascade When an Admin Deletes a User - #17050
Conversation
|
Lighthouse CI failed. The last 80 log lines contain the measured budgets and assertion failures. |
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: 1361a11605
ℹ️ 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".
| role, | ||
| tenantId, | ||
| }: AdminErasureTarget): Promise<AdminErasureContext> { | ||
| const appConfig = await deps.getAppConfig({ role, userId: id, tenantId }); |
There was a problem hiding this comment.
When principal, override, or runtime-augmentation lookup fails, getAppConfig defaults to logging the error and returning the base config unless failClosed: true is requested. For a target whose Assistants storage is enabled through a user/group override, this makes processDeleteRequest unable to initialize the required provider client; those storage errors are best effort, the file metadata and user are still deleted, and the external files become permanently orphaned instead of being retried. Request fail-closed resolution for this destructive cascade so a transient config-store failure reaches the handler and preserves the account for retry.
AGENTS.md reference: AGENTS.md:L117-L122
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 311a37b: createErasureResolver now requests failClosed, so a failed override lookup rejects before any erasure and the handler releases the fences and returns 500. resolver.spec.ts asserts the option and the rejection.
codegraph-librechat
Bot
added
🗺️ Backend Infra
311a37b to
e5dd0aa
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5dd0aa552
ℹ️ 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 quiesceUserSchedules(id, scheduleSuspensionToken))) { | ||
| throw new Error('Scheduled executions could not be confirmed stopped'); | ||
| } | ||
| await stopUserGenerations(id, targetUser?.tenantId, generationJobs); |
There was a problem hiding this comment.
If a chat or agent request authenticates just before this handler sets the user fence but reaches GenerationJobManager.createJob after this one-shot job listing, its new generation is absent from the drain. api/strategies/jwtStrategy.js checks agentTriggerDeletionStartedAt only during authentication, while GenerationJobManager.createJob does not recheck it, so the provider can continue writing after eraseUserData and recreate orphaned messages or files for the deleted account. Add a generation-admission barrier tied to the deletion fence, or atomically recheck the fence during job creation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred: pre-existing on the base, the self-service deleteUserController has the same window between the job listing and createJob; this PR only reuses it. Tracked in berry-13#340.
There was a problem hiding this comment.
Fixed in bbf81ca: createJob now runs an admission guard after the job is committed and refuses with ACCOUNT_DELETION_IN_PROGRESS, so a generation admitted after the fence is rejected and one admitted before it is in the drain.
| { userId: id, tenantId: targetUser?.tenantId, checkpointer }, | ||
| erasureDeps, | ||
| ); | ||
| const result = await deleteUserById(id); |
There was a problem hiding this comment.
When auth-cache eviction fails after MongoDB has already deleted the row, packages/data-schemas/src/methods/user.ts:601-614 makes deleteUserById reject instead of returning its deletedCount. At this call, userDeleted therefore remains false and the catch returns 500 before config and code-environment cleanup, even though the account is gone and a retry can only return 404. Fresh evidence relative to the earlier thread is that this dependency can reject post-commit, not only while leaving the account in place. Preserve the required cache invalidation, but expose or reconcile the committed delete so post-commit cleanup still runs.
AGENTS.md reference: AGENTS.md:L154-L158
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred: deleteUserById rejecting after a committed delete is pre-existing for every caller, and the fix changes its return contract. Tracked in berry-13#341.
| const fileRequest = { | ||
| user: { id, _id: id, role, tenantId }, |
There was a problem hiding this comment.
When Azure Assistants uses target-derived configured headers such as {{LIBRECHAT_USER_EMAIL}}, this synthetic request contains only the ID, role, and tenant ID. resolveHeaders(..., stripUnresolved: true) consequently turns other target placeholders into empty values, so the upstream file delete can fail; processDeleteRequest and eraseUserData then remove the metadata under their best-effort policy, permanently orphaning the provider object. Load and pass the target fields needed to construct the same safe user identity used during self-deletion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a3206a9: the handler loads the target's profile fields and the resolver passes them into the storage request, so header templates resolve as in self-deletion.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3206a9949
ℹ️ 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".
| role?: string; | ||
| tenantId?: string; | ||
| /** The target's profile fields, which storage header templates such as the user email read. */ | ||
| profile?: Partial<IUser>; |
There was a problem hiding this comment.
IUser extends Mongoose's Document, so exporting profile?: Partial<IUser> widens the packages/api public contract with persistence-specific members. Define this as a storage-neutral Pick of the profile fields required by header templates (or use an existing plain user-record type) instead; the repository guidance explicitly requires new exported API signatures to avoid Mongoose types even where older contracts already leak them.
AGENTS.md reference: AGENTS.md:L84-L89
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2417a15: the profile is now a Pick of plain scalar user fields, no Mongoose members in the exported type.
| profile, | ||
| }: AdminErasureTarget): Promise<AdminErasureContext> { | ||
| /** A failed override lookup must abort the deletion, not fall back to a base config that cannot reach the target's storage. */ | ||
| const appConfig = await deps.getAppConfig({ role, userId: id, tenantId, failClosed: true }); |
There was a problem hiding this comment.
This call omits idOnTheSource even though the handler has just loaded the target profile. Consequently, getUserPrincipals treats that field as unknown and executes its fallback User.findById(...).select('role idOnTheSource') before it can check the config cache, adding a serial Mongo read to every admin-erasure config/file path. Project and pass idOnTheSource (or pass already resolved principals) so the loaded user data is reused as required.
AGENTS.md reference: AGENTS.md:L187-L190
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2417a15: the handler selects idOnTheSource and passes it to getAppConfig, so principal resolution reuses the loaded user.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2417a157de
ℹ️ 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".
| const fileRequest = { | ||
| user: { ...profile, id, _id: id, role, tenantId }, |
There was a problem hiding this comment.
When Azure Assistants headers use {{LIBRECHAT_OPENID_ACCESS_TOKEN}} or {{LIBRECHAT_OPENID_ID_TOKEN}}, this synthetic target request cannot reproduce a working self-delete request: the projected profile contains openidId but no federatedTokens/openidTokens, which resolveHeaders requires and openIdJwtStrategy normally attaches at authentication time. Header resolution therefore throws a reauthentication error; eraseUserData treats that storage failure as best effort and subsequently removes all file metadata, permanently orphaning the upstream objects. Fresh evidence beyond the earlier profile-field thread is that the added projection covers ordinary LIBRECHAT_USER_* fields but still omits the credential source used by supported OpenID placeholders; resolve a target-scoped credential or retain a retry anchor when it is unavailable.
AGENTS.md reference: AGENTS.md:L117-L122
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbf81ca6a4
ℹ️ 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".
| let admissionFailure: unknown; | ||
| if (!this.shuttingDown && this.admissionGuard != null) { | ||
| try { | ||
| admissionDenied = !(await this.admissionGuard(userId)); |
There was a problem hiding this comment.
When the standard server handles an agent start, api/server/index.js:139 installs isAgentTriggerPrincipalActive as this guard, but api/server/controllers/agents/request.js:1764 invokes the same Mongo predicate again immediately after createJob. Both calls are awaited serially, so every generation now incurs an unnecessary user-document read—250 ms in the repository's Lighthouse lane—on the latency-critical startup path. Consolidate these into a single post-commit admission barrier rather than querying twice.
AGENTS.md reference: AGENTS.md:L57-L62
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b1b6202: the controller's recheck is removed, so createJob's guard is the single post-commit admission query.
| processDeleteRequest: (params: { | ||
| req: Record<string, unknown>; | ||
| files: IMongoFile[]; | ||
| }) => Promise<unknown>; |
There was a problem hiding this comment.
IMongoFile extends Mongoose's Document (packages/data-schemas/src/types/file.ts:85), so this newly exported ErasureResolverDeps parameter exposes storage-engine members through the packages/api public contract. Although the target profile was narrowed to scalar fields, this parameter still widens the same persistence coupling; define a plain file DTO containing only the fields consumed by deletion instead.
AGENTS.md reference: AGENTS.md:L84-L89
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b1b6202: the resolver takes a Pick of the plain stored-file fields instead of IMongoFile.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1b62026c4
ℹ️ 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".
| cleanupOnComplete: !isEnabled(process.env.STREAM_KEEP_COMPLETED_JOBS), | ||
| }); | ||
| GenerationJobManager.setApprovalExpiredHandler(recordExpiredScheduleApproval); | ||
| GenerationJobManager.setAdmissionGuard(agentEventMethods.isAgentTriggerPrincipalActive); |
There was a problem hiding this comment.
When the app is launched via npm run backend:experimental, api/server/experimental.js:366-372 initializes GenerationJobManager but never calls setAdmissionGuard. Because this patch also removes the controller-level isAgentTriggerPrincipalActive check, a generation authenticated before self-deletion can commit after that worker's deletion drain and start without ever observing the deletion fence, allowing it to recreate user data after erasure. Configure the same guard in every server entrypoint or retain a shared request-level barrier.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fb444cc: experimental.js installs the same admission guard.
| let admissionFailure: unknown; | ||
| if (!this.shuttingDown && this.admissionGuard != null) { | ||
| try { | ||
| admissionDenied = !(await this.admissionGuard(userId)); |
There was a problem hiding this comment.
For every OpenAI or Responses agent run on the standard server, this awaits isAgentTriggerPrincipalActive, then enrollAgentExecution immediately awaits isSubagentOwnerAdmissible (packages/api/src/agents/remote/lifecycle.ts:214-216); the latter's Mongo predicate already checks agentTriggerDeletionStartedAt. These serial User.exists calls add a redundant database round trip to every remote generation. Fresh evidence relative to the earlier duplicate-query comment is that the ordinary controller duplicate was removed, but both remote API paths still perform the two reads; combine their account-deletion and subagent-fence checks into one call-specific admission barrier.
AGENTS.md reference: AGENTS.md:L187-L190
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fb444cc: enrollAgentExecution skips the createJob guard and keeps its single principal check.
| await this.completeJob( | ||
| streamId, | ||
| admissionRefused ? ACCOUNT_DELETION_JOB_ERROR : SHUTDOWN_JOB_ERROR, | ||
| jobData.createdAt, |
There was a problem hiding this comment.
If a generation commits after account deletion has taken its one-shot job listing, this guard correctly denies provider startup, but completeJob(..., ACCOUNT_DELETION_JOB_ERROR) leaves the error job durable by design (finishTerminalJob skips deletion for error status at lines 4409-4451). The retained job still contains the user's initialMetadata, including the preliminary user message and MCP request body, and the erasure flow never lists it again, so account deletion can finish while user content remains in the stream store. Fresh evidence beyond the resolved admission-race thread is that its proposed fix terminalizes rather than removes these post-drain jobs; explicitly purge an unexposed admission-refused job or include it in a final deletion barrier.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fb444cc: a refused job is purged with the CAS delete after it is retired, so no user metadata is retained.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb444cce0a
ℹ️ 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 this.jobStore.deleteJob(streamId, jobData.createdAt).catch((purgeError) => { | ||
| logger.error( | ||
| `[GenerationJobManager] Failed to purge refused generation ${streamId}:`, | ||
| purgeError, | ||
| ); |
There was a problem hiding this comment.
Fresh evidence beyond the earlier retained-job comment is that the newly added purge remains best-effort: when deleteJob rejects, this catch logs the failure and still returns the normal admission error, while a completeJob rejection above skips the purge entirely. Because this job can be committed after account deletion's one-shot generation listing and contains the user's preliminary message and MCP body, deletion can finish while that content remains durable; make the purge failure retriable or add a final deletion barrier rather than swallowing it.
AGENTS.md reference: AGENTS.md:L117-L122
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 21c32cf: the purge runs in a finally block even if retiring the job fails, and a purge failure now fails the request instead of being logged.
| if (admissionFailure != null) { | ||
| throw admissionFailure; |
There was a problem hiding this comment.
When the injected Mongo-backed admission guard rejects, this rethrows the original operational exception. The ordinary agent controller then reaches its generic initialization branch and sends error.message in the HTTP 500 response (api/server/controllers/agents/request.js:3574-3579), potentially exposing Mongo topology, hostnames, or other internal diagnostics. Log the original exception but throw a stable coded error with user-safe text.
AGENTS.md reference: AGENTS.md:L123-L129
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 21c32cf: the original error is logged and the caller gets a stable GENERATION_ADMISSION_UNAVAILABLE error with fixed text.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21c32cf53f
ℹ️ 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".
| const job = await deps.manager.createJob(runId, userId, conversationId, { | ||
| /** The principal check below is this path's single post-commit admission read. */ | ||
| skipAdmissionGuard: true, |
There was a problem hiding this comment.
When account deletion takes its one-shot job listing just before this remote run commits, skipAdmissionGuard allows the job to bypass createJob's refusal-and-purge path; the subsequent principal check sees the deletion fence, but retireRejectedEnrollment only calls completeJob, and error jobs remain durable. Deletion can therefore finish while the stream store still retains the deleted user's ID and conversation ID. Fresh evidence beyond the resolved refused-job thread is that this remote path now explicitly bypasses the manager guard without implementing the same purge.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f958a09: remote enrollment passes its principal predicate as createJob's admissionCheck, so a refused run is retired and purged by the same path.
| /** A refused job never ran, and its metadata holds the user's message; the erasure that | ||
| * refused it will not list it again, so purge it even when retiring it failed. A purge | ||
| * failure propagates so the caller fails instead of reporting a clean refusal. */ | ||
| await this.jobStore.deleteJob(streamId, jobData.createdAt); |
There was a problem hiding this comment.
When deleteJob rejects during an account-deletion refusal, this finally propagates the raw store exception instead of reaching GenerationAdmissionDeniedError or GenerationAdmissionUnavailableError. The ordinary agent controller's fallback at api/server/controllers/agents/request.js:3574-3579 returns that exception's message in the HTTP 500 response, so Redis/Mongo connection details can be exposed precisely during a cleanup outage. Fresh evidence beyond the resolved admission-lookup thread is that the newly added purge-failure test explicitly preserves this raw rejection path; log it and throw a fixed coded error while still failing the request.
AGENTS.md reference: AGENTS.md:L123-L129
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f958a09: a purge failure is logged and surfaces a fixed GENERATION_ADMISSION_UNAVAILABLE error.
| await this.processDurableReplacementReceipts(streamId, jobData, conversationId).catch( | ||
| (handoffError) => { | ||
| logger.error( | ||
| `[GenerationJobManager] Failed to hand off predecessor during shutdown ${streamId}:`, | ||
| `[GenerationJobManager] Failed to hand off predecessor during refused admission ${streamId}:`, | ||
| handoffError, |
There was a problem hiding this comment.
When this committed job replaced an active same-stream generation, its replacement receipt is the only durable link to the predecessor because account cleanup enumerates only the current job. The admission lookup runs before this handoff, so deletion can abort the new job and proceed without waiting for the displaced provider; moreover, if notification or drain confirmation fails here, the error is swallowed and the finally at line 2793 deletes the receipt anyway. The predecessor can then keep persisting user data after erasure, so a refused replacement must remain cleanup-blocking until every inherited predecessor receipt is confirmed drained.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f958a09: the refused job is purged only after the predecessor handoff is confirmed; otherwise it is retained and the request fails.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34daaec619
ℹ️ 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".
| `[GenerationJobManager] Failed to hand off predecessor during refused admission ${streamId}:`, | ||
| getSafeErrorMetadata(handoffError), | ||
| ); | ||
| return false; |
There was a problem hiding this comment.
When a replacement commits after account deletion's one-shot generation listing and this handoff fails, returning false only causes the generation request to fail; the refused job and its predecessor receipt remain durable, while the deletion request proceeds from stopUserGenerations directly into eraseUserData without rechecking them. The predecessor may therefore continue writing after erasure, and the retained job still contains user metadata. Fresh evidence beyond the resolved thread is that the current admin flow has no final cleanup barrier that observes the intentionally retained job; persist a failure signal checked by deletion or repeat the drain before deleting the user.
AGENTS.md reference: AGENTS.md:L117-L122
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 24317c0: createJob now refuses before creating any job, receipt or claim, so there is no refused job or predecessor handoff left.
| return false; | ||
| }); | ||
| try { | ||
| await this.completeJob( |
There was a problem hiding this comment.
When admission is refused and completeJob rejects, the finally block may successfully purge the job and then the original store exception escapes this await. Unlike the sanitized admission-check and purge branches, the ordinary agent controller handles that exception through its generic fallback and returns error.message, so Redis/Mongo connection details can reach the HTTP response. Log the retirement failure and translate it to GenerationAdmissionUnavailableError before it leaves createJob.
AGENTS.md reference: AGENTS.md:L123-L129
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 24317c0: the retirement step no longer exists; admission check failures map to a fixed GENERATION_ADMISSION_UNAVAILABLE error.
| * refused it will not list it again, so purge it even when retiring it failed. It stays | ||
| * until the displaced predecessor is confirmed drained, because the replacement receipt | ||
| * is the only durable link to that provider. A purge failure fails the request. */ | ||
| await this.jobStore.deleteJob(streamId, jobData.createdAt).catch((purgeError) => { |
There was a problem hiding this comment.
For an ordinary chat submission with a client request ID, createJob has already marked both idempotency claims as started before reaching the admission check. If the guard throws transiently, this deletion removes the only job, but the controller cannot release the started claim; retrying the advertised 503 with the same request ID enters the missing-job/started-claim path and returns a settled 200 even though no provider ran and no response was persisted. Purge or explicitly terminalize the matching claims together with the unadmitted job so the request remains genuinely retryable.
AGENTS.md reference: AGENTS.md:L50-L53
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 24317c0: refusal happens before the idempotency claim is marked, so nothing needs clearing.
| if (!(await quiesceUserSchedules(id, scheduleSuspensionToken))) { | ||
| throw new Error('Scheduled executions could not be confirmed stopped'); | ||
| } | ||
| await stopUserGenerations(id, targetUser?.tenantId, generationJobs); |
There was a problem hiding this comment.
If an /api/assistants/v2/chat request passes requireJwtAuth just before the deletion fence is set, it never creates a GenerationJobManager job, so this drain cannot see or cancel it. That controller can later reach saveAssistantMessage after eraseUserData has removed the target's records, recreating messages and conversation state for a deleted user; the v1 Assistants path has the same separate run lifecycle. Register these runs with the account cleanup barrier or add a post-auth deletion check and drain before their persistence steps.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred: Assistants runs live outside GenerationJobManager and the gap is pre-existing for self-service deletion; tracked in berry-13#352.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24317c05c5
ℹ️ 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 (admissionCheck != null) { | ||
| let admitted: boolean; | ||
| try { | ||
| admitted = await admissionCheck(userId); |
There was a problem hiding this comment.
When this lookup returns active just before deletion sets the fence, execution continues into the later jobStore.createJob call without another check. The deletion path only performs two generation scans separated by the fixed two-second grace period, so a delayed store create can commit after both scans and then start its provider after the account has been erased. Fresh evidence relative to the earlier resolved admission-race thread is that the final code now runs the guard before publishing any durable job, rather than after commit as that resolution described; make admission atomic with job publication or recheck after commit while keeping a refused job cleanup-blocking until it is safely purged.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted by design: admission is checked before any durable state and deletion drains twice around a documented grace period; a post-commit barrier was removed on purpose because its refusal path kept producing cleanup defects. A create delayed beyond the grace period is the residual window.
There was a problem hiding this comment.
Fixed in abd65e9: createJob re-checks the deletion marker after the job is committed and retires a refused job, and deletion sets the marker before it lists, so a delayed create sees the marker; no timer is involved.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abd65e9448
ℹ️ 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 this.completeJob( | ||
| streamId, | ||
| postCommitRefusal != null ? ACCOUNT_DELETION_JOB_ERROR : SHUTDOWN_JOB_ERROR, | ||
| jobData.createdAt, | ||
| ); |
There was a problem hiding this comment.
When a job commits after account deletion's one-shot generation scan, the second admission check reaches this branch, but completeJob deliberately retains error jobs for late subscribers. The retained row contains the deleted user's initial metadata, including the preliminary message and MCP request body, so deletion can report success while that content remains in the stream store. Fresh evidence beyond the previously resolved retained-job thread is that the final refactor reintroduced completeJob here without the conditional purge; remove this unexposed job after safely settling its predecessors.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted by the owner's design: a post-commit refusal retires the job through the normal terminal path, with no bespoke purge; the row follows normal job retention.
| return false; | ||
| }, |
There was a problem hiding this comment.
When the refused post-commit job replaced an active same-stream generation, processDurableReplacementReceipts can return false or reject because the predecessor could not be notified or confirmed drained, but this branch ignores both outcomes and terminalizes the replacement. If deletion already scanned before the replacement committed, it can then erase the account while the displaced provider continues writing. Fresh evidence beyond the previously resolved predecessor-handoff thread is that this final refactor again swallows the handoff result; retain a cleanup-visible blocker or otherwise make account deletion wait until every receipt is confirmed.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted by the owner's design: refusal reuses the existing terminal path rather than adding a handoff barrier.
| /** Refuse before any durable state exists; the common case leaves nothing to clean up. */ | ||
| await this.assertAdmitted(streamId, userId, options); |
There was a problem hiding this comment.
On both server entrypoints the installed guard is isAgentTriggerPrincipalActive, which performs User.exists, so every successfully admitted generation now waits for this Mongo query here and then waits for the identical query again at line 2786. This adds two serial request-path database round trips—about 500 ms in the repository's Lighthouse lane—to every normal generation. Fresh evidence beyond the earlier resolved duplicate-query thread is that the final refactor introduced both the pre-commit and post-commit calls; consolidate the invariant without querying twice.
AGENTS.md reference: AGENTS.md:L187-L190
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted by the owner's design: the pre-check refuses before any durable state and the post-check closes the race, so both reads are intended.
| resolveErasure, | ||
| cancelSubagentTasks: (userId, tenantId) => | ||
| subagentThreadTaskStore.cancelAndDrainForOwner(userId, tenantId), | ||
| quiesceUserSchedules, | ||
| restoreUserSchedulesFromDeletion, | ||
| generationJobs: GenerationJobManager, |
There was a problem hiding this comment.
This adds the full admin-erasure dependency graph, but the repository-wide search finds its only HTTP binding still commented out at api/server/routes/admin/users.js:80, and there is no other caller of handlers.deleteUser; consequently none of this cascade can run when an administrator uses the shipped application. Wire the handler to the capability-protected route and provide the corresponding frontend entry point so the advertised admin deletion behavior is observable.
AGENTS.md reference: AGENTS.md:L50-L53
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted: mounting the admin DELETE route is a recorded non-goal; it stays commented out on dev.
b00ee4a to
82a5dcc
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82a5dccbd9
ℹ️ 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".
| let postCommitRefusal: Error | undefined; | ||
| if (!this.shuttingDown) { | ||
| try { | ||
| await this.assertAdmitted(streamId, userId, options); | ||
| } catch (error) { | ||
| postCommitRefusal = error as Error; |
There was a problem hiding this comment.
When the pre-commit guard succeeds but the second admission lookup transiently rejects, this branch has already committed the job and marked its idempotency claim as started, then terminalizes the job and returns a retryable 503. releaseGeneration deliberately refuses to clear started claims, so retrying with the same client request ID follows the duplicate/settled path instead of starting the provider, leaving the user's submission unprocessed until the claim expires. Fresh evidence relative to the earlier claim-cleanup thread is that this final code reintroduces a refusal after the claim is committed; an unavailable post-commit check needs a genuinely retryable claim/job outcome.
AGENTS.md reference: AGENTS.md:L50-L53
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted: when the marker cannot be read the request must fail closed, and reaching this branch needs two consecutive lookup failures; the started claim then expires on its own.
| await this.completeJob( | ||
| streamId, | ||
| postCommitRefusal != null ? ACCOUNT_DELETION_JOB_ERROR : SHUTDOWN_JOB_ERROR, | ||
| jobData.createdAt, | ||
| ); |
There was a problem hiding this comment.
When admission is refused after commit and this terminalization rejects because the Redis/Mongo store is unavailable, the raw storage exception replaces the stable GenerationAdmissionDeniedError or GenerationAdmissionUnavailableError. The ordinary agent controller then takes its generic 500 branch and returns error.message, which can expose topology or connection details. Fresh evidence relative to the earlier retirement-failure thread is that the final refactor has reintroduced an awaited completeJob on this refusal path; log the original failure and surface a fixed coded error instead.
AGENTS.md reference: AGENTS.md:L123-L127
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 95a0175: a retirement failure is logged with safe metadata and surfaces the fixed GENERATION_ADMISSION_UNAVAILABLE error.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95a0175b70
ℹ️ 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".
| let postCommitRefusal: Error | undefined; | ||
| if (!this.shuttingDown) { | ||
| try { | ||
| await this.assertAdmitted(streamId, userId, options); |
There was a problem hiding this comment.
If jobStore.createJob commits after account deletion's generation scan but its response is lost, and recoverCommittedCreate also fails because the store remains unavailable, createJob exits at the ambiguous-create catch before reaching this post-commit admission check. The durable running job can therefore retain the deleted user's preliminary message and MCP request body indefinitely, even though no provider starts and deletion proceeds. Fresh evidence beyond the resolved ordinary race is the explicit commit-then-recovery-failure path at lines 2668-2685; reconcile that possible commit against the deletion fence or add a final cleanup barrier.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted: a committed create whose recovery also fails is an existing createJob ambiguity for every caller under a double store fault, not specific to deletion; the leftover job is reaped by normal expiry.
56ad7b0 to
46c9faa
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46c9faa7cc
ℹ️ 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".
| const inTargetTenant = <T>(operation: () => Promise<T>): Promise<T> => | ||
| tenantId ? tenantStorage.run({ tenantId, userId: id }, operation) : runAsSystem(operation); |
There was a problem hiding this comment.
createErasureResolver is otherwise dependency-injected, but this helper directly reaches into the process-wide tenantStorage/runAsSystem implementation. That couples the exported packages/api service to the current Mongoose/ALS runtime and prevents another host, storage implementation, or isolated test from supplying the tenant boundary; accept a withTenant dependency from the caller instead.
AGENTS.md reference: AGENTS.md:L94-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in db52747: createErasureResolver now takes a withTenant dependency and the route supplies it.
…the Erasure File Type
db52747 to
f085493
Compare
Summary
The admin user delete handler in
packages/api/src/admin/users.tsremoved only the user row, trigger deliveries, tokens, passkeys, config and ACL entries, so every conversation, message, file, agent, prompt, memory and stored object the account owned stayed in the database after an admin deletion. It also never suspended the user's schedules or stopped their in-flight generations, so a running job could keep writing for an account that no longer exists. A compliance scan flagged it (the self-service path and the CLI are covered by #17033, which this builds on).The admin handler now runs the same
eraseUserDatacascade as self-service deletion. Before erasing, it cancels the user's subagent tasks, suspends their schedules under a per-attempt token and stops their generations through a new sharedstopUserGenerations. If any step fails, the user row is kept, schedules are restored, the trigger fences are released and the request returns 500 so it can be retried. Stored files are deleted as the target user, not the calling admin. The dependency assembly lives increateErasureResolver(packages/api/src/user/resolver.ts), and the self-service controller now callsstopUserGenerationsinstead of its inline copy. The adminDELETE /:idroute is still commented out on dev, so this takes effect once it is mounted.A generation could also be admitted just before an account deletion began and keep writing after the erasure.
GenerationJobManager.createJobnow runs an injected admission guard once the job is committed, retires the job and refuses withACCOUNT_DELETION_IN_PROGRESSwhen the deletion fence is set, so self-service and admin deletion are both covered (closes berry-13#340).Depends on #17033.
Type of change
Testing
Tested environments/configuration: unit tests with mongodb-memory-server.
Automated tests:
users.spec.ts: cascade runs before the user row is removed, a cascade failure returns 500 and releases fences, schedule restore ordering, failed suspend, abort scoped to the target's jobs.generations.spec.ts: uses a class-instance job manager whose methods readthis(fails if the persistence wait is handed an unbound method).resolver.spec.ts: target config and checkpointer, file deletion as the target user, MCP cleanup scoped to the target, missing MCP manager.packages/apisrc/userandsrc/admin/users.spec.ts: 62 passed.api/server/controllers/__tests__/deleteUser.spec.js: 12 passed.admissionGuard.spec.ts: a job admitted during deletion is refused, a guard failure retires the job, and a job admitted before the fence is stopped bystopUserGenerations.tsc --noEmitclean in packages/api; eslint, prettier and import sort clean.Screenshots / recordings
No user-facing change.
Risk / compatibility
Storage provider failures during erasure stay best effort, as #17033 decided: they are logged and the account is still erased. The admin handler no longer takes a
deleteTokensdependency because the cascade deletes tokens.Checklist