Fix copy-db producing a silently corrupt, non-restorable database copy

[kriszyp]

Fixes #2048.

copyDb()/copyDbi() in bin/copyDb.ts — behind the copy-db CLI verb and storage.compactOnStart — degraded the copy five independent ways while logging copied N entries and exiting 0. Each is a separate defect in the same ~145-line function; each is fixed on its own terms rather than by patching the one heuristic they shared.

	Defect	Fix
1	A value.length < 14 tombstone test also matched the primary DBI's shared-structures dictionary when it was small (short attribute names), so every record in the copy decoded as null	symbol-keyed entries are never classified; a tombstone is identified by decoding with the table's own record decoder; a primary DBI's copy fails if its dictionary did not land
2	getKeys() + one getEntry() per key yields one entry per unique key, collapsing every dupSort secondary index	one getRange pass per DBI, which yields every duplicate (and drops the second lookup per key)
3	The blob store was never copied, so the copy was unreadable anywhere but its origin	blobs is now a required argument: 'copy' writes each root to <target>-blobs/<rootIndex>/ with a README; 'preserve-source-roots' is for the in-place replacement compactOnStart does
4	The audit store was copied into the source environment (rootStore, not targetEnv), un-awaited	both handles opened on the correct environment, raw, and awaited
5	Every recognised tombstone was dropped regardless of age	same auditRetention cutoff the runtime uses, fixed once per copy

Channel 5 is not in the issue; I found it while fixing channel 1. Dropping a live tombstone loses the delete, so a peer that missed it can resurrect the record.

Two things worth knowing about the classifier: a length cannot decide this question in either direction (a real delete carries node-id metadata and runs to 17 bytes, past the 14 the old test used), and RecordEncoder.decode returning null is not sufficient either — it also returns null for a record whose shared structure is missing on this node (resources/RecordEncoder.ts:470). So the classifier requires the metadata-bearing decode only a real tombstone produces and keeps anything it cannot prove.

Silent-success paths. A fix for "produces a corrupt copy and exits 0" cannot keep the paths that made that possible, so these now fail the copy: a per-record error, an exhausted resume, a target that already exists (it was opened and merged into), and a failed compaction backup (it overwrote the only good copy). The retry bound drops from 10,000,000 to 1,000, resume no longer advances past the key it failed on — it used to bump a string key to <prefix>z, skipping everything in between and then reporting success — and a partial copy is removed. compactOnStart now skips a RocksDB database, skips one whose tables span multiple environments (it would relocate tables and strand blobs), and rolls back only backups it created this run.

Where to look

• bin/copyDb.ts:394 isDeletedRecord — the classifier, and the one place a bug still silently deletes data. It runs only for values whose trailing byte is msgpack nil.
• bin/copyDb.ts:476 the resume path. Retrying from the same key relies on a re-put of identical bytes being a no-op (and a dupSort pair being a set). A permanently unreadable key now fails the copy after 1,000 attempts instead of skipping a key range.
• bin/copyDb.ts:206 useRawBytes. This mutates the handle openDB returned. It is safe because lmdb-js constructs a new LMDBStore per openDB call (node_modules/lmdb/open.js:415, no instance cache) — the pre-existing code depended on the same fact. Both outside reviewers' first pass flagged this as mutating the live store; it does not, and the live primaryStore the classifier decodes with keeps its decoder.
• dataLayer/blobBackup.ts — snapshotBlobs's copy core is extracted as copyBlobRootsByIndex and its staging directory moves from blobs/.tmp-<id> to <snapshotDir>.tmp (same filesystem either way, so the rename stays atomic). blobsReadmeContent's archive boolean becomes a variant.
• unitTests/bin/copyDB.test.js:26 — this suite never ran: it gated on a config value that is unset under mocha, so it skipped in both engine runs. That is why none of this was caught. Fixing the gate surfaced two pre-existing failures, and the 85%-compaction assertions turned out to be measuring the audit log being dropped — they are replaced with copy-not-larger-than-source plus record readability.

Verification

Route (b), new integration-grade regression coverage in the unit suite (it drives real LMDB environments, real tables and real blob files), plus (a) the repaired existing suite.

unitTests/bin/copyDbIntegrity.test.js (10 cases) — HARPER_STORAGE_ENGINE=lmdb npm run test:unit:bin → 178 passing; default engine → 156 passing, 7 pending; npm run test:unit:backup → 77 passing; npm run test:unit:dataLayer → 245 passing.

Reads go to the copy at its own path, opened with the same OpenDBIObject the runtime uses. Swapping the copy over the source path proves nothing: lmdb-js returns the already-open environment for a path, and the live stores answer point reads from cache — an early version of these tests passed for that reason.

Fails-on-base (same tests against origin/main, quantified):

test	on base	on branch
records readable with a small dictionary	0 / 3000	3000 / 3000
dupSort index entries for one value	1	20
blob files beside the copy	none (no -blobs)	all, byte-identical
restore as a different database and read the attachment	n/a (nothing to restore)	byte-exact
copy has an audit store	no — and every entry logged Illegal extended type while being written into the source	full log, source untouched
expired tombstone purged	no	yes

npm run test:unit:main cannot run on this machine (a local Harper instance holds the RocksDB system lock; it fails at module load, before any test) — relying on CI for it.

Open items

• No fault-injection test for the new failure paths (partial-copy cleanup, resume exhaustion). Both outside reviewers asked for one; AGENTS.md forbids new sinon/stub-based tests, and I could not reach those paths through the real modules without stubbing, so I left it rather than take the shortcut. The paths are small and the review verified them by reading.
• Not covered: more than 5,000 duplicates under a single dupSort key. The 5,000-outstanding-write await fence itself is crossed by the 3,000-record fixture.
• compactOnStart can now fail a startup where it previously produced a bad copy and continued. That is intended, and the config flag is cleared before the work, so it will not retry-loop.
• Docs companion: Document copy-db's blob companion directory and restore steps.

Generated by Claude Opus 5. Reviewed pre-push by Codex (graded) + Gemini + a Harper-domain adjudication pass; two Gemini blockers were verified false and dropped (documented above), and every kept production finding is fixed in 4ecb258.

Rebase note (2026-08-26)

Rebased onto latest main (was 214 commits behind; main had independently landed the
same blob-classification feature this PR also touches in dataLayer/blobBackup.ts).
Conflicts were in dataLayer/blobBackup.ts only (the copyBlobRootsByIndex extraction
this PR's first commit adds vs. main's own classify/substitute-marker feature landing in
the same function) — resolved by keeping both: the reusable copyBlobRootsByIndex(destDir, blobRoots) signature this PR needs for copy-db, generalized so its warning message no
longer references a backupId that's out of scope for a non-backup caller.

While rebasing, the pre-push review CLI (codex + gemini + cursor-grok + a Harper-domain
adjudication pass) surfaced two more, unrelated real bugs on top of the rebase, both fixed
and covered by a regression test:

• isWithin()'s startsWith('..') containment check was character-based, not
  segment-based, so a real child directory whose name happens to start with .. (e.g. a
  copy target literally named ..copy.mdb) was misclassified as outside the source blob
  root — letting a copy's blob destination land inside the very root it walks. Fixed to
  compare only the first path segment; regression test added
  (unitTests/bin/copyDbIntegrity.test.js: "rejects a blob-copy target inside the source
  blob root even when its name starts with '..'").
• The same containment check only validated the final blob destination, not the other
  paths a failed copy actually removes (targetDatabasePath, its -lock sibling, the
  blob staging directory's .tmp sibling) — widened to check all of them.

For the human reviewer

The domain-adjudicated review left these open (not fixed here — out of this rebase's
scope, and some are pre-existing / design tradeoffs the PR author should weigh in on):

• minor, in-scope — a blob-copy where every configured root is missing still warns
  and returns success rather than failing closed (bin/copyDb.ts copyDatabaseBlobs).
  Ambiguous against the legitimate "no blobs yet" case, so I only added a warning rather
  than unilaterally deciding to fail closed.
• minor, in-scope — the final database/blob copy targets are reserved by a
  non-atomic existsSync check, not an atomic reservation or stage-then-rename; two
  concurrent copy-db invocations to the same target can race.
• minor, in-scope — the containment check is still purely lexical (no realpath),
  so a symlinked path component could in theory alias into a blob root.
• minor, pre-existing — a legacy audit store in its own environment is never copied
  (sourceAuditStore ends up falsy for it), so its copy silently ships with no audit log.
• minor, pre-existing — compaction stages its copy inside the scanned database
  directory; a crash between the copy and the two renames leaves a <db>-copy.mdb that
  the boot scanner loads as a live database.
• A second review round's Gemini leg flagged verifyStructuresCopied awaiting a
  DBI-shared written promise as possibly stale across DBI boundaries
  (bin/copyDb.ts around the copyDbi/verifyStructuresCopied closure). I traced the
  control flow and believe it's correct — written is only read synchronously right
  after its owning copyDbi call resolves, before anything else can reassign it — but
  flagging for a second pair of eyes since domain adjudication didn't run on this delta
  round to confirm.

Pre-existing (not introduced by this rebase, reproduced on main-tip base too):
unitTests/bin/copyDbIntegrity.test.js has 3 failing tests around tombstone
version/localTime preservation and audit-log-into-target — same 3 failures on the
unrebased branch tip against its old base.

Independent review: codex (graded) + gemini + cursor-grok + harper-domain, round 1 full;
codex + gemini, round 2 delta. Receipt at 162cc49b2609.

_{Review-Coverage: authored=claude; ran=codex,gemini; declined=cursor-grok,cursor-composer,domain; rounds=2 @ 162cc49}

_{Human-Review-Need: 4 @ 162cc49}

[gemini-code-assist]

Code Review

This pull request enhances the database copy and compaction processes for LMDB environments to ensure data integrity and prevent silent degradation. Key improvements include requiring a blob disposition strategy to handle file-backed blobs, verifying that shared-structures dictionaries are successfully copied, preserving duplicate keys in dupSort indexes, and correctly routing audit logs to the target environment. Additionally, delete tombstones are now retained if they fall within the audit-retention window. Review feedback recommends adhering to the repository style guide by using the node: prefix for the path import and suggests adding a defensive null check for entry values during iteration.

[claude]

Reviewed; no blockers found.

[claude]

Suggestion (non-blocking): This test asserts the colliding database is registered (getDatabases()['collision-source-copy']) before compacting. Both the old copyDatabaseRootStores/relative(...) === '' check and the new existsSync(copyDest) check protect a registered database whose path matches exactly, so this test would still pass with faf06aafb ("Fail closed on occupied compaction targets") reverted — it doesn't exercise the gap that fix actually closed (an unregistered occupant at the target path: a stray leftover, a failed-to-load, or restore-blocked database, none of which appear in getDatabases()). Consider adding/adjusting a case that writes an arbitrary file or directory at copyDest without registering a database there, to prove the new existsSync-based check (not just the old registration-based one) is what's guarding the path.

[claude]

Suggestion (non-blocking): When every configured blob root is missing, this only warns and then returns (copy reports success with no blobs). Per the PR's own framing, this is ambiguous between "never had blobs" and "an unmounted/misconfigured root" — the latter produces exactly the kind of silently-degraded, non-restorable copy this PR set out to fix. Consider failing closed by default here (throw) with an explicit opt-in for the "I know this database never had blobs" case, rather than a warning an unattended/scripted copy-db run won't see.