-
Notifications
You must be signed in to change notification settings - Fork 10
Fix interrupted RocksDB table drops #2168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -403,6 +403,7 @@ export function makeTable(options) { | |||||
| // new ones (droppingTable) once a drop has actually started. | ||||||
| const pendingSourceCommits = new Set<Promise<any>>(); | ||||||
| let droppingTable = false; | ||||||
| const tableIsDropping = () => droppingTable || (dbisDb as any)?.getSync?.(tableName + '/')?.dropping === true; | ||||||
| let createdTimeProperty: Attribute | undefined, | ||||||
| updatedTimeProperty: Attribute | undefined, | ||||||
| expiresAtProperty: Attribute | undefined; | ||||||
|
|
@@ -1475,21 +1476,32 @@ export function makeTable(options) { | |||||
| while (!rootStore.tryLock('update-attributes')) {} | ||||||
| let removed = false; | ||||||
| try { | ||||||
| for (const attribute of attributes) { | ||||||
| const index = indices[attribute.name]; | ||||||
| if (index) | ||||||
| const currentPrimary = (dbisDb as any).getSync(TableResource.tableName + '/'); | ||||||
| if (!currentPrimary?.dropping) { | ||||||
| removed = false; | ||||||
| } else { | ||||||
| const columnPrefix = TableResource.tableName + '/'; | ||||||
| for (const columnName of (rootStore as any).columns) { | ||||||
| if (!columnName.startsWith(columnPrefix)) continue; | ||||||
| const columnStore = new RocksDatabase(rootStore.path, { name: columnName }).open(); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bypassing the centralized
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion (non-blocking): This new orphaned-column-family sweep opens each column with |
||||||
| try { | ||||||
| index.dropSync(); | ||||||
| columnStore.dropSync(); | ||||||
| } catch (error) { | ||||||
| ignoreAlreadyDropped(error); | ||||||
| } finally { | ||||||
| columnStore.close(); | ||||||
| } | ||||||
| } | ||||||
| const remainingColumnFamilies = (rootStore as any).columns.filter((columnName) => | ||||||
| columnName.startsWith(columnPrefix) | ||||||
| ); | ||||||
| if (remainingColumnFamilies.length) { | ||||||
| throw new Error( | ||||||
| `Column families remain after drop of table ${databaseName}.${tableName}: ${remainingColumnFamilies.join(', ')}` | ||||||
| ); | ||||||
| } | ||||||
| removed = removeTombstonedCatalog(); | ||||||
| } | ||||||
| try { | ||||||
| primaryStore.dropSync(); | ||||||
| } catch (error) { | ||||||
| ignoreAlreadyDropped(error); | ||||||
| } | ||||||
| removed = removeTombstonedCatalog(); | ||||||
| } finally { | ||||||
| rootStore.unlock('update-attributes'); | ||||||
| } | ||||||
|
|
@@ -5969,6 +5981,10 @@ export function makeTable(options) { | |||||
| nodeName: 'source', | ||||||
| commit: (txnTime, existingEntry, _retry, transaction: any) => { | ||||||
| sourceWrite.skipped = false; // reset on each retry; cleanup happens after commit if still true | ||||||
| if (tableIsDropping()) { | ||||||
| sourceWrite.skipped = true; | ||||||
| return; | ||||||
| } | ||||||
| if (existingEntry?.version !== existingVersion) { | ||||||
| // don't do anything if the version has changed | ||||||
| sourceWrite.skipped = true; | ||||||
|
|
@@ -6090,7 +6106,7 @@ export function makeTable(options) { | |||||
| TableResource.userEmbedders | ||||||
| ); | ||||||
| if (embedBefore) await embedBefore(); | ||||||
| if (droppingTable) { | ||||||
| if (tableIsDropping()) { | ||||||
| // Re-check right before staging the write: dropTable() may have started | ||||||
| // while we were awaiting the embed step above (harper#1381). | ||||||
| sourceContext.transaction.abort(); | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File: resources/Table.ts:406, 5984, 6109
What:
tableIsDropping()adds a new OR branch — reading the persisted tombstone viadbisDb.getSync(tableName + '/')?.dropping— specifically so a different worker's drop (which never sets this worker's in-memorydroppingTable) is still caught before a source-cache write lands. This is the cross-worker race the PR description says it fixes ("Prevents cross-worker source-cache writes from racing a table drop"). Every existing test (dropTableGhost.test.js,Resource-get-context.test.js) only exercises thedroppingTableleg, by callingdropTable()in the same worker/process before triggering the write. None sets the tombstone directly (as several ghost tests already do, e.g. line 90-92) and then drives agetFromSource-style write through the commit callback (line 5984) or the pre-stage check (line 6109) to confirm it aborts.Why it matters: This is the exact "production" leg of a new two-branch guard, and it's the one the in-process tests structurally cannot reach (calling
dropTable()always also flipsdroppingTabletrue locally). Without a test for the persisted-only path, a regression here (e.g. wrong key, wrong table variable, or the check being dropped in a future refactor) would silently reintroduce the cross-worker write-after-drop race this PR sets out to close, undetected by the suite.Suggested fix: Add a test that, without calling
dropTable(), writes{ dropping: true }onto the table's primary catalog row (mirroring the existing ghost tests' tombstone-injection pattern) and then drives a source-populated write (viasourcedFrom+get(), asResource-get-context.test.jsalready does) to assert the cache write is skipped rather than staged.