From 11edaa0522d4856af265126316f3febf3090fbec Mon Sep 17 00:00:00 2001 From: kjgbot Date: Sat, 5 Sep 2026 20:16:30 +0200 Subject: [PATCH] fix(kernel): adopt only a journal that resume can actually use #185, a regression I introduced in #177. `Engine::start` creates the journal, appends RunSpawned, then registers, so a crash leaves TWO different residues and #177 only recognised one: * killed AFTER RunSpawned -- a real run missing its index entry. Adopt it; that is what #177 fixed. * killed BEFORE RunSpawned -- an empty journal that never became a run. #177's gate was `journal.run_id() == params.run_id`. An empty journal still carries a meta row with the run id, so it passed: the file was adopted, registered, and resume then died on `read run spec: Query returned no rows` -- an internal failure where the honest answer is that the run does not exist. Before #177 that case returned a clean `run_not_found`, so the change traded a legible refusal for a confusing error in the one sub-case it did not anticipate. Adoption now also requires `run_spec()` to succeed. That is the honest predicate because it is exactly what resume calls next: adopt only what resume can actually use. Found by the diagnostics from #175 and #176 on run 33982088411, where the crash-resume test failed in 63 seconds instead of hanging, and the dump printed the cause: stderr (114 bytes): Error: journal_write_failed: read run spec: SQLite journal failed: Query returned no rows --- journal (0 entries) --- Evidence. Every command below is literally runnable from the repository root, and the output is complete: $ shasum -a 256 kernel/relayflowd/src/server.rs f5e10e4b3d00a9e5c16c24624f9253d578e4eeefabb6483e5d1ebda26fefc88f kernel/relayflowd/src/server.rs $ (cd kernel && PATH="$HOME/.cargo/bin:$PATH" RUSTUP_TOOLCHAIN=stable cargo test -p relayflowd --lib server::tests::run_resume) Finished `test` profile [unoptimized + debuginfo] target(s) in 0.18s Running unittests src/lib.rs (target/debug/deps/relayflowd-01b0ec98cb5c81e7) running 4 tests test server::tests::run_resume_asks_the_registry_instead_of_treating_an_orphan_file_as_a_run ... ok test server::tests::run_resume_refuses_a_journal_that_never_recorded_its_run ... ok test server::tests::run_resume_refuses_a_valid_journal_that_belongs_to_another_run ... ok test server::tests::run_resume_adopts_a_real_journal_whose_registry_row_is_missing ... ok test result: ok. 4 passed; 0 failed; 0 ignored; 0 measured; 26 filtered out; finished in 0.02s # MUTATION: drop the run_spec() requirement -- back to #177 behaviour $ shasum -a 256 kernel/relayflowd/src/server.rs 47e5ae9042c1b02ea8a3d1eea3ce96c32408b484b483a8fe8ea1e78862662432 kernel/relayflowd/src/server.rs $ (cd kernel && PATH="$HOME/.cargo/bin:$PATH" RUSTUP_TOOLCHAIN=stable cargo test -p relayflowd --lib server::tests::run_resume) Compiling relayflowd v0.1.0 (/Users/khaliqgant/AgentWorkforce/flows-185/kernel/relayflowd) Finished `test` profile [unoptimized + debuginfo] target(s) in 0.87s Running unittests src/lib.rs (target/debug/deps/relayflowd-01b0ec98cb5c81e7) running 4 tests test server::tests::run_resume_asks_the_registry_instead_of_treating_an_orphan_file_as_a_run ... ok test server::tests::run_resume_refuses_a_journal_that_never_recorded_its_run ... FAILED test server::tests::run_resume_refuses_a_valid_journal_that_belongs_to_another_run ... ok test server::tests::run_resume_adopts_a_real_journal_whose_registry_row_is_missing ... ok failures: ---- server::tests::run_resume_refuses_a_journal_that_never_recorded_its_run stdout ---- thread 'server::tests::run_resume_refuses_a_journal_that_never_recorded_its_run' (18692000) panicked at relayflowd/src/server/tests.rs:295:5: assertion `left == right` failed: refusing it as not-found is the honest answer; an internal spec-read failure is not left: "journal_write_failed" right: "run_not_found" note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace failures: server::tests::run_resume_refuses_a_journal_that_never_recorded_its_run test result: FAILED. 3 passed; 1 failed; 0 ignored; 0 measured; 26 filtered out; finished in 0.03s error: test failed, to rerun pass `-p relayflowd --lib` # RESTORED $ shasum -a 256 kernel/relayflowd/src/server.rs f5e10e4b3d00a9e5c16c24624f9253d578e4eeefabb6483e5d1ebda26fefc88f kernel/relayflowd/src/server.rs The mutation drops the `run_spec()` requirement, restoring #177's gate exactly. It fails only the new test, while `run_resume_adopts_a_real_journal_whose_registry_row_is_missing` keeps passing -- so this narrows adoption without undoing what #177 fixed. Kernel workspace at this head: 157 passed, 0 failed, no warnings. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- kernel/relayflowd/src/server.rs | 19 +++++++++- kernel/relayflowd/src/server/tests.rs | 51 +++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/kernel/relayflowd/src/server.rs b/kernel/relayflowd/src/server.rs index 8ee07ed99..16bee6a3f 100644 --- a/kernel/relayflowd/src/server.rs +++ b/kernel/relayflowd/src/server.rs @@ -187,8 +187,25 @@ fn handle_request( // by registry-owned lookup for exactly that reason, so the // comparison below is what keeps this a repair of the index // rather than a reopening of that hole. + // + // "Says it is this run" is necessary but NOT sufficient, which + // #177 missed. `Engine::start` creates the journal, appends + // RunSpawned, then registers -- so a crash has TWO possible + // residues, not one: + // + // * killed after RunSpawned: a real run with no index entry. + // Adopt it; that is the bug #177 fixed. + // * killed BEFORE RunSpawned: an empty journal that never + // became a run. It still carries a meta row with the run + // id, so an id check alone accepts it -- and resume then + // dies on `read run spec: Query returned no rows` instead + // of saying the run does not exist (#185). + // + // `run_spec()` is the honest predicate because it is exactly + // what resume will call next: adopt only what resume can + // actually use. let adopted = match relayflowd_journal::SqliteJournal::open(&path) { - Ok(journal) => journal.run_id() == params.run_id, + Ok(journal) => journal.run_id() == params.run_id && journal.run_spec().is_ok(), Err(_) => false, }; if !adopted { diff --git a/kernel/relayflowd/src/server/tests.rs b/kernel/relayflowd/src/server/tests.rs index cf2809cf6..9adf8decc 100644 --- a/kernel/relayflowd/src/server/tests.rs +++ b/kernel/relayflowd/src/server/tests.rs @@ -255,6 +255,57 @@ fn run_resume_adopts_a_real_journal_whose_registry_row_is_missing() { ); } +/// #185. The fourth case, and the one #177 got wrong: a journal that was +/// CREATED but never recorded its run. +/// +/// `Engine::start` creates the journal, appends RunSpawned, then registers, so a +/// crash has two residues. One is a real run missing its index entry -- adopt +/// it. The other is an empty file that never became a run, and it still carries +/// a meta row with the run id, so an id check alone accepts it. #177 did exactly +/// that, and resume then died on `read run spec: Query returned no rows` instead +/// of saying the run does not exist. +#[test] +fn run_resume_refuses_a_journal_that_never_recorded_its_run() { + let directory = tempdir().unwrap(); + let data_dir = directory.path(); + let run_id = "01EMPTYJOURNALEMPTYJOURNAL"; + + // Exactly what a kill between `create` and the RunSpawned append leaves: + // a valid journal for this run id, with no entries at all. + std::fs::create_dir_all(data_dir.join("runs")).unwrap(); + let path = data_dir.join("runs").join(format!("{run_id}.sqlite3")); + let journal = relayflowd_journal::SqliteJournal::create(&path, run_id, 0).unwrap(); + assert_eq!(journal.run_id(), run_id, "the meta row is what makes this tempting"); + assert!(journal.run_spec().is_err(), "and there is no spec to resume"); + drop(journal); + + let hub = Arc::new(ProtocolHub::default()); + let (writer, _peer) = shared_writer(); + let response = request( + data_dir, + &hub, + 1, + &writer, + &format!(r#"{{"id":"resume","verb":"run.resume","params":{{"run_id":"{run_id}"}}}}"#), + ); + + let error = response + .error + .expect("a journal that never recorded its run must be refused"); + assert_eq!( + error.code, "run_not_found", + "refusing it as not-found is the honest answer; an internal spec-read \ + failure is not" + ); + + let registry = + relayflowd_journal::Registry::open(data_dir.join("relayflowd.sqlite3")).unwrap(); + assert!( + registry.lookup(run_id).unwrap().is_none(), + "a refused journal must not leave a registry row behind" + ); +} + /// #174, third case: a journal that is structurally VALID but belongs to a /// different run must still be refused. ///