Add a public format-version marker writer for rollback (#830) - #863
Conversation
A rollback restores the pre-update states.db snapshot, but the two VERSION files are separate metadata the snapshot does not cover. Both markers keep naming the newer format, and the earlier binary can select no migration for a marker it does not know, so the restored database will not open. The existing marker helper always writes CARGO_PKG_VERSION, which is exactly the version a rollback must not record. Give it the version to write and expose write_version_markers on top of it, so the caller can stamp the format version it recorded before the update onto both directories through the same atomic per-file path a migration uses. Part of #831
The marker tests all started from a directory with no VERSION, so nothing proved that a rollback actually overwrites what the update left behind rather than appending to it. Write the newer markers first, in one case non-canonically, and assert the older version lands byte for byte. Part of #831
create_version_file used to run only over the data and backup directories of a migration, but it now also serves the rollback marker writer, so a failure to flush the directory entry reported "the data dir" for three of its four call sites. Say which file's directory it is instead, and let the caller's context name which of the two it was. Part of #831
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #863 +/- ##
==========================================
+ Coverage 84.50% 84.58% +0.07%
==========================================
Files 92 92
Lines 36927 37105 +178
==========================================
+ Hits 31207 31385 +178
Misses 5720 5720 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
[Reviewer Round 1]\n\nNo findings. The public re-export and independent path generics match the requested API; write_version_markers validates before any directory creation, canonicalizes the parsed version, and preserves the existing per-marker sync/rename path with data/backup-specific error context. The focused filesystem tests cover invalid input without mutation, missing directories, replacement, partial failure and retry, while the populated 0.46 store test demonstrates that markers written through the public API let the migration chain run and finish with current markers. The PR body also links the issue and includes a complete test plan. |
|
[Review Verdict Round 1: APPROVED] |
Suggested squash commitTitle Body |
Summary
Adds
write_version_markers, the public entry point a rollback uses to put the twoVERSIONmarkers back in step with a restored database snapshot.A migration body rewrites
data_dir/states.db, but the format version lives beside it indata_dir/VERSIONandbackup_dir/VERSION. Restoring the pre-update snapshot therefore puts the older database back under markers that still name the newer format, and the earlier binary finds no migration for a marker it does not know. The new function takes the version the caller recorded before the update — the operation attempt'spre_update_version— and writes it to both markers.pub fn write_version_markers<P: AsRef<Path>, Q: AsRef<Path>>(data_dir: P, backup_dir: Q, version: &str) -> Result<()>, re-exported fromlib.rs. It parses the whole input assemver::Versionbefore touching the filesystem and writes that parsed value's canonicalto_string()form, with no trailing bytes. Validation is syntactic only: it does not readstates.db, does not read the markers it replaces, and does not judge whether any binary can migrate from the version. No public marker reader or per-marker writer is exposed.create_dir_all— but only after validation succeeds, so invalid input creates neither directory nor marker and leaves an existing marker untouched.create_version_filenow takes the version to write instead of hard-codingCARGO_PKG_VERSION, so both the migration path and the rollback path go through the same write-to-a-temporary, file-sync, rename, directory-sync replacement. Its callers inmigrate_data_dirandretrieve_or_create_versionpassCARGO_PKG_VERSIONexplicitly, leaving normal migration behaviour unchanged.VERSION_TMP_FILE_NAMErustdoc keeps the fixedVERSION.tmpname and now explains that it makes every marker writer in a directory collide, so both startup migration and rollback marker writing require exclusive access to each affected directory.#[cfg(test)]write_versionhelper and its raw-marker call sites are retained — its non-canonical and otherwise invalid writes are exactly the setup the public API must reject.Closes #830
Test plan
write_version_markers_writes_canonical_markers— a valid version in two temporary directories putsVersion::parse(input)?.to_string()in bothVERSIONfiles, byte for byte.write_version_markers_keeps_prerelease_versions— a prerelease such as0.47.0-alpha.1survives the round trip, so the alpha format versions this crate migrates between can be recorded.write_version_markers_creates_missing_directories— absent directories, including intermediate components, are created and receive canonical markers.write_version_markers_rejects_invalid_versions—"",0.46,not a version,0.46.0, and0.46.0extraare each rejected; no directory or marker appears, a pre-existing marker keeps its original contents, and noVERSION.tmpis left behind.write_version_markers_replaces_existing_markers— a marker naming a newer format, and one stored non-canonically, are replaced rather than appended to.write_version_markers_reports_the_failing_directory_and_retries— a regular file at the backup path makes its marker operation fail with an error naming the backup directory; replacing it with a directory and retrying the same version leaves both canonical markers equal.write_version_markers_reports_a_failing_data_directory— a failure on the data marker names the data directory and stops before the backup one, so the pair is never left describing two different formats.markers_written_for_rollback_let_migration_run_again— a populated 0.46-format store built in atempdirasassert_migration_creates_new_column_familiesdoes, with both markers set to0.46.0through the public API, letsmigrate_data_dirsucceed instead of reportingmigration from 0.46.0 is not supported. It adds the current column families, converts the populated agent and external-service records, and reachesCARGO_PKG_VERSIONin both markers.CARGO_PKG_VERSIONto both markers, and that an individual replacement stays atomic, still pass withcreate_version_file's new argument.cargo fmt -- --check --config group_imports=StdExternalCratecargo clippy --bins --tests --all-features -- -D warningscargo test --all-features— 623 lib tests, 12 doctests, 2 compile-fail doctests, 0 failures.markdownlint-cli2 CHANGELOG.md