diff --git a/.config/nextest.toml b/.config/nextest.toml index f29a06d8f..90fd4c88f 100644 --- a/.config/nextest.toml +++ b/.config/nextest.toml @@ -157,7 +157,7 @@ binary_id(cyber-account) | binary_id(cyber-server::worktree_setup_journal) | binary_id(cyber-server::worktree_setup_output) | binary_id(cyber-snapshot) -| binary_id(cyber-store) & (test(migrate::tests::messaging_) | test(migrate::tests::usage_origin_)) +| binary_id(cyber-store) & (test(writer::tests::) | test(migrate::tests::messaging_) | test(migrate::tests::usage_origin_)) | binary_id(cyber-store::store) | binary_id(cyber-tools) & (test(tools::bash::shell_tests) | test(plugin_host::) | test(browser::) | test(dev_server::) | test(permissions::engine::tests::message_send) | test(permissions::engine::tests::remote_attach) | test(remote_skills) | test(lsp::) | test(mcp::stdio::tests) | test(mcp::roots::tests) | test(mcp::elicitation::tests) | test(mcp::sampling::tests) | test(tools::process::tests) | test(tools::tool_search::tests) | test(mcp::pool::tests) | test(mcp::launch::tests) | test(mcp::http::tests) | test(mcp::oauth) | test(hook_commands::launch::tests) | test(sandboxing::tests) | test(permissions::filesystem) | test(tools::background::tests) | test(tools::monitor::) | test(tools::deliver::tests) | test(tools::powershell::tests) | test(permissions::dry_run) | test(sandboxing::tests::refusal_lines) | test(permissions::engine::tests::sandbox_boundary_needs_an_explicit_user_allow) | test(permissions::engine::tests::workflow_run) | test(permissions::taint::) | test(untrusted::)) | binary_id(cyber-tools::agent_context) diff --git a/crates/cyber-server/Cargo.toml b/crates/cyber-server/Cargo.toml index 9293bdf04..09d4d4279 100644 --- a/crates/cyber-server/Cargo.toml +++ b/crates/cyber-server/Cargo.toml @@ -61,6 +61,7 @@ tempfile.workspace = true rustix = { version = "=1.1.5", default-features = false, features = ["fs", "std"] } [dev-dependencies] +rusqlite = { workspace = true, features = ["hooks"] } cap-std = "4.0" tempfile.workspace = true reqwest = { version = "0.13", features = ["json", "stream"] } diff --git a/crates/cyber-server/tests/location_activity.rs b/crates/cyber-server/tests/location_activity.rs index 4e2f38d58..c287d39ed 100644 --- a/crates/cyber-server/tests/location_activity.rs +++ b/crates/cyber-server/tests/location_activity.rs @@ -1,5 +1,6 @@ mod support; +use cyber_core::worktrees::Managed; use cyber_server::runtime::*; use futures::future::BoxFuture; use std::sync::{ @@ -19,6 +20,7 @@ struct Activity { claim_unresponsive: AtomicBool, preserve_binding: AtomicBool, fail_settlement: AtomicBool, + shell_launches: AtomicUsize, cwd_hooks: Mutex>, } @@ -111,6 +113,7 @@ impl ToolHost for Host { _: &str, cancel: CancellationToken, ) -> BoxFuture<'_, Result> { + self.activity.shell_launches.fetch_add(1, Ordering::SeqCst); Box::pin(async move { self.tools.started.notify_one(); if self.activity.shell_unresponsive.load(Ordering::SeqCst) { @@ -475,43 +478,66 @@ fn retained_location_proof_preserves_acknowledged_guard_until_disposal() { drop(LocationLease::unmanaged().settle_retained().unwrap()); } +struct Relocation { + h: Harness, + runtime: Runtime, + activity: Arc, + info: SessionInfo, + managed: Managed, +} + +async fn running_relocation() -> Relocation { + let h = Harness::new(Setup { + scripts: vec![( + "test/main", + vec![tools(&[("lease_call", "read", "{}")]), text("done")], + )], + ..Default::default() + }); + h.tools.set("read", Behavior::Gated); + let (runtime, activity) = owned_runtime(&h); + activity.preserve_binding.store(true, Ordering::SeqCst); + let info = create(&runtime, &h, "test/main").await; + assert!(info.worktree_id.is_none()); + let destination = h.dir.path().join("destination"); + std::fs::create_dir(&destination).unwrap(); + let managed = Managed { + id: "wt_test".into(), + name: "destination".into(), + path: destination.clone(), + branch: "cyber/destination".into(), + base: "main".into(), + common_dir: h.repo.join(".git"), + ready: true, + included: vec![], + }; + runtime + .admit(&info.id, Admission::text("run", Delivery::Queue)) + .await + .unwrap(); + tokio::time::timeout(Duration::from_secs(5), h.tools.started.notified()) + .await + .unwrap(); + assert_eq!(activity.active.load(Ordering::SeqCst), 1); + Relocation { + h, + runtime, + activity, + info, + managed, + } +} + #[tokio::test] async fn cwd_changed_requires_acknowledged_previous_location_settlement() { - use cyber_core::worktrees::Managed; - for unacknowledged in [false, true] { - let h = Harness::new(Setup { - scripts: vec![( - "test/main", - vec![tools(&[("lease_call", "read", "{}")]), text("done")], - )], - ..Default::default() - }); - h.tools.set("read", Behavior::Gated); - let (runtime, activity) = owned_runtime(&h); - activity.preserve_binding.store(true, Ordering::SeqCst); - let info = create(&runtime, &h, "test/main").await; - assert!(info.worktree_id.is_none()); - let destination = h.dir.path().join("destination"); - std::fs::create_dir(&destination).unwrap(); - let managed = Managed { - id: "wt_test".into(), - name: "destination".into(), - path: destination.clone(), - branch: "cyber/destination".into(), - base: "main".into(), - common_dir: h.repo.join(".git"), - ready: true, - included: vec![], - }; - runtime - .admit(&info.id, Admission::text("run", Delivery::Queue)) - .await - .unwrap(); - tokio::time::timeout(Duration::from_secs(5), h.tools.started.notified()) - .await - .unwrap(); - assert_eq!(activity.active.load(Ordering::SeqCst), 1); + let Relocation { + h, + runtime, + activity, + info, + managed, + } = running_relocation().await; activity .fail_settlement .store(unacknowledged, Ordering::SeqCst); @@ -530,7 +556,7 @@ async fn cwd_changed_requires_acknowledged_previous_location_settlement() { // The path is committed before settlement of the previous admission. An // uncertain acknowledgement must not turn that record into a success hook. let state = runtime.state(&info.id).await.unwrap(); - assert_eq!(state.info.directory, destination.display().to_string()); + assert_eq!(state.info.directory, managed.path.display().to_string()); assert_eq!(state.info.worktree_id.as_deref(), Some("wt_test")); let events = h .store @@ -597,3 +623,144 @@ async fn cwd_changed_requires_acknowledged_previous_location_settlement() { ); } } + +#[tokio::test] +async fn cwd_changed_lost_commit_ack_is_silent_and_replay_does_not_dispatch() { + use std::cell::RefCell; + + // SQLite invokes this callback after committing and propagates its error to + // COMMIT. Each fixture owns a writer thread, so parallel tests share no arm. + std::thread_local! { + static FAIL_WAL_ACK: RefCell>> = const { RefCell::new(None) }; + } + fn fail_wal_ack(_: &rusqlite::hooks::Wal, _: i32) -> rusqlite::Result<()> { + FAIL_WAL_ACK.with(|state| match state.borrow_mut().take() { + Some(reached) => { + reached.store(true, Ordering::SeqCst); + Err(rusqlite::Error::SqliteFailure( + rusqlite::ffi::Error::new(rusqlite::ffi::SQLITE_ERROR), + Some("injected lost WAL commit acknowledgement".into()), + )) + } + None => Ok(()), + }) + } + + let Relocation { + h, + runtime, + activity, + info, + managed, + } = running_relocation().await; + runtime + .enter_worktree( + &info.id, + "lease_call", + &managed, + false, + CancellationToken::new(), + ) + .await + .unwrap(); + h.tools.release.notify_one(); + tokio::time::timeout(Duration::from_secs(5), runtime.wait_idle(&info.id)) + .await + .unwrap(); + assert_eq!(activity.cwd_hooks.lock().unwrap().len(), 1); + let before = h + .store + .read_events(&info.id, -1, cyber_store::MAX_PAGE_LIMIT) + .unwrap(); + assert!(!before.has_more); + + let reached = Arc::new(AtomicBool::new(false)); + let fault = Arc::clone(&reached); + h.store + .transaction(move |tx| { + FAIL_WAL_ACK.with(|state| *state.borrow_mut() = Some(fault)); + tx.wal_hook(Some(fail_wal_ack)); + Ok(()) + }) + .unwrap(); + // Installing the callback writes no pages and must not consume the fault. + assert!(!reached.load(Ordering::SeqCst)); + assert!(!h.store.has_failed()); + + let error = tokio::time::timeout(Duration::from_secs(5), runtime.exit_idle_worktree(&info.id)) + .await + .unwrap() + .unwrap_err(); + assert!( + error.to_string().contains("StorageUnavailableError"), + "{error}" + ); + assert!( + reached.load(Ordering::SeqCst), + "the real WAL commit boundary was not reached" + ); + assert!( + h.store.has_failed(), + "unknown fatal storage outcome must stop mutations" + ); + let after = h + .store + .read_events(&info.id, -1, cyber_store::MAX_PAGE_LIMIT) + .unwrap(); + assert!(!after.has_more); + assert_eq!(after.events.len(), before.events.len() + 1); + let exit = after.events.last().unwrap(); + assert_eq!(exit.kind, "session.worktree.exited.1"); + assert_eq!(exit.data["to"]["path"], info.directory); + // The caller did not receive the acknowledgement and cannot claim success. + assert_eq!( + runtime.state(&info.id).await.unwrap().info.directory, + managed.path.display().to_string() + ); + assert_eq!( + activity.cwd_hooks.lock().unwrap().len(), + 1, + "an uncertain committed exit emitted CwdChanged" + ); + assert!(runtime.exit_idle_worktree(&info.id).await.is_err()); + let shell = tokio::time::timeout( + Duration::from_secs(5), + runtime.shell(&info.id, "must not dispatch after uncertain commit"), + ) + .await + .unwrap(); + assert!( + shell.is_err(), + "a failed Store admitted another shell: {shell:?}" + ); + assert_eq!(activity.shell_launches.load(Ordering::SeqCst), 0); + + // A fresh runtime reads the committed exit, without replaying its handler. + // This is reducer/runtime replay over the same failed Store, not a restarted process. + let (replayed, replay_activity) = owned_runtime(&h); + let state = replayed.state(&info.id).await.unwrap(); + assert_eq!(state.info.directory, info.directory); + assert!(state.info.worktree_id.is_none()); + assert!( + replayed + .exit_idle_worktree(&info.id) + .await + .unwrap() + .is_none() + ); + assert!(replay_activity.cwd_hooks.lock().unwrap().is_empty()); + let final_events = h + .store + .read_events(&info.id, -1, cyber_store::MAX_PAGE_LIMIT) + .unwrap(); + assert!(!final_events.has_more); + assert_eq!(final_events.events.len(), after.events.len()); + assert_eq!(final_events.events.last().unwrap().id, exit.id); + for instance in [runtime, replayed] { + tokio::time::timeout(Duration::from_secs(5), instance.shutdown()) + .await + .unwrap(); + } + assert_eq!(activity.active.load(Ordering::SeqCst), 0); + assert_eq!(replay_activity.active.load(Ordering::SeqCst), 0); +} diff --git a/crates/cyber-store/Cargo.toml b/crates/cyber-store/Cargo.toml index c1b252455..3758a9f01 100644 --- a/crates/cyber-store/Cargo.toml +++ b/crates/cyber-store/Cargo.toml @@ -17,7 +17,7 @@ serde_json.workspace = true thiserror.workspace = true [dev-dependencies] -rusqlite.workspace = true +rusqlite = { workspace = true, features = ["hooks"] } tempfile.workspace = true [lints] diff --git a/crates/cyber-store/src/writer.rs b/crates/cyber-store/src/writer.rs index cb9171716..491ccb988 100644 --- a/crates/cyber-store/src/writer.rs +++ b/crates/cyber-store/src/writer.rs @@ -90,7 +90,17 @@ impl Writer { self.submit(move |conn| { let tx = conn.transaction_with_behavior(TransactionBehavior::Immediate)?; let value = f(&tx)?; - tx.commit()?; + // Inspect the transaction before its Drop can roll it back. SQLite + // can report an error after COMMIT completed (for example, a WAL + // hook error); that outcome must fence the writer regardless of code. + if let Err(error) = tx.execute_batch("COMMIT") { + if tx.is_autocommit() { + return Err(StoreError::Unavailable(format!( + "commit outcome unknown: {error}" + ))); + } + return Err(error.into()); + } Ok(value) }) } @@ -161,4 +171,94 @@ mod tests { assert!(blocker.join().unwrap().is_ok()); assert_eq!(queued.join().unwrap().unwrap(), 7); } + + #[test] + fn committed_wal_error_stops_later_mutations() { + fn lost_ack(_: &rusqlite::hooks::Wal, _: i32) -> rusqlite::Result<()> { + Err(rusqlite::Error::SqliteFailure( + rusqlite::ffi::Error::new(rusqlite::ffi::SQLITE_ERROR), + Some("injected lost commit acknowledgement".into()), + )) + } + + let directory = tempfile::tempdir().unwrap(); + let database = directory.path().join("writer.db"); + let conn = Connection::open(&database).unwrap(); + conn.execute_batch("PRAGMA journal_mode=WAL; CREATE TABLE committed (value INTEGER)") + .unwrap(); + let writer = Writer::spawn(conn, 1, Duration::from_secs(1)).unwrap(); + writer + .transaction(|tx| { + tx.wal_hook(Some(lost_ack)); + Ok(()) + }) + .unwrap(); + let result = writer.transaction(|tx| { + tx.execute("INSERT INTO committed VALUES (7)", [])?; + Ok(7) + }); + // A second connection proves this was committed, not a before-write fault. + let reader = Connection::open(&database).unwrap(); + assert_eq!( + reader + .query_row("SELECT value FROM committed", [], |row| row + .get::<_, i64>(0)) + .unwrap(), + 7 + ); + assert!( + matches!(result, Err(StoreError::Unavailable(_))), + "an unacknowledged completed transaction was not fatal: {result:?}" + ); + assert!(writer.has_failed()); + assert!(matches!( + writer.transaction(|tx| { + tx.execute("INSERT INTO committed VALUES (8)", [])?; + Ok(()) + }), + Err(StoreError::Unavailable(_)) + )); + assert_eq!( + reader + .query_row("SELECT count(*) FROM committed", [], |row| row + .get::<_, i64>(0)) + .unwrap(), + 1 + ); + } + + #[test] + fn rejected_commit_rolls_back_without_stopping_the_writer() { + let conn = Connection::open_in_memory().unwrap(); + conn.execute_batch( + "PRAGMA foreign_keys=ON; + CREATE TABLE parent (id INTEGER PRIMARY KEY); + CREATE TABLE child (parent_id INTEGER REFERENCES parent(id) + DEFERRABLE INITIALLY DEFERRED)", + ) + .unwrap(); + let writer = Writer::spawn(conn, 1, Duration::from_secs(1)).unwrap(); + let result = writer.transaction(|tx| { + tx.execute("INSERT INTO child VALUES (7)", [])?; + Ok(()) + }); + assert!(matches!(result, Err(StoreError::Sqlite(_))), "{result:?}"); + assert!(!writer.has_failed()); + assert_eq!( + writer + .submit(|conn| Ok( + conn.query_row("SELECT count(*) FROM child", [], |row| row.get::<_, i64>(0))? + )) + .unwrap(), + 0 + ); + writer + .transaction(|tx| { + tx.execute("INSERT INTO parent VALUES (7)", [])?; + tx.execute("INSERT INTO child VALUES (7)", [])?; + Ok(()) + }) + .unwrap(); + assert!(!writer.has_failed()); + } } diff --git a/docs/goal-delivery.md b/docs/goal-delivery.md index f1575cac4..2f0b3c8e6 100644 --- a/docs/goal-delivery.md +++ b/docs/goal-delivery.md @@ -71,15 +71,22 @@ main composition; see the PR #88 merged at `9cbbf08b` after all 18 checks passed, including all five CwdChanged lifecycle cases on Windows. -Continue the first workstream in PR #89 on `test/cwd-unknown-settlement`: -unacknowledged Location settlement must not emit successful CwdChanged, and the -FileChanged acceptance record is reconciled against accepted main. The regression -uses the actual runtime/Store with a simulated lease owner; its local positive -suite and behavioral negative control pass, while current-head hosted acceptance -is required. No production native activation or protocol implementation is -claimed. Keep native activation and Linux dependency changes behind their -existing review/acceptance gates. The complete Setup marker/migration choices -and DirectoryAdded proposal also need their recorded review before implementation. +PR #89 merged at `bbc233d5` after all 16 current-head checks passed. Its +[receipt](../openspec/changes/complete-plugin-hook-runtime/evidence/cwd-settlement-9ad0b369.json) +verifies the original unacknowledged Location-settlement regression actually +executed on Linux/macOS/Windows. This accepts its simulated lease-owner case, +not native confinement or the complete producer matrix. + +Continue the first workstream on `test/cwd-wal-commit-ack`: the +[Store commit correction](../openspec/changes/complete-plugin-hook-runtime/store-commit-uncertainty.md) +fences generic errors after a transaction has ended, preserves known rollback, +and verifies no CwdChanged or later shell dispatch follows the uncertain outcome. +The real WAL-error fixture and main comparison pass locally after correction; +current-head hosted acceptance remains required. The new replay case uses a fresh +runtime over the same Store rather than a process restart. Keep native activation +and Linux dependency changes behind their existing review/acceptance gates. The +complete Setup marker/migration choices and DirectoryAdded proposal also need +their recorded review before implementation. Native confinement foundations and regressions exist, but production enforcement and the remaining plugin/hook scope are incomplete. The diff --git a/openspec/changes/complete-plugin-hook-runtime/evidence/cwd-settlement-9ad0b369.json b/openspec/changes/complete-plugin-hook-runtime/evidence/cwd-settlement-9ad0b369.json new file mode 100644 index 000000000..6462d3e33 --- /dev/null +++ b/openspec/changes/complete-plugin-hook-runtime/evidence/cwd-settlement-9ad0b369.json @@ -0,0 +1,46 @@ +{ + "head": "9ad0b369c4b43349a2df582150d5c9fa0d09b5ff", + "tested_merge": "db443ee1e468e055fa040196948936eaff267afb", + "tree": "e5c0cf825c640ff6637d8855a167da62b8591608", + "checks_passed": 16, + "platforms": { + "linux": { + "checkout": "db443ee1e468e055fa040196948936eaff267afb", + "new_case_executed": true, + "log_sha256": "508d58723b2ea135f0acec0d848d7fa1a467ad5b845df1fe7ef8af06b7c6a2ff", + "summaries": [ + "Summary [1254.059s] 5505 tests run: 5505 passed (2 slow), 21 skipped" + ], + "warnings": [], + "job_url": "https://github.com/CyberdyneCorp/cybercode-cli/actions/runs/41229206650/job/124619766981" + }, + "macos": { + "checkout": "db443ee1e468e055fa040196948936eaff267afb", + "new_case_executed": true, + "log_sha256": "30f7762a32f70ff382c562e0cdb3b4ada808938477957d7407a9e1d94164d366", + "summaries": [ + "Summary [1294.429s] 5505 tests run: 5505 passed (2 slow), 23 skipped" + ], + "warnings": [], + "job_url": "https://github.com/CyberdyneCorp/cybercode-cli/actions/runs/41229206650/job/124619767050" + }, + "windows": { + "checkout": "db443ee1e468e055fa040196948936eaff267afb", + "new_case_executed": true, + "log_sha256": "8f061728fc0c194fc2e5b84668ec23d45d8309325e88ff49842966429b837e5a", + "summaries": [ + "Summary [1225.725s] 3781 tests run: 3781 passed (1 slow, 1 leaky), 648 skipped", + "Summary [ 0.033s] 4 tests run: 4 passed, 387 skipped", + "Summary [ 0.122s] 9 tests run: 9 passed, 0 skipped", + "Summary [ 2.480s] 18 tests run: 18 passed, 0 skipped" + ], + "warnings": [ + "LEAK [ 26.449s] ( 520/3781) cyber-cli::messages_cli an_exec_session_refuses_messages_unless_it_accepts_them" + ], + "job_url": "https://github.com/CyberdyneCorp/cybercode-cli/actions/runs/41229206650/job/124619766932" + } + }, + "accepted_merge": "bbc233d572b44468bd906308687de33f6e7c7954", + "accepted_tree_matches": true, + "limits": "This accepts the original simulated Location-settlement case at 9ad0b369, not the new WAL-error follow-up, native confinement, or the complete producer matrix." +} diff --git a/openspec/changes/complete-plugin-hook-runtime/evidence/store-commit-main-5d5826b7.json b/openspec/changes/complete-plugin-hook-runtime/evidence/store-commit-main-5d5826b7.json new file mode 100644 index 000000000..366143241 --- /dev/null +++ b/openspec/changes/complete-plugin-hook-runtime/evidence/store-commit-main-5d5826b7.json @@ -0,0 +1,14 @@ +{ + "main": "5d5826b72d41d6a833b523237bdbbd85ac51d31a", + "test": "writer::tests::committed_wal_error_stops_later_mutations", + "exit_code": 101, + "outcome": "named behavior assertion failed after a second connection verified committed data", + "log": "store-commit-main-5d5826b7.log", + "log_sha256": "10cfaf1f6c969585b768ee9013a711ea842f14b9181305cce40cc2cc5d49cd99", + "production_writer_sha256": "bad2bfd6e839535160f02c591f4508fc10caa7920340fb9b454a7bdc42e31931", + "overlays": [ + "crates/cyber-store/Cargo.toml: dev-only rusqlite hooks", + "crates/cyber-store/src/writer.rs: the new test only; production code unchanged" + ], + "proof": "The test first verifies value 7 from a second SQLite connection, then fails because the completed COMMIT error was not fatal." +} diff --git a/openspec/changes/complete-plugin-hook-runtime/evidence/store-commit-main-5d5826b7.log b/openspec/changes/complete-plugin-hook-runtime/evidence/store-commit-main-5d5826b7.log new file mode 100644 index 000000000..23981157c --- /dev/null +++ b/openspec/changes/complete-plugin-hook-runtime/evidence/store-commit-main-5d5826b7.log @@ -0,0 +1,20 @@ + Compiling cyber-core v0.1.0 (/Users/edvane/work/cybercode-file-observation-acceptance/crates/cyber-core) + Compiling cyber-store v0.1.0 (/Users/edvane/work/cybercode-file-observation-acceptance/crates/cyber-store) + Finished `test` profile [unoptimized + debuginfo] target(s) in 7.25s + Running unittests src/lib.rs (/Users/edvane/work/cybercode-cli/target/debug/deps/cyber_store-0a7f402bc991f0d9) + +running 1 test + +thread 'writer::tests::committed_wal_error_stops_later_mutations' (20790699) panicked at crates/cyber-store/src/writer.rs:198:9: +an unacknowledged completed transaction was not fatal: Err(Sqlite(SqliteFailure(Error { code: Unknown, extended_code: 1 }, Some("SQL logic error")))) +note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace +test writer::tests::committed_wal_error_stops_later_mutations ... FAILED + +failures: + +failures: + writer::tests::committed_wal_error_stops_later_mutations + +test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 9 filtered out; finished in 0.01s + +error: test failed, to rerun pass `-p cyber-store --lib` diff --git a/openspec/changes/complete-plugin-hook-runtime/specs/hooks/spec.md b/openspec/changes/complete-plugin-hook-runtime/specs/hooks/spec.md index 8a20a9178..0c48fd68f 100644 --- a/openspec/changes/complete-plugin-hook-runtime/specs/hooks/spec.md +++ b/openspec/changes/complete-plugin-hook-runtime/specs/hooks/spec.md @@ -26,6 +26,11 @@ - **WHEN** a Session successfully enters or exits a worktree, returns while idle, or is rebound to a different effective path - **THEN** `CwdChanged` uses the resulting Session Location and includes the previous/destination directories and durable transition ID, sequence and kind, after ownership locks settle - **AND** a refused or unknown transition, same-path rebind, no-op exit or reducer replay does not emit a successful path observation + +#### Scenario: Unacknowledged committed Location transition +- **WHEN** a Location transition persists but COMMIT reports an error after its transaction ended +- **THEN** no successful CwdChanged observation is emitted and later mutations remain fenced for recovery +- **AND** replay may recover the persisted Location and stable transition identity without dispatching the uncertain transition's handler - **AND** observational decisions or unavailable handlers do not veto the already committed transition or change its reported result #### Scenario: Durable file observation ownership - **WHEN** a new watcher generation claims a Location whose metadata and feedback allowance were already recorded diff --git a/openspec/changes/complete-plugin-hook-runtime/specs/storage-events/spec.md b/openspec/changes/complete-plugin-hook-runtime/specs/storage-events/spec.md new file mode 100644 index 000000000..10d5f33a3 --- /dev/null +++ b/openspec/changes/complete-plugin-hook-runtime/specs/storage-events/spec.md @@ -0,0 +1,18 @@ +## MODIFIED Requirements + +### Requirement: Durability boundary and storage failure +(P0) The server SHALL acknowledge admission and dispatch tool execution only after the associated transaction commits. File-backed SQLite SHALL use WAL and FULL synchronous on every writer connection, assuming a filesystem and device that honor synchronization. This guarantee SHALL cover acknowledged database facts, not unrecorded stream deltas or external side effects. In-memory mode SHALL report `durability: ephemeral`. Disk-full, I/O and corruption errors SHALL stop new mutations and tool dispatch with `StorageUnavailableError`; the system SHALL NOT silently replace the database or fall back to memory. When COMMIT reports an error after the transaction has ended, the writer SHALL preserve the uncertain outcome, return `StorageUnavailableError` and stop later mutations regardless of the SQLite error code. It SHALL NOT manufacture a success acknowledgement or blindly retry that transaction. A failed COMMIT that leaves the transaction active SHALL retain rollback and existing error classification. Known network filesystems SHALL be rejected for the live database; unsupported or undetectable filesystem guarantees SHALL be documented by doctor. + +#### Scenario: Failed admission commit +- **WHEN** the disk fills before a prompt transaction commits +- **THEN** the prompt receives no success receipt and no provider or tool work starts for that admission + +#### Scenario: Commit completed without a successful acknowledgement +- **WHEN** COMMIT returns an error with no active transaction remaining, including a WAL callback error after persistence +- **THEN** the writer reports an uncertain storage outcome and rejects later mutations until recovery rather than treating a generic error code as proof of rollback +- **AND** any persisted facts remain available to replay without manufacturing a successful live observation or replaying an external handler for a receipt + +#### Scenario: Rejected commit retains known rollback +- **WHEN** a deferred constraint rejects COMMIT while the transaction remains active +- **THEN** the failed transaction rolls back and retains its existing error classification +- **AND** a nonfatal rejection does not stop an otherwise healthy writer from committing later valid work diff --git a/openspec/changes/complete-plugin-hook-runtime/store-commit-uncertainty.md b/openspec/changes/complete-plugin-hook-runtime/store-commit-uncertainty.md new file mode 100644 index 000000000..18131ad85 --- /dev/null +++ b/openspec/changes/complete-plugin-hook-runtime/store-commit-uncertainty.md @@ -0,0 +1,73 @@ +# Unacknowledged Store commits and CwdChanged + +This fixes an observed durability-boundary gap while implementing task 5.3. It +does not close the full producer matrix or the plugin/native workstream. + +## Failure and main comparison + +SQLite's [WAL callback contract](https://www.sqlite.org/c3ref/wal_hook.html) +allows a callback error to propagate to COMMIT even though the transaction has +already committed. The test uses this documented boundary with `SQLITE_ERROR`; +it does not claim to reproduce a hardware failure or a historical CI stall. + +At main `5d5826b72d41d6a833b523237bdbbd85ac51d31a`, the new Store regression +`writer::tests::committed_wal_error_stops_later_mutations` fails its named fatal +outcome assertion. A second connection first verifies the inserted row, proving +that the transaction committed. The caller receives a generic SQLite error and +the writer's fatal flag is not set. The comparison runs actual main with only +the test and test-only rusqlite hook feature overlaid; its production writer +remains unchanged. The [receipt](evidence/store-commit-main-5d5826b7.json) and +[original failure log](evidence/store-commit-main-5d5826b7.log) retain that check. The prototype branch and all saved work are restored afterward. + +## Correction + +The writer issues COMMIT through the existing transaction's connection and +inspects `is_autocommit()` before dropping that transaction. Calling the consuming +`Transaction::commit()` first would allow its Drop rollback to erase the +distinction between an ended transaction and one still eligible for rollback. + +An error after the transaction ended has an uncertain outcome, so it becomes +`StorageUnavailableError` and follows the existing fatal-writer path. Queued and +later mutations cannot proceed. This does not claim that every ended transaction +committed: an error may also have caused automatic rollback. The caller lacks a +successful acknowledgement in either case and must recover rather than assume. + +If a rejected COMMIT leaves the transaction active, the existing transaction +Drop rolls it back and the original SQLite error classification remains. The +deferred-foreign-key regression proves that this known nonfatal rejection rolls +back its row while leaving the writer able to commit later valid work. Queue +admission, registered append validation and before-commit fault behavior are +unchanged. No schema or new public error type is introduced. + +## Runtime regression and verification + +`cwd_changed_lost_commit_ack_is_silent_and_replay_does_not_dispatch` uses the real +runtime, file-backed Store, reducers and lifecycle callback with a simulated +Location lease owner. It enters a worktree successfully, then installs a one-shot +WAL error on that Store's writer thread before idle exit. It verifies the callback +was reached, the durable exit and its identity exist despite the caller error, +the writer is fenced, no exit CwdChanged is emitted, and no repeated exit is +committed. A subsequent shell request is refused without invoking the host shell +method. A fresh runtime loads the committed destination without invoking a hook +or changing that exit identity. Both runtime instances settle their leases. + +The replay check uses the same failed Store; it is not a process-restart test. +Rusqlite's hook feature is enabled only in dev dependencies. Each fixture owns a +writer thread and a thread-local one-shot arm, so parallel tests do not share the +fault. Installing the callback performs no database writes and must not consume +the arm. Hook installation does not add a production fault-injection API. + +The Windows nextest profile selects the portable `writer::tests::` module as well +as the complete `location_activity` binary. This ensures the committed-WAL error, +known deferred rollback and runtime lost-ack case require actual Windows +execution; Linux/macOS already select the full workspace. Local nextest listing +checks selection only and is not native Windows acceptance. + +Locally on macOS, all 49 Store tests (including the process-kill recovery test) +and all 13 Location activity tests pass after the fix. Scoped Clippy for all Store +targets and the actual Location test target passes with warnings denied. Formatting, +strict OpenSpec (114/0) and spec lint (0 errors/25 warnings) pass. Those results do not claim +the new uncertain-commit case was tested through a process restart, on Windows +or Linux, or across the full local workspace. Local code/security review found no blocking issue. Hosted acceptance and final +PR review remain required before merging this follow-up. Native support/design +review gates, DirectoryAdded/Setup and task 5.3 remain open. diff --git a/openspec/changes/complete-plugin-hook-runtime/tasks.md b/openspec/changes/complete-plugin-hook-runtime/tasks.md index 0417ed790..54226995b 100644 --- a/openspec/changes/complete-plugin-hook-runtime/tasks.md +++ b/openspec/changes/complete-plugin-hook-runtime/tasks.md @@ -136,8 +136,11 @@ Location owner, not native OS confinement or an uncertain Store commit. The complete 12-case Location activity suite passes locally on macOS. A behavioral negative control that falls through after failed settlement executes this exact case and fails because it emitted CwdChanged; production source is restored. -Hosted platform acceptance and final review remain required. Task 5.3 stays open -for the complete producer matrix, including DirectoryAdded and Setup. +The original case is accepted by merged PR #89: the +[receipt](evidence/cwd-settlement-9ad0b369.json) verifies actual execution on +Linux/macOS/Windows at its original source. The new Store-commit follow-up below +requires its own current-head acceptance. Task 5.3 stays open for the complete +producer matrix, including DirectoryAdded and Setup. ## FileChanged acceptance after main integration @@ -149,3 +152,15 @@ reviewed composition with PR #88. See the [acceptance audit](file-observation-acceptance.md) and its final receipts for the scope, intentional skips and unverified Windows leaked-process warning. This closes only task 5.2; tasks 5.1, 5.3 and full workstream acceptance stay open. + + +## Store commit uncertainty correction + +Task 5.3's unknown-transition acceptance exposed a Store boundary gap: a real +SQLite WAL callback can report an error after the exit has persisted. The +[correction and main comparison](store-commit-uncertainty.md) record the named +failing regression on main, distinguish unknown completed COMMIT from a known +rollback, and verify that CwdChanged stays silent while reducer replay recovers +its committed identity. All 49 Store and 13 Location activity tests pass locally +on macOS. Hosted platform acceptance and final review remain required; task 5.3 +and the other native/producer/workstream obligations stay open.