From e3e9e794597de0613c487c4dc5bb91fea21b672f Mon Sep 17 00:00:00 2001 From: Peter Permenter <41281403+TusanHomichi@users.noreply.github.com> Date: Thu, 15 Oct 2026 14:22:34 -0700 Subject: [PATCH 1/3] fix(doctor): report every migration history serve would refuse doctor compared only migration versions, so a changed checksum or a migration recorded as failed read "ok" while serve refused to start. One storage function now judges an applied history the way startup does (a version this build does not know, a row recorded as unfinished, a checksum that differs from this build's), in sqlx 0.9's order, and doctor reports each refusal as a fail naming the version and what to do, read-only as ADR 0016 requires. Startup's own newer-database check reads through the same function, so the wording is defined once. doctor_migrations runs the real binary: for each condition doctor fails and serve refuses the same database, with an intact database as the positive control; doctor_read_only proves the database and its sidecars stay byte-identical while doctor reports a changed checksum. Closes #138 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 + README.md | 4 +- crates/consolebook-server/src/doctor.rs | 74 ++--- crates/consolebook-server/src/storage.rs | 143 +++++++- .../tests/doctor_migrations.rs | 313 ++++++++++++++++++ .../tests/doctor_read_only.rs | 104 +++++- docs/install.md | 5 +- docs/operations.md | 6 + 8 files changed, 596 insertions(+), 59 deletions(-) create mode 100644 crates/consolebook-server/tests/doctor_migrations.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index b5c1cb0..6e6a8d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,12 @@ not a production-readiness statement. and passwords are byte-for-byte compatible, and a test pins the released migration checksums and the export container's bytes. +### Fixed + +- `doctor` reports a migration history `serve` would refuse (a changed + checksum or a failed migration), instead of `ok` + ([#138](https://github.com/FieldmouseWorks/consolebook/issues/138)). + ## [0.2.0] - 2026-10-15 Safer upgrades and recovery for a pilot: a backup before every migration, diff --git a/README.md b/README.md index 98176b2..fd99c7a 100644 --- a/README.md +++ b/README.md @@ -110,7 +110,9 @@ authentication, and [ADR 0005](docs/decisions/0005-embedded-web-interface.md) for the embedded interface. `doctor` opens the database read-only and reports journal mismatches without -repairing them. SQLite may create WAL sidecars or update shared-memory +repairing them. It also reports every recorded migration that `serve` would +refuse to start on: a version this build does not know, a changed checksum, +or a migration recorded as failed. SQLite may create WAL sidecars or update shared-memory coordination; read-only storage without usable sidecars can prevent diagnosis. Its connection-local PRAGMA checks describe the diagnostic connection. See [ADR 0016](docs/decisions/0016-read-only-diagnostics.md) for the precise contract. diff --git a/crates/consolebook-server/src/doctor.rs b/crates/consolebook-server/src/doctor.rs index 04e35b6..b6075f3 100644 --- a/crates/consolebook-server/src/doctor.rs +++ b/crates/consolebook-server/src/doctor.rs @@ -165,48 +165,42 @@ async fn check_invariants(pool: &SqlitePool, findings: &mut Vec) { } } +/// Reports each recorded migration `serve` would refuse to start on, using +/// the same judgment as startup's own check ([`storage::migration_history`]). async fn check_migrations(pool: &SqlitePool, findings: &mut Vec) { - let applied: Result, sqlx::Error> = - sqlx::query_scalar("SELECT version FROM _sqlx_migrations ORDER BY version") - .fetch_all(pool) - .await; - match applied { - Err(err) => findings.push(Finding::fail( - "migrations", - format!("cannot read migration history: {err}"), - )), - Ok(applied) => { - let expected: Vec = storage::MIGRATOR.iter().map(|m| m.version).collect(); - let missing: Vec = expected - .iter() - .filter(|v| !applied.contains(v)) - .map(ToString::to_string) - .collect(); - let unknown: Vec = applied - .iter() - .filter(|v| !expected.contains(v)) - .map(ToString::to_string) - .collect(); - if missing.is_empty() && unknown.is_empty() { - findings.push(Finding::ok( - "migrations", - format!("{} applied, up to date", applied.len()), - )); - } else if !missing.is_empty() { - findings.push(Finding::fail( - "migrations", - format!("pending: {}", missing.join(", ")), - )); - } else { - findings.push(Finding::fail( - "migrations", - format!( - "database has migrations this build does not know: {}", - unknown.join(", ") - ), - )); - } + const CHECK: &str = "migrations"; + let history = match storage::migration_history(pool).await { + Err(err) => { + findings.push(Finding::fail( + CHECK, + format!("cannot read migration history: {err:#}"), + )); + return; + } + Ok(None) => { + findings.push(Finding::fail( + CHECK, + "cannot read migration history: the database has no _sqlx_migrations table", + )); + return; } + Ok(Some(history)) => history, + }; + for refusal in &history.refusals { + findings.push(Finding::fail(CHECK, refusal.to_string())); + } + if !history.unapplied.is_empty() { + let pending: Vec = history.unapplied.iter().map(ToString::to_string).collect(); + findings.push(Finding::fail( + CHECK, + format!("pending: {}", pending.join(", ")), + )); + } + if history.refusals.is_empty() && history.unapplied.is_empty() { + findings.push(Finding::ok( + CHECK, + format!("{} applied, up to date", history.recorded), + )); } } diff --git a/crates/consolebook-server/src/storage.rs b/crates/consolebook-server/src/storage.rs index 2bff5f8..d9820df 100644 --- a/crates/consolebook-server/src/storage.rs +++ b/crates/consolebook-server/src/storage.rs @@ -86,10 +86,8 @@ pub async fn open(db_path: &Path) -> Result { let pool = connect(db_path, true).await?; let newer = newer_migrations(&pool).await?; - if let Some(version) = newer.first() { - bail!( - "this database was upgraded by a newer Consolebook (it has migration {version}, which this version does not know); install that release again, or restore the snapshot taken before the upgrade (docs/install.md, \"Upgrading\")" - ); + if let Some(&version) = newer.first() { + bail!("{}", RefusedHistory::Newer(version)); } MIGRATOR @@ -119,7 +117,14 @@ pub async fn open(db_path: &Path) -> Result { /// Empty for a database that is up to date, and for one that has never been /// migrated (a new installation), which has nothing to lose. Reads only. pub async fn pending_migrations(pool: &SqlitePool) -> Result> { - let applied = applied_migrations(pool).await?; + let Some(recorded) = recorded_history(pool).await? else { + return Ok(Vec::new()); + }; + let applied: Vec = recorded + .iter() + .filter(|row| row.success) + .map(|row| row.version) + .collect(); if applied.is_empty() { return Ok(Vec::new()); } @@ -134,16 +139,112 @@ pub async fn pending_migrations(pool: &SqlitePool) -> Result> { /// Applied migrations this executable does not know: the database was /// upgraded by a newer release. Reads only. pub async fn newer_migrations(pool: &SqlitePool) -> Result> { - Ok(applied_migrations(pool) - .await? + let recorded = recorded_history(pool).await?.unwrap_or_default(); + Ok(refusals(&recorded) .into_iter() - .filter(|version| MIGRATOR.iter().all(|known| known.version != *version)) + .filter_map(|refusal| match refusal { + RefusedHistory::Newer(version) => Some(version), + _ => None, + }) .collect()) } -/// Successfully applied migration versions, in order; empty when the -/// database has never been migrated. -async fn applied_migrations(pool: &SqlitePool) -> Result> { +/// A recorded migration this executable refuses to start on. [`open`] +/// refuses each of these, through its own check or `SQLx`'s migrator +/// (`sqlx-core` 0.9 `Migrator::run_direct`): a version it does not embed +/// (`ignore_missing` is off), a row recorded as unsuccessful, or a stored +/// checksum that differs from the embedded migration's. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum RefusedHistory { + /// A newer release applied a migration this build does not embed. + Newer(i64), + /// The migration started but is recorded as not successful. + Unfinished(i64), + /// The stored checksum differs from this build's migration. + Changed(i64), +} + +impl std::fmt::Display for RefusedHistory { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Newer(version) => write!( + f, + "this database was upgraded by a newer Consolebook (it has migration {version}, which this version does not know); install that release again, or restore the snapshot taken before the upgrade (docs/install.md, \"Upgrading\")" + ), + Self::Unfinished(version) => write!( + f, + "migration {version} did not finish (it is recorded as failed), and the service refuses to start on it; restore the snapshot taken before the upgrade (docs/install.md, \"Upgrading\")" + ), + Self::Changed(version) => write!( + f, + "migration {version} was applied by a different build's version of it (its recorded checksum differs from this build's), and the service refuses to start on it; restore a backup made by this build, or run the build that applied it" + ), + } + } +} + +/// The recorded migration history, judged against this build. Reads only. +#[derive(Debug)] +pub struct MigrationHistory { + /// Rows recorded in `_sqlx_migrations`, successful or not. + pub recorded: usize, + /// Every recorded migration startup would refuse, in version order. + pub refusals: Vec, + /// Embedded migrations with no recorded row, which startup applies. + pub unapplied: Vec, +} + +/// Judges the recorded history; `None` when the database has no migration +/// table. Reads only, so `doctor` can use it (ADR 0016). +pub async fn migration_history(pool: &SqlitePool) -> Result> { + let Some(recorded) = recorded_history(pool).await? else { + return Ok(None); + }; + let unapplied = MIGRATOR + .iter() + .filter(|migration| migration.migration_type.is_up_migration()) + .map(|migration| migration.version) + .filter(|version| recorded.iter().all(|row| row.version != *version)) + .collect(); + Ok(Some(MigrationHistory { + recorded: recorded.len(), + refusals: refusals(&recorded), + unapplied, + })) +} + +/// One row of `_sqlx_migrations`. +struct RecordedMigration { + version: i64, + checksum: Vec, + success: bool, +} + +/// Each row's refusal, in the order `SQLx` checks: unsuccessful rows, then +/// versions this build does not embed, then checksums of up migrations. +fn refusals(recorded: &[RecordedMigration]) -> Vec { + recorded + .iter() + .filter_map(|row| { + if !row.success { + return Some(RefusedHistory::Unfinished(row.version)); + } + if !MIGRATOR.version_exists(row.version) { + return Some(RefusedHistory::Newer(row.version)); + } + MIGRATOR + .iter() + .filter(|migration| migration.migration_type.is_up_migration()) + .find(|migration| migration.version == row.version) + .filter(|migration| *migration.checksum != *row.checksum) + .map(|_| RefusedHistory::Changed(row.version)) + }) + .collect() +} + +/// Every recorded migration row, in version order; `None` when the database +/// has never been migrated. +async fn recorded_history(pool: &SqlitePool) -> Result>> { let tracked: Option = sqlx::query_scalar( "SELECT name FROM sqlite_master WHERE type = 'table' AND name = '_sqlx_migrations'", ) @@ -151,12 +252,22 @@ async fn applied_migrations(pool: &SqlitePool) -> Result> { .await .context("looking for the migrations table")?; if tracked.is_none() { - return Ok(Vec::new()); + return Ok(None); } - sqlx::query_scalar("SELECT version FROM _sqlx_migrations WHERE success = 1 ORDER BY version") - .fetch_all(pool) - .await - .context("reading applied migrations") + let rows: Vec<(i64, Vec, bool)> = + sqlx::query_as("SELECT version, checksum, success FROM _sqlx_migrations ORDER BY version") + .fetch_all(pool) + .await + .context("reading applied migrations")?; + Ok(Some( + rows.into_iter() + .map(|(version, checksum, success)| RecordedMigration { + version, + checksum, + success, + }) + .collect(), + )) } /// Opens an existing database without creating one and without migrating. diff --git a/crates/consolebook-server/tests/doctor_migrations.rs b/crates/consolebook-server/tests/doctor_migrations.rs new file mode 100644 index 0000000..e544d56 --- /dev/null +++ b/crates/consolebook-server/tests/doctor_migrations.rs @@ -0,0 +1,313 @@ +//! `doctor` reports every applied migration history that `serve` refuses +//! (#138), and says the history is fine only when `serve` would accept it. +//! +//! Each refusing condition is shown twice on the same scratch database: a +//! `FAIL` from `doctor` and a refusal from the real `serve` command, so the +//! parity is observed rather than assumed. The intact installation is the +//! positive control for both. + +use std::io::BufRead; +use std::process::{Command, ExitStatus, Output, Stdio}; +use std::sync::mpsc::RecvTimeoutError; +use std::time::{Duration, Instant}; + +use consolebook_server::data_dir::DataDir; +use consolebook_server::doctor::{self, Finding, Verdict}; +use consolebook_server::storage::{self, MIGRATOR}; + +/// A migrated, stopped installation. +async fn installation() -> (tempfile::TempDir, DataDir) { + let tmp = tempfile::tempdir().expect("scratch directory"); + let data = DataDir::new(tmp.path().join("data")); + data.ensure_layout().expect("layout"); + storage::open(&data.database()) + .await + .expect("initialize") + .close() + .await; + (tmp, data) +} + +fn newest_version() -> i64 { + MIGRATOR + .iter() + .map(|migration| migration.version) + .max() + .expect("embedded migrations") +} + +/// Rewrites the migration history the way a different build, a crash, or a +/// hand edit would leave it. +async fn alter_history(data: &DataDir, statement: &'static str, version: i64) { + let pool = storage::open_existing(&data.database()) + .await + .expect("open for the fixture"); + let changed = sqlx::query(statement) + .bind(version) + .execute(&pool) + .await + .expect("alter migration history") + .rows_affected(); + pool.close().await; + assert_eq!(changed, 1, "{statement}"); +} + +async fn migration_findings(data: &DataDir) -> Vec { + doctor::run(data, &doctor::Options::default()) + .await + .into_iter() + .filter(|finding| finding.check == "migrations") + .collect() +} + +fn command(data: &DataDir) -> Command { + let mut command = Command::new(env!("CARGO_BIN_EXE_consolebook-server")); + // Only what the test passes reaches the process. + command.env_clear().arg("--data-dir").arg(data.root()); + command +} + +fn run_doctor(data: &DataDir) -> Output { + command(data) + .arg("doctor") + .output() + .expect("doctor process") +} + +/// How the real `serve` command ended. +#[derive(Debug)] +enum Served { + /// It logged that it is listening: it accepted the database. + Listening, + /// It exited first, with this status and log (`serve` logs to stdout). + Exited(ExitStatus, String), +} + +/// Runs `serve` until it either exits or starts listening, then stops it. +fn run_serve(data: &DataDir) -> Served { + let mut child = command(data) + .args(["serve", "--bind", "127.0.0.1:0"]) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn() + .expect("serve process"); + let stdout = child.stdout.take().expect("serve stdout"); + let (lines, logged) = std::sync::mpsc::channel(); + std::thread::spawn(move || { + for line in BufRead::lines(std::io::BufReader::new(stdout)) { + let Ok(line) = line else { break }; + if lines.send(line).is_err() { + break; + } + } + }); + let deadline = Instant::now() + Duration::from_secs(60); + let mut log = String::new(); + loop { + let left = deadline.saturating_duration_since(Instant::now()); + match logged.recv_timeout(left) { + Ok(line) if line.contains("listening") => { + child.kill().expect("stop serve"); + child.wait().expect("reap serve"); + return Served::Listening; + } + Ok(line) => { + log.push_str(&line); + log.push('\n'); + } + // Standard output closed: the process exited. + Err(RecvTimeoutError::Disconnected) => { + return Served::Exited(child.wait().expect("serve status"), log); + } + Err(RecvTimeoutError::Timeout) => { + child.kill().expect("stop serve"); + panic!("serve neither refused nor started listening within a minute: {log}"); + } + } + } +} + +fn assert_serve_refuses(data: &DataDir, reason: &str) { + match run_serve(data) { + Served::Exited(status, log) => { + assert_eq!(status.code(), Some(1), "{log}"); + assert!(log.contains(reason), "{log}"); + } + Served::Listening => panic!("serve must refuse this database, not start"), + } +} + +/// Whether `text` names migration `version` (and not, say, `version`0). +fn names_migration(text: &str, version: i64) -> bool { + let needle = format!("migration {version}"); + text.match_indices(&needle).any(|(at, _)| { + !text[at + needle.len()..] + .chars() + .next() + .is_some_and(|next| next.is_ascii_digit()) + }) +} + +fn assert_doctor_exits_failing(data: &DataDir, version: i64) { + let output = run_doctor(data); + assert_eq!(output.status.code(), Some(1), "{output:?}"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.lines().any(|line| line.starts_with("FAIL") + && line.contains("migrations") + && names_migration(line, version)), + "{stdout}" + ); +} + +/// The one migrations finding is a failure that names `version`. +fn assert_fails_naming(findings: &[Finding], version: i64, meaning: &str) -> String { + assert_eq!(findings.len(), 1, "{findings:?}"); + let finding = &findings[0]; + assert_eq!(finding.verdict, Verdict::Fail, "{finding:?}"); + assert!(names_migration(&finding.detail, version), "{finding:?}"); + assert!(finding.detail.contains(meaning), "{finding:?}"); + finding.detail.clone() +} + +#[tokio::test] +async fn an_intact_history_is_ok_and_the_service_starts() { + let (_tmp, data) = installation().await; + + let findings = migration_findings(&data).await; + assert_eq!(findings.len(), 1, "{findings:?}"); + assert_eq!(findings[0].verdict, Verdict::Ok, "{findings:?}"); + assert_eq!( + findings[0].detail, + format!("{} applied, up to date", MIGRATOR.iter().count()) + ); + let output = run_doctor(&data); + assert!(output.status.success(), "{output:?}"); + + let served = run_serve(&data); + assert!( + matches!(served, Served::Listening), + "serve must accept an intact history: {served:?}" + ); +} + +#[tokio::test] +async fn a_changed_checksum_fails_doctor_and_the_service_refuses_it() { + let (_tmp, data) = installation().await; + let version = newest_version(); + alter_history( + &data, + "UPDATE _sqlx_migrations SET checksum = zeroblob(length(checksum)) WHERE version = ?1", + version, + ) + .await; + + let detail = assert_fails_naming( + &migration_findings(&data).await, + version, + "a different build", + ); + assert!(detail.contains("restore"), "{detail}"); + assert_doctor_exits_failing(&data, version); + + assert_serve_refuses( + &data, + &format!("migration {version} was previously applied but has been modified"), + ); +} + +#[tokio::test] +async fn a_failed_migration_fails_doctor_and_the_service_refuses_it() { + let (_tmp, data) = installation().await; + let version = newest_version(); + alter_history( + &data, + "UPDATE _sqlx_migrations SET success = 0 WHERE version = ?1", + version, + ) + .await; + + // Reported once, as failed, and not also as pending. + assert_fails_naming(&migration_findings(&data).await, version, "did not finish"); + assert_doctor_exits_failing(&data, version); + + assert_serve_refuses(&data, &format!("migration {version} is partially applied")); +} + +#[tokio::test] +async fn a_migration_this_build_does_not_embed_fails_doctor_and_the_service_refuses_it() { + let (_tmp, data) = installation().await; + let future = newest_version() + 1; + alter_history( + &data, + "INSERT INTO _sqlx_migrations (version, description, success, checksum, execution_time) + VALUES (?1, 'invented future migration', 1, X'00', 0)", + future, + ) + .await; + + // The same words startup uses for this condition. + assert_fails_naming( + &migration_findings(&data).await, + future, + "upgraded by a newer Consolebook", + ); + assert_doctor_exits_failing(&data, future); + + assert_serve_refuses(&data, "upgraded by a newer Consolebook"); +} + +#[tokio::test] +async fn every_refusing_condition_is_reported_not_only_the_first() { + let (_tmp, data) = installation().await; + let first = MIGRATOR.iter().next().expect("migrations").version; + let newest = newest_version(); + alter_history( + &data, + "UPDATE _sqlx_migrations SET checksum = zeroblob(length(checksum)) WHERE version = ?1", + first, + ) + .await; + alter_history( + &data, + "UPDATE _sqlx_migrations SET success = 0 WHERE version = ?1", + newest, + ) + .await; + + let findings = migration_findings(&data).await; + assert_eq!(findings.len(), 2, "{findings:?}"); + assert!(findings.iter().all(|f| f.verdict == Verdict::Fail)); + assert!(names_migration(&findings[0].detail, first), "{findings:?}"); + assert!(names_migration(&findings[1].detail, newest), "{findings:?}"); +} + +#[tokio::test] +async fn a_pending_migration_is_still_reported_as_pending() { + let (_tmp, data) = installation().await; + let version = newest_version(); + alter_history( + &data, + "DELETE FROM _sqlx_migrations WHERE version = ?1", + version, + ) + .await; + + let findings = migration_findings(&data).await; + assert_eq!(findings.len(), 1, "{findings:?}"); + assert_eq!(findings[0].detail, format!("pending: {version}")); +} + +/// `storage::migration_history` mirrors what `SQLx`'s migrator refuses under +/// this configuration. If it changes (missing versions ignored, or another +/// table), the mirror and these tests must change with it. +#[test] +fn the_migrator_configuration_the_diagnosis_assumes() { + assert!(!MIGRATOR.ignore_missing); + assert_eq!(MIGRATOR.table_name, "_sqlx_migrations"); + assert!( + MIGRATOR + .iter() + .all(|migration| migration.migration_type.is_up_migration()) + ); +} diff --git a/crates/consolebook-server/tests/doctor_read_only.rs b/crates/consolebook-server/tests/doctor_read_only.rs index 1996589..6bc16ce 100644 --- a/crates/consolebook-server/tests/doctor_read_only.rs +++ b/crates/consolebook-server/tests/doctor_read_only.rs @@ -166,9 +166,63 @@ async fn doctor_reads_stopped_wal_installation_without_changing_database_bytes() } } +const CHANGE_NEWEST_CHECKSUM: &str = + "UPDATE _sqlx_migrations SET checksum = zeroblob(length(checksum)) + WHERE version = (SELECT MAX(version) FROM _sqlx_migrations)"; + +fn newest_migration() -> i64 { + storage::MIGRATOR + .iter() + .map(|migration| migration.version) + .max() + .expect("migrations") +} + +/// A live writer holds the changed checksum only in its WAL, so the finding +/// proves `doctor` read the live history, and both files stay byte-identical. +#[tokio::test] +async fn doctor_reports_a_changed_migration_checksum_in_live_wal_without_changing_bytes() { + let (_tmp, data) = installation(); + let writer = storage::open(&data.database()).await.expect("initialize"); + let mut connection = writer.acquire().await.expect("writer connection"); + sqlx::query("PRAGMA wal_checkpoint(TRUNCATE)") + .execute(&mut *connection) + .await + .expect("checkpoint fixture"); + let changed = sqlx::query(CHANGE_NEWEST_CHECKSUM) + .execute(&mut *connection) + .await + .expect("alter one checksum in WAL") + .rows_affected(); + assert_eq!(changed, 1); + let wal_path = data.root().join("consolebook.db-wal"); + let before = std::fs::read(data.database()).expect("before"); + let wal_before = std::fs::read(&wal_path).expect("WAL before"); + assert!(wal_before.len() > 32, "fixture needs committed WAL frames"); + + let findings = doctor::run(&data, &doctor::Options::default()).await; + let failures: Vec<_> = findings + .iter() + .filter(|f| f.verdict == doctor::Verdict::Fail) + .collect(); + assert_eq!(failures.len(), 1, "{findings:?}"); + assert_eq!(failures[0].check, "migrations"); + let named = format!( + "migration {} was applied by a different build", + newest_migration() + ); + assert!(failures[0].detail.contains(&named), "{findings:?}"); + assert_eq!(before, std::fs::read(data.database()).expect("after")); + assert_eq!(wal_before, std::fs::read(wal_path).expect("WAL after")); + drop(connection); + writer.close().await; +} + #[cfg(unix)] mod permissions { - use super::{initialize_stopped, installation, storage}; + use super::{ + CHANGE_NEWEST_CHECKSUM, initialize_stopped, installation, newest_migration, storage, + }; use std::os::unix::fs::PermissionsExt; fn run_doctor(data: &consolebook_server::data_dir::DataDir) -> std::process::Output { @@ -234,6 +288,54 @@ mod permissions { assert!(!data.root().join("consolebook.db-shm").exists()); } + /// The database and both sidecars stay byte-identical while `doctor` + /// reports a changed migration checksum and exits non-zero. + #[tokio::test] + async fn a_changed_migration_checksum_fails_without_changing_database_or_sidecars() { + let (_tmp, data) = installation(); + let writer = storage::open(&data.database()).await.expect("initialize"); + sqlx::query(CHANGE_NEWEST_CHECKSUM) + .execute(&writer) + .await + .expect("alter one checksum"); + let permissions = ReadOnlyDirectory::new(data.root()); + assert!( + std::fs::File::create(data.root().join("write-probe")).is_err(), + "run permission tests as an unprivileged user" + ); + let paths = [ + data.database(), + data.root().join("consolebook.db-wal"), + data.root().join("consolebook.db-shm"), + ]; + let before: Vec<_> = paths + .iter() + .map(|p| std::fs::read(p).expect("before")) + .collect(); + let output = run_doctor(&data); + assert_eq!(output.status.code(), Some(1), "{output:?}"); + let stdout = String::from_utf8_lossy(&output.stdout); + let named = format!( + "migration {} was applied by a different build", + newest_migration() + ); + assert!( + stdout + .lines() + .any(|line| line.starts_with("FAIL") && line.contains(&named)), + "{stdout}" + ); + for (path, bytes) in paths.iter().zip(before) { + assert!( + bytes == std::fs::read(path).expect("after"), + "changed {}", + path.display() + ); + } + drop(permissions); + writer.close().await; + } + #[tokio::test] async fn readable_live_wal_sidecars_allow_diagnosis_in_read_only_directory() { let (_tmp, data) = installation(); diff --git a/docs/install.md b/docs/install.md index 6588cd4..63453a1 100644 --- a/docs/install.md +++ b/docs/install.md @@ -143,7 +143,10 @@ invented example, and shows backup health. Or work through the list by hand: Run the same install command again. It replaces the program and the service definition, keeps `/var/lib/consolebook` and your settings, restarts the service, and prints the address. The database is upgraded on start, and -startup refuses to run against data it cannot verify. +startup refuses to run against data it cannot verify. `consolebook doctor` +reports, without changing anything, each recorded migration the service +would refuse: one this version does not know, one changed since it was +applied (a different checksum), or one recorded as failed. When a new version changes the database, it first takes a validated backup of it as it was, `backups/consolebook-premigration-.db`, and changes diff --git a/docs/operations.md b/docs/operations.md index a0bb390..cac4cf2 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -60,6 +60,12 @@ only on `FAIL`. Look at two lines: you set `CONSOLEBOOK_BACKUP_INTERVAL_HOURS` to something else, pass the same value: `--backup-interval-hours 6`. +Every other line should be `ok`. The `migrations` line compares each +migration the database records with this version's: its number, its +checksum, and whether it finished. A `FAIL` there names the migration and +means the service will refuse to start on this database; the line says what +to do next. + An administrator also sees backup health on their home page, and gets a notice when a copy fails. From ee46ad8792f59fb61d324ffaa723de3a93257641 Mon Sep 17 00:00:00 2001 From: Peter Permenter <41281403+TusanHomichi@users.noreply.github.com> Date: Thu, 15 Oct 2026 14:34:47 -0700 Subject: [PATCH 2/3] fix(doctor): warn, not fail, on migrations the service applies by itself doctor now matches serve in both directions: a refusal is a fail, while pending migrations and a database with no migration history yet are a warning that says what the service does on its next start (take a validated backup, then migrate), so running doctor between installing a new version and restarting no longer exits non-zero. Parity tests run the real binary: one migration behind warns, serve starts, takes the pre-migration backup, and doctor then reports ok. Review follow-ups: the Changed and Unfinished messages say what is known and where to go (docs/install.md, Upgrading); the read-only-directory test claims what it proves; the ordering comment is accurate; a timed-out serve is reaped; the operations guide says only named-migration lines mean the service will refuse. Refs #138 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 +- README.md | 5 +- crates/consolebook-server/src/doctor.rs | 38 ++++--- crates/consolebook-server/src/storage.rs | 14 ++- .../tests/doctor_migrations.rs | 104 ++++++++++++++++-- .../tests/doctor_read_only.rs | 8 +- docs/install.md | 2 +- docs/operations.md | 8 +- 8 files changed, 145 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e6a8d0..08d1885 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,7 +43,8 @@ not a production-readiness statement. ### Fixed - `doctor` reports a migration history `serve` would refuse (a changed - checksum or a failed migration), instead of `ok` + checksum or a failed migration), instead of `ok`. Pending migrations, + which the service applies on its next start, now warn instead of failing ([#138](https://github.com/FieldmouseWorks/consolebook/issues/138)). ## [0.2.0] - 2026-10-15 diff --git a/README.md b/README.md index fd99c7a..e95d7f8 100644 --- a/README.md +++ b/README.md @@ -112,8 +112,9 @@ for the embedded interface. `doctor` opens the database read-only and reports journal mismatches without repairing them. It also reports every recorded migration that `serve` would refuse to start on: a version this build does not know, a changed checksum, -or a migration recorded as failed. SQLite may create WAL sidecars or update shared-memory -coordination; read-only storage without usable sidecars can prevent diagnosis. +or a migration recorded as unsuccessful. SQLite may create WAL sidecars or +update shared-memory coordination; read-only storage without usable sidecars +can prevent diagnosis. Its connection-local PRAGMA checks describe the diagnostic connection. See [ADR 0016](docs/decisions/0016-read-only-diagnostics.md) for the precise contract. diff --git a/crates/consolebook-server/src/doctor.rs b/crates/consolebook-server/src/doctor.rs index b6075f3..9786db7 100644 --- a/crates/consolebook-server/src/doctor.rs +++ b/crates/consolebook-server/src/doctor.rs @@ -167,8 +167,12 @@ async fn check_invariants(pool: &SqlitePool, findings: &mut Vec) { /// Reports each recorded migration `serve` would refuse to start on, using /// the same judgment as startup's own check ([`storage::migration_history`]). +/// What the service does by itself on its next start (create the history, +/// apply pending migrations) is a warning, never a failure: `doctor` fails +/// only where `serve` would refuse. async fn check_migrations(pool: &SqlitePool, findings: &mut Vec) { const CHECK: &str = "migrations"; + const NO_HISTORY: &str = "no migration history yet; the service creates it and applies every migration on its first start"; let history = match storage::migration_history(pool).await { Err(err) => { findings.push(Finding::fail( @@ -178,30 +182,38 @@ async fn check_migrations(pool: &SqlitePool, findings: &mut Vec) { return; } Ok(None) => { - findings.push(Finding::fail( - CHECK, - "cannot read migration history: the database has no _sqlx_migrations table", - )); + findings.push(Finding::warn(CHECK, NO_HISTORY)); return; } Ok(Some(history)) => history, }; - for refusal in &history.refusals { - findings.push(Finding::fail(CHECK, refusal.to_string())); + if !history.refusals.is_empty() { + // The service will not start, so nothing pending would be applied. + for refusal in &history.refusals { + findings.push(Finding::fail(CHECK, refusal.to_string())); + } + return; } - if !history.unapplied.is_empty() { - let pending: Vec = history.unapplied.iter().map(ToString::to_string).collect(); - findings.push(Finding::fail( - CHECK, - format!("pending: {}", pending.join(", ")), - )); + if history.recorded == 0 { + findings.push(Finding::warn(CHECK, NO_HISTORY)); + return; } - if history.refusals.is_empty() && history.unapplied.is_empty() { + if history.unapplied.is_empty() { findings.push(Finding::ok( CHECK, format!("{} applied, up to date", history.recorded), )); + return; } + let pending: Vec = history.unapplied.iter().map(ToString::to_string).collect(); + findings.push(Finding::warn( + CHECK, + format!( + "{} migration(s) pending ({}); the service applies them on its next start, after taking a validated backup", + pending.len(), + pending.join(", ") + ), + )); } async fn check_integrity(pool: &SqlitePool, findings: &mut Vec) { diff --git a/crates/consolebook-server/src/storage.rs b/crates/consolebook-server/src/storage.rs index d9820df..98ea65b 100644 --- a/crates/consolebook-server/src/storage.rs +++ b/crates/consolebook-server/src/storage.rs @@ -158,7 +158,8 @@ pub async fn newer_migrations(pool: &SqlitePool) -> Result> { pub enum RefusedHistory { /// A newer release applied a migration this build does not embed. Newer(i64), - /// The migration started but is recorded as not successful. + /// The row is recorded as not successful. `SQLx` 0.9 records only + /// successes, so this comes from a hand edit or another tool. Unfinished(i64), /// The stored checksum differs from this build's migration. Changed(i64), @@ -173,11 +174,11 @@ impl std::fmt::Display for RefusedHistory { ), Self::Unfinished(version) => write!( f, - "migration {version} did not finish (it is recorded as failed), and the service refuses to start on it; restore the snapshot taken before the upgrade (docs/install.md, \"Upgrading\")" + "migration {version} is recorded as unsuccessful, and the service refuses to start on it; restore the snapshot taken before the upgrade (docs/install.md, \"Upgrading\")" ), Self::Changed(version) => write!( f, - "migration {version} was applied by a different build's version of it (its recorded checksum differs from this build's), and the service refuses to start on it; restore a backup made by this build, or run the build that applied it" + "migration {version} was applied by a different build's version of it (its recorded checksum differs from this build's), and the service refuses to start on it; restore the snapshot taken before the upgrade, or a backup taken while this version was running the service, or run the build that applied it (docs/install.md, \"Upgrading\")" ), } } @@ -220,8 +221,11 @@ struct RecordedMigration { success: bool, } -/// Each row's refusal, in the order `SQLx` checks: unsuccessful rows, then -/// versions this build does not embed, then checksums of up migrations. +/// Every row startup would refuse, in version order. Each row gets the +/// refusal `SQLx` would give it (unsuccessful first, then a version this +/// build does not embed, then an up migration's checksum). Startup stops +/// at its first refusal, the lowest unsuccessful row before any unknown +/// version before any changed checksum; `doctor` reports them all. fn refusals(recorded: &[RecordedMigration]) -> Vec { recorded .iter() diff --git a/crates/consolebook-server/tests/doctor_migrations.rs b/crates/consolebook-server/tests/doctor_migrations.rs index e544d56..983ea2f 100644 --- a/crates/consolebook-server/tests/doctor_migrations.rs +++ b/crates/consolebook-server/tests/doctor_migrations.rs @@ -121,6 +121,7 @@ fn run_serve(data: &DataDir) -> Served { } Err(RecvTimeoutError::Timeout) => { child.kill().expect("stop serve"); + child.wait().expect("reap serve"); panic!("serve neither refused nor started listening within a minute: {log}"); } } @@ -228,7 +229,11 @@ async fn a_failed_migration_fails_doctor_and_the_service_refuses_it() { .await; // Reported once, as failed, and not also as pending. - assert_fails_naming(&migration_findings(&data).await, version, "did not finish"); + assert_fails_naming( + &migration_findings(&data).await, + version, + "recorded as unsuccessful", + ); assert_doctor_exits_failing(&data, version); assert_serve_refuses(&data, &format!("migration {version} is partially applied")); @@ -282,20 +287,101 @@ async fn every_refusing_condition_is_reported_not_only_the_first() { assert!(names_migration(&findings[1].detail, newest), "{findings:?}"); } +/// A database the previous release left, one migration behind, as found +/// between installing a new version and restarting the service. +async fn installation_one_behind() -> (tempfile::TempDir, DataDir) { + let tmp = tempfile::tempdir().expect("scratch directory"); + let data = DataDir::new(tmp.path().join("data")); + data.ensure_layout().expect("layout"); + let all = &MIGRATOR.migrations; + let previous = sqlx::migrate::Migrator::with_migrations(all[..all.len() - 1].to_vec()); + let pool = sqlx::SqlitePool::connect_with( + sqlx::sqlite::SqliteConnectOptions::new() + .filename(data.database()) + .create_if_missing(true) + .journal_mode(sqlx::sqlite::SqliteJournalMode::Wal), + ) + .await + .expect("fixture connection"); + previous.run(&pool).await.expect("older migrations"); + sqlx::query("INSERT INTO instance (id, installation_id, created_at_utc) VALUES (1, 'invented-older-id', '2026-10-01T00:00:00Z')") + .execute(&pool) + .await + .expect("fixture identity"); + pool.close().await; + (tmp, data) +} + #[tokio::test] -async fn a_pending_migration_is_still_reported_as_pending() { - let (_tmp, data) = installation().await; +async fn a_pending_migration_warns_and_the_service_applies_it() { + let (_tmp, data) = installation_one_behind().await; let version = newest_version(); - alter_history( - &data, - "DELETE FROM _sqlx_migrations WHERE version = ?1", - version, + + let findings = migration_findings(&data).await; + assert_eq!(findings.len(), 1, "{findings:?}"); + assert_eq!(findings[0].verdict, Verdict::Warn, "{findings:?}"); + assert_eq!( + findings[0].detail, + format!( + "1 migration(s) pending ({version}); the service applies them on its next start, after taking a validated backup" + ) + ); + // A warning does not fail `doctor`, just as the service starts. + let output = run_doctor(&data); + assert!(output.status.success(), "{output:?}"); + + let served = run_serve(&data); + assert!( + matches!(served, Served::Listening), + "serve must apply a pending migration and start: {served:?}" + ); + // The backup the warning promises. + let premigration = std::fs::read_dir(data.backups()) + .expect("backups") + .filter_map(Result::ok) + .any(|entry| { + entry + .file_name() + .to_string_lossy() + .starts_with("consolebook-premigration-") + }); + assert!(premigration, "serve took no pre-migration backup"); + let after = migration_findings(&data).await; + assert_eq!(after.len(), 1, "{after:?}"); + assert_eq!(after[0].verdict, Verdict::Ok, "{after:?}"); +} + +#[tokio::test] +async fn a_database_without_migration_history_warns_and_the_service_creates_it() { + let tmp = tempfile::tempdir().expect("scratch directory"); + let data = DataDir::new(tmp.path().join("data")); + data.ensure_layout().expect("layout"); + // An empty WAL database: no tables at all. + let pool = sqlx::SqlitePool::connect_with( + sqlx::sqlite::SqliteConnectOptions::new() + .filename(data.database()) + .create_if_missing(true) + .journal_mode(sqlx::sqlite::SqliteJournalMode::Wal), ) - .await; + .await + .expect("empty database"); + pool.close().await; let findings = migration_findings(&data).await; assert_eq!(findings.len(), 1, "{findings:?}"); - assert_eq!(findings[0].detail, format!("pending: {version}")); + assert_eq!(findings[0].verdict, Verdict::Warn, "{findings:?}"); + assert!( + findings[0].detail.contains("no migration history yet"), + "{findings:?}" + ); + + let served = run_serve(&data); + assert!( + matches!(served, Served::Listening), + "serve must create the history and start: {served:?}" + ); + let after = migration_findings(&data).await; + assert_eq!(after[0].verdict, Verdict::Ok, "{after:?}"); } /// `storage::migration_history` mirrors what `SQLx`'s migrator refuses under diff --git a/crates/consolebook-server/tests/doctor_read_only.rs b/crates/consolebook-server/tests/doctor_read_only.rs index 6bc16ce..1217c8b 100644 --- a/crates/consolebook-server/tests/doctor_read_only.rs +++ b/crates/consolebook-server/tests/doctor_read_only.rs @@ -288,10 +288,12 @@ mod permissions { assert!(!data.root().join("consolebook.db-shm").exists()); } - /// The database and both sidecars stay byte-identical while `doctor` - /// reports a changed migration checksum and exits non-zero. + /// With live sidecars in a read-only directory, `doctor` still reads the + /// migration history and reports a changed checksum, exiting non-zero. + /// File modes block writes here, so this is not a write control; the + /// live-WAL test above is. #[tokio::test] - async fn a_changed_migration_checksum_fails_without_changing_database_or_sidecars() { + async fn a_changed_migration_checksum_is_diagnosed_in_a_read_only_directory() { let (_tmp, data) = installation(); let writer = storage::open(&data.database()).await.expect("initialize"); sqlx::query(CHANGE_NEWEST_CHECKSUM) diff --git a/docs/install.md b/docs/install.md index 63453a1..291f163 100644 --- a/docs/install.md +++ b/docs/install.md @@ -146,7 +146,7 @@ service, and prints the address. The database is upgraded on start, and startup refuses to run against data it cannot verify. `consolebook doctor` reports, without changing anything, each recorded migration the service would refuse: one this version does not know, one changed since it was -applied (a different checksum), or one recorded as failed. +applied (a different checksum), or one recorded as unsuccessful. When a new version changes the database, it first takes a validated backup of it as it was, `backups/consolebook-premigration-.db`, and changes diff --git a/docs/operations.md b/docs/operations.md index cac4cf2..3d5393c 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -62,9 +62,11 @@ value: `--backup-interval-hours 6`. Every other line should be `ok`. The `migrations` line compares each migration the database records with this version's: its number, its -checksum, and whether it finished. A `FAIL` there names the migration and -means the service will refuse to start on this database; the line says what -to do next. +checksum, and whether it is recorded as successful. A `FAIL` there names the +migration and means the service will refuse to start on this database; the +line says what to do next. A `warn` that migrations are `pending` is +expected between installing a new version and restarting: the service +applies them on its next start, after taking a validated backup. An administrator also sees backup health on their home page, and gets a notice when a copy fails. From 8d403e5c3f9298a21baf0cc00628c5511d7a857f Mon Sep 17 00:00:00 2001 From: Peter Permenter <41281403+TusanHomichi@users.noreply.github.com> Date: Thu, 15 Oct 2026 15:31:24 -0700 Subject: [PATCH 3/3] fix(doctor): fail a populated database with no migration history; record ADR 0029 A database that holds tables but has no migration history (the table dropped or emptied) now reads as a refusal: doctor fails and startup refuses it before sqlx tries migration 1 over the existing tables, both through the same storage judgment (MissingHistory). Only a pristine database, with no schema objects of its own, keeps the "no migration history yet" warning. Parity tests run the real binary for the dropped and emptied cases and for the pristine empty-table case. ADR 0029 records the verdict contract operators and monitoring read from doctor's exit status: fail means serve will refuse, warn means the service acts on its next start, ok means nothing is pending or refused. ADR 0016 and the task index link it. Refs #138 Co-Authored-By: Claude Opus 5.5 --- crates/consolebook-server/src/doctor.rs | 7 +- crates/consolebook-server/src/storage.rs | 79 +++++++++----- .../tests/doctor_migrations.rs | 100 ++++++++++++++++++ docs/decisions/0016-read-only-diagnostics.md | 2 + .../0029-doctor-migration-verdict.md | 65 ++++++++++++ docs/development.md | 2 +- docs/operations.md | 11 +- 7 files changed, 228 insertions(+), 38 deletions(-) create mode 100644 docs/decisions/0029-doctor-migration-verdict.md diff --git a/crates/consolebook-server/src/doctor.rs b/crates/consolebook-server/src/doctor.rs index 9786db7..b748aa7 100644 --- a/crates/consolebook-server/src/doctor.rs +++ b/crates/consolebook-server/src/doctor.rs @@ -181,11 +181,7 @@ async fn check_migrations(pool: &SqlitePool, findings: &mut Vec) { )); return; } - Ok(None) => { - findings.push(Finding::warn(CHECK, NO_HISTORY)); - return; - } - Ok(Some(history)) => history, + Ok(history) => history, }; if !history.refusals.is_empty() { // The service will not start, so nothing pending would be applied. @@ -195,6 +191,7 @@ async fn check_migrations(pool: &SqlitePool, findings: &mut Vec) { return; } if history.recorded == 0 { + // Pristine: anything else is a `MissingHistory` refusal above. findings.push(Finding::warn(CHECK, NO_HISTORY)); return; } diff --git a/crates/consolebook-server/src/storage.rs b/crates/consolebook-server/src/storage.rs index 98ea65b..2e03c49 100644 --- a/crates/consolebook-server/src/storage.rs +++ b/crates/consolebook-server/src/storage.rs @@ -85,9 +85,16 @@ async fn connect(db_path: &Path, create: bool) -> Result { pub async fn open(db_path: &Path) -> Result { let pool = connect(db_path, true).await?; - let newer = newer_migrations(&pool).await?; - if let Some(&version) = newer.first() { - bail!("{}", RefusedHistory::Newer(version)); + // Refusals `SQLx` would not word plainly, or would reach only after + // trying to migrate. It words the others itself. + let history = migration_history(&pool).await?; + if let Some(refusal) = history.refusals.iter().find(|refusal| { + matches!( + refusal, + RefusedHistory::Newer(_) | RefusedHistory::MissingHistory + ) + }) { + bail!("{refusal}"); } MIGRATOR @@ -136,26 +143,18 @@ pub async fn pending_migrations(pool: &SqlitePool) -> Result> { .collect()) } -/// Applied migrations this executable does not know: the database was -/// upgraded by a newer release. Reads only. -pub async fn newer_migrations(pool: &SqlitePool) -> Result> { - let recorded = recorded_history(pool).await?.unwrap_or_default(); - Ok(refusals(&recorded) - .into_iter() - .filter_map(|refusal| match refusal { - RefusedHistory::Newer(version) => Some(version), - _ => None, - }) - .collect()) -} - -/// A recorded migration this executable refuses to start on. [`open`] +/// A migration history this executable refuses to start on. [`open`] /// refuses each of these, through its own check or `SQLx`'s migrator /// (`sqlx-core` 0.9 `Migrator::run_direct`): a version it does not embed -/// (`ignore_missing` is off), a row recorded as unsuccessful, or a stored -/// checksum that differs from the embedded migration's. +/// (`ignore_missing` is off), a row recorded as unsuccessful, a stored +/// checksum that differs from the embedded migration's, or no recorded +/// history in a database that already holds a schema (migration 1 cannot +/// run over existing tables). #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum RefusedHistory { + /// No migration is recorded (the table is missing or empty), yet the + /// database already holds schema objects of its own. + MissingHistory, /// A newer release applied a migration this build does not embed. Newer(i64), /// The row is recorded as not successful. `SQLx` 0.9 records only @@ -168,6 +167,10 @@ pub enum RefusedHistory { impl std::fmt::Display for RefusedHistory { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { + Self::MissingHistory => write!( + f, + "this database's migration history is missing (no migration is recorded) while it already holds data, and the service refuses to start on it: migration 1 cannot run over the existing tables; restore the snapshot taken before the upgrade, or the newest backup of this installation (docs/install.md, \"Upgrading\")" + ), Self::Newer(version) => write!( f, "this database was upgraded by a newer Consolebook (it has migration {version}, which this version does not know); install that release again, or restore the snapshot taken before the upgrade (docs/install.md, \"Upgrading\")" @@ -187,7 +190,8 @@ impl std::fmt::Display for RefusedHistory { /// The recorded migration history, judged against this build. Reads only. #[derive(Debug)] pub struct MigrationHistory { - /// Rows recorded in `_sqlx_migrations`, successful or not. + /// Rows recorded in `_sqlx_migrations`, successful or not; zero when the + /// table is missing. pub recorded: usize, /// Every recorded migration startup would refuse, in version order. pub refusals: Vec, @@ -195,11 +199,30 @@ pub struct MigrationHistory { pub unapplied: Vec, } -/// Judges the recorded history; `None` when the database has no migration -/// table. Reads only, so `doctor` can use it (ADR 0016). -pub async fn migration_history(pool: &SqlitePool) -> Result> { - let Some(recorded) = recorded_history(pool).await? else { - return Ok(None); +/// Judges the recorded history against this build. Startup ([`open`]) +/// refuses on its refusals and `doctor` reports them. Reads only, so +/// `doctor` can use it (ADR 0016). +/// +/// With no recorded migration, a database is pristine only when it holds no +/// schema objects besides `SQLite`'s own and the migrations table; anything +/// else is [`RefusedHistory::MissingHistory`]. +pub async fn migration_history(pool: &SqlitePool) -> Result { + let recorded = recorded_history(pool).await?.unwrap_or_default(); + let refusals = if recorded.is_empty() { + let objects: i64 = sqlx::query_scalar( + "SELECT COUNT(*) FROM sqlite_master + WHERE name NOT LIKE 'sqlite\\_%' ESCAPE '\\' AND name != '_sqlx_migrations'", + ) + .fetch_one(pool) + .await + .context("looking for an existing schema")?; + if objects == 0 { + Vec::new() + } else { + vec![RefusedHistory::MissingHistory] + } + } else { + refusals(&recorded) }; let unapplied = MIGRATOR .iter() @@ -207,11 +230,11 @@ pub async fn migration_history(pool: &SqlitePool) -> Result (tempfile::TempDir, DataDir) { + let tmp = tempfile::tempdir().expect("scratch directory"); + let data = DataDir::new(tmp.path().join("data")); + data.ensure_layout().expect("layout"); + let pool = sqlx::SqlitePool::connect_with( + sqlx::sqlite::SqliteConnectOptions::new() + .filename(data.database()) + .create_if_missing(true) + .journal_mode(sqlx::sqlite::SqliteJournalMode::Wal), + ) + .await + .expect("empty database"); + if with_empty_history { + // The table `SQLx` 0.9 creates, with no rows. + sqlx::query( + "CREATE TABLE _sqlx_migrations ( + version BIGINT PRIMARY KEY, + description TEXT NOT NULL, + installed_on TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, + success BOOLEAN NOT NULL, + checksum BLOB NOT NULL, + execution_time BIGINT NOT NULL + )", + ) + .execute(&pool) + .await + .expect("empty migrations table"); + } + pool.close().await; + (tmp, data) +} + +#[tokio::test] +async fn an_empty_migrations_table_on_an_empty_database_warns_and_the_service_starts() { + let (_tmp, data) = pristine_installation(true).await; + + let findings = migration_findings(&data).await; + assert_eq!(findings.len(), 1, "{findings:?}"); + assert_eq!(findings[0].verdict, Verdict::Warn, "{findings:?}"); + assert!( + findings[0].detail.contains("no migration history yet"), + "{findings:?}" + ); + + let served = run_serve(&data); + assert!( + matches!(served, Served::Listening), + "serve must apply every migration and start: {served:?}" + ); +} + +/// A populated installation whose migration history was then lost. +async fn populated_without_history(statement: &'static str) -> (tempfile::TempDir, DataDir) { + let (tmp, data) = installation().await; + let pool = storage::open_existing(&data.database()) + .await + .expect("open for the fixture"); + sqlx::query(statement) + .execute(&pool) + .await + .expect("lose the migration history"); + pool.close().await; + (tmp, data) +} + +const MISSING_HISTORY: &str = "migration history is missing"; + +async fn assert_missing_history_refused(data: &DataDir) { + let findings = migration_findings(data).await; + assert_eq!(findings.len(), 1, "{findings:?}"); + assert_eq!(findings[0].verdict, Verdict::Fail, "{findings:?}"); + assert!(findings[0].detail.contains(MISSING_HISTORY), "{findings:?}"); + + let output = run_doctor(data); + assert_eq!(output.status.code(), Some(1), "{output:?}"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout + .lines() + .any(|line| line.starts_with("FAIL") && line.contains(MISSING_HISTORY)), + "{stdout}" + ); + + assert_serve_refuses(data, MISSING_HISTORY); +} + +#[tokio::test] +async fn a_populated_database_with_its_migrations_table_dropped_fails_and_serve_refuses() { + let (_tmp, data) = populated_without_history("DROP TABLE _sqlx_migrations").await; + assert_missing_history_refused(&data).await; +} + +#[tokio::test] +async fn a_populated_database_with_its_migrations_table_emptied_fails_and_serve_refuses() { + let (_tmp, data) = populated_without_history("DELETE FROM _sqlx_migrations").await; + assert_missing_history_refused(&data).await; +} + /// `storage::migration_history` mirrors what `SQLx`'s migrator refuses under /// this configuration. If it changes (missing versions ignored, or another /// table), the mirror and these tests must change with it. diff --git a/docs/decisions/0016-read-only-diagnostics.md b/docs/decisions/0016-read-only-diagnostics.md index 94f766b..d39925c 100644 --- a/docs/decisions/0016-read-only-diagnostics.md +++ b/docs/decisions/0016-read-only-diagnostics.md @@ -4,6 +4,8 @@ - **Date:** 2026-09-05 - **Issue:** [#56](https://github.com/FieldmouseWorks/consolebook/issues/56) - **Amends:** [ADR 0003](0003-sqlite-connection-invariants.md) +- **Amended by:** [ADR 0029](0029-doctor-migration-verdict.md) (the migration + verdict matches what `serve` will do) ## Context diff --git a/docs/decisions/0029-doctor-migration-verdict.md b/docs/decisions/0029-doctor-migration-verdict.md new file mode 100644 index 0000000..91973e3 --- /dev/null +++ b/docs/decisions/0029-doctor-migration-verdict.md @@ -0,0 +1,65 @@ +# ADR 0029: doctor's migration verdict matches what serve will do + +- **Status:** Accepted +- **Date:** 2026-10-15 +- **Issue:** [#138](https://github.com/FieldmouseWorks/consolebook/issues/138) +- **Amends:** [ADR 0016](0016-read-only-diagnostics.md) (adds the migration + verdict to the read-only diagnosis; the read-only contract is unchanged) + +## Context + +`doctor` exits non-zero when any check reports `fail` and zero otherwise. +Operators and monitoring read that exit status, so what each check counts +as a failure is a contract. + +The migrations check compared migration versions only. That made its verdict +disagree with `serve` in both directions: + +- **Doctor said ok, serve refused.** A database whose recorded checksum + differed from this build's, or whose migration was recorded as unsuccessful, + read `ok`. `serve` refused to start on it ("migration 16 was previously + applied but has been modified"). Found while proving #136. +- **Doctor said fail, serve started.** Pending migrations read `fail`, yet + `serve` applies them on start after a validated backup (ADR 0023). An operator + who installs a new version and runs `doctor` before restarting was told the + installation had failed. + +## Decision + +- **One owning judgment.** A single storage function judges an applied + migration history the way startup does, and both `doctor` and startup's own + newer-database check read through it. Its refusal list was derived from + sqlx 0.9's migrator: a version this build does not know, a row recorded as + unsuccessful, and a checksum that differs from this build's. One refusal is + this product's own: a database that already holds Consolebook tables but has + no migration history, because migration 1 cannot run over existing tables. +- **`fail` means the service will refuse to start.** Each refusal is a `fail` + finding that names the migration and says what to do, so `doctor` exits + non-zero exactly where `serve` would refuse. +- **`warn` means the service will act on its own next start.** Pending + migrations are a `warn`: the service takes a validated backup, then + migrates. A pristine database with no history yet is also a `warn`: it holds + no tables, and the service creates the history on first start. Neither + changes the exit status. +- **`ok` means nothing is pending and nothing would be refused.** +- **The diagnosis stays read-only, as ADR 0016 requires.** It runs SELECTs on + the read-only connection, and tests prove the database and its sidecars are + byte-identical afterwards. +- **Real-binary parity tests (`tests/doctor_migrations.rs`) are the guard.** + For each case they run `doctor` and `serve` on the same database, so the two + verdicts cannot drift apart unnoticed. + +## Consequences + +- **Exit status changes for monitoring.** A monitor that alerted on + `doctor`'s exit status during the window between installing a new version + and restarting no longer alerts. A monitor that needs to see pending + migrations must read the `warn` line, not the exit status. +- **A tampered or foreign migration history now fails `doctor` before a + restart attempts it.** +- **Startup itself still delegates part of the judgment to sqlx.** The + unfinished-row and changed-checksum refusals come from sqlx's migrator at + startup. Because sqlx applies pending migrations in order while validating, + startup can change a database before it refuses one. #142 tracks moving + startup onto the same pre-check, so that a refusal changes nothing. When it + lands, this ADR's "one owning judgment" covers startup completely. diff --git a/docs/development.md b/docs/development.md index 36185e6..8ca2f3d 100644 --- a/docs/development.md +++ b/docs/development.md @@ -14,7 +14,7 @@ tests show what is implemented. [Roadmap](roadmap.md) owns milestone status. | Task | Start in `crates/consolebook-server/src/` | Supporting context | | --- | --- | --- | -| Process, storage, diagnostics | `main.rs`, `data_dir.rs`, `storage.rs`, `doctor.rs` | [Architecture](architecture.md), [ADR 0003](decisions/0003-sqlite-connection-invariants.md), [ADR 0016](decisions/0016-read-only-diagnostics.md) | +| Process, storage, diagnostics | `main.rs`, `data_dir.rs`, `storage.rs`, `doctor.rs` | [Architecture](architecture.md), [ADR 0003](decisions/0003-sqlite-connection-invariants.md), [ADR 0016](decisions/0016-read-only-diagnostics.md), [ADR 0029](decisions/0029-doctor-migration-verdict.md) | | Backups and restore | `backup.rs`, `backup_copy.rs`, `scheduler.rs`, `restore.rs`, `serve_lock.rs` | [ADR 0006](decisions/0006-backup-scheduling-and-restore.md), [ADR 0024](decisions/0024-off-server-backup-copies.md), [ADR 0027](decisions/0027-backup-copy-encryption.md), [Restore drill](restore-drill.md), `../../../scripts/restore-drill/` | | Setup, login, recovery | `setup.rs`, `users.rs`, `sessions.rs`, `secrets.rs`, `public_url.rs` | [ADR 0004](decisions/0004-local-authentication.md), [ADR 0025](decisions/0025-public-url-and-secure-cookies.md) | | Capabilities and assignments | `capabilities.rs`, `assignments.rs`, `draft_access.rs` | [ADR 0010](decisions/0010-service-owned-authorization-boundary.md), [Domain model](domain-model.md) | diff --git a/docs/operations.md b/docs/operations.md index 3d5393c..9d95022 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -62,11 +62,14 @@ value: `--backup-interval-hours 6`. Every other line should be `ok`. The `migrations` line compares each migration the database records with this version's: its number, its -checksum, and whether it is recorded as successful. A `FAIL` there names the -migration and means the service will refuse to start on this database; the -line says what to do next. A `warn` that migrations are `pending` is +checksum, and whether it is recorded as successful. A `FAIL` there means +the service will refuse to start on this database: it names the migration, +or says the migration history is missing from a database that already holds +data, and what to do next. A `warn` that migrations are `pending` is expected between installing a new version and restarting: the service -applies them on its next start, after taking a validated backup. +applies them on its next start, after taking a validated backup. A `warn` +that there is no migration history yet appears only for an empty database, +before the service's first start. An administrator also sees backup health on their home page, and gets a notice when a copy fails.