Skip to content

fix(platform): make BYO storage switches and blob backfill trustworthy - #3239

Merged
larryro merged 14 commits into
mainfrom
fix/files-object-storage-integrity
Sep 6, 2026
Merged

fix(platform): make BYO storage switches and blob backfill trustworthy#3239
larryro merged 14 commits into
mainfrom
fix/files-object-storage-integrity

Conversation

@larryro

@larryro larryro commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

Saving a BYO object-storage connection made every blob the org had stored before that moment unreadable, and the blob backfill that was supposed to move them copied without a content type, without verification, without deleting the source, over only two of the four ref-holding tables, and without a status fence. This PR makes the switch and the move trustworthy:

  • Mixed-store reads. resolveOrgObjectStoresForRead lists own-then-default (only when the default tree names a different physical bucket); locateOrgObjectStore / locateOrgObject HEAD them in turn (no round-trip for a single-store org; the WebDAV GET reuses that HEAD instead of issuing a second one); deleteOrgObject clears every store that may hold a key. Every serve/read/delete lane (files, sandbox stage, TTS, project text, knowledge, WebDAV, erasure, blob_access) and every reclaim lane (abandoned upload intents, replacement-upload cleanup) now goes through them; mint lanes keep resolving the org's current store.
  • A verified move. handleRef HEADs source and target, GETs the source with its stored Content-Type, PUTs it as-is, HEADs the copy to verify the size (a short copy is deleted and counted failed), and only then deletes the source. A target copy finishes a cut-off move only when size AND stored content type agree with the source (a source with no type compares on size); any other target copy — including the application/octet-stream copies the pre-move engine left behind — is re-copied over and the source retired after.
  • Same-store refusal, proven not inferred. sameObjectStore compares an endpoint identity (URL-parsed, host case-folded, trailing slash dropped; the same bucket with no endpoint is the same AWS bucket whatever region string names it). On top of that, every run — dry runs too — probes physical identity with sharesPhysicalStore: a marker written through the target under a fresh org-namespaced key must stay invisible through the source; the marker is deleted again on every path. Either check failing the run as SAME_STORE before any source delete.
  • Every ref table. Two more phases walk app.tts_audio_chunks and app.video_link_jobs; migration 0078 widens the phase CHECK (renumbered from 0077 — re-check at merge time, 0077 belongs to fix(platform): close conversation send, dedupe and scoping gaps #3225).
  • A fenced writer. Every stamp and the completion/failure UPDATE are fenced on status = 'running' with RETURNING; nothing matched stops the engine without touching the terminal row; a stamp is due at least every 60 s inside a batch so the watchdog measures real liveness.
  • POST /files/upload reclaims the blob it just minted when the intent write fails; the lazy sweep inside recordUploadIntent never fails the mint.
  • The unused /files/upload-handoff door, createUploadHandoff, and the dead file_utils exports are gone; the two empty .catch(() => {}) in audio_preprocess.ts now warn.
  • Docs (en/de/fr data-residency) describe the move the engine performs.

Findings fixed

  • files-1 — read/serve/delete lanes locate a pre-switch blob in the default store after a BYO switch (locateOrgObjectStore / locateOrgObject / deleteOrgObject).
  • files-2 — backfill copies with the stored content type and verifies the landed size before counting a blob migrated; an existing same-size octet-stream copy is re-copied, not trusted.
  • files-v1 — backfill deletes the source after verification; a re-run finishes a move cut off between PUT and delete; source and target that are one physical bucket (by config OR by probe) refuse with SAME_STORE.
  • files-3 — backfill walks tts_audio_chunks and video_link_jobs too (migration 0078 widens the phase CHECK).
  • files-9 — every progress/terminal UPDATE is fenced on status = 'running'; a fenced engine stops; time-based stamps inside a batch.
  • files-8 — /files/upload reclaims the minted blob when recordUploadIntent fails; the inline sweep is guarded.
  • files-4 — /files/upload-handoff + createUploadHandoff + schema removed; integration-check journeys mint through /blob-upload.
  • files-5 — MAX_FILE_SIZE_BYTES + parseObjectStorageSecretsJson removed.
  • files-6 — chunk-directory cleanup failures in audio_preprocess.ts are logged.

Review round 1 (adversarial) — addressed

  • Blocking (a) — aliased buckets pass the SAME_STORE guard and the move deletes the only copy. sameObjectStore normalises the endpoint (endpointIdentity), and runBackfill proves physical identity with sharesPhysicalStore before anything else happens (dry runs too, so a preview cannot say "nothing to move" for a run that would delete everything). Tests: same bucket spelled with a trailing slash / other host case (config), a DNS alias the config cannot see (probe, dry and real), the marker gone on every path, the failure UPDATE fenced on status = 'running', the seeded source object surviving. Commit 84a801555.
  • Blocking (b) — the resume branch retired the typed source behind an octet-stream copy. s3HeadObject returns { size, contentType } (S3ObjectHead); handleRef finishes a cut-off move only when size AND type agree, else re-copies. Tests: application/pdf source beside a same-size application/octet-stream target → target re-copied as application/pdf, source deleted, migrated=1 not skipped=1; a typeless source resumes on size alone. Commit 84a801555.
  • Non-blocking — sweepUploadIntents and runReplacementCleanup delete through deleteOrgObject (every store that may hold the key) instead of the mint store (5e74b6f28); getWebdavBlobUrl reuses the HEAD locateOrgObject already issued (57cac5339); stampIfDue has a fake-timer test that advances past STAMP_INTERVAL_MS inside one batch; the SAME_STORE tests assert the failure UPDATE's AND status = 'running' fence.
  • Non-blocking, not taken — dry run still counts a target copy that already matches as skipped: an honest "pending source delete" number needs a new counter column + contract + UI (a migration for a status-line nuance), and counting it as a 0-byte candidate would make the dry run's candidates disagree with the real run's migrated. The docs describe the real run as deleting the source after verification, which covers the matching copies too.
  • Commit headers: the three headers over 72 chars are in already-pushed history; the PR title (70 chars) is what the squash-merge keeps.

Skipped

  • none

Tests & gates observed (final tree, 57cac5339)

  • bunx tsc --noEmit (services/platform): TSC_OK.
  • bunx oxlint --type-aware (services/platform): exit 0, no findings.
  • bunx vitest --run --project server: Test Files 521 passed (521) / Tests 6140 passed (6140).
  • bun run check (worktree root): Tasks: 38 successful, 40 total — the one red task is @tale/platform#test with exactly one failure, lib/engine/core/execute/execute.test.ts > subautomation > resolves name@version explicitly (Test Files 1 failed | 528 passed (529) / Tests 1 failed | 73378 passed (73379)); this branch touches nothing under lib/engine, and the file passes 3/3 reruns in isolation (Tests 29 passed (29)). turbo listed Failed: @tale/platform#test as the only failed task (38 successful of 40; the 40th is the task downstream of it).
  • bun run --filter @tale/platform test:ui alone (run while the integration proof was also running): Test Files 1 failed | 455 passed (456) / Tests 1 failed | 3511 passed (3512), the one failure a Timed out in waitFor in settings-rail.test.tsx (untouched); rerun of that file alone: Test Files 1 passed (1) / Tests 2 passed (2). The branch's only app-side change is the BackfillPhase union in app/lib/backend/contract/object-storage.ts.
  • bun run knip:check: exit 0 (one pre-existing configuration hint, cron-parser in services/platform/knip.config.ts, untouched).
  • Real-Postgres integration proof (run-itest.sh, branch contains fix(platform): pass the corpus scope rows as one jsonb parameter #3222 so no cherry-pick): first run (alongside test:ui): RUN TRUNCATED at checkTasksCollabIntegrity — 121/134 lanes ran on a Postgres 40001 serialization failure in audit_logs lockChainHead (tasks/collab lane, 426/428 before it). Rerun alone on a quiet machine: [itest] 474/476 checks passed across 134/134 lanes, no truncation. This theme's lane both times: PASS data residency: object storage connection + blob backfill to BYO — … dry=completed/69c/0m, real=completed/39m/208253B, landed=true, sourceRetired=true, preMoved=true, servedBeforeMove=true, servedAfterMove=true, type=text/markdown→text/markdown (want kept). The two reds are outside this theme: webdav re-home (red on main before the campaign) and messages: concurrent appends each take their own slot — appends=12, failed=1 (want 0) (conversations lane, fix(platform): close conversation send, dedupe and scoping gaps #3225; it PASSED on this same tree in the first run — a concurrency flake, flagged to the coordinator).

Notes for the reviewer

  • Migration renumbered 0077 → 0078 (0078_object_storage_backfill_phases.sql), assigned by the campaign coordinator; 0077 is fix(platform): close conversation send, dedupe and scoping gaps #3225's (merged). Please re-check the number at merge time.
  • Reads for an org with its own bucket cost one HEAD per served blob until the backfill has moved it (and one HEAD against the own store after). Single-store orgs pay nothing. A completed non-dry backfill short-circuiting the default-store fallback is a possible follow-up, not in this PR.
  • The backfill now DELETES the source after a size+type-verified copy, and refuses (SAME_STORE) when source and target are one physical bucket by config or by probe. The probe writes and deletes one marker object under <prefix>/<org>/<uuid> in the target on every run, dry runs included.
  • The webdav re-home integration lane is red on main before this campaign (known, not this theme).
  • Branch merged with origin/main (f02b00412, no conflicts); git merge-tree --write-tree origin/main HEAD is CLEAN at 3997f6a2a.

…YO switch

Saving an org's own object-storage connection made every blob the org
had stored before that moment unreadable: `resolveOrgObjectStore` is
`own ?? default`, every serve/read/delete lane used it, and a blob ref
carries no store identity — so presigned GETs, the sandbox stage, TTS
audio, knowledge (re)indexing and WebDAV reads all 404ed until the admin
discovered and finished the blob backfill, while the docs, the settings
UI and the service header all promised mixed references.

`resolveOrgObjectStoresForRead` lists the org's store and — only when
the org has its own bucket and the default tree names a different one —
the deployment default behind it; `locateOrgObjectStore` HEADs them in
turn (no round-trip for a single-store org) and `deleteOrgObject`
clears every store that may hold a key, so erasure/retention deletes no
longer leave a default-store copy behind. Mint lanes keep resolving the
org's current store. `s3GetObject` returns the stored Content-Type with
the bytes for the backfill's copy.

Finding: files-1.
…table

The backfill copied every object as application/octet-stream, counted a
copy as migrated without checking it landed, never deleted the source
(so a residency tenant's history stayed in the shared default store, and
later deletes missed that copy), walked only documents + file_metadata
(TTS audio and video-link blobs were left behind while the run reported
completed), and stamped progress/completion without a status fence — a
run the watchdog failed kept copying beside a fresh one and finally
flipped its own row back to completed.

handleRef now HEADs source and target, GETs the source with its stored
Content-Type, PUTs it as-is, HEADs the copy to verify the size (a short
copy is deleted and counted failed), and only then deletes the source;
a target copy that already matches finishes a cut-off move, one of the
wrong size is re-copied. Two more phases walk app.tts_audio_chunks and
app.video_link_jobs (migration 0077 widens the phase CHECK; the TTS
sample entry carries no name because chunk text is message content).
Source and target naming one physical bucket is refused (SAME_STORE).
Every stamp and the completion UPDATE are fenced on status = 'running'
and RETURNING; nothing matched stops the engine without touching the
terminal row, and a stamp is due at least every 60 s inside a batch so
the watchdog window measures real liveness.

Findings: files-2, files-v1, files-3, files-9.
The data-residency page promised a byte-for-byte round-trip check and
row rewrites the engine never did. Say what it does now: the copy lands
with its content type, is verified against the source's size, and only
then is the source deleted; nothing is rewritten because keys stay
identical and reads locate the blob; a re-run finishes cut-off moves;
every ref-holding table is walked; the same-store case is refused.
English, German and French in the same change.

Findings: files-2, files-v1, files-3.
POST /files/upload stores the bytes first and records the upload intent
second; the intent row is the only record that the blob exists, and
recordUploadIntent also ran the lazy ledger sweep unguarded on the
request path. A DB error between the PUT and the INSERT (or inside the
sweep) answered 500 with an object in the bucket that no reclaim lane
could ever find — a permanent orphan, re-created on every client retry.

The route now deletes the blob it just minted (best-effort, via the
shared reclaim helper) before rethrowing when the intent write fails,
and the sweep inside recordUploadIntent is bookkeeping: it logs and
never fails the mint.

Finding: files-8.
The third session presign door — POST /files/upload-handoff with its own
size rule checked at presign — had no client: the app uploads through
/files/upload and /files/blob-upload, the REST door presigns on its own,
and the only sender was the backend's own integration check. Every
upload-policy change had to be replicated to it for nobody.

Delete the route, its schema and createUploadHandoff; the nine
integration-check journeys mint through /blob-upload instead (same
intent + register semantics). The 0067 migration comment and the
service header name the doors that remain.

Finding: files-4.
MAX_FILE_SIZE_BYTES (a 64 KB cap no reader of the object-storage config
files enforced) and parseObjectStorageSecretsJson (the credentials are
read through decryptSecretsFile + inline validation) had no consumer
anywhere in the repo; knip cannot flag them because backend/** are
entry files. A live-looking guard nothing enforces invites a reader to
rely on it.

Finding: files-5.
…ssing

chunkCompressedAudio removed its /tmp chunk directory with
`.catch(() => {})` on both the cleanup closure and the error path, so a
directory left behind by ENOTEMPTY/EBUSY accumulated across
transcription jobs with no line to diagnose the eventual disk-full.
Both sites now warn with the directory path, like cleanupTmp beside
them.

Finding: files-6.
The data-residency lane now serves a file stored before the org saved
its bucket connection, both before the backfill (from the default
store) and after it (from the BYO bucket), asserts the served
Content-Type survived the move, and that the source object is gone from
the default store once the run completed.

Findings: files-1, files-2, files-v1.
0077 is taken by the conversation-messages unique-external-id migration (PR #3225), which pushed first; two files sharing a number would fail the boot of whoever merges second. Renamed before the branch was ever pushed, so no deployment has recorded the old name.

Findings: files-3 (the phase CHECK widening).
The lane picked the org's oldest file_metadata row as the blob stored
before the BYO switch, but earlier lanes seed rows whose refs never had a
blob (stuck.mp3 → s3:stuck-audio-1, created 40 min in the past), so the
probe served a ghost and failed before and after the move alike.

Mint the pre-switch blob through the app's own door (blob-upload → PUT →
register, text/markdown) while the org still resolves to the default
store, probe that file, and assert that this exact blob is in the BYO
bucket and gone from the default store once the run completed.

Findings: files-1, files-2, files-v1 (integration proof).
The blob backfill became a destructive move (PUT, HEAD-verify, DELETE
source) guarded by two checks that could not carry that weight:

- `sameObjectStore` was a raw string compare of bucket/region/endpoint,
  so `http://minio:9000/` against `http://minio:9000`, a host spelled in
  another case, a DNS alias, or a proxy in front of the same bucket
  passed it; handleRef then found the "target" copy present at the
  source's size and deleted the only copy of every blob the org owned.
- The resume branch took any size-matching target copy as a finished
  move. Every org that ran the pre-move engine holds exactly such copies,
  written as application/octet-stream beside the correctly typed source;
  the first run after this change would have deleted the typed source for
  each of them and made the untyped copy permanent.

Now `sameObjectStore` compares an endpoint identity (URL-parsed, host
case-folded, trailing slash dropped; the same bucket with no endpoint is
the same AWS bucket whatever region string names it), and runBackfill
additionally proves physical identity with `sharesPhysicalStore`: a
marker written through the target under a fresh org-namespaced key must
stay invisible through the source, deleted again on every path. Dry runs
probe too, so a preview cannot report "nothing to move" for a run that
would delete everything. `s3HeadObject` returns the stored Content-Type
next to the size (`S3ObjectHead`), and handleRef finishes a cut-off move
only when size AND type agree (a source with no type compares on size);
any other target copy is re-copied over and the source retired after.

`locateOrgObject` returns the located store together with that HEAD for
readers that need it anyway. Tests: aliased endpoints by config and by
probe (dry and real, marker gone, failure UPDATE fenced), the
octet-stream re-copy counted migrated, the typeless-source resume, the
60 s liveness stamp inside one batch, and the new helpers.

Addresses PR #3239 review: blocking (a) and (b), stampIfDue test.
The abandoned-upload sweep and the replacement-upload cleanup resolved the
org's CURRENT store and deleted there. An intent minted before the org
connected its own bucket, whose grace window expires after the switch,
was "deleted" from the org bucket (S3 answers 204 for a missing key), its
ledger row dropped, and the blob stayed orphaned in the deployment default
store forever.

Both lanes now go through `deleteOrgObject`, which clears every store that
may hold the key — the concept the mixed-store read path already uses —
and the sweep no longer resolves a store it does not need.

Addresses PR #3239 review (non-blocking: upload-intents.ts:246/267,
replacement.ts:933-944).
`getWebdavBlobUrl` located the store holding the key (one HEAD per
candidate store) and then HEADed the located store again before
presigning — two round-trips per WebDAV GET for a BYO org. It now uses
`locateOrgObject`, which returns the located store together with that
HEAD, and presigns against it.

Addresses PR #3239 review (non-blocking: webdav/handlers.ts:914-920).
@larryro
larryro merged commit 4e0175b into main Sep 6, 2026
64 of 65 checks passed
@larryro
larryro deleted the fix/files-object-storage-integrity branch September 6, 2026 01:50
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.

1 participant