fix(upgrade): fail the boot when the data-version stamp is not recorded

[kriszyp]

An upgrade boot could report success while system.hdb_info still held the old data version, so the migration re-ran on every subsequent boot. Two independent paths produced that state; both are closed here, and boot now refuses to continue when the stamp is not confirmed.

The failure was swallowed. runUpgrade caught everything insertHdbUpgradeInfo() threw, logged it, and returned normally — so bin/run.ts's existing "Got an error while trying to upgrade your Harper instance… Exiting Harper." / process.exit(1) path never fired. It now logs what state the instance is in and rethrows.

The stamp could also no-op with no error at all. insertHdbUpgradeInfo derives the new info_id as max(existing) + 1 from a search whose failures getAllHdbInfoRecords swallows into []. On that path newId is 1, which collides with the install record — and createRecords sets requires_no_existing, so upsertRecords pushes a colliding key onto skipped rather than writing it. The call then resolves successfully having written nothing. insertHdbUpgradeInfo now requires the expected id to come back as inserted and throws VersionStampNotRecordedError otherwise, which covers all three of its call sites (the upgrade path plus the downgrade-confirmed and no-directives-needed stamps in getVersionUpdateInfo). This second path fits the issue report better than the first: the reported boots show the migration notices and no stamp error.

The invariant: upgrade() returns normally only if the new data version is actually recorded, because the stamp is the only thing stopping a directive re-running against data it has already migrated.

Framing-Verdict: chosen-approach-sound (planning review, before implementation).

Re-deciding the pinned trade-off

The reversed unit test pinned "log and continue" by name. Three facts decided the reversal:

• The swallow was the outlier, not the policy. insertHdbUpgradeInfo's other two call sites are already fatal to boot — they run inside getVersionUpdateInfo's try/catch, which log.fatals and rethrows, and bin/run.ts exits 1.
• What was ignored is not bookkeeping. QA finding F-296 measured that today's re-run is benign (4 record shapes byte-identical by sha256 after a forced re-run, positive control armed) — which is exactly why the swallow survived. It costs nothing until the first non-idempotent directive, at which point it costs data.
• Failing boot leaves a safe state. The data is already migrated and the process exits naming the version; booting with a stale stamp leaves an instance that re-migrates on every boot and reports success each time.

For the human reviewer

Decisions taken during the pre-push review that a reader should weigh rather than assume:

• An orchestrated restart loop is the accepted cost, and it is not new. One reviewer called the exit a blocker: a supervisor (systemd, Docker, Kubernetes) will restart Harper immediately, so the "do not restart" advice cannot be honoured unattended. The directives are what mutate data, and they re-run once per boot either way — under today's behaviour the instance also comes up and serves traffic while doing it, so this change strictly reduces exposure and makes the loop visible. The failure message now tells the operator to stop the supervisor first. Whether a stamp failure should additionally use an exit code or marker that suppresses automatic restart is a design decision left open.
• A transient search failure now fails boot on a healthy instance. If pSearchSearchByValue throws inside getAllHdbInfoRecords during the stamp, newId collapses to 1, the insert is skipped, and boot exits — indistinguishable from a real persistence failure. That is the intended fail-closed direction, but it is a real behaviour change. The latest review round sharpened the consequence: because the guard lives in insertHdbUpgradeInfo rather than in runUpgrade, it also arms the no-directives stamp in getVersionUpdateInfo, where the throw escapes before bin/run.ts reaches upgrade.upgrade() — so that path gets the generic UPGRADE_ERR line, not the supervisor-restart guidance. Making the postcondition state-shaped (re-read the latest record and accept a data_version_num that already equals the target) instead of receipt-shaped would remove the false negative; that is a deliberate open decision, not an oversight, and is left for the human reviewer.
• insertHdbInstallInfo was deliberately left alone. It has the same silent-skip exposure (a re-install where info_id 1 exists resolves without writing). Extending the check there needs the install path's re-run semantics traced first, and this change is scoped to the upgrade stamp. Reported separately rather than changed here.
• The failure path has no end-to-end test, by construction. Both remaining reviewer findings ask for one. Forcing the stamp to fail requires injecting a storage or search failure at boot, which nothing in the integration harness can do. The unit tests carry that contract; the integration suite anchors the postcondition that had no coverage at all.
• Two review-bot suggestions were taken, one declined. Falling back to packageJson.version for the stamped version and optional-chaining the test's package.json read are in. Adding null guards on assertVersionRecorded's expectedId/newVersionString was declined: both are computed by the only caller a few lines above, and with the version fallback in place neither can arrive undefined.
• The original error is not logged twice. The new message deliberately omits err; bin/run.ts logs the thrown error on the exit path it reaches.
• storage.writeAsync: true durability is out of scope. It sets LMDB noSync, so a successfully written stamp can still be lost to a hard kill — the mechanism the issue title hypothesises. Nothing here syncs the stamp; a storage-engine-specific sync at boot is a separate decision.

Verification

Re-verified after rebasing onto main at 3ea2639ae (see the rebase note below); the PR's own five commits are unchanged.

• npm run build — clean.
• npm run test:unit:resources — 1841 passing, 23 pending, 0 failing. Caching › Can load cached indexed data is green again.
• npm run test:unit:main — 5129 passing, 195 pending, 1 failing: configValidator › does not warn when a relative rootPath resolves within the limit. That case resolves relative/root against process.cwd(), so it only fails from a deeply nested checkout path; it is green in CI on main and on this branch.
• npx mocha unitTests/bin/upgrade.test.js unitTests/dataLayer/hdbInfoController.test.js unitTests/dataLayer/hdbInfoVersionStamp.test.js — 10 passing, 19 pending.
• Earlier rounds (pre-rebase, unchanged code): npm run test:integration -- "integrationTests/upgrade/**/*.test.ts" — 16 passing, 3 skipped (env-gated on a prior-release install), 0 failing; integrationTests/server/** — 189 passing, the 6 failures needing a local Ollama server.
• Fails on base: with bin/upgrade.js and dataLayer/hdbInfoController.ts reverted and dist rebuilt, the reversed unit test fails with expected undefined to equal Error: Oh boy...it is an error, and the new stamp tests cannot run at all (assertVersionRecorded is not a function).

Why CI was red, and where it stands now

All three Unit Test (Node.js v22/v24/v26) jobs failed on the same case, Caching › Can load cached indexed data — "the fencing source fill should reach the indexed subscription". That was a red on main, not a regression here: it is the fence named in the commit message of b3235b8c6 "Deliver RocksDB subscription events for source fills whose version differs from the log key", which landed after this branch's merge-base. The per-node extras were separate known flakes — HNSW greedy routing above layer 0 on v22 (open: #2373) and an ENOTEMPTY teardown on v26 (open: #2276).

Checks run against the PR's merge ref, so main's fix reaches this PR whether or not the branch is rebased; the rebase re-triggered CI and makes the local tree match what CI actually tests, which is what the verification above was run against. All three Unit Test jobs now pass at 127d1adbf.

One check is red, and it is main's, not this diff's. Unit Test (Windows, Node.js v24) — the gate added by #2361 after this branch's base — fails on the unitTests/config/** group (1 of 9), on two cases from #2245: storage exhaustion during boot (#847) › isStorageExhausted › recognizes EDQUOT by errno when the platform gives no usable code (false !== true) and › persistConfigDuringBoot › swallows storage exhaustion so startup continues (Error: Unknown system error -undefined) — Windows has no EDQUOT errno. The failure is byte-identical on main's own runs (job on 655787409), which have been red on it since the gate landed. Nothing in this diff touches unitTests/config/ or config/configUtils.ts.

Integration Tests 6/6 (Windows, Node.js v24) is red for the same reason: an application with a branched database › keeps the application's own data across a restart (a restart must not discard the branch), from #2352. It fails on main's own last two Integration runs (655787409, 1db23823b). Both Windows gates landed on main after this branch's base, so this is the first run of #2398 that has ever seen them.

Refs #2158

Complexity: complicated

_{Review-Coverage: authored=claude; ran=gemini,cursor-composer,codex; adjudicated=domain; declined=cursor-grok; rounds=5 @ 127d1ad}

_{Human-Review-Need: 3 (decisions: fail-boot-on-unrecorded-stamp, guard-placed-in-controller-not-caller, receipt-vs-state-postcondition, integration-suite-that-passes-on-main) @ 127d1ad}

[gemini-code-assist]

Code Review

This pull request ensures that Harper's boot process fails if the new data version cannot be successfully recorded in the system.hdb_info table, preventing upgrade directives from running repeatedly on subsequent restarts. It introduces verification logic to confirm the database insert succeeded and updates the upgrade runner to rethrow errors rather than continuing unstamped. The feedback suggests adding a fallback version if the upgrade object's version is undefined, incorporating defensive checks in the new assertion helper, and using optional chaining when parsing package.json in tests to prevent unhandled type errors.

[claude]

Suggestion (non-blocking): this hand-rolls the exact pattern ServerError already provides (utility/errors/hdbError.ts:61-67) — extend it instead of Error directly, e.g.:

Suggested change

	export class VersionStampNotRecordedError extends Error {
	statusCode: number;
	constructor(message: string) {
	super(message);
	this.name = 'VersionStampNotRecordedError';
	this.statusCode = 500;
	}
	}
	export class VersionStampNotRecordedError extends ServerError {
	constructor(message: string) {
	super(message, 500);
	this.name = 'VersionStampNotRecordedError';
	}
	}

IndexRebuildingError/UpdateAttributesLockTimeoutError in the same base-error module follow this exact shape (extend ServerError, set this.name explicitly since the base leaves it as 'Error').

[claude]

1 finding posted inline: insertHdbInstallInfo (dataLayer/hdbInfoController.ts) has the same silent-skip hazard on hdb_info.info_id: 1 that this PR just fixed for insertHdbUpgradeInfo, and it's reachable via a normal harper run boot in container/restore scenarios where the data volume persists but boot-props files don't.

[claude]

1. insertHdbInstallInfo has the same silent-skip hazard this PR just fixed here

File: dataLayer/hdbInfoController.ts:126 (new guard) vs. dataLayer/hdbInfoController.ts:77-90, insertHdbInstallInfo (sibling, unguarded)

What: insertHdbInstallInfo inserts the initial hdb_info row (info_id: 1) and returns insert.insert(...) directly with no verification, unlike insertHdbUpgradeInfo right here, which this PR now guards with assertVersionRecorded. insert.insert reports a pre-existing key as skipped_hashes, not as a failure — the exact mechanism issue #2158 (and this PR) is about.

Why it matters: This path is reachable on a normal harper run boot, not just a fresh install. isHdbInstalled() (utility/installation.ts:14) only checks for the boot-props/settings files on disk — it never looks at whether system.hdb_info already has data. bin/run.ts:99 runs install() whenever those files are missing, which calls insertHdbVersionInfo() → insertHdbInstallInfo(vers) (utility/install/installer.ts:737-744) with no result check. In a container/restore scenario where the data volume persists but the boot-props/settings file doesn't, install() re-runs against a data dir where hdb_info.info_id: 1 already exists — the insert is silently skipped, and install reports success with the version stamp never recorded: the same silent failure this PR closes on the upgrade path.

Suggested fix: Call assertVersionRecorded(insertResult, 1, newVersionString) after the insert in insertHdbInstallInfo, mirroring insertHdbUpgradeInfo.

[dawsontoth]

Makes sense. Separate note from the PR: it would be really nice to get CI green again :\

[DavidCockerill]

Commenting — the intent here is right, and I'd take it with one change.

🧊 In plain terms: this makes Harper refuse to boot when it can't record which data version it just migrated to, which is the correct instinct — a silent failure there means the next boot thinks the migration already happened. Two rough edges: in one path Harper can now mark the data "fully migrated" after running zero migrations, and in another a failed read of the version table is reported to the operator as a failed write.

The one I'd actually fix before merge is the first, and specifically the test rather than the code. The fallback at bin/upgrade.js:115 isn't reachable today — getVersionUpdateInfo throws before it can produce a missing upgrade_version — but unitTests/bin/upgrade.test.js:71-77 now asserts the fail-open result, so the unsafe answer is pinned for any future caller that hand-builds an UpgradeObject. An unreachable branch is harmless; a test that locks in "stamp as current after running nothing" is how it becomes reachable later without anyone noticing.

Both inline threads state the property rather than prescribing a patch, since the right shape depends on whether you'd rather abstain or throw.

Verified and dropped: the version stamp itself is written before the boot proceeds, the refuse-to-boot message at bin/upgrade.js:117 carries actionable operator guidance on the path that matters, and checkIfInstallIsSupported really does run first, which is what keeps the read-failure case improbable rather than likely.

_{— DAIvid (Claude Opus 5) · cross-model: Codex (graded) + Gemini + Harper domain adjudication · graded leg called the first item major; domain adjudication downgraded it on reachability, and I agree with the downgrade}

[DavidCockerill]

The two consumers of a missing upgrade_version disagree, and this fallback makes the disagreement permanent.

With upgrade_version absent, directivesController.getVersionsForUpgrade returns [] (upgrade/directives/directivesController.ts:46-57 logs "trying to start the server anyways" and gives up), so zero directives run. The new ?? packageJson.version then stamps the data as fully migrated at the running version. One consumer reads the missing field as "can't decide, run nothing"; the other reads it as "we're current". No later boot re-checks, because the stamp says there is nothing to do.

Not reachable today — the only caller is bin/run.ts:170, whose object comes from getVersionUpdateInfo, which sets upgrade_version = packageJson.version after throwing when that is falsy (dataLayer/hdbInfoController.ts:197-200). That's why this is a comment and not a change request.

What makes it worth raising anyway is the test below: unitTests/bin/upgrade.test.js:71-77 now asserts this outcome, which pins the fail-open behaviour for whatever future caller hand-builds an UpgradeObject. A guard that is currently unreachable is fine; a test that locks in the unsafe answer is what turns it into a trap.

The property the fix needs: a missing upgrade_version must not produce a stamp. The same condition that makes directive selection abstain should make stamping abstain, or throw.

— DAIvid (Claude Opus 5)

[DavidCockerill]

This guard can't tell "the write didn't happen" from "we couldn't read the table", and only the first deserves refusing to boot.

expectedId derives from getAllHdbInfoRecords(), which swallows a failed search to console.error and returns [] (dataLayer/hdbInfoController.ts:147-153). So a read failure on a table that does have rows yields newId = 1, the insert is skipped as an existing key, and assertVersionRecorded reports "the insert … reported inserted [] and skipped [1]" — telling the operator the write was refused when the actual fault was the read. Harper then refuses to start.

In plain terms: one bad read of hdb_info becomes a permanent "Harper will not start", with an error pointing at the wrong thing.

Low probability, since the search runs after checkIfInstallIsSupported has already proven the table loads — so non-blocking. But the fail-open catch in getAllHdbInfoRecords is what converts a read fault into a misattributed write fault, and that's the seam worth closing rather than the guard itself.

Separately, on placement: the guard sits inside insertHdbUpgradeInfo, so it also fires on the downgrade-confirmed path and the "upgraded version, no directives required" path (:231, :260). On the latter nothing was migrated, so a failed stamp costs only a repeated no-op version check next boot — yet it aborts startup, and via getVersionUpdateInfo's rethrow it does so without the operator guidance at bin/upgrade.js:117 (the caller sees log.fatal + UPGRADE_ERR). Fail-closed is right where directives ran; it's disproportionate where none did.

— DAIvid (Claude Opus 5)