fix(connections): finish the drop and delete lifecycle the catalog adoption started - #3033
Merged
Merged
Conversation
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.
The rest of the drop and delete lifecycle, reported on #3029 and #3032 rather than fixed there.
Four separate defects in one change, because they are one subsystem and three of them are two lines each. The fifth item on that list is not here, and the last section says why.
A column resized moments before its table was dropped came back
CatalogChangeService.recordDroppedTablesclears the table's saved settings and then sends.dropped, which closes its tab. Every subscriber chains.receive(on: RunLoop.main), so the teardown always lands a run-loop turn later, andDataGridView.dismantleNSViewcallsflushPendingColumnLayoutPersistence()on the way out. A column width still inside its 500ms debounce was written back after the clear, and re-marked dirty for sync, waiting for a table recreated with the same name.closeTabsForRemovedObjectsnow discards the pending write instead. That mirrorscloseTabsByUser, which flushes at the same point for the opposite reason: there the user closed a tab whose table still exists, so the write is wanted.Reordering the adopt and the send was considered and is a no-op:
send()does not block on delivery, so both finish before the deferred sink either way. A "refuse writes for this scope" gate in the stores was considered and rejected as the shim CLAUDE.md forbids, with no sound lifetime, since a recreated table's write is legitimate and indistinguishable from a stale one.A connection kept pointing at a database that was dropped
adoptContainerRenamecallsretargetSavedConnectionDatabase, whose doc comment says the saved default is what a reconnect and Reopen Last Session both use.adoptContainerDrophad no counterpart, so dropping the database a connection is configured for left every later connect failing on a database that cannot come back.A rename has a new name to point at. A drop has none, so the field is emptied, guarded on an exact match with the dropped name. MySQL connects with no default database, MongoDB picks one, PostgreSQL falls back to the connecting user's own name, and SQLite has no
dropDatabaseat all so the branch cannot fire for it. The argument against, stated plainly: for PostgreSQL this can trade one "does not exist" error for another if the user's own name is not a database either.Two things the review caught, both of which would have made this worse than the bug. The last database the session remembered has to go too, because
selectDatabaseFromLastSessionfires precisely when the saved default is empty: clearing one and leaving the other turns that action on and aims it at the database that was just dropped, so MySQL, MariaDB, ClickHouse, MSSQL, TiDB and OceanBase would try to switch to it on every connect, forever. And a type whose form requires a database value must not be emptied at all: Snowflake is.apiOnlywith database switching and nohidesBuiltInDatabase,DROP DATABASEis ordinary Snowflake DDL, and the saved connection would have been left failing the form's own validation with nothing on screen saying why.That predicate now lives in one place.
ConnectionDatabaseRequirementowns it and the connection form reads it too, rather than the adoption carrying a second copy that could drift from the one the form validates against.CatalogEditAdoptiontakes itsConnectionStorageandAppSettingsStoragenow, defaulted to.shared. That is what made this testable, and it brought the rename path its first test in the process.A remote connection delete pushed its own deletion back at the sender
purgeFavoritessplits on origin because, per theOrigindoc comment, a remote delete must not leave tombstones or it pushes a deletion back at the device that sent it.purgeAsyncStores, added in #3032, did not, andSQLFavoriteManager.removeFavoritesAndFolderstombstones every record it removes.This is live, not theoretical.
SyncCoordinator.applyRemoteChangessetschangeTracker.isSuppressedfor the length of its own synchronous body and resets it in adefer.purgefires aTask, which starts after that body returns, so the suppression is already off by the time the favorites are removed.removeFavoritesAndFoldersWithoutSync(for:)mirrorsFavoriteTablesStorage's existing split, andpurgeAsyncStorestakesoriginwith no default, so every caller states which it is.The review found the same defect two lines above, in the loop that purges the table-scoped stores:
FileColumnLayoutPersister.purgeConnectionsends inmarkDeleted, so a remote delete tombstoned every synced settings record as well.purgeConnectionstakesleavesTombstonesnow; only the synced store branches on it.Skipping the tombstone is not enough on its own either.
markDeletedis also what clears the dirty mark, so a record that was dirty when the other device deleted it kept its id forever, and every later push looked for a record that was gone and skipped it.SyncChangeTracker.discardDirtydrains those ids without tombstoning, and both without-sync paths call it.The guard that should have caught all of this
everyTableScopedStoreIsRegisteredscanned one directory forclassdeclarations mentioning one of three sentinel type names.FavoriteTablesStoragekeys on its ownFavoriteEntrystruct and names none of them, so it was invisible, which is how a dropped table went on leaving its star behind with nothing failing.It finds a store by the shape of its key now: a struct of at most six stored properties carrying both
connectionIdanddatabase, and any type that takes one in a function signature and writes. On this tree that is exactly the five registered stores,ForeignKeyLabelColumnStore, and the two Favorites stores, with nothing else.The two Favorites stores are exempt, with the reason in the map: their lifecycle turns on the local-or-remote origin that
TableScopedSettingsStoredoes not carry, soConnectionLocalStateandCatalogEditAdoptiondrive them by name. An exemption is now a line in a diff someone has to justify rather than a silent blind spot, and a second test fails if one goes stale.Two passes were needed to get the detector honest, and both failures are worth recording. It first matched key types as substrings, so the nested
Keyof one cache matched everyforKey:parameter in the app and the scan demanded conformance from twenty-two types that persist nothing table-scoped. It then gated on a file merely mentioning a persistence type, which caught a view model whose display target happens to carry a connection and a database. It matches whole identifiers and requires an actual write now. The guard's own detector had, briefly, the exact defect class it exists to catch.Not here: a raw SQL DROP still leaves its settings
DROP TABLE orders;typed in the editor refreshes the sidebar and clears nothing, while the sidebar's own Drop Table clears everything. Wiring.statementsRanto the adoption path is unsafe, and the code says so itself.MainContentCoordinator.postStatementRan:It is worse than that.
MultiStatementFailure.ranStatementCountcounts the failing statement as ran, soDROP TABLE orders; SELECT * FROM missing;submitted together reportsordersas dropped even when the batch rolls back and the table is still there. Adopting on that signal deletes a user's saved filters, layouts and highlight rules for tables that exist.A safe version exists and is a PR of its own: parse candidate names with a dialect-aware
DropTargetParser, then confirm them against the reloaded catalog throughLoadedBrowseCatalog.staleRefsbefore adopting, which is the shapereconcileDropsAfterFailedSavealready uses for auto-committing-DDL engines. It costs a new per-dialect parser and a confirm-after-refresh step, and it would clear the settings one drain cycle later than the sidebar path does.Tests
152 pass across the ten suites the change touches, which includes every store whose
purgeConnectionssignature moved.New: a discarded debounce writes nothing even when a flush follows it, and the discard only fires when the closed tabs include the selected one, so dropping one table cannot throw away a resize in progress on another; the saved database is emptied on a matching drop, untouched on a different one, and untouched when already blank; the rename counterpart, which had shipped untested, is pinned beside it; a remote purge removes the SQL favorites and tombstones neither them nor their folders while draining their dirty marks, a local one tombstones both; a remote connection delete drops the saved column layouts without tombstoning them and a local one tombstones them; and the widened guard, with a second test that fails if an exemption stops naming a store the scan finds.
Verification
verify.sh build: PASSverify.sh testover ten suites: PASS, 152 executed, 152 passedverify.sh lintover every changed file: PASS, 0 violations