pneumo

M2: Storage adapter: S3 client, in-memory fake, bucket CORS and upload mode (ENG-249) #12

Merged

demi.dev 1w ago

targeting main from stoopkidddd/eng-249-m2-storage-adapter-s3-client-in-memory-fake-bucket-cors-and

Merged opened by and assigned to

demi.dev 1w ago targeting main from stoopkidddd/eng-249-m2-storage-adapter-s3-client-in-memory-fake-bucket-cors-and

Linear: https://linear.app/engineereddev/issue/ENG-249

What's in this PR:

apps/server/src/storage/: the Storage contract (types.ts), S3Storage (AWS SDK v3, presigned PUT/GET, prefix-stripped listing, conditional create with ifNoneMatch: "*"), MemoryStorage (in-memory fake with presigned-request emulation), and one shared contract suite that runs against both.
upload-mode.ts: resolves UPLOAD_MODE=auto at boot by applying bucket CORS with a 2 s deadline; falls back to proxy with one warning (operation + safe reason only).
app.ts / index.ts: the app now takes the real storage and the resolved upload mode; the bucket origin goes into CSP connect-src only in direct mode. Storage is destroyed on shutdown.
log.ts: S3 key names are redacted.
test:e2e script + vitest.e2e.config.ts + MinIO global setup (reuses scripts/e2e/minio.ts).
README: storage and upload-mode notes.

Verify (lead):

mise run check:server → exit 0 (8 files, 88 tests)
pnpm --filter @pneumo/server test:e2e → exit 0 (7 tests against MinIO)
After e2e: no pneumo-e2e-* containers, no leftover listeners.

Deviations:

MinIO rejects PutBucketCors, so detection selects proxy against MinIO. Direct mode still needs a staging check on Railway's bucket (spec §18.1).
AWS SDK default retries are kept: with retries off, MinIO reset the connection after a 412 conflict.
The e2e:server mise task is added by the lead after ENG-277 merges (mise.toml is held by ENG-277).

🤖 Generated with Claude Code

Commits 0

Could not load commits.

Comments

v0 1w ago

demi.dev 1w ago

pneumo-reviewer: round 1, REQUEST_CHANGES #

The storage contract, S3 and memory adapters, upload-mode detection and CSP wiring are correct and well tested. One wiring bug blocks the merge.

Findings #

FIX (often 2 × bad 3 = 6), apps/server/src/index.ts:19: storage.destroy() is registered as a shutdown hook. Hooks run at SIGTERM alongside server.close(), not after the drain, and S3Client.destroy() kills sockets in use. The reviewer reproduced ECONNRESET on an in-flight GetObject and on a streamed body. This breaks §12.7's 10 s drain for every storage-backed request during a deploy. Fix: remove the hook; if wanted, destroy after shutdownServer(...) resolves.
FILE (often 1 × bad 2 = 2), apps/server/src/storage/s3.ts:93-94: mapError sets bucketMissing: true for any 404 in the bucket context, including GetBucketCors's NoSuchCORSConfiguration. That could cause a misleading 10 s retry and log line on a backend that accepts PutBucketCors but returns no config on read-back. Not hit today (MinIO fails at applyCors first). Filed, code left alone.

Spec checks #

§5.3 OK: prefix applied to every key, invisible to callers, stripped on list; two prefixes isolated on MinIO.
§11 OK: CORS rule matches; direct only if apply + read-back succeed; one warn line otherwise. MinIO: existing bucket → proxy in 2 ms; missing bucket → bounded retry, proxy after ~10 s; unreachable endpoint → proxy, server still starts.
§12.3 / §12.4 / §12.6 OK: bucket origin in CSP only in direct mode; S3 key names redacted and asserted.
§12.7: deviation, finding 1.
§18.1: MinIO resolves to proxy (applyCors Unsupported); Railway still to check in staging.

DoD checks #

Unit tests with MemoryStorage and fake clock: present. MinIO contract suite via test:e2e (ifNoneMatch ran, not skipped): present. Redaction assertions for S3 keys, failing presign and failing list: present. check:server excludes *.e2e.ts: yes. Edits outside Touches (app.ts, log.ts, README.md) are each needed.

Tests run by the reviewer: mise run check:server exit 0 (88 tests); test:e2e exit 0 (7 tests); no leftover containers.

Next: omp round 2 fixes finding 1.

v1 1w ago

demi.dev 1w ago

pneumo-reviewer: round 2, APPROVE #

Round 2 (7f5de8c) fixes the blocking finding.

Findings #

Round 1 FIX (score 6), apps/server/src/index.ts:19: resolved. The storage.destroy() shutdown hook is gone (const shutdownHooks: ShutdownHook[] = [];). shutdownServer now only closes the server and waits up to 10 s, so requests waiting on the bucket finish; process.exit releases the S3 sockets afterwards. The new README sentence is accurate.
Round 1 FILE (score 2), apps/server/src/storage/s3.ts:93-94: unchanged, recorded for triage on ENG-275. Not blocking.

Spec / DoD #

§12.7 now OK. §5.3, §11, §12.3, §12.4, §12.6, §18.1 unchanged from round 1 (OK).
DoD: MemoryStorage + fake-clock unit tests, MinIO contract suite (test:e2e), redaction assertions: present. This round touches only index.ts and one README line.

Tests run by the reviewer on 7f5de8c: mise run check:server exit 0 (88 tests); test:e2e exit 0 (7 tests); no leftover containers.

Lead verify on 7f5de8c: same results. The e2e:server mise task lands in a small follow-up after ENG-277 merges (mise.toml is held by ENG-277).

Versions

pneumo-reviewer: round 1, REQUEST_CHANGES #

Findings #

Spec checks #

DoD checks #

pneumo-reviewer: round 2, APPROVE #

Findings #

Spec / DoD #