Skip to content

🪫 fix: Back Off Failed Retention Deletes - #15608

Merged
danny-avila merged 6 commits into
devfrom
danny-avila/sweep-deletion-backoff
Sep 4, 2026
Merged

danny-avila merged 6 commits into
devfrom
danny-avila/sweep-deletion-backoff

Conversation

@danny-avila

@danny-avila danny-avila commented Sep 4, 2026 •

Copy link
Copy Markdown
Collaborator

The LibreChat half of #15511. LibreChat-AI/code-interpreter#85 closed the service side and explicitly defers the rest: "the sweep's missing give-up/backoff in LibreChat's packages/api/src/files/sweep.ts — client-side, separate change."

Two commits, both from #15511's own "worth considering alongside" list.


1. Stop undeletable files starving the retention sweep

Three facts compose into a sweep that can stop making progress permanently:

getExpiredFiles returns expiredAt <= now sorted ascending, capped at limit (100)
processDeleteRequest leaves the Mongo record intact when storage deletion fails — only deletedFileIds reach db.deleteFiles
sweepExpiredFiles counted the failure and recorded nothing

So a file whose backing storage refuses deletion is refetched and retried on every hourly pass, forever. It is not only wasted work: because the batch is bounded and ordered oldest-first, limit permanently-undeletable files at the front of the queue starve every file that expires after them for the life of the deployment.

That is not hypothetical. The deployment in #15511 accumulated ~29k objects whose session ownership had lapsed. Code Interpreter #85 makes deletion work again for sessions registered after the upgrade, but those pre-upgrade sessions have no durable owner record and are denied (reason: unknown → 403). Without this change their sweep still recovers nothing — the backlog crowds out everything newer. The reporter's own measurement, "the same object paths recur roughly six times each in a 24-hour window", is the signature of a queue that never advances.

Failures are now recorded on the file, and the query respects them:

  • deletionRetryAt — the file is skipped until its backoff elapses, doubling from one sweep interval (1h) up to a day.
  • deletionAttempts — the file leaves the query entirely at FILE_RETENTION_SWEEP_MAX_ATTEMPTS (default 10, so ~5 days of retries).

Both fields are absent on existing records, and absence means "never attempted", so nothing already in the collection changes eligibility.

The record is kept, not deleted. The reference is what an operator needs to reconcile a bucket the sweep could not clear, and dropping it would restore exactly the silence that made this leak invisible for six weeks. Raising FILE_RETENTION_SWEEP_MAX_ATTEMPTS re-admits everything previously given up on — that is the supported way to resume once the storage-side failure is fixed, and the give-up log line says so.

Two details worth calling out:

  • The counter is incremented with $inc server-side, so concurrent sweeps on separate nodes (the reporting deployment is two-node) cannot overwrite each other's progress toward the cap.
  • A failure to record a failure is logged and skipped rather than aborting the batch — a Mongo blip should not cost the rest of the pass.

2. Delete through the route that exists

deleteCodeEnvFile tried /sessions/:sid/objects/:fid before falling back to /files/:sid/:fid. Worth being precise, because #15511 gets this backwards: both URLs have been there since #13424 (e3cc2a9c62, tagged v0.8.6, the same commit that introduced the function), so the missing route was never why deletion failed on released LibreChat — the fallback fired, and the 403 from the expired session key is what threw. What the first URL did cost was a guaranteed 404 and a wasted round trip on every delete.

Collapsing to /files/:sid/:fid is the safe direction: codeapi has mounted it since its first release, so it works against pre-#85 deployments as well as upgraded ones. Keeping the other path instead would not.

Removing the loop tightens two behaviours that only existed to serve it:

  • 405 now surfaces rather than being swallowed on the way to a second attempt. There is no second route to try, and a service refusing the method should say so.
  • 404 is still treated as "already gone" — the only thing it can now mean — but it is logged instead of passed over silently. A 404 from a misconfigured base URL looks identical, and this branch drops the file's metadata record either way. That is [Bug]: Code Interpreter file deletion has never worked: deleteCodeEnvFile calls a route that does not exist (404) #15511's "do not treat 404 as unconditionally terminal", which is what masked this whole class of bug.

Not in scope

SESSION_OWNER_TTL (90d service-side) must exceed the retention window. Fine for the 30d default, but MAX_RETENTION_HOURS permits 365d — a docs matter for the interpreter repo.

Verification

  • packages/data-schemas — 2768/2768, full suite. New coverage for backoff exclusion, the attempt cap (including that raising it re-admits), and deferExpiredFile's atomic increment, all against real MongoDB via mongodb-memory-server.
  • packages/api — sweep.spec.ts 10/10. Two failures elsewhere in src/files (libreoffice) reproduce on unmodified origin/dev content.
  • api — server/services/Files 373/373, including Code/crud.spec.js 31/31.
  • Every new or changed test was re-run with its fix reverted and confirmed failing (5 of 10 sweep tests, both new query tests, 6 of the crud tests).
  • tsc --noEmit clean in both packages/api and packages/data-schemas; prettier and eslint clean.

`getExpiredFiles` returns the oldest `expiredAt` first, capped at `limit`,
and `processDeleteRequest` leaves the record in place when storage deletion
fails. Nothing records the failure, so the same files come back at the head
of the next batch an hour later, forever: no backoff, no cap, and — once
`limit` of them cannot be deleted — no file that expires afterwards is ever
swept again. On the deployment behind #15511 that is ~29k stranded objects
permanently occupying a 100-slot queue, which is why fixing the Code
Interpreter side alone (LibreChat-AI/code-interpreter#85) would not have
resumed deletion there.

Failures are now recorded on the file. `deletionRetryAt` holds it back with
a backoff doubling from one sweep interval to a day, and `deletionAttempts`
retires it from the query entirely once it reaches
`FILE_RETENTION_SWEEP_MAX_ATTEMPTS` (10, so roughly five days of retries).
Both fields are absent on existing records and absence means "never
attempted", so nothing already in the collection changes eligibility.

The record itself is kept rather than deleted — the reference is what an
operator needs to reconcile a bucket the sweep could not clear, and dropping
it would restore the silence that made this leak invisible. Raising
`FILE_RETENTION_SWEEP_MAX_ATTEMPTS` re-admits everything previously given up
on, which is the supported way to resume once the storage-side failure is
fixed; the give-up is logged with that instruction.

The counter is incremented server-side with `$inc` so concurrent sweeps on
separate nodes cannot overwrite each other's progress toward the cap, and a
failure to record a failure is logged and skipped rather than aborting the
rest of the batch.
Copilot AI lite review requested due to automatic review settings September 4, 2026 16:18
`deleteCodeEnvFile` tried `/sessions/:sid/objects/:fid` before falling back
to `/files/:sid/:fid`. Both have been there since #13424, but only the
second is mounted by any released codeapi — the first gained DELETE in
LibreChat-AI/code-interpreter#85 — so every deletion paid a guaranteed 404
and a wasted round trip, and #15511 read that 404 as the whole bug.

Call `/files/:sid/:fid` directly. It is the safe direction to collapse
toward: codeapi has mounted it since its first release, so this works
against older deployments as well as post-#85 ones, whereas keeping the
other path would not.

Collapsing the loop tightens two behaviours that only existed to serve it.
A 405 now surfaces instead of being swallowed on the way to a second
attempt; there is no second route to try, and a service that refuses the
method should say so. A 404 is still treated as "already gone" — that is
the only thing it can now mean — but it is logged rather than passed over
in silence, because a 404 caused by a misconfigured base URL looks
identical and this branch drops the file's metadata record either way.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Retry backoff is documented as based on the configured sweep interval, but the current implementation hard-codes a 1-hour base delay, which can diverge from operator configuration.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes a retention-sweep starvation failure mode by persisting per-file delete failures (attempt count + retry backoff) and filtering the expired-file query to skip backed-off or max-attempt files, ensuring the hourly sweep continues making forward progress even when some backing storage objects are permanently undeletable.

Changes:

  • Add deletionAttempts / deletionRetryAt fields to the File model and schema to track consecutive deletion failures and next retry time.
  • Update getExpiredFiles to exclude files still in backoff and (optionally) files at/over the configured max attempts; add deferExpiredFile for atomic $inc + retry timestamp updates.
  • Update sweepExpiredFiles to record failures (with exponential backoff and a give-up cap) and add/adjust unit tests across packages/api, packages/data-schemas, and legacy api integration.
File summaries
File Description
packages/data-schemas/src/types/file.ts Adds typed fields for sweep failure tracking (deletionAttempts, deletionRetryAt).
packages/data-schemas/src/schema/file.ts Persists the new failure/backoff fields in the Mongo schema.
packages/data-schemas/src/methods/file.ts Extends getExpiredFiles filtering and adds deferExpiredFile atomic updater.
packages/data-schemas/src/methods/file.spec.ts Adds coverage for backoff exclusion, attempt cap behavior, and atomic increment semantics.
packages/api/src/files/sweep.ts Implements failure recording with backoff + max-attempt give-up, and wires the cap into the expired-file query.
packages/api/src/files/sweep.spec.ts Adds coverage for deferral on failure paths, give-up logging, and delay/max-attempt parsing helpers.
api/server/services/Files/process.js Wires deferExpiredFile dependency through the legacy service wrapper.
api/server/services/Files/process.spec.js Updates dependency expectations for the legacy wrapper integration test.
Review details
  • Files reviewed: 10/10 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/api/src/files/sweep.ts Outdated

const DEFAULT_FILE_RETENTION_SWEEP_INTERVAL_MS = 60 * 60 * 1000;
const DEFAULT_FILE_RETENTION_MAX_ATTEMPTS = 10;
const FILE_RETENTION_RETRY_BASE_MS = 60 * 60 * 1000;
@danny-avila

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 4, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-04T17:57:15.026151Z 27fedcb Manual request
ℹ️ 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" or "@codex security review".

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 chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0e529249f6

ℹ️ 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".

Comment thread packages/api/src/files/sweep.ts Outdated
Comment on lines +261 to +262
const attempts = (file.deletionAttempts ?? 0) + 1;
const retryAt = new Date(Date.now() + getExpiredFileRetryDelay(attempts));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Derive retry state from the atomic increment

When two application pods sweep the same file concurrently, both read the same deletionAttempts value here, calculate the same shorter backoff, and then each atomically increments the stored counter. The persisted count can therefore reach maxAttempts while both callers still believe it is below the cap, so the file is silently excluded without the intended give-up log, and earlier retries are scheduled using a stale attempt count. Have deferExpiredFile atomically compute and return the resulting attempt count/retry timestamp, or claim each record before deletion, rather than deriving retry state from the queried snapshot.

Useful? React with 👍 / 👎.

Two findings from the review of 0e52924.

- `recordFailure` derived the attempt number by adding one to the count the
  batch had queried. Two nodes sweeping the same file read the same value,
  so both believed themselves to be the same attempt: each `$inc` landed,
  the stored count crossed `FILE_RETENTION_SWEEP_MAX_ATTEMPTS`, and neither
  caller ever saw the threshold. The file drops out of the query — the
  starvation guard still holds — but the give-up is never reported, and
  that log line is the operator's only notice, and carries the instruction
  for resuming. Return the count from the increment itself so every caller
  gets a distinct attempt number and exactly one observes the cap.

  The same staleness shortened the backoff, so `deferExpiredFile` now
  writes with `$max`: a deferral can only move later, and a node that
  computed a shorter delay cannot pull the file forward past one another
  node already committed.

- The backoff doubled from a hard-coded hour while claiming to start from
  one sweep interval. At the default they coincide; away from it the
  schedule stops meaning anything — on a six-hour sweep the first three
  attempts all land on consecutive passes, and on a five-minute one the
  first retry skips twelve. Derive the base from
  `FILE_RETENTION_SWEEP_INTERVAL_MS`, floored at a minute so a
  pathologically short interval cannot spend the whole give-up budget on a
  transient outage.
@danny-avila

Copy link
Copy Markdown
Collaborator Author

Both findings from the first round are addressed in ee1bac5.

P2 — retry state derived from the queried snapshot. Correct, and the consequence is narrower but sharper than "earlier retries": the file still leaves the query once the stored count crosses the cap, so starvation stays fixed — what is lost is the give-up log, which is the operator's only notice and carries the instruction for resuming. incrementFileDeletionAttempts now returns the count from the increment itself, so concurrent sweeps get distinct attempt numbers and exactly one observes the threshold. The same staleness shortened the backoff, so deferExpiredFile writes with $max — a deferral can only move later, and a node that computed a shorter delay cannot pull the file forward past one another node already committed.

Backoff base vs. sweep interval. Taken. At the default the two coincide, but away from it the schedule stops meaning anything: on a six-hour sweep the first three attempts all land on consecutive passes, and on a five-minute one the first retry skips twelve. The base now derives from FILE_RETENTION_SWEEP_INTERVAL_MS, floored at a minute so a pathologically short interval cannot spend the whole give-up budget on a transient outage.

New coverage: concurrent increments returning [1, 2, 3], $max refusing to pull a deferral earlier, and the interval-derived base plus its floor. Each was re-run with its fix reverted and confirmed failing.

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ee1bac5e16

ℹ️ 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".

Comment thread packages/api/src/files/sweep.ts Outdated
Comment on lines +284 to +288
attempts = await incrementFileDeletionAttempts(file.file_id);
await deferExpiredFile(
file.file_id,
new Date(Date.now() + getExpiredFileRetryDelay(attempts)),
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Report give-up before the fallible deferral write

The new two-write sequence still loses the give-up notice if incrementFileDeletionAttempts succeeds at the cap but deferExpiredFile then fails (for example, during a transient MongoDB interruption). The catch returns before the threshold check, while the persisted counter now causes getExpiredFiles to exclude the file permanently, leaving only the generic recording-error log and omitting the reconciliation/resume instruction. Check and report the threshold immediately after the increment, or make the counter and deferral update atomic.

Useful? React with 👍 / 👎.

Comment thread packages/api/src/files/sweep.ts Outdated
attempts = await incrementFileDeletionAttempts(file.file_id);
await deferExpiredFile(
file.file_id,
new Date(Date.now() + getExpiredFileRetryDelay(attempts)),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Anchor retry deadlines to the sweep start

Because startExpiredFileSweep anchors its interval before the asynchronous sweep finishes, computing this deadline from Date.now() after deletion I/O means a one-interval delay expires just after the next scheduled pass. For example, an hourly sweep whose failed request takes ten seconds writes retryAt = start + 1h10s, so the pass at start + 1h skips it and the first retry occurs around two hours later; every delay that is an exact interval multiple has the same extra-pass drift. Capture the sweep start time and derive deadlines from that anchor so the documented first retry actually lands on the following sweep.

Useful? React with 👍 / 👎.

Comment thread packages/api/src/files/sweep.ts Outdated
Comment on lines +297 to +301
if (attempts < maxAttempts) {
return;
}
logger.error(
`[sweepExpiredFiles] Giving up on expired file ${file.file_id} after ${attempts} failed deletions. Its backing storage was not removed; the record is kept so the reference survives for reconciliation. Raise FILE_RETENTION_SWEEP_MAX_ATTEMPTS to retry it.`,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 Badge Log only the worker that reaches the retry cap

Fresh evidence in the revised concurrency path is that when two workers fetched a file at maxAttempts - 1, their atomic increments return maxAttempts and maxAttempts + 1; this comparison lets both through, so both emit the give-up error despite the stated exactly-once threshold notice. With multiple replicas and many exhausted files this produces one error per replica/file rather than one actionable notification; gate the notice on attempts === maxAttempts instead.

Useful? React with 👍 / 👎.

Comment on lines +155 to +159
deletionAttempts: {
/* Consecutive failed sweep deletions. Files at or past
* `FILE_RETENTION_SWEEP_MAX_ATTEMPTS` are excluded from the sweep
* query so a permanently undeletable file cannot starve the bounded
* batch; raising that limit re-admits them. */

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reset deletion retry state when reusing a file record

When processCodeOutput reuses an existing (filename, conversationId) record, both its createFile upsert and updateFile path set only the supplied fields, so these new retry fields survive even when the backing bytes, storage reference, and retention deadline are replaced. A record that previously reached the cap can therefore receive a fresh output and future expiredAt yet remain permanently excluded when that new deadline arrives; a partially failed record similarly starts the new object's retry budget partway spent. Clear deletionAttempts and deletionRetryAt whenever a file record is repurposed with new content or a new retention lifecycle.

Useful? React with 👍 / 👎.

@danny-avila danny-avila changed the title 🧹 fix: Stop Undeletable Files Starving the Retention Sweep 🪫 fix: Back Off Failed Retention Deletes Sep 4, 2026
Four findings from the review of ee1bac5.

- The give-up was reported after the deferral write, inside the same catch.
  Once the increment lands the counter is durable, so `getExpiredFiles`
  already excludes the file; a deferral that then failed left only the
  generic recording error and dropped the one line naming the file and
  saying how to resume. Report it as soon as the increment returns, and let
  the deferral fail on its own.

- Retry deadlines were measured from `Date.now()` after the deletion I/O,
  but `startExpiredFileSweep` arms its interval before the sweep runs. A
  one-interval delay therefore expired just *after* the next scheduled
  pass, which skipped the file and pushed its first retry out by a whole
  extra interval — and the same drift applied to every delay that is an
  exact multiple. Anchor deadlines to the sweep's start instead.

- The threshold used `>=`, so once two nodes pushed a file past the cap
  every one of them past it logged the give-up: one error per replica per
  exhausted file rather than one actionable notice. Only the attempt that
  lands exactly on the cap reports it.

- Retry state outlived the content it described. `processCodeOutput` reuses
  a record for a repeated `(filename, conversationId)` — new bytes, new
  storage key, and a fresh `expiredAt` from `getRetentionExpiry` — while
  `createFile` and `updateFile` set only supplied fields. A record carried
  to the cap by its previous content stayed excluded from the sweep
  forever, stranding the new object exactly as this PR set out to prevent;
  a partially failed one started the new object's budget already spent.
  Both write paths now clear the fields: the budget belongs to the storage
  a record currently points at.

The retry-state fixtures in `file.spec.ts` were seeded through `createFile`,
which now clears them, so they drive the real methods the sweep uses.
@danny-avila

Copy link
Copy Markdown
Collaborator Author

All four round-2 findings are addressed in 8d26f9b. Each was verified against the code before acting, and each fix was re-run with itself reverted to confirm the new test catches it.

P2 — give-up reported after the fallible deferral. Right, and it undoes the round-1 fix in the one case that matters: once the increment lands the counter is durable, so getExpiredFiles already excludes the file. A failing deferral left only the generic recording error and dropped the line naming the file and saying how to resume. The threshold is now checked as soon as the increment returns; the deferral fails independently with its own message.

P2 — deadlines anchored after the I/O. Confirmed: startExpiredFileSweep arms setInterval before the sweep runs, so a one-interval delay measured post-I/O expires just after the next scheduled pass and the file waits a whole extra interval. Deadlines now come off the sweep's start.

P3 — >= lets every worker past the cap log. Correct, and my round-1 comment claiming "exactly one observes the cap" was wrong for precisely this reason. Now === maxAttempts, so only the attempt that lands on the cap reports it.

P2 — retry state surviving record reuse. The sharpest of the four, and it reintroduced this PR's own bug through a path I had not considered. Verified end to end: claimCodeFile reuses the record for a repeated (filename, conversationId), baseFile spreads a fresh expiredAt from getRetentionExpiry(req) on that path, and createFile/updateFile set only supplied fields — so a record carried to the cap by its previous content stayed excluded forever while pointing at a new object. Both write paths now clear the fields: the budget belongs to the storage a record currently points at, so a write that redescribes the content restarts it. updateFileUsage is a separate method and unaffected, so the cap still holds for a stable file.

That last one also invalidated two existing fixtures, which had been seeding the retry fields through createFile. They now drive the same methods the sweep itself uses, and there is a new test that a repurposed record becomes sweepable again.

packages/data-schemas 2771/2771, api/server/services/Files 373/373, sweep.spec.ts 12/12; tsc, prettier and eslint clean.

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8d26f9ba8e

ℹ️ 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 updateOperation = {
$set: update,
$unset: { expiresAt: '' },
$unset: { expiresAt: '', ...CLEARED_DELETION_RETRY_STATE },

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reset retry state only for storage replacement

When an exhausted file receives a metadata-only update, this unconditional $unset erases its retry cap even though the backing object has not changed. For example, prepareImagesLocal calls updateFile({ file_id }) whenever an existing image is encoded for another chat, and the preview routes use the same method merely to transition status; either path makes an expired, undeletable record immediately eligible for another full set of deletion attempts and another give-up notice. Only clear these fields on writes that actually install new content or start a new retention lifecycle.

Useful? React with 👍 / 👎.

Comment on lines +195 to +198
const file = await File.findOneAndUpdate(
{ file_id },
{ $inc: { deletionAttempts: 1 } },
{ new: true, projection: { deletionAttempts: 1 } },

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep retry bookkeeping from changing updatedAt

On the timestamped File schema, this update and the following deferExpiredFile update automatically advance updatedAt, even though they only record sweep bookkeeping. If a background code-output harvest for a legacy record starts before a failed sweep and settles afterward, processCodeOutput falls back to updatedAt as its writer-order stamp and therefore treats the sweep update as a newer content writer, skipping the generated attachment. Pass timestamps: false to both retry-state writes, as is already done for other bookkeeping-only file updates.

Useful? React with 👍 / 👎.

Both findings from the review of 8d26f9b, and both are consequences of that
commit's reset rather than of the original change.

- The reset was unconditional on every `createFile`/`updateFile`, on the
  reasoning that those paths write content. Two of them do not.
  `prepareImages{Local,Azure,Firebase}` call `updateFile({ file_id })` with
  nothing but the id — a TTL touch — every time an existing image is
  encoded for another chat, and the deferred preview uses the same method
  to transition `status`. Either handed a stranded record a fresh set of
  attempts and another give-up notice, on repeat, defeating the cap for the
  files most likely to be stranded.

  Gate it on the write actually setting `expiredAt`. The budget belongs to
  a retention lifecycle: that is the write which starts a new one, it is
  what `processCodeOutput` supplies when it repurposes a record, and a
  record with no retention deadline is never swept, so the fields are inert
  there anyway.

- Both retry writes bumped `updatedAt`. `processCodeOutput` falls back to
  `updatedAt` as the writer-order stamp for records that predate
  `metadata.sourceDispatchedAt`, so a failed sweep landing mid-harvest read
  as a newer content writer and the harvest dropped its attachment. Mark
  them `timestamps: false`, for the reason `claimCodeFile` already does:
  bookkeeping is not a content write.
@danny-avila

Copy link
Copy Markdown
Collaborator Author

Both addressed in cda8ee9. Both are consequences of the previous commit's reset rather than of the original change, so this round is narrowing my own fix.

P2 — reset too broad. Correct, and my reasoning last round was wrong: I assumed updateFile's callers all write content. Verified they do not — prepareImages{Local,Azure,Firebase} call updateFile({ file_id }) with nothing but the id, a TTL touch, every time an existing image is encoded for another chat, and the deferred preview uses the same method to transition status. Either handed a stranded record a fresh set of attempts and another give-up notice, repeatedly, defeating the cap for exactly the files most likely to be stranded.

The reset is now gated on the write setting expiredAt. That is the right seam rather than a narrower content check: the budget belongs to a retention lifecycle, expiredAt is the write that starts a new one, it is what processCodeOutput supplies when it repurposes a record, and a record without a retention deadline is never swept — so the fields are inert there regardless.

P2 — bookkeeping bumping updatedAt. Confirmed, and the codebase already knew: claimCodeFile passes timestamps: false with a comment about this exact guard. A failed sweep landing mid-harvest made the row look like a newer content writer to processCodeOutput's fallback stamp, and the harvest dropped its attachment. Both retry writes are now timestamps: false.

New tests: a touch-only updateFile leaves an exhausted record excluded, and the retry writes leave updatedAt untouched. Both fail with their fix reverted.

packages/data-schemas 2773/2773, api/server/services/Files 373/373; tsc, prettier, eslint clean.

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cda8ee9551

ℹ️ 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".

Comment on lines +470 to +471
if (startsRetentionLifecycle(data)) {
update.$unset = DELETION_RETRY_STATE;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reset retry state when replacement bytes inherit a deadline

Fresh evidence against narrowing the reset solely to writes containing expiredAt is that getRetentionExpiry returns {} when its conversation lookup fails, while processCodeOutput still commits the replacement bytes. Because createFile and updateFile use $set, omitting expiredAt preserves the row's existing retention deadline; this condition also preserves an exhausted deletionAttempts, so a reused code-output row can point to new storage while remaining permanently excluded from the sweep. Treat storage replacement as a reset trigger too, or explicitly remove the inherited deadline when no retention lifecycle applies.

Useful? React with 👍 / 👎.

*/
async function deferExpiredFile(file_id: string, deletionRetryAt: Date): Promise<void> {
const File = mongoose.models.File as Model<IMongoFile>;
await File.updateOne({ file_id }, { $max: { deletionRetryAt } }, { timestamps: false }).exec();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Bind retry writes to the retention lifecycle that failed

When a code-output row is reused while a sweep is handling its previous contents, both retry writes match only file_id. If the replacement resets the retry fields between the increment and this deferral, this stale write restores the old lifecycle's deletionRetryAt; at later attempts that deadline can fall hours or a day after the new object's expiredAt, delaying its required deletion. If replacement happens before the increment, the old failure consumes the new lifecycle's budget as well. Carry the queried lifecycle identifier/deadline into a conditional update, or record the increment and deferral atomically against that lifecycle.

Useful? React with 👍 / 👎.

Three review rounds in a row found defects in how the give-up cap interacts
with record reuse, each in the fix for the last. That is a design error, not
a bug list: a permanent exclusion has to be bound precisely to the content
lifecycle it was recorded against, and File records outlive their content.
`processCodeOutput` repurposes a row for a repeated
`(filename, conversationId)`, `createFile`/`updateFile` set only supplied
fields, and `getRetentionExpiry` returns `{}` on a lookup failure so the row
inherits its old deadline — three separate ways for bookkeeping to survive
into a lifecycle it does not describe, each needing its own guard, and the
two retry writes needing to be lifecycle-conditional on top.

Remove the category instead. `deletionRetryAt` becomes the sweep's only
hold, and reaching `FILE_RETENTION_SWEEP_MAX_ATTEMPTS` parks the file for a
month rather than excluding it. The bound on the batch is the same — a
stranded file costs one slot a month instead of one an hour — but a deadline
that outlives its content can only delay the next object, never lose it, so
nothing outside the sweep has to reason about this state at all.

That deletes more than it adds:

- `createFile` and `updateFile` go back to their original form. No reset, so
  no question of which writes install content, and `prepareImages*` and the
  deferred preview stop mattering here.
- `getExpiredFiles` loses `maxAttempts` and its `$and`; eligibility is one
  `$or` on the deadline.
- The interleaving race between the increment and the deferral degrades from
  a stranded object to a delayed one.

Working through a large backlog is throughput-bound either way: every
attempt costs a slot in the bounded batch, so N stranded files need N ×
`FILE_RETENTION_SWEEP_MAX_ATTEMPTS` passes to settle. Lower that limit when
recovering a deployment that has accumulated many.
@danny-avila

Copy link
Copy Markdown
Collaborator Author

Both findings verified, and rather than guard them individually I've changed the design in 27fedcb.

The first is real and my last round's reasoning was wrong. I justified gating the reset on expiredAt partly on "a record with no retention deadline is never swept, so the fields are inert" — which misses the inherited case exactly as you describe. Confirmed in computeRetentionExpiry: it returns {} both when the conversation lookup throws and when the conversation has no expiration, while processCodeOutput commits the replacement bytes regardless, so the row keeps its old deadline and its exhausted budget.

The second is real too, though narrower — a reuse interleaved between the increment and the deferral.

What matters more than either: this is the third consecutive round finding a defect in the previous round's fix, all on the same axis. That is a design error rather than a bug list. A permanent exclusion has to be bound precisely to the content lifecycle it was recorded against, and File records outlive their content — the reuse path, the $set semantics, and now the inherited deadline are three separate ways for state to survive into a lifecycle it does not describe, each wanting its own guard, with lifecycle-conditional writes on top.

So the exclusion is gone. deletionRetryAt is now the sweep's only hold, and reaching the cap parks the file for a month instead of excluding it. The bound on the batch is unchanged — a stranded file costs one slot a month rather than one an hour — but a deadline that outlives its content can only delay the next object, never lose it, so nothing outside the sweep needs to reason about this state.

It removes more than it adds: createFile and updateFile return to their original form (no reset, so no question of which writes install content), getExpiredFiles loses maxAttempts and its $and for a single $or on the deadline, and your second finding degrades from a stranded object to a delayed one. Net −36 lines.

One property worth stating plainly: working through a large backlog is throughput-bound either way, since every attempt costs a slot in the bounded batch. N stranded files need N × FILE_RETENTION_SWEEP_MAX_ATTEMPTS passes to settle, so that limit is the lever when recovering a deployment that has accumulated many. That was equally true of the excluding version; it is just now documented.

packages/data-schemas 2772/2772, api/server/services/Files 373/373, sweep.spec.ts 12/12; tsc, prettier, eslint clean.

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit: 27fedcb9a6

ℹ️ 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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants