feat(server): attachment object storage, quotas, and reference lifecycle - #1986
Draft
liuchao-001 wants to merge 10 commits into
Draft
feat(server): attachment object storage, quotas, and reference lifecycle#1986liuchao-001 wants to merge 10 commits into
liuchao-001 wants to merge 10 commits into
Conversation
…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>
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
byteapayloads.Storage boundary
attachments/<id>, and agent avatars atavatars/<uuid>(overwritten in place). Config is the optionalFIRST_TREE_S3_*env group; without it the server boots and serves pre-migration rows, but uploads answer503until storage is configured (no silent bytea fallback — that would keep the unbounded-growth path alive).attachmentsgainsorganization_id(quota accounting unit),object_key, and a three-statestatecolumn (pending → stored → deleting); the legacydatabytea 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
Upload is reservation-first: a
pendingrow (the durable quota reservation) is inserted under a per-org advisory xact lock, the body then streams straight into the object-storage PUT, and apending → storedCAS finalizes..returning()payload exposure is gone with the write path itself. —attachments-route.test.tspnpm --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)413(ATTACHMENT_TOO_LARGE); org totals 2 GB / 1000 objects →422with stableATTACHMENT_QUOTA_EXCEEDED, rejected before the body is read. Quota checks run underpg_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 ofMAX_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.tscollectAttachmentIdscovers the single-imagecontent.imageId, the batchcontent.attachments[].imageId, andmetadata.attachments[].attachmentIdshapes. 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).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.editMessagenow runs in a transaction (it was bare sequential statements).pendingreservations, deletes aged zero-edgestoredrows, and retries leftoverdeletingtombstones. —attachment-references.test.ts,attachment-sweep.test.tsmessages.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.Content-Lengthis now required →411; the declared size is what quota reservation is based on). Download:proxymode (default) pipes the object stream through the server;redirectmode answers302to a ≤300 s presigned URL with pinned response-content headers. Default is proxy because the web app consumes attachments with authenticatedfetch— 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.humanAgentId, default 4 parallel streams, env-tunable) answers429ATTACHMENT_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: requiresContent-Length(411on chunked/absent),413+ code for oversize (previously 400/413 split),422+ code for org quotas,429+ code for concurrency,409if an upload outlives its reservation window,503when storage is unconfigured. Success responses are unchanged.content.imageIdreferences (existence,storedstate, same-org-or-legacy) — previously shape-checked only. Fabricated/foreign ids get400; legitimate upload-then-send flows are unaffected. Legacy NULL-org attachments stay referenceable.GET /attachments/:id: unchanged for live rows (ETag/304 intact;Content-Dispositiongains an RFC 5987filename*);pending/deletingrows are 404.Content-Length(411); other avatar semantics (400 on oversize/empty/bad mime) are unchanged.codefield via the existingattrs.codeserialization; new codes are exported from@first-tree/shared(ATTACHMENT_ERROR_CODES), includingATTACHMENT_LENGTH_REQUIREDon the 411s.Two reviewer-accepted implementation deviations from the design doc: quota rejections reuse
UnprocessableErrorwithattrs.codeinstead of introducing a dedicated error class (the handler only readsattrs.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
0083applies on boot (adds columns/tables only; existing rows become validstoredrows). Uploads answer 503 until storage is configured; existing attachments keep downloading from PG.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.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.FIRST_TREE_ATTACHMENT_DOWNLOAD_MODE=redirectonce the bucket is browser-reachable and carries CORS for the web origin (FIRST_TREE_S3_PUBLIC_ENDPOINTsupports split-horizon presigning).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-latestfor bothci.ymlandpublish-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, theCI_S3_ENDPOINT/CI_S3_ACCESS_KEY_ID/CI_S3_SECRET_ACCESS_KEYescape hatch mirrorsCI_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.tsgrown 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 typecheckgreen; 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 intoclient-switch-transactionprocess 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).