Repository navigation
feat: add bucket versioning status and version-aware object writes - #1347
Merged
Merged
Conversation
Coverage Report for CI Build 37915906656Coverage increased (+0.3%) to 85.259%Details
Uncovered Changes
Coverage Regressions1 previously-covered line in 1 file lost coverage.
Coverage Stats💛 - Coveralls |
Closed
TylerHillery
force-pushed
the
tyler/feat/object-versioning-wave-3
branch
from
August 24, 2026 20:35
5c024ac to
ea8f277
Compare
TylerHillery
force-pushed
the
tyler/feat/object-versioning-wave-3
branch
2 times, most recently
from
August 25, 2026 22:17
14cfdee to
f79566b
Compare
TylerHillery
force-pushed
the
tyler/feat/object-versioning-wave-3
branch
from
August 26, 2026 12:49
f79566b to
e979085
Compare
TylerHillery
force-pushed
the
tyler/feat/object-versioning-wave-3
branch
from
August 26, 2026 12:55
e979085 to
44909f1
Compare
TylerHillery
force-pushed
the
tyler/feat/object-versioning-wave-3
branch
from
August 27, 2026 01:13
44909f1 to
32c4170
Compare
TylerHillery
force-pushed
the
tyler/feat/object-versioning-wave-3
branch
2 times, most recently
from
August 27, 2026 01:18
4c7c1f3 to
8b7c848
Compare
TylerHillery
force-pushed
the
tyler/feat/object-versioning-wave-3
branch
from
August 27, 2026 13:52
8b7c848 to
17bf505
Compare
TylerHillery
force-pushed
the
tyler/feat/object-versioning-wave-3
branch
from
August 27, 2026 16:20
17bf505 to
f40a702
Compare
TylerHillery
force-pushed
the
tyler/feat/object-versioning-wave-3
branch
from
August 27, 2026 18:53
f40a702 to
25ba8ba
Compare
TylerHillery
force-pushed
the
tyler/feat/object-versioning-wave-3
branch
from
August 31, 2026 19:09
25ba8ba to
a549f6d
Compare
TylerHillery
force-pushed
the
tyler/feat/object-versioning-wave-3
branch
from
August 31, 2026 20:54
a549f6d to
0ab0a0f
Compare
TylerHillery
changed the base branch from
tyler/feat/object-versioning-wave-2
to
tyler/feat/object-versioning-wave-2a
August 31, 2026 20:57
Replace the module-level WeakMap that tracked which scope was applied to a transaction with a small holder created together with the transaction and shared, through the options, by every StoragePgDB bound to it (including asSuperUser copies). The state now lives exactly as long as the database objects bound to that transaction and is not reachable from anywhere else in the module. A transaction supplied from outside has no holder and keeps the conservative unknown-scope behaviour.
…free Four defects found by the object versioning fuzz suite. The database write now reports the row it replaced in place (the DISABLED current row, or the SUSPENDED null-version row, current or archived) through extra RETURNING columns whose scalar subqueries read the statement snapshot. Every writer frees those bytes through one helper (replacedContent): copy, move, single delete and bulk delete used to look only at the current row and leaked the bytes of an archived null-version row that a SUSPENDED write resurrected. The uploader no longer needs its separate locking lookup. Uploader.canUpload authorized writes by running the real archive UPDATE and INSERT in a rolled-back savepoint without the key's advisory lock. When a concurrent writer committed a new current row the probe collided with it on idx_objects_current_version and the upload or copy failed with ResourceAlreadyExists; the probe's two row locks, taken without the key lock, also closed lock cycles that Postgres resolved with a deadlock. The probe is now a single status-independent INSERT ... ON CONFLICT (current row) DO UPDATE (upsertObject with probe: true) that takes no bucket status lock, never touches the versioning flags and waits for a concurrent writer instead of failing. The uploader, copy and single delete take the bucket's shared status lock right after the advisory lock, the order move and bulk delete already used, and pass the status to their writes. authorizeMove probed the destination write before the source removal and treated "nothing deleted" as a policy rejection, so a same-path move of the null-version row under SUSPENDED (which rewrites that row first) and a move racing a delete of its source failed with AccessDenied. The source removal is probed first and a vanished row is reported as NoSuchKey. Copy and move read the source row without a lock and copy its bytes before locking; a concurrent hard delete surfaced the raw object store error. A missing source is now resolved again and the copy retried once with the version the row points at, otherwise NoSuchKey is reported.
resetMigration only deletes rows from storage.migrations, so a reset is a replay: the runner re-executes the files from that point. Two migrations were not safe to replay once a later migration had reverted part of them. storage-schema recreated the bucketid_objname unique index that drop-bucketid-objname-index removes, which fails on versioned duplicates or blocks versioned writes; object-versioning-core re-added the dark check that unlock-object-versioning drops, which fails on an ENABLED bucket or silently re-locks versioning. MIGRATION_RESET_FLOORS blocked such resets from a side table, keyed on rows still present in storage.migrations, so two successive per-tenant resets slipped past it, and the idempotency runner needed a flag to bypass it and skip those two files. Fix the migrations instead. storage-schema creates the legacy index only while the versioning transition function from unlock-object-versioning is absent, so tenants below the drop still get it and a replay on a version-aware schema leaves it dropped. object-versioning-core no longer adds the dark check: the application refuses to enable versioning before unlock-object-versioning, and that migration installs the transition trigger that guards status changes afterwards. Existing tenants pick up the new file hashes through the hash refresh. Remove reset-floor.ts, the skipResetFloorValidation option and the replay check in resetMigration, and let the idempotency script replay every migration again. Verified with a full replay on a tenant seeded with an ENABLED bucket and two versions of the same name, a fresh tenant frozen before the drop, and the before/after pg_dump diff of the CI check.
… start archived_at orders a key's versions: listings sort noncurrent versions by it and promotion after a hard delete picks the most recently archived row. Both archive UPDATEs used now(), which is the transaction start time. A writer that began earlier but reached the key lock later would archive with an older timestamp than a write that already committed, flipping the version order and promoting the wrong row. clock_timestamp() is taken when the statement runs, after the lock, so it follows lock order.
A delete without a versionId on an ENABLED or SUSPENDED bucket writes a delete marker row, but that row carried no owner or owner_id, so it was the only kind of row a principal could create anonymously. That breaks owner-scoped policies such as USING (owner = auth.uid()): the marker probe for a missing key fails its WITH CHECK, a later upload over the caller's own marker fails the ON CONFLICT DO UPDATE policy check on the marker, and the marker cannot be listed or deleted by owner. Thread the caller's owner from the HTTP routes and the S3 handler through ObjectStorage.deleteObject/deleteObjects and moveObject into the single and batched marker writes, including the SUSPENDED in-place rewrite of the not-versioned row.
…content The finish hook of a resumable upload can run more than once for the same version: a retried final PATCH reaches onUploadFinish again once the offset equals the size. completeUpload now checks the key inside the transaction and, when the current row already carries the incoming version, reports the upload as done without rewriting it. Before this a non-upsert replay threw KeyAlreadyExists and an ENABLED replay hit the key/version unique index, and both landed in the catch block, which scheduled the removal of that version's bytes: the live object lost its content. The catch block itself no longer trusts that a failure means the version was never committed. It looks the version up first and only removes the bytes of a version no row references; if the lookup fails it keeps them, since an orphaned version is recoverable and a current version without content is not.
Exercises the HTTP routes with a user token against a FOR ALL policy on owner = auth.uid(): deleting a missing key, bulk deleting missing keys, and uploading again over the caller's own delete marker. All three fail without the marker owner, with AccessDenied on the marker INSERT probe and on the upsert probe's ON CONFLICT DO UPDATE against the ownerless marker.
The UpsertObjectOptions flag that routes upsertObject to the rolled-back RLS permission probe is now named for the row it targets; the option's doc comment carries the full contract (probe shape, testPermission-only, never a versioning toggle for real writes).
The pre-copy authorize pass claimed to take no locks, but its RLS probes take row locks (source, then destination); two opposite concurrent moves acquire them in inverted order and can deadlock, as can a move racing a batch delete that covers both keys. The pass now enters the same advisory-lock gate every other writer uses before touching rows, scoped to the rolled-back testPermission transaction, so nothing stays locked across the backend copy.
Drop the inert pre-copy spread from the copy upsert so the input lists exactly the fields the write reads, and document which RLS policy governs a versionless delete: the DELETE policy for an existing key (versioning must not change who may delete), the INSERT policy for a marker on a missing one.
The 0002 replay guard keyed on the trigger function from 0073, but the bucketid_objname index is dropped by 0072, so a replay against a schema sitting between the two rebuilt the index: a transactional CREATE UNIQUE INDEX that ShareLocks storage.objects for the whole build, fails if any names are duplicated, and is dropped again when 0072 replays. Key the guard on idx_objects_current_version from 0066 instead - the earliest schema marker of the several-versions era, never dropped or renamed by a later migration. Verified by migrating a scratch database to 0072, rewinding the migrations bookkeeping, and replaying 0002: the old guard recreated the index, the new one skips it; a fresh install still creates it.
completeUpload queued the delete of the replaced row's bytes (and the created webhook) inside the write transaction. The queue lives in another database, so a rollback after the send left a delete in flight for a row that was still the live object, destroying its content. The transaction now only returns what was written; the replaced-bytes delete, the webhook and the success metric all run after commit. If a post-commit send fails the replaced bytes are merely orphaned, which is recoverable.
moveObject returned the transaction promise from inside the try block without awaiting it, so a rejected transaction (status change, existing destination, denied RLS) never reached the catch that removes the destination bytes copied before the transaction started, orphaning them in the backend. The test now pins that the cleanup event fires when the transaction is rejected.
copySourceContent retries a copy whose source bytes vanished by re-resolving the row, but copyObject discarded the re-resolved row and kept building the destination - both the backend copy's replacement metadata and the destination row - from the stale pre-copy source. The copy callback now receives the resolved row and re-derives the destination metadata from it, so the retry describes the bytes it actually copied. moveObject already used the returned row; only its callback signature changes.
findObjects and findObjectVersions ordered their locking reads by name in the default collation while findObjectTargets locks in name COLLATE "C" order. Two multi-row lockers sorting the same names under different collations can acquire row locks in inverted order and deadlock; the C collation also matches the objects indexes.
findObjectTargets combined its two target forms with an OR, and Postgres cannot BitmapOr an arm containing a subquery join: a batch mixing plain names and (name, version) pairs fell back to walking every row of the bucket (301k buffers vs 74 on a 300k-row bench bucket). Each form now resolves in its own UNION ALL subquery on its intended index and the outer statement locks the union by id, keeping the single deterministic (name, version) lock order. Single-form batches keep their direct predicate and plan unchanged.
Deleting a missing key on a versioned bucket still writes a delete marker (S3 parity), but authorizing it as the caller's INSERT broke the rule that DELETE policies govern deletes: delete-rights users could not delete missing keys, insert-only users could tombstone names they could never delete, and per-name verdicts needed a probe per name because a denied INSERT aborts the whole statement while the batch held every key's advisory lock. A DELETE policy only evaluates against rows, so the probe now materializes the markers as superuser inside the rolled-back testPermission transaction and asks, in one RLS-filtered statement, which of them the caller may delete. Per-name verdicts in a constant number of statements, the sequential probe loop is gone, and every delete - existing, versioned or missing - is governed by the same DELETE policy. A denied missing-key single delete now reports NoSuchKey instead of AccessDenied, matching what a caller without rights could already observe.
The S3 surface never read VersionId: a versioned GetObject or HeadObject silently served the current version, a versioned DeleteObject silently deleted or markered the current version, and a versionId on x-amz-copy-source was glued onto the source key. Now that versioned writes exist, acting on the wrong version is live misbehavior, so the routes parse versionId and the handlers reject it as NotSupported until version-addressed operations land: GetObject, HeadObject (backend and db variants), DeleteObject, per-entry DeleteObjects, CopyObject and UploadPartCopy sources.
Move dispatched the queued deletes of every byte range it freed (the replaced destination row, the marker-replaced null-version row, the source content) and both webhooks inside its transaction, and copy did the same for its replaced destination row: a rollback after a send left deletes in flight for rows the rollback restored. Both catch blocks also assumed any failure meant nothing committed, so a post-commit send failure would have deleted the committed destination's bytes. The transactions now only report what they freed; sends and webhooks run after commit, and the cleanup catch fires only while nothing has committed.
…load bytes copyObject and moveObject tracked a local `committed` boolean, set once their write transaction resolved, to decide whether a later failure could safely delete the bytes just copied to the destination. That flag isn't safe: a transaction can COMMIT on the server and still surface an error to the client (an acknowledgement lost after COMMIT, for instance), which would leave `committed` false while a row now references those bytes, deleting a live object's content. Both now reuse the same database probe completeUpload already relies on for this (extracted as the standalone isCommittedVersion), checking whether a row actually references the version before deleting it. completeUpload's post-commit queue send for a replaced version's bytes had no failure handling, unlike the webhook send next to it. A transient failure there (the write already committed) aborted the whole completion and reported an already-successful upload as failed to the caller. It now logs and swallows the failure the same way the webhook send does, since the orphaned bytes are recoverable.
getTenantCapabilities overwrote the application flag with the tenant feature flag in multitenant mode, so object_versioning reported false with STORAGE_VERSIONING_ENABLED=true. The enforcement path in versioning/guards.ts already treats the application flag as a global override, so the reported capability contradicted what writes allowed.
Rebasing onto master dropped the stub adaptations that previously lived in the merge commit, and master grew suites that predate them: - uploader, bucket mime-type and TUS upload-id fixtures stub hasMigration, which canUpload and completeUpload now consult - the UploadPartCopy CopySource cases go through storage.from(bucket), since the source lookup is routed through ObjectStorage - the S3 multipart MIME probe asserts the upsertObject options argument, which the permission probe now passes
completeUpload, copyObject and moveObject each opened a caller-role transaction wrapping a superuser one, for the sole purpose of running a second RLS probe next to the write. Every one of those probes repeats a check the same request already passed before any bytes moved: - completeUpload: prepareUpload for standard and S3 single-part writes, canUpload in onIncomingRequest on the final TUS PATCH, canUpload in completeMultiPartUpload. Signed TUS routes register dbSuperUser, so there the probe could not fail at all. - copyObject: the canUpload above the backend copy. - moveObject: the authorizeMove above the backend copy, whose inputs the locked pass already revalidates — a drift in the versioning statuses or the source row aborts the move with ResourceLocked instead of falling through to a second probe. The delete-marker state the probes read under lock can only have made the earlier probe stricter, never weaker: a key that gained a marker is one the caller holds INSERT rights over, and a key that gained a live row is rejected by the KeyAlreadyExists check next to it. The only coverage lost is evaluating policies against backend metadata rather than the declared metadata, which the pre-checks have never done. What it cost: a savepoint, two role switches, the probe write and a rollback per write, all inside the advisory key lock and the row locks, where they cap per-key throughput rather than just adding latency. The nested scope stays in deleteObject and deleteObjects, where the probe is the only DELETE-policy check and has to see the locked row.
ferhatelmas
reviewed
Oct 8, 2026
ferhatelmas
reviewed
Oct 8, 2026
Signed-off-by: Ferhat Elmas <elmas.ferhat@gmail.com>
ferhatelmas
approved these changes
Oct 9, 2026
5 tasks done
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.
What kind of change does this PR introduce?
Feature to factor in the bucket versioning status into object write and delete operations.
What is the current behavior?
Currently, a bucket's versioning status can only be
DISABLEDand is not factored into operations such as PUT or DELETE.What is the new behavior?
All object write and delete operations now factor in the bucket's versioning status. The status can be
DISABLED,ENABLED, orSUSPENDED.Once versioning is enabled, the bucket can never return to
DISABLEDbecause enabling it permanently changes how certain operations are performed. Distinguishing a bucket that has never had versioning enabled is important because it requires less bookkeeping.For example, in the
SUSPENDEDstate, a PUT on an existing key still preserves the current object if it hasis_versionedset totrue, meaning it was created while the bucket wasENABLED. In theDISABLEDstate, we know the current object could never be versioned, so we can continue using the normal upsert behavior instead of archiving and inserting.Here are the main changes for each status:
ENABLED
PUT: We update the current version's
archived_atand insert a new row with a differentversion. I refer to this as archive and insert.DELETE: This follows the same archive-and-insert flow as PUT, but the newly inserted row has
is_delete_markerset totrue.SUSPENDED
PUT: This is similar to the archive-and-insert flow, with one key distinction. We archive the current row and then upsert on
(bucket_id, name) WHERE NOT is_versioned. If a null version already exists for this object, it is replaced rather than retained as another historical version.DELETE: This follows the same archive-and-upsert flow as PUT, but the null version becomes a delete marker. This is another important distinction between
SUSPENDEDandDISABLED, sinceDISABLEDdoes not create delete-marker rows.Additional context
archived_attonull.is_versionedset tofalseis not preserved when another PUT or DELETE occurs unless the bucket is currentlyENABLED.