From a57a1305e7f327605b4c9c70906bb8907e1d3af5 Mon Sep 17 00:00:00 2001 From: "K. Isom" Date: Wed, 23 Sep 2026 15:33:23 -0700 Subject: [PATCH] M3b plan: follow-up tasks 14 to 17 for the review's lower findings 14 moves the pipe handling out of container.rs (a pure move, replayed on its own); 15 starts threads with Builder and bounds output collection with a 2 s grace period; 16 escapes container errors in the log and fixes two texts; 17 fixes toolkit's thread start, casts and the egress-proxy form. Each checked against a reference, which is not kept. Tips T24 to T26 from this run. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/implementer-lessons.md | 3 + docs/implementer-log.md | 8 +- docs/plans/M3b/14-brokerd-pipes-module.md | 63 ++++++++ docs/plans/M3b/15-brokerd-pipes-grace.md | 152 ++++++++++++++++++ docs/plans/M3b/16-brokerd-log-escaping.md | 61 +++++++ docs/plans/M3b/17-toolkit-nits.md | 69 ++++++++ docs/plans/M3b/README.md | 10 +- .../crates/brokerd/tests/container_grace.rs | 86 ++++++++++ .../crates/brokerd/tests/container_log.rs | 128 +++++++++++++++ .../M3b/files/crates/brokerd/tests/notices.rs | 80 +++++++++ .../files/crates/toolkit/tests/egress_form.rs | 42 +++++ 11 files changed, 695 insertions(+), 7 deletions(-) create mode 100644 docs/plans/M3b/14-brokerd-pipes-module.md create mode 100644 docs/plans/M3b/15-brokerd-pipes-grace.md create mode 100644 docs/plans/M3b/16-brokerd-log-escaping.md create mode 100644 docs/plans/M3b/17-toolkit-nits.md create mode 100644 docs/plans/M3b/files/crates/brokerd/tests/container_grace.rs create mode 100644 docs/plans/M3b/files/crates/brokerd/tests/container_log.rs create mode 100644 docs/plans/M3b/files/crates/brokerd/tests/notices.rs create mode 100644 docs/plans/M3b/files/crates/toolkit/tests/egress_form.rs diff --git a/docs/implementer-lessons.md b/docs/implementer-lessons.md index d665869..e38de88 100644 --- a/docs/implementer-lessons.md +++ b/docs/implementer-lessons.md @@ -58,6 +58,9 @@ How it is used: | T21 | When two components must agree on a set (which files are the log, which names are ids), give the tests one case that walks both. Each half was tested alone and they still disagreed. | M3a finding 2: `brokerd` accepted `2026-0x-18.jsonl` as a log file and `bxctl audit verify` ignored it, calling the log `ok` while leaving out half its records. | | T22 | List the fail-closed states a task creates, with their runbook anchors, in the task itself. A script can check that a pointer names an existing entry; nothing can check for a pointer that was never written. | M3a finding 3: four startup failures in `serve` and `main` print no pointer, and the spec's own list of pointers omitted them. | | T23 | A test that writes a script and then runs it must hold a lock shared by every test in its binary that starts a process. Otherwise another test's fork can hold the new script open for writing at that moment, and running it fails with "Text file busy" (ETXTBSY), about once in seven runs. Give the lock in the support file and say why. | M3b plan checks: the fake `curl` tests failed 8 times in 40 until every forking test took `serial()`; then 0 in 60. | +| T24 | A task that changes a `Cargo.toml` must stage `Cargo.lock` in its `git add` line. Better, put `Cargo.lock` in every task's `git add`; it is a no-op when unchanged. | M3b task 04: committed correctly, left the lock behind, and the driver stopped on an unclean tree. | +| T25 | Size a task by the largest function the model must hold in one turn, not by the task. Ornith writes one function with a few branches well; a function with half a dozen branches and threads (M3b's `run`) it plans in its head until the turn runs out, with nothing written. Give such a task a compiling skeleton with the big function already written as glue over small `todo!()` helpers, and say to fill one at a time with `cargo check` between. | M3b task 11: four sessions. Two wrote nothing; a whole-file skeleton got five of six functions; the finer skeleton finished it in ten minutes, and tasks 12 and 13 followed without a stop. | +| T26 | Replay each task's end state on its own, **and** read the task file against the reference for anything the reference has that the task does not ask for. The replay proves the tests can pass; only the reading finds a field the reference reads through a getter the task never mentions. | M3b task 11: the task stored `egress_dir` without the reference's getter, so the field was never read and clippy failed; the replay passed because the reference had the getter. | ## What worked and should be kept diff --git a/docs/implementer-log.md b/docs/implementer-log.md index e646e0e..9fe57d3 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -463,11 +463,11 @@ claim of the milestone held. |---|---|---|---|---| | 1 | medium | plan (task 06, spec 6) | `curl` expands globs in the URL (documented behaviour: `https://example.com/[1-3]` is three requests; the review first wrote "seen on straylight", but only the last response was seen, not counted), so `[1-99999999]` would hammer an allowed host and buffer every body. After the fix, the same URL is one request, counted on straylight. The fixed argument list lacks `--globoff`. | Fixed by the design model (`--globoff`, and `--disable` first) | | 2 | low | plan (task 10, spec 6) | `podman run` has no `--pull=never`: with an image that is not loaded, Podman tries to pull it (seen on straylight). Here the name starts `localhost/`, so the pull fails, but a pull is unlisted egress and the call should fail at once. | Fixed by the design model (`--pull=never`); a missing image now fails in 46 ms, no pull | -| 3 | low | implementer (11), plan | `std::thread::spawn` in `container.rs` (three) and `toolkit/src/fetch.rs` panics if a thread cannot be made; after the spawn of the container, a panic drops the `Child` without `podman kill`, so the container runs on without its limit. The reference had the same; the task did not say. | Follow-up | -| 4 | low | implementer (11) | The time limit bounds the wait, not the joins after it: if another process held the pipes, `run` would block until it let go (shown with a fake `podman` without `exec`: 6 s for a 0.3 s limit). Real Podman released them at the kill (8 s limit, 8 s seen). | Follow-up: join with a grace deadline | -| 5 | low | implementer (11) | Podman's standard error, which the tool can write to, goes into `brokerd`'s log unescaped, so a tool can forge log lines (a fake runbook pointer). It never reaches a `RunError`. | Follow-up: escape | +| 3 | low | implementer (11), plan | `std::thread::spawn` in `container.rs` (three) and `toolkit/src/fetch.rs` panics if a thread cannot be made; after the spawn of the container, a panic drops the `Child` without `podman kill`, so the container runs on without its limit. The reference had the same; the task did not say. | Task 15 (brokerd), task 17 (toolkit) | +| 4 | low | implementer (11) | The time limit bounds the wait, not the joins after it: if another process held the pipes, `run` would block until it let go (shown with a fake `podman` without `exec`: 6 s for a 0.3 s limit). Real Podman released them at the kill (8 s limit, 8 s seen). | Task 15 | +| 5 | low | implementer (11) | Podman's standard error, which the tool can write to, goes into `brokerd`'s log unescaped, so a tool can forge log lines (a fake runbook pointer). It never reaches a `RunError`. | Task 16 | | 6 | low | spec (section 5) | `is_public` passes local-use NAT64 `64:ff9b:1::/48` and 6to4 `2002::/16` with a private IPv4 inside. Neither is in use on straylight. A host with its own public address would be reachable by an allowed name that points at it; straylight has none (its addresses are LAN, tailnet and Tailscale's ULA, all refused). | Spec, when next touched | -| 7 | nit | implementer | The runtime notice lacks its `brokerd:` prefix; two config messages use `{}` where the task gave `{:?}`; `chunk[..take]` where the skeleton said `get`; `egress-proxy` accepts trailing arguments; `as u64` on two constants. | When next touched | +| 7 | nit | implementer | The runtime notice lacks its `brokerd:` prefix; two config messages use `{}` where the task gave `{:?}`; `chunk[..take]` where the skeleton said `get`; `egress-proxy` accepts trailing arguments; `as u64` on two constants. | Tasks 16 and 17 (all but the indexing, gone with task 15) | What was good: the proxy handshake reads exactly what the protocol gives against one deadline, has no panic path, and tries only public addresses (the independent review probed it and found it diff --git a/docs/plans/M3b/14-brokerd-pipes-module.md b/docs/plans/M3b/14-brokerd-pipes-module.md new file mode 100644 index 0000000..1cc407a --- /dev/null +++ b/docs/plans/M3b/14-brokerd-pipes-module.md @@ -0,0 +1,63 @@ +# M3b task 14: move the pipe handling into its own module + +**Branch:** `m3b` (run `git switch m3b`; `git status --short` must be empty, otherwise stop) +**Commit subject:** `brokerd: move the container's pipe handling into pipes.rs` + +## Goal + +`container.rs` is 432 lines, and the next two tasks add to the pipe handling. Before they do, move +that code into its own file, **without changing what it does**. This is a pure move: every test +passes before and after, unchanged. + +## Files + +- Create: `crates/brokerd/src/pipes.rs` +- Modify: `crates/brokerd/src/container.rs`, `crates/brokerd/src/lib.rs`, `docs/implementer-log.md` +- No test files change. + +## What moves + +From `container.rs`, cut these three items, **bodies unchanged**, and paste them into `pipes.rs`: + +1. `struct Io { … }` (the three `JoinHandle` fields) → `pub(crate) struct Io`. +2. `impl Io { fn start(…) … fn finish(…) … }` → the same, with `pub(crate) fn start` and + `pub(crate) fn finish`. +3. `fn read_capped(…)` → `pub(crate) fn read_capped`. + +Their doc comments move with them. `pipes.rs` starts with a module doc comment: + +```rust +//! The three pipes of one container: its arguments go in on standard input, its output and errors +//! come back, each on its own thread so no pipe can block another. +``` + +and the `use` lines those items need (`std::io::{Read, Write}`, `std::process::Child`, +`std::thread::JoinHandle`). `Io::start` uses `STDERR_KEPT`: leave that constant in `container.rs` +(the tests import it from there) and write `crate::container::STDERR_KEPT` in `pipes.rs`. + +In `lib.rs`: `pub mod pipes;`, between `pub mod ledger;` and `pub mod podman;`. + +In `container.rs`: add `use crate::pipes::Io;`, and remove the `use` lines the compiler then says +are unused. Nothing else in `container.rs` changes. + +## Steps + +- [ ] **1. Branch.** `git switch m3b`. +- [ ] **2. Move the three items** as above. Run `cargo fmt --all`. +- [ ] **3. Check it builds.** `cargo check -p brokerd`, then + `cargo clippy -p brokerd --all-targets -- -D warnings`. Fix only imports. +- [ ] **4. See every test pass, unchanged.** + `cargo test -p brokerd --test container --test container_egress --test serve_runner`. Expected: + 11, 6 and 2 passed. +- [ ] **5. Check it moved.** `grep -n "struct Io\|fn read_capped" crates/brokerd/src/container.rs` + prints nothing; `wc -l crates/brokerd/src/container.rs` is under 380. +- [ ] **6. Run the gate.** `make gate`. Expected last line: `gate: ok`. +- [ ] **7. Log and commit.** `git add crates/brokerd docs/implementer-log.md Cargo.lock && git commit` + +## Done when + +- `container.rs` no longer holds `Io` or `read_capped`; `make gate` prints `gate: ok`. + +## Stop and report if + +- Any test fails after the move: a pure move cannot break one, so report instead of changing code. diff --git a/docs/plans/M3b/15-brokerd-pipes-grace.md b/docs/plans/M3b/15-brokerd-pipes-grace.md new file mode 100644 index 0000000..ec8de7a --- /dev/null +++ b/docs/plans/M3b/15-brokerd-pipes-grace.md @@ -0,0 +1,152 @@ +# M3b task 15: no thread panics, and a grace period for the output + +**Branch:** `m3b` (run `git switch m3b`; `git status --short` must be empty, otherwise stop) +**Commit subject:** `brokerd: start pipe threads safely and collect output within a grace period` + +## Goal + +Two defects from the M3b review, both in `pipes.rs` (task 14): + +- **Finding 3.** `std::thread::spawn` panics when the system cannot make a thread. The panic drops + the `Child` without stopping the container, which then runs on without its time limit. Use + `std::thread::Builder`, which returns an error instead, and stop the container on that error. +- **Finding 4.** `Io::finish` joins the readers with no limit. Each reader stops only at its pipe's + end, so if any other process still holds a pipe, `brokerd` waits for it: the call's time limit + does not bound it. After the container has ended, give the output a **grace period** of 2 s, + then abandon what has not ended. + +## Files + +- Copy: `crates/brokerd/tests/container_grace.rs` +- Modify: `crates/brokerd/src/pipes.rs`, `crates/brokerd/src/container.rs`, + `docs/implementer-log.md` + +## `pipes.rs`, new shape + +```rust +use std::sync::mpsc::{self, Receiver, RecvTimeoutError}; +use std::thread::Builder; +use std::time::{Duration, Instant}; + +/// How long, after the container has ended, its output may take to reach its end. +pub const GRACE: Duration = Duration::from_secs(2); + +/// What came back. +pub(crate) struct Finished { + pub out: Vec, + pub truncated: bool, + pub err: String, + /// A pipe had not ended by the grace deadline: something outside the container holds it. + pub open: bool, +} + +pub(crate) struct Io { + stdout: Option, bool)>>, + stderr: Option, bool)>>, +} + +impl Io { + pub(crate) fn start(child: &mut Child, input: String, cap: usize, err_cap: usize) + -> std::io::Result; + pub(crate) fn finish(self, grace: Duration) -> Finished; +} + +pub(crate) fn read_capped(from: impl Read, cap: usize) -> (Vec, bool); // as now +``` + +Write each function in turn, running `cargo check -p brokerd` after each. + +### `Io::start` + +1. If `child.stdin.take()` is `Some(stdin)`: start a thread with `Builder::new().spawn(…)` that + writes `input` with `write_all` (ignore its error) and then drops `stdin`. **Do not keep its + handle and never join it**: it ends when the input is written or the pipe breaks. A spawn error + → return it (`?`). +2. For standard output with `cap`, and then standard error with `err_cap`: if the pipe is + `Some`, make a channel (`mpsc::channel()`), start a thread with `Builder::new().spawn(…)` that + runs `read_capped(pipe, cap)` and `send`s the result (ignore a send error), and keep the + `Receiver`. A spawn error → return it. Write a small private function + `fn reader(pipe: impl Read + Send + 'static, cap: usize) -> std::io::Result, bool)>>` + and use it for both. +3. `Ok(Io { stdout, stderr })`. + +### `Io::finish(grace)` + +One deadline for both readers: `let until = Instant::now() + grace;`. For each receiver (standard +output first), a small private function `collect(rx, until) -> (Vec, bool, bool)`: + +- `None` (no pipe) → `(empty, false, false)`. +- `rx.recv_timeout(until.saturating_duration_since(Instant::now()))`: + - `Ok((bytes, truncated))` → `(bytes, truncated, false)`; + - `Err(RecvTimeoutError::Timeout)` → `(empty, false, true)`: **open**; + - `Err(RecvTimeoutError::Disconnected)` (the reader panicked) → `(empty, false, false)`. + +Return `Finished { out, truncated, err: String::from_utf8_lossy(&err_bytes).into_owned(), +open: stdout_open || stderr_open }`. + +### `read_capped` + +Unchanged in what it does, but no indexing and no subtraction that could wrap: keep +`let take = n.min(cap.saturating_sub(kept.len()));`, copy with +`kept.extend_from_slice(chunk.get(..take).unwrap_or_default());`, and set `truncated` when +`take < n`. Retry `ErrorKind::Interrupted`; stop on any other error. + +## `container.rs` + +1. A new constant after `TIMED_OUT`: + `pub const OUTPUT_OPEN: &str = "the tool left its output open";` +2. `use crate::pipes::{GRACE, Io};` +3. In `run_container`, replace the lines from `let io = Io::start(…)` to the end of the function + with exactly this: + +```rust +let io = match Io::start(&mut child, input, cap, STDERR_KEPT) { + Ok(io) => io, + Err(e) => { + // The container may be running: stop it before answering. + self.podman(&["kill", name]); + self.podman(&["rm", "-f", name]); + let _ = child.kill(); + let _ = child.wait(); + (self.log)(&format!("brokerd: cannot start a thread for {name}: {e}\n{RUNBOOK}")); + return Err(RunError::Unavailable(CANNOT_START.to_string())); + } +}; +let status = self.wait(&mut child, name, started, limit); +let done = io.finish(GRACE); +if done.open && status.is_some() { + (self.log)(&format!( + "brokerd: {name} ended but something still holds its output; it was abandoned" + )); + return Err(RunError::Failed(OUTPUT_OPEN.to_string())); +} +self.answer(name, status, done.out, done.truncated, &done.err) +``` + +When the time limit was passed (`status` is `None`), an open pipe changes nothing: the answer is +still `TIMED_OUT`, from `answer`. + +Also: `grep -n "thread::spawn" crates/brokerd/src/` must print nothing when you are done. + +## Steps + +- [ ] **1. Copy.** `git switch m3b`, then + `cp docs/plans/M3b/files/crates/brokerd/tests/container_grace.rs crates/brokerd/tests/` +- [ ] **2. See it fail.** `cargo test -p brokerd --test container_grace`. Expected: it does not + compile (`OUTPUT_OPEN` and `pipes::GRACE` do not exist). +- [ ] **3. Write `pipes.rs`, one function at a time, then `container.rs`.** Run `cargo fmt --all`. +- [ ] **4. See it pass.** `cargo test -p brokerd --test container_grace`. Expected: 4 passed, in + about 4 s (two tests wait out the grace period on purpose). Then + `cargo test -p brokerd --test container --test container_egress`: 11 and 6 passed. Run all three + five times. +- [ ] **5. Run the gate.** `make gate`. Expected last line: `gate: ok`. +- [ ] **6. Log and commit.** `git add crates/brokerd docs/implementer-log.md Cargo.lock && git commit` + +## Done when + +- The three suites pass five times running; no `thread::spawn` in `crates/brokerd/src/`; `make + gate` prints `gate: ok`. + +## Stop and report if + +- A test takes much longer than stated, or passes only sometimes. diff --git a/docs/plans/M3b/16-brokerd-log-escaping.md b/docs/plans/M3b/16-brokerd-log-escaping.md new file mode 100644 index 0000000..af798d3 --- /dev/null +++ b/docs/plans/M3b/16-brokerd-log-escaping.md @@ -0,0 +1,61 @@ +# M3b task 16: escape what the container writes before logging it, and three small texts + +**Branch:** `m3b` (run `git switch m3b`; `git status --short` must be empty, otherwise stop) +**Commit subject:** `brokerd: escape container errors in the log; prefix and quote two messages` + +## Goal + +**M3b review finding 5.** What a container writes on standard error (and Podman's own errors, which +the tool can influence) goes into `brokerd`'s log as it is, newlines included. A tool can therefore +write a line that looks like one of `brokerd`'s own, for example a fake +`see docs/runbook.md#…` pointer. Log it with Rust's debug formatting (`{err:?}`), which quotes it and +writes every newline and control character as an escape: one event stays one entry. + +And three small texts from the review (finding 7): the runtime notice lacks its `brokerd:` prefix, +and two `[runner]` errors should quote the bad value. + +## Files + +- Copy: `crates/brokerd/tests/container_log.rs`, `crates/brokerd/tests/notices.rs` +- Modify: `crates/brokerd/src/container.rs`, `crates/brokerd/src/main.rs`, + `crates/brokerd/src/config.rs`, `docs/implementer-log.md` + +## The changes, exactly + +In `container.rs`, `answer`, three log calls become: + +| Case | Log call | +|---|---| +| exit 2 | `(self.log)(&format!("brokerd: {name}: the tool could not run: {err:?}"));` | +| exit 125 to 127 | `(self.log)(&format!("brokerd: podman could not start {name}: {err:?}\n{RUNBOOK}"));` | +| any other ending | `(self.log)(&format!("brokerd: container {name} exited {status}: {err:?}"));` | + +In `start_egress`, where the proxy's `podman run -d` does not succeed, the standard error in the log +call becomes `{stderr:?}` in the same way (the rest of that line, and its `\n{RUNBOOK}`, stay). + +The only raw `\n` left in any of these is the one before `RUNBOOK`, which is ours. + +In `main.rs`: the line that prints the runtime notice becomes `eprintln!("brokerd: {runtime_notice}");`. + +In `config.rs`: in the two messages `[runner] image is {}; …` and `[runner] memory is {}; …`, the +`{}` becomes `{:?}`, so the value is shown in quotes. + +## Steps + +- [ ] **1. Copy.** `git switch m3b`, then + `cp docs/plans/M3b/files/crates/brokerd/tests/container_log.rs docs/plans/M3b/files/crates/brokerd/tests/notices.rs crates/brokerd/tests/` +- [ ] **2. See them fail.** `cargo test -p brokerd --no-fail-fast --test container_log --test notices`. + Expected: 4 and 2 fail. +- [ ] **3. Make the changes.** Run `cargo fmt --all`. +- [ ] **4. See them pass.** `cargo test -p brokerd --test container_log --test notices --test serve_runner --test config_runner`. + Expected: 4, 2, 2 and 7 passed. +- [ ] **5. Run the gate.** `make gate`. Expected last line: `gate: ok`. +- [ ] **6. Log and commit.** `git add crates/brokerd docs/implementer-log.md Cargo.lock && git commit` + +## Done when + +- The four suites pass; `make gate` prints `gate: ok`. + +## Stop and report if + +- A test wants container output anywhere but the log. diff --git a/docs/plans/M3b/17-toolkit-nits.md b/docs/plans/M3b/17-toolkit-nits.md new file mode 100644 index 0000000..f241154 --- /dev/null +++ b/docs/plans/M3b/17-toolkit-nits.md @@ -0,0 +1,69 @@ +# M3b task 17: three small fixes in `toolkit` + +**Branch:** `m3b` (run `git switch m3b`; `git status --short` must be empty, otherwise stop) +**Commit subject:** `toolkit: no thread panic in http_fetch, no casts, exact egress-proxy form` + +## Goal + +Three findings from the M3b review (3 and 7), all small: + +1. `fetch.rs` starts its standard-error reader with `std::thread::spawn`, which panics when the + system cannot make a thread. Use `std::thread::Builder`. +2. `input.rs` and `files.rs` compute a byte limit with an `as u64` cast, which AGENTS forbids. +3. `toolkit egress-proxy --socket --allow extra` is taken as the proxy form and + listens. The form is **exactly** five words; anything longer is not it. + +## Files + +- Copy: `crates/toolkit/tests/egress_form.rs` +- Modify: `crates/toolkit/src/fetch.rs`, `crates/toolkit/src/input.rs`, + `crates/toolkit/src/files.rs`, `crates/toolkit/src/main.rs`, `docs/implementer-log.md` + +## The changes, exactly + +**1. `fetch.rs`.** Replace +`let stderr_handle = std::thread::spawn(move || read_capped(stderr_reader));` with: + +```rust +// `Builder`, not `spawn`, which panics when the system refuses a thread. +let stderr_handle = match std::thread::Builder::new().spawn(move || read_capped(stderr_reader)) { + Ok(handle) => handle, + Err(e) => { + let _ = child.kill(); + let _ = child.wait(); + return Outcome::tool_error(format!("http_fetch: cannot start a thread: {e}")); + } +}; +``` + +**2. `input.rs` and `files.rs`.** Replace `.take(MAX_INPUT as u64 + 1)` with +`.take(u64::try_from(MAX_INPUT).map_or(u64::MAX, |n| n.saturating_add(1)))`, and the same for +`MAX_READ` in `files.rs`. + +**3. `main.rs`.** In `parse_egress_proxy`, the first check becomes: + +```rust +// Exactly five words: a longer list is not this form, and goes to the tool form (exit 2). +if args.len() != 5 || args.first()?.as_bytes() != b"egress-proxy" { + return None; +} +``` + +## Steps + +- [ ] **1. Copy.** `git switch m3b`, then + `cp docs/plans/M3b/files/crates/toolkit/tests/egress_form.rs crates/toolkit/tests/` +- [ ] **2. See it fail.** `cargo test -p toolkit --test egress_form`. Expected: 1 fails after about + 3 s ("it is listening"). +- [ ] **3. Make the three changes.** Run `cargo fmt --all`. +- [ ] **4. See it pass.** `cargo test -p toolkit`. Expected: every suite passes; `egress_form` 1 + passed quickly. +- [ ] **5. Check.** `grep -n "thread::spawn\| as u64" crates/toolkit/src/` prints nothing. +- [ ] **6. Run the gate.** `make gate`. Expected last line: `gate: ok`, with about 649 tests. +- [ ] **7. Log and commit.** `git add crates/toolkit docs/implementer-log.md Cargo.lock && git commit` + +This is the last follow-up task of M3b. Stop after the commit. + +## Done when + +- `cargo test -p toolkit` passes; the grep prints nothing; `make gate` prints `gate: ok`. diff --git a/docs/plans/M3b/README.md b/docs/plans/M3b/README.md index 82b297c..86a9ec7 100644 --- a/docs/plans/M3b/README.md +++ b/docs/plans/M3b/README.md @@ -51,8 +51,12 @@ it at that task's end state, then the reference was deleted so it cannot be read | 11 | `11-brokerd-container.md` | the Podman runtime | `container.rs`, `support/fake_podman.rs` | reference; 20 runs clean | | 12 | `12-brokerd-egress.md` | the proxy's lifecycle for `http_fetch` | `container_egress.rs` | reference; 20 runs clean | | 13 | `13-brokerd-serve-runner.md` | `brokerd serve` uses the runtime | `serve_runner.rs` | reference | +| 14 | `14-brokerd-pipes-module.md` | review follow-up: the pipe handling moves to `pipes.rs`, unchanged | none new | replayed on its own | +| 15 | `15-brokerd-pipes-grace.md` | review findings 3 and 4: `thread::Builder`; a 2 s grace period for the output | `container_grace.rs` | reference; 5 runs clean | +| 16 | `16-brokerd-log-escaping.md` | review findings 5 and 7: container errors escaped in the log; two texts | `container_log.rs`, `notices.rs` | reference; red without it | +| 17 | `17-toolkit-nits.md` | review findings 3 and 7 in `toolkit` | `egress_form.rs` | reference; red without it | -At the end: `make gate` prints `gate: ok` with about 638 tests. +At the end of task 13: about 638 tests; at the end of task 17: about 649. ## Changes during the run @@ -85,8 +89,8 @@ At the end: `make gate` prints `gate: ok` with about 638 tests. - 2026-09-23, after the review: two plan defects found on straylight were fixed by the design model: `curl` gains `--globoff` (and a leading `--disable`), since `[1-3]` in a URL made three requests; both `podman run`s gain `--pull=never`, since a missing image made Podman try to pull. - The given `fetch.rs` and the six golden files changed with them. Follow-up tasks 14 to 16 are for - the implementer. + The given `fetch.rs` and the six golden files changed with them. Follow-up tasks 14 to 17 are for + the implementer; run them with `tools/run-plan.sh docs/plans/M3b 14`. ## Running it diff --git a/docs/plans/M3b/files/crates/brokerd/tests/container_grace.rs b/docs/plans/M3b/files/crates/brokerd/tests/container_grace.rs new file mode 100644 index 0000000..09cb7ab --- /dev/null +++ b/docs/plans/M3b/files/crates/brokerd/tests/container_grace.rs @@ -0,0 +1,86 @@ +//! After a container ends, its output is collected within a grace period, never waited on for +//! ever: something outside the container that still holds a pipe must not hold `brokerd` (M3b +//! review finding 4). Against a fake `podman` whose shell leaves a background `sleep` holding the +//! pipes, which real Podman does not do. Do not edit. + +#[path = "support/build.rs"] +mod build; +#[path = "support/fake_podman.rs"] +mod fake_podman; + +use std::time::{Duration, Instant}; + +use brokerd::container::{OUTPUT_OPEN, Podman, TIMED_OUT}; +use brokerd::pipes::GRACE; +use brokerd::policy::{Outcome, SessionState, decide}; +use brokerd::runner::run; +use build::{grant, now, read, set}; +use fake_podman::{Fake, Lines, serial}; +use proto::{Mode, ToolResponse}; + +fn call(fake: &Fake, extra: &str, log: &Lines) -> (ToolResponse, Duration) { + let podman = Podman::new(fake.runner(extra), fake.dir.join("egress"), log.sink()); + let grants = set(vec![grant("notes", "read_file", Mode::Auto).paths(&["/n"])]); + let decision = match decide(read("/n/a"), &grants, SessionState::default(), now()) { + Outcome::Allowed(d) => d, + other => panic!("not allowed: {other:?}"), + }; + let started = Instant::now(); + let got = run(decision, &podman); + (got, started.elapsed()) +} + +#[test] +fn the_grace_period_is_two_seconds() { + assert_eq!(GRACE, Duration::from_secs(2)); +} + +#[test] +fn output_held_open_after_the_container_ended_is_abandoned_after_the_grace_period() { + let _s = serial(); + // The shell exits at once; the background sleep keeps standard output and error open. + let fake = Fake::new("grace-open", "cat > /dev/null; printf ok; sleep 6 & exit 0"); + let log = Lines::default(); + let (got, took) = call(&fake, "", &log); + assert_eq!( + got, + ToolResponse::Failed { + message: OUTPUT_OPEN.to_string() + } + ); + assert!(took >= GRACE, "{took:?}"); + assert!(took < GRACE + Duration::from_secs(2), "{took:?}"); + assert!(log.all().contains("abandoned"), "{}", log.all()); +} + +#[test] +fn a_tool_past_its_limit_is_answered_within_the_grace_period_even_if_its_pipes_stay_open() { + let _s = serial(); + // No `exec`: killing the shell leaves the sleep holding the pipes. + let fake = Fake::new("grace-slow", "cat > /dev/null; sleep 6"); + let log = Lines::default(); + let (got, took) = call(&fake, "read_file_ms = 300", &log); + assert_eq!( + got, + ToolResponse::Failed { + message: TIMED_OUT.to_string() + } + ); + assert!( + took < Duration::from_millis(300) + GRACE + Duration::from_secs(2), + "{took:?}" + ); +} + +#[test] +fn a_tool_that_ends_normally_is_not_slowed_by_the_grace_period() { + let _s = serial(); + let fake = Fake::new("grace-ok", "cat > /dev/null; printf done; exit 0"); + let log = Lines::default(); + let (got, took) = call(&fake, "", &log); + assert!( + matches!(&got, ToolResponse::Result { content, .. } if content == "done"), + "{got:?}" + ); + assert!(took < Duration::from_secs(1), "{took:?}"); +} diff --git a/docs/plans/M3b/files/crates/brokerd/tests/container_log.rs b/docs/plans/M3b/files/crates/brokerd/tests/container_log.rs new file mode 100644 index 0000000..dd5990b --- /dev/null +++ b/docs/plans/M3b/files/crates/brokerd/tests/container_log.rs @@ -0,0 +1,128 @@ +//! What a tool or Podman writes on standard error reaches `brokerd`'s log escaped, one entry per +//! event: it cannot start a line of its own or forge a runbook pointer (M3b review finding 5). +//! Do not edit. + +#[path = "support/build.rs"] +mod build; +#[path = "support/fake_podman.rs"] +mod fake_podman; + +use brokerd::container::{CANNOT_START, COULD_NOT_RUN, Podman, UNEXPECTED}; +use brokerd::policy::{Outcome, SessionState, decide}; +use brokerd::runner::run; +use build::{fetch, grant, now, read, set}; +use fake_podman::{Fake, Lines, serial}; +use proto::{Mode, ToolRequest, ToolResponse}; + +const FORGED: &str = "real line\nbrokerd: forged\nsee docs/runbook.md#grants-invalid"; + +fn call(fake: &Fake, req: ToolRequest, grants: Vec, log: &Lines) -> ToolResponse { + let podman = Podman::new(fake.runner(""), fake.dir.join("egress"), log.sink()); + let decision = match decide(req, &set(grants), SessionState::default(), now()) { + Outcome::Allowed(d) => d, + other => panic!("not allowed: {other:?}"), + }; + run(decision, &podman) +} + +/// No entry holds the forged text as lines of its own; the one that carries it has it escaped. +fn escaped(log: &Lines) { + let entries = log.0.lock().unwrap().clone(); + for entry in &entries { + assert!(!entry.contains("\nbrokerd: forged"), "raw: {entry:?}"); + assert!( + !entry.contains("\nsee docs/runbook.md#grants-invalid"), + "raw: {entry:?}" + ); + } + assert!( + entries + .iter() + .any(|e| e.contains(r"real line\nbrokerd: forged")), + "the error is still logged, escaped: {entries:?}" + ); +} + +fn notes() -> Vec { + vec![grant("notes", "read_file", Mode::Auto).paths(&["/n"])] +} + +#[test] +fn a_tool_that_could_not_run() { + let _s = serial(); + let fake = Fake::new( + "log-2", + &format!("cat > /dev/null; printf '{FORGED}' >&2; exit 2"), + ); + let log = Lines::default(); + assert_eq!( + call(&fake, read("/n/a"), notes(), &log), + ToolResponse::Failed { + message: COULD_NOT_RUN.to_string() + } + ); + escaped(&log); +} + +#[test] +fn a_container_podman_could_not_start_keeps_its_one_real_pointer() { + let _s = serial(); + let fake = Fake::new( + "log-125", + &format!("cat > /dev/null; printf '{FORGED}' >&2; exit 125"), + ); + let log = Lines::default(); + assert_eq!( + call(&fake, read("/n/a"), notes(), &log), + ToolResponse::Failed { + message: CANNOT_START.to_string() + } + ); + escaped(&log); + let entries = log.0.lock().unwrap().clone(); + assert!( + entries + .iter() + .any(|e| e.ends_with("\nsee docs/runbook.md#runner-unavailable")), + "{entries:?}" + ); +} + +#[test] +fn an_unexpected_ending() { + let _s = serial(); + let fake = Fake::new( + "log-3", + &format!("cat > /dev/null; printf '{FORGED}' >&2; exit 3"), + ); + let log = Lines::default(); + assert_eq!( + call(&fake, read("/n/a"), notes(), &log), + ToolResponse::Failed { + message: UNEXPECTED.to_string() + } + ); + escaped(&log); +} + +#[test] +fn a_proxy_podman_could_not_start() { + let _s = serial(); + let body = + format!("if [ \"$2\" = -d ]; then printf '{FORGED}' >&2; exit 125; fi; cat > /dev/null"); + let fake = Fake::new("log-egress", &body); + let log = Lines::default(); + let got = call( + &fake, + fetch("https://example.com/"), + vec![grant("web", "http_fetch", Mode::Auto).hosts(&["example.com"])], + &log, + ); + assert_eq!( + got, + ToolResponse::Failed { + message: CANNOT_START.to_string() + } + ); + escaped(&log); +} diff --git a/docs/plans/M3b/files/crates/brokerd/tests/notices.rs b/docs/plans/M3b/files/crates/brokerd/tests/notices.rs new file mode 100644 index 0000000..65aae3e --- /dev/null +++ b/docs/plans/M3b/files/crates/brokerd/tests/notices.rs @@ -0,0 +1,80 @@ +//! Two small texts from the M3b review: every line `brokerd serve` prints about its runtime starts +//! with `brokerd:`, and a bad `[runner]` value is quoted in its error. Do not edit. + +#[path = "support/fake_podman.rs"] +mod fake_podman; + +use std::io::Read; +use std::os::unix::net::UnixStream; +use std::process::{Command, Stdio}; +use std::time::{Duration, Instant}; + +use brokerd::config::{Config, ConfigError}; +use fake_podman::{Fake, IMAGE, serial}; + +#[test] +fn the_runtime_notice_starts_with_brokerd() { + let _s = serial(); + let fake = Fake::new("notice", "exit 0"); + let home = fake.dir.join("home"); + std::fs::create_dir_all(home.join("grants")).unwrap(); + let config = fake.dir.join("brokerd.toml"); + std::fs::write( + &config, + format!( + "[paths]\nhome = \"{0}\"\ngrants = \"{0}/grants\"\n[runner]\npodman = \"{1}\"\nimage = \"{IMAGE}\"\n", + home.display(), + fake.script.display() + ), + ) + .unwrap(); + let mut child = Command::new(env!("CARGO_BIN_EXE_brokerd")) + .args(["serve", "--config"]) + .arg(&config) + .stderr(Stdio::piped()) + .spawn() + .unwrap(); + let until = Instant::now() + Duration::from_secs(10); + while UnixStream::connect(home.join("run/loop-broker/broker.sock")).is_err() { + assert!(Instant::now() < until, "brokerd never listened"); + std::thread::sleep(Duration::from_millis(20)); + } + std::thread::sleep(Duration::from_millis(100)); + child.kill().unwrap(); + child.wait().unwrap(); + let mut printed = String::new(); + child + .stderr + .take() + .unwrap() + .read_to_string(&mut printed) + .unwrap(); + assert!( + printed + .lines() + .any(|l| l == format!("brokerd: tools run in containers from {IMAGE}")), + "{printed}" + ); +} + +#[test] +fn a_bad_runner_value_is_quoted() { + let dir = std::env::temp_dir().join(format!("bx-notice-cfg-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let cases = [ + ("image = \"not by digest\"", "\"not by digest\""), + ( + &*format!("image = \"{IMAGE}\"\nmemory = \"lots\""), + "\"lots\"", + ), + ]; + for (n, (body, quoted)) in cases.iter().enumerate() { + let path = dir.join(format!("q{n}.toml")); + std::fs::write(&path, format!("[runner]\n{body}\n")).unwrap(); + match Config::load(&path) { + Err(ConfigError::Invalid(_, why)) => assert!(why.contains(quoted), "{why}"), + other => panic!("{body}: {other:?}"), + } + } + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/docs/plans/M3b/files/crates/toolkit/tests/egress_form.rs b/docs/plans/M3b/files/crates/toolkit/tests/egress_form.rs new file mode 100644 index 0000000..033168f --- /dev/null +++ b/docs/plans/M3b/files/crates/toolkit/tests/egress_form.rs @@ -0,0 +1,42 @@ +//! `toolkit egress-proxy` takes exactly `--socket --allow `: with anything after +//! them it is not the proxy, and exits 2 at once rather than listening (M3b review). Do not edit. + +use std::process::{Command, Stdio}; +use std::time::{Duration, Instant}; + +#[test] +fn trailing_arguments_are_not_the_proxy_form() { + let dir = std::env::temp_dir().join(format!("tk-egress-form-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let socket = dir.join("egress.sock"); + let mut child = Command::new(env!("CARGO_BIN_EXE_toolkit")) + .args([ + "egress-proxy", + "--socket", + socket.to_str().unwrap(), + "--allow", + "example.com", + "extra", + ]) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .unwrap(); + let until = Instant::now() + Duration::from_secs(3); + let status = loop { + if let Some(status) = child.try_wait().unwrap() { + break status; + } + if Instant::now() > until { + let _ = child.kill(); + let _ = child.wait(); + panic!("it is listening: trailing arguments were accepted"); + } + std::thread::sleep(Duration::from_millis(20)); + }; + assert_eq!(status.code(), Some(2)); + assert!(!socket.exists(), "no socket was made"); + let _ = std::fs::remove_dir_all(&dir); +}