feat(storage): make the reconciler lane the only physical-bucket minting driver - #1783
Merged
Conversation
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
This comment has been minimized.
This comment has been minimized.
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.
Summary
Phase 1 of the storage convergence (constructive-io/constructive-planning#1891): the reconciler lane is the only thing that mints a physical bucket; everything in this repo reads.
Three drivers could create a physical bucket. Two of them lived here and are removed:
provisionAndRecordPhysicalBucket()minted a name from a deploymentresolveBucketNamepolicy, called the provisioner, and wrotephysical_namefrom inside a presign/upload request. That is why "whoever uploads first" was the de-facto provisioner.provisionBucketmutation — a second, complete provisioning implementation that also swallowed its failures, returning{ success: false, error }from acatchthat only logged.What remains is a read: the recorded
physical_nameis the only physical coordinate, and NULL means not reconciled yet.StorageBucketNotReconciledErrorcarriescode: 'STORAGE_BUCKET_NOT_RECONCILED'andretryable: true(also inextensions, so it reaches the client) — one typed error, one construction site, no provider call and no database write on the failure path.provisionBucketbecomes "reconcile now"The mutation still resolves the bucket row under RLS, then enqueues the same job the generated INSERT trigger enqueues instead of provisioning again. Nothing catches; an enqueue failure is a GraphQL error.
The payload stops claiming a synchronous provisioning result it no longer performs:
The enqueue shape is keyed off the storage module's stored
entity_field— never the scope name, percompute/AGENTS.md— which is exactly one ofNULL(global scopes),'database_id'(database scope), or'owner_id'(entity scopes):entity_fieldadd_jobargsNULL{id, scope}'database_id'{database_id, id, scope}db_id,entity_id,organization_id => NULL,entity_type'owner_id'{id, owner_id, scope}entity_id,organization_id => <get_org_fn>(entity_type, entity_id),entity_typeget_org_fnis resolved at runtime throughmetaschema.resolve_entity_context_by_field(...), the same resolverdata_job_triggeruses, so the entity-scope attribution matches the trigger's rather than being dropped.This mirroring of the generated trigger is a deliberate, commented wart. The durable fix is to generate a callable enqueue function beside the trigger so the trigger, this mutation and the constructive-db backfill share one implementation — noted for follow-up, out of scope here.
Deployment naming policy is gone
mintPhysicalBucketName(prefix, databaseId, bucketKey)and theresolveBucketName/ensureBucketProvisionedoptions are deleted from@constructive-io/bucket-provisioner,graphile-presigned-url-plugin,graphile-bucket-provisioner-pluginandgraphile-settings(includingcreateBucketNameResolver,createEnsureBucketProvisioned,getBucketNamePrefixand thebucket-provisioner-resolvermodule).physicalBucketName(identity)— derived from the bucket row's own scope/database/key rather than deployment config — is the surviving policy, and only the reconciler consults it.CDN_BUCKET_NAMEremains the connection defaults3.bucket, which was never a tenant bucket.BucketProvisionerPluginOptionshad nothing left in it, so the plugin and its preset now take no options.Companion PR
The other half is in
constructive-io/constructive-db: the reconciler switches tophysicalBucketName,provision_mode(lazy/eager) is removed so every bucket INSERT enqueues, and existing rows with a NULLphysical_nameare backfilled onto the job lane. Deployments that relied on lazy minting are covered by that backfill — without it, uploads to those rows would start returningSTORAGE_BUCKET_NOT_RECONCILED.Testing
pnpm lintclean; unit suites green for all four touched packages. Coverage added for: an upload against an unreconciled bucket rejecting withSTORAGE_BUCKET_NOT_RECONCILEDand making no provisioner/S3 call; the enqueue payload andadd_jobarguments for all threeentity_fieldshapes (including entity scope with and without an organization resolver); and a failing enqueue propagating instead of being swallowed.Proof
An upload against a bucket whose
physical_nameis still NULL rejects with the typed error and touches neither the provisioner nor S3 — the two named cases arerejects an unreconciled bucket without calling S3 or provisioningandreports an unreconciled bucket with a retryable typed error:The other half of the claim is only observable against a live database, so it is proved in the companion PR (constructive-io/constructive-db#3542): a bucket row inserted by
psql— no GraphQL — reaches a non-NULLphysical_namethrough the job lane and a real MinIO, theprovisionBucketmutation'sadd_jobstatement produces a byte-identical job, and a provisioning failure propagates with the bucket id and scope named instead of being logged and swallowed.Link to Devin session: https://app.devin.ai/sessions/67bb2f8f1a0e48b48517fac54f069f1c
Open in Devin Desktop: https://app.devin.ai/desktop/session/67bb2f8f1a0e48b48517fac54f069f1c?variant=devin
Requested by: @pyramation