fix(uploads): treat a missing storage object as absent metadata, not a failure - #6378

Summary

Root cause of the chronic FilesServeAPI / FileAuthorization ERROR noise — 1,000–2,000 lines/day for at least 14 days, peaking at 2,105 on 07-29.

A workspace file is rewritten under a new key on every content update, and the superseded object is deleted immediately (cleanupWorkspaceStorageObject(oldKey, 'version replacement'), from #5991). Any reader still holding the previous key therefore finds nothing — which is an ordinary outcome, not a failure.

getFileMetadata let that not-found propagate out of all three provider branches. verifyWorkspaceFileAccess caught it in its generic handler and logged ERROR, so the branch already written for exactly this case — logger.warn('Workspace file missing authorization metadata') at authorization.ts:264 — was unreachable.

Changes

• getFileMetadata returns {} when the object is absent — the same value it already returns when no provider is configured, so no contract or caller changes
• Genuine failures (permission, network, provider 5xx) still propagate and still log ERROR
• Collapsed the three divergent per-provider not-found predicates (S3 NotFound/NoSuchKey, Azure BlobNotFound, GCS numeric 404) onto one shared isObjectNotFoundError, rather than adding a fourth copy

Verified against the AWS SDK v3 source: HeadObjectCommand throws NotFound and GetObjectCommand throws NoSuchKey, both __BaseException with $fault: 'client' and $metadata.httpStatusCode: 404 — matching the production payload exactly.

Not in scope

This corrects how absence is modelled. The underlying design issue — URLs addressing a mutable storage key rather than the stable file id — is a separate change; a stale key legitimately 404s.

Type of Change

• Bug fix

Testing

8 new tests. reports an absent object as no metadata rather than throwing verified red before the fix and green after. Full lib/uploads suite: 374 passing across 31 files.

Checklist

• Code follows project style guidelines
• Self-reviewed my changes
• Tests added/updated and passing
• No new warnings introduced
• I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

Audit of every changed line

A defect in this PR's own predicate was found and fixed (a5b3…). The first version read the label as typeof name === 'string' ? name : code, so a non-matching name short-circuited the code check entirely. Azure raises a RestError whose name carries the class and whose code carries the reason, so BlobNotFound was missed — narrower than the per-provider check it replaced. name and code are now tested independently, with a regression test verified red before / green after.

All three callers audited for fail-closed behavior. getFileMetadata has exactly 3 callers, all in authorization.ts:

The third is a genuine behavior change and is called out deliberately: an absent copilot object previously threw and denied; it now takes the same path as an object that exists without metadata. That path grants only when cloudKey.split('/')[0] === userId — strictly the caller's own namespace — so no cross-user access is reachable. Downstream, a granted read of an absent object 404s at storage and a granted delete is a no-op. No exposure either way.

Other checks: no import cycle (core/errors.ts imports nothing; providers are loaded dynamically from core). Provider files changed by import position only — verified by diffing after biome --write --unsafe. Full lib/uploads suite 375 passing across 31 files. Typecheck clean; the sole error (heic-convert) reproduces on untouched origin/main.

Second audit round — container-level 404s

A high-effort review pass caught a defect the first audit missed: NoSuchBucket and ContainerNotFound also answer 404, so the status-only clause read them as an absent object. A deleted or misconfigured bucket would have degraded into silent "no metadata" — every file read failing closed, with the ERROR that used to alert on it now downgraded to the routine stale-key warn. A total storage outage would have been indistinguishable from ordinary noise.

Container-level labels are now excluded before the object-level check, verified red before / green after:

{ name: 'NoSuchBucket', $metadata: { httpStatusCode: 404 } }        → false
{ name: 'RestError', code: 'ContainerNotFound', statusCode: 404 }   → false

Known limitation, stated rather than implied solved: GCS reports both object- and bucket-level misses as a bare numeric code: 404 with no distinguishing label, so the two are not separable there. S3 is the production provider and does distinguish them.

app/api/tools/s3/head-object/route.ts:86 still hand-rolls its own check and was deliberately not migrated — it is a user-facing tool against user-supplied credentials that reports exists: false, and adopting the shared predicate would turn a nonexistent bucket into an error rather than a negative result. That is a user-visible semantics change and does not belong in this PR.

Suites after the fix: lib/uploads 376 passing / 31 files, app/api/files 238 passing / 16 files.

Third audit round — the abstraction itself

The earlier rounds kept finding defects in one place: the shared not-found predicate. That was the symptom. The cause was that getFileMetadata re-implemented the S3 and Blob HEAD calls inline, duplicating headS3Object and headBlobObject — including Azure's entire connection-string / shared-key branch, verbatim. Because it owned those calls, it had to interpret provider errors itself, and doing that safely across providers forced a second, stricter predicate.

Deleting the duplication removes the whole problem:

• getFileMetadata now calls headS3Object / headBlobObject, which already report absence as null, and maps that to {}
• its try/catch is gone — it no longer inspects errors at all
• the second predicate (hasObjectNotFoundLabel) is gone with it; one predicate again, used only by provider clients that know they just performed an object-level operation
• GCS still raises, exactly as before this PR

Each provider now owns its own not-found semantics and the dispatcher just maps null → {}.

Test coverage gap this exposed

Reverting the S3 not-found handling broke no test — storage-client.test.ts mocks headS3Object, so the seam hid whether the two layers agree. Three tests were added to the S3 client's own suite exercising the real headS3Object over a mocked SDK: absent object → null, NoSuchBucket → raises, AccessDenied → raises. Reverting the handling now fails reports an absent object as null rather than raising, as it should.

An earlier attempt at this used vi.resetModules() + vi.doMock() + await import(); that is banned by CLAUDE.md's testing rules and did not work regardless (the file-level mock shadows the real module). Replaced with vi.hoisted() + vi.mock() + static imports, matching the file it lives in.

Final: lib/uploads 380 passing / 31 files, app/api/files 238 passing / 16 files, biome clean on all 8 touched files, typecheck clean apart from the pre-existing heic-convert.

Completing the fix: the other two thirds

The change above removes one of the three ERROR lines a stale key produces. Production over 24h shows they fire in near-lockstep, because all three come from the same request:

Once authorization correctly denies, the route throws FileNotFoundError; the inner handler logs it at error and rethrows, and the outer handler logs it at error again. So an ordinary 404 was reported twice as a server fault — and route.ts made that plain:

logger.error('Error serving file:', error)   // logged as a failure first
if (error instanceof FileNotFoundError) {
  return createErrorResponse(error)          // then handled correctly as a 404
}

The precedent for the right behaviour sits a few lines above: DocCompileUserError is already logged at info with a comment noting it is "not a server fault". FileNotFoundError now gets the same treatment, via one logServeFailure helper shared by all five catch sites in the file — error is reserved for failures that really are the server's.

Nothing is silenced: the 404 is still recorded, still carries its reason, and every non-FileNotFoundError still logs at error. Two tests cover both directions, verified red before / green after.

With this, the module's ~823 ERROR lines/day drop to ~0 for the stale-key case, rather than the ~549 that would have remained.

Suites: app/api/files 240 passing / 16 files, lib/uploads 380 passing / 31 files.

Per-provider verification

All three cloud providers now assert absence and non-404 rethrow, each mutation-verified: removing the guard fails exactly one test in S3 and one in Blob. Blob had no not-found coverage at all before this, and it is the one provider that puts the reason in code rather than name, so the container-level exclusion was the least-verified path in the change.

headS3Object / headBlobObject are also reached through headObject, which serves the TikTok upload tool, workspace forking (2 sites), and workspace-file-manager (2 sites). Raising on a missing bucket brings those call sites into line with headObject's documented contract — "Returns … null when missing. Throws on errors other than 'not found'" — which returning null for a missing bucket had violated, sending callers into a doomed path instead of failing fast.

Two narrow behaviour changes, neither reachable from current callers: a containerName-only custom config is no longer honoured in the Blob branch (every call site passes undefined), and abortMultipartUpload now emits a warn on a missing container where it was previously silent.