From 8fb6588e6374278d4f9f0dca904717e514d7e432 Mon Sep 17 00:00:00 2001 From: Transaction Science Date: Mon, 19 Oct 2026 21:36:33 -0400 Subject: [PATCH] jcp-wire: one writer per journal, or the replay spends twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SPEC §5 requires an enforcement point that can restart to record every allowed decision durably and to re-establish spend by **replaying** the record. Nothing said the record may have only one writer, and `FileJournal::open` was OpenOptions::new().read(true).append(true).create(true) with no lock at all. **The failure is a double-spend, not a corrupt file.** Two endpoints sharing one journal each replay it, each derive the *same* remaining budget, each decide against it and each append. The budget is spent twice, every record still verifies, and nothing in the journal shows it — the durability requirement is defeated one layer beneath itself. `open` also truncates a torn trailing record, so a concurrent opener can cut the file out from under an appender. `open` now takes an exclusive lock, and takes it **before** the read and the truncation: an opener refused afterwards would already have cut the holder's file. The lock is `std::fs::File::try_lock`, stable since Rust 1.89: no `unsafe`, no new dependency, no lockfile change in any workspace, and no platform caveat, because std covers Windows as well as unix. Verified on the toolchain rather than assumed — a second `try_lock` on a held file returns `WouldBlock`, and the lock releases when the file is dropped. It is also released when the process dies, so a crash leaves no stale lock to recover; a lock-file sidecar would need a story for that case, and that story is where such schemes go wrong. Where a platform cannot lock at all, `open` **fails** rather than proceeding. Deciding against a record another endpoint may also be writing is the overspend this exists to stop, and a platform limit is not a licence for it. Falsification, five mutations, all killed by their named test. Two are worth reading: - **the lock taken *after* the read and truncation** fails **only** the no-truncation test, which isolates the ordering from the presence of the lock; - **the unsupported-platform error swallowed** survived the first round, because that arm is unreachable on a platform whose locks work — and an unreachable arm is exactly where a fail-**open** hides. `lock_outcome` is split out from the I/O so every arm is reachable from a test, and the mutation now fails only its own. A blocking-lock mutation was deliberately not used: dropping the non-blocking attempt would hang the suite rather than fail a test, and a mutation that hangs reports nothing. SPEC §5 gains the requirement, since the gap was in the spec before it was in the code: a single writer per record is MUST, failing to start beats proceeding beside another writer, an exclusive claim the OS releases on death is preferred, and a platform that cannot provide exclusivity means failing to open rather than proceeding. No conformance clause is added: the pack has no vector for it, and numbering a clause nothing tests would claim coverage that does not exist. `mesh-core`'s journal had the identical defect and is fixed the same way on its own branch, since the two standards deliberately keep their own copies of this machinery. Co-Authored-By: Claude Opus 5 --- jcp/SPEC.md | 18 +++ jcp/jcp-rs/crates/jcp-wire/src/journal.rs | 159 +++++++++++++++++++++- 2 files changed, 176 insertions(+), 1 deletion(-) diff --git a/jcp/SPEC.md b/jcp/SPEC.md index eabcb481c..d7c796774 100644 --- a/jcp/SPEC.md +++ b/jcp/SPEC.md @@ -540,6 +540,24 @@ the recorded decisions through the gate** is RECOMMENDED over restoring a stored total: a replay cannot produce a spend the gate would not have produced, and a stored total is one more figure that can be wrong or tampered with. +**One writer per record (MUST).** The replay above re-establishes spend *from* +the record, so two enforcement points sharing one record each re-establish the +**same** spend, each decide against it, and each append. The budget is spent +twice and every entry still verifies — the requirement above is defeated one +layer beneath itself, and nothing in the record shows it. An implementation +**MUST** therefore ensure a single writer per record: it **MUST** take +exclusive access when it opens the record, and **MUST** fail to start rather +than proceed where another writer already holds it. + +An exclusive claim an operating system releases when the holding process dies +is preferable to one an implementation must clean up, because a crash then +leaves nothing to recover. Where a platform cannot provide exclusivity at all, +the implementation **MUST** fail to open the record rather than proceed — +deciding against a record another endpoint may also be writing is the overspend +this requirement exists to prevent, and a platform limit is not a licence for +it. An implementation that instead proceeds **MUST** make that fact checkable +at run time rather than leave the type to imply a property it does not hold. + **The decision is recorded before the action runs.** Recording it afterwards loses the debit for any action interrupted between the two — the grant was debited when the decision was made, so a record written after the fact reports a diff --git a/jcp/jcp-rs/crates/jcp-wire/src/journal.rs b/jcp/jcp-rs/crates/jcp-wire/src/journal.rs index c4ae85831..d4fe6c0f1 100644 --- a/jcp/jcp-rs/crates/jcp-wire/src/journal.rs +++ b/jcp/jcp-rs/crates/jcp-wire/src/journal.rs @@ -3,7 +3,7 @@ //! gate when it restarts, so a restart gives no budget back, unrevokes //! nothing, and forgets no request it may still be shown again. -use std::fs::{File, OpenOptions}; +use std::fs::{File, OpenOptions, TryLockError}; use std::io::{self, Read, Write}; use std::path::Path; @@ -65,6 +65,11 @@ impl FileJournal { /// records it already holds, for [`Endpoint::with_journal`](crate::Endpoint::with_journal) /// to replay. /// + /// Takes an **exclusive** lock first, so a second process cannot hold the + /// same journal. Fails with [`io::ErrorKind::WouldBlock`] where the + /// journal is already held, and fails rather than proceeding where the + /// platform cannot provide a lock at all. + /// /// A final record cut short is an append the endpoint was interrupted in. /// It is dropped and cut from the file, which is safe for both kinds: a /// request never recorded was never decided, and a revocation never @@ -72,6 +77,10 @@ impl FileJournal { /// error, because a journal that cannot be read cannot re-establish state. pub fn open(path: impl AsRef) -> io::Result<(Self, Vec)> { let mut file = OpenOptions::new().read(true).append(true).create(true).open(path)?; + // Before the read, and before the truncation below. A would-be second + // writer must be refused *before* it can cut a torn trailing record + // out from under the holder's appends. + lock_exclusively(&file)?; let mut bytes = Vec::new(); file.read_to_end(&mut bytes)?; let mut records = Vec::new(); @@ -96,6 +105,44 @@ impl FileJournal { } } +/// Take an exclusive lock on the journal file, so a second endpoint cannot +/// hold it. +/// +/// **What this prevents is a double-spend, not a corrupt file.** Two endpoints +/// on one journal each replay it, each derive the *same* remaining budget, each +/// admit against it and each append. The budget is spent twice and every record +/// still verifies — the replay that makes spend durable is defeated one layer +/// below itself. [`FileJournal::open`] also truncates a torn trailing record, +/// so a concurrent opener can cut the file out from under an appender. +/// +/// The lock is the operating system's, held by the open file and released when +/// it closes **or the process dies**, so a crash leaves nothing to recover. A +/// lock-file sidecar would need a story for the crash case, and that story is +/// where such schemes go wrong. +fn lock_exclusively(file: &File) -> io::Result<()> { + lock_outcome(file.try_lock()) +} + +/// Turn a lock attempt into an outcome for [`FileJournal::open`]. +/// +/// Separate from the I/O so every arm is testable. The +/// [`TryLockError::Error`] arm cannot be reached on a platform whose locks +/// work, and an unreachable arm is exactly where a fail-**open** would sit +/// unnoticed. +fn lock_outcome(attempt: Result<(), TryLockError>) -> io::Result<()> { + match attempt { + Ok(()) => Ok(()), + Err(TryLockError::WouldBlock) => Err(io::Error::new( + io::ErrorKind::WouldBlock, + "journal already held by another writer: a second one would spend the same budget twice", + )), + // The platform could not provide exclusivity. Failing to open is the + // safe direction: the alternative is deciding against a record another + // endpoint may also be writing. + Err(TryLockError::Error(e)) => Err(e), + } +} + impl Journal for FileJournal { fn append(&mut self, record: &JournalRecord) -> io::Result<()> { let body = canonical_cbor(record).map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e.to_string()))?; @@ -106,3 +153,113 @@ impl Journal for FileJournal { self.file.sync_data() } } + +#[cfg(test)] +mod tests { + use super::*; + + /// A unique path per test, in the process's temp dir. + fn tmp_path(tag: &str) -> std::path::PathBuf { + static N: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); + let n = N.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + let p = std::env::temp_dir() + .join(format!("jcp-wire-journal-{}-{tag}-{n}.log", std::process::id())); + let _ = std::fs::remove_file(&p); + p + } + + /// A second endpoint must not be able to hold the journal. + /// + /// `std::fs::File::try_lock` is the operating system's lock, so this needs + /// no platform caveat and no `unsafe`: where a platform cannot provide one + /// at all, `open` fails rather than proceeding. + #[test] + fn the_single_writer_claim_matches_what_open_does() { + let path = tmp_path("second-open"); + let (_held, records) = FileJournal::open(&path).expect("the first open"); + assert!(records.is_empty()); + + let err = FileJournal::open(&path) + .err() + .expect("a second open of a held journal must be refused"); + assert_eq!(err.kind(), io::ErrorKind::WouldBlock, "{err}"); + let _ = std::fs::remove_file(&path); + } + + /// Every arm of the lock policy, including the one `open` cannot reach on + /// a platform whose locks work. A platform that cannot lock **MUST NOT** + /// yield an open journal: the alternative is deciding against a record + /// another endpoint may also be writing. + #[test] + fn a_platform_that_cannot_lock_fails_closed() { + assert!(lock_outcome(Ok(())).is_ok(), "an acquired lock opens the journal"); + + let contended = lock_outcome(Err(TryLockError::WouldBlock)) + .expect_err("a contended lock must not open the journal"); + assert_eq!(contended.kind(), io::ErrorKind::WouldBlock); + + let unsupported = + lock_outcome(Err(TryLockError::Error(io::Error::from(io::ErrorKind::Unsupported)))) + .expect_err("an unsupported lock must not open the journal"); + assert_eq!( + unsupported.kind(), + io::ErrorKind::Unsupported, + "the platform's own error is propagated, not replaced" + ); + } + + /// The lock is the kernel's, held by the descriptor, so dropping the + /// journal frees it. A crash frees it the same way, which is why this is + /// an advisory lock rather than a lock file. + #[test] + fn dropping_the_journal_releases_the_lock() { + let path = tmp_path("release"); + { + let (_held, _) = FileJournal::open(&path).expect("the first open"); + } // dropped here + let (_again, records) = FileJournal::open(&path).expect("reopen after drop"); + assert!(records.is_empty()); + let _ = std::fs::remove_file(&path); + } + + /// **The lock is taken before the truncation.** `open` cuts a torn + /// trailing record from the file, so an opener refused *after* that point + /// would already have cut the holder's file. A refused open must leave the + /// journal byte-for-byte as it was. + /// + /// Raw bytes rather than a real record: the property under test is the + /// ordering of the lock against the truncation, and a torn frame is all + /// that takes. + #[test] + fn a_refused_open_does_not_truncate_the_holders_journal() { + let path = tmp_path("no-truncate"); + let (held, _) = FileJournal::open(&path).expect("the first open"); + + // A torn frame appended behind the holder's back: a length header + // promising 64 bytes with 3 following, which `open` truncates away. + { + use std::io::Write; + let mut raw = OpenOptions::new().append(true).open(&path).expect("raw append"); + raw.write_all(&[0, 0, 0, 64]).expect("torn header"); + raw.write_all(&[1, 2, 3]).expect("short body"); + raw.sync_data().expect("sync"); + } + let before = std::fs::metadata(&path).expect("stat").len(); + assert_eq!(before, 7, "the torn frame is on disk"); + + assert!(FileJournal::open(&path).is_err(), "the second open must be refused"); + assert_eq!( + std::fs::metadata(&path).expect("stat").len(), + before, + "a refused open truncated the file it was refused from" + ); + drop(held); + + // Once the holder is gone the torn frame is cut, as it always was — + // the truncation still happens, just never from a refused writer. + let (_now, records) = FileJournal::open(&path).expect("reopen"); + assert!(records.is_empty(), "the torn frame yields no record"); + assert_eq!(std::fs::metadata(&path).expect("stat").len(), 0, "and is cut"); + let _ = std::fs::remove_file(&path); + } +} -- 2.54.0 (Apple Git-157)