Repository navigation
Add a restart regression test: dropped tables stay dropped and same-name recreates start empty - #2901
Conversation
Release cherry-pick
|
There was a problem hiding this comment.
Code Review
This pull request adds integration tests and updates design documentation to ensure that dropped tables remain dropped across restarts and that same-name recreations start empty. The feedback recommends guarding the after hook in the new test suite to prevent secondary TypeError exceptions from masking setup failures if the before hook fails.
| after(async () => { | ||
| await teardownHarper(ctx); | ||
| }); |
There was a problem hiding this comment.
In node:test, the after hook is executed even if the before hook fails. If setupHarperWithFixture fails in the before hook, ctx.harper will be undefined, and calling teardownHarper(ctx) will throw a secondary TypeError, masking the original failure. Guard the teardown call with a safety check on ctx.harper.
after(async () => {
if (ctx.harper) {
await teardownHarper(ctx);
}
});References
- In test teardown hooks (such as
afterorafterEach), ensure cleanup operations for temporary resources are guarded so they only execute if the resources were successfully initialized. Wrap independent teardown steps intry/finallyblocks to guarantee all cleanup executes and to prevent secondary errors from masking the original setup failure. - In node:test, after hooks are executed even if a before hook throws an error. Do not duplicate cleanup logic or wrap assertions in try...catch blocks inside before hooks solely to ensure cleanup.
…me-name recreate starts empty Runs a real Harper process through drop_table followed by a graceful restart or a SIGKILL, for three ways the table comes back: recreated in-process before the restart (the ghost-table incident flow), recreated with create_table after it, and recreated at boot from a component's schema.graphql. Each arm checks the primary store and a secondary index, writes a fresh row, and restarts again. The in-process arms fail on pre-#1246 core with rocksdb-js 2.0.0 ("Invalid column family specified in write batch") and pass from rocksdb-js 2.1.0 on. DESIGN.md records that the drop tombstone is node-local: a peer offline for the drop re-creates the table on reconnect (#1212). Refs #1212 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…stic Send SIGKILL directly instead of racing the SIGTERM handler's immediate exit, wait for a barrier row written just before each restart so reads never run ahead of the unawaited boot replay, time each arm separately, and scope the header and design note to acknowledged drops. Refs #1212 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ushed Refs #1212 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e in design notes Refs #1212 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2af50b7 to
7c253b9
Compare
Adds an integration test that drives a real Harper process through
drop_table, then a graceful restart or a SIGKILL, then a same-name recreate, and checks that no pre-drop row ever comes back. Refs #1212. It does not close that issue: the offline-peer case was reproduced on a cluster before main gained #2962 and #3119 and needs a harper-pro test to confirm it is fixed.For the human reviewer
drop_tablereturns, not during it.dropTableremoves itsdroppingtombstone and awaits the commit before returning, so this test never reachescompleteInterruptedDrop. An interrupted drop is covered only by the unit simulation inunitTests/resources/dropTableGhost.test.js. A fault-injection arm could be added later.replayLogs(resources/databases.ts:1201). Before each restart the test writes a barrier row, and after boot it waits until that row is visible. This relies on replay applying one database's log in order. If replay were ever parallelized, the barrier would stop working without any error. The alternative is for boot to expose a replay-complete signal.Refs, notCloses. This is a regression test; the fix for the cluster case is Stamp table generations and keep a durable drop marker so a peer that missed a replicated drop_table cannot bring the table back #2962 and Refuse a peer's retired table generation, and keep newer or node-local tables when a peer drops one #3119, which this PR does not touch.Changes
integrationTests/database/drop-table-restart-recreate.test.ts: the test matrix is three recreate paths × {graceful, SIGKILL}. The paths are an in-process recreate before the restart (the ghost-table incident flow from fix(schema): atomic table drops, interrupted-drop completion at startup, and create-lock release (ghost tables) #1246),create_tableafter the restart, and a componentschema.graphqlrecreating the table at boot. Each arm checks a primary scan and a secondary index, then writes one fresh row and restarts again. The restart helper sends SIGKILL directly, because Harper's SIGTERM handler exits immediately and would otherwise race the helper's timer. It also waits on the replay barrier.integrationTests/database/drop-table-restart-recreate/config.yamlandintegrationTests/database/drop-table-restart-recreate/schema.graphql: the fixture component. It holds the two boot-recreated tables with an indexedn, plus theReplayBarriertable.Verification
Test results and the cluster run
End-to-end route: the new integration test.
npm run test:integration -- integrationTests/database/drop-table-restart-recreate.test.ts7c06b8364(current main), rocksdb-js 2.10.0signalCode === 'SIGKILL'17a3f60d2) with rocksdb-js 2.0.0, which predates HarperFast/rocksdb-js#647Invalid column family specified in write batch(the ghost-table signature)Cluster run, not in this PR: harper-pro main
505ff332with core7c06b8364, 2 nodes, bidirectional, rows written on both.drop_tableon A (A reports Bfailed: ECONNREFUSED), B restartedNot run locally: the Bun runtime (not installed here), Windows, and the full
test:integration:all(CI runs them). prettier, oxlint andtsc -p integrationTestsare clean for the changed files.Rebased onto main at
7c253b96a: the earlierDESIGN.mdedits were dropped because main's lifecycle-stamp note supersedes them. The test passes 6/6 on the rebased build.Planning review:
Framing-Verdict: chosen-approach-sound.Signed: Claude Opus 5.5
Complexity: medium
Release target: v5.4 — new test files only; not on v5.3
Related PRs: #1246 overlaps, 2 others independent
🤖 Generated with Claude Code
Origin — the dispatch brief this PR was written from
LIVE CONVERSATION about #1212.
You are answering a person, in a thread, one turn at a time. Every turn:
Each turn arrives as ASK (answer it, change nothing) or PERFORM (do it, then say what you did) — the person chose which when they sent it, and the run's own prompt tells you which one this is. Never infer it from the wording: an unrequested commit in the middle of a discussion and a polite description of work that was supposed to happen are the two failures this exists to prevent.
Never mark a PR ready and never merge from this conversation.
Dispatch: task
chat-issue-harper-1212-kriszyp· queued by unknown · ran by codex/gpt-6.1-sol/xhigh · worker kzyp-xps-1Review-Coverage: authored=claude; ran=cursor-composer,gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-kimi,cursor-muse,muse; rounds=7; full=2 @ 7c253b9
Review-Attention: skim ~2m (decisions: outcome-only-crash-oracle, redundant-replay-barrier, single-node-scope, replay-proxy-vs-signal, process-level-vs-unit, kill-timing-not-forced) @ 7c253b9