diff --git a/CHANGELOG.md b/CHANGELOG.md index 9852563..99956fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,11 @@ Semantic Versioning after its first public release. ### Added +- Shared terminal owned-resource finalization now routes historical and matrix + runs, benchmarks, and guarded execution through completion-before-release, + exactly-once admission release, and fail-closed release-error precedence. + Deterministic coverage includes watchdog, journal, and cache-pin ordering; + native guard-exec qualification remains separate and NOT_RUN. - Matrix v2 `doctor` and `dry-run` now inspect every declared runtime using lexically ordered, digest-labeled reports. Doctor performs one bounded runtime probe per entry; dry-run renders each explicit workspace and command diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 7708251..46cdf25 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -87,6 +87,15 @@ executor, cache state, execution mode, target platform and optional requested CPU/memory ceilings. Persistence is advisory: failures produce a generic warning without changing process, admission, cancellation or receipt semantics. +The run, benchmark, and guarded-terminal paths share a private terminal +finalization primitive. It completes the owned workload result, joins any +applicable watchdog, and then attempts the admission release exactly once; +release failure overrides the primary result. This primitive does not change +the cache-pin lifetime: a managed-cache pin remains held for the guarded child +lifecycle and is released when that operation returns. Nor does it change the +source-snapshot lifecycle: a run cleans its snapshot after terminal admission +finalization and before sealing the receipt. + On macOS, a fresh strict sample from the absolute system tools is required after slot acquisition and before heavy work. `run` starts a two-second resource watchdog before local execution; `benchmark` has pre-start admission diff --git a/docs/COORDINATION_RUNBOOK.md b/docs/COORDINATION_RUNBOOK.md index 583e305..32adcac 100644 --- a/docs/COORDINATION_RUNBOOK.md +++ b/docs/COORDINATION_RUNBOOK.md @@ -11,6 +11,9 @@ OrbStack/Docker workloads or quarantining another activity's state. The local shell is not the ownership boundary. The CCP admission root and its OS locks are host-wide. A process list from one activity cannot prove that the host-wide slot is idle. +A child exit is not a slot-release handoff. A terminal handoff requires the +explicit terminal result plus fresh `admission status --json`, `docker ps -q`, +and `resource status --json` results before another activity proceeds. ## What is coordinated diff --git a/docs/LOCAL_RUN.md b/docs/LOCAL_RUN.md index 87a04c1..5dd7bc8 100644 --- a/docs/LOCAL_RUN.md +++ b/docs/LOCAL_RUN.md @@ -92,6 +92,8 @@ child runtime timeout. Both guard timeouts default to six hours, are capped at joins before slot release. Compressor occupancy alone never cancels an otherwise healthy in-progress run. `benchmark` has no mid-workload watchdog in this tranche. + The watchdog joins before admission release, and admission release is + attempted exactly once. A release failure overrides the primary result. 13. Execute the `run` checks or benchmark workload with timeout, cancellation, and stale-generation guards. 14. Mark cache entries complete only when every check passes. diff --git a/docs/TESTING_AND_FAULT_INJECTION.md b/docs/TESTING_AND_FAULT_INJECTION.md index b750854..32169d6 100644 --- a/docs/TESTING_AND_FAULT_INJECTION.md +++ b/docs/TESTING_AND_FAULT_INJECTION.md @@ -86,6 +86,13 @@ journal avoids the limitation by publishing immutable create-new events. Native crash/power-loss and Windows-host qualification remain separate gates. Deterministic source tests do not claim either result. +Deterministic fake-closure tests prove terminal ordering and precedence: the +completion step precedes exactly one release, and a release failure overrides +the primary result. Process-tree and Docker lifecycle tests prove their own +containment boundaries only; they do not prove a real admission root, host +cleanup, a published receipt, or another platform. Process lists do not prove +release. + ## Managed-cache pin contract The managed-cache pin tests are deterministic contract tests over an owned diff --git a/docs/superpowers/plans/2026-08-26-terminal-owned-resource-release.md b/docs/superpowers/plans/2026-08-26-terminal-owned-resource-release.md new file mode 100644 index 0000000..ddaccaa --- /dev/null +++ b/docs/superpowers/plans/2026-08-26-terminal-owned-resource-release.md @@ -0,0 +1,1083 @@ +# Terminal Owned-Resource Release Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Give `run`, matrix `run`, `benchmark`, and `guard exec` one deterministic terminal barrier that completes owned execution cleanup before exactly one admission-release attempt and fails closed when release is uncertain. + +**Architecture:** Add a private generic finalizer in a focused binary module, then keep command-specific classification and journaling in thin adapters in `src/main.rs`. The generic finalizer owns only ordering and precedence; existing admission, watchdog, journal, cache-pin, process-supervisor, Docker, snapshot, workspace, and receipt types retain their current ownership and schemas. + +**Tech Stack:** Rust 1.96.0, Cargo locked resolution, built-in unit and integration test harnesses, Clippy, rustfmt. + +**Spec:** `docs/superpowers/specs/2026-08-26-terminal-owned-resource-release-design.md` + +## Global Constraints + +- Work only in `/private/tmp/ccp-terminal-resource-release-v1` on branch `feat/terminal-resource-release-v1` based on `2b4b55ce1a4be0a2b610656ae4a56a7641b29f26`. +- Preserve `/Users/marco1/Documents/CODICE con VS CODE/commit-ci-preflight` and the public-documentation and static-analysis worktrees unchanged. +- Start every shell command with `rtk`. +- Use TDD: add the named failing test, observe the expected RED failure, implement the minimum production change, then observe GREEN. +- Use only deterministic fake closures or existing local fixtures for the new terminal contract. Do not invoke real admission coordination, child processes, Docker, network access, CCP heavy commands, or host resource probes in the new unit tests. +- Do not manually terminate processes, manipulate swap/compressor state, or edit/remove admission tickets, leases, locks, journals, cache ownership, or receipts. +- Preserve existing CLI exit codes, serialized schemas, resource-history behavior, receipt behavior, source-snapshot ordering, and cache-pin lifetime. +- Admission release failure overrides a completed primary result. If the subsequent `run` cleanup-pending journal transition also fails, `CliError::RunJournal` retains precedence and no release claim is allowed. +- `benchmark` retains pre-start admission only; this tranche does not add a mid-workload benchmark watchdog. +- Do not push, open a pull request, run CCP, publish evidence, merge, or clean branches/evidence. + +## File Structure + +- Create `src/terminal.rs`: private family-neutral `TerminalFailure` and `finalize_owned_terminal`, plus its deterministic unit tests. +- Modify `src/main.rs`: declare the private module; add thin benchmark and run adapters; route guard, historical run, and matrix run through the shared primitive; add adapter and cache-pin lifetime tests. +- Modify `docs/LOCAL_RUN.md`: operator-facing terminal order and benchmark exception. +- Modify `docs/COORDINATION_RUNBOOK.md`: exact criteria for claiming slot release and cleanup uncertainty. +- Modify `docs/ARCHITECTURE.md`: shared finalization architecture and ownership boundary. +- Modify `docs/TESTING_AND_FAULT_INJECTION.md`: deterministic fault matrix and separation from real process/runtime tests. + +--- + +### Task 1: Private family-neutral terminal primitive + +**Files:** +- Create: `src/terminal.rs` +- Modify: `src/main.rs:14-70` + +**Interfaces:** +- Consumes: a primary `Result`, one `FnOnce` completion closure, and one `FnOnce` release closure. +- Produces: `terminal::finalize_owned_terminal` and `terminal::TerminalFailure`, both visible only to the parent binary module. + +- [ ] **Step 1: Verify the exact implementation worktree before editing** + +Run: + +```bash +rtk git status --short --branch +rtk git rev-parse HEAD +rtk git rev-parse HEAD^ +rtk git rev-parse origin/main +``` + +Expected: clean `feat/terminal-resource-release-v1`, HEAD +is the reviewed plan commit supplied in the execution handoff, its first parent +is design commit `056ceea228178c6df5cadb1c843f16cfe7fd63d2`, and `origin/main` is +`2b4b55ce1a4be0a2b610656ae4a56a7641b29f26`. + +- [ ] **Step 2: Add the module declaration and failing primitive tests** + +Add this declaration after the standard header and before the imports in +`src/main.rs`: + +```rust +mod terminal; +``` + +Create `src/terminal.rs` with tests that name the not-yet-implemented API: + +```rust +#[cfg(test)] +mod tests { + use super::{TerminalFailure, finalize_owned_terminal}; + use std::cell::{Cell, RefCell}; + use std::rc::Rc; + + struct FakeGuard { + releases: Rc>, + released: bool, + } + + impl FakeGuard { + fn release(mut self) { + if !self.released { + self.releases.set(self.releases.get() + 1); + self.released = true; + } + } + } + + impl Drop for FakeGuard { + fn drop(&mut self) { + if !self.released { + self.releases.set(self.releases.get() + 1); + self.released = true; + } + } + } + + #[test] + fn completion_precedes_exactly_one_release() { + let events = RefCell::new(Vec::new()); + let releases = Cell::new(0); + + let result = finalize_owned_terminal( + Ok::<_, &'static str>(7_u8), + |primary| { + events.borrow_mut().push("complete"); + primary + }, + || { + events.borrow_mut().push("release"); + releases.set(releases.get() + 1); + Ok::<_, &'static str>(()) + }, + ); + + assert_eq!(result, Ok(7)); + assert_eq!(&*events.borrow(), &["complete", "release"]); + assert_eq!(releases.get(), 1); + } + + #[test] + fn primary_failure_survives_successful_release() { + let result = finalize_owned_terminal( + Err::<(), _>("workload"), + |primary| primary, + || Ok::<_, &'static str>(()), + ); + + assert_eq!(result, Err(TerminalFailure::Primary("workload"))); + } + + #[test] + fn release_failure_overrides_success_or_primary_failure() { + for primary in [Ok(()), Err("workload")] { + let result = finalize_owned_terminal( + primary, + |primary| primary, + || Err::<(), _>("release"), + ); + + assert_eq!(result, Err(TerminalFailure::Release("release"))); + } + } + + #[test] + fn completion_failure_still_releases_once() { + let releases = Cell::new(0); + let result = finalize_owned_terminal( + Ok::<_, &'static str>(()), + |_| Err("watchdog"), + || { + releases.set(releases.get() + 1); + Ok::<_, &'static str>(()) + }, + ); + + assert_eq!(result, Err(TerminalFailure::Primary("watchdog"))); + assert_eq!(releases.get(), 1); + } + + #[test] + fn explicit_release_consumes_guard_without_drop_release() { + let releases = Rc::new(Cell::new(0)); + let guard = FakeGuard { + releases: Rc::clone(&releases), + released: false, + }; + + let result = finalize_owned_terminal( + Ok::<_, &'static str>(()), + |primary| primary, + || { + guard.release(); + Ok::<_, &'static str>(()) + }, + ); + + assert_eq!(result, Ok(())); + assert_eq!(releases.get(), 1); + } +} +``` + +- [ ] **Step 3: Run the focused test module and confirm RED** + +Run: + +```bash +rtk cargo test --offline --locked --bin commit-ci-preflight terminal::tests:: +``` + +Expected: compilation fails because `TerminalFailure` and +`finalize_owned_terminal` do not yet exist. A failure caused by toolchain, +dependency, or filesystem access is inconclusive and must not be accepted as +the RED result. + +- [ ] **Step 4: Implement the minimum pure primitive above the tests** + +Add to `src/terminal.rs`: + +```rust +#[derive(Debug, PartialEq, Eq)] +pub(super) enum TerminalFailure { + Primary(P), + Release(R), +} + +pub(super) fn finalize_owned_terminal( + primary: Result, + complete_owned: impl FnOnce(Result) -> Result, + release: impl FnOnce() -> Result<(), R>, +) -> Result> { + let completed = complete_owned(primary); + match release() { + Ok(()) => completed.map_err(TerminalFailure::Primary), + Err(error) => Err(TerminalFailure::Release(error)), + } +} +``` + +- [ ] **Step 5: Run focused GREEN and formatting** + +Run: + +```bash +rtk cargo test --offline --locked --bin commit-ci-preflight terminal::tests:: +rtk cargo fmt --check +rtk git diff --check +``` + +Expected: five primitive tests pass; formatting and diff checks exit 0. + +- [ ] **Step 6: Commit the independently green primitive** + +Run: + +```bash +rtk git add src/main.rs src/terminal.rs +rtk git commit -m "refactor: add terminal finalization primitive" +``` + +Expected: one local commit containing only the private module and declaration. + +--- + +### Task 2: Benchmark and guard-exec adapters + +**Files:** +- Modify: `src/main.rs:532-574` +- Modify: `src/main.rs:681-751` +- Modify: `src/main.rs:1804-1810` +- Modify: `src/main.rs:1919-1944` +- Modify: `src/main.rs:2045-2071` +- Test: `src/main.rs:2551-3285` + +**Interfaces:** +- Consumes: `terminal::finalize_owned_terminal` and + `terminal::TerminalFailure` from Task 1. +- Produces: private `finalize_benchmark_terminal` and a + `finalize_guard_exec_result` adapter that delegates release precedence to the + shared primitive. + +- [ ] **Step 1: Import the shared primitive and add failing adapter tests** + +Add to the `src/main.rs` imports: + +```rust +use terminal::{TerminalFailure, finalize_owned_terminal}; +``` + +Add `finalize_benchmark_terminal` to the test module's `use super::{...}` list, +import `AdmissionError`, then add: + +```rust +#[test] +fn benchmark_terminal_preserves_primary_and_release_precedence() { + let releases = AtomicUsize::new(0); + let primary = finalize_benchmark_terminal(Err::<(), _>(CliError::Benchmark( + commit_ci_preflight::benchmark::BenchmarkError::NoSamples, + )), || { + releases.fetch_add(1, Ordering::SeqCst); + Ok(()) + }); + assert!(matches!(primary, Err(CliError::Benchmark(_)))); + + let release = finalize_benchmark_terminal(Ok(()), || { + releases.fetch_add(1, Ordering::SeqCst); + Err(AdmissionError::Clock) + }); + assert!(matches!(release, Err(CliError::Admission(AdmissionError::Clock)))); + assert_eq!(releases.load(Ordering::SeqCst), 2); +} +``` + +Extend `guard_exec_finalization_releases_once_for_success_error_and_resource_pressure` +with a release-failure case: + +```rust +let release_failure = finalize_guard_exec_result( + Err(GuardExecError::InternalFailure), + &cancellation, + None, + None, + || { + release_count.fetch_add(1, Ordering::SeqCst); + Err(GuardExecError::Admission(AdmissionError::Clock)) + }, +); +assert!(matches!( + release_failure, + Err(GuardExecError::Admission(AdmissionError::Clock)) +)); +assert_eq!(release_count.load(Ordering::SeqCst), 4); +``` + +Add a cache-pin test that calls the real guard adapter inside the existing pin +scope: + +```rust +#[test] +fn guard_cache_pin_remains_live_through_terminal_release() { + let (cache, source, base) = guard_cache_fixture("terminal-release"); + let cancellation = CancellationToken::default(); + + let result = super::with_guard_cache_pins( + Some(&cache), + std::slice::from_ref(&source), + || { + finalize_guard_exec_result( + Ok(completed_process_result()), + &cancellation, + None, + None, + || { + assert!(matches!( + cache.pin_completed_sources(std::slice::from_ref(&source)), + Err(CacheError::LockBusy(_)) + )); + Ok(()) + }, + ) + }, + ); + + assert!(result.is_ok()); + assert!( + cache + .pin_completed_sources(std::slice::from_ref(&source)) + .is_ok() + ); + cleanup_guard_cache_fixture(&base); +} +``` + +- [ ] **Step 2: Run the exact adapter tests and confirm RED** + +Run: + +```bash +rtk cargo test --offline --locked --bin commit-ci-preflight tests::benchmark_terminal_preserves_primary_and_release_precedence +rtk cargo test --offline --locked --bin commit-ci-preflight tests::guard_exec_finalization_releases_once_for_success_error_and_resource_pressure +rtk cargo test --offline --locked --bin commit-ci-preflight tests::guard_cache_pin_remains_live_through_terminal_release +``` + +Expected: the benchmark test fails to compile because +`finalize_benchmark_terminal` is missing. The extended guard and cache-pin +tests characterize behavior that may already pass once the compiler error is +resolved; they are required regression locks before refactoring. + +- [ ] **Step 3: Add the benchmark adapter and replace `release_admission`** + +Replace `release_admission` with: + +```rust +fn finalize_benchmark_terminal( + primary: Result, + release: impl FnOnce() -> Result<(), AdmissionError>, +) -> Result { + match finalize_owned_terminal(primary, std::convert::identity, release) { + Ok(value) => Ok(value), + Err(TerminalFailure::Primary(error)) => Err(error), + Err(TerminalFailure::Release(error)) => Err(CliError::Admission(error)), + } +} +``` + +Change `print_benchmark` to: + +```rust +let envelope = finalize_benchmark_terminal(result, || guard.release())?; +``` + +- [ ] **Step 4: Make guard classification the completion closure** + +Refactor `finalize_guard_exec_result` without changing its signature: + +```rust +fn finalize_guard_exec_result( + result: Result, + cancellation: &CancellationToken, + join_error: Option, + trip: Option, + release: impl FnOnce() -> Result<(), GuardExecError>, +) -> Result { + match finalize_owned_terminal( + result, + |result| { + if let Some(error) = join_error { + Err(GuardExecError::Resource(ResourceGuardError::Watchdog(error))) + } else if let Some(reason) = trip { + Err(GuardExecError::Resource( + ResourceGuardError::WatchdogTripped(reason), + )) + } else if cancellation.reason() == Some(CancellationReason::ResourcePressure) { + Err(GuardExecError::ResourcePressure) + } else { + result + } + }, + release, + ) { + Ok(value) => Ok(value), + Err(TerminalFailure::Primary(error) | TerminalFailure::Release(error)) => Err(error), + } +} +``` + +Keep `GuardExecSession::finish` as the sole watchdog-join owner: it joins once, +captures `trip` and `take_join_error()`, persists the resource observation, +takes the admission guard, then calls this adapter. The adapter's completion +closure only classifies the already-captured terminal state; it must not join +or read the barrier again. Keep the lexical cache-pin scope around +`session.finish` unchanged. + +- [ ] **Step 5: Run focused GREEN and existing guard regressions** + +Run: + +```bash +rtk cargo test --offline --locked --bin commit-ci-preflight tests::benchmark_terminal_preserves_primary_and_release_precedence +rtk cargo test --offline --locked --bin commit-ci-preflight tests::guard_exec_finalization_releases_once_for_success_error_and_resource_pressure +rtk cargo test --offline --locked --bin commit-ci-preflight tests::guard_cache_pin_remains_live_through_terminal_release +rtk cargo test --offline --locked --bin commit-ci-preflight tests::guard_cache_pins_ +rtk cargo test --offline --locked --test guard_exec_cli +rtk cargo fmt --check +rtk git diff --check +``` + +Expected: the named deterministic tests pass; `guard_exec_cli` compiles and +exits 0, but its native opt-in test remains ignored (0 passed, 1 ignored) and +is NOT_RUN, not PASS. No pin is released before the terminal release closure +returns. + +- [ ] **Step 6: Commit the benchmark and guard adapters** + +Run: + +```bash +rtk git add src/main.rs +rtk git commit -m "refactor: unify benchmark and guard finalization" +``` + +Expected: one local commit with no documentation or unrelated formatting. + +--- + +### Task 3: Historical and matrix run terminal adapter + +**Files:** +- Modify: `src/main.rs:1064-1149` +- Modify: `src/main.rs:1215-1297` +- Modify: `src/main.rs:1354-1417` +- Test: `src/main.rs:2551-3285` + +**Interfaces:** +- Consumes: `finalize_owned_terminal`, `TerminalFailure`, `CliError`, + `AdmissionError`, `RunFailureKindV1`, and `RunJournalStateV1`. +- Produces: private `RunTerminalJournalEvent` and + `finalize_run_terminal`. Both historical and matrix run call this adapter + after admission acquisition. + +- [ ] **Step 1: Add failing tests for run release and journal precedence** + +Add `finalize_run_terminal` and `RunTerminalJournalEvent` to the test module's +`use super::{...}` list, then add: + +```rust +#[test] +fn run_terminal_orders_watchdog_release_and_primary_journal() { + use std::cell::RefCell; + + let events = RefCell::new(Vec::new()); + let result = finalize_run_terminal( + Err::<(), _>(CliError::Resource(ResourceGuardError::PreStartDenied)), + |primary| { + events.borrow_mut().push("complete"); + primary + }, + || { + events.borrow_mut().push("release"); + Ok(()) + }, + |event| { + events.borrow_mut().push(match event { + RunTerminalJournalEvent::PrimaryFailure( + RunFailureKindV1::ResourcePressure, + ) => "journal-primary", + _ => "unexpected-journal", + }); + Ok(()) + }, + ); + + assert!(matches!(result, Err(CliError::Resource(_)))); + assert_eq!( + &*events.borrow(), + &["complete", "release", "journal-primary"] + ); +} + +#[test] +fn run_terminal_release_failure_journals_cleanup_pending() { + use std::cell::RefCell; + + let events = RefCell::new(Vec::new()); + let result = finalize_run_terminal( + Ok(()), + |primary| primary, + || Err(AdmissionError::Clock), + |event| { + events.borrow_mut().push(event); + Ok(()) + }, + ); + + assert!(matches!(result, Err(CliError::Admission(AdmissionError::Clock)))); + assert_eq!( + &*events.borrow(), + &[RunTerminalJournalEvent::ReleaseFailure] + ); +} + +#[test] +fn run_terminal_journal_failure_overrides_release_failure() { + let result = finalize_run_terminal( + Ok(()), + |primary| primary, + || Err(AdmissionError::Clock), + |_| Err(CliError::RunJournal(RunJournalError::InvalidTransition)), + ); + + assert!(matches!( + result, + Err(CliError::RunJournal(RunJournalError::InvalidTransition)) + )); +} +``` + +Import `AdmissionError`, `ResourceGuardError`, `RunFailureKindV1`, and +`RunJournalError` into the test module from their existing crate paths. + +- [ ] **Step 2: Run the exact run-adapter tests and confirm RED** + +Run: + +```bash +rtk cargo test --offline --locked --bin commit-ci-preflight tests::run_terminal_ +``` + +Expected: compilation fails because `finalize_run_terminal` and +`RunTerminalJournalEvent` do not yet exist. + +- [ ] **Step 3: Implement the run adapter** + +Add near the current finalization helpers: + +```rust +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum RunTerminalJournalEvent { + PrimaryFailure(RunFailureKindV1), + ReleaseFailure, +} + +fn finalize_run_terminal( + primary: Result, + complete_owned: impl FnOnce(Result) -> Result, + release: impl FnOnce() -> Result<(), AdmissionError>, + mut journal: impl FnMut(RunTerminalJournalEvent) -> Result<(), CliError>, +) -> Result { + match finalize_owned_terminal(primary, complete_owned, release) { + Ok(value) => Ok(value), + Err(TerminalFailure::Primary(error)) => { + journal(RunTerminalJournalEvent::PrimaryFailure(cli_failure_kind( + &error, + )))?; + Err(error) + } + Err(TerminalFailure::Release(error)) => { + journal(RunTerminalJournalEvent::ReleaseFailure)?; + Err(CliError::Admission(error)) + } + } +} +``` + +Add one production mapping closure at each run call site: + +```rust +|event| match event { + RunTerminalJournalEvent::PrimaryFailure(kind) => lifecycle.fail(kind), + RunTerminalJournalEvent::ReleaseFailure => { + lifecycle.transition_state(RunJournalStateV1::CleanupPending, None) + } +} +``` + +- [ ] **Step 4: Route historical run through the adapter** + +For resource pre-start failure and current-directory failure after admission, +call `finalize_run_terminal` with identity completion, `|| guard.release()`, +and the production journal closure. Do not rely on `AdmissionGuard::Drop` for +these normal error paths. + +Use this exact shape for the resource pre-start branch: + +```rust +if let Err(error) = resource_pre_start(supervisor.clone(), &cancellation) { + return finalize_run_terminal( + Err::<(), _>(error), + std::convert::identity, + || guard.release(), + |event| match event { + RunTerminalJournalEvent::PrimaryFailure(kind) => lifecycle.fail(kind), + RunTerminalJournalEvent::ReleaseFailure => { + lifecycle.transition_state(RunJournalStateV1::CleanupPending, None) + } + }, + ); +} +``` + +Use this exact early-return shape if `current_dir` cannot be resolved after +admission: + +```rust +let current_dir = match std::env::current_dir() { + Ok(path) => path, + Err(error) => { + return finalize_run_terminal( + Err::<(), _>(CliError::internal(error)), + std::convert::identity, + || guard.release(), + |event| match event { + RunTerminalJournalEvent::PrimaryFailure(kind) => lifecycle.fail(kind), + RunTerminalJournalEvent::ReleaseFailure => lifecycle + .transition_state(RunJournalStateV1::CleanupPending, None), + }, + ); + } +}; +``` + +For the executed historical run, replace the explicit join/reconcile/release +block with: + +```rust +let outcome = finalize_run_terminal( + run_result.map_err(CliError::Run), + |outcome| { + completion_barrier.ensure_joined(); + reconcile_watchdog_outcome(outcome, &mut completion_barrier) + }, + || guard.release(), + |event| match event { + RunTerminalJournalEvent::PrimaryFailure(kind) => lifecycle.fail(kind), + RunTerminalJournalEvent::ReleaseFailure => { + lifecycle.transition_state(RunJournalStateV1::CleanupPending, None) + } + }, +)?; +``` + +Leave successful source-snapshot cleanup, finalization, receipt writing, and +sealing after this block in their existing order. + +Immediately compile the historical path before editing matrix run: + +```bash +rtk cargo check --offline --locked --bin commit-ci-preflight +rtk cargo test --offline --locked --bin commit-ci-preflight tests::run_terminal_ +rtk cargo test --offline --locked --bin commit-ci-preflight tests::watchdog_barrier_joins_once_after_early_run_error +``` + +Expected: the binary compiles and the historical terminal/watchdog tests pass; +this independently validates that the early-return closures consume `guard` +only on paths that return from the enclosing function. + +- [ ] **Step 5: Route matrix run through the adapter** + +For matrix resource pre-start and current-directory failures after admission, +use the same identity-completion adapter path and exact early-return snippets +from Step 4. The enclosing function also returns `Result<(), CliError>`, so the +annotated `Err::<(), _>` remains the correct type. Do not leave the current +post-admission `std::env::current_dir().map_err(CliError::internal)?` path in +place. + +Replace the explicit matrix join/result/release block with: + +```rust +let outcome = finalize_run_terminal( + result, + |result| { + completion_barrier.ensure_joined(); + if let Some(error) = completion_barrier.take_join_error() { + Err(CliError::Resource(ResourceGuardError::Watchdog(error))) + } else { + result + } + }, + || guard.release(), + |event| match event { + RunTerminalJournalEvent::PrimaryFailure(kind) => lifecycle.fail(kind), + RunTerminalJournalEvent::ReleaseFailure => { + lifecycle.transition_state(RunJournalStateV1::CleanupPending, None) + } + }, +)?; +``` + +Preserve matrix execution-barrier trip semantics and the existing finalizing +and sealed transitions. + +Immediately compile the matrix path before adding the cross-family tests: + +```bash +rtk cargo check --offline --locked --bin commit-ci-preflight +rtk cargo test --offline --locked --test runtime_cli +rtk cargo test --offline --locked --test matrix_contract +``` + +Expected: the binary compiles and the existing matrix CLI/contract tests pass. + +- [ ] **Step 6: Add one cross-family regression test** + +Add this test; it supplements rather than replaces the primitive's event-order +test: + +```rust +#[test] +fn all_heavy_family_adapters_release_once_and_fail_closed() { + let releases = AtomicUsize::new(0); + + let benchmark = finalize_benchmark_terminal(Ok(()), || { + releases.fetch_add(1, Ordering::SeqCst); + Err(AdmissionError::Clock) + }); + assert!(matches!( + benchmark, + Err(CliError::Admission(AdmissionError::Clock)) + )); + + let run = finalize_run_terminal( + Ok(()), + std::convert::identity, + || { + releases.fetch_add(1, Ordering::SeqCst); + Err(AdmissionError::Clock) + }, + |_| Ok(()), + ); + assert!(matches!( + run, + Err(CliError::Admission(AdmissionError::Clock)) + )); + + let cancellation = CancellationToken::default(); + let guard = finalize_guard_exec_result( + Ok(completed_process_result()), + &cancellation, + None, + None, + || { + releases.fetch_add(1, Ordering::SeqCst); + Err(GuardExecError::Admission(AdmissionError::Clock)) + }, + ); + assert!(matches!( + guard, + Err(GuardExecError::Admission(AdmissionError::Clock)) + )); + + assert_eq!(releases.load(Ordering::SeqCst), 3); +} +``` + +Also add an opaque-success preservation test for the two adapters whose +timeout/cancellation/resource state is already encoded inside their successful +domain outcome before terminal release: + +```rust +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum OpaqueTerminalOutcome { + TimedOut, + UserCancelled, + ResourcePressure, +} + +#[test] +fn benchmark_and_run_preserve_opaque_terminal_outcomes() { + for expected in [ + OpaqueTerminalOutcome::TimedOut, + OpaqueTerminalOutcome::UserCancelled, + OpaqueTerminalOutcome::ResourcePressure, + ] { + assert_eq!( + finalize_benchmark_terminal(Ok(expected), || Ok(())) + .expect("benchmark terminal outcome"), + expected + ); + assert_eq!( + finalize_run_terminal( + Ok(expected), + std::convert::identity, + || Ok(()), + |_| Ok(()), + ) + .expect("run terminal outcome"), + expected + ); + } +} +``` + +For `guard exec`, extend the existing finalizer test with +`GuardExecError::ChildExit(7)`, `GuardExecError::TimedOut`, and +`GuardExecError::UserCancelled` cases, each with a successful release closure, +and assert each exact exit classification survives: + +```rust +for (primary, expected_exit) in [ + (GuardExecError::ChildExit(7), 7), + (GuardExecError::TimedOut, 124), + (GuardExecError::UserCancelled, 130), +] { + let result = finalize_guard_exec_result( + Err(primary), + &CancellationToken::default(), + None, + None, + || Ok(()), + ); + assert_eq!(result.expect_err("guard primary failure").exit_code(), expected_exit); +} +``` + +Keep the existing resource-pressure test and +`watchdog_barrier_joins_once_after_early_run_error` as the actual resource and +watchdog classification coverage. + +- [ ] **Step 7: Run focused GREEN and existing run/watchdog regressions** + +Run: + +```bash +rtk cargo test --offline --locked --bin commit-ci-preflight tests::run_terminal_ +rtk cargo test --offline --locked --bin commit-ci-preflight tests::all_heavy_family_adapters_release_once_and_fail_closed +rtk cargo test --offline --locked --bin commit-ci-preflight tests::benchmark_and_run_preserve_opaque_terminal_outcomes +rtk cargo test --offline --locked --bin commit-ci-preflight tests::watchdog_barrier_joins_once_after_early_run_error +rtk cargo test --offline --locked --lib run::tests:: +rtk cargo test --offline --locked --lib resource::tests::watchdog_ +rtk cargo test --offline --locked --test runtime_cli +rtk cargo test --offline --locked --test matrix_contract +rtk cargo test --offline --locked --test recover_cli +rtk cargo fmt --check +rtk git diff --check +``` + +Expected: all focused and existing regression tests pass. No command starts a +real CCP run or Docker container. + +- [ ] **Step 8: Commit the run adapters** + +Run: + +```bash +rtk git add src/main.rs +rtk git commit -m "refactor: unify run terminal resource release" +``` + +Expected: one local commit containing historical run, matrix run, adapter, and +their unit tests only. + +--- + +### Task 4: Operator documentation + +**Files:** +- Modify: `docs/LOCAL_RUN.md:84-117` +- Modify: `docs/COORDINATION_RUNBOOK.md:15-38,162-178` +- Modify: `docs/ARCHITECTURE.md:65-95` +- Modify: `docs/TESTING_AND_FAULT_INJECTION.md:1-110` + +**Interfaces:** +- Consumes: the exact terminal ordering and failure behavior implemented in + Tasks 1-3. +- Produces: a discoverable operator contract that cannot silently claim a slot + release from child exit, `ps`, or an uncertain release result. + +- [ ] **Step 1: Add bounded, consistent documentation** + +Add the following exact claims once in the named canonical documents, with +surrounding prose that preserves existing command-family distinctions: + +- `docs/LOCAL_RUN.md`: “The watchdog joins before admission release, and + admission release is attempted exactly once. A release failure overrides + the primary result.” Also state: “benchmark has no mid-workload watchdog.” +- `docs/COORDINATION_RUNBOOK.md`: “A child exit is not a slot-release handoff.” + Require explicit terminal result plus fresh admission/runtime/resource status + before handoff. +- `docs/ARCHITECTURE.md`: describe the shared private terminal primitive and + state “release failure overrides the primary result.” Preserve cache-pin and + source-snapshot lifecycle exceptions. +- `docs/TESTING_AND_FAULT_INJECTION.md`: state that deterministic fake closure + tests prove ordering and precedence, while process-tree and Docker lifecycle + tests prove their own containment boundaries. Include the exact phrase + “process lists do not prove release.” + +Do not claim that unit tests prove a real admission root, Docker cleanup, a +published receipt, or another platform. + +- [ ] **Step 2: Review the semantic documentation diff** + +Read the complete four-document diff and verify each claim against the exact +implemented call-site ordering from Tasks 1-3. Confirm that the prose +distinguishes facts, operator requirements, and non-claims; does not describe a +unit test as host qualification; and does not broaden CCP ownership to swap, +unrelated processes, foreign containers, or undeclared paths. + +- [ ] **Step 3: Run related existing contracts** + +Run: + +```bash +rtk cargo test --offline --locked --test cache_pin_contract +rtk cargo test --offline --locked --test repository_hygiene_contract +rtk cargo test --offline --locked --test release_hardening_contract +rtk git diff --check +``` + +Expected: all three existing contract suites pass with no broken ownership or public +evidence claims. + +- [ ] **Step 4: Commit documentation** + +Run: + +```bash +rtk git add docs/LOCAL_RUN.md docs/COORDINATION_RUNBOOK.md docs/ARCHITECTURE.md docs/TESTING_AND_FAULT_INJECTION.md +rtk git commit -m "docs: define terminal resource release evidence" +``` + +Expected: one local documentation commit. + +--- + +### Task 5: Exact-head verification and review package + +**Files:** +- Modify only if a verification failure proves a scoped defect in Tasks 1-4. +- Record: `/private/tmp/ccp-terminal-resource-release-verification.md` + +**Interfaces:** +- Consumes: all locally committed deliverables from Tasks 1-4. +- Produces: exact-head local verification evidence and a bounded review package; + it does not produce a CCP receipt or remote evidence. + +- [ ] **Step 1: Confirm exact local scope before verification** + +Run: + +```bash +rtk git status --short --branch +rtk git rev-parse HEAD +rtk git log --oneline origin/main..HEAD +rtk git diff --stat origin/main...HEAD +rtk git diff --check origin/main...HEAD +``` + +Expected: clean branch; only the approved spec, plan, private primitive, +terminal adapters/tests, and four bounded operator documents differ from +`origin/main`. + +- [ ] **Step 2: Run formatting and strict static checks** + +Run: + +```bash +rtk cargo fmt --check +rtk cargo clippy --offline --locked --all-targets --all-features -- -D warnings +``` + +Expected: both exit 0 under the pinned Rust 1.96.0 toolchain. Preserve complete +diagnostics for any failure; do not weaken `-D warnings`. + +- [ ] **Step 3: Run focused terminal, guard, run, journal, and runtime tests** + +Run: + +```bash +rtk cargo test --offline --locked --bin commit-ci-preflight terminal::tests:: +rtk cargo test --offline --locked --bin commit-ci-preflight tests::run_terminal_ +rtk cargo test --offline --locked --bin commit-ci-preflight tests::all_heavy_family_adapters_release_once_and_fail_closed +rtk cargo test --offline --locked --bin commit-ci-preflight tests::guard_cache_pin_remains_live_through_terminal_release +rtk cargo test --offline --locked --bin commit-ci-preflight tests::watchdog_barrier_joins_once_after_early_run_error +rtk cargo test --offline --locked --lib run::tests:: +rtk cargo test --offline --locked --lib resource::tests::watchdog_ +rtk cargo test --offline --locked --test guard_exec_cli +rtk cargo test --offline --locked --test process_supervisor +rtk cargo test --offline --locked --test recover_cli +rtk cargo test --offline --locked --test runtime_cli +rtk cargo test --offline --locked --test matrix_contract +rtk cargo test --offline --locked --test cache_pin_contract +``` + +Expected: all named deterministic suites pass. `guard_exec_cli` compiles and +exits 0, but its native opt-in test remains ignored (0 passed, 1 ignored) and +is NOT_RUN, not PASS; this does not block deterministic task closure. +`process_supervisor` may start its existing local fixture processes, but no +test may acquire admission or invoke CCP/Docker. + +- [ ] **Step 4: Run the complete native suite and doctests** + +Run: + +```bash +rtk cargo test --offline --locked --all-targets --all-features +rtk cargo test --offline --locked --doc +``` + +Expected: terminal PASS for the complete native suite and doctests. Record the +exact passed/ignored/failed counts; do not infer them from an interrupted or +truncated command. + +- [ ] **Step 5: Write the local verification report** + +Create `/private/tmp/ccp-terminal-resource-release-verification.md` containing: + +- repository and worktree absolute path; +- branch, exact HEAD, exact `origin/main`, and clean/dirty state; +- ordered local commits; +- each exact command, exit code, and terminal test count; +- the distinction between deterministic fake terminal evidence and existing + process/runtime integration evidence; +- confirmation that no CCP run, Docker workload, network action, push, PR, + evidence publication, or merge occurred; +- any remaining unknowns or limitations. + +- [ ] **Step 6: Request exact-head code review and fix only proven findings** + +Give the reviewer the design, plan, `origin/main...HEAD` diff, verification +report, and exact head. Require findings by severity with file/line anchors. +For a proven finding, return to the smallest affected RED test, implement the +minimum fix, rerun the affected focused suite and full required gates, then +commit a scoped fix. Do not make speculative cleanup changes. + +- [ ] **Step 7: Freeze the local handoff state** + +Run: + +```bash +rtk git status --short --branch +rtk git rev-parse HEAD +rtk shasum -a 256 /private/tmp/ccp-terminal-resource-release-verification.md +``` + +Expected: clean local branch and a hash-bound verification report. Stop before +push, PR, CCP, evidence publication, merge, or branch cleanup. diff --git a/docs/superpowers/specs/2026-08-26-terminal-owned-resource-release-design.md b/docs/superpowers/specs/2026-08-26-terminal-owned-resource-release-design.md new file mode 100644 index 0000000..98bea7e --- /dev/null +++ b/docs/superpowers/specs/2026-08-26-terminal-owned-resource-release-design.md @@ -0,0 +1,378 @@ +# Terminal owned-resource release design + +Status: approved by owner on 2026-08-26 +Date: 2026-08-26 +Baseline: `2b4b55ce1a4be0a2b610656ae4a56a7641b29f26` +Scope: make terminal cleanup and admission release ordering explicit and +uniform across `run`, `benchmark`, and `guard exec` + +## Decision + +CCP will add one private, family-neutral terminal finalization primitive and +three thin command-family adapters. + +The shared primitive will enforce this order after admission has been +acquired: + +1. finish the command family's owned workload and containment cleanup; +2. complete any owned terminal barrier, including watchdog join; +3. release admission exactly once; +4. return the primary outcome when release succeeds; +5. return the release failure when release fails, even if the primary outcome + was already a failure. + +The primitive will be pure and closure-driven. Tests can therefore prove the +ordering, release count, and failure precedence without admission state, +Docker, child processes, resource probes, network access, or host mutation. + +The change consolidates existing behavior. It does not create a new process +manager, Docker cleanup mechanism, admission authority, or recovery path. + +## Problem and evidence + +The three heavy command families already release admission after their work, +but they encode the terminal contract separately. + +- `benchmark` acquires admission and calls `release_admission` after its + resource pre-start check and native workload (`src/main.rs:539-555`). It has + no mid-workload resource watchdog. +- historical `run` joins and reconciles its watchdog before releasing + admission (`src/main.rs:1110-1142`). Its pre-start and current-directory + failures release through separate inline branches (`src/main.rs:1077-1097`). +- matrix `run` repeats the same terminal structure around + `src/main.rs:1215-1290`. +- `guard exec` owns an admission guard, watchdog barrier, and optional resource + observation in `GuardExecSession` (`src/main.rs:1812-1816`). Its explicit + `finish` path joins the watchdog, records the observation, takes the guard, + and releases it (`src/main.rs:1919-1944`). +- `finalize_guard_exec_result` already provides a pure release closure and + makes release failure override watchdog or child outcomes + (`src/main.rs:2045-2071`). +- `GuardExecSession::Drop`, `WatchdogCompletionBarrier::Drop`, and + `AdmissionGuard::Drop` are best-effort safety fallbacks. They are not + evidence-producing terminal paths (`src/main.rs:2073-2141` and + `src/admission.rs:1093-1096`). + +This duplication makes a later edit capable of releasing the host-wide slot +before a watchdog has joined, releasing twice on a fallback path, or applying +different failure precedence to different command families. + +The defect is contract drift risk, not evidence of a currently leaked slot. +The design must not reinterpret a historical journal, ticket, lease, or +receipt. + +## Goals + +- Give `run`, matrix `run`, `benchmark`, and `guard exec` one explicit terminal + ordering contract after admission acquisition. +- Prove that a watchdog or other terminal barrier completes before admission + release. +- Prove that explicit admission release occurs exactly once on every covered + terminal outcome. +- Preserve fail-closed precedence: a release failure overrides a successful or + failed primary result because slot ownership is then uncertain. +- Cover success, workload or child failure, timeout, user cancellation, + resource-pressure cancellation or watchdog trip, watchdog join failure, and + release failure with deterministic tests. +- Preserve existing command-specific exit codes, journal transitions, resource + history, receipts, and output schemas. + +## Non-goals + +- No real CCP heavy command, Docker invocation, child process, network request, + or host resource probe is part of the unit contract. +- No manual process termination, swap manipulation, compressor manipulation, + admission-root editing, lock removal, ticket removal, lease rewriting, + journal rewriting, or receipt rewriting. +- No attempt to supervise processes or containers that the current CCP + lifecycle did not create. +- No change to admission policy thresholds, queue ordering, heartbeat format, + resource history schema, receipt schema, or public exit-code assignments. +- No new watchdog for the native `benchmark` loop in this tranche. +- No promise that an interrupted process can always publish terminal evidence; + RAII remains a best-effort fallback for unwinding and process-local exits. + +## Ownership boundary + +The terminal barrier covers only active execution resources owned by the +current CCP command: + +- its admission ticket, slot, lease, heartbeat, and related locks; +- its supervised child process tree; +- its watchdog thread and resource observation; +- Docker containers created by that command's `DockerLifecyclePlan`; + +Managed-cache pins, source snapshots, and command-local workspaces remain +CCP-owned, but keep their existing independent RAII and lifecycle ordering. +In particular, `guard exec` cache pins remain held through admission release, +and successful `run` source-snapshot cleanup continues after admission release. +This tranche must not silently reorder either lifecycle. + +It does not cover: + +- unrelated host processes; +- swap, compressor, or other operating-system-wide memory state; +- Docker or OrbStack containers not created by the current lifecycle; +- undeclared paths embedded in opaque `guard exec` child arguments; +- a ticket or lease owned by another process merely because that owner is not + visible in a local `ps` sample. + +Ownership is determined by CCP state and the current lifecycle, not by process +name matching. + +## Alternatives considered + +### A. Keep the three implementations separate and add only more tests + +Rejected. Tests would document the current duplication but would not prevent +future semantic drift between command families. + +### B. Build a new RAII session abstraction for every heavy command + +Rejected for this tranche. A broad ownership rewrite would touch admission, +journaling, process supervision, runtime cleanup, and receipt publication at +once. The existing types already own these resources correctly; the missing +piece is one terminal ordering seam. + +### C. One pure finalizer with thin family adapters + +Selected. It reuses current ownership types, makes the failure contract +testable without the host, and limits production edits to terminal call sites. + +## Shared primitive + +The implementation will introduce a private generic result that distinguishes +a primary command failure from an admission release failure. Its exact Rust +names may be refined during the TDD plan, but its semantics are fixed: + +```rust +enum TerminalFailure { + Primary(P), + Release(R), +} + +fn finalize_owned_terminal( + primary: Result, + complete_owned: impl FnOnce(Result) -> Result, + release: impl FnOnce() -> Result<(), R>, +) -> Result>; +``` + +`complete_owned` runs exactly once before `release`. It is the family adapter's +terminal barrier: + +- for historical `run`, it joins the watchdog and reconciles its join error or + trip with the run outcome; +- for matrix `run`, it preserves the execution barrier's existing trip + semantics, joins the watchdog, and promotes any join error over the matrix + outcome; +- for `guard exec`, it joins the watchdog, preserves resource-observation + ordering, and applies the existing watchdog, cancellation, and child-result + classification; +- for `benchmark`, it is an identity completion because the current native + workload has no watchdog. + +`release` runs exactly once after completion. If it succeeds, the completed +primary result is returned. If it fails, `TerminalFailure::Release` is +returned regardless of the primary result. + +Using distinct primary and release error types lets each command adapter retain +its current error mapping and side effects. In particular, `run` can mark its +journal `cleanup-pending` for a release failure in the shared terminal adapter +and can record the normal failure kind only after successful release. Existing +later cleanup paths may independently use `cleanup-pending`. + +The primitive does not own or drop the real admission guard. Each adapter moves +its guard into the one-shot release closure. Rust ownership and `FnOnce` +prevent a second explicit release from the same terminal path. + +## Command-family adapters + +### `benchmark` + +After admission acquisition, every primary result passes through the shared +primitive. The adapter uses identity completion and maps a release failure to +`CliError::Admission`. Existing output and optional receipt writing remain +after successful finalization. + +Admission acquisition failure remains outside the finalizer because no guard +is owned. + +### historical and matrix `run` + +Every path after successful admission acquisition passes through the same run +adapter, including resource pre-start failure and failures that occur before a +watchdog is created. A barrier with no watchdog is a valid no-op completion. + +The adapter performs this sequence: + +1. ensure the completion barrier has joined at most once; +2. apply the path's existing watchdog semantics: historical `run` reconciles + join error, trip, and resource-pressure evidence; matrix `run` preserves the + barrier's trip result and promotes a join error over the matrix outcome; +3. release the admission guard once; +4. on release failure, transition the journal to `cleanup-pending` and return + `CliError::Admission`; +5. on primary failure after successful release, record the existing terminal + failure kind and return that primary error; +6. on success, continue existing source-snapshot cleanup, sealing, and receipt + behavior. + +Historical and matrix paths may share one adapter if their lifecycle types and +borrow rules permit it. If not, both adapters must delegate the ordering and +precedence to the same primitive; they must not duplicate the primitive. + +The cleanup-pending transition is itself fallible and remains outside the pure +primitive. If admission release fails, the adapter first attempts that journal +transition. A transition failure retains the existing `CliError::RunJournal` +precedence because the durable terminal record is then also uncertain; +otherwise the adapter returns the admission release failure. Neither case may +claim that the slot was released. This rule applies uniformly to resource +pre-start, current-directory, workload, watchdog, and other failures after +admission acquisition. + +### `guard exec` + +`GuardExecSession::finish` remains the evidence-producing explicit terminal +path. Resource observation must still be persisted after watchdog join and +before admission release. + +The current `finalize_guard_exec_result` classification becomes the guard +adapter's completion logic and delegates release precedence to the shared +primitive. The `Option::take` guard ownership remains, so `Drop` sees no guard +after explicit completion and cannot release it again. + +Early managed-cache, resource pre-start, pin acquisition, and child execution +failures continue to call `finish`. Cache pins remain alive through explicit +session finish and admission release. + +The guard adapter does not move pin ownership into the shared primitive. A +focused adapter test must retain an instrumented pin owner around finalization +and prove that it drops only after the release closure has run. + +## Terminal ordering + +The normative order for the active terminal barrier of an admitted command is: + +```text +owned workload terminates + -> child/container containment cleanup completes or fails + -> watchdog joins and terminal observation is captured + -> admission release is attempted exactly once + -> command-specific journal/exit/receipt handling continues +``` + +A command must not report the slot as released merely because its child has +exited or because no matching process appears in `ps`. + +Docker lifecycle cleanup remains inside the runtime/process primary outcome. +If Docker cleanup fails, that uncertainty becomes the primary failure before +the terminal finalizer runs. If admission release then also fails, the release +failure wins because host-wide ownership is still uncertain. + +Auxiliary RAII resources that intentionally outlive slot release are not part +of this ordering. Existing cache-pin, source-snapshot, workspace, and receipt +lifecycles remain unchanged. + +## Failure precedence + +The following table is normative: + +| Completed primary outcome | Admission release | Final outcome | +| --- | --- | --- | +| success | success | success | +| workload or child failure | success | original failure | +| timeout | success | timeout | +| user cancellation | success | cancellation | +| resource-pressure cancellation or trip | success | resource failure | +| watchdog join failure | success | watchdog failure | +| any of the above | failure | admission release failure | + +Within the completed primary outcome, existing family-specific precedence is +preserved. For `guard exec`, watchdog join error precedes trip, resource +pressure, and child outcome. For `run`, the existing reconciliation rules for +a receipt already containing resource-pressure `NOT_RUN` evidence remain +unchanged. + +## Fallback behavior + +RAII `Drop` implementations remain safety fallbacks: + +- watchdog drop joins at most once; +- session or admission-guard drop attempts best-effort release only while it + still owns the guard; +- explicit finalization consumes or takes the guard, so subsequent drop cannot + perform a second release. + +A suppressed error from `Drop` is not terminal proof. Normal command paths +must use explicit finalization so cleanup failure can be returned and, for +`run`, journaled as `cleanup-pending`. + +## Deterministic TDD contract + +Tests will be serial and use fake closures or small fake adapters only. An +event trace will assert `complete` occurs before `release`, and an atomic or +cell counter will assert release is called exactly once. + +The table-driven contract must cover each adapter for: + +- success; +- workload or child failure; +- timeout; +- user cancellation; +- resource-pressure cancellation or watchdog trip; +- watchdog join failure where the family has a watchdog; +- release failure overriding every representative primary outcome; +- explicit finalization followed by drop without a second release; +- for `guard exec`, cache-pin ownership remaining live through completion and + admission release. + +Existing process-tree and Docker lifecycle tests remain the source of truth for +real child timeout containment and container cleanup. This tranche will not add +a fake Docker implementation to the shared terminal primitive because Docker +cleanup must finish before the primitive is entered. + +At least one cross-family contract test must exercise the same shared +primitive through all three adapters. Tests that independently duplicate the +expected ordering in three helpers are insufficient. + +## Documentation changes + +After tests and implementation are green, bounded updates will be made to: + +- `docs/LOCAL_RUN.md`: terminal order, benchmark watchdog exception, and + release-failure precedence; +- `docs/COORDINATION_RUNBOOK.md`: a terminal child result is not a release + handoff; release uncertainty remains fail-closed; +- `docs/ARCHITECTURE.md`: shared terminal finalization contract and ownership + boundary; +- `docs/TESTING_AND_FAULT_INJECTION.md`: deterministic fault matrix and the + separation between fake terminal tests and real process/runtime tests. + +No public documentation may claim qualification beyond the exact tests that +were executed. + +## Verification boundary + +The implementation tranche will require, at minimum: + +1. focused RED/GREEN tests for the shared primitive and all adapters; +2. existing guard-exec, admission, process-supervisor, run-journal, runtime, + and repository-hygiene tests relevant to the touched paths; +3. `cargo fmt --check`; +4. strict Clippy with the repository-pinned toolchain and all targets/features; +5. the complete native test suite and doctests; +6. `git diff --check` and an exact-head scoped review. + +These are local source checks. They do not themselves authorize or constitute +a CCP `run`, Docker workload, evidence publication, push, pull request, or +merge. + +## Stop boundaries + +The written design must be reviewed and locally committed before the TDD +implementation plan is written. The implementation plan must then be reviewed +before production code changes begin. + +No push, pull request, CCP heavy command, Docker workload, evidence +publication, or merge is authorized by this design. diff --git a/src/main.rs b/src/main.rs index 524fdd2..7a80c4f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -12,6 +12,9 @@ // See the License for the specific language governing permissions and // limitations under the License. +mod terminal; +use terminal::{TerminalFailure, finalize_owned_terminal}; + use std::collections::BTreeMap; use std::ffi::OsString; use std::fmt; @@ -551,7 +554,7 @@ fn print_benchmark( .map_err(CliError::Admission)?; let result = resource_pre_start(supervisor.clone(), &cancellation) .and_then(|_| run_benchmark(commit, runtime_probe.as_ref()).map_err(CliError::Benchmark)); - let envelope = release_admission(guard, result)?; + let envelope = finalize_benchmark_terminal(result, || guard.release())?; if let Some(path) = output { write_new_receipt(path, &envelope).map_err(CliError::Benchmark)?; } @@ -1075,25 +1078,33 @@ fn print_run( }; lifecycle.transition_state(RunJournalStateV1::Admitted, None)?; if let Err(error) = resource_pre_start(supervisor.clone(), &cancellation) { - return match guard.release() { - Ok(()) => { - lifecycle.fail(cli_failure_kind(&error))?; - Err(error) - } - Err(release_error) => { - lifecycle.transition_state(RunJournalStateV1::CleanupPending, None)?; - Err(CliError::Admission(release_error)) - } - }; + return finalize_run_terminal( + Err::<(), _>(error), + std::convert::identity, + || guard.release(), + |event| match event { + RunTerminalJournalEvent::PrimaryFailure(kind) => lifecycle.fail(kind), + RunTerminalJournalEvent::ReleaseFailure => { + lifecycle.transition_state(RunJournalStateV1::CleanupPending, None) + } + }, + ); } let watchdog = if ResourcePlatform::current() == ResourcePlatform::MacOs { let current_dir = match std::env::current_dir() { Ok(path) => path, Err(error) => { - return match guard.release() { - Ok(()) => Err(CliError::internal(error)), - Err(release_error) => Err(CliError::Admission(release_error)), - }; + return finalize_run_terminal( + Err::<(), _>(CliError::internal(error)), + std::convert::identity, + || guard.release(), + |event| match event { + RunTerminalJournalEvent::PrimaryFailure(kind) => lifecycle.fail(kind), + RunTerminalJournalEvent::ReleaseFailure => { + lifecycle.transition_state(RunJournalStateV1::CleanupPending, None) + } + }, + ); } }; Some(ResourceWatchdog::start( @@ -1124,22 +1135,20 @@ fn print_run( &mut lifecycle, runtime_preflight, ); - let outcome = run_result.map_err(CliError::Run); - completion_barrier.ensure_joined(); - let outcome = reconcile_watchdog_outcome(outcome, &mut completion_barrier); - let outcome = match guard.release() { - Ok(()) => match outcome { - Ok(outcome) => outcome, - Err(error) => { - lifecycle.fail(cli_failure_kind(&error))?; - return Err(error); + let outcome = finalize_run_terminal( + run_result.map_err(CliError::Run), + |outcome| { + completion_barrier.ensure_joined(); + reconcile_watchdog_outcome(outcome, &mut completion_barrier) + }, + || guard.release(), + |event| match event { + RunTerminalJournalEvent::PrimaryFailure(kind) => lifecycle.fail(kind), + RunTerminalJournalEvent::ReleaseFailure => { + lifecycle.transition_state(RunJournalStateV1::CleanupPending, None) } }, - Err(error) => { - lifecycle.transition_state(RunJournalStateV1::CleanupPending, None)?; - return Err(CliError::Admission(error)); - } - }; + )?; if let Err(error) = source_snapshot.cleanup() { lifecycle.transition_state(RunJournalStateV1::CleanupPending, None)?; return Err(CliError::Run(RunError::SourceSnapshot(error))); @@ -1226,19 +1235,35 @@ fn print_matrix_run( }; lifecycle.transition_state(RunJournalStateV1::Admitted, None)?; if let Err(error) = resource_pre_start(supervisor.clone(), &cancellation) { - return match guard.release() { - Ok(()) => { - lifecycle.fail(cli_failure_kind(&error))?; - Err(error) - } - Err(release_error) => { - lifecycle.transition_state(RunJournalStateV1::CleanupPending, None)?; - Err(CliError::Admission(release_error)) - } - }; + return finalize_run_terminal( + Err::<(), _>(error), + std::convert::identity, + || guard.release(), + |event| match event { + RunTerminalJournalEvent::PrimaryFailure(kind) => lifecycle.fail(kind), + RunTerminalJournalEvent::ReleaseFailure => { + lifecycle.transition_state(RunJournalStateV1::CleanupPending, None) + } + }, + ); } let watchdog = if ResourcePlatform::current() == ResourcePlatform::MacOs { - let current_dir = std::env::current_dir().map_err(CliError::internal)?; + let current_dir = match std::env::current_dir() { + Ok(path) => path, + Err(error) => { + return finalize_run_terminal( + Err::<(), _>(CliError::internal(error)), + std::convert::identity, + || guard.release(), + |event| match event { + RunTerminalJournalEvent::PrimaryFailure(kind) => lifecycle.fail(kind), + RunTerminalJournalEvent::ReleaseFailure => { + lifecycle.transition_state(RunJournalStateV1::CleanupPending, None) + } + }, + ); + } + }; Some(ResourceWatchdog::start( ResourceProbe::new(SupervisorResourceRunner::new( supervisor.clone(), @@ -1270,25 +1295,24 @@ fn print_matrix_run( &mut completion_barrier, ) .map_err(CliError::Matrix); - completion_barrier.ensure_joined(); - let result = if let Some(error) = completion_barrier.take_join_error() { - Err(CliError::Resource(ResourceGuardError::Watchdog(error))) - } else { - result - }; - let outcome = match guard.release() { - Ok(()) => match result { - Ok(outcome) => outcome, - Err(error) => { - lifecycle.fail(cli_failure_kind(&error))?; - return Err(error); + let outcome = finalize_run_terminal( + result, + |result| { + completion_barrier.ensure_joined(); + if let Some(error) = completion_barrier.take_join_error() { + Err(CliError::Resource(ResourceGuardError::Watchdog(error))) + } else { + result } }, - Err(error) => { - lifecycle.transition_state(RunJournalStateV1::CleanupPending, None)?; - return Err(CliError::Admission(error)); - } - }; + || guard.release(), + |event| match event { + RunTerminalJournalEvent::PrimaryFailure(kind) => lifecycle.fail(kind), + RunTerminalJournalEvent::ReleaseFailure => { + lifecycle.transition_state(RunJournalStateV1::CleanupPending, None) + } + }, + )?; lifecycle .transition(RunLifecyclePhase::Finalizing) .map_err(CliError::Run)?; @@ -1801,11 +1825,41 @@ fn resource_pre_start( } } -fn release_admission(guard: AdmissionGuard, result: Result) -> Result { - let release = guard.release().map_err(CliError::Admission); - match release { - Ok(()) => result, - Err(error) => Err(error), +fn finalize_benchmark_terminal( + primary: Result, + release: impl FnOnce() -> Result<(), AdmissionError>, +) -> Result { + match finalize_owned_terminal(primary, std::convert::identity, release) { + Ok(value) => Ok(value), + Err(TerminalFailure::Primary(error)) => Err(error), + Err(TerminalFailure::Release(error)) => Err(CliError::Admission(error)), + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum RunTerminalJournalEvent { + PrimaryFailure(RunFailureKindV1), + ReleaseFailure, +} + +fn finalize_run_terminal( + primary: Result, + complete_owned: impl FnOnce(Result) -> Result, + release: impl FnOnce() -> Result<(), AdmissionError>, + mut journal: impl FnMut(RunTerminalJournalEvent) -> Result<(), CliError>, +) -> Result { + match finalize_owned_terminal(primary, complete_owned, release) { + Ok(value) => Ok(value), + Err(TerminalFailure::Primary(error)) => { + journal(RunTerminalJournalEvent::PrimaryFailure(cli_failure_kind( + &error, + )))?; + Err(error) + } + Err(TerminalFailure::Release(error)) => { + journal(RunTerminalJournalEvent::ReleaseFailure)?; + Err(CliError::Admission(error)) + } } } @@ -2049,24 +2103,27 @@ fn finalize_guard_exec_result( trip: Option, release: impl FnOnce() -> Result<(), GuardExecError>, ) -> Result { - let result = if let Some(error) = join_error { - Err(GuardExecError::Resource(ResourceGuardError::Watchdog( - error, - ))) - } else if let Some(reason) = trip { - Err(GuardExecError::Resource( - ResourceGuardError::WatchdogTripped(reason), - )) - } else if cancellation.reason() - == Some(commit_ci_preflight::process::CancellationReason::ResourcePressure) - { - Err(GuardExecError::ResourcePressure) - } else { - result - }; - match release() { - Ok(()) => result, - Err(error) => Err(error), + match finalize_owned_terminal( + result, + |result| { + if let Some(error) = join_error { + Err(GuardExecError::Resource(ResourceGuardError::Watchdog( + error, + ))) + } else if let Some(reason) = trip { + Err(GuardExecError::Resource( + ResourceGuardError::WatchdogTripped(reason), + )) + } else if cancellation.reason() == Some(CancellationReason::ResourcePressure) { + Err(GuardExecError::ResourcePressure) + } else { + result + } + }, + release, + ) { + Ok(value) => Ok(value), + Err(TerminalFailure::Primary(error) | TerminalFailure::Release(error)) => Err(error), } } @@ -2552,11 +2609,13 @@ impl std::error::Error for CliMessageError {} mod tests { use super::{ Cli, CliError, GuardCommand, GuardExecArgs, GuardExecError, ResourceCacheStateArg, - ResourceExecutionModeArg, ResourceExecutorArg, WatchdogCompletionBarrier, - detect_resource_executor, finalize_guard_exec_result, new_journal_id, + ResourceExecutionModeArg, ResourceExecutorArg, RunTerminalJournalEvent, + WatchdogCompletionBarrier, detect_resource_executor, finalize_benchmark_terminal, + finalize_guard_exec_result, finalize_run_terminal, new_journal_id, reconcile_watchdog_outcome, resource_run_outcome, resource_terminal_detail, }; use clap::{CommandFactory, Parser}; + use commit_ci_preflight::admission::AdmissionError; use commit_ci_preflight::cache::{ CacheError, CacheKey, CacheRootSource, ManagedCache, ResolvedCacheRoot, }; @@ -2565,10 +2624,12 @@ mod tests { use commit_ci_preflight::process::{ CleanupStatus, ExitOutcome, ProcessResult, ProcessTermination, RunIdentity, }; + use commit_ci_preflight::resource::ResourceGuardError; use commit_ci_preflight::resource::{ ResourceCommand, ResourceCommandRunner, ResourceProbe, ResourceProbeError, ResourceWatchdog, }; use commit_ci_preflight::resource_history::{ResourceExecutorV2, ResourceTerminalDetailV2}; + use commit_ci_preflight::run_journal::{RunFailureKindV1, RunJournalError}; use std::ffi::OsString; use std::fs; use std::path::{Path, PathBuf}; @@ -3095,6 +3156,215 @@ timeout_seconds = 60 ); assert!(matches!(resource, Err(GuardExecError::ResourcePressure))); assert_eq!(release_count.load(Ordering::SeqCst), 3); + + let release_failure = finalize_guard_exec_result( + Err(GuardExecError::InternalFailure), + &cancellation, + None, + None, + || { + release_count.fetch_add(1, Ordering::SeqCst); + Err(GuardExecError::Admission(AdmissionError::Clock)) + }, + ); + assert!(matches!( + release_failure, + Err(GuardExecError::Admission(AdmissionError::Clock)) + )); + assert_eq!(release_count.load(Ordering::SeqCst), 4); + } + + #[test] + fn benchmark_terminal_preserves_primary_and_release_precedence() { + let releases = AtomicUsize::new(0); + let primary = finalize_benchmark_terminal( + Err::<(), _>(CliError::Benchmark( + commit_ci_preflight::benchmark::BenchmarkError::NoSamples, + )), + || { + releases.fetch_add(1, Ordering::SeqCst); + Ok(()) + }, + ); + assert!(matches!(primary, Err(CliError::Benchmark(_)))); + + let release = finalize_benchmark_terminal(Ok(()), || { + releases.fetch_add(1, Ordering::SeqCst); + Err(AdmissionError::Clock) + }); + assert!(matches!( + release, + Err(CliError::Admission(AdmissionError::Clock)) + )); + assert_eq!(releases.load(Ordering::SeqCst), 2); + } + + #[test] + fn run_terminal_orders_watchdog_release_and_primary_journal() { + use std::cell::RefCell; + let events = RefCell::new(Vec::new()); + let result = finalize_run_terminal( + Err::<(), _>(CliError::Resource(ResourceGuardError::PreStartDenied)), + |primary| { + events.borrow_mut().push("complete"); + primary + }, + || { + events.borrow_mut().push("release"); + Ok(()) + }, + |event| { + events.borrow_mut().push(match event { + RunTerminalJournalEvent::PrimaryFailure(RunFailureKindV1::ResourcePressure) => { + "journal-primary" + } + _ => "unexpected-journal", + }); + Ok(()) + }, + ); + assert!(matches!(result, Err(CliError::Resource(_)))); + assert_eq!( + &*events.borrow(), + &["complete", "release", "journal-primary"] + ); + } + + #[test] + fn run_terminal_release_failure_journals_cleanup_pending() { + use std::cell::RefCell; + let events = RefCell::new(Vec::new()); + let result = finalize_run_terminal( + Ok(()), + |primary| primary, + || Err(AdmissionError::Clock), + |event| { + events.borrow_mut().push(event); + Ok(()) + }, + ); + assert!(matches!( + result, + Err(CliError::Admission(AdmissionError::Clock)) + )); + assert_eq!( + &*events.borrow(), + &[RunTerminalJournalEvent::ReleaseFailure] + ); + } + + #[test] + fn run_terminal_journal_failure_overrides_release_failure() { + let result = finalize_run_terminal( + Ok(()), + |primary| primary, + || Err(AdmissionError::Clock), + |_| Err(CliError::RunJournal(RunJournalError::InvalidTransition)), + ); + assert!(matches!( + result, + Err(CliError::RunJournal(RunJournalError::InvalidTransition)) + )); + } + + #[test] + fn all_heavy_family_adapters_release_once_and_fail_closed() { + let releases = AtomicUsize::new(0); + let benchmark = finalize_benchmark_terminal(Ok(()), || { + releases.fetch_add(1, Ordering::SeqCst); + Err(AdmissionError::Clock) + }); + assert!(matches!( + benchmark, + Err(CliError::Admission(AdmissionError::Clock)) + )); + let run = finalize_run_terminal( + Ok(()), + std::convert::identity, + || { + releases.fetch_add(1, Ordering::SeqCst); + Err(AdmissionError::Clock) + }, + |_| Ok(()), + ); + assert!(matches!( + run, + Err(CliError::Admission(AdmissionError::Clock)) + )); + let cancellation = CancellationToken::default(); + let guard = finalize_guard_exec_result( + Ok(completed_process_result()), + &cancellation, + None, + None, + || { + releases.fetch_add(1, Ordering::SeqCst); + Err(GuardExecError::Admission(AdmissionError::Clock)) + }, + ); + assert!(matches!( + guard, + Err(GuardExecError::Admission(AdmissionError::Clock)) + )); + assert_eq!(releases.load(Ordering::SeqCst), 3); + } + + #[derive(Debug, Clone, Copy, PartialEq, Eq)] + enum OpaqueTerminalOutcome { + TimedOut, + UserCancelled, + ResourcePressure, + } + + #[test] + fn benchmark_and_run_preserve_opaque_terminal_outcomes() { + for expected in [ + OpaqueTerminalOutcome::TimedOut, + OpaqueTerminalOutcome::UserCancelled, + OpaqueTerminalOutcome::ResourcePressure, + ] { + assert_eq!( + finalize_benchmark_terminal(Ok(expected), || Ok(())) + .expect("benchmark terminal outcome"), + expected + ); + assert_eq!( + finalize_run_terminal(Ok(expected), std::convert::identity, || Ok(()), |_| Ok(())) + .expect("run terminal outcome"), + expected + ); + } + } + + #[test] + fn guard_cache_pin_remains_live_through_terminal_release() { + let (cache, source, base) = guard_cache_fixture("terminal-release"); + let cancellation = CancellationToken::default(); + + let result = + super::with_guard_cache_pins(Some(&cache), std::slice::from_ref(&source), || { + finalize_guard_exec_result( + Ok(completed_process_result()), + &cancellation, + None, + None, + || { + assert!(matches!( + cache.pin_completed_sources(std::slice::from_ref(&source)), + Err(CacheError::LockBusy(_)) + )); + Ok(()) + }, + ) + }); + + assert!(result.is_ok()); + assert!( + cache + .pin_completed_sources(std::slice::from_ref(&source)) + .is_ok() + ); + cleanup_guard_cache_fixture(&base); } #[test] diff --git a/src/terminal.rs b/src/terminal.rs new file mode 100644 index 0000000..376c14a --- /dev/null +++ b/src/terminal.rs @@ -0,0 +1,128 @@ +#[derive(Debug, PartialEq, Eq)] +pub(super) enum TerminalFailure { + Primary(P), + Release(R), +} + +pub(super) fn finalize_owned_terminal( + primary: Result, + complete_owned: impl FnOnce(Result) -> Result, + release: impl FnOnce() -> Result<(), R>, +) -> Result> { + let completed = complete_owned(primary); + match release() { + Ok(()) => completed.map_err(TerminalFailure::Primary), + Err(error) => Err(TerminalFailure::Release(error)), + } +} + +#[cfg(test)] +mod tests { + use super::{TerminalFailure, finalize_owned_terminal}; + use std::cell::{Cell, RefCell}; + use std::rc::Rc; + + struct FakeGuard { + releases: Rc>, + released: bool, + } + + impl FakeGuard { + fn release(mut self) { + if !self.released { + self.releases.set(self.releases.get() + 1); + self.released = true; + } + } + } + + impl Drop for FakeGuard { + fn drop(&mut self) { + if !self.released { + self.releases.set(self.releases.get() + 1); + self.released = true; + } + } + } + + #[test] + fn completion_precedes_exactly_one_release() { + let events = RefCell::new(Vec::new()); + let releases = Cell::new(0); + + let result = finalize_owned_terminal( + Ok::<_, &'static str>(7_u8), + |primary| { + events.borrow_mut().push("complete"); + primary + }, + || { + events.borrow_mut().push("release"); + releases.set(releases.get() + 1); + Ok::<_, &'static str>(()) + }, + ); + + assert_eq!(result, Ok(7)); + assert_eq!(&*events.borrow(), &["complete", "release"]); + assert_eq!(releases.get(), 1); + } + + #[test] + fn primary_failure_survives_successful_release() { + let result = finalize_owned_terminal( + Err::<(), _>("workload"), + |primary| primary, + || Ok::<(), &'static str>(()), + ); + + assert_eq!(result, Err(TerminalFailure::Primary("workload"))); + } + + #[test] + fn release_failure_overrides_success_or_primary_failure() { + for primary in [Ok(()), Err("workload")] { + let result = + finalize_owned_terminal(primary, |primary| primary, || Err::<(), _>("release")); + + assert_eq!(result, Err(TerminalFailure::Release("release"))); + } + } + + #[test] + fn completion_failure_still_releases_once() { + let releases = Cell::new(0); + let result = finalize_owned_terminal( + Ok::<(), &'static str>(()), + |_| Err("watchdog"), + || { + releases.set(releases.get() + 1); + Ok::<(), &'static str>(()) + }, + ); + + assert_eq!(result, Err(TerminalFailure::Primary("watchdog"))); + assert_eq!(releases.get(), 1); + } + + #[test] + fn explicit_release_consumes_guard_without_drop_release() { + let releases = Rc::new(Cell::new(0)); + let guard = FakeGuard { + releases: Rc::clone(&releases), + released: false, + }; + + let result = finalize_owned_terminal( + Ok::<(), &'static str>(()), + |primary| primary, + || { + guard.release(); + Ok::<(), &'static str>(()) + }, + ); + + assert_eq!(result, Ok(())); + assert_eq!(releases.get(), 1); + } +}