Skip to content

feat(server): attachment object storage, quotas, and reference lifecycle - #1986

Draft
liuchao-001 wants to merge 10 commits into
mainfrom
fix/1664-attachment-s3-migration
Draft

feat(server): attachment object storage, quotas, and reference lifecycle#1986
liuchao-001 wants to merge 10 commits into
mainfrom
fix/1664-attachment-s3-migration

Conversation

@liuchao-001

@liuchao-001 liuchao-001 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Closes #1664.

Implements the full governance scope pinned in the issue's binding "GoF Competition scope & acceptance" comment: attachment binary payloads move to S3-compatible object storage (PostgreSQL keeps metadata only), with hard-reject quotas, a reference lifecycle with orphan cleanup, streaming on both transfer paths, bounded upload concurrency, and an idempotent data-migration command for existing bytea payloads.

Storage boundary

  • Object storage (any S3-compatible backend — AWS S3, R2, MinIO): every attachment payload at the deterministic key attachments/<id>, and agent avatars at avatars/<uuid> (overwritten in place). Config is the optional FIRST_TREE_S3_* env group; without it the server boots and serves pre-migration rows, but uploads answer 503 until storage is configured (no silent bytea fallback — that would keep the unbounded-growth path alive).
  • PostgreSQL: metadata + lifecycle state only. attachments gains organization_id (quota accounting unit), object_key, and a three-state state column (pending → stored → deleting); the legacy data bytea turns nullable and is cleared by the migration command. Dropping the column entirely is a small follow-up PR once deployments have migrated (the scope comment explicitly allows null-then-drop).

Acceptance checklist → implementation

  1. New uploads land in object storage; the row holds metadata + key only.
    Upload is reservation-first: a pending row (the durable quota reservation) is inserted under a per-org advisory xact lock, the body then streams straight into the object-storage PUT, and a pending → stored CAS finalizes. .returning() payload exposure is gone with the write path itself. — attachments-route.test.ts
  2. Migration command moves existing payloads; pre-migration attachments keep downloading.
    pnpm --filter @first-tree/server migrate:attachments — five idempotent phases: org backfill from uploader rows, reference-ledger backfill from message content/metadata, attachment payload move (atomic key+NULL swap per row), avatar payload move, verify (non-zero exit while any payload remains inline). Runs against a live server; the download path serves remaining bytea rows directly and falls through to the deterministic key when a row migrates between its metadata and payload reads. A 0-row swap is adjudicated by re-reading the row — the object is deleted only when genuinely ownerless (row destroyed/tombstoned), never when a rival run or a concurrent online avatar upload won the swap; test hooks expose the between-PUT-and-UPDATE window for deterministic race coverage. — attachment-migration.test.ts (byte-identical round-trip, org/edge backfill, dangling-id filtering, idempotent rerun, rival-run/sweep/online-upload races)
  3. Quotas, hard reject. Single file 10 MiB → 413 (ATTACHMENT_TOO_LARGE); org totals 2 GB / 1000 objects → 422 with stable ATTACHMENT_QUOTA_EXCEEDED, rejected before the body is read. Quota checks run under pg_advisory_xact_lock(hashtext('attachment_quota'), hashtext(orgId)) (the repo's existing two-int lock idiom), so concurrent uploads cannot jointly overshoot — covered by a race test asserting exactly one of two jointly-over-quota uploads is admitted. The governed "2 GB" is implemented as 2 GiB (2^31), consistent with the binary units of MAX_ATTACHMENT_BYTES (and slightly generous rather than strict); both limits are env-tunable with the governed values as defaults, and the reject semantics never soften. — attachment-quota.test.ts
  4. Reference lifecycle & orphan cleanup.
    • Discovery has one source of truth: collectAttachmentIds covers the single-image content.imageId, the batch content.attachments[].imageId, and metadata.attachments[].attachmentId shapes. Agent avatars are deliberately not attachment references in this codebase — they are a separate 1:1, size-capped, overwrite-in-place surface (agents.avatar_*), migrated and served through the same storage but exempt from reference counting and org quotas (bounded by 512 KiB × agent count; nothing accumulates).
    • An attachment_references (attachment_id, message_id) ledger is maintained inside the message write transaction (send and edit), with RESTRICT FKs — a cascade could only ever fire on a bug deleting a still-referenced attachment, and would destroy the evidence.
    • Delete on last reference removed: message edit — the one live event that can drop a reference today, since messages/chats/orgs have no delete paths — reconciles the ledger and tombstones + destroys attachments whose last edge disappeared. editMessage now runs in a transaction (it was bare sequential statements).
    • 24 h orphan sweep: a background pass (SKIP LOCKED claims, concurrent-replica safe, no leader election — same claim shape as the cron scheduler) reclaims expired pending reservations, deletes aged zero-edge stored rows, and retries leftover deleting tombstones. — attachment-references.test.ts, attachment-sweep.test.ts
    • Deploy-window safety: between deploying this PR and running the backfill, every pre-existing attachment has zero edges. The orphan pass therefore carries a verify scan: candidates are matched as literal text against messages.content/metadata (one scan per batch) and any hit is vetoed and logged. Text matching can only err toward keeping; the pre-backfill window is a performance note, not a data-loss window.
  5. Streaming, no full buffering. Upload: a plugin-scoped content-type parser hands the raw request stream through a byte-exact limit stream into the PUT (Content-Length is now required → 411; the declared size is what quota reservation is based on). Download: proxy mode (default) pipes the object stream through the server; redirect mode answers 302 to a ≤300 s presigned URL with pinned response-content headers. Default is proxy because the web app consumes attachments with authenticated fetch — redirect-by-default would require every existing deployment to configure browser-reachable buckets + CORS before images render again. Avatars always proxy regardless of mode: a per-request presigned redirect varies the URL every time and would defeat the immutable browser cache that surface depends on.
  6. Bounded concurrency. A per-uploader in-process gate (keyed by humanAgentId, default 4 parallel streams, env-tunable) answers 429 ATTACHMENT_CONCURRENCY_EXCEEDED; the global rate limiter still applies. Per-instance scoping matches the repo's in-memory rate-limit precedent (PostgreSQL stays the only shared backend).

Wire-contract changes

  • POST /orgs/:orgId/attachments: requires Content-Length (411 on chunked/absent), 413 + code for oversize (previously 400/413 split), 422 + code for org quotas, 429 + code for concurrency, 409 if an upload outlives its reservation window, 503 when storage is unconfigured. Success responses are unchanged.
  • Message writes now validate content.imageId references (existence, stored state, same-org-or-legacy) — previously shape-checked only. Fabricated/foreign ids get 400; legitimate upload-then-send flows are unaffected. Legacy NULL-org attachments stay referenceable.
  • GET /attachments/:id: unchanged for live rows (ETag/304 intact; Content-Disposition gains an RFC 5987 filename*); pending/deleting rows are 404.
  • Avatar upload requires Content-Length (411); other avatar semantics (400 on oversize/empty/bad mime) are unchanged.
  • Error bodies carry the stable code field via the existing attrs.code serialization; new codes are exported from @first-tree/shared (ATTACHMENT_ERROR_CODES), including ATTACHMENT_LENGTH_REQUIRED on the 411s.

Two reviewer-accepted implementation deviations from the design doc: quota rejections reuse UnprocessableError with attrs.code instead of introducing a dedicated error class (the handler only reads attrs.code), and the migration verifies payload length from the local buffer + column normalization instead of a HeadObject round-trip (equivalent under the SDK's enforced ContentLength).

Deployment runbook

  1. Deploy. Schema migration 0083 applies on boot (adds columns/tables only; existing rows become valid stored rows). Uploads answer 503 until storage is configured; existing attachments keep downloading from PG.
  2. Configure FIRST_TREE_S3_* (see .env.example; MinIO ships in docker-compose for dev) and restart. The server ensures the bucket on boot (or pre-provision it under least-privilege credentials). Uploads now stream to object storage.
  3. Run pnpm --filter @first-tree/server migrate:attachments (idempotent, resumable, live-safe). Rerun after a crash or to converge references recorded while it ran; it exits non-zero while any payload remains inline.
  4. Optional scale-out: set FIRST_TREE_ATTACHMENT_DOWNLOAD_MODE=redirect once the bucket is browser-reachable and carries CORS for the web origin (FIRST_TREE_S3_PUBLIC_ENDPOINT supports split-horizon presigning).
  5. Follow-up PR (after fleet migration): drop attachments.data + agents.avatar_image_data.

CI / test infrastructure

Server tests provision a pinned MinIO via testcontainers in global setup (started concurrently with the PostgreSQL container), with one bucket per vitest worker mirroring the per-worker databases. No workflow changes: this works identically on ubuntu-latest for both ci.yml and publish-npm-package.yml (a sidecar approach would have had to edit both). It is the first time CI actually exercises the testcontainers path (locally it always did); if that ever proves flaky, the CI_S3_ENDPOINT / CI_S3_ACCESS_KEY_ID / CI_S3_SECRET_ACCESS_KEY escape hatch mirrors CI_DATABASE_URL — a one-line workflow edit switches to a sidecar with zero code changes. Tests require no real cloud credentials anywhere.

New suites: attachment-references.test.ts (9 — incl. the wire-400 pin for invalid content refs), attachment-sweep.test.ts (5), attachment-migration.test.ts (4 — the end-to-end plus three hook-driven races), attachment-quota.test.ts (5 — incl. the concurrency race with a SUM-≤-quota invariant and a held-slot 429 over the wire), stream-limit.test.ts (7 — byte-limit semantics and the settleStreamingUpload cross-cancel matrix under an unhandled-rejection probe); attachments-route.test.ts grown to 17 (streaming, 411/413/503 with stable codes, undershot-Content-Length cleanup with a bucket-count leak check, redirect + presigned URL followed end-to-end). Existing suites adapted where contracts intentionally changed. Local gate: pnpm check + pnpm typecheck green; server 2664/2664 across 248 files (zero unhandled rejections), shared 694/694, client/web/qa green. Two machine-local false failures observed and verified unrelated: the known agent-env leak into client-switch-transaction process scans (pre-existing; 21/21 under a sanitized env) and git-fixture timeouts in skill-evals under parallel load (green rerun in isolation).

Out of scope (per the binding comment)

CDN/edge caching, thumbnails, virus scanning, resumable-upload UI, cross-region replication, storage-class tiering, per-user quotas. Also intentionally not introduced: message/chat/org delete APIs (the reference machinery is generic set-diff, so future removal events plug into the same helper).

liuchao-001 and others added 8 commits July 23, 2026 18:42
…r classes

Groundwork for attachment S3 migration and governance (#1664):

- shared: default org attachment quotas (2 GiB / 1000 objects — the
  governed "2 GB" implemented in binary units consistent with
  MAX_ATTACHMENT_BYTES), stable machine-readable attachment error codes,
  and the fixed 300s presign TTL.
- shared config: optional `objectStorage` group (S3-compatible endpoint,
  bucket, credentials, path-style, split-horizon public endpoint) and an
  `attachments` governance group (download mode, quotas, sweep cadence,
  orphan grace, pending TTL, per-uploader concurrency cap).
- server errors: PayloadTooLargeError (413), LengthRequiredError (411),
  TooManyRequestsError (429); wire codes ride the existing attrs.code
  serialization.
- tests: carry the new required `attachments` config group in the three
  full-Config literals.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…erence ledger, avatar key

Migration 0083 (drizzle-kit generate):

- attachments: + organization_id (quota accounting unit, FK to
  organizations, nullable only for legacy rows), + object_key
  (deterministic `attachments/<id>`), + three-state lifecycle column
  (pending → stored → deleting; column default 'stored' exists only to
  legalize pre-existing rows), data bytea now nullable (migration target),
  quota and sweep indexes.
- attachment_references: (attachment_id, message_id) reference ledger with
  RESTRICT FKs — a cascade could only fire on a logic bug deleting a
  still-referenced attachment and would silently destroy the evidence.
- agents: + avatar_object_key (`avatars/<uuid>`, overwritten in place;
  avatars are deliberately not attachments rows).
- test setup: TRUNCATE list gains attachment_references + attachments
  (the latter was a pre-existing gap).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rastructure

- services/object-storage.ts: thin storage boundary over @aws-sdk/client-s3
  (streaming put with pinned Content-Length, streaming get, idempotent
  delete, presigned GET with response-content headers, best-effort
  ensureBucket) plus deterministic key derivation and a shared RFC 6266/
  5987 Content-Disposition builder. Split-horizon presigning goes through
  the optional public endpoint.
- app: decorate `objectStorage` (null until FIRST_TREE_S3_* is set),
  ensure the bucket on ready, and warn loudly when storage is absent.
- test infra: vitest global setup now boots a pinned MinIO via
  testcontainers alongside Postgres (CI_S3_ENDPOINT escape hatch mirrors
  CI_DATABASE_URL), provisions one bucket per pool worker, and hands the
  target to suites via VITEST_S3_*; helpers gain workerObjectStorage().
- shared: export the new attachment governance constants from the package
  entry point.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…vatar payloads in object storage

Upload (POST /orgs/:orgId/attachments):
- plugin-scoped stream passthrough parser replaces the buffering default;
  the body pipes straight into the object-storage PUT via a byte-exact
  limit stream — never materialized in Node heap.
- Content-Length is required (411): quota is reserved from the declared
  size up front. Reservation-first lifecycle: pending row insert under the
  per-org advisory xact lock (413 / 422 ATTACHMENT_QUOTA_EXCEEDED on the
  governed limits), stream, then pending→stored CAS (409 if the pending
  TTL sweep won the race). Failures best-effort delete object + row.
- per-uploader concurrency gate (429 ATTACHMENT_CONCURRENCY_EXCEEDED),
  keyed by humanAgentId, per instance.

Download (GET /attachments/:id):
- pending/deleting rows are invisible (404); ETag/304 unchanged.
- legacy bytea rows serve from PG until migrated (with a deterministic-key
  fallthrough for the migrate-during-download race); S3-backed rows stream
  through the server (proxy, default) or 302 to a ≤300s presigned URL
  (redirect mode).

Avatars: payloads now live at avatars/<uuid> in object storage (streamed
upload, overwrite in place, legacy inline bytea still served until
migrated); the public route always proxies — a per-request presigned
redirect would defeat the immutable browser cache this surface depends
on. Delete clears the row first, then best-effort deletes the object.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dit, delete on last reference removed

- services/attachment-references.ts: collectAttachmentIds is THE single
  discovery source for every reference shape (content single/batch imageId,
  metadata.attachments[] attachmentId); syncMessageAttachmentReferences
  reconciles the edge ledger inside the message write transaction with one
  ascending-id FOR UPDATE over the touched set (global lock order, no
  cycles), validating added refs (exists / stored / same-org-or-legacy) and
  tombstoning attachments whose last edge was removed;
  destroyDeletingAttachments performs the post-commit object+row destroy
  with the sweep as crash backstop.
- sendMessageInner records edges at insert — content imageIds get their
  first existence/state/org gate (previously shape-checked only).
- editMessage now runs in a transaction (it was bare sequential calls) and
  reconciles references on content replacement — the one live event that
  can drop a reference; callers pass objectStorage for the post-commit
  destroy.
- integration suite: edge recording (single/batch/metadata), rejection of
  unknown/pending/deleting/cross-org refs, legacy NULL-org grandfathering,
  last-reference destroy, edge move on image replacement, shared-reference
  survival, storage-unavailable tombstone retention.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…pass, tombstone retry

sweepAttachments runs three SKIP LOCKED passes (the cron-scheduler claim
shape; concurrent replicas split batches, no leader election):

1. pending reservations older than the TTL — reclaims quota leaked by
   crashed/abandoned uploads.
2. stored rows past the orphan grace with zero ledger edges — guarded by
   the verify scan: candidates are matched as literal text against
   messages.content/metadata (one jsonb_array_elements_text-driven scan
   per batch) and any hit is vetoed + logged. This is the safety net that
   makes the deploy-before-backfill window loss-free: pre-migration
   attachments all have zero edges until migrate:attachments records
   them, and text matching can only err toward keeping.
3. leftover deleting tombstones — crash/storage-outage retry.

Wired as the fourth background-tasks timer (config-gated interval, 0
disables; per-instance overlap latch), stopped on close.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…storage with governance backfill

migrateAttachmentsToObjectStorage (service, tested) + thin tsx runner
wired as `pnpm --filter @first-tree/server migrate:attachments`. Five
idempotent phases, safe against a live server:

  A. attachments.organization_id from the uploader's agent row (soft-
     deleted agents keep rows, so backfill is near-total; the remainder
     stays NULL = quota-exempt legacy)
  B. attachment_references from message content/metadata via the same
     collectAttachmentIds the live path uses; dangling historic ids and
     tombstoned rows are filtered by the insert's join
  C. attachment payloads → attachments/<id>: PUT, then one atomic UPDATE
     sets object_key + clears data (+ normalizes size_bytes); rows the
     sweep tombstoned mid-flight skip and the fresh object is removed
  D. avatar payloads → avatars/<uuid>
  E. verify — non-zero exit while any payload remains inline

Dev/docs: docker-compose gains a pinned MinIO service (bucket auto-created
on server boot), .env.example documents the objectStorage + governance
groups (2 GB implemented as 2 GiB, noted), DEVELOPMENT.md covers the new
quickstart surface, AGENTS.md's storage rule now names the S3 boundary.

Also: quota suite (422 byte/count + stable code, concurrent both-fit-
jointly-exceed race admits exactly one, upload-gate 429 unit) and a
redirect-mode download test that follows the presigned URL end to end.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…um body wrapping

A byte-limit failure mid-stream used to reject BOTH halves of the upload
(the source→limiter pipeline and the SDK PUT): Promise.all surfaced one
and orphaned the other as an unhandledRejection, and the surviving SDK
call could stall for its full timeout on the half-fed body.
settleStreamingUpload now owns the coordination: a producer failure
aborts the in-flight PUT via AbortSignal, a consumer failure destroys the
limiter so the backpressured pipeline settles, both halves are always
awaited, and the producer's root-cause error wins.

The S3 client also opts out of default CRC32 request-checksum wrapping
(WHEN_REQUIRED): the extra internal body pipe it adds leaked one more
unhandled rejection on aborted uploads, none of our calls require
checksums, and some S3-compatible backends reject checksum trailers.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@liuchao-001 liuchao-001 added the fire_wip GoF: draft PR in progress label Jul 23, 2026
liuchao-001 and others added 2 commits July 23, 2026 20:04
…ndly

A 0-row UPDATE in the migration's payload-move phases has two very
different causes, and the old branch deleted the object for both:

- row destroyed/tombstoned mid-flight → the object IS ownerless; delete.
- row alive with the key already set → a rival migration run (phase C) or
  an online avatar upload (phase D) won the swap; the row OWNS the key,
  and deleting would permanently destroy a live payload — for avatars,
  one a user uploaded seconds earlier, with no operator misuse required.

Both phases now re-read the row and delete only in the ownerless case,
and recheck the row right before the PUT so the fixed-key last-writer-
wins overwrite window stays negligible. Test hooks (cron-scheduler's
afterClaimForTest pattern) expose the between-PUT-and-UPDATE window.

Also from review: phase B pages skip-and-log on the narrow FK race
instead of aborting the command; 411 responses carry the stable
ATTACHMENT_LENGTH_REQUIRED code (parity with 413/422/429); the reference
ledger comment says NO ACTION (what the DDL declares) instead of
RESTRICT; DEVELOPMENT.md documents the migrate:attachments runbook step;
Content-Disposition ASCII fallback also neutralizes backslashes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- stream-limit.test.ts: byte-limit exact/overshoot/undershoot, and the
  settleStreamingUpload matrix — producer failure aborts the consumer,
  consumer failure destroys the limiter to release backpressure, genuine
  double rejection surfaces the producer's root cause, clean success —
  all under an unhandledRejection collector asserting zero orphans (the
  reason the coordinator exists).
- migration races, driven through the new hooks: rival-run 0-row swap
  keeps the live object, sweep-destroyed row still deletes the ownerless
  object, and an avatar uploaded mid-migration survives phase D.
- route-level pins: undershot Content-Length cleans up reservation + no
  bucket object leak; 411/429 carry their stable codes over the wire;
  invalid content imageId surfaces as a wire 400 on the chat message
  route; concurrent-quota race also asserts the SUM-≤-quota invariant;
  avatars stay proxied under downloadMode=redirect.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fire_wip GoF: draft PR in progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[PERF-001][High] Attachment storage, download, and lifecycle are unbounded

1 participant