Repository navigation
🌳 fix: Preserve Primary Files Until Secondary Cleanup Succeeds - #17228
lia-by-librechat[bot] wants to merge 9 commits into
Conversation
|
Head Coverage added for injected secondary failures with readable real primary files, partial-cleanup retry, primary failure, provider arguments, source deduplication, and both Code Environment reference formats. Sixteen isolated Node 24 smoke cases passed against the actual changed source. Local Jest/typecheck, full static checks, and Lighthouse are blocked by the absent shared dependency installation; no install was attempted. CI is pending. Independent exact-head review is unavailable in this runtime: no PR Reviewer delegation tool is exposed. This is not a clean-review claim. Frozen review target: base |
|
Head Confirmed at this head in CI: package build, TypeScript checks, all four Frozen review target: base |
codegraph-librechat
Bot
added
the
🗺️ Core Infra & Sec
lia-by-librechat
Bot
changed the title
|
Independent review completed for head |
|
CI diagnosis for reviewed head |
|
Independent review completed for refreshed head |
|
Final validation for head Skipped by CI selection: |
codegraph-librechat
Bot
added
the
🗺️ File Storage
|
@codex review the latest head, final review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a070bb73c7
ℹ️ 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".
|
Head
Added owning-contract tests for erasure retries, saved-agent File Search reprovisioning, multiple secondary failures, failed persistence, and concurrent namespace/content updates. Fifteen isolated Node 24 source smoke cases passed. Focused Jest, Review target: base |
|
Head The preceding implementation head passed all API and data-schemas test shards, TypeScript, package builds, MongoDB/Redis integration, Redis-cache integration, circular checks, and Lighthouse. Those results are not claimed as current-head verification; fresh CI is pending. Local dependency-backed commands remain blocked by the absent shared install. Finding ledger: DEL-1 P1 supported and fixed in Frozen review target: repository |
|
Review head:
Passed: 8 actual-source Node 24 smoke scenarios with injected external boundaries; scoped formatting, imports, diff checks. Added real-Mongo and real-priming regressions. Local Jest, workspace typechecks, builds, lint/circular checks, and Lighthouse are dependency-blocked. Current-head CI and fresh independent review are pending. |
|
Review head:
Source checks passed for project repair/dedup, first-search failure/retry, gates, version/view, and legacy attachment ownership. Scoped formatting/import/syntax/diff checks passed. Owning real-Mongo, initialization, callback, and membership regressions were added. Full repository tests/typechecks/builds/lint/Lighthouse remain dependency-blocked locally. Fresh-head CI and independent review are pending. |
|
Head |
|
Head |
|
Independent review completed for head |
|
Final validation for head |
|
@codex review the latest head, final review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ceff39c34
ℹ️ 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".
| file: { file_id: string; embedded?: boolean }, | ||
| ) => Promise<{ deleted: true } | undefined> { | ||
| return async (req, file) => { | ||
| if (!file.embedded || !baseUrl) return undefined; |
There was a problem hiding this comment.
When an embedded file is deleted on a process where RAG_API_URL is unset, such as after a configuration rollback or on a mixed-version worker, this branch resolves undefined; createDeleteFileWithSecondaryStorage only uses { deleted: true } for reconciliation but does not reject other fulfilled results, so it proceeds to delete the primary and reports the file deleted. This permanently orphans the existing vector copy, and account erasure can then remove the user and credential retry anchors despite no vector deletion request being sent; require a confirmed receipt before deleting primary storage.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
| const hasCodeEnvRef = file.metadata?.codeEnvRef != null || file.metadata?.codeEnvRefs != null; | ||
| if (hasCodeEnvRef && source !== FileSources.execute_code) { | ||
| secondaries.push(getDeleteFile(FileSources.execute_code)); |
There was a problem hiding this comment.
When the code-environment delete succeeds but the vector reconciliation or primary delete subsequently fails, this path retains the file row with its original codeEnvRef/codeEnvRefs. checkSessionsAlive trusts references whose provisionedAt is inside its six-hour safe window without querying the remote file list, so the next turn treats the just-deleted sandbox file as alive and does not enqueue it for reprovisioning; code execution then receives a path to a missing object, and an unavailable probe can extend this indefinitely. Record the confirmed code deletion by invalidating the affected references while preserving destination intent before returning the later failure.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
| aliveFileIds: new Set(), | ||
| agentScopedFileIds: new Set(), | ||
| }; | ||
| provisionState.vectorDBFiles.push(file); |
There was a problem hiding this comment.
When a retained Chat Project file has searchDestination but no vectors, this appends it directly to vectorDBFiles after primeResources. Unlike current, deferred, and persistent agent files, these project files never pass filterFilesByEndpointRuntimeConfig, so a project created under a looser endpoint—or whose current endpoint now disables files, rejects the MIME type, or has lower size limits—will upload its primary bytes back to RAG on first search despite the current admin policy. Apply the same endpoint screening and shared size budget before adding the repair candidate.
Useful? React with 👍 / 👎.
| const result = await File.updateOne( | ||
| query, | ||
| { | ||
| $set: { embedded: false, 'metadata.searchDestination': true }, |
There was a problem hiding this comment.
During a rolling deployment, a new worker can set embedded: false and rely exclusively on the newly introduced metadata.searchDestination marker after deleting vectors, while any still-running pre-change worker recognizes only embedded and queries Chat Project files with embedded: true. Requests routed to those older workers therefore omit the retained file and cannot lazily repair it, causing saved-agent and Chat Project File Search to silently lose the file until a new worker handles a suitable turn. Deploy marker-aware readers before enabling this write, or retain a backward-compatible eligibility signal until all workers have upgraded.
AGENTS.md reference: AGENTS.md:L45-L48
Useful? React with 👍 / 👎.
| user: userId, | ||
| tenantId: tenantId != null && tenantId !== '' ? tenantId : null, | ||
| embedded: true, | ||
| $or: [{ embedded: true }, { 'metadata.searchDestination': true }], |
There was a problem hiding this comment.
The picker now evaluates metadata.searchDestination on every request, but the existing project_file_picker compound index is still partial on embedded: true, and no index covers the new marker branch. In the common tenantId: null case, satisfying that second $or branch can scan the broad population of unscoped file records and then sort by _id, even when no retained files exist, so picker latency grows with the deployment's file collection instead of the requesting user's eligible files. Add a compound or partial index that covers the marker branch, or restructure eligibility around an indexed field.
AGENTS.md reference: AGENTS.md:L59-L62
Useful? React with 👍 / 👎.
|
@codex review the latest head, final review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This branch has not been deployed
2 participants
Summary
Deleting a file removed its primary bytes before cleaning up its Code Interpreter or vector copies. A failed secondary delete retained the file record for retry, leaving conversations pointing at missing primary storage.
Keep the primary until secondary cleanup succeeds. Confirmed vector removals clear copy-presence markers while preserving search-destination intent, so a later code/primary failure leaves chat attachments, saved agent files, and Chat Project files eligible for File Search provisioning. Account erasure now stops on failed or incomplete file cleanup instead of deleting the remaining retry anchors.
Fixes #17006. Related to #15971 and #16007.
How it works
The ordering and receipt handling live in
packages/api, with clients and persistence injected. CJS supplies wiring only. The vector adapter reports confirmed deletion/404 separately from a skipped request and propagates operational failures. A conditional data-schemas write clearsembeddedandmetadata.embeddedEntitiesand atomically retainsmetadata.searchDestination, matching owner, tenant, timestamp, and namespace snapshot. The optional marker carries destination intent, not vector presence. Authorized, enabled File Search can lazily repair it; chooser-declined destinations stay declined. Ordinary metadata hydration preserves the marker without extra reads, a migration, or a new setting. Code retry references, unrelated metadata, and retention bookkeeping remain intact. Project hydration, picker eligibility, membership validation, and runtime records preserve that intent without inventing presence. Project search repairs join the existing lazy provisioning state after priming, before the first search, including a new conversation. They retain their owner namespace and storage routing; deployment/tool/role gates remain unchanged. Resource fingerprints include the intent marker, and historical attachments retain tool ownership while awaiting repair. Existing authorized batch reads are reused. Persistence failures stop before primary deletion and cannot be forgiven as storage absence.Self-, admin-, and CLI erasure all await the shared cascade before deleting the user. Its file-cleanup dependency now returns deleted/failed IDs rather than
unknown; reported failures, thrown errors, and missing success receipts abort before remaining file records and provider keys are removed.Type of change
Testing
632e4fad3ecf34c143d84d84b3194cc777be8a41: actual-source project query/projection, runtime/view/version, repair queue/dedup, disabled/role/capability gates, unavailable controls, owner-namespace first-search failure/retry, and legacy ownership checks. External DB/provider boundaries were injected; these do not replace real-Mongo/Jest verification.tsc --noEmit, tsdown builds, ESLint, and circular checks remain blocked by the absent shared dependency installation. Lighthouse stops in preparation becauserimrafis absent. No full install or lockfile change; isolated diagnostic tools were removed.632e4fad3ecf34c143d84d84b3194cc777be8a41. Fresh-head CI and independent review are pending. Earlier-head CI results do not verify this head.Screenshots / recordings
No UI changes.
Risk / compatibility
The shared path includes retention sweeps, so a failing secondary delays primary erasure too. Sweep retry bookkeeping is unchanged. Manual-delete backoff and attempt caps from #15971 remain separate. This does not repair primary copies already lost before the change. Account erasure remains an idempotent cascade, not a rollback transaction: earlier cleanup may have completed when a later storage failure causes it to stop for retry.
Checklist