Skip to content

feat(storage): storage coordinates only from storage_module; STORAGE_* credentials are the only env - #1879

Open
pyramation wants to merge 2 commits into
mainfrom
devin/1791334000-storage-db-driven
Open

pyramation wants to merge 2 commits into
mainfrom
devin/1791334000-storage-db-driven

Conversation

@pyramation

@pyramation pyramation commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Storage configuration is now 100% DB-driven. The cdn env model is deleted. The only storage env is STORAGE_ACCESS_KEY_ID / STORAGE_SECRET_ACCESS_KEY.

  • pgpm/types/pgpm/env/graphql/types: CDNOptions and pgpmDefaults.cdn are gone, and so is parsing of CDN_*/BUCKET_*/AWS_*/MINIO_*. They're replaced by storage?: { accessKeyId, secretAccessKey }. The pgpm env command prints only the two credentials.
  • graphile-presigned-url-plugin:
    • storage-module-cache (the existing LRU) selects endpoint/provider/region from the platform row, coalesce(sm.public_url_prefix, platform row), and connection_overrides. That last one lists any coordinate a non-platform row sets on its own.
    • resolveS3ForDatabase builds one S3Client per resolved [provider, endpoint, region], memoized. It throws STORAGE_CONNECTION_NOT_CONFIGURED when provider or region is missing. It throws STORAGE_CONNECTION_OVERRIDE_REFUSED when a module row names its own endpoint/provider/region, so the platform STORAGE_* credentials never sign for a tenant-chosen host (test: physical-bucket.test.ts).
    • Options are now { credentials } and not { s3 }. There's no global-S3 fallback in downloadUrl anymore.
  • graphile-settings: getStorageCredentials() (throws STORAGE_CREDENTIALS_MISSING) replaces the env S3 config. upload-resolver streams through the DB-resolved client and the bucket's physical_name. The process-wide BUCKET_NAME streamer is gone.
  • Deleted the env-driven graphql/server/src/scripts/create-bucket.ts, uploads/s3-streamer/src/s3.ts (getClient) and the unused graphql/explorer/src/resolvers/uploads.ts. Streamer now takes { client, defaultBucket }.
  • Seeds (simple-seed-storage, db-scope-storage) put http://localhost:9000 / minio / us-east-1 on the storage_module row. CI env is just the two credentials.

This PR's storage suites only go green once @pgpm/metaschema-modules with region is published and pgpm.json is bumped (step 2 below). Verified locally: with region overlaid in extensions/, graphql/server-test upload + db-scope-upload pass 51/51. The presigned-url plugin passes 91/91.

Single source of truth

metaschema_modules_public.storage_module holds endpoint, provider, region (new column, added in place) and public_url_prefix. Buckets get their physical identity from each bucket row's physical_name.

  • The deployment's object store is the platform database's scope = 'platform', key = 'default' row.
  • endpoint/provider/region always come from that platform row. Other planes may set only public_url_prefix, and a row that sets its own coordinates is refused (STORAGE_CONNECTION_OVERRIDE_REFUSED). This is resolved inside the query that is already cached, so there's no extra DB read.
  • platform_config and platform_secrets hold no storage coordinates.
  • The only env is STORAGE_ACCESS_KEY_ID / STORAGE_SECRET_ACCESS_KEY. Each storage consumer requires them and fails fast with an error naming both. There are no aliases and no fallbacks.
  • Presigned URLs are signed for the DB endpoint, so the DB has to hold a client-reachable host.

Merge / publish order (4 PRs)

  1. constructive-io/pgpm-modules devin/1791334000-storage-region: merge, then publish @pgpm/metaschema-modules (the next lerna release after 0.47.0). This repo is what npm publishes. constructive-db's pgpm-modules/ is a vendored mirror (docs/architecture/npm-publication-sources.md).
  2. constructive-io/constructive devin/1791334000-storage-db-driven: bump the root pgpm.json fixture @pgpm/metaschema-modules to that release (re-run pnpm fixtures:install). The storage suites need storage_module.region, so they only pass after that bump. Merge, then publish the graphile-*, @constructive-io/* and pgpm packages.
  3. constructive-io/constructive-db devin/1791334000-storage-db-driven (targets main): CI doesn't depend on any npm publish. Its Feature document check needs a constructive-testing-seeds release generated from that branch, because the pinned seed predates region. Before deploying, bump @constructive-io/graphql-server / graphile-settings to the step-2 releases (the published ones before that still read CDN_*). Relabel the mirror to the step-1 version.
  4. constructive-io/constructive-cloud devin/1791334000-storage-db-driven: apply only together with a release promotion that pins images built from steps 2 and 3. The bootstrap writes storage_module.region, and pods no longer get CDN_*/BUCKET_*.

Link to Devin session: https://app.devin.ai/sessions/f3fd69925240491c9d7507d3b97aa5cb
Open in Devin Desktop: https://app.devin.ai/desktop/session/f3fd69925240491c9d7507d3b97aa5cb?variant=devin
Requested by: @pyramation

@devin-ai-integration

Copy link
Copy Markdown
Contributor

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

@tenki-reviewer

tenki-reviewer Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Review complete. 🟡 1 medium

💬 Inline comments (1)

  • 🟡 AWS_ credential env aliases dropped with no fallback* — env.ts:55

The change centralizes object-store connection details in the database: storage_module rows now carry endpoint, provider, region, and public URL prefix, coalesced against a platform-plane default row, while only credentials remain in env (STORAGE_ACCESS_KEY_ID/STORAGE_SECRET_ACCESS_KEY). Plugin option shapes were renamed (s3 → credentials), the s3-streamer Streamer constructor is now client-only, and removed resolver/scripts are cleaned up. The downloadUrl resolver dropped its catch fallback so lookup errors surface instead of degrading, and the S3 client cache now keys on (provider, endpoint, region).

Files Change
graphile/graphile-presigned-url-plugin, graphile/graphile-settings Resolve S3 coordinates from storage_module rows with platform inheritance; cache and credential config reshaped
pgpm/env, pgpm/cli/src/commands/env.ts, pgpm/types Replace cdn/AWS_* env block with STORAGE_* vars and updated pgpm env output
uploads/s3-streamer Streamer now takes a prebuilt S3Client ({ client, defaultBucket }) instead of raw connection options
graphql/server-test fixtures, CI workflow, seeds Fixtures hardcode endpoint/region values; test workflow updated for the new env shape
deleted resolvers/scripts Removed graphql/explorer uploads resolver and create-bucket script with no remaining importers

Reviewed commit: bcfcc60

@tenki-reviewer tenki-reviewer Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR moves S3 storage coordinates (endpoint/provider/region/public URL prefix) from cdn env config into storage_module rows with platform-plane inheritance, renames s3 plugin options to credentials, and drops legacy AWS_*/CDN_* env aliases in favor of STORAGE_*.

Key findings

  • 🟡 AWS_ credential env aliases dropped with no fallback* — env.ts:55

Comment thread pgpm/env/src/env.ts
Comment on lines +55 to +56
STORAGE_ACCESS_KEY_ID,
STORAGE_SECRET_ACCESS_KEY,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 bug · medium

AWS_ credential env aliases dropped with no fallback*

getEnvVars now reads only STORAGE_ACCESS_KEY_ID / STORAGE_SECRET_ACCESS_KEY (pgpm/env/src/env.ts:55). The previous implementation explicitly merged AWS_ACCESS_KEY || AWS_ACCESS_KEY_ID and AWS_SECRET_KEY || AWS_SECRET_ACCESS_KEY into the storage credentials, so deployments authenticating via the standard AWS SDK env names silently lose object-store credentials after upgrade. The failure surfaces only as a runtime STORAGE_CREDENTIALS_MISSING error on the first upload or presigned-URL request, not at boot.

📋 Prompt for AI Agents

In pgpm/env/src/env.ts around lines 55-56 and 138-141, restore backward compatibility: destructure AWS_ACCESS_KEY, AWS_ACCESS_KEY_ID, AWS_SECRET_KEY, AWS_SECRET_ACCESS_KEY from env and populate storage.accessKeyId with STORAGE_ACCESS_KEY_ID || AWS_ACCESS_KEY_ID || AWS_ACCESS_KEY and storage.secretAccessKey with STORAGE_SECRET_ACCESS_KEY || AWS_SECRET_ACCESS_KEY || AWS_SECRET_KEY, logging a deprecation notice when only the AWS_* names are present. The prior code honored these standard aliases; dropping them silently breaks existing deployments at first upload.

@blacksmith-sh

blacksmith-sh Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Found 16 test failures on Blacksmith runners:

Failures

Test View Logs
database-scope upload surface › presigned upload against the database-scope plane/
accepts the PUT, into a physical bucket recorded on the tenant row
View Logs
database-scope upload surface › presigned upload against the database-scope plane/
records the file row against the tenant plane, not the app-scope one
View Logs
database-scope upload surface › presigned upload against the database-scope plane/
returns a presigned PUT URL via uploadFile
View Logs
Integration tests (uploads, tenant isolation, RLS) › Auto-provision on bucket create is
disabled (Alice)/createAppBucket records an unreconciled row and uploads reject it
View Logs
Integration tests (uploads, tenant isolation, RLS) › Presigned URL uploads (Alice)/
Deduplication › should return deduplicated=true for an existing content hash
View Logs
Integration tests (uploads, tenant isolation, RLS) › Presigned URL uploads (Alice)/
Private file upload › should accept a PUT to the presigned URL
View Logs
Integration tests (uploads, tenant isolation, RLS) › Presigned URL uploads (Alice)/
Private file upload › should return a presigned PUT URL via uploadAppFile
View Logs
Integration tests (uploads, tenant isolation, RLS) › Presigned URL uploads (Alice)/
Public file upload › should accept a PUT to the presigned URL
View Logs
Integration tests (uploads, tenant isolation, RLS) › Presigned URL uploads (Alice)/
Public file upload › should return a presigned PUT URL via uploadAppFile
View Logs
Integration tests (uploads, tenant isolation, RLS) › Reconciliation enqueue via provisi
onBucket (Alice)/
can enqueue reconciliation repeatedly without changing the recorded name
View Logs

...and 6 more test failures. View all on Blacksmith

Fix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

…dpoint/provider/region for the platform credentials
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