Skip to content

Quiesce workers before dropping RocksDB tables - #2206

Draft
kriszyp wants to merge 40 commits into
mainfrom
fix/idempotent-interrupted-drop-recovery
Draft

Quiesce workers before dropping RocksDB tables#2206
kriszyp wants to merge 40 commits into
mainfrom
fix/idempotent-interrupted-drop-recovery

Conversation

@kriszyp

@kriszyp kriszyp commented Aug 18, 2026

Copy link
Copy Markdown
Member

Problem

#1381 — Cross-worker write can race RocksDB table drop and poison catalog cleanup is an ordering failure, not an already-absent-column-family cleanup failure.

The failing integration run showed one worker dropping a table while another worker committed a background source-cache write through a retained handle. RocksDB rejected the late batch with Invalid column family specified in write batch; that shared write-path failure then prevented catalog cleanup.

The fix must establish this invariant before dropSync() runs:

Every thread that could have opened a table handle has stopped admission, settled all submitted work, and released that handle.

Fix

This PR adds a generation-stamped, cross-worker quiescence barrier for shared RocksDB table drops.

  • Persist dropping, dropGeneration, dropQuiesced, and a random process-incarnation marker in the primary catalog row.
  • Start the coordinator drain and strict peer barrier together. A coordinator failure can no longer leave peers acknowledging writes that restart recovery would later delete.
  • Publish barrier readiness atomically at the centralized RocksDB open boundary. Workers that have not opened storage can be skipped; a worker with a handle cannot be skipped because of event-loop timing.
  • Stop new table reads and writes, cancel resumable scans/subscriptions, and drain source-cache commits, staged transactions, native read iterators, cleanup/audit/history work, ID allocation, eviction, and index backfills.
  • Track every submitted index/catalog write and drain it on normal completion, table-drop cancellation, and worker-generation interruption.
  • Close remote handles before acknowledgement. Worker-originated barriers relay through main while excluding the already-prepared origin worker, preserving coordinator handles shared by a database alias.
  • Under the existing update-attributes lock, revalidate the generation, set dropQuiesced, synchronously drop the column families, then remove only the matching catalog generation.
  • On restart, complete a quiesced tombstone immediately. Defer an unquiesced tombstone while the recorded process incarnation can still own handles.
  • Retain a table class after a failed preparation so the next strict barrier can retry and close handles even after reconciliation removes the class from the live schema; release it after a successful retry.
  • Preserve LMDB behavior by limiting mid-scan cancellation to RocksDB, matching the RocksDB-only admission and drain protocol.

The supporting transaction hardening closes the drain boundary:

  • Reject a drop from its own active read/write transaction before tombstoning.
  • Track transactions touching each store without widening unrelated-table reads.
  • Abort every linked transaction after a terminal failure and release drain bookkeeping on every cleanup path, including synchronous native throws.
  • Preserve replay iterators and terminal error identity while preventing a cleared replay from retrying into false success.
  • Release eviction and audit-store references that could otherwise retain a dropped handle.

Human review decisions

Decision Current contract Review question
Failed drain/barrier Fail closed; table remains unavailable until a full Harper restart Is restart-only recovery acceptable for a routine DDL operation?
Drain budget Reuses the 10-second LOCK_TIMEOUT Should this be longer/configurable and aligned with transaction/large-blob budgets?
Worker exit A confirmed direct Worker exit counts as quiesced; an ambiguous sibling MessagePort close NACKs Does rocksdb-js guarantee that already-submitted native work cannot outlive a terminated JS worker?
Source apply ERR_TABLE_DROPPING rejects any write to the dropping store, including source-applied transactions How should replication order a table drop against a multi-table transaction? Core withholds sequence advancement on commit failure; harper-pro retry/stall behavior needs explicit review.
Transaction context A distinct explicit transaction context becomes the ambient AsyncLocalStorage context Is the nested authorization/audit-context semantic the desired public contract?
Barrier participants Job workers participate because they can own the same native handles Is the stricter availability cost preferable to excluding transient workers?
Recovery identity Tombstones persist a process-start UUID propagated through workerData/environment Is this the desired long-lived recovery identity format?

Two bounded review notes remain: a stuck-drop preparation intentionally retains its table class until the mandated restart, and write transactions pay one WeakRef/Set enrollment so the drain can find writes staged before the drop began. Gating enrollment only after a drop starts would miss exactly those pre-existing writes.

Validation

  • Focused quiescence/transaction/create/worker set: 87 passing, 1 pending.
  • Focused LMDB record-count and audit coverage: 23 passing, 5 pending.
  • unitTests/resources/dropTableQuiescence.test.js: real worker-thread coverage for local/remote reads, staged writes, source-cache writes, coordinator failure, NACK, worker exit, readiness, aliases, interrupted recovery, and every direct operation token.
  • integrationTests/apiTests/blob.test.mjs: 22 passing.
  • Bun integrationTests/apiTests/delete.test.mjs: 76 passing, including the numeric-string table drop from the original CI failure.
  • Latest full resource run: 1,586 passing, 15 pending, 3 local-only failures. Those three are the deliberately preserved working-copy mismatch: local node_modules/package-lock.json select structon@1.0.7, while the committed branch requires coherent structon@1.0.8. The protected local lockfile change is not committed or pushed.
  • npm run build, npm run lint:required, npm run format:check, and git diff --check: pass.
  • The full clean-install PR CI matrix at final c1384db15dc4 passed for Node 22/24/26, Bun, Windows, uWS, and the downstream Next.js adapter. Two unrelated Windows dependency-child hangs and one apt-mirror stall passed on bounded isolated reruns; no task-related CI failure remains.

Review coverage

GPT-5 Codex authored the changes. Nineteen pre-push rounds used an independent Claude graded reviewer plus Harper-domain adjudication; major fixes restarted whole-diff review, and the exact c1384db15dc4 engine-scope delta was re-reviewed successfully. The LMDB-scan finding is resolved. The remaining major labels are the explicit restart/timeout and source-apply policy decisions above. Gemini and both Cursor legs were pruned from the final exact-SHA run.

Related work

This supersedes #2168 — Fix interrupted RocksDB table drops, which addresses the same race through a different design. Its dependency on rocksdb-js #787 — Serialize database destruction with concurrent opens concerns database destroy/open serialization; this PR handles live column-family handles before an individual table drop.

This also narrows #1276 — Follow-up: residual partial-failure risks in table-drop hardening by generation-guarding catalog cleanup. It does not claim to solve every broader create/drop rollback policy.

Refs #1381

Supersedes #2168

Generated by GPT-5 Codex.

Review-Coverage: authored=unknown; ran=none; rounds=1 @ d29ccf8

Human-Review-Need: 4 @ d29ccf8

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request implements a robust table drop quiescence mechanism to safely coordinate table drops across multiple worker threads. It ensures that all in-flight operations, active write transactions, and read iterators against the target table are fully drained and closed before any destructive column family drops occur. This is achieved by introducing strict inter-thread communication (ITC) barriers and tracking process instance IDs to handle restarts safely. Comprehensive unit tests are added to verify this quiescence behavior. The review feedback suggests improving the robustness of the test teardown blocks in the new test file by declaring worker variables outside the try blocks and guarding their cleanup in finally blocks with existence checks and error catching to prevent flakiness or error masking during partial startups.

Comment thread unitTests/resources/dropTableQuiescence.test.js
@claude

claude Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

@kriszyp
kriszyp force-pushed the fix/idempotent-interrupted-drop-recovery branch from c1384db to 890164b Compare August 19, 2026 23:18
Comment thread resources/Table.ts
entriesScanned++;
await rest();
if (reverseScanned >= limit) break;
if (isRocksDB && droppingTable) throw tableDroppingError();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion (non-blocking): This isRocksDB && guard (added across 7 mid-scan cancellation points in this push, e.g. lines 4980, 5020, 5347, 5358, 5398, 5409, 5432) correctly restores pre-existing LMDB behavior — good fix. But unitTests/resources/dropTableQuiescence.test.js skips entirely under HARPER_STORAGE_ENGINE=lmdb (line 117), and no other test exercises "an LMDB table scan continues uninterrupted while a drop is in progress." Since this exact regression (RocksDB-only quiescence checks leaking into the LMDB scan path) was just found and fixed here, a small regression test running an LMDB scan concurrently with dropTable() and asserting it completes without ERR_TABLE_DROPPING would guard against it recurring.

@kriszyp
kriszyp force-pushed the fix/idempotent-interrupted-drop-recovery branch from 890164b to d29ccf8 Compare August 20, 2026 12:40
kriszyp and others added 22 commits August 20, 2026 08:34
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Track transaction-less table scans and clears so drop waits for their native handles. Gate strict worker broadcasts on ITC readiness and reject malformed events instead of acknowledging them.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Cancel client-paced and abandoned direct scans when a RocksDB table begins dropping, while retaining labeled drain tokens for diagnostics. Scope ITC validation to owned events and publish worker readiness through an atomic signal so coordinator observation cannot lag store opening.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Reject a post-drop next() before the generator can re-enter its closed native iterator, and document the tracked-iterator attribution invariant.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Avoid sending the internal readiness envelope to raw Node workers, where it can be mistaken for an application reply. Keep resource-test worker fixtures safe when Mocha imports them without a parent port.

Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
kriszyp and others added 18 commits August 20, 2026 08:35
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
@kriszyp
kriszyp force-pushed the fix/idempotent-interrupted-drop-recovery branch from d29ccf8 to b558fb1 Compare August 20, 2026 14:37
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