* 🧹 fix: Stop Undeletable Files Starving the Retention Sweep
`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.
* 🧭 fix: Delete Code Environment Files Through the Route That Exists
`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.
* 🔒 fix: Settle sweep retry state from the write, not the read
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.
* 🧯 fix: Keep the give-up notice and the retry budget honest
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.
* 🎯 fix: Clear the retry budget only when a retention lifecycle starts
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.
* 🅿️ refactor: Park exhausted files instead of excluding them
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.
fe30e3db32