feat(storage): add the pending drops quarantine ledger - #1064
Draft
aparajon wants to merge 1 commit into
Draft
Conversation
Records the tables a deployment moved into an engine's pending-drops quarantine, in that deployment's own storage. Discovery of servers holding expired quarantines becomes an index lookup rather than a scan, so cleanup cost scales with drops instead of with the number of registered databases, and a deployment that never quarantines writes no rows and no-ops by construction. Rows also carry the run identifier that performed the move, which is what lets an interrupted DROP phase tell its own completed rename apart from drift or from an earlier apply's quarantined copy. No caller yet: the quarantine path and the reaper are wired in follow-ups.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a durable “pending drops” quarantine ledger to storage so a deployment can record which tables it quarantined (including the apply run_id) and later distinguish “we renamed it earlier” from “it disappeared for some other reason.” This fits into SchemaBot’s storage layer and schema bootstrap/parity testing, providing the foundation for convergent re-runs of the DROP phase on the quarantine path.
Changes:
- Introduces
pending_dropsstorage model/types and aPendingDropStoreinterface, exposed viaStorage.PendingDrops(). - Implements the SQL-backed store (
Record,LatestForTable,ListExpired,ListQuarantined,SetState,Prune) in the shared sqlstore. - Adds MySQL/Postgres schema DDL plus cross-dialect parity tests for the new store.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/storage/types.go | Adds PendingDrop and PendingDropState types for the quarantine ledger. |
| pkg/storage/storage.go | Extends the public storage API with PendingDropStore and Storage.PendingDrops(). |
| pkg/storage/internal/sqlstore/storage.go | Wires the new pendingDropStore into the sqlstore Storage implementation. |
| pkg/storage/internal/sqlstore/pending_drops.go | Implements the SQL store for pending-drops ledger operations. |
| pkg/schema/mysql/pending_drops.sql | Adds MySQL DDL for the pending_drops table and indexes. |
| pkg/schema/postgres/pending_drops.sql | Adds Postgres DDL for the pending_drops table and indexes. |
| pkg/storage/storagetest/storagetest.go | Registers the new parity suite in the storage test harness. |
| pkg/storage/storagetest/pending_drops.go | Adds cross-dialect behavioral tests for PendingDropStore. |
| pkg/api/handlers_test.go | Updates the storage mock to satisfy the expanded storage.Storage interface. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Adds a
pending_dropstable and its storage accessor, so a deployment has a durable record of the tables it quarantined and can tell its own earlier rename apart from a table that vanished for some other reason.This PR has no consumer yet and should not merge ahead of one. It is the foundation for making a re-run of the DROP phase converge on the quarantine path: that fix needs the
run_idrecorded here as proof, because without it a resumed apply cannot distinguish a table it renamed itself from one another actor dropped. The equivalent fix on the direct path needs no ledger and ships in #1057.Design notes worth knowing at review time:
LatestForTabledeliberately does not filter by run id. The caller compares it, which keeps "no record at all" distinguishable from "an earlier apply's copy" — those fail closed for different reasons.Pruneonly touches rows that are no longer quarantined, so a reaped row stays as evidence until it ages out.original_tableisvarchar(64), the real table-identifier limit; at 255 the origin index exceeded MySQL's 3072-byte key limit.Tests live in the cross-dialect parity suite, so all 14 subtests run against both MySQL and PostgreSQL.
Authored by Claude Code (claude-opus-5).