Skip to content

TC-735: persist KV blobs before the per-space sequence lock - #301

Merged
samgbafa merged 33 commits into
mainfrom
skgbafa/tc-735-prepersist-blobs
Oct 25, 2026
Merged

samgbafa merged 33 commits into
mainfrom
skgbafa/tc-735-prepersist-blobs

Conversation

@samgbafa

@samgbafa samgbafa commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Change

KV puts persist content-addressed blobs before the per-space sequence lock. A preparatory transaction authorizes the invocation, checks that every space it names is hosted, and claims each distinct blob under a durable attempt; it commits all claims or none. Uploads then hold no pooled database connection, transaction or lock. The commit transaction takes the sequence guard, verifies its own claim, re-authorizes, checks preconditions, assigns the sequence and commits the reference, so kv/sync cursors stay commit-ordered.

  • Claims: an active (space, content hash, generation) fence plus one claim per writer. The last unreferenced claim moves the fence to deleting; no writer can deduplicate or upload over it until physical absence is confirmed. Known failures refund once after deletion.
  • Generations: first uploads keep the bare hash key (generation zero); an upload after an orphan deletion uses hash@N.
  • Collector: every 60 seconds, five-minute database-time grace, bounded pages. It closes, without deleting, a deleting fence whose blob a committed write references.
  • Storage: per-(space, hash, generation) serialization of physical operations; filesystem staging publishes complete files atomically; S3 startup rebuilds pending refunds from the same listing as the quota count. Quota stays conservative and per process.
  • Protected legacy meeting writes keep the pre-upload freeze barrier (30 seconds).
  • Migration m20261023_010000_kv_orphan_candidates adds the generation columns and three claim tables with if_not_exists, so it can be applied again over its own tables on PostgreSQL and SQLite (MySQL index creation has no such guard).
  • Refusal order: frozen legacy space, authorization, unhosted space, pending deletion (503), then upload and the precondition (412). Authorization is checked before a conditional write's precondition.
  • A lost connection during the preparatory transaction that survives its retry answers 503 unavailable_retryable; the claims are idempotent per attempt ID and hold only candidate metadata.

Round trips (PostgreSQL, measured in CI)

sign-in 71, kv put 42, kv get 16, batch get 16, batch head 16, two-blob put 48, one-blob two-key put 45. The ceilings in postgres_round_trip_budget.rs equal these. The hosted-space check is part of each claim statement, so no separate query runs on a put with a blob.

Rollback

Recovery after deployment is a forward fix; an image rollback alone cannot boot, and a binary without this change is not a supported rollback target (docs/kv-sync.md, "Recovery after deploying generation-keyed blobs"). Take a named full database backup before deploying. The document holds the current_kv.generation repair statement and the self-aborting emergency script for the case where no record has a generation above zero.

Tests

New: all-or-nothing two-blob registration (registration level, invoke level and route level), a deleting fence over a referenced blob, migration re-run over populated tables (SQLite and PostgreSQL), bounded preparatory BEGIN retry with no retry after a transaction statement, multi-write round-trip rows. PostgreSQL-backed tests run in CI.

Details: TC-735

@samgbafa

Copy link
Copy Markdown
Contributor Author

Correction on current head 5f34b4d: the PostgreSQL-backed workspace CI timeout is a TC-735 write-path freeze-ack regression, not a proven TC-1176 read-audit issue. legacy_freeze_ack_waits_for_actual_in_flight_kv_storage_and_commit (SQLite) timed out because a freeze acknowledged while its test upload was paused, the assertion panicked before releasing that upload, and teardown hung. Origin/main passed the single test and freeze module 20/20 each; a new PG16 delayed-upload RED test observed the early acknowledgement directly. A local, not-yet-pushed fix acquires the durable freeze row before upload, keeps sequence locking after upload, bounds freeze attempts to 30s, and rejects post-freeze writes before uploading. Added failed-upload refund/GC regression. Focused PG16 freeze module 13/13 and 20/20 repeated runs passed. Full PG-backed workspace suite and fresh CI/review are still required before this PR is ready; testing pauses for the TC-1149 20:35–21:05Z exclusive host window. No read_audit or nonmutating KV read changes, no merge/deploy. TC-735 root cause. The ~93/180 same-space throughput targets remain unmet (separate pre-existing epoch-head bottleneck TC-1218).

@samgbafa

Copy link
Copy Markdown
Contributor Author

REQUEST_CHANGES: see TC-735

@samgbafa samgbafa left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

REQUEST_CHANGES — TC-735

@samgbafa

Copy link
Copy Markdown
Contributor Author

REQUEST_CHANGES: see TC-735

@samgbafa samgbafa left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

REQUEST_CHANGES — TC-735

@samgbafa

Copy link
Copy Markdown
Contributor Author

REQUEST_CHANGES: see TC-735

@samgbafa samgbafa left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

REQUEST_CHANGES — TC-735

@samgbafa

Copy link
Copy Markdown
Contributor Author

REQUEST_CHANGES: see TC-735

@samgbafa samgbafa left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

REQUEST_CHANGES — TC-735

@samgbafa

Copy link
Copy Markdown
Contributor Author

REQUEST_CHANGES: see TC-735

@samgbafa samgbafa left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

REQUEST_CHANGES — TC-735

@samgbafa

Copy link
Copy Markdown
Contributor Author

APPROVE: see TC-735

@samgbafa

Copy link
Copy Markdown
Contributor Author

REQUEST_CHANGES: see TC-735

@samgbafa samgbafa left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

REQUEST_CHANGES — TC-735

@samgbafa

Copy link
Copy Markdown
Contributor Author

APPROVE: see TC-735

@samgbafa samgbafa left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

REQUEST_CHANGES — TC-735

@samgbafa

Copy link
Copy Markdown
Contributor Author

APPROVE: see TC-735

Brings main a735c4d (TC-1071 CID-alias revocations, TC-1176 boot
lock_timeout, request telemetry) and #302 580abe5, whose batched auth
snapshot resolves alias revocations in its one statement.

Startup keeps the pre-S3-listing migration (now main's lock_timeout-
bounded migrate) so deleting fences are read before the quota listing,
then opens the database through main's new_with_encryption, which finds
migrations current and runs the CID-alias backfill with the column key
before serving requests.
TC-735's later kv_orphan_candidates migration made Migrator::down(1)
undo it instead of the cursor index. Identical to #341's fix.
#337 fixed S3 remove sizing with HeadObject; this branch already sizes
and deletes through HeadObject with generation keys, operation locks and
confirmed-absence refunds, so keep its remove and drop main's now-unused
helpers. Main's TC-1354 MinIO test uses generation 0 (the bare key).
Rename the unshipped kv_orphan_candidates migration from
m20261015_020000 to m20261017_020000 so it sorts after main's
m20261017_000000_epoch_order_space_index, per the second-merger rule.
Take main's cursor-index lock test (migrated_through), dropping ours.
CI aborted policy_owner_may_be_a_session_the_space_owner_delegated_to
on the 2 MiB test stack. gdb at the overflow: the deepest path is
invoke -> boxed commit block -> transact -> process_admitted -> save.
transact's debug frame (~188 KB) and the test's own frame (~936 KB)
are the same as main. This branch adds the boxed commit block's frame
(106,608 bytes) under the outer invoke frame. In debug builds every
awaited temporary in that block has its own stack slot.

Move the upload loop and the post-transact reads into their own
methods, so their temporaries are on the stack only while they run.
The block's frame is now 78,208 bytes. The test's minimum passing
stack falls from 2,056 KiB to 2,028 KiB (main: 2,032 KiB). The test
stack is unchanged.
dryrun added 15 commits October 23, 2026 23:02
Pre-upload authorization is one read statement and registration one write
statement that also checks every invocation space exists, replacing the
preparatory BEGIN/COMMIT and the separate hosted-space COUNT. Other backends
keep the preparatory transaction. Add round-trip rows for multi-blob and
shared-blob multipart puts; their budgets are placeholders until measured.
Route the PostgreSQL pre-upload authorization reads and the idempotent
registration statement through the pool's connection-loss recovery, as the
preparatory BEGIN was, without marking a user-visible write.
…cking

Set the KV put budget to the measured 41 and add exact rows for a two-blob and
a one-blob two-key multipart put. Cleanup of candidate metadata after a failed
write no longer marks the request as having possibly applied a write, so a
failure before the KV transaction began stays retryable.
The pool classifies the authorization snapshot's multi-CTE statement as a
possible write, which made a failure before any KV transaction unknown-outcome.
Run those reads through a view that retries once after a lost connection and
does not count as a write.
The PostgreSQL autocommit registration applies only to at most one distinct
blob; several blobs share one preparatory transaction that commits every claim
or none. The registration statement is a write: it is not retried and a lost
connection around it is an unknown outcome. The authorization view accepts
only pure reads.
…d-space check on PostgreSQL

The PostgreSQL autocommit registration cannot absorb a stale pooled connection
without retrying a write, which the pool's recovery tests rely on. Authorization
and every blob's claim share one transaction again, committing all claims or
none; each PostgreSQL claim statement also checks the invocation's spaces are
hosted, replacing the separate COUNT. The multi-write budget rows keep the
ceiling below the expected count until measured.
…ion; recovery notes

The collector checks references before removing a blob and closes a deleting
fence whose blob a committed write references. The claim-table migration uses
if-not-exists on every table and index. Add tests for both, for all-or-nothing
two-blob registration and for the bounded preparatory BEGIN retry; pin the
measured round-trip ceilings; document recovery as a forward fix.

Details: TC-735
…nvoke level all-or-nothing tests

Document the recovery script, the seven round trips of a put, and the
retryable classification of a lost connection during the preparatory
transaction. Add an invoke-level test and a route-level test that a two-blob
put with a pending deletion claims nothing.

Details: TC-735
@samgbafa

Copy link
Copy Markdown
Contributor Author

APPROVE: see TC-735 (reviewed exact head e54bf89)

@samgbafa

Copy link
Copy Markdown
Contributor Author

APPROVE: see TC-735 (reviewed exact head e1bfad0)
This head is a plain merge of main 5257602 into the approved head e54bf89; the non-CHANGELOG patch-id is unchanged (96c122a4…), behind main 0.

@samgbafa
samgbafa merged commit af5c213 into main Oct 25, 2026
22 checks passed
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