fix: preserve the real 2.0 database before any fleet migration - #19
Conversation
- Make manifest writes genuinely atomic (temp file + fsync + os.replace, plus directory fsync) so a crash mid-finalize-rewrite can no longer truncate a valid manifest and turn a successful migration into a permanent MigrationSafetyError. - Recover an unmerged WAL from a crashed legacy source by copying the database and its -wal/-shm sidecars into scratch space and checkpointing that copy, instead of relying on a read-only open of the real source (which is not reliable when there is no live connection left to have checkpointed it — exactly the crashed-service upgrade this task exists for). - Match manifest proof by normalized path (os.path.normcase + os.path.abspath) instead of exact string equality, so casing or separator drift on a Windows reinstall can't make valid proof invisible and refuse to start a healthy machine. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Try a checkpoint-consistent read-only backup() of the real source first (handles a live writer safely); only fall back to the scratch trio-copy when that open fails, and guard the copy window with a before/after stat check (retry once, then fail closed) so a torn db/-wal/-shm pairing can no longer silently pass verification. - Require target_exists proof to re-verify the source file's current SHA256 against the manifest's recorded source_digest, not just its path, so swapping in a different database at the same path can't ride a stale manifest. - fsync the target's parent directory right after the rename and before the manifest finalize write, so power loss between them can't leave the manifest and the target disagreeing about whether it happened. - same_path detection now uses the same normalized path comparison manifest matching already uses, instead of raw Path equality. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Build SQLite read-only URIs from Path.as_uri() instead of raw string
interpolation. A plain f"file:{path}?mode=ro" breaks on Windows paths
containing backslashes, a drive-letter colon, or a literal %/?/#, and a
failed proof-digest recompute at startup meant a healthy already-migrated
machine would refuse to start.
- Refuse a symlink (or reparse point, on the installer side) planted at the
legacy db path or its -wal/-shm sidecars, which could otherwise redirect
the migration read outside the application tree. Routes through the same
MigrationSafetyError (explicit source) / "failed" (inferred source)
paths as every other guard.
- Collapse a near-identical duplicate of the unmerged-WAL recovery test
that a previous fix pass added instead of just re-verifying the original.
Declined by the controller and not implemented: hashing WAL bytes into
source_digest.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a3bfe690d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if staging.exists(): | ||
| staging.unlink() | ||
|
|
||
| source_digest = _sha256(legacy) |
There was a problem hiding this comment.
When the legacy database is in WAL mode and a writer commits after the backup snapshot, the main database file can remain byte-for-byte unchanged while the new rows exist only in -wal. Because source_digest hashes only the main file, a later target_exists check accepts the old manifest as proof and starts with a target missing those committed rows; this is reproducible with wal_autocheckpoint=0 and an open writer. Fingerprint the logical SQLite snapshot or otherwise include WAL-backed changes when validating that the source has not changed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declined, recorded ruling. The scenario needs a writer committing to the legacy database after the backup snapshot. On this fleet the 2.0 service is stopped before 2.1 runs, and after migration nothing opens that file again. Making the digest WAL-sensitive adds a false-refusal path (any later recovery or checkpoint of the untouched source changes what the digest sees) that is likelier than the crash window it guards. The updater lane that could reintroduce a live writer is Task 14 scope and is called out in the PR body.
| name, or a stray trailing separator still finds its own manifest instead | ||
| of refusing to start on a healthy machine. | ||
| """ | ||
| return os.path.normcase(os.path.abspath(str(path))) |
There was a problem hiding this comment.
On Windows, normcase(abspath(...)) normalizes case and separators but does not expand an 8.3 path such as C:\HUMWAT~1 to its long equivalent. If a reinstall supplies the same legacy or target file through that alternate spelling, the manifest comparison at _find_manifest_proof rejects the valid prior migration and an explicit source causes startup to fail with MigrationSafetyError, despite this helper explicitly intending to tolerate 8.3 drift. Resolve paths to their final filesystem names before comparing them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Partially accepted. The docstring overclaimed 8.3 tolerance and now says the opposite (fixed in cd33acc). Behavior change declined: expanding 8.3 aliases needs realpath's Windows-only resolution we cannot test in this environment, the installer only supplies long-form paths, and an alias mismatch fails safe with a visible refusal plus merge guidance rather than losing data.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd33acc4f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if manifest.get("status") == "migrated": | ||
| return manifest_path |
There was a problem hiding this comment.
When a prior migration's archive survives but the target is later replaced—such as after a rollback or restore creates a fresh empty database at the same ProgramData path—this branch accepts the finalized manifest without examining the current target at all. The unchanged legacy source still satisfies the source digest, so startup returns target_exists and silently adopts the replacement database, recreating the history-loss condition this safety check is meant to prevent; finalized proof needs a durable identity check against the current target rather than trusting status alone.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepted in weakened form, fixed in 3a8b82d. Finalized proof now also requires the current target to be a plausible migrated database (opens as SQLite, has the required 2.0 tables, at least one machine_info row), so a rollback-created empty replacement is no longer adopted. Deliberately NOT comparing the manifest's target_digest or row counts for finalized proof: the live service mutates the target from first boot and retention pruning shrinks row counts, so any equality check would refuse startup on every healthy machine. Covered by two new tests (empty replacement refused, lived-in target still accepted).
… database Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a8b82dd25
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _fsync_dir(path.parent) | ||
| _fsync_dir(path.parent.parent) |
There was a problem hiding this comment.
On the first migration, mkdir(parents=True) may create both the timestamp directory and migration-archive, but these fsyncs cover only the manifest entry and the timestamp entry inside the archive. They never make the new migration-archive entry durable in resolved_data_root before the target rename. A power loss after staging.replace(target) but before the later target-parent fsync can therefore preserve the target while losing all manifest proof, causing every subsequent startup with the explicit source to raise MigrationSafetyError; fsync the data-root directory before publishing the target.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b85898c. The data root is fsynced right after the pending manifest write and before the target rename, so the migration-archive directory entry is durable before the target is published. The event-order test now asserts pending-write, then data-root fsync, then rename, then target-parent fsync, then finalize.
…arget Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b85898cb72
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| $LegacyDatabasePath = Join-Path $AppDir "humwatch.db" | ||
| if (Test-Path -LiteralPath $LegacyDatabasePath -PathType Leaf) { |
There was a problem hiding this comment.
When a pre-hardening installation used the documented db_path setting with anything other than the default filename (for example telemetry.db or an absolute custom path), the existing database is not located at this hard-coded path. The installer still overrides HUMWATCH_DB with the ProgramData destination but omits HUMWATCH_LEGACY_DB, so legacy_database_path() sees only the absolute destination, reports no source, and startup initializes an empty database while leaving the user's history behind. Derive the legacy location from the existing config.json/service configuration rather than checking only $AppDir\humwatch.db.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declined. For service installs the installer has always injected HUMWATCH_DB, overriding config.json, so a service box's history already lives at the env-provided path regardless of a custom db_path name. The stranded-custom-name scenario requires a hand-edited config running outside the service, and those setups never execute service-setup.ps1, so this fix could not reach them either. Arbitrary 2.0 configurations are handled explicitly by the Task 13 migration wizard, which is where the plan maps the rest of issue #14.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Task 1 of the 2.1 fleet hub plan. Closes #14.
What this does
The Windows upgrade bug: an absolute db destination made migration silently skip the real legacy database and adopt an empty one. This PR makes preservation explicit and makes failure visible.
HUMWATCH_LEGACY_DB/legacy_db_pathname the source explicitly. The installer sets it only whenC:\HumWatch\humwatch.dbexists.sqlite3.Connection.backup()(read-only open first, checkpoint-consistent under a live writer), with a stat-guarded trio-copy fallback that recovers a crashed service's unmerged WAL from a scratch copy. The real source is never written or unlinked.pendingbefore, final after), so a crash in any window leaves provable state.target_existsproceeds only on manifest proof bound to the source digest and normalized paths. An unproven pre-existing target with an explicit source refuses startup with both paths, row counts, and merge guidance instead of quietly adopting the empty database.Path.as_uri()(Windows backslash/percent-safe). Source and sidecars get lstat no-follow regular-file guards, and the installer rejects reparse points.Suite: 260 passed, 5 skipped (baseline 241/5). TDD evidence including the deliberate RED failure is in the task report referenced by the review receipt.
Review provenance
Internal Opus review plus scoped re-reviews, one codex CLI round, and two Luna 5.6 xhigh rounds. All Critical/Important findings fixed except two conscious declines:
source_digest: declined. Post-migration nothing writes the legacy db, and a WAL-sensitive digest adds a false-refusal path likelier than the crash window it guards.scripts/update-core.ps1never re-runs service-setup, so portable upgrades never receiveHUMWATCH_LEGACY_DB): real, and deferred to Task 14 which owns the updater transition. Zero field exposure before 2.1.0 ships because update.bat installs only published releases.Minor deferrals are recorded in the SDD ledger for the whole-branch review at the end of the program.
🤖 Generated with Claude Code