Reproduce the cross-worker drop_table race behind the blob lifecycle CI failures as a skipped unit test (blocked on rocksdb-js#806) - #2456
Merged
Conversation
…oad result Scope.handleEntry's wrapper removed a rejected async operation from pendingOperations as soon as it settled, so a failure that settled before the entry handler's `ready` event was gone by the time the initial-load result was computed: waitForInitialLoads() resolved, the component reported a successful load, and the rejection was left with no handler (the "unhandledRejection in worker thread" lines when a blob table's interrupted drop could not be completed, harper#1381). Record the first failure seen before the initial load settles and fail the load with it after every pending operation has finished; never leave the bookkeeping chains rejecting on their own. Also adds a (skipped) worker-thread regression test that reproduces the drop_table race behind the blob.test.mjs failures: a cross-worker source-fill commit landing on a just-dropped column family latches a fatal RocksDB background error under the binding's default parallel OCC validation. Refs #1381 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R38MPckiquqdMqYbn96qGD
…ch's main landed the same entry-handler failure handling in #2432 (operations retained until the ready drain, allSettled, two-branch cleanup), with tests for the pre-ready rejection and the unhandled-rejection leak that this branch's Scope change and test guarded, so both resolve to main's versions. What remains on the branch is the skipped cross-worker drop_table race reproducer for harper#1381, which waits on rocksdb-js#806. Refs #1381 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013yFRAueahXUwcVM5BjKPMq
… whole catalog prefix From the planning review: a fixed 50 ms wait after dropTable() could let an iteration end before the worker's late commit settled. The fixture now reports commit-settled once getFromSource releases the record lock (the seam caching.test.js already waits on), the test waits for that before probing, unexpected worker events include unhandled rejections again, and the catalog assertion scans every row under the table's prefix instead of the primary row alone. Comment trimmed to the invariant, the skip condition and the issues. Refs #1381 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013yFRAueahXUwcVM5BjKPMq
…, and validate worker errors From the round-1 review: the suite now returns early under HARPER_STORAGE_ENGINE=lmdb (no column families to drop there), an always-on control drops the table after the worker's commit has settled so the fixture's settlement signal, probe write and catalog check run on every resources run, the worker reports whether its commit was still in flight when the drop started and the race test asserts at least one iteration caught it, and the only worker error the race test tolerates is a commit that lost to the drop before reaching RocksDB's write path. The worker fixture calls setMainIsWorker like the other thread fixtures. Refs #1381 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013yFRAueahXUwcVM5BjKPMq
…and reset worker state From the adjudicated review: the control now round-trips through the worker before asserting on its events, so anything it reported because of the drop has crossed the thread boundary; each test discards what the worker reported before it began; the race test samples the shared record lock on the dropping thread right before the drop instead of trusting a worker-side sample that predates the message hop; setMainIsWorker is restored after the suite; two narrating comments are gone. Refs #1381 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013yFRAueahXUwcVM5BjKPMq
…the probe writes directly Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01St6cyvN2KtgxeaboFBFcHN
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.
drop_tableon one worker can drop a table's column families while another worker's source-fill cache write to that table is still committing:getFromSourceresolves its caller before the write commits, anddropTable()drains only its own thread's in-flight source commits. Under rocksdb-js's default parallel OCC validation the racing commit is admitted past conflict validation, fails inside RocksDB's memtable inserter, and RocksDB latches that failure as a fatal background error on the whole environment, so every later write to that database fails withInvalid column family specified in write batch: the drop's own catalog cleanup (the 500 inintegrationTests/apiTests/blob.test.mjs), every worker's interrupted-drop completion (Failed to complete interrupted drop … attempt 1), and the restarted worker's component load (theClass extends value undefinedcascade, becausedatabases.blob.BlobCachenever loads). CI runs 33076221779 and 33139269321; the root cause with RocksDB's own log timeline is on harper#1381, and the binding defect is rocksdb-js#806, which reproduces it with no Harper code at all. So the invalid column family is not a stale handle in Harper's drop bookkeeping: it is a real write to a really-dropped family, admitted below Harper.This PR is test-only. It adds the deterministic in-process reproducer as a skipped unit test with the reason in place: a worker thread source-fills a blob record (the blob write is what delays the native commit) while the main thread drops the table. Unskipped, it fails on iteration 0 with the CI signature on the rocksdb-js 2.8.0 pin main carries today. Beside it, an always-on control drops the table only after the worker's commit has settled, so the fixture's settlement signal, probe write and catalog check run on every resources run rather than waiting for the unskip. It changes no production code: the fix is the drop/commit interlock in rocksdb-js, after which harper bumps the pin and unskips the race test; acceptance for harper#1381 closes there, not here. The
unhandledRejection in worker threadlines in the same runs were a second, independent defect inScope.handleEntry; that fix landed on main in #2432, and this branch's version of it was resolved to main's in the merge commit, so squash on merge: the net change is the two test files.For the human reviewer
Framing-Verdict: better-alternative-existsfrom the planning review. The reviewer agrees the fix belongs in rocksdb-js and that the option set spans the axes (binding interlock; Harper-wide quiescence, the parked Quiesce workers before dropping RocksDB tables #2206;kValidateSerial; 2.8.0 detect-and-resume()). The alternative it names refines the interlock's lifecycle rather than its layer: take per-column-family admission at native commit time, all families in the batch all-or-nothing, released by native scope exit on every path, instead of a touch taken at transactionalPut/Deletestaging as rocksdb-js#806's first fix direction describes, because a staging-time touch deadlocks a same-threaddropSync()(Harper drops under a synchronous exclusive schema section). Nothing in this PR implements an interlock, so the refinement is recorded on rocksdb-js#806 for the binding task rather than adopted here. What you decide: whether this test-only PR lands now or waits for the binding release; the task owner chose now so the reproducer is in the tree.it.skipwith the reason and both issues in the comment above it. Alternatives: delete it until the binding ships, or make it self-enabling on a binding-version probe. Rejected because the skip has no automatic revival trigger either way and the version is not known yet; the unskip is part of the pin-bump PR, and rocksdb-js#806 links here so it is not forgotten. An active test asserting today's failure signature was also rejected: it would assert the bug and fail in the wrong direction the moment the binding fixes it. Reversal costs deleting two files.startWorker, not the plainnew Workerthe otherunitTests/resources/*-thread.jsfixtures use.dropTable()broadcasts a drop to every registered worker thread, each of which holds its own handle to the same column family, and the worker needs to receive that broadcast to behave like a production worker; the cost is production thread-management state in the shared mocha process, which the full resources suite tolerated on every run. Switching later means rewriting the fixture's message protocol.CONTAINED_COMMIT_LOSS). This couples the assertion to log wording that is not API; the alternative, asserting only on observables the test already has, was rejected because an unrelated failed source fill would release the record lock and let the race test pass vacuously. Where to look hardest: the pattern names today's contained error (Could not access column family N) and a plausible post-interlock one; the unskip should pin it to whatever the binding actually reports.commit-settledonceprimaryStore.hasLock(id)clears, the seamunitTests/resources/caching.test.jsalready waits on, andgetFromSourcereleases that lock in both branches of its commit handler. The race test samples the same lock on the dropping thread immediately beforedropTable(); that is a sample, so it shows the race was exercised at least once and cannot show that a given iteration overlapped. The deterministic overlap proof belongs to the binding's native tests (rocksdb-js#806).sync()is an arrival barrier, not quiescence. A ping/pong round-trip proves everything the worker posted before answering has arrived; it cannot prove the drop will provoke nothing later. That is enough for the control (nothing is in flight when it drops) and the race test usescommit-settledinstead; the helper is local to this file and its comment says what it guarantees.expect()on the worker'serror/exitwith the exit code, where a crashed worker used to hang to the 120 s mocha timeout with no cause attached. Messages the worker posted before dying are still delivered first, becauseafterterminates the worker deliberately and that exit must not read as a crash; the cost is that a crash after a test's final barrier is invisible to the fixture, which is why the main-thread probe write stays as the independent check. A reviewer who values fail-fast over delivery order can swap the two checks innext().Verification
Route (b): the reproducer is the end-to-end artifact. It cannot run green until rocksdb-js#806 ships, so the evidence is fails-on-current-binding for the race test and a passing control on every run.
trueon iteration 0, and the iteration fails with the CI signature. Worker log:Error committing cache update … Invalid column family specified in write batch; test failure:Error: Remove failed: Invalid argument: Invalid column family specified in write batchfromdropTable()'s catalog cleanup.1 passing, 1 pending(the control passes with no worker event at all; the race test is pending). UnderHARPER_STORAGE_ENGINE=lmdb: no tests registered.npm run test:unit:resourceson the final HEAD: 1923 passing, 28 pending (the race test among them), exit 0; the suite was also run on each earlier review-round commit with the same result.expect()reject withfixture worker exited with code 1immediately; the scratch test was not committed.prettier --checkandoxlint --deny-warningsclean on both files.Format Checkjob is main's, not this PR's:unitTests/resources/query-array-scoping.test.js(from Pin element-scoping semantics of queries over array-valued properties #2437, untouched and byte-identical here) failsprettier --checkon main itself, and main's own Format Check runs have been red since that commit. Left for a one-file format fix on main rather than widening this PR.Refs #1381
Complexity: medium
— Claude Fable 5.1
🤖 Generated with Claude Code
https://claude.ai/code/session_013yFRAueahXUwcVM5BjKPMq
https://claude.ai/code/session_01St6cyvN2KtgxeaboFBFcHN
Review-Coverage: authored=claude; ran=codex; adjudicated=domain; declined=gemini,cursor-grok,cursor-composer; rounds=4 @ d513d70
Human-Review-Need: 3 (decisions: shipping-a-skipped-reproducer, control-test-as-the-shipped-coverage, death-does-not-preempt-queued-events, sync-is-liveness-not-quiescence, fixture-uses-startworker, worker-error-capture-by-log-interception) @ d513d70