pipeline: transactional sequential pipelines over a shared COW workdir (RFC #65 Phase 1)#148
Open
dzerik wants to merge 33 commits into
Open
pipeline: transactional sequential pipelines over a shared COW workdir (RFC #65 Phase 1)#148dzerik wants to merge 33 commits into
dzerik wants to merge 33 commits into
Conversation
Contributor
Author
|
Implemented the first-committer-win lock the body flagged as unimplemented (latest commit). Before the merge, Scoped to the transactional commit path only; the single-sandbox |
Contributor
|
Design review (no code details). Core shape is sound: coordinator owns one branch, stages attach, finalize on every exit path. Five things. 1. The
This contradicts the doc comment on Separately, adding 2.
The RFC names 3. The commit lock is not first-committer-win. The Losing the race also returns 4. Phase 1 acceptance is not met. RFC Phase 1 specifies Slicing is fine, but then call it Phase 1a and say what is still owed. As written it reads as closing Phase 1. The body is stale regardless: it still says the workdir lock is unimplemented. 5. Error typing undercuts the RFC's CLI goal. Every validation failure is |
This was referenced Jul 22, 2026
…ikernel#65 Phase 1) Add Pipeline::run_transactional(): sequential stages share one COW upper over a common workdir, so a later stage sees an earlier stage's writes (read-committed), the real workdir is untouched during the run, and the pipeline commits all-or-nothing -- every stage exits 0 -> one commit; any stage fails -> the shared upper is discarded and the workdir is byte-identical. Stages exchange data through the shared workspace, not inter-stage pipes; the streaming run()/`|` path is unchanged. Sequential stages let one shared upper stand in for the RFC's per-stage lower-lookup chain (the shared SeccompCowBranch is injected into each stage's runtime; wait()/Drop leave it for the coordinator's single commit/abort). Per-stage uppers (parallel siblings) are Phase 2. Hardening from an adversarial self-review: - Add `impl Drop for SeccompCowBranch` (cleanup unless finished) so an aborted- by-error/timeout/panic transaction -- or any abandoned branch -- can't orphan its upper on disk. `keep()` marks BranchAction::Keep so the backstop preserves a kept upper instead of cleaning it. - run_pipeline_txn finalizes (commit on clean success, else abort) on EVERY exit path, including a driver error, before propagating it -- the branch is never left dangling; the branch is taken out from under the async mutex before the sync merge. - Guardrails: reject a <2-stage pipeline (no stages[0] panic), a stage with chroot, or a stage whose fs_storage/max_disk differs from stage 0 (all share one upper). canonicalize the workdir before creating the branch dir so a failure there can't leak an empty dir. TxnOutcome derives Debug/Clone. Integration tests cover commit-on-success, abort-on-failure (no leak), timeout-abort, and the guardrails (pinned to the error message); a unit test covers the Drop backstop and keep() preservation.
The initial suite tested 3 of 6 guardrails and only the last-stage-failure / clean-abort/commit paths. Add the missing cases so a regression in any guardrail or boundary hard-fails instead of slipping through: Guardrails (no sandbox needed): - mismatched workdir across stages (distinct from missing workdir) - no_supervisor=true stage - chroot stage - mismatched fs_storage/max_disk across stages Behavioral (sandbox-gated): - first-stage failure aborts AND stops early (outcome.stages holds only the failed stage — later stages must not run) - timeout aborts AND reclaims the shared upper (the reclaim test only covered clean abort/commit) - COW deletion (whiteout): commit applies the deletion and a later stage sees it (read-committed); abort discards it and the file survives byte-identical All 14 pass on a real cage.
…abort Clarify that stages only carries per-stage results on a stage-failure abort; on a timeout or driver-error abort the stage-driver future is cancelled and its accumulated results are dropped, so the field is empty while committed and abort_reason still report the outcome.
…merge
A transactional pipeline's final commit merges the shared upper into the workdir
file-by-file, so two concurrent transactions committing into one shared workdir
could interleave and corrupt it. Before the merge, `run_pipeline_txn` now takes
an exclusive, non-blocking flock on the workdir: a transaction that finds it held
LOSES the first-committer-win race — reported as a benign non-commit outcome
(`TxnOutcome { committed: false, abort_reason: Some("...lost the
first-committer-win race...") }`) with its upper aborted, not as a hard error and
not by racing the merge. This matches the maintainer's stated intent
("first-committer-win for concurrent commits") for RFC multikernel#65 Phase 1.
The lock is scoped to the transactional commit path, NOT the general
`SeccompCowBranch::commit()` — the single-sandbox `on_exit=Commit` path is
unchanged (it discards commit errors and would have silently lost data). The lock
serializes only overlapping merges; it does not detect a stale base, so two
non-overlapping transactions touching the same file still resolve last-writer-wins
(this is concurrency safety, not serializable isolation).
Integration test holds the workdir lock and asserts a fully-successful pipeline
returns committed=false with the race named in abort_reason and nothing merged;
verified it commits (races) without the lock.
`commit()` set `finished = true` only after the whole merge succeeded, so every `?` inside the merge loop returned with the flag still false and the `Drop` backstop then reclaimed the branch storage. On `ENOSPC`, `EACCES` or an obstructing symlink the workdir was left half-merged AND the upper holding the unmerged remainder was deleted — worse than the pre-`Drop` behavior, where that upper survived and was recoverable. It also foreclosed a later recovery sweep, whose whole premise is durable branch state. Replace the `finished` bool with an explicit three-state disposition. `commit()` enters `MergeInterrupted` BEFORE the first destructive operation, so any early return keeps the storage; only `Open` (no disposition ever attempted) reclaims on `Drop`. The state is not latched as finished on entry, so `commit()` stays retryable once the cause is cleared — a naive entry guard would turn the retry into a silent no-op reporting success. Restate the `Drop` doc as what it is: a behavior change for every holder of a branch, not only transactional pipelines, and deliberately not a clean-up-on-error hook. Two unit tests, both injecting the failure the way it actually occurs (a workdir symlink where the upper has a regular file, so the merge's `openat2(O_NOFOLLOW)` returns ELOOP — no permission games, so it also fails as intended when the suite runs as root): - the upper and its remainder survive both the failure and the drop; - clearing the obstruction and committing again completes the merge.
…action type `Pipeline` is defined by `|`: N stages running concurrently, stdout wired to stdin through kernel pipes. `run_transactional` ran the same object sequentially with no pipes and inherited stdio — two incompatible execution models on one builder, selected by method name. `a | grep foo | c` under `run_transactional()` handed `grep` the parent's stdin, silently and with no error, and there was no way to reject it: `BitOr` is available on every `Pipeline` and the `pub stages` field gives no signal to discriminate on. Introduce `Transaction`, constructible only from an explicit stage list, with a private field and deliberately no `BitOr`/`From<Pipeline>` impl. A `|`-built chain can no longer reach the sequential runner at all, so the footgun is closed by construction rather than by a doc comment. This is a pure move: `TxnOutcome`, the guardrails and the coordinator are carried over unchanged and the 15 existing tests keep passing unmodified apart from their construction site. Nothing to break downstream — `Pipeline::run_transactional` was never released, and is absent from the FFI, the header, Python and Go. Side effect worth noting: the `pipeline` module header's claim that "data flows through kernel pipe buffers between stages" is true again for everything left in that module. Naming: `Transaction::new` here takes stages, not sandboxes. RFC multikernel#65's end-state sketch is `Transaction::new(sandboxes).run()`, which cannot be built as written — a `Sandbox` carries no argv, and the coordinator needs the command. Happy to reserve `new` for that signature and rename this to `Transaction::from_stages` if preferred.
…er-win
The `flock` covers only the merge, so it never was first-committer-win.
Transaction A can snapshot, B can commit, and A can then merge over B's
result from a stale base: a lost update. What the lock actually provides is
mutual exclusion between two merges, which matters because `commit()`
rewrites the workdir file-by-file and interleaving would tear it. The comment
now says that, and says explicitly that it is not serializable isolation and
that a plain `Sandbox` committing into the same workdir does not take it.
Losing the race also returned `Ok(TxnOutcome { committed: false, .. })`
alongside a stages vector in which every stage succeeded, so a caller that
did not inspect `committed` silently lost a whole transaction's work behind
a success return. Since merges are short, wait instead: `acquire_commit_lock`
polls the non-blocking `flock` up to a deadline. A transaction that ran every
stage successfully now publishes its work rather than discarding it, and the
benign-but-ignorable non-commit outcome is gone from the API. A lock that is
never released is an `Err` naming the workdir — unignorable through `?` —
not an indefinite hang.
A failed merge now names the preserved upper in the error, so the remainder
that the previous commit made recoverable is also findable.
Tests: three unit tests on the helper (waits for a held lock, bounded, no
latency when uncontended) and one end-to-end test where the lock is released
mid-run and the transaction must still commit. Mutation-proven — removing the
wait loop, or making it unbounded, turns each of them red.
Three gaps, all cheap now and expensive once a Python SDK parses them. Error typing. Every validation failure was `SandlockError::Runtime(SandboxRuntimeError::Child(..))`, so a misconfigured transaction was indistinguishable from a child process error; the guardrails now raise `SandboxError::Invalid`, which is what they are. `abort_reason` was an `Option<String>`, so callers had to string-match to tell a timeout from a stage failure — our own test did exactly that. It is now an `AbortReason` enum carrying the failing stage's index and status. `#[non_exhaustive]`, because RFC multikernel#65 puts merge conflicts in Phase 2 and enumerates no variants itself, so this set is knowably incomplete. `TxnOutcome.changes`. The RFC's `TxnOutcome` carries it and ours did not, so there was no way to see what a transaction did without diffing the workdir. It reuses `dry_run::Change` exactly as RFC design note 5 asks. The capture has to happen before the branch is disposed of — after a commit or an abort the upper is gone — so the test that guards it is mutation-proven by moving the capture after the disposition, not merely by asserting a non-empty vec. `dry_run()`. Runs every stage, reports the changes, discards them; the same contract `Sandbox::dry_run` already has for one sandbox. Because there is exactly one shared upper, its change set is already the union across stages. It enforces the same guardrails as `run` — proven by a test, since skipping validation would have made it a way around them. Also fixed rather than documented: `stages` used to be empty on a timeout because the driver future's results died with it. A previous commit documented that emptiness with no test to carry the claim; instead of testing a wart, results are now published as each stage finishes, so the outcome reports the completed prefix — which is what RFC design note 4 asks for. `TxnOutcome` is `#[must_use]`. Scope: this is RFC multikernel#65 Phase 1a. Still owed, and not silently: the Python surface plus its end-to-end test (next PR, per the agreed core -> FFI -> Python split), `conflicts`/`MergeConflict` (Phase 2 by RFC text), `prepare()` and the context manager. Two deliberate deviations from the Phase 1 text: stages short-circuit at the first failure rather than "waiting for all stages", because under a sequential shared-workspace model stage N+1's inputs do not exist if stage N failed; and non-default `on_exit`/`on_error` are rejected rather than forced to `Keep`, which is the resolution agreed in the RFC thread. The "fail fast on non-seccomp backends" bullet is moot — branchfs and overlayfs are gone from the tree.
A transaction whose every stage exited 0 could still lose all of its work. `run_txn` propagated the commit-lock timeout out with the shared branch still in `BranchState::Open`: nothing had called `commit()`, so nothing set the preserved state, and the branch's `Drop` reclaimed the upper on the way out. A 2-stage transaction with every stage exiting 0 returned `Err` after the full wait with zero branch dirs left in storage — the exact failure class the waiting commit lock was introduced to close, on the path where the wait expires instead of succeeding. Generalize the branch's preserved state so it can name why the storage was handed over (`MergeInterrupted`, `CommitDeferred`, `Kept`) and mark the branch `CommitDeferred` before returning, so `Drop` keeps it. The error names the preserved upper, as the merge-failure path already did. Add `Transaction::commit_lock_wait` so the bound is a caller decision rather than a hardcoded 30s, and so the expiry path is testable without a 30s test. The new integration test holds the workdir lock for the whole run and asserts the workdir is untouched AND the preserved upper still holds both stages' bytes.
The branch `Drop` backstop keys off `BranchState`, which only a disposition call ever sets. `Sandbox` applies its configured `BranchAction` from `rt.seccomp_cow`, and that is filled exclusively by a `wait()` that ran to completion — so a sandbox built with `on_exit(Keep)`/`on_error(Keep)` and abandoned without `wait()` left its branch `Open`, and the backstop deleted the upper. `Keep` means "preserve for later inspection", and abandonment is precisely the forensic case, so the backstop was silently overriding a user-facing option (CLI `--on-exit keep`, profile "keep", FFI variant 2). Record the request on the branch itself at creation time, where its own `Drop` can honour it. `Commit`/`Abort` are deliberately not carried over: an abandoned run has no exit status, and merging the writes of a run that never finished is not something it can ask for, so those keep the reclaiming default. `dry_run`/`dry_run_interactive` set the action to `Keep` internally for a run whose changes they read out and then abort. That would now also ask an abandoned dry run (a `?` out of create/wait) to leave its upper behind, which for a dry run is a pure leak — so they use `Abort`, which is identical on every path they actually take. The test asserts both halves, and waits for the default-action branch to be reclaimed first: that reclaim is the proof the branch drops have run, without which the Keep assertion would pass on a branch that had simply not been dropped yet.
`AbortReason` typed the ABORT channel, but every failure on the COMMIT channel
— branch creation, the commit-lock timeout (the RFC's "conflict"), and the
merge itself — was one untyped `SandboxRuntimeError::Child(String)`. A caller
that wanted to retry a conflict but not a broken merge had to match on
message text.
Introduce `TxnError` with a variant per way a transaction can fail to be
carried out: `Invalid`, `Branch`, `Stage { index }`, `Conflict`, `CommitLock`,
`Merge`. Conflict is separated from CommitLock by an internal `LockFailure`,
so contention (retry) is not confused with a workdir that cannot be opened.
The paths carried by `Conflict`/`CommitLock`/`Merge` are fields, not text.
`TxnError::exit_code` maps the channels to the conventional sysexits codes —
78 configuration, 70 stage driver, 74 commit, 75 EX_TEMPFAIL for a conflict —
and `TxnOutcome::exit_code` reports the abort channel: 0 committed or dry run,
the failing stage's own code (128+signal when signalled), 124 on timeout.
`From<TxnError> for SandlockError` keeps the previous property that a
misconfigured transaction stays a `Sandbox` error rather than masquerading as
a child failure.
…nge set
`commit()` copied each entry into the workdir but never removed it from the
upper, and `changes()` walks the whole upper unconditionally. After a 19/20
merge `changes()` therefore returned 20 — byte for byte what a 0/20 merge
returns — so the operator the doc points at ("call `changes()` to inspect what
is outstanding") could not tell which files still had to land. The same
inaccuracy sat in `BranchState::MergeInterrupted`'s doc and in the
transaction's merge-failure message, both of which say "remainder".
Drop each change from the upper once its workdir side is in place, and forget
each deletion once it is applied. The remainder is now literally what is left
in the branch: `changes()` answers the recovery question, and a retry does not
re-copy what already landed.
Merge entries in sorted order so a partial merge is a prefix of a deterministic
sequence, and collect the walk before mutating the tree it walked.
The unlink is deliberately after the copy, so the failure mode of the
bookkeeping is a change merged twice (idempotent: the copy truncates, the
symlink is recreated), never a change silently lost.
`BranchState::MergeInterrupted` claimed the preserved storage was "the only thing a retry or an out-of-band recovery can work from". The retry half was true; the out-of-band half was not. The state existed only in this process's memory and the branch dir held nothing but `upper/` — no workdir, no marker, no metadata. After the process exited a preserved upper was indistinguishable from any orphaned upper, and a sweep could not know which workdir it belonged to. That left RFC Phase 3, whose premise is a recovery sweep over durable branch state, foreclosed while the doc said otherwise. Preserving now writes a `PRESERVED` marker next to the upper carrying the reason, the workdir, the upper and the pid, and `read_preserved`/ `list_preserved` read it back — the sweep primitive. `commit()` writes it before it touches the workdir, so a crash mid-merge is covered too. Paths go through the marker as raw bytes with `\` and `\n` escaped: a workdir path may legally contain a newline and need not be UTF-8, and a line-based format that lost such a path would lose the only pointer back to the workdir. The leak itself is stated plainly rather than papered over: preserved storage is a deliberate leak, nothing in the process frees it, and reclaiming it is out-of-band work the marker exists to make possible.
`test_txn_waits_for_a_concurrent_commit_lock` took a raw flock, released it after a fixed 400ms, ran a 2-stage transaction and asserted only `outcome.committed`. Nothing verified that the transaction ever met the lock. It passed GREEN in 2.03s with the wait loop mutated to a single non-blocking attempt and stage 0 slowed so the 400ms hold expired 1.6s before the commit — green on exactly the code it names as broken, because by then there was no lock left to contend for. Hold the lock until the transaction is demonstrably blocked on it instead. The test drives the transaction concurrently and fails the moment it finishes while the lock is still held; it releases only once the last stage's write has appeared in the shared upper (so the only thing left is the commit) plus a grace period. Under the fail-fast mutation it now fails with the conflict the transaction returned. It also asserts the merged bytes, not just that the files exist.
`commit_lock_is_immediate_when_uncontended` asserted only `elapsed() < 100ms`, which is not the property its comment claimed: it stayed GREEN with the poll loop made to add an unconditional 20ms to the uncontended path — a 20ms regression cannot trip a 100ms budget — and a wall-clock bound also flakes on a loaded runner. Assert on the paused test clock instead, which advances only when something actually sleeps: the uncontended acquisition must not advance it at all. The same 20ms mutation now fails it, and there is no wall-clock dependency left.
- The stage-count guardrail test said it "is the only thing standing between a
one-stage transaction and an out-of-bounds index in the coordinator". A
one-stage transaction indexes `stages[0]` fine — verified by removing the
guard: the one-stage case commits, and only the ZERO-stage case panics with
"index out of bounds" at transaction.rs's `&stages[0]`.
- `Transaction::new`'s doc said "from an explicit list of stages (at least 2)"
while the constructor is infallible and enforces nothing. Say where the
minimum is actually checked, and why that is deliberately asymmetric with
`Pipeline::new`, which returns `Result` for the same check.
- `TxnOutcome.stages` said nothing about the fact that every `RunResult` in it
has `stdout == None` and `stderr == None` by construction: stages are created
with full stdio inherit, so their output has already gone to the parent's fd
1/2. On `StageFailed { index, status }` a caller otherwise gets an index and
an exit code and no stated way to see why.
Also: `AbortReason`'s `non_exhaustive` note promised a Phase 2 merge-conflict
variant that now lives on the error channel as `TxnError::Conflict`, and the
`Transaction` type doc described commit-or-discard without the third case.
The explicit iterator plus while-let was the branch's only clippy delta against main (needless_range_loop family). Behaviour is unchanged: the entries are still collected before the merge starts, because the merge unlinks from the upper as it goes.
`commit()` applied deletions best-effort (`let _ = ...`) and dropped each one from `self.deleted` only if the entry had actually vanished, but the successful tail never looked at what was left. A deletion that could not be applied therefore returned `Ok(())`: the merge reported all-or-nothing success, the tail removed the storage dir, and the only record of the change that never landed went with it. At the transaction layer that surfaced as `committed: true` with the deletion still listed in `TxnOutcome.changes`. Stop after the deletion group if any deletion is still outstanding, before a single addition is copied, and report which one failed and why. The branch is already `Preserved(MergeInterrupted)` at that point, so the change set survives the failure and a sweep can find it; the transaction reports `TxnError::Merge`. Test injects the failure without permission games (so it holds when the suite runs as root): a symlinked parent component in the workdir, which the confined `unlinkat` resolves inside the workdir root and so cannot reach.
… run cannot destroy it The owned branch sat in `BranchState::Open` across `acquire_commit_lock`'s await. Dropping the `run()` future during that wait — up to 30s by default — ran `SeccompCowBranch::drop`, which reclaimed the storage: every stage had exited 0 and the whole change set was gone, with no `Err`, no `AbortReason` and no `PRESERVED` marker. Cancellation is not an exit path, so none of the code that preserves on the `return`/`?` exits ran. The trigger is manufactured by the API: `run`'s own `timeout` covers the stage phase only, so a caller wanting a wall-clock bound on the whole thing wraps `run()` in `tokio::time::timeout`, and a shutdown `select!` does the same. Move the change-set read and the whole disposition into one `spawn_blocking` with the branch moved in. A dropped future detaches the blocking task instead of stopping it, so the branch is never dropped `Open` by a cancellation: the change set is published or preserved either way. That also takes the two unbounded walks (`changes()`, and `commit()`'s per-file copy plus directory fsyncs) off the executor worker, which the crate already does for every other blocking filesystem step, and stops a worker being parked on a cross-process `flock`. The lock wait becomes blocking with the poll sleep injectable, so the uncontended case is asserted by counting sleeps instead of by the paused clock. `TxnError::CommitAbandoned` covers the one case the new shape adds: the runtime shutting down before the detached task ran. Documents what a cancelled run actually gives up — the report, not the work — replacing two statements that claimed more than the code carried.
…esurrect them Deletions were tracked only in the branch's in-RAM `deleted` set: no whiteout is written into the upper, and the `PRESERVED` marker carried only reason/workdir/upper/pid. A preserved branch therefore held additions and modifications and nothing else, while `PreserveReason::CommitDeferred` and `TxnError::Conflict`/`CommitLock` told the operator the whole change set was preserved and `TxnError::Merge` told him that recovering that storage — not retrying — is how the transaction gets finished. Recovering it would have restored every file the run deleted. Write the deletions into the marker as repeated `deleted=` lines, through the same escaping as the other paths (a name a child chose may contain a newline and need not be UTF-8), and read them back into `PreservedBranch::deleted`. The set is written as it stands when `preserve` is called, which for `commit()` is before any deletion is applied, so a recovery may re-apply one that has since landed; that is a no-op and it is the safe direction. Also corrects the reason and error strings to name the storage rather than the upper alone, and drops the claim that a `MergeInterrupted` marker means the workdir IS partially modified — it is written before the first destructive step, so a merge still in flight looks exactly the same.
…ailure `From<TxnError> for SandlockError` collapsed `Conflict`, `CommitLock`, `Merge` and `Branch` into `SandboxRuntimeError::Child(String)`, which states that a child process failed. A child process failing is the abort channel — an `Ok(TxnOutcome)` with an `AbortReason` — and never reaches this conversion. What does reach it is a disposition of the shared COW branch that could not be carried out, which is what `SandboxRuntimeError::Branch` is for. The rendered message is kept rather than only the `BranchError` source, which `Conflict` does not have at all: the message names the preserved upper, and that path cannot be reconstructed from anything else. The test asserted `Runtime(_)`, so it survived any mutation within `Runtime` — including the one this commit fixes. It now pins each channel to its exact variant and covers all three commit-channel variants plus the stage passthrough.
A stage that execs a file an earlier stage created had no test. The file really lives in the shared upper, and Landlock checks EXECUTE against the real path at execve time, so the grant in the shared-COW arm of `create_with_io` is load-bearing: removing it makes the stage exit 126 — and the whole suite stayed green. The plain-`Sandbox` half of the same grant is covered by `test_cow::test_seccomp_cow_exec_packed_argv_relocation`; this is the missing half of that pair.
…olds The module header said the two types are separate "precisely so a `|`-built chain cannot be handed to the sequential runner and silently lose its pipes", but `Pipeline::stages` was a public field and `Transaction::new(impl IntoIterator<Item = Stage>)` takes exactly that, so `Transaction::new(chain.stages)` compiled from outside the crate and did lose them, silently. Make the field private and give the decomposition a name, `Pipeline::into_stages`, whose doc says what handing the result to another runner costs. `len`/`is_empty` replace the field's only other use. The header now describes what is actually closed — no implicit conversion — rather than claiming the operation is impossible. There is no compile-fail harness in the workspace, so the "does not compile" half of this is carried by the type, not by a test.
…terrupted one `commit()` writes the `MergeInterrupted` marker before its first destructive step, so for the whole duration of a merge `list_preserved` reports a live branch as work awaiting recovery, and the reason's text asserted that the workdir IS partially modified when nothing may have been merged yet — steering an operator away from the retry that is in fact correct. The ordering is right and stays: the alternative loses the crash the marker exists for. Document what it costs instead, at all three places an operator meets it — the reason, `list_preserved`, and `PreservedBranch::pid`, which is the only thing that separates a live merge from an abandoned one. `TxnError::Merge` no longer states that the workdir IS partially merged either: since a deletion that cannot be applied fails the merge before anything is copied, that variant now also covers "nothing landed".
`set_keep_if_abandoned` is passed `on_exit == Keep || on_error == Keep`, but the only test configured `on_exit(Keep)`, so dropping the `on_error` half left the suite green. An abandoned run has no exit status and so no way to choose between the two actions — either one asking for `Keep` has to preserve — which is exactly the half that was untested.
The comment above the disposition still described only the async mutex guard. Name the other two reasons the move exists: the merge must not run on an executor worker, and it must not be droppable by a cancellation.
…weep Six properties the module carries that nothing checked: - `matches()` must exclude the branch's own storage. With `fs_storage` inside the workdir the upper is itself under the workdir prefix, so without the exclusion an access to the upper is treated as a workdir path and copied up again. - a path outside the workdir must be neither mapped by `safe_rel` nor intercepted, including a sibling directory whose name merely extends the workdir's — the trap a non-component-wise prefix test falls into. - one branch dir a sweep cannot parse (a marker cut short, or the live storage of a running branch, which has no marker at all) must not hide the preserved branches beside it. `list_preserved` documents this and nothing exercised it. - a branch kept for inspection must read back as `PreserveReason::Kept`. The reason is what tells a recovery what state the workdir is in, and only `MergeInterrupted` and `CommitDeferred` had a test. - `abort()` after `keep()` must not destroy the change set that was deliberately handed over to the caller. - a second `preserve()` must replace the reason recorded on disk: a branch deferred with the workdir untouched, and then merged part way, is no longer "commit-deferred". Each test is proven by mutating the code it guards.
The unit tests reached the commit lock and the error channels but never finish_branch itself, the function that decides what happens to a run's change set. Cover its four outcomes directly, with no sandbox needed: discarded (the changes are still reported, read before the abort), merged (bytes in the workdir, lock released), deferred (preserved as CommitDeferred when the lock is contended) and interrupted (the remainder preserved as MergeInterrupted when the merge cannot write an entry). The reason on the marker is the part a recovery acts on: CommitDeferred says the workdir was never touched, MergeInterrupted says it may already be half merged. Nothing pinned which one each path writes. Also pin TxnOutcome::exit_code for the statuses only it can produce — a signalled stage is 128 + signal, not the signal — and that a zero commit lock wait gives up without sleeping, i.e. the deadline is checked before the poll sleeps.
…ncelled stage phase Transaction-level cases with no coverage, each proven by mutating the code it guards: - a dry run neither takes the workdir commit lock nor keeps its upper; - a dry run stopped by a failing stage or a timeout reports that, not DryRun; - Sandbox::dry_run's forced branch action, which is only observable when the run is abandoned - on_exit=Keep must not survive into a kept upper - and the same override mutating the sandbox, so a policy that was dry-run is afterwards rejected as a transaction stage; - the run timeout bounds the stage phase only: a commit waiting out another transaction's merge is governed by commit_lock_wait alone; - a deadline that expires before the first stage aborts with an empty stages vector and still reclaims the upper; - a rejected transaction creates no branch and starts no stage, asserted with a sentinel outside the workdir that the COW layer cannot hide; - stage results are in execution order, including the failing stage's own; - a command that cannot be exec'd is a stage failure reporting 127, not a TxnError::Stage driver failure; - cancelling during the stage phase RECLAIMS the change set, the mirror image of the commit phase preserving it; - read-committed for a MODIFIED file: a later stage must read the new bytes, the half of that contract which fails silently rather than with ENOENT.
The transaction-layer unit tests asserted the commit lock and the branch
disposition but left the two things a caller meets first — the cross-stage
validator and the TxnError/AbortReason algebra — largely unguarded. Three
gaps were provable by mutation against the suite as it stood:
- narrowing the branch-action check to on_exit alone kept every existing
test green, so on_error was unguarded. on_error(Keep) is what a caller
who wants forensics on a failed stage reaches for, and it would preserve
the SHARED upper behind the coordinator's back;
- making the loop skip stage 0 kept every test_txn_rejects_* green, because
each puts its violation on the second stage — while stage 0 is the index
a refactor is most likely to special-case, since it is also the baseline;
- renumbering the config channel from 78 kept the distinctness assertion
green, so no exit code was pinned to its documented value.
Adds fourteen tests over: all nine branch-action cells (including that an
explicitly-written default is accepted, which is what a TOML profile spelling
out on_exit = "commit" produces); every guardrail that can fire at stage 0;
the absolute chroot/no_supervisor rules against the merely-must-agree
workdir/storage ones; check precedence across and within stages; that an
unset max_disk and a zero max_disk are one quota; that the workdir is
compared lexically, so a symlink alias is rejected even though the branch
would canonicalize it to the same directory; that a workdir-less transaction
is an error rather than the panic run_txn's expect() would otherwise raise,
from both entry points; the exact exit code of every TxnError variant, via an
exhaustive match that also stops a new variant slipping into the flatten
impl's catch-all arm unclassified; the phrase each commit-channel failure
must keep when flattened, including the two variants that carry no preserved
upper; that a stage exiting 74 still reports 74; every AbortReason rendering;
that a zero commit_lock_wait still takes a free lock; and that a lock that
fails for a reason other than contention preserves the upper too.
Every test was verified by mutating the code it guards and confirming it
turns red.
…contract Adds integration coverage for the plain-Sandbox COW path: - which branch action a completed run applies is selected by the child's exit status, and Keep records the run's deletions in the marker as well as its upper; - a merge that cannot run leaves the change set under a MergeInterrupted marker, which on this path is the only record the caller ever gets: the disposition runs in Drop and the commit error is discarded; - default storage goes to a per-process base and list_preserved reads exactly one level, so a sweep of $TMPDIR does not reach it; - O_CREAT|O_EXCL spans both layers, and unlink/rmdir type mismatches reach the child as EISDIR and ENOTDIR from either layer.
Adds 26 tests to the COW branch and fixes the defects six of them
expose. Every test is proven by mutating the code it guards.
Deletions:
- A deletion was classified with `is_dir()`, which follows a symlink,
so a symlink pointing at a directory went to the recursive remove
and came back ENOTDIR. The deletion then stayed outstanding and the
guard failed the whole merge — identically on every retry, with no
way past it. `mv ld renamed` on a workdir holding such a link
reaches it. Classify with `symlink_metadata` instead.
- `handle_unlink` of the workdir root recorded the empty relative
path, which `commit()` handed to the recursive remove as
"everything under the root" and then failed EINVAL on the root
itself. An `rmdir` of its own cwd emptied the workdir permanently.
Refuse it with EBUSY before anything is recorded.
- The comment claiming the merge is all-or-nothing on the deletion
side was false: the loop applies each deletion in turn, so a
failure leaves every removable one already applied. Only the
additions are all-or-nothing. Say so.
What `Ok(())` is allowed to mean — three routes closed:
- An upper entry whose name is not UTF-8 was skipped, and the
successful tail then removed the storage: success reported while
the only copy of the change was destroyed. Fail instead, so the
branch is preserved for a sweep.
- `mkdirp_in_root` reports EEXIST as success and does not check the
type, so an upper directory over a workdir file merged to nothing
and still returned Ok(()). Check the destination type.
- The merge opened the destination with a create mode, which does
nothing to an existing file, so a mode was never published: a
chmod-only change reported Modified and landed nowhere, and a
script the run made executable arrived un-executable. fchmod the
destination to the upper's mode, symmetrically with the copy-up.
Also covered, unchanged: deletion of a symlink or a dangling symlink,
nested and already-absent deletions, order-independence of overlapping
ones, deletions-before-additions, retry past the deletion guard, the
commit latch, changes() labelling against the live workdir, safe_rel's
normalise/reject/pass-through split, handle_symlink's refusal of an
absolute or escaping target, and the on-disk marker: backward and
forward compatibility, required keys, truncation at every byte offset,
a non-UTF-8 workdir path, and the fact that a child cannot forge one.
commit()'s rustdoc asserted "Ok(()) means every recorded change landed. Nothing else may return it" while the short-circuit four lines below it returns Ok(()) for any branch is_disposed() accepts — and that includes Preserved(Kept), whose upper still holds the entire change set. Probed: a Kept branch with an unmerged added.txt returns Ok(()) and the file does not land. Finished is a defensible no-op (the storage is already gone). Kept is a caller error reported as success, so the doc now names it as a wart to resolve before the surface is public rather than an invariant. Pinned by a_kept_branch_reports_a_commit_it_did_not_perform, which asserts the surprising behaviour explicitly so it cannot be lost: removing Kept from is_disposed() turns it red.
dzerik
force-pushed
the
feat/pipeline-transactions
branch
from
July 22, 2026 14:42
9c1ddf0 to
2be77ab
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Transactional sequential pipelines over a shared COW workdir — the Rust-core slice of RFC #65.
Rewritten after your design review; the previous body described a different shape and is no longer accurate.
Fair warning on the size of this: your five items turned out to be more than I bargained for. Item 1 was one bug in one place, or so it looked — fixing it surfaced the same failure class inside the fix for item 3, and fixing that surfaced it again at a cancellation point one
awaitdeeper. Turtles all the way down, three levels of them, and each level looked closed until the next probe went in. That is why the diff is larger than a five-item review has any right to produce, and why a good part of it is tests.What it is now
A dedicated
Transactiontype (crates/sandlock-core/src/transaction.rs), not a second method onPipeline. Stages run sequentially over one shared COW upper; the workdir is untouched until the commit, which is all-or-nothing.Your five points
1.
Dropdestroying data on a failed commit.commit()now entersPreserved(MergeInterrupted)before the first destructive operation, so every?inside the merge returns withDropdisarmed and the upper intact. The blast radius of addingDroptoSeccompCowBranchis stated in its rustdoc rather than framed as a leak fix, and an explicitBranchAction::Keepis no longer overridden — an abandoned sandbox that asked to keep its branch keeps it (set_keep_if_abandoned).Two more instances of the same failure class turned up while chasing this one, both fixed and both with tests that fail without the fix: the commit-lock timeout path returned
Errwhile the branch was stillOpen, and — the one I would have missed by reading exit paths only — cancelling the future during the lock wait dropped anOpenbranch. Cancellation is not an exit path, so no code runs; the fix moves the whole commit phase ontospawn_blockingwith the branch moved in, which makes the loss structurally impossible and also gets the walk + per-file copy + fsyncs off the async executor.2. Wrong type.
run_transactionalis gone fromPipeline.Transactionhas noBitOrand is built only from an explicit stage list, andPipeline::stagesis now private (into_stages()) soTransaction::new(chain.stages)no longer compiles — the footgun is closed by construction rather than by a doc comment. Its rustdoc states the execution model in the first paragraph: sequential, no inter-stage pipes, stdio inherited, data exchanged through the shared workdir.I did not build
Transaction::new(sandboxes)from the RFC's end state: aSandboxcarries no argv, so the coordinator needsStage { sandbox, args }. If you want that name reserved for the eventual signature, say so and this becomesTransaction::from_stages.3. Not first-committer-win. Correct, and the name is gone. What the lock provides is mutual exclusion between merges, stated as such in the code. It is now wait-with-timeout rather than fail-fast, and losing the wait is no longer
Ok(TxnOutcome { committed: false })— it isTxnError::Conflict, so a caller that ignores the distinction cannot silently lose a pipeline's work. The stale-base lost update you described is unchanged and documented: this is concurrency safety, not serializable isolation.4. Phase 1 acceptance. You are right that this does not close Phase 1, and I am not claiming it does — see the scope section below.
5. Error typing.
AbortReasonis an enum (StageFailed { index, status },TimedOut,DryRun);TxnErrorseparates the commit channel (Branch,CommitLock,Conflict,Merge,CommitAbandoned) with distinct exit codes, and flattens toSandboxRuntimeError::Branchrather thanChild(String). No string matching required to tell a stage failure from a commit failure from a conflict.Scope: this is Phase 1a
Still owed, not done here:
commit()/abort()as a public surface (onlyrun/dry_runship), the Python and Go bindings, the Python end-to-end test the RFC's acceptance names, and tooling to act on a preserved branch —list_preserveddiscovers them, but there is norecover()/discard()primitive yet, so a preserved upper is currently the operator's problem.TxnOutcomedoes carry the RFC'schangesfield, anddry_run()exists.Preserved storage
A preserved branch now records its deletions in the on-disk marker alongside the upper, because a recovery that replays only the upper would resurrect every file the transaction deleted — and
TxnError::Mergetells the operator that recovering that storage is how the transaction gets finished. Two limits are documented rather than hidden: the deletion set is written at preserve time, so a recovery may re-apply a deletion that has since landed (a no-op), and there is no recovery command yet.Tests
seccomp.rs64 → 106 unit tests,transaction.rs27,test_transaction.rs38, plus additions totest_cow.rs; suites on this branch are 599 lib / 342 integration.Every test was required to fail against broken code before being counted — the ones that could not were rewritten or dropped. Two that shipped earlier in this branch turned out to pass vacuously (a lock-contention test whose contention had already expired, and a latency test whose only assertion was a wall-clock bound that a 20ms regression could not trip); both are fixed.
One caveat you will see in CI:
test_cow::test_seccomp_cow_exec_packed_argv_relocation(be654d7, onmain) flakes under compile load — roughly 2 runs in 3 here, clean in isolation. Not from this branch, but it will red this PR intermittently.Out of scope, filed separately
Writing the COW merge tests turned up four defects in the COW layer that predate this work: #158 (copy-up of a FIFO wedges the supervisor indefinitely), #159 (whiteouts are not recursive, so a deleted directory's contents stay readable and can be republished by
commit()), #160 (renaming a lower-only directory silently destroys its contents), #161 (rmdiron a non-empty directory succeeds where the kernel givesENOTEMPTY). None are touched here.Refs #65.