Give restored and copied databases a new generation, and end subscriptions from before the copy instead of resuming them - #2914
Conversation
…ted against the source resumes against the copy restore_backup (online and offline), branch checkpoints, the LMDB->RocksDB migration and `harper copydb` now stamp a fresh database generation into the copy before it can be read, flushed so the stamp is durable. A resumable position is bound to the generation and checked against a per-generation resume floor that every prune raises alongside the audit floor, so an unknown audit floor no longer blocks resume and reconciliation's floor keeps its meaning. An open never repairs a generation; a live subscription registered before its database was replaced is ended with DatabaseGenerationChangedError instead of silently receiving the copy's events. Refs #2451. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…key the purge test's fake per record commitAuditMetadata picked the RocksDB branch from the transaction owner's type, so an audit store whose root is not a RocksDatabase instance (the purge test's stand-in) was routed through the LMDB branch. Decide it from the store, as before, and also accept a bare RocksDatabase for stamping a copy. The purge test's fake now stores each record under its own key, since a prune writes the resume floor alongside the audit floor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…he on-disk path out of the terminal subscription error Review round 1: the open-time catch-up missed read-only opens and a missing resume floor written below a higher audit floor; bounding the resume floor by a finite audit floor when it is read covers both, so the catch-up write is gone. DatabaseGenerationChangedError now lives with the other ClientError types, carries no filesystem path, and is sent as the final message of a replaced subscription. Added comments are cut to the constraints they state. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…owngrade case the resume floor cannot see Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lock-free skip still applies Starting it at 0 made the first retention pass on every database open a write transaction to raise it, even when the audit floor already covered the cutoff. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces database generations and resumable positions to prevent database copies, restores, and branches from incorrectly inheriting or resuming subscriptions or positions from their source states. It implements tracking for a database generation ID and a resume floor in the audit store metadata, stamps a new generation during copy, restore, and branch materialization, and terminates live subscriptions with a DatabaseGenerationChangedError when a generation change is detected. As there are no review comments provided, I have no feedback to provide on the review.
…ma rescan opens a database under a restore The suite's table writes queue an analytics flush one second out. The flush declares hdb_raw_analytics, and the schema rescan that follows opens every database under STORAGE_PATH and keeps it open. On Linux CI it fired during rocksdbBackup.test.js, which had pointed STORAGE_PATH at its own temp directory: the in-place restore then purged files under Harper's open handle, every later backup of that database failed on a missing .sst, and the handle failed its exit flush. The same flush could land inside this suite, between a closeDatabase and the restore that follows it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Reviewed; no blockers found. |
…stener resubscribing while it is ended cannot rejoin the copy's registry Review (cb1kenobi): endSubscriptionsOfOtherGenerations detaches the path's registry entry and then runs listener code through close(error). A listener that resubscribed through a handle of the replaced database, such as a TableResource loaded before the restore, created a fresh entry tagged with the old generation. The copy's subscribers joined that entry, and the next ordinary reopen of the copy ended every one of them. Static subscribe was already refused by getResource's closed-root check; resource instances and direct callers were not. The sweep now records the reopened store's generation as the path's current one before any listener runs, and addSubscription refuses a registration through a table of any other generation with DatabaseGenerationChangedError (409). A full-database registration is skipped rather than refused, which leaves replication's whenNextTransaction as it was. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… tests Review round 3 carried it as the remainder of the narration nit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eClosingError when the generation is unchanged Review (kriszyp): a reopen under the same generation left its subscriptions registered and open, but their commit listener belonged to the closed audit store handle, so no later write reached them unless another subscription happened to reattach a listener. Reattaching would not be enough either: live delivery reads each record through the subscribing table's primary store (eventFromAudit), which belongs to the closed handle too. endSubscriptionsFromEarlierHandles (renamed from endSubscriptionsOfOtherGenerations) now ends every subscription registered on the path before the open. One from another or an unknown generation still gets DatabaseGenerationChangedError; one from the same generation gets the retryable DatabaseClosingError, named for the database rather than its path, and a resubscribe through the reopened table resumes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kriszyp
left a comment
There was a problem hiding this comment.
This looks good, although I don't think the overhead/complexity of supporting LMDB here is worth keeping.
🤖 Reviewed with Codex
cb1kenobi
left a comment
There was a problem hiding this comment.
No confirmed blocking defects remain on the changed lines. Copy, restore, branch, and migration stamp a new generation before the copy is readable, and a replaced-database subscriber is ended and cannot rejoin through a stale handle. Same-generation reopen still leaves existing subscriptions registered without reattaching the commit listener; that is the pre-existing behavior this PR kept and is already on the thread.
—
Reviewed 99923de
… last opened with Review round 4 on the same-generation change: after a reopen that kept the generation, a consumer retrying through the table handle it already held passed the generation check, since the id matched. It registered into a fresh registry entry bound to the closed stores, and never delivered. The same held between a close and the reopen. The path's current audit store, rather than its generation id, is now what a registration must match, and a handle whose root is closed is refused too. The error follows the sweep's rule: DatabaseGenerationChangedError when the generation differs or is unknown, the retryable DatabaseClosingError when it is unchanged. endSubscriptionsFromEarlierHandles takes the reopened audit store and records it before any listener runs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tions with DATABASE_CLOSING Follows HarperFast/harper#2914's review: a reopen under an unchanged generation now ends the subscriptions from before it with the retryable DatabaseClosingError. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…database's closed store graph is not retained Review round 5: currentAuditStores held each path's last audit store, and nothing removed an entry, so a dropped database or a removed branch kept its closed store graph reachable for the life of the process. The map now holds a per-open token and the generation id; the store carries its token. The docstring on endSubscriptionsFromEarlierHandles is cut to its constraints, and DESIGN.md records that a legacy LMDB auditPath root, which is reopened on every metadata read, keeps main's subscription behavior. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cb1kenobi
left a comment
There was a problem hiding this comment.
The resume-floor lookup can observe inconsistent metadata during a concurrent prune and accept a cursor whose events were deleted. Read the records from one snapshot, or read the audit floor before the monotonically raised resume floor.
—
Reviewed 0d7830e
…he design note fits its budget after merging main main added three lines to resources/DESIGN.md since this branch's base, so the merge CI lints came to 1002 lines against a 999-line budget. The words are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… between the two reads is seen Review (cb1kenobi): getAuditResumeFloor read the resume floor, then the audit floor. With the audit floor unknown, a prune committing between the two reads raised only the resume floor, which had already been read, so the result was the pre-prune bound and a cursor below the new cutoff resumed. Both records only rise and an unknown audit floor stays unknown, so reading the audit floor first makes the pair equal to the state at the second read. The predicate's JSDoc now says a prune after it returns is the caller's to order against its replay. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… and legacy audit root as they were Review (kriszyp): supporting LMDB is not worth the overhead. An LMDB audit store now gets no genesis, and a prune on LMDB raises the audit floor alone, with no resume-floor record. harper copydb's LMDB copy no longer skips generation keys or stamps its target, and the legacy LMDB auditPath root no longer establishes a generation; bin/copyDb.ts, databases.ts and copyDB.test.js are back to main apart from the migration's RocksDB stamp. isResumablePosition is always false on LMDB, and its JSDoc says a caller applies the check to RocksDB alone. A reopen still ends the subscriptions from before it on both engines, but a store that tracks no generation is never reported as replaced: an LMDB reopen ends them with the retryable DatabaseClosingError. The commit's metadata write keeps its LMDB branch, which the audit floor has always used. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| const generationId = auditStore.databaseGeneration?.id; | ||
| const token = Symbol(basename(path)); | ||
| auditStore[HANDLE_TOKEN] = token; | ||
| currentHandles.set(path, { token, generationId, tracksGeneration }); |
There was a problem hiding this comment.
Each openAuditStore adds or replaces a currentHandles entry, but nothing removes one when its database is closed or dropped. A long-lived worker that creates and drops databases under distinct paths retains every path and generation ID indefinitely. Could close/drop remove the entry when its token still matches? The closed-store guard at resources/transactionBroadcast.ts:53 can continue rejecting registrations before the next open.
…tions with DATABASE_CLOSING Follows HarperFast/harper#2914's review: a reopen under an unchanged generation now ends the subscriptions from before it with the retryable DatabaseClosingError. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d of 5.3.0 HarperFast/harper#2914 merged after v5.3.0 was tagged, so 5.3.0 does not end a pre-restore subscription. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
restore_backup(online and offline), branch databases and the LMDB→RocksDB migration now stamp a new database generation into the copy before it can be read. Generations are RocksDB-only; an LMDB database carries none. A live subscription from before the copy is ended withDatabaseGenerationChangedErrorinstead of silently receiving the copy's events. This is the core of #2451. A resumable position is valid only if it names the current generation and sits at or above a per-generation resume floor that every prune raises (isResumablePosition). #2448 wires that check intoTable.subscribe. The design comment on #2451 covers three planning-review rounds.For the human reviewer
Table.subscribe; that is Consume the audit staleness floor in Table.subscribe: an opt-in stale-start check so MQTT durable resume signals truncation instead of replaying short #2448. Replication's per-peer state and base copy are harper-pro's: Replication should treat a peer whose database generation changed as a new incarnation, so a restored node does not silently diverge harper-pro#944.better-alternative-existsin all three planning rounds. Each alternative refined the one before and was adopted; the design comment's resolution table lists every one. Gemini cleared rounds 2 and 3.harper copydbtarget, no longer applies now that LMDB copies stamp nothing:cursor >= epochwithout an id. That certifies the 900/970/1000 legacy-bootstrap cursor, because no timestamp is immune to clock rollback. Adding it later reopens that window.audit-resume-floorsits beside the audit floor. Every prune raises it in the same transaction, and it is read as never below a finite audit floor.Table.commit's out-of-order reconciliation walks history onInfinityand skips below a finite floor, so rewriting the floor changes merge results.openRocksDatabaseoptions, stamp it, flush, and close it, all beforecompleteRestore.DatabaseGenerationChangedError(aClientError, 409, codeDATABASE_GENERATION_CHANGED, with a message that names no path) and closes.DatabaseClosingError(503,DATABASE_CLOSING), from kriszyp's review. Before, it stayed open with its commit listener on the closed audit store and silently received nothing. Rebinding it would not work: live delivery reads each record through the subscribing table's primary store (eventFromAudit), which belongs to the closed handle too. In production a same-generation reopen follows only an online restore that failed before changing anything; a schema reload never closes the stores.DatabaseGenerationChangedErrorif the generation differs, elseDatabaseClosingError. That covers a resubscribe a listener makes while its old subscription is being ended (from cb1kenobi's review), and a retry afterDatabaseClosingErrorthrough the handle it already held. Staticsubscribewas already refused bygetResource's closed-root check; aTableResourceloaded before the close was not. The alternative was to rebind such a registration to the current database. A refusal can be loosened later without breaking a caller, but a rebind, once callers rely on it, cannot be taken back.DatabaseGenerationChangedError(409, resynchronize) and the existingDatabaseClosingError(503, retry), so clients branch on both. A single dedicated class was the alternative; the existing one already means "this database handle closed, retry".auditPathlayout keeps main's behavior, with no teardown and no handle check. That root is reopened on every metadata read, so a sweep there would end its subscriptions each time.resources/DESIGN.mdlists it as not covered.harper copydband compaction (LMDB paths) stamp nothing, andisResumablePositionis always false there, so Consume the audit staleness floor in Table.subscribe: an opt-in stale-start check so MQTT durable resume signals truncation instead of replaying short #2448 applies the check to RocksDB alone and LMDB resumes stay as they are today. An LMDB reopen still ends its subscriptions, with the retryableDatabaseClosingError, since nothing on LMDB can report a replaced database. The LMDB branch of the metadata write stays, because the audit floor has always used it.resources/transactionBroadcast.ts, inSubscription.end()and the delivery loops. The overlap is textual only, and whichever PR lands second rebases.Framing-Verdict: better-alternative-exists (5cef2e51e4bd)
Changes
resources/auditStore.tsopenAuditStorenow establishes the generation, then ends the subscriptions registered through an earlier handle.updateAuditFlooris rebuilt oncommitAuditMetadata. It writes all records or none: every write is read back, and a mismatch throws inside the transaction.raiseAuditFloorskips only when the floor, and on RocksDB the resume record, cover the cutoff, and raises the resume floor on RocksDB in the same transaction as the audit floor.getDatabaseGenerationandgetAuditResumeFloor, which is never below a finite audit floor and reads the audit floor first, so a prune committing between its two reads is seen (from cb1kenobi's review).isResumablePosition.establishDatabaseGeneration, which skips an LMDB store. Genesis is a compare-and-set, and starts at the audit floor. An unreadable record is never repaired.stampDatabaseGenerationandstampDatabaseDirectory, which does a private open, stamps, and flushes.getAuditFloorand copies.resources/transactionBroadcast.tsendSubscriptionsFromEarlierHandlesdetaches that entry, then ends each subscription:DatabaseGenerationChangedErrorfor another or an unknown generation, the retryableDatabaseClosingErrorfor the same one. The close is forced even when a listener throws.addSubscriptionrefuses a registration through any other handle or a closed one. A full-database registration (replication'swhenNextTransaction) is skipped rather than refused, which leaves replication as it was.utility/errors/hdbError.ts:DatabaseGenerationChangedError, a 409ClientError.dataLayer/rocksdbBackup.ts: the online and offline restores, each beforecompleteRestore.resources/branchDatabase.ts: the branch staging checkpoint, before the rename.bin/copyDb.ts:resources/DESIGN.md: a new section. The floor section is condensed where the generation supersedes it (see the intro and copies).dataLayer/DESIGN.md: the restore stamp step.DESIGN.md: the index.restore_backupreference and the 5.3 release notes that an online restore ends each pre-restore subscription withDATABASE_GENERATION_CHANGED(409). Nothing else here is user-facing yet:isResumablePositionhas no public caller until Consume the audit staleness floor in Table.subscribe: an opt-in stale-start check so MQTT durable resume signals truncation instead of replaying short #2448.Verification
New unit suites
unitTests/resources/databaseGeneration.test.js: 22 tests on RocksDB, and 2 on LMDB: an LMDB database gets neither record, and a reopen ends its subscriptions withDatabaseClosingError. It covers genesis, the resume floor (including a prune committing between its two reads), stamping, resumable positions and live subscriptions, including a resubscribe from inside the teardown, a resource loaded before the replacement, a same-generation reopen and a retry through the handle that reopen closed.unitTests/dataLayer/restoreGeneration.test.js(RocksDB) drives the productionrestoreBackupOfflineand a real reopen: the old subscriber is ended, and old positions are refused while new ones are accepted. It keeps analytics off while it runs: an analytics flush is followed by a schema rescan that opens every database underSTORAGE_PATH, which could be one the suite is about to restore or, once the suite has ended, the next suite's. Linux CI caught the second case inrocksdbBackup.test.js.Fails on base: the first restore test fails on
af42ef2a4because the pre-restore subscription stays open, and it passes here. The other two restore tests fail on base only becausegetDatabaseGenerationdoes not exist. The two registration tests fail on2c2b2c3f6, the head before the fix. There the reentrant resubscribe is accepted, and its entry's old generation tag makes the next same-generation reopen end the copy's subscribers. A resource loaded before the replacement fails with aTypeErrorinstead of the 409. The same-generation test times out on99923de8f: the first listener gets neither the write nor a terminal signal, which is kriszyp's check. The retry test fails on8f0ebdf83with a missing rejection.Extended suites
unitTests/resources/branchDatabase.test.js: a fork mints a new generation, and adopting it again keeps it.unitTests/bin/migrationStagingRecovery.test.js: a migrated store is not genesis.unitTests/resources/auditPurge.test.js: the fake now keys records separately.Gates. Failures are attributed by name against the same files on
af42ef2a4. Thetest:unit:resourcesrows are onf72bcec47, the subscription integration row onb59c9c5cb, the Linux row on2c2b2c3f6, and the rest ona4c517ba4.test:unit:resources(RocksDB)sourceApplyConflictRetryfailures, which fail on basetest:unit:resources(LMDB)test:unit:mainapplicationSpawn.test.js, which hangs locally. The same 19 fail on base.test:unit:mainon Linux (a Node 24 container)test:integration:alllint:required,prettier --check,tscandcheck-design-docsare clean.End-to-end route. A refused persisted cursor is not observable through a public surface until Consume the audit staleness floor in Table.subscribe: an opt-in stale-start check so MQTT durable resume signals truncation instead of replaying short #2448 calls
isResumablePositionfromTable.subscribe. The restore test drives the production restore and a real reopen.🤖 Generated with Claude Code
Complexity: complicated
Origin — the dispatch brief this PR was written from
Give restored and copied databases a new generation, and end subscriptions from before the copy instead of resuming them
LIVE CONVERSATION about #2914.
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-pr-harper-2914-kriszyp· queued by unknown · ran by codex/gpt-6-astra/low · worker kzyp-xps-1Review-Coverage: authored=claude; ran=gemini,codex,cursor-composer; adjudicated=domain; blocked=cursor-grok(failed); declined=cursor-kimi,cursor-muse; rounds=6; full=1 @ f72bcec
Human-Review-Need: 3 (decisions: rocksdb-only-generations, unknown-floor-exclusion-documented, handle-token-gate, every-reopen-ends-streams, legacy-layout-excluded, primitive-before-consumer, unreadable-generation-is-permanent, generation-error-contract, per-node-generation) @ f72bcec