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

[kriszyp]

Human-Review-Need: 4 @ ee2b9bc
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.

[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.

[gemini-code-assist]

If value is null or undefined (e.g., due to a corrupt entry or unexpected state), accessing value.length at line 437 will throw a TypeError and crash the database copy process. Adding a defensive guard to skip null/undefined values ensures robustness.

Suggested change

	start = key;
	start = key;
	if (value == null) {
	skippedRecord++;
	continue;
	}

References

1. When iterating over array elements that could potentially be null or undefined, include a null-entry guard (e.g., if (!entry) continue;) to prevent runtime TypeErrors.

[gemini-code-assist]

According to the repository style guide, Node builtins must use the node: prefix. Please import from node:path instead of path.

Suggested change

	import { isAbsolute, join, relative } from 'path';
	import { isAbsolute, join, relative } from 'node:path';

References

1. Node builtins use the node: prefix. These are constraints, not style choices. ^(link)

[claude]

Reviewed; no blockers found.