Repository navigation
Conversation
itslenny
added this pull request to stack #1355
September 21, 2026 22:00
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
itslenny
force-pushed
the
lenny/lifecycle-log-fixes-after-versioning
branch
from
September 22, 2026 14:50
468291e to
c1ab0d1
Compare
Every superuser statement issued inside a request-scoped transaction was wrapped in its own SAVEPOINT, scope switch, scope restore and RELEASE, and every nested withTransaction re-applied the scope of the enclosing role. Track the scope applied to each transaction so a nested unit only switches when the transaction is scoped to another role, queries issued inside a superuser unit run without per-statement isolation, and scope restoration targets the scope that was actually active before the switch (a doubly elevated unit previously left the elevated scope behind). Rolling back to a savepoint reverts transaction-local settings, so the tracker is reset to the savepoint state on rollback.
Add waitObjectLocks to the database adapter. It deduplicates the keys, sorts them by advisory-lock hash so overlapping batches can never deadlock each other, and acquires all of them in a single round trip using the same lock_timeout CTE shape as waitObjectLock (or the top-level statements on Multigres). The statement aggregates the lock calls so every key is locked before lock_timeout is restored, and the caller verifies the acquired count.
…ints Add findBucketsById (one statement, ascending id order, which is also the lock order for FOR SHARE/FOR UPDATE reads) and findObjectTargets (current rows for a list of names plus exact rows for a list of (name, version) pairs in one statement ordered by (name, version), so a locking read takes its row locks in one deterministic order across both target forms). upsertObject, deleteObject and deleteObjects accept a versioningStatus hint. A caller that already read the bucket status under its shared lock in the current transaction passes it so the write skips its own status lock and read.
Promotion picked the newest remaining row in a plain subquery. If that candidate was being hard-deleted by a concurrent transaction, the UPDATE waited on the row, found it gone once the other transaction committed and updated nothing, leaving the key with archived rows but no current one. Lock the candidate inside the subquery (single delete) or a LATERAL subquery (bulk delete) so a deleted candidate is skipped and the next remaining version is promoted instead.
…get read Split deleteObjects into partition, lock, authorize, apply and cleanup steps. Each batch now takes one batched advisory lock statement for all keys, reads the bucket status once under its shared lock, and reads the current rows and exact version rows in one FOR UPDATE statement ordered by (name, version). The status is passed as a hint to the writes so they skip their own status lock. The superuser work runs in one scoped unit. Deleting a missing key on a versioned bucket writes a delete marker, an INSERT under RLS whose policy violation throws instead of filtering rows. That error used to abort the whole batch, including authorized keys. Missing names are now probed together first and, only when that probe is rejected, one by one, so a rejection drops just that key. A bulk delete of 500 existing keys on an ENABLED bucket goes from 2,524 statements to 17, independent of the batch size.
… one deleteObject took the key-level advisory lock for every delete, so all version deletes of one key serialized with each other and with writers. Deleting a non-current version changes nothing the current row depends on, so it now locks only that version. Deleting the current row (no versionId, or the versionId of the current version) still takes the key lock, because writing a delete marker or promoting the next version changes what is current. The lock scope is decided from an unlocked read and verified under the row lock; a state change in between rejects the delete with ResourceLocked rather than proceeding under the wrong lock. The superuser work now runs in one scoped unit.
The pre-copy authorization pass no longer takes advisory locks or bucket share locks: the post-copy pass re-reads everything under the final locks and rejects the move if the status snapshot or the source row changed, so the preflight only needs consistent reads. The post-copy pass takes both key locks in one batched statement and reads both bucket statuses in one FOR SHARE statement ordered by id, and passes the statuses to the destination upsert and the source delete so they skip their own status locks. The superuser work runs in one scoped unit. A same-bucket move on an ENABLED bucket goes from 96 statements to 43.
…upload units authorizeUpload took the current-row delete-marker state as a positional boolean. Make it an optional field of CanUploadOptions: canUpload looks it up only when the caller did not supply it, and completeUpload and copyObject, which already hold the current row, pass it along. Both run their superuser work in one scoped unit, so the statements inside no longer pay a savepoint and scope switch each. An ENABLED upsert goes from 43 statements to 28.
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
itslenny
force-pushed
the
lenny/lifecycle-log-fixes-after-versioning
branch
from
October 8, 2026 14:30
c1ab0d1 to
e469b06
Compare
Coverage Report for CI Build 37792990870Warning No base build found for commit Coverage: 85.221%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
This branch has not been deployed
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
4 participants
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?
Bug fix
What is the current behavior?
The lifecycle event logs used for usage tracking have accuracy issues:
ObjectCreated:Put/ObjectCreated:Copydoes not include old object size when overwriting an existing objectWhat is the new behavior?
ObjectCreated:Put/ObjectCreated:Copyadd an oldObject field with the replaced object's size. This lets usage be correctly attributed.ObjectRemoved:Deletewhen a move operation causes an object deletion (e.g. restore version)Lifecycle log change
This is an example of a lifecycle log diff after this change including
oldObjectif there is an object being overwritten{ "tenant": { "ref": "PROJECT_REF" }, "name": "obj-name.txt", "version": "UUID", "bucketId": "bucket-name", "metadata": { "httpStatusCode": 200, "cacheControl": "no-cache", "eTag": "ETAG", "mimetype": "text/plain", "contentLength": 56, "lastModified": "TIMESTAMP", "size": 56 }, "reqId": "REQID", "uploadType": "standard", + "oldObject": { + "name": "obj-name.txt", + "bucketId": "bucket-name", + "version": "UUID", + "metadata": { + "eTag": "ETAG", + "size": 30, + "mimetype": "text/plain", + "cacheControl": "no-cache", + "lastModified": "TIMESTAMP", + "contentLength": 30, + "httpStatusCode": 200 + }, "reqId": "REQID" } }