Fix interrupted RocksDB table drops - #2168
Conversation
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
There was a problem hiding this comment.
Code Review
This pull request improves the robustness of table dropping and promise rejection handling. It ensures that redundant or interrupted table drops on RocksDB are handled safely by verifying that all prefixed column families are completely removed before clearing catalog rows, and it prevents unhandled promise rejections during scope initial loads. Feedback on the changes suggests using the centralized openRocksDatabase helper in resources/Table.ts instead of instantiating RocksDatabase directly to ensure database configurations are consistently applied.
| 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(); |
There was a problem hiding this comment.
Bypassing the centralized openRocksDatabase helper and directly instantiating new RocksDatabase means that important configurations such as disableWAL, compression (and compressionForAllColumnFamilies), and readOnly mode are not applied to the opened column family store. To maintain consistency and ensure all database options are correctly respected, please export openRocksDatabase from resources/databases.ts, import it in resources/Table.ts, and use it here.
| const columnStore = new RocksDatabase(rootStore.path, { name: columnName }).open(); | |
| const columnStore = openRocksDatabase(rootStore.path, { name: columnName }); |
| // 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; |
There was a problem hiding this comment.
File: resources/Table.ts:406, 5984, 6109
What: tableIsDropping() adds a new OR branch — reading the persisted tombstone via dbisDb.getSync(tableName + '/')?.dropping — specifically so a different worker's drop (which never sets this worker's in-memory droppingTable) 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 the droppingTable leg, by calling dropTable() 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 a getFromSource-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 flips droppingTable true 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 (via sourcedFrom + get(), as Resource-get-context.test.js already does) to assert the cache write is skipped rather than staged.
| 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(); |
There was a problem hiding this comment.
Suggestion (non-blocking): This new orphaned-column-family sweep opens each column with new RocksDatabase(rootStore.path, { name: columnName }).open() directly — the only such direct use in this file. Every other RocksDB open in the codebase goes through openRocksDatabase() in resources/databases.ts, which applies WAL/compression settings and the read-only-mode guard (resources/databases.ts:286-341), and resources/databases.ts's own completeInterruptedDrop (which this block closely mirrors, including the new remainingColumnFamilies check) already uses it. Consider exporting openRocksDatabase and reusing it here (or extracting the whole drop-and-verify loop into one helper shared by dropTable() and completeInterruptedDrop()) instead of duplicating the column-sweep logic with a divergent open path.
|
Reviewed. One blocker: the new cross-worker leg of |
Fixes #1381.\n\nPrevents cross-worker source-cache writes from racing a table drop, sweeps surviving table column families before retiring tombstones, and handles async entry failures without unhandled rejections.\n\nTests: focused ghost/Scope suites; Blob lifecycle integration passed twice.