From e18a4f4c54a355ba7839b14283a6da9b625e5521 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 27 Oct 2026 01:17:40 +0000 Subject: [PATCH 1/4] fix(transactions): resumable rollback (T-4) and failed-save safety (T-5) T-4: `jk rollback` undoes a transaction's operations last-first and stops at the first error, leaving the transaction Active with the later operations already undone. The retry then stopped at the first, already undone, operation ("already undone"), so a failed rollback could never finish. cmd_rollback now skips operations already marked undone. T-5: begin, add_operation, commit and mark_rolled_back mutated the in-memory log before `save()?`, and save was a plain `fs::write`. A failed save left memory and disk disagreeing; a crash mid-write could corrupt the log. Each mutator now builds the successor log on a copy, saves it, and only then makes it the in-memory log; the save writes a sibling temporary file, fsyncs it, and renames it over the log. Tests: rollback_resume_test (real `jk`, two cases) and four unit tests in transaction.rs. Each was checked against the old behaviour: the T-4 tests fail on the old cmd_rollback; the T-5 tests fail when memory is updated before the save, when the save is a plain fs::write, and when a failed save leaves its temporary file behind. Ledger: T-4 and T-5 move OPEN -> TESTED. Crash-safety of the rename is by construction (write, fsync, rename), not crash-injected. T-8 (OPEN) records that the operation metadata log has the same plain-write save, and that an undo whose `mark_undone` fails to persist is still not retryable. Dashboards: 119 -> 125 tests. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01Kjdf4Mf4Z41bNQh2TrsEp9 --- PROOF-NEEDS.adoc | 18 +- READINESS.adoc | 4 +- TOPOLOGY.adoc | 2 +- crates/januskey-cli/src/main.rs | 10 +- .../tests/rollback_resume_test.rs | 104 ++++++++ crates/reversible-core/src/transaction.rs | 232 +++++++++++++++--- 6 files changed, 331 insertions(+), 39 deletions(-) create mode 100644 crates/januskey-cli/tests/rollback_resume_test.rs diff --git a/PROOF-NEEDS.adoc b/PROOF-NEEDS.adoc index eb16108..de9bcc5 100644 --- a/PROOF-NEEDS.adoc +++ b/PROOF-NEEDS.adoc @@ -70,10 +70,11 @@ tables must agree. | T-1 | Undoing a fully executed transaction's operations in reverse order restores the state it began in, given each step's undo restores its execute (the sequential Theorem 3.4) | PROVEN | `+TransactionAtomicity.rollback-restores+`; `+tests/reject-agda/ForwardRollback.agda+` refused | T-2 | begin, add_operation, commit and mark_rolled_back each preserve the log invariant (given a fresh id for begin) and rewrite only the Active transaction, so a completed one is never modified | PROVEN | `+TransactionAtomicity.lifecycle-preserves-good+`; `+tests/reject-agda/CommitWhenIdle.agda+` refused | T-3 | At most one transaction is Active, and exactly one when the active pointer is set | PROVEN | `+TransactionAtomicity.at-most-one-active+` -| T-4 | A rollback that fails part-way can be retried to completion | OPEN | reproduced 2026-10-26 with the real `+jk+`: after one undo error the repaired retry stops at the first, already undone, operation and the transaction stays Active (`+cmd_rollback+`, `+?+` on `+undo+`) -| T-5 | The transaction log survives an I/O failure or a crash during save without disagreeing with memory or being corrupted | OPEN | by inspection, not reproduced: begin, add_operation and commit mutate the in-memory log before `+save()?+`, and `+save+` is a plain `+fs::write+` +| T-4 | A rollback that fails part-way can be retried to completion | TESTED | `+rollback_resume_test+` drives the real `+jk+`: after one undo error and a repair, `+jk rollback+` skips the already undone operation and finishes (before the fix the retry stopped at it every time, reproduced 2026-10-26); fixed in `+cmd_rollback+` +| T-5 | The transaction log is never left disagreeing with memory by a failed save, and is replaced by write, fsync and rename rather than overwritten in place | TESTED | `+transaction::tests::failed_save_*+`: a save that fails at the final step leaves begin, add_operation, commit and rollback unapplied in memory; mutants (memory first, plain `+fs::write+`, no temp cleanup) each fail a test. Crash-safety of the rename itself is by construction, not crash-injected | T-6 | A failed execute leaves the filesystem unchanged, and each undo restores the state its execute started from (premises of T-1; single steps are TESTED by `+execute_then_undo_is_identity+`) | ASSUMED | not checked for the failure path | T-7 | Transaction ids (UUID v4) never collide within one log (the freshness premise of T-2) | ASSUMED | probabilistic, not checked +| T-8 | The operation metadata log is saved atomically, and an undo whose `+mark_undone+` fails to persist can be retried | OPEN | by inspection: `+MetadataStore::save+` is the same plain `+fs::write+`; `+undo+` runs the undo, then `+mark_undone+` persists, so a failed persist leaves the file undone but the log not |=== Removed, with the reason recorded beside the replacement in source: @@ -137,7 +138,7 @@ attestations undermine trust |Content store integrity |Content-addressed storage never returns wrong content |Hash collisions or bugs corrupt stored data -|Transaction atomicity |Delta application is atomic and reversible (the model is proved, T-1 to T-3; the partial-rollback, I/O-failure and id-freshness gaps are T-4 to T-7) +|Transaction atomicity |Delta application is atomic and reversible (the model is proved, T-1 to T-3; the Rust is tested for partial-rollback retry, T-4, and failed saves, T-5; the premises T-6 and T-7 are assumed and the metadata log, T-8, is open) |Partial transactions corrupt state |reversible-core |Undo/redo preserves data integrity |Core crate used by @@ -188,13 +189,18 @@ the status table above. |A-4: A linearly held library handle cannot be forgotten |TESTED |`tests/reject-idris/ForgottenClose.idr::ForgottenClose` |`bash scripts/check-idris-reject.sh ForgottenClose` |A-5: A status code the header does not define never decodes as Ok |TESTED |`tests/reject-idris/UnknownStatusIsOk.idr::UnknownStatusIsOk` |`bash scripts/check-idris-reject.sh UnknownStatusIsOk` |src/abi uses no proof bypass: no believe_me, assert_total, assert_smaller, partial, postulate, sorry or holes |TESTED |`tests/aspect/cross_cutting_test.sh::No holes in proofs` |`bash tests/aspect/cross_cutting_test.sh` +|T-4: After a rollback fails part-way and the cause is repaired, `jk rollback` skips the operations already undone and completes (the retry used to stop at the first, already undone, operation) |TESTED |`crates/januskey-cli/tests/rollback_resume_test.rs::rollback_retry_completes_after_a_partial_failure` |`cargo test --locked -p januskey --test rollback_resume_test rollback_retry_completes_after_a_partial_failure` +|T-4: The retry does not undo an already undone operation a second time, so later edits to its file survive |TESTED |`crates/januskey-cli/tests/rollback_resume_test.rs::rollback_does_not_undo_an_operation_twice` |`cargo test --locked -p januskey --test rollback_resume_test rollback_does_not_undo_an_operation_twice` +|T-5: A begin whose save fails is not applied in memory, and can be retried |TESTED |`crates/reversible-core/src/transaction.rs::failed_save_leaves_begin_unapplied` |`cargo test --locked -p reversible-core --lib failed_save_leaves_begin_unapplied` +|T-5: An add_operation whose save fails is not applied in memory, and the retry records it exactly once on disk |TESTED |`crates/reversible-core/src/transaction.rs::failed_save_leaves_add_operation_unapplied` |`cargo test --locked -p reversible-core --lib failed_save_leaves_add_operation_unapplied` +|T-5: A commit or rollback whose save fails leaves the transaction Active, and the retry settles it |TESTED |`crates/reversible-core/src/transaction.rs::failed_save_leaves_commit_and_rollback_unapplied` |`cargo test --locked -p reversible-core --lib failed_save_leaves_commit_and_rollback_unapplied` +|T-5: A save replaces the log by rename, leaves no temporary file, and ignores a torn temporary file from an earlier crash |TESTED |`crates/reversible-core/src/transaction.rs::save_replaces_atomically_and_leaves_no_temporary_file` |`cargo test --locked -p reversible-core --lib save_replaces_atomically_and_leaves_no_temporary_file` |G-2: The Idris2 IsCNO / ReversibleOp pair corresponds to the Agda ReversiblePair (by transcription) |ASSUMED |`verification/proofs/agda/Reversibility.agda` |- |X-1: Hash one-wayness and physical erasure (cryptographic and physical assumptions) |ASSUMED |`src/abi/JanusKey/ABI/Proofs.idr` |- |G-3: The FFI bridge preserves grade 𝟙 (a homomorphism; resource-algebra lists homomorphisms as OPEN) |OPEN |`verification/proofs/agda/Reversibility.agda` |- |E-1: An attacker cannot recover obliterated content or use a revoked key (adversaryCannotKnow holds only by construction of the model) |OPEN |`src/abi/JanusKey/ABI/Types.idr` |- |The six security obligations under "What Needs Proving" (obliteration, key derivation, attestation chain, content store, transaction atomicity, reversible-core) are proved |OPEN |`PROOF-NEEDS.adoc` |- -|T-4: A rollback that fails part-way can be retried to completion (reproduced 2026-10-26: after one undo error the repaired `jk rollback` stops at the first, already undone, operation and the transaction stays Active) |OPEN |`crates/januskey-cli/src/main.rs` |- -|T-5: The transaction log survives an I/O failure or a crash during save without disagreeing with memory or being corrupted (by inspection: the log is mutated before `save()?`, and `save` is a plain `fs::write`) |OPEN |`crates/reversible-core/src/transaction.rs` |- +|T-8: The operation metadata log is saved atomically, and an undo whose `+mark_undone+` fails to persist can be retried (by inspection: `+MetadataStore::save+` is a plain `+fs::write+`) |OPEN |`crates/reversible-core/src/metadata.rs` |- |T-6: A failed execute leaves the filesystem unchanged, and each undo restores the state its execute started from (premises of T-1) |ASSUMED |`crates/januskey-cli/src/operations.rs` |- |T-7: Transaction ids (UUID v4) never collide within one log (the freshness premise of T-2) |ASSUMED |`crates/reversible-core/src/transaction.rs` |- |Every operation is reversible (MPR), as a theorem linked to the Rust (needs G-2 made mechanical, and G-3) |OPEN |`verification/proofs/agda/Reversibility.agda` |- @@ -207,7 +213,7 @@ ABI/runtime layer (ruling 2026-10-12): kernel statements live in `+verification/proofs/agda/+` with siblings consumed by pin (`+scripts/fetch-siblings.sh+`); the ABI lives in `+src/abi/+`. Transaction atomicity is stated there for the model -(`+verification/proofs/agda/TransactionAtomicity.agda+`, rows T-1 to T-3); +(`+verification/proofs/agda/TransactionAtomicity.agda+`, rows T-1 to T-3; the Rust fixes behind T-4 and T-5 are tested, not proved); obliteration completeness and attestation chain integrity are still to be stated. The 225 `+unwrap()+` calls should be systematically replaced with proven error handling. diff --git a/READINESS.adoc b/READINESS.adoc index 505a2db..c1d6b4b 100644 --- a/READINESS.adoc +++ b/READINESS.adoc @@ -8,7 +8,7 @@ standards/testing-and-benchmarking/TESTING-TAXONOMY.adoc v1.0 === CRG Grade: D (Alpha — Unstable) -*Justification:* 119 tests declared (`cargo test --workspace --locked -- --list`; rust-ci runs them); the +*Justification:* 125 tests declared (`cargo test --workspace --locked -- --list`; rust-ci runs them); the Idris2 ABI typechecks and 11 proofs are checked (PROVEN ledger rows: ABI, grade and transaction-model lemmas, not the security obligations); benchmarks exist (5 Criterion groups). But: no fuzz testing, no mutation testing, 225 unwrap() calls, E2E mostly skips, @@ -65,7 +65,7 @@ handling (run by idris-abi.yml, agda-proofs.yml and dashboard-check) |=== -*Total declared:* 119 tests + 5 benchmark groups; 11 proofs checked (PROVEN ledger rows) *Total +*Total declared:* 125 tests + 5 benchmark groups; 11 proofs checked (PROVEN ledger rows) *Total missing:* Fuzz, mutation, chaos, compatibility === Aspect Matrix diff --git a/TOPOLOGY.adoc b/TOPOLOGY.adoc index 469db4b..cff8df4 100644 --- a/TOPOLOGY.adoc +++ b/TOPOLOGY.adoc @@ -79,7 +79,7 @@ INTERFACES & RESEARCH MPR Methodology ████░░░░░░ ~40% Design documented; FORMAL PROOFS PENDING (Idris2 ABI typechecks; 11 proofs checked: ABI, grade and transaction-model lemmas, not the security obligations; MPR not linked to the Rust) - Testing (READINESS matrix) ██████░░░░ ~60% 119 tests + 5 benches; missing fuzz, mutation, + Testing (READINESS matrix) ██████░░░░ ~60% 125 tests + 5 benches; missing fuzz, mutation, chaos, compatibility (Grade D) REPO INFRASTRUCTURE diff --git a/crates/januskey-cli/src/main.rs b/crates/januskey-cli/src/main.rs index 182bdc6..35fe1d5 100644 --- a/crates/januskey-cli/src/main.rs +++ b/crates/januskey-cli/src/main.rs @@ -810,8 +810,16 @@ fn cmd_rollback(dir: &Path) -> Result<()> { .ok_or_else(|| anyhow::anyhow!("No active transaction"))? .clone(); - // Undo operations in reverse order (Theorem 3.4: Sequential Reversibility) + // Undo operations in reverse order (Theorem 3.4: Sequential Reversibility). + // + // Resumable: an undo that fails leaves the transaction Active with the later + // operations already undone, so a retry must skip those and carry on from the + // operation that failed (PROOF-NEEDS T-4). Without this the retry stops at + // the first, already undone, operation and the rollback can never finish. for op_id in active_tx.operation_ids.iter().rev() { + if jk.metadata_store.get(op_id).is_some_and(|op| op.undone) { + continue; + } let mut executor = OperationExecutor::new(&jk.content_store, &mut jk.metadata_store); executor.undo(op_id)?; } diff --git a/crates/januskey-cli/tests/rollback_resume_test.rs b/crates/januskey-cli/tests/rollback_resume_test.rs new file mode 100644 index 0000000..4ee289c --- /dev/null +++ b/crates/januskey-cli/tests/rollback_resume_test.rs @@ -0,0 +1,104 @@ +// SPDX-License-Identifier: MPL-2.0 +// Copyright (c) Jonathan D.A. Jewell +// SPDX-FileCopyrightText: 2026 Jonathan D.A. Jewell +// +// PROOF-NEEDS T-4: a rollback that fails part-way can be retried to completion. +// +// `jk rollback` undoes a transaction's operations last-first. When one undo +// fails it stops with the transaction still Active and the later operations +// already undone. The retry must skip those and finish; before the fix it +// stopped at the first, already undone, operation, every time. +// +// Driven through the real `jk` binary, because the fault is in `cmd_rollback`. + +use assert_cmd::Command; +use predicates::prelude::*; +use std::fs; +use std::path::Path; +use tempfile::TempDir; + +fn jk(dir: &Path) -> Command { + let mut cmd = Command::cargo_bin("jk").expect("jk binary"); + cmd.arg("--yes").arg("--dir").arg(dir); + cmd +} + +/// A repo with an Active transaction holding two operations, oldest first: +/// 1. `copy a.txt b.txt` (undone by deleting b.txt) +/// 2. `modify c.txt` (undone by restoring "one") +/// +/// Rollback undoes 2 first, then 1. +fn repo_with_two_operations() -> TempDir { + let tmp = TempDir::new().expect("temp dir"); + let dir = tmp.path(); + jk(dir).arg("init").assert().success(); + fs::write(dir.join("a.txt"), "alpha").expect("a.txt"); + fs::write(dir.join("c.txt"), "one").expect("c.txt"); + + jk(dir).args(["begin", "t"]).assert().success(); + jk(dir).args(["copy", "a.txt", "b.txt"]).assert().success(); + jk(dir) + .args(["modify", "s/one/two/g", "c.txt"]) + .assert() + .success(); + assert_eq!(fs::read_to_string(dir.join("c.txt")).unwrap(), "two"); + assert!(dir.join("b.txt").exists()); + tmp +} + +#[test] +fn rollback_retry_completes_after_a_partial_failure() { + let tmp = repo_with_two_operations(); + let dir = tmp.path(); + + // Break the second undo: b.txt is gone, so deleting it to undo the copy fails. + fs::remove_file(dir.join("b.txt")).expect("remove b.txt"); + jk(dir).arg("rollback").assert().failure(); + + // The undo of the modify (run first) took effect; the transaction is still Active. + assert_eq!(fs::read_to_string(dir.join("c.txt")).unwrap(), "one"); + jk(dir) + .arg("status") + .assert() + .success() + .stdout(predicate::str::contains("Active")); + + // Repair the problem and retry: it must finish, not stop at the modify. + fs::write(dir.join("b.txt"), "alpha").expect("restore b.txt"); + jk(dir) + .arg("rollback") + .assert() + .success() + .stdout(predicate::str::contains("Rolled back transaction")); + + assert!(!dir.join("b.txt").exists(), "the copy is undone"); + assert_eq!(fs::read_to_string(dir.join("c.txt")).unwrap(), "one"); + assert_eq!(fs::read_to_string(dir.join("a.txt")).unwrap(), "alpha"); + + // The transaction is settled: nothing left to roll back. + jk(dir) + .arg("rollback") + .assert() + .failure() + .stderr(predicate::str::contains("No active transaction")); +} + +#[test] +fn rollback_does_not_undo_an_operation_twice() { + let tmp = repo_with_two_operations(); + let dir = tmp.path(); + + fs::remove_file(dir.join("b.txt")).expect("remove b.txt"); + jk(dir).arg("rollback").assert().failure(); + + // The user edits c.txt after the failed attempt. The retry must leave it + // alone: its operation was already undone, and re-undoing would clobber it. + fs::write(dir.join("c.txt"), "edited by hand").expect("hand edit"); + fs::write(dir.join("b.txt"), "alpha").expect("restore b.txt"); + jk(dir).arg("rollback").assert().success(); + + assert_eq!( + fs::read_to_string(dir.join("c.txt")).unwrap(), + "edited by hand" + ); +} diff --git a/crates/reversible-core/src/transaction.rs b/crates/reversible-core/src/transaction.rs index 8f47601..3864c75 100644 --- a/crates/reversible-core/src/transaction.rs +++ b/crates/reversible-core/src/transaction.rs @@ -13,7 +13,8 @@ use crate::error::{Result, ReversibleError}; use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; use std::fs; -use std::path::PathBuf; +use std::io::Write; +use std::path::{Path, PathBuf}; use uuid::Uuid; /// Transaction state @@ -105,6 +106,36 @@ impl TransactionLog { } } +/// Replace `path` with `content` so a reader sees the old file or the new one, +/// never a torn write: write a sibling temporary file, flush it to disk, then +/// rename it over `path`. +fn write_atomic(path: &Path, content: &[u8]) -> std::io::Result<()> { + let mut tmp = path.as_os_str().to_owned(); + tmp.push(".tmp"); + let tmp = PathBuf::from(tmp); + + let written = (|| { + let mut file = fs::File::create(&tmp)?; + file.write_all(content)?; + file.sync_all()?; + fs::rename(&tmp, path) + })(); + if written.is_err() { + // Best effort: a stale temporary file is harmless (the next save + // truncates it) but there is no reason to leave it behind. + let _ = fs::remove_file(&tmp); + } + written?; + + // Make the rename itself durable. Not every platform can open a directory. + if let Some(parent) = path.parent() { + if let Ok(dir) = fs::File::open(parent) { + let _ = dir.sync_all(); + } + } + Ok(()) +} + /// Manager for transaction lifecycle (data + persistence only). /// /// Filesystem-level rollback (undoing operations) is handled by @@ -138,16 +169,47 @@ impl TransactionManager { Ok(Self { path, log }) } - /// Save the log to disk - fn save(&self) -> Result<()> { + /// Persist `next` and, only once it is on disk, make it the in-memory log. + /// + /// Every mutator builds its successor log on a copy and ends here, so a + /// failed save leaves memory and disk both at the previous log (T-5). + fn persist(&mut self, next: TransactionLog) -> Result<()> { if let Some(parent) = self.path.parent() { fs::create_dir_all(parent)?; } - let content = serde_json::to_string_pretty(&self.log)?; - fs::write(&self.path, content)?; + let content = serde_json::to_string_pretty(&next)?; + write_atomic(&self.path, content.as_bytes())?; + self.log = next; Ok(()) } + /// Apply `f` to the active transaction of a copy of the log, optionally + /// clear the active pointer, persist, and return the updated transaction. + fn update_active( + &mut self, + settle: bool, + f: impl FnOnce(&mut Transaction), + ) -> Result { + let active_id = self + .log + .active_transaction_id + .clone() + .ok_or(ReversibleError::NoActiveTransaction)?; + let mut next = self.log.clone(); + let transaction = next + .transactions + .iter_mut() + .find(|t| t.id == active_id) + .ok_or(ReversibleError::NoActiveTransaction)?; + f(transaction); + let result = transaction.clone(); + if settle { + next.active_transaction_id = None; + } + self.persist(next)?; + Ok(result) + } + /// Begin a new transaction pub fn begin(&mut self, name: Option) -> Result<&Transaction> { if let Some(ref active_id) = self.log.active_transaction_id { @@ -156,9 +218,10 @@ impl TransactionManager { let transaction = Transaction::new(name); let id = transaction.id.clone(); - self.log.transactions.push(transaction); - self.log.active_transaction_id = Some(id); - self.save()?; + let mut next = self.log.clone(); + next.transactions.push(transaction); + next.active_transaction_id = Some(id); + self.persist(next)?; // SAFETY: We just pushed a transaction above, so last() is guaranteed Some Ok(self @@ -184,23 +247,13 @@ impl TransactionManager { /// Add operation to active transaction pub fn add_operation(&mut self, operation_id: String) -> Result<()> { - let transaction = self - .active_mut() - .ok_or(ReversibleError::NoActiveTransaction)?; - transaction.add_operation(operation_id); - self.save() + self.update_active(false, |t| t.add_operation(operation_id)) + .map(|_| ()) } /// Commit the active transaction (marks state only — no filesystem effects) pub fn commit(&mut self) -> Result { - let transaction = self - .active_mut() - .ok_or(ReversibleError::NoActiveTransaction)?; - transaction.commit(); - let result = transaction.clone(); - self.log.active_transaction_id = None; - self.save()?; - Ok(result) + self.update_active(true, Transaction::commit) } /// Mark the active transaction as rolled back (state only). @@ -208,14 +261,7 @@ impl TransactionManager { /// The caller is responsible for actually undoing the operations /// via the appropriate executor before calling this. pub fn mark_rolled_back(&mut self) -> Result { - let transaction = self - .active_mut() - .ok_or(ReversibleError::NoActiveTransaction)?; - transaction.rollback(); - let result = transaction.clone(); - self.log.active_transaction_id = None; - self.save()?; - Ok(result) + self.update_active(true, Transaction::rollback) } /// Get transaction by ID @@ -320,6 +366,134 @@ mod tests { assert!(!manager.has_active()); } + /// Make the save fail at its last step, on every platform and for root too: + /// plant a non-empty directory where the log file goes, so the temporary + /// file is written but the rename over it is refused. + fn break_log_file(path: &std::path::Path) { + let _ = fs::remove_file(path); + fs::create_dir_all(path).expect("failed to plant a directory over the log"); + fs::write(path.join("blocker"), b"x").expect("failed to fill the planted directory"); + } + + fn repair_log_file(path: &std::path::Path) { + assert!( + !path.with_extension("json.tmp").exists(), + "a failed save must not leave its temporary file behind" + ); + fs::remove_dir_all(path).expect("failed to remove the planted directory"); + } + + #[test] + fn failed_save_leaves_begin_unapplied() { + let tmp = TempDir::new().expect("failed to create temp dir"); + let dir = tmp.path().join("state"); + let mut manager = TransactionManager::new(dir.join("transactions.json")) + .expect("failed to create transaction manager"); + + break_log_file(&dir.join("transactions.json")); + assert!(manager.begin(Some("t".to_string())).is_err()); + assert!(!manager.has_active(), "a failed begin must not activate"); + assert!(manager.all().is_empty(), "a failed begin must not record"); + + repair_log_file(&dir.join("transactions.json")); + manager.begin(Some("t".to_string())).expect("retry begin"); + assert_eq!(manager.all().len(), 1); + } + + #[test] + fn failed_save_leaves_add_operation_unapplied() { + let tmp = TempDir::new().expect("failed to create temp dir"); + let dir = tmp.path().join("state"); + let path = dir.join("transactions.json"); + let mut manager = TransactionManager::new(path.clone()).expect("new manager"); + manager.begin(None).expect("begin"); + manager.add_operation("op-1".to_string()).expect("op-1"); + + break_log_file(&dir.join("transactions.json")); + assert!(manager.add_operation("op-2".to_string()).is_err()); + let ops = &manager.active().expect("still active").operation_ids; + assert_eq!( + ops, + &["op-1".to_string()], + "a failed save must not record op-2" + ); + + repair_log_file(&dir.join("transactions.json")); + manager + .add_operation("op-2".to_string()) + .expect("retry op-2"); + let reloaded = TransactionManager::new(path).expect("reload"); + assert_eq!( + reloaded.active().expect("active on disk").operation_ids, + ["op-1".to_string(), "op-2".to_string()], + "memory and disk agree, op-2 recorded exactly once" + ); + } + + #[test] + fn failed_save_leaves_commit_and_rollback_unapplied() { + for commit in [true, false] { + let tmp = TempDir::new().expect("failed to create temp dir"); + let dir = tmp.path().join("state"); + let mut manager = + TransactionManager::new(dir.join("transactions.json")).expect("new manager"); + manager.begin(None).expect("begin"); + manager.add_operation("op-1".to_string()).expect("op-1"); + + break_log_file(&dir.join("transactions.json")); + let settled = if commit { + manager.commit() + } else { + manager.mark_rolled_back() + }; + assert!(settled.is_err()); + assert!(manager.has_active(), "a failed save must leave it Active"); + assert_eq!( + manager.active().expect("active").state, + TransactionState::Active + ); + + repair_log_file(&dir.join("transactions.json")); + let tx = if commit { + manager.commit() + } else { + manager.mark_rolled_back() + } + .expect("retry settles"); + assert_eq!( + tx.state, + if commit { + TransactionState::Committed + } else { + TransactionState::RolledBack + } + ); + assert!(!manager.has_active()); + } + } + + #[test] + fn save_replaces_atomically_and_leaves_no_temporary_file() { + let tmp = TempDir::new().expect("failed to create temp dir"); + let path = tmp.path().join("transactions.json"); + // A torn temporary file left by an earlier crash must not matter. + fs::write(tmp.path().join("transactions.json.tmp"), b"{ torn").expect("plant stale tmp"); + + let mut manager = TransactionManager::new(path.clone()).expect("new manager"); + manager.begin(None).expect("begin"); + manager.add_operation("op-1".to_string()).expect("op-1"); + + assert!( + !tmp.path().join("transactions.json.tmp").exists(), + "the temporary file is renamed away" + ); + let reloaded = TransactionManager::new(path).expect("log parses after saves"); + assert_eq!( + reloaded.active().expect("active").operation_ids, + ["op-1".to_string()] + ); + } + #[test] fn test_cannot_begin_while_active() { let tmp = TempDir::new().expect("failed to create temp dir"); From bd4ae481e0b7d0e2e7b00e6d983ebfe8895bb3f3 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 27 Oct 2026 01:23:11 +0000 Subject: [PATCH 2/4] fix(transactions): sync the directory of a bare-filename log after the rename `Path::new("transactions.json").parent()` is `Some("")`, which cannot be opened, so the post-rename directory fsync was silently skipped for a log given as a bare filename. A bare filename now syncs the current directory. Adds a unit test for the helper, a doc comment on each new test, and moves the dashboard test count 125 -> 126. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01Kjdf4Mf4Z41bNQh2TrsEp9 --- READINESS.adoc | 4 +- TOPOLOGY.adoc | 2 +- .../tests/rollback_resume_test.rs | 4 ++ crates/reversible-core/src/transaction.rs | 40 +++++++++++++++++-- 4 files changed, 43 insertions(+), 7 deletions(-) diff --git a/READINESS.adoc b/READINESS.adoc index c1d6b4b..8c514ee 100644 --- a/READINESS.adoc +++ b/READINESS.adoc @@ -8,7 +8,7 @@ standards/testing-and-benchmarking/TESTING-TAXONOMY.adoc v1.0 === CRG Grade: D (Alpha — Unstable) -*Justification:* 125 tests declared (`cargo test --workspace --locked -- --list`; rust-ci runs them); the +*Justification:* 126 tests declared (`cargo test --workspace --locked -- --list`; rust-ci runs them); the Idris2 ABI typechecks and 11 proofs are checked (PROVEN ledger rows: ABI, grade and transaction-model lemmas, not the security obligations); benchmarks exist (5 Criterion groups). But: no fuzz testing, no mutation testing, 225 unwrap() calls, E2E mostly skips, @@ -65,7 +65,7 @@ handling (run by idris-abi.yml, agda-proofs.yml and dashboard-check) |=== -*Total declared:* 125 tests + 5 benchmark groups; 11 proofs checked (PROVEN ledger rows) *Total +*Total declared:* 126 tests + 5 benchmark groups; 11 proofs checked (PROVEN ledger rows) *Total missing:* Fuzz, mutation, chaos, compatibility === Aspect Matrix diff --git a/TOPOLOGY.adoc b/TOPOLOGY.adoc index cff8df4..a16847c 100644 --- a/TOPOLOGY.adoc +++ b/TOPOLOGY.adoc @@ -79,7 +79,7 @@ INTERFACES & RESEARCH MPR Methodology ████░░░░░░ ~40% Design documented; FORMAL PROOFS PENDING (Idris2 ABI typechecks; 11 proofs checked: ABI, grade and transaction-model lemmas, not the security obligations; MPR not linked to the Rust) - Testing (READINESS matrix) ██████░░░░ ~60% 125 tests + 5 benches; missing fuzz, mutation, + Testing (READINESS matrix) ██████░░░░ ~60% 126 tests + 5 benches; missing fuzz, mutation, chaos, compatibility (Grade D) REPO INFRASTRUCTURE diff --git a/crates/januskey-cli/tests/rollback_resume_test.rs b/crates/januskey-cli/tests/rollback_resume_test.rs index 4ee289c..f2bca9e 100644 --- a/crates/januskey-cli/tests/rollback_resume_test.rs +++ b/crates/januskey-cli/tests/rollback_resume_test.rs @@ -46,6 +46,8 @@ fn repo_with_two_operations() -> TempDir { tmp } +/// After one undo fails and the cause is repaired, `jk rollback` finishes the +/// operations that are left instead of stopping at the one already undone. #[test] fn rollback_retry_completes_after_a_partial_failure() { let tmp = repo_with_two_operations(); @@ -83,6 +85,8 @@ fn rollback_retry_completes_after_a_partial_failure() { .stderr(predicate::str::contains("No active transaction")); } +/// The retry leaves an already undone operation alone, so later edits to its +/// file are not overwritten. #[test] fn rollback_does_not_undo_an_operation_twice() { let tmp = repo_with_two_operations(); diff --git a/crates/reversible-core/src/transaction.rs b/crates/reversible-core/src/transaction.rs index 3864c75..9c2429a 100644 --- a/crates/reversible-core/src/transaction.rs +++ b/crates/reversible-core/src/transaction.rs @@ -128,14 +128,21 @@ fn write_atomic(path: &Path, content: &[u8]) -> std::io::Result<()> { written?; // Make the rename itself durable. Not every platform can open a directory. - if let Some(parent) = path.parent() { - if let Ok(dir) = fs::File::open(parent) { - let _ = dir.sync_all(); - } + if let Ok(dir) = fs::File::open(containing_dir(path)) { + let _ = dir.sync_all(); } Ok(()) } +/// The directory that holds `path`. A bare filename has the empty path as its +/// parent, which cannot be opened, so it is the current directory. +fn containing_dir(path: &Path) -> &Path { + match path.parent() { + Some(parent) if !parent.as_os_str().is_empty() => parent, + _ => Path::new("."), + } +} + /// Manager for transaction lifecycle (data + persistence only). /// /// Filesystem-level rollback (undoing operations) is handled by @@ -383,6 +390,25 @@ mod tests { fs::remove_dir_all(path).expect("failed to remove the planted directory"); } + /// A bare filename's directory is `.`, not the unopenable empty path. + #[test] + fn containing_dir_of_a_bare_filename_is_the_current_directory() { + assert_eq!( + containing_dir(Path::new("transactions.json")), + Path::new(".") + ); + assert_eq!( + containing_dir(Path::new("a/transactions.json")), + Path::new("a") + ); + assert_eq!( + containing_dir(Path::new("/transactions.json")), + Path::new("/") + ); + assert!(fs::File::open(containing_dir(Path::new("transactions.json"))).is_ok()); + } + + /// T-5: a begin whose save fails changes nothing in memory and can be retried. #[test] fn failed_save_leaves_begin_unapplied() { let tmp = TempDir::new().expect("failed to create temp dir"); @@ -400,6 +426,8 @@ mod tests { assert_eq!(manager.all().len(), 1); } + /// T-5: an add_operation whose save fails is not recorded in memory, and the + /// retry records it exactly once, in memory and on disk. #[test] fn failed_save_leaves_add_operation_unapplied() { let tmp = TempDir::new().expect("failed to create temp dir"); @@ -430,6 +458,8 @@ mod tests { ); } + /// T-5: a commit or rollback whose save fails leaves the transaction Active, + /// and the retry settles it. #[test] fn failed_save_leaves_commit_and_rollback_unapplied() { for commit in [true, false] { @@ -472,6 +502,8 @@ mod tests { } } + /// T-5: a save replaces the log by rename, leaves no temporary file, and is + /// not confused by a torn temporary file left by an earlier crash. #[test] fn save_replaces_atomically_and_leaves_no_temporary_file() { let tmp = TempDir::new().expect("failed to create temp dir"); From a56cbde683533f5c3ea76c5b8304088578ae8ac1 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 27 Oct 2026 01:39:32 +0000 Subject: [PATCH 3/4] fix(transactions): per-save temporary file, and keep the log's permissions Review findings on the atomic save: - Every save used the one `.tmp`, so two writers could truncate, rename or delete each other's file, and a rename could carry the other writer's bytes. Each save now writes `...tmp`, opened with create_new so it can never adopt an existing file. - Replacing the log by rename gave it the umask default, widening a log the user had restricted (the old in-place write kept the inode's mode). The new file now takes the replaced log's permissions before the rename. The directory fsync stays best effort and is now documented as such: by then the rename has happened, so failing the save would leave disk on the new log and memory on the old, the disagreement T-5 exists to prevent. Tests: each_save_uses_its_own_temporary_file and save_preserves_the_logs_permissions, each checked against a mutant (shared name, truncating open, no permission copy). Ledger T-5 gains two TESTED rows and states what is not claimed. Dashboards: 126 -> 128 tests. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01Kjdf4Mf4Z41bNQh2TrsEp9 --- PROOF-NEEDS.adoc | 4 +- READINESS.adoc | 4 +- TOPOLOGY.adoc | 2 +- crates/reversible-core/src/transaction.rs | 129 +++++++++++++++++++--- 4 files changed, 119 insertions(+), 20 deletions(-) diff --git a/PROOF-NEEDS.adoc b/PROOF-NEEDS.adoc index de9bcc5..36fb7c5 100644 --- a/PROOF-NEEDS.adoc +++ b/PROOF-NEEDS.adoc @@ -71,7 +71,7 @@ tables must agree. | T-2 | begin, add_operation, commit and mark_rolled_back each preserve the log invariant (given a fresh id for begin) and rewrite only the Active transaction, so a completed one is never modified | PROVEN | `+TransactionAtomicity.lifecycle-preserves-good+`; `+tests/reject-agda/CommitWhenIdle.agda+` refused | T-3 | At most one transaction is Active, and exactly one when the active pointer is set | PROVEN | `+TransactionAtomicity.at-most-one-active+` | T-4 | A rollback that fails part-way can be retried to completion | TESTED | `+rollback_resume_test+` drives the real `+jk+`: after one undo error and a repair, `+jk rollback+` skips the already undone operation and finishes (before the fix the retry stopped at it every time, reproduced 2026-10-26); fixed in `+cmd_rollback+` -| T-5 | The transaction log is never left disagreeing with memory by a failed save, and is replaced by write, fsync and rename rather than overwritten in place | TESTED | `+transaction::tests::failed_save_*+`: a save that fails at the final step leaves begin, add_operation, commit and rollback unapplied in memory; mutants (memory first, plain `+fs::write+`, no temp cleanup) each fail a test. Crash-safety of the rename itself is by construction, not crash-injected +| T-5 | The transaction log is never left disagreeing with memory by a failed save, and is replaced by write, fsync and rename (a uniquely named temporary file, the old file's permissions kept) rather than overwritten in place | TESTED | `+transaction::tests::failed_save_*+`: a save that fails at the final step leaves begin, add_operation, commit and rollback unapplied in memory; mutants (memory first, plain `+fs::write+`, no temp cleanup, shared temp name, truncating open, no permission copy) each fail a test. Not claimed: crash-safety of the rename itself is by construction, not crash-injected; the directory fsync is best effort, because failing the save after the rename would leave disk new and memory old; concurrent writers are out of scope (each save's content is intact, the last rename wins) | T-6 | A failed execute leaves the filesystem unchanged, and each undo restores the state its execute started from (premises of T-1; single steps are TESTED by `+execute_then_undo_is_identity+`) | ASSUMED | not checked for the failure path | T-7 | Transaction ids (UUID v4) never collide within one log (the freshness premise of T-2) | ASSUMED | probabilistic, not checked | T-8 | The operation metadata log is saved atomically, and an undo whose `+mark_undone+` fails to persist can be retried | OPEN | by inspection: `+MetadataStore::save+` is the same plain `+fs::write+`; `+undo+` runs the undo, then `+mark_undone+` persists, so a failed persist leaves the file undone but the log not @@ -195,6 +195,8 @@ the status table above. |T-5: An add_operation whose save fails is not applied in memory, and the retry records it exactly once on disk |TESTED |`crates/reversible-core/src/transaction.rs::failed_save_leaves_add_operation_unapplied` |`cargo test --locked -p reversible-core --lib failed_save_leaves_add_operation_unapplied` |T-5: A commit or rollback whose save fails leaves the transaction Active, and the retry settles it |TESTED |`crates/reversible-core/src/transaction.rs::failed_save_leaves_commit_and_rollback_unapplied` |`cargo test --locked -p reversible-core --lib failed_save_leaves_commit_and_rollback_unapplied` |T-5: A save replaces the log by rename, leaves no temporary file, and ignores a torn temporary file from an earlier crash |TESTED |`crates/reversible-core/src/transaction.rs::save_replaces_atomically_and_leaves_no_temporary_file` |`cargo test --locked -p reversible-core --lib save_replaces_atomically_and_leaves_no_temporary_file` +|T-5: Two saves never share a temporary file, and the open refuses one that exists, so a writer cannot truncate, rename or delete another's |TESTED |`crates/reversible-core/src/transaction.rs::each_save_uses_its_own_temporary_file` |`cargo test --locked -p reversible-core --lib each_save_uses_its_own_temporary_file` +|T-5: A save keeps the permissions of the log it replaces (rename would otherwise widen a 0640 log to the umask default) |TESTED |`crates/reversible-core/src/transaction.rs::save_preserves_the_logs_permissions` |`cargo test --locked -p reversible-core --lib save_preserves_the_logs_permissions` |G-2: The Idris2 IsCNO / ReversibleOp pair corresponds to the Agda ReversiblePair (by transcription) |ASSUMED |`verification/proofs/agda/Reversibility.agda` |- |X-1: Hash one-wayness and physical erasure (cryptographic and physical assumptions) |ASSUMED |`src/abi/JanusKey/ABI/Proofs.idr` |- |G-3: The FFI bridge preserves grade 𝟙 (a homomorphism; resource-algebra lists homomorphisms as OPEN) |OPEN |`verification/proofs/agda/Reversibility.agda` |- diff --git a/READINESS.adoc b/READINESS.adoc index 8c514ee..80f4d09 100644 --- a/READINESS.adoc +++ b/READINESS.adoc @@ -8,7 +8,7 @@ standards/testing-and-benchmarking/TESTING-TAXONOMY.adoc v1.0 === CRG Grade: D (Alpha — Unstable) -*Justification:* 126 tests declared (`cargo test --workspace --locked -- --list`; rust-ci runs them); the +*Justification:* 128 tests declared (`cargo test --workspace --locked -- --list`; rust-ci runs them); the Idris2 ABI typechecks and 11 proofs are checked (PROVEN ledger rows: ABI, grade and transaction-model lemmas, not the security obligations); benchmarks exist (5 Criterion groups). But: no fuzz testing, no mutation testing, 225 unwrap() calls, E2E mostly skips, @@ -65,7 +65,7 @@ handling (run by idris-abi.yml, agda-proofs.yml and dashboard-check) |=== -*Total declared:* 126 tests + 5 benchmark groups; 11 proofs checked (PROVEN ledger rows) *Total +*Total declared:* 128 tests + 5 benchmark groups; 11 proofs checked (PROVEN ledger rows) *Total missing:* Fuzz, mutation, chaos, compatibility === Aspect Matrix diff --git a/TOPOLOGY.adoc b/TOPOLOGY.adoc index a16847c..410d392 100644 --- a/TOPOLOGY.adoc +++ b/TOPOLOGY.adoc @@ -79,7 +79,7 @@ INTERFACES & RESEARCH MPR Methodology ████░░░░░░ ~40% Design documented; FORMAL PROOFS PENDING (Idris2 ABI typechecks; 11 proofs checked: ABI, grade and transaction-model lemmas, not the security obligations; MPR not linked to the Rust) - Testing (READINESS matrix) ██████░░░░ ~60% 126 tests + 5 benches; missing fuzz, mutation, + Testing (READINESS matrix) ██████░░░░ ~60% 128 tests + 5 benches; missing fuzz, mutation, chaos, compatibility (Grade D) REPO INFRASTRUCTURE diff --git a/crates/reversible-core/src/transaction.rs b/crates/reversible-core/src/transaction.rs index 9c2429a..7d1e869 100644 --- a/crates/reversible-core/src/transaction.rs +++ b/crates/reversible-core/src/transaction.rs @@ -15,6 +15,7 @@ use serde::{Deserialize, Serialize}; use std::fs; use std::io::Write; use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU64, Ordering}; use uuid::Uuid; /// Transaction state @@ -106,28 +107,57 @@ impl TransactionLog { } } +/// Distinguishes the temporary files of saves made by one process. +static SAVE_SEQ: AtomicU64 = AtomicU64::new(0); + +/// A sibling of `path` that no other save, in this or another process, uses. +fn unique_tmp_path(path: &Path) -> PathBuf { + let mut tmp = path.as_os_str().to_owned(); + tmp.push(format!( + ".{}.{}.tmp", + std::process::id(), + SAVE_SEQ.fetch_add(1, Ordering::Relaxed) + )); + PathBuf::from(tmp) +} + +/// Create the temporary file, failing if it exists: never truncate or adopt a +/// file another writer may be using. +fn create_tmp(tmp: &Path) -> std::io::Result { + fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(tmp) +} + /// Replace `path` with `content` so a reader sees the old file or the new one, -/// never a torn write: write a sibling temporary file, flush it to disk, then -/// rename it over `path`. +/// never a torn write: write a uniquely named sibling temporary file, flush it +/// to disk, then rename it over `path`. The file keeps the permissions of the +/// one it replaces. fn write_atomic(path: &Path, content: &[u8]) -> std::io::Result<()> { - let mut tmp = path.as_os_str().to_owned(); - tmp.push(".tmp"); - let tmp = PathBuf::from(tmp); + let tmp = unique_tmp_path(path); let written = (|| { - let mut file = fs::File::create(&tmp)?; + let mut file = create_tmp(&tmp)?; file.write_all(content)?; + if let Ok(existing) = fs::metadata(path) { + file.set_permissions(existing.permissions())?; + } file.sync_all()?; fs::rename(&tmp, path) })(); if written.is_err() { - // Best effort: a stale temporary file is harmless (the next save - // truncates it) but there is no reason to leave it behind. + // Best effort: nothing reads a stray temporary file, but there is no + // reason to leave one behind. let _ = fs::remove_file(&tmp); } written?; - // Make the rename itself durable. Not every platform can open a directory. + // Make the rename itself durable. This is best effort, deliberately: the + // rename has already happened, so failing the save here would leave the + // disk on the new log and memory on the old one, the disagreement this + // function exists to prevent. Not every platform can open a directory, and + // some filesystems refuse to sync one. if let Ok(dir) = fs::File::open(containing_dir(path)) { let _ = dir.sync_all(); } @@ -382,9 +412,25 @@ mod tests { fs::write(path.join("blocker"), b"x").expect("failed to fill the planted directory"); } + /// The temporary files in `dir`: anything a save could have left behind. + fn temporary_files(dir: &std::path::Path) -> Vec { + fs::read_dir(dir) + .expect("failed to list the log directory") + .map(|entry| { + entry + .expect("dir entry") + .file_name() + .to_string_lossy() + .into_owned() + }) + .filter(|name| name.ends_with(".tmp")) + .collect() + } + fn repair_log_file(path: &std::path::Path) { - assert!( - !path.with_extension("json.tmp").exists(), + assert_eq!( + temporary_files(path.parent().expect("log has a parent")), + Vec::::new(), "a failed save must not leave its temporary file behind" ); fs::remove_dir_all(path).expect("failed to remove the planted directory"); @@ -508,16 +554,23 @@ mod tests { fn save_replaces_atomically_and_leaves_no_temporary_file() { let tmp = TempDir::new().expect("failed to create temp dir"); let path = tmp.path().join("transactions.json"); - // A torn temporary file left by an earlier crash must not matter. - fs::write(tmp.path().join("transactions.json.tmp"), b"{ torn").expect("plant stale tmp"); + // A torn temporary file left by an earlier crash must not matter, and + // must not be touched: it may belong to a save still in flight. + let stale = "transactions.json.99999.0.tmp"; + fs::write(tmp.path().join(stale), b"{ torn").expect("plant stale tmp"); let mut manager = TransactionManager::new(path.clone()).expect("new manager"); manager.begin(None).expect("begin"); manager.add_operation("op-1".to_string()).expect("op-1"); - assert!( - !tmp.path().join("transactions.json.tmp").exists(), - "the temporary file is renamed away" + assert_eq!( + temporary_files(tmp.path()), + [stale.to_string()], + "every save's own temporary file is renamed away" + ); + assert_eq!( + fs::read(tmp.path().join(stale)).expect("stale file"), + b"{ torn" ); let reloaded = TransactionManager::new(path).expect("log parses after saves"); assert_eq!( @@ -526,6 +579,50 @@ mod tests { ); } + /// T-5: two saves never share a temporary file, so one writer cannot + /// truncate, rename or delete the file another is still writing. + #[test] + fn each_save_uses_its_own_temporary_file() { + let path = std::path::Path::new("dir/transactions.json"); + let (a, b) = (unique_tmp_path(path), unique_tmp_path(path)); + assert_ne!(a, b); + for tmp in [&a, &b] { + assert_eq!( + tmp.parent(), + path.parent(), + "a sibling, so the rename is atomic" + ); + assert!(tmp.to_string_lossy().ends_with(".tmp")); + } + + // And the open refuses to adopt a file that is already there. + let dir = TempDir::new().expect("failed to create temp dir"); + let taken = dir.path().join("taken.tmp"); + fs::write(&taken, b"another writer's bytes").expect("plant"); + assert!(create_tmp(&taken).is_err()); + assert_eq!(fs::read(&taken).expect("read"), b"another writer's bytes"); + } + + /// T-5: replacing the log by rename keeps the permissions of the log it + /// replaces; an in-place write did, and a fresh file would silently widen them. + #[cfg(unix)] + #[test] + fn save_preserves_the_logs_permissions() { + use std::os::unix::fs::PermissionsExt; + + let tmp = TempDir::new().expect("failed to create temp dir"); + let path = tmp.path().join("transactions.json"); + let mut manager = TransactionManager::new(path.clone()).expect("new manager"); + manager.begin(None).expect("begin"); + + fs::set_permissions(&path, fs::Permissions::from_mode(0o640)).expect("chmod"); + manager.add_operation("op-1".to_string()).expect("op-1"); + manager.commit().expect("commit"); + + let mode = fs::metadata(&path).expect("stat").permissions().mode() & 0o777; + assert_eq!(mode, 0o640, "saves keep the log's mode"); + } + #[test] fn test_cannot_begin_while_active() { let tmp = TempDir::new().expect("failed to create temp dir"); From 2ab64debeaeeb4d61b5a842303efbca85d06d594 Mon Sep 17 00:00:00 2001 From: "coderabbitai[bot]" <136622811+coderabbitai[bot]@users.noreply.github.com> Date: Tue, 27 Oct 2026 01:42:16 +0000 Subject: [PATCH 4/4] docs(transactions): document persistence errors and rollback retry behavior --- crates/januskey-cli/src/main.rs | 10 ++++ crates/reversible-core/src/transaction.rs | 72 ++++++++++++++++++++--- 2 files changed, 74 insertions(+), 8 deletions(-) diff --git a/crates/januskey-cli/src/main.rs b/crates/januskey-cli/src/main.rs index 35fe1d5..864413c 100644 --- a/crates/januskey-cli/src/main.rs +++ b/crates/januskey-cli/src/main.rs @@ -800,6 +800,16 @@ fn cmd_commit(dir: &Path) -> Result<()> { Ok(()) } +/// Undo the active transaction in `dir` in reverse order, then mark it rolled back. +/// +/// Operations already recorded as undone are skipped on retry. On success, +/// print a summary; its operation count includes those skipped operations. +/// +/// # Errors +/// +/// Propagates errors opening the repository, undoing operations or saving the +/// final transaction state, and errors if no transaction is active. An undo or +/// final save error leaves the transaction active; completed undos remain applied. fn cmd_rollback(dir: &Path) -> Result<()> { let mut jk = JanusKey::open(dir).context("Failed to open JanusKey directory")?; diff --git a/crates/reversible-core/src/transaction.rs b/crates/reversible-core/src/transaction.rs index 7d1e869..6077cd4 100644 --- a/crates/reversible-core/src/transaction.rs +++ b/crates/reversible-core/src/transaction.rs @@ -110,7 +110,9 @@ impl TransactionLog { /// Distinguishes the temporary files of saves made by one process. static SAVE_SEQ: AtomicU64 = AtomicU64::new(0); -/// A sibling of `path` that no other save, in this or another process, uses. +/// Build a sibling temporary pathname using the process ID and save sequence. +/// +/// Does not create or reserve the path; [`create_tmp`] checks for existing files. fn unique_tmp_path(path: &Path) -> PathBuf { let mut tmp = path.as_os_str().to_owned(); tmp.push(format!( @@ -123,6 +125,10 @@ fn unique_tmp_path(path: &Path) -> PathBuf { /// Create the temporary file, failing if it exists: never truncate or adopt a /// file another writer may be using. +/// +/// # Errors +/// +/// Returns an I/O error if the path already exists or the file cannot be created. fn create_tmp(tmp: &Path) -> std::io::Result { fs::OpenOptions::new() .write(true) @@ -132,8 +138,15 @@ fn create_tmp(tmp: &Path) -> std::io::Result { /// Replace `path` with `content` so a reader sees the old file or the new one, /// never a torn write: write a uniquely named sibling temporary file, flush it -/// to disk, then rename it over `path`. The file keeps the permissions of the -/// one it replaces. +/// to disk, then rename it over `path`. Copies the existing file's permissions +/// when its metadata can be read; otherwise keeps the new file's permissions. +/// +/// # Errors +/// +/// Returns temporary-file creation, write, permission-setting, sync or rename +/// errors, attempting to remove the temporary file on failure. Metadata lookup, +/// cleanup and parent-directory sync errors are ignored, so success does not +/// guarantee the rename will survive a crash. fn write_atomic(path: &Path, content: &[u8]) -> std::io::Result<()> { let tmp = unique_tmp_path(path); @@ -208,8 +221,12 @@ impl TransactionManager { /// Persist `next` and, only once it is on disk, make it the in-memory log. /// - /// Every mutator builds its successor log on a copy and ends here, so a - /// failed save leaves memory and disk both at the previous log (T-5). + /// Creates missing parent directories. A failed save leaves the in-memory + /// log unchanged and does not replace the existing log file (T-5). + /// + /// # Errors + /// + /// Propagates directory creation, JSON serialisation and atomic-write errors. fn persist(&mut self, next: TransactionLog) -> Result<()> { if let Some(parent) = self.path.parent() { fs::create_dir_all(parent)?; @@ -222,6 +239,14 @@ impl TransactionManager { /// Apply `f` to the active transaction of a copy of the log, optionally /// clear the active pointer, persist, and return the updated transaction. + /// + /// When `settle` is true, also clears the active pointer. + /// + /// # Errors + /// + /// Returns `ReversibleError::NoActiveTransaction` if the active pointer is + /// absent or its transaction is missing. Propagates persistence errors, + /// leaving the in-memory log unchanged. fn update_active( &mut self, settle: bool, @@ -247,7 +272,13 @@ impl TransactionManager { Ok(result) } - /// Begin a new transaction + /// Begin and persist a new active transaction, returning its stored record. + /// + /// # Errors + /// + /// Returns `ReversibleError::TransactionActive` if an active pointer is already + /// set. Propagates JSON serialisation and I/O errors without changing the + /// in-memory log. pub fn begin(&mut self, name: Option) -> Result<&Transaction> { if let Some(ref active_id) = self.log.active_transaction_id { return Err(ReversibleError::TransactionActive(active_id.clone())); @@ -282,13 +313,30 @@ impl TransactionManager { self.log.transactions.iter_mut().find(|t| t.id == active_id) } - /// Add operation to active transaction + /// Append an operation ID to the active transaction and persist the log. + /// + /// IDs are recorded in the supplied order without validation or deduplication. + /// + /// # Errors + /// + /// Returns `ReversibleError::NoActiveTransaction` if no active transaction is + /// found. Propagates JSON serialisation and I/O errors without changing the + /// in-memory log. pub fn add_operation(&mut self, operation_id: String) -> Result<()> { self.update_active(false, |t| t.add_operation(operation_id)) .map(|_| ()) } - /// Commit the active transaction (marks state only — no filesystem effects) + /// Commit the active transaction and return its completed record. + /// + /// Persists the completion time and clears the active pointer. The recorded + /// file operations are left as they are. + /// + /// # Errors + /// + /// Returns `ReversibleError::NoActiveTransaction` if no active transaction is + /// found. Propagates JSON serialisation and I/O errors without changing the + /// in-memory log. pub fn commit(&mut self) -> Result { self.update_active(true, Transaction::commit) } @@ -297,6 +345,14 @@ impl TransactionManager { /// /// The caller is responsible for actually undoing the operations /// via the appropriate executor before calling this. + /// Persists the completion time, clears the active pointer and returns the + /// completed transaction record. + /// + /// # Errors + /// + /// Returns `ReversibleError::NoActiveTransaction` if no active transaction is + /// found. Propagates JSON serialisation and I/O errors without changing the + /// in-memory log; any undos performed by the caller remain applied. pub fn mark_rolled_back(&mut self) -> Result { self.update_active(true, Transaction::rollback) }