From 595faec41ef86aba42111f04393a5818c9eab3b8 Mon Sep 17 00:00:00 2001 From: blooop Date: Sun, 30 Aug 2026 17:57:46 +0000 Subject: [PATCH 01/11] Ask the listing's questions together `dl --ls` reads the workspace list once and then asks `devpod status` about every workspace in it, because the STATE column is reported for every row and `devpod list` carries no state. That is one round trip per workspace and it cannot be avoided. What it did not have to do was wait for each answer before asking the next question: nothing devpod says about one workspace changes what is asked about another. At the 0.45s a trip costs (docs/performance.md), a forty workspace machine spent about eighteen seconds in `dl --ls`. The trips now go out in batches of eight, so the same forty cost about five rounds of one trip. The number of trips is unchanged, which is why `the_listing_costs_one_list_and_one_status_per_workspace` reads exactly as before; only the waiting overlaps. **`Runner` gains `Sync`, which is the real decision here.** Handing one `&dyn Runner` to several threads means the seam has to promise it can be shared. It cost the production implementation nothing, `ProcessRunner` being a unit struct, and cost three test wrappers the change from `RefCell` to `Mutex` that any shared recorder needs anyway. The binding half is deliberate: no future implementation may keep a `RefCell` inside it. A seam that can only be driven from one thread makes every concurrent flow above it impossible, and putting the bound at each call site instead would let an implementation exist that satisfies some callers and not others. One row of `devlaunch-runner/public-api.txt` moves; the promised `api` tier is untouched. Two details that are not obvious from the diff: The stage is opened once around the whole batch rather than once per trip. The registry admits one owner per stage, so per-trip staging would have had whichever thread opened `devpod-up` close it while its siblings were still running, and their spans would have landed outside any stage. An empty listing returns before opening one at all, because the serial version never reached the function that opened it and an empty stage is a reported step that did not happen. A worker's panic is carried rather than replaced, so a listing that panics still says why. `the_status_trips_of_one_listing_overlap` pins the property with a rendezvous rather than a sleep: every trip announces itself and waits for one more, so overlap returns at once and a serial build fails on the high-water mark. It expects a literal 2 rather than the pool width, because expressing the bar in terms of the constant under test is how an earlier version of it passed against a build deliberately serialised to one. --- docs/performance.md | 25 ++ .../src/flows/agent_worktrees/tests.rs | 17 +- rust/devlaunch-core/src/flows/listing.rs | 276 ++++++++++++++++-- rust/devlaunch-core/src/flows/repo_manager.rs | 10 +- .../src/flows/workspace_clone.rs | 38 ++- rust/devlaunch-runner/public-api.txt | 2 +- rust/devlaunch-runner/src/lib.rs | 19 +- 7 files changed, 337 insertions(+), 50 deletions(-) diff --git a/docs/performance.md b/docs/performance.md index 7731369a..b70568e6 100644 --- a/docs/performance.md +++ b/docs/performance.md @@ -20,6 +20,31 @@ naming it and then the tools probe, rides a single setup pass. So an interactive `dl ` and a one-shot `dl -- ` cost the same trips. +## The listing's questions are asked together + +`dl --ls` is the one command whose cost grows with the machine. It reads the +workspace list once, and then asks `devpod status` about every workspace in it, +including the ones devlaunch did not make, because the `STATE` column is reported +for every row. That is one round trip per workspace and there is no way around it: +`devpod list` does not carry a state, so a listing of forty workspaces asks forty +questions. + +What it no longer does is wait for each one before asking the next. The questions +are independent, so they go out in batches of eight and the waiting overlaps: at +the 0.45s above, forty workspaces cost about five rounds of a single trip rather +than forty trips end to end. The trips themselves are unchanged in number, which +is why `the_listing_costs_one_list_and_one_status_per_workspace` still reads the +same; what changed is only how much of the waiting happens at once. + +Eight is chosen for the shape of the wait rather than for the core count. A trip +is one process blocking on devpod's own work rather than arithmetic, so the useful +width is set by how many of those the machine will schedule; and it is bounded +rather than unlimited because a row costs a devpod process, and sixty at once +would spend more on contention than the serial version spent waiting. + +This is also why the listing is a command somebody runs rather than something on +the launch path. A launch asks about one workspace, and pays one trip for it. + ## One connection per workspace The trip that carries `dl -- ` at a terminal is OpenSSH, over the host diff --git a/rust/devlaunch-core/src/flows/agent_worktrees/tests.rs b/rust/devlaunch-core/src/flows/agent_worktrees/tests.rs index a218d67f..0b9b0d28 100644 --- a/rust/devlaunch-core/src/flows/agent_worktrees/tests.rs +++ b/rust/devlaunch-core/src/flows/agent_worktrees/tests.rs @@ -13,7 +13,7 @@ //! the absence of a path — a plan that contains no unit for it, a spawn log that //! contains no invocation naming it — rather than a guard firing. -use std::cell::RefCell; +use std::sync::Mutex; use devlaunch_runner::{ CapturedText, DetachOutcome, Invocation, Outcome, ProcessRunner, Runner, SpawnSpec, @@ -156,7 +156,7 @@ impl Clone { plan: &CloneWorktrees, forgets_must_be_absent: bool, ) -> (WorktreeReport, Vec>) { - let calls = RefCell::new(Vec::new()); + let calls = Mutex::new(Vec::new()); let runner = Recording { real: ProcessRunner::new(), calls: &calls, @@ -165,7 +165,7 @@ impl Clone { let git = Git::new(&runner); let mut report = WorktreeReport::default(); reclaim(&git, plan, Some(&self.bare), &mut report); - (report, calls.into_inner()) + (report, calls.into_inner().expect("the recorded calls")) } fn listing(&self) -> String { @@ -214,7 +214,7 @@ impl OtherRepository { /// forget is invoked, which is P2 asserted directly (devlaunch#462). struct Recording<'a> { real: ProcessRunner, - calls: &'a RefCell>>, + calls: &'a Mutex>>, /// Assert P2 at every forget: the argument must not exist when the spawn /// happens. Off for the one fixture whose recorded path deliberately /// resolves into another repository, where the point is git's refusal. @@ -239,7 +239,7 @@ impl Runner for Recording<'_> { invoked, and {target} does" ); } - self.calls.borrow_mut().push(argv); + self.calls.lock().expect("the recorded calls").push(argv); self.real.capture(spec) } @@ -1106,7 +1106,7 @@ fn a_foreign_leaf_colliding_with_our_admin_name_is_not_probed_through_our_index( let theirs = other.worktree_at(&worktrees_dir(&outer).join("agent-outer"), "agent-outer"); world.containerise(); - let calls = RefCell::new(Vec::new()); + let calls = Mutex::new(Vec::new()); let runner = Recording { real: ProcessRunner::new(), calls: &calls, @@ -1139,11 +1139,12 @@ fn a_foreign_leaf_colliding_with_our_admin_name_is_not_probed_through_our_index( let theirs_spelled = format!("--work-tree={}", theirs.display()); assert!( !calls - .borrow() + .lock() + .expect("the recorded calls") .iter() .any(|argv| argv.iter().any(|arg| arg == &theirs_spelled)), "the foreign site must never be probed: {:?}", - calls.borrow() + calls.lock().expect("the recorded calls") ); } diff --git a/rust/devlaunch-core/src/flows/listing.rs b/rust/devlaunch-core/src/flows/listing.rs index 7dffcbf9..46563cc3 100644 --- a/rust/devlaunch-core/src/flows/listing.rs +++ b/rust/devlaunch-core/src/flows/listing.rs @@ -1008,9 +1008,11 @@ pub struct ListedWorkspace { /// /// Costs one `devpod list` (the command's snapshot) plus one `devpod status` per /// listed workspace — including the ones devlaunch did not make, which Python asks -/// about too. Under [`Sizes::Measure`] it also walks each of dl's own clones, -/// which is O(files) with no ceiling and the reason `--size` is asked for rather -/// than always answered. +/// about too. The status trips are asked concurrently ([`container_states`]); +/// everything else about a row is local work and stays sequential. Under +/// [`Sizes::Measure`] it also walks each of dl's own clones, which is O(files) +/// with no ceiling and the reason `--size` is asked for rather than always +/// answered. pub fn enriched_listing( context: &mut CommandContext<'_>, view: &DlView<'_>, @@ -1019,18 +1021,108 @@ pub fn enriched_listing( let workspaces = context.workspaces()?; let git = context.git(); let runner = context.runner(); + // Every status trip first, together, because they are the only part of a row + // that leaves this machine and they do not depend on each other. + let states = container_states(runner, &workspaces); Ok(workspaces .iter() - .map(|workspace| enriched_row(runner, &git, view, sizes, workspace)) + .zip(states) + .map(|(workspace, state)| enriched_row(&git, view, sizes, workspace, state)) .collect()) } +/// How many `devpod status` trips are in flight at once. +/// +/// Not unbounded: a row costs a devpod process, and a machine with sixty +/// workspaces would otherwise fork sixty at once and spend more on the contention +/// than the serial version spent waiting. Eight is chosen for the shape of the +/// wait rather than for the core count — the trip is one process blocking on +/// devpod's own work, not arithmetic, so the useful width is set by how many of +/// those the machine will schedule rather than by how many can compute at once. +const STATUS_TRIPS_AT_ONCE: usize = 8; + +/// The container state of each workspace, in the order they were given. +/// +/// One `devpod status` per workspace, which is the cost this listing has always +/// paid; what changed is that the waiting overlaps. The trips are independent — +/// each asks devpod about one id and nothing it learns changes what another +/// asks — so the only thing serialising them was the loop they were written in, +/// and at a measured 0.454s each (`docs/performance.md`) a machine with forty +/// workspaces waited about eighteen seconds for a command whose answer was ready +/// in two. +/// +/// It is staged at all because Python stages `get_workspace_state` itself +/// (`@timing.staged("devpod-up")`) and the JSON timing document reports spans +/// *inside* the stage that was open: unstaged, these round trips would appear in +/// the prose summary and be missing from the document, which is the shape a +/// listing reports the most of. Python marks the stage `ok` for a devpod that ran +/// and refused, gave non-JSON, or omitted `state`, and `failed` only where devpod +/// could not be run at all, which is the distinction `never_ran` carries here. +/// +/// **The stage is opened once, here, rather than once per trip.** Every worker +/// would otherwise race to open and close the same [`timing::Stage::DevpodUp`], +/// and the registry admits one owner per stage: whichever thread opened it would +/// close it while the others were still running, and the spans they then recorded +/// would land outside any stage. Opening it on this thread, around the whole +/// batch, is what keeps the timing document reporting the same shape it did when +/// the trips were serial. The stage fails if devpod could not be run at all, for +/// exactly the reason the serial version failed it: a stage must not report `ok` +/// for a step devpod never ran (P12). +fn container_states(runner: &dyn Runner, workspaces: &[Workspace]) -> Vec> { + // Before the stage, not inside it: a listing with nothing to ask about opened + // no `devpod-up` stage when the trips were made one at a time, because the + // function that opened one was never reached. Opening it here regardless would + // put an empty stage in the timing document of every `dl --ls` on a machine + // with no workspaces, which is a reported step that never happened. + if workspaces.is_empty() { + return Vec::new(); + } + + let mut stage = timing::stage(timing::Stage::DevpodUp); + let mut answers: Vec> = Vec::with_capacity(workspaces.len()); + let mut never_ran = false; + + for batch in workspaces.chunks(STATUS_TRIPS_AT_ONCE) { + // Scoped threads so the runner is borrowed rather than shared by + // reference count: the batch is joined before this loop turns over, so + // nothing outlives the borrow and there is no `Arc` to explain. + let batched: Vec<_> = std::thread::scope(|scope| { + let handles: Vec<_> = batch + .iter() + .map(|workspace| { + scope.spawn(|| devpod::status(runner, &workspace.id, Patience::AsLongAsItTakes)) + }) + .collect(); + handles + .into_iter() + // Carry a worker's panic rather than replacing it: the serial + // version unwound with whatever `devpod::status` said, and a + // listing that panics should still say why. + .map(|handle| { + handle + .join() + .unwrap_or_else(|panic| std::panic::resume_unwind(panic)) + }) + .collect() + }); + for answer in batched { + never_ran |= matches!(answer, Err(devpod::StatusUnreadable::NotRun(_))); + answers.push(answer.ok()); + } + } + + if never_ran { + stage.fail(); + } + answers +} + fn enriched_row( - runner: &dyn Runner, git: &Git<'_>, view: &DlView<'_>, sizes: Sizes, workspace: &Workspace, + state: Option, ) -> ListedWorkspace { // One question asked once. Whether this workspace is dl's, which directory the // row is about, and what is in it all read this answer, rather than each @@ -1071,7 +1163,7 @@ fn enriched_row( ListedWorkspace { id: workspace.id.clone(), last_used: workspace.last_used.clone(), - state: container_state(runner, &workspace.id), + state, clone, disk: DiskField::of(sizes, measurable.as_deref()), sweep, @@ -1130,25 +1222,6 @@ impl SweptRepoNote { /// and JSON with no `state` in it. The distinctions exist one layer down /// ([`devpod::StatusUnreadable`]) for a caller that has something different to do /// about each; this one does not. -fn container_state(runner: &dyn Runner, workspace_id: &str) -> Option { - // Staged, because Python stages `get_workspace_state` itself - // (`@timing.staged("devpod-up")`), and the JSON timing document reports spans - // *inside* the stage that was open. Unstaged, the `devpod status` round trips - // this makes are in the prose summary and missing from the document — which is - // the shape a listing of five workspaces reports the most of. - let mut stage = timing::stage(timing::Stage::DevpodUp); - let answer = devpod::status(runner, workspace_id, Patience::AsLongAsItTakes); - // Python stages `get_workspace_state`, which returns `None` (stage `ok`) for a - // devpod that ran and refused, gave non-JSON, or omitted `state`, and only - // marks the stage `failed` when devpod could not be run at all — the spawn - // that raises `DevpodNotInstalled`. Mirror that: a `NotRun` fails the stage so - // the timing document does not report `ok` for a step devpod never ran (P12). - if matches!(answer, Err(devpod::StatusUnreadable::NotRun(_))) { - stage.fail(); - } - answer.ok() -} - /// What deleting *workspace_id* would destroy, as far as dl can establish. /// /// The `dl rm` guard's reader. Answers [`Unsaved::NothingToLose`] for a @@ -1432,6 +1505,7 @@ mod tests { use std::collections::BTreeMap; use std::path::{Path, PathBuf}; use std::process::Command; + use std::sync::{Condvar, Mutex}; use devlaunch_runner::{ CapturedText, DetachOutcome, Invocation, Outcome, ProcessRunner, SpawnSpec, @@ -2768,6 +2842,158 @@ mod tests { ); } + /// A runner that answers `devpod status` and reports how many answers it was + /// producing at the same moment. + /// + /// The instrument is a rendezvous rather than a sleep: every trip announces + /// itself and then waits for the rest of its batch to arrive. If the trips + /// overlap they all arrive and every one returns at once; if they are serial + /// the first waits alone, times out, and the high-water mark stays at one. So + /// a pass is quick and a regression is a clean assertion failure after the + /// timeout rather than a hang. + struct Overlapping { + state: Mutex, + arrived: Condvar, + /// How many this test expects to be in flight together. + want: usize, + } + + #[derive(Default)] + struct Overlap { + in_flight: usize, + high_water: usize, + /// Arrivals ever, which only goes up. The rendezvous waits on this and + /// not on `in_flight`: a thread that has already been released decrements + /// `in_flight` on its way out, so waiting on that count lets the last + /// arrival free itself and leave the earlier ones waiting for a number + /// that has just gone back down. That was a ten second timeout per run. + arrivals: usize, + } + + impl Overlapping { + fn expecting(want: usize) -> Self { + Self { + state: Mutex::new(Overlap::default()), + arrived: Condvar::new(), + want, + } + } + + fn high_water(&self) -> usize { + self.state.lock().expect("the overlap").high_water + } + } + + impl Runner for Overlapping { + fn capture(&self, spec: &SpawnSpec) -> Outcome { + let argv = spec.invocation.argv(); + assert_eq!( + argv[1], "status", + "this fake answers status and nothing else" + ); + + let mut state = self.state.lock().expect("the overlap"); + state.in_flight += 1; + state.arrivals += 1; + state.high_water = state.high_water.max(state.in_flight); + self.arrived.notify_all(); + while state.arrivals < self.want { + let (guard, timed_out) = self + .arrived + .wait_timeout(state, std::time::Duration::from_secs(10)) + .expect("the overlap"); + state = guard; + if timed_out.timed_out() { + break; + } + } + state.in_flight -= 1; + drop(state); + + Outcome::Ran { + exit: devlaunch_runner::Exit::Code(0), + io: CapturedText { + // The id is echoed *as the state*, which `ContainerState` + // keeps whole as `Unknown`. That is what lets the ordering + // test read the returned vector and see which answer landed + // where, without the fake having to record anything. + stdout: format!(r#"{{"state":"{}"}}"#, argv[2]), + stderr: String::new(), + }, + } + } + + fn passthrough(&self, _spec: &SpawnSpec) -> Outcome { + unreachable!("a listing captures") + } + + fn session(&self, _spec: &SpawnSpec, _on_stderr_line: &mut dyn FnMut(&str)) -> Outcome { + unreachable!("a listing opens no session") + } + + fn detach(&self, _what: &Invocation) -> DetachOutcome { + unreachable!("a listing detaches nothing") + } + } + + #[test] + fn the_status_trips_of_one_listing_overlap() { + // The whole point of the change: the trips are independent, so the waiting + // is shared rather than added up. Serial, the high-water mark is 1. + // + // Two, as a literal, and not `STATUS_TRIPS_AT_ONCE`: expressing the + // expectation in terms of the width under test is how this test passed + // against a deliberately serialised build, since setting the width to 1 + // moved the bar down with it. Overlap at all is the property; how wide the + // pool is is a tuning decision the other test covers. + const TOGETHER: usize = 2; + let workspaces: Vec<_> = (0..4) + .map(|n| workspace(&format!("ws-{n}"), local(Path::new("/tmp")))) + .collect(); + let runner = Overlapping::expecting(TOGETHER); + + let states = container_states(&runner, &workspaces); + + assert!( + runner.high_water() >= TOGETHER, + "the trips ran one at a time: high water {}", + runner.high_water() + ); + assert_eq!(states.len(), workspaces.len(), "one answer per workspace"); + } + + #[test] + fn a_batch_larger_than_the_width_still_answers_for_every_workspace_in_order() { + // The chunking is the part that could silently drop or reorder a row: the + // answers come back per batch and are appended, so a listing wider than + // the pool has to read the same as one narrower than it. Nine against a + // width of eight is the smallest case with a short second chunk. + let ids: Vec = (0..STATUS_TRIPS_AT_ONCE + 1) + .map(|n| format!("ws-{n}")) + .collect(); + let workspaces: Vec<_> = ids + .iter() + .map(|id| workspace(id, local(Path::new("/tmp")))) + .collect(); + // Only the first chunk can rendezvous; the short one must not wait for a + // full batch that will never arrive. + let runner = Overlapping::expecting(1); + + let states = container_states(&runner, &workspaces); + + let answered: Vec = states + .iter() + .map(|state| match state { + Some(ContainerState::Unknown(word)) => word.clone(), + other => panic!("the fake answers its own id as the state: {other:?}"), + }) + .collect(); + assert_eq!( + answered, ids, + "every workspace answered for, in the order it was given" + ); + } + #[test] fn the_listing_costs_one_list_and_one_status_per_workspace() { // Including the workspaces devlaunch did not make: `state` is reported for diff --git a/rust/devlaunch-core/src/flows/repo_manager.rs b/rust/devlaunch-core/src/flows/repo_manager.rs index e897be66..8193804f 100644 --- a/rust/devlaunch-core/src/flows/repo_manager.rs +++ b/rust/devlaunch-core/src/flows/repo_manager.rs @@ -1835,7 +1835,10 @@ pub(crate) mod tests { } /// Something a test wants to happen when a given argv is spawned. - type Effect = Box; + /// `Send + Sync` because [`Runner`] is: the listing fans its `devpod status` + /// round trips out across threads, so anything a test hangs off a spawn has to + /// be shareable too. + type Effect = Box; impl FakeGit { pub(crate) fn new() -> Self { @@ -1869,7 +1872,10 @@ pub(crate) mod tests { /// Do this as well, whenever a call is made. For the effect a test needs /// that git would have had — a pull that materializes a pointer file. #[must_use] - pub(crate) fn and_then(mut self, effect: impl Fn(&[String]) + 'static) -> Self { + pub(crate) fn and_then( + mut self, + effect: impl Fn(&[String]) + Send + Sync + 'static, + ) -> Self { self.extra.push(Box::new(effect)); self } diff --git a/rust/devlaunch-core/src/flows/workspace_clone.rs b/rust/devlaunch-core/src/flows/workspace_clone.rs index 96c5d7dc..910ab6f6 100644 --- a/rust/devlaunch-core/src/flows/workspace_clone.rs +++ b/rust/devlaunch-core/src/flows/workspace_clone.rs @@ -1373,9 +1373,8 @@ mod tests { //! Real git-lfs is used where nothing else can answer, and those tests step //! aside when the machine has no git-lfs (see [`lfs_is_usable`]). - use std::cell::RefCell; use std::process::Command; - use std::rc::Rc; + use std::sync::{Arc, Mutex}; use std::time::Duration; use super::*; @@ -2544,11 +2543,11 @@ mod tests { // Recorded through a shared cell rather than returned, because the hook runs // inside the call it is observing. /// One git call, and whether the repo lock was held while it ran. - type Observed = Rc, bool)>>>; - let observed: Observed = Rc::new(RefCell::new(Vec::new())); + type Observed = Arc, bool)>>>; + let observed: Observed = Arc::new(Mutex::new(Vec::new())); let fake = FakeGit::new().and_then({ let lock_path = lock_path.clone(); - let observed = Rc::clone(&observed); + let observed = Arc::clone(&observed); move |argv: &[String]| { // A second open file description on the same path: flock is // per-open-file-description, so this conflicts with the production @@ -2557,7 +2556,10 @@ mod tests { let free = locks::run_if_lock_free(&lock_path, || ()) .expect("no error") .is_some(); - observed.borrow_mut().push((argv.to_vec(), !free)); + observed + .lock() + .expect("the observed calls") + .push((argv.to_vec(), !free)); } }); let manager = a_clone_manager(&cache, Git::new(&fake), GitLfs::NotInstalled); @@ -2573,7 +2575,7 @@ mod tests { ) .expect("prepared"); - let observed = observed.borrow(); + let observed = observed.lock().expect("the observed calls"); assert!(!observed.is_empty(), "no git call was observed at all"); for (argv, was_held) in observed.iter() { assert!( @@ -4067,7 +4069,7 @@ mod tests { struct StubbedLfs { real: ProcessRunner, reports: Vec, - calls: RefCell>>, + calls: Mutex>>, } impl StubbedLfs { @@ -4075,13 +4077,14 @@ mod tests { Self { real: ProcessRunner::new(), reports: names.iter().map(|name| (*name).to_string()).collect(), - calls: RefCell::new(Vec::new()), + calls: Mutex::new(Vec::new()), } } fn forked_git_lfs(&self) -> bool { self.calls - .borrow() + .lock() + .expect("the recorded calls") .iter() .any(|argv| argv.get(1).is_some_and(|arg| arg == "lfs")) } @@ -4090,7 +4093,10 @@ mod tests { impl Runner for StubbedLfs { fn capture(&self, spec: &SpawnSpec) -> Outcome { let argv = spec.invocation.argv(); - self.calls.borrow_mut().push(argv.clone()); + self.calls + .lock() + .expect("the recorded calls") + .push(argv.clone()); if argv.get(1).is_some_and(|arg| arg == "lfs") { return Outcome::Ran { exit: Exit::Code(0), @@ -4108,7 +4114,10 @@ mod tests { } fn passthrough(&self, spec: &SpawnSpec) -> Outcome { - self.calls.borrow_mut().push(spec.invocation.argv()); + self.calls + .lock() + .expect("the recorded calls") + .push(spec.invocation.argv()); Outcome::Ran { exit: Exit::Code(0), io: (), @@ -4120,7 +4129,10 @@ mod tests { } fn detach(&self, what: &Invocation) -> DetachOutcome { - self.calls.borrow_mut().push(what.argv()); + self.calls + .lock() + .expect("the recorded calls") + .push(what.argv()); DetachOutcome::Started { pid: 900_001 } } } diff --git a/rust/devlaunch-runner/public-api.txt b/rust/devlaunch-runner/public-api.txt index 3bb4d926..b7efbfe1 100644 --- a/rust/devlaunch-runner/public-api.txt +++ b/rust/devlaunch-runner/public-api.txt @@ -190,7 +190,7 @@ pub fn devlaunch_runner::SpawnSpec::default() -> devlaunch_runner::SpawnSpec impl core::fmt::Debug for devlaunch_runner::SpawnSpec pub fn devlaunch_runner::SpawnSpec::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::marker::StructuralPartialEq for devlaunch_runner::SpawnSpec -pub trait devlaunch_runner::Runner +pub trait devlaunch_runner::Runner: core::marker::Sync pub fn devlaunch_runner::Runner::capture(&self, &devlaunch_runner::SpawnSpec) -> devlaunch_runner::Outcome pub fn devlaunch_runner::Runner::detach(&self, &devlaunch_runner::Invocation) -> devlaunch_runner::DetachOutcome pub fn devlaunch_runner::Runner::passthrough(&self, &devlaunch_runner::SpawnSpec) -> devlaunch_runner::Outcome diff --git a/rust/devlaunch-runner/src/lib.rs b/rust/devlaunch-runner/src/lib.rs index c892bb81..ab507689 100644 --- a/rust/devlaunch-runner/src/lib.rs +++ b/rust/devlaunch-runner/src/lib.rs @@ -426,7 +426,24 @@ pub enum DetachOutcome { /// is not a fake devpod at all: it plays back a list the test handed it and /// keeps the argv for the assertions to read. `flows::provision`'s `Trips` is /// the example, and its doc says why a recorder cannot join the corpus. -pub trait Runner { +/// # `Sync`, because one command's round trips are not one conversation +/// +/// A listing asks devpod about every workspace it lists, and those questions are +/// independent: nothing devpod says about one changes what is asked about +/// another. `flows::listing` therefore asks them together, which means handing +/// the same `&dyn Runner` to several threads at once, which means this trait has +/// to promise it can be shared. +/// +/// The promise costs the production implementation nothing ([`ProcessRunner`] is +/// a unit struct) and cost the fakes only the change from `RefCell` to `Mutex` +/// that any shared recorder needs anyway. What it does do is bind every future +/// implementation: a runner that wants `RefCell` inside it is no longer writable, +/// and that is the deliberate half of the trade. A seam that can only be driven +/// from one thread makes every concurrent flow above it impossible, and the +/// alternative spelling, a `Sync` bound at each call site that needs it, puts the +/// requirement in the callers rather than in the contract and lets an +/// implementation exist that satisfies some callers and not others. +pub trait Runner: Sync { /// Run to completion, reading both streams as text. fn capture(&self, spec: &SpawnSpec) -> Outcome; From 2d5280787d17c45e677d25dfa8b048b556202e41 Mon Sep 17 00:00:00 2001 From: blooop Date: Sun, 30 Aug 2026 18:01:13 +0000 Subject: [PATCH 02/11] Pin the supertrait, since it is a promise like a method is `rust-coverage` went red on the snapshot guard: it holds the Runner row by exact string, and the row is now `pub trait devlaunch_runner::Runner: core::marker::Sync`. My own fault for regenerating the snapshot after running the workspace and not running it again, so the failure reached CI instead of this machine. Pinned as the whole row rather than loosened to a prefix match. The test's subject is "the trait an implementer writes against", and a supertrait is part of that in the same way a method is: `Sync` is what says a runner may be handed to several threads at once, which is what lets the listing ask its status trips together. Dropping it later would break every implementation that had come to rely on being shareable, so it should cost a deliberate edit to this line, which a prefix match would not have. --- rust/devlaunch-runner/tests/public_api_snapshot.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/rust/devlaunch-runner/tests/public_api_snapshot.rs b/rust/devlaunch-runner/tests/public_api_snapshot.rs index 603a720b..13f3cddc 100644 --- a/rust/devlaunch-runner/tests/public_api_snapshot.rs +++ b/rust/devlaunch-runner/tests/public_api_snapshot.rs @@ -40,9 +40,16 @@ fn the_seam_carries_a_snapshot_of_its_own() { #[test] fn the_snapshot_pins_the_trait_an_implementer_writes_against() { + // The whole row, supertraits included, because a supertrait is a promise to + // whoever implements this trait exactly as a method is: `Sync` says a runner + // may be handed to several threads at once, which is what lets `flows::listing` + // ask its status round trips together, and dropping it would break every + // implementation that had come to rely on being shareable. So it is pinned + // here rather than tolerated by a prefix match, and changing it is a + // deliberate edit to this line. assert!( - rows(SNAPSHOT).contains(&"pub trait devlaunch_runner::Runner"), - "the Runner trait is missing from the snapshot" + rows(SNAPSHOT).contains(&"pub trait devlaunch_runner::Runner: core::marker::Sync"), + "the Runner trait is missing from the snapshot, or no longer requires Sync" ); for method in ["capture", "passthrough", "session", "detach"] { assert!( From 802f292d487c1459dbb4b7b6a06193e5d99386c4 Mon Sep 17 00:00:00 2001 From: blooop Date: Sun, 30 Aug 2026 18:28:17 +0000 Subject: [PATCH 03/11] Answer the review: the new tests were corrupting other tests' timing A fresh-context review of this branch found the two new tests writing into the process-global timing registry without holding the exclusion every other fixture in this module takes. Measured, not argued: run either of them beside `launch`'s `a_warm_launch_reports_the_devpod_probe_and_the_attach_and_nothing_else` under `--test-threads=8` and it failed 12 runs in 15. Zero in 25 with the guard. `container_states` opens `devpod-up` on the global registry and every worker records a span into it, so a listing built without the guard writes into whatever document a concurrent measured test installed, and its stage guard closes a stage that test opened. `FakeDevpodRealGit` already takes `timing::exclusive()` for exactly this reason and its doc says so; `Overlapping` is a second runner in the same module and skipped it. It also runs both ways: the listing's stage can be the one a launch test's `stage_result` finds `AlreadyOpen`, after which the launch document reports `devpod-up` seconds it never spent. That would have surfaced as an unreproducible flake on a narrowed filter, since the full suite hides it. **The order guard was weak.** With a rendezvous of one, no trip ever overlapped another, so the test exercised the chunk boundary but never out-of-order completion, which is the only way a reordering implementation manifests. The review measured a completion-order collector slipping past it four runs in five. It is now two full chunks at the full width, so every trip in a chunk is released together; a reordering build fails on essentially every run, which was checked. Also from the review: - Deleting `container_state` left its doc block orphaned onto `unsaved_work_in`, which rustdoc rendered as one comment opening with two paragraphs about status parsing. Removed, and the part that is still true (every unreadable answer collapses to `None`) moved onto the function that now does it. - `docs/performance.md` said forty workspaces cost "about five rounds of a single trip". `chunks` is a barrier, not a pool: a batch costs its slowest trip and the next does not start until the last returns. Says so now, along with what a work queue would buy, and no longer asserts a contention figure nobody measured. Also records that the `devpod-up` stage seconds shrink, since the stage is now the batch loop's wall time rather than the sum of the rows. - The empty-listing early return had no test. It has one. - `zip` would drop rows rather than fail if `container_states` ever came back short. `debug_assert_eq!` states the invariant where it is relied on. - `repo_manager`'s new bound said "`Send + Sync` because `Runner` is". `Runner` gained `Sync` only. - CHANGELOG entry, which a seam contract change had no business omitting. One open question the review could not settle, now settled: no out-of-tree implementation of `Runner` is broken by the supertrait, because wayfinder does not link `devlaunch-core` at all. Its Cargo.lock holds no devlaunch entry; the only mentions in its tree are issue references and fixture strings. --- CHANGELOG.md | 28 ++++++ docs/performance.md | 32 +++++-- rust/devlaunch-core/src/flows/listing.rs | 85 +++++++++++++++---- rust/devlaunch-core/src/flows/repo_manager.rs | 8 +- 4 files changed, 126 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 591f359a..ff09c570 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -148,6 +148,34 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **`dl --ls` asks its `devpod status` round trips together, and `Runner` now + requires `Sync`.** A listing reads the workspace list once and then asks devpod + about every workspace in it, because the `STATE` column is reported for every + row and `devpod list` carries no state. Those trips are required and none has + been removed. What the listing no longer does is wait for each answer before + asking the next question: nothing devpod says about one workspace changes what + is asked about another. They go out in batches of eight, so a forty workspace + machine pays five batches rather than forty trips end to end, at a measured + 0.45s a trip. The number of trips is unchanged, which is why the test that pins + that cost reads exactly as before. + + **The seam change is the part with consequences beyond this repository.** + `devlaunch_runner::Runner` gains `Sync` as a supertrait, which is what lets one + `&dyn Runner` be handed to several threads. Any out-of-tree implementation + holding a `RefCell`, `Rc` or `Cell` no longer compiles. In tree it cost + nothing: `ProcessRunner` is a unit struct, and three test wrappers took the + change from `RefCell` to `Mutex` that a shared recorder wants anyway. The + alternative, a `Sync` bound written at each call site that needs one, was + rejected because it puts the requirement in the callers rather than in the + contract and so permits an implementation that satisfies some callers and not + others. One row of `devlaunch-runner/public-api.txt` moves; the promised `api` + tier is untouched. + + A timing document for `dl --ls` reports smaller `devpod-up` **stage** seconds as + a result, since that stage is now the wall time of the batch loop rather than + the sum of the per-row status times. The spans themselves, and their count, are + unchanged. + - **A workspace id is derived once, and the three signatures that had a triple in hand stopped flattening it into loose strings.** `WorkspaceId::value()` ran the whole derivation on every call — a SHA-256 over the triple, three slug passes diff --git a/docs/performance.md b/docs/performance.md index b70568e6..9797716a 100644 --- a/docs/performance.md +++ b/docs/performance.md @@ -30,17 +30,33 @@ for every row. That is one round trip per workspace and there is no way around i questions. What it no longer does is wait for each one before asking the next. The questions -are independent, so they go out in batches of eight and the waiting overlaps: at -the 0.45s above, forty workspaces cost about five rounds of a single trip rather -than forty trips end to end. The trips themselves are unchanged in number, which -is why `the_listing_costs_one_list_and_one_status_per_workspace` still reads the -same; what changed is only how much of the waiting happens at once. +are independent, so they go out in batches of eight and the waiting overlaps: +forty workspaces cost five batches rather than forty trips end to end. The trips +themselves are unchanged in number, which is why +`the_listing_costs_one_list_and_one_status_per_workspace` still reads the same; +what changed is only how much of the waiting happens at once. + +**A batch costs the slowest trip in it, not the average.** The eight are started +together and all eight are waited for before the next eight begin, so this is a +barrier rather than a pool of eight permits: one slow answer leaves seven threads +idle until it lands. Five batches of 0.45s is therefore the figure to expect when +the trips are alike, and the honest floor rather than a promise. It is never worse +than asking serially, which is what it replaced, but a work queue that started the +ninth trip the moment any of the first eight returned would be better on a machine +where one workspace is much slower to answer than the rest. Worth knowing before +reading a slow `--ls` as something else. Eight is chosen for the shape of the wait rather than for the core count. A trip is one process blocking on devpod's own work rather than arithmetic, so the useful -width is set by how many of those the machine will schedule; and it is bounded -rather than unlimited because a row costs a devpod process, and sixty at once -would spend more on contention than the serial version spent waiting. +width is set by how many of those the machine will schedule. It is bounded rather +than unlimited because a row costs a devpod process, and the cost of starting +sixty at once is a real one; how that trades against the extra overlap has not +been measured here, and eight is a conservative pick rather than a tuned one. + +One thing the change does move: the `devpod-up` **stage** seconds a timing +document reports for `dl --ls` used to be the sum of the per-row status times and +are now the wall time of the batch loop, which is smaller. The span count, and +every individual span, are unchanged. This is also why the listing is a command somebody runs rather than something on the launch path. A launch asks about one workspace, and pays one trip for it. diff --git a/rust/devlaunch-core/src/flows/listing.rs b/rust/devlaunch-core/src/flows/listing.rs index 46563cc3..cc0bf696 100644 --- a/rust/devlaunch-core/src/flows/listing.rs +++ b/rust/devlaunch-core/src/flows/listing.rs @@ -1024,6 +1024,16 @@ pub fn enriched_listing( // Every status trip first, together, because they are the only part of a row // that leaves this machine and they do not depend on each other. let states = container_states(runner, &workspaces); + // `zip` stops at the shorter side, so a `container_states` that ever came back + // short would drop workspaces off the end of `dl --ls` rather than fail. It + // cannot today (one answer is pushed per workspace, and the empty case returns + // an empty vector), which is exactly why the invariant is worth stating where + // it is relied on. + debug_assert_eq!( + states.len(), + workspaces.len(), + "one state per workspace, or the listing loses rows" + ); Ok(workspaces .iter() .zip(states) @@ -1043,6 +1053,14 @@ const STATUS_TRIPS_AT_ONCE: usize = 8; /// The container state of each workspace, in the order they were given. /// +/// An answer devpod would not give reads as `None`, whichever way it would not +/// give it: Python collapses every unreadable answer to `None` and the wire field +/// is `null` for all of them, so a devpod that refused the question, output that +/// was not JSON, and JSON with no `state` in it are one row here. The distinctions +/// exist one layer down ([`devpod::StatusUnreadable`]) for a caller that has +/// something different to do about each; this one only tells `NotRun` apart, and +/// only to fail the stage. +/// /// One `devpod status` per workspace, which is the cost this listing has always /// paid; what changed is that the waiting overlaps. The trips are independent — /// each asks devpod about one id and nothing it learns changes what another @@ -1215,13 +1233,6 @@ impl SweptRepoNote { } } -/// devpod's state for one workspace, or nothing when it would not answer. -/// -/// Python collapses every unreadable answer to `None` and the wire field is `null` -/// for all of them: a devpod that refused the question, output that was not JSON, -/// and JSON with no `state` in it. The distinctions exist one layer down -/// ([`devpod::StatusUnreadable`]) for a caller that has something different to do -/// about each; this one does not. /// What deleting *workspace_id* would destroy, as far as dl can establish. /// /// The `dl rm` guard's reader. Answers [`Unsaved::NothingToLose`] for a @@ -1537,9 +1548,10 @@ mod tests { impl FakeDevpodRealGit { /// The runner, and the timing exclusion for as long as it lives. /// - /// [`container_state`] opens the `devpod-up` stage on the **process-global** - /// registry, once per row — so an enriched listing built without the guard - /// writes into whatever document a concurrent measured test installed, and + /// [`container_states`] opens the `devpod-up` stage on the **process-global** + /// registry, once per listing — so an enriched listing built without the + /// guard writes into whatever document a concurrent measured test installed, + /// and /// its stage guard closes a stage that test opened rather than one of its /// own. In the fixture rather than per test, as `lifecycle`'s `Devpod` and /// `launch`'s `Scene` do it, so a new test cannot forget. @@ -2856,6 +2868,21 @@ mod tests { arrived: Condvar, /// How many this test expects to be in flight together. want: usize, + /// The timing exclusion, for the same reason [`FakeDevpodRealGit`] holds + /// one: [`container_states`] opens the `devpod-up` stage on the + /// **process-global** registry and every worker records a span into it, so + /// a listing built without the guard writes into whatever document a + /// concurrent measured test installed, and its stage guard closes a stage + /// that test opened rather than one of its own. Measured rather than + /// feared: without this field, running these two tests beside + /// `launch`'s `a_warm_launch_reports_the_devpod_probe_and_the_attach_and_nothing_else` + /// failed 12 runs in 15. + /// + /// Safe against the reentrancy note on [`repo_manager`]'s `FakeGit`, which + /// deliberately holds no guard because it is built inside worker threads: + /// this one is built on the calling thread, before any worker exists, and + /// the workers never ask for a guard of their own. + _serialized: timing::Exclusive, } #[derive(Default)] @@ -2876,6 +2903,7 @@ mod tests { state: Mutex::new(Overlap::default()), arrived: Condvar::new(), want, + _serialized: timing::exclusive(), } } @@ -2936,6 +2964,25 @@ mod tests { } } + #[test] + fn an_empty_listing_asks_nothing_and_opens_no_stage() { + // The early return is about the timing document rather than about the + // round trips: a machine with no workspaces reported no `devpod-up` stage + // when the trips were serial, because the function that opened one was + // never reached. Opening one unconditionally would put a step that never + // happened into every `dl --ls` on an empty machine. + let runner = Overlapping::expecting(1); + + let states = container_states(&runner, &[]); + + assert!(states.is_empty(), "no workspaces, no answers"); + assert_eq!( + runner.high_water(), + 0, + "an empty listing asks devpod nothing" + ); + } + #[test] fn the_status_trips_of_one_listing_overlap() { // The whole point of the change: the trips are independent, so the waiting @@ -2966,18 +3013,24 @@ mod tests { fn a_batch_larger_than_the_width_still_answers_for_every_workspace_in_order() { // The chunking is the part that could silently drop or reorder a row: the // answers come back per batch and are appended, so a listing wider than - // the pool has to read the same as one narrower than it. Nine against a - // width of eight is the smallest case with a short second chunk. - let ids: Vec = (0..STATUS_TRIPS_AT_ONCE + 1) + // the pool has to read the same as one narrower than it. + // + // Two full chunks rather than a full one and a short one, and a rendezvous + // of the full width rather than of one, because the defect this test names + // is *reordering* and a trip that never overlaps another cannot reorder + // anything. Collecting in completion order instead of input order was + // measured against the earlier shape of this test (nine ids, a rendezvous + // of one) and went unnoticed in about four runs in five. Held to the width, + // every trip in a chunk is released together, so a collector that reads + // completion order sees a shuffled chunk on essentially every run. + let ids: Vec = (0..STATUS_TRIPS_AT_ONCE * 2) .map(|n| format!("ws-{n}")) .collect(); let workspaces: Vec<_> = ids .iter() .map(|id| workspace(id, local(Path::new("/tmp")))) .collect(); - // Only the first chunk can rendezvous; the short one must not wait for a - // full batch that will never arrive. - let runner = Overlapping::expecting(1); + let runner = Overlapping::expecting(STATUS_TRIPS_AT_ONCE); let states = container_states(&runner, &workspaces); diff --git a/rust/devlaunch-core/src/flows/repo_manager.rs b/rust/devlaunch-core/src/flows/repo_manager.rs index 8193804f..8acd8b21 100644 --- a/rust/devlaunch-core/src/flows/repo_manager.rs +++ b/rust/devlaunch-core/src/flows/repo_manager.rs @@ -1835,9 +1835,11 @@ pub(crate) mod tests { } /// Something a test wants to happen when a given argv is spawned. - /// `Send + Sync` because [`Runner`] is: the listing fans its `devpod status` - /// round trips out across threads, so anything a test hangs off a spawn has to - /// be shareable too. + /// `Send + Sync` because a `FakeGit` has to be `Sync`, and it can only be that + /// if what it holds is. [`Runner`] itself requires `Sync` alone, which is what + /// lets the listing fan its `devpod status` round trips out across threads; the + /// `Send` here is the ordinary companion bound on a boxed closure rather than + /// anything the trait asks for. type Effect = Box; impl FakeGit { From aef6c2bbb796c4ad24562a7786b4e5c2e15e3eeb Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Tue, 1 Sep 2026 08:50:32 +0000 Subject: [PATCH 04/11] fix: the fan-out width was a second copy with nothing diffing it `STATUS_TRIPS_AT_ONCE` is stated in prose five times on `docs/performance.md` -- "batches of eight", "a pool of eight permits", "eight is a conservative pick", and the worked example "forty workspaces cost five batches" twice over. That is a second hand-maintained copy of one number, which this repository allows only with a test beside it that diffs the copies. Nothing would have caught the drift. `test_bench_doc.py` reads this page for the bench harness rows and `test_docs_prose.py` reads it for em dashes; neither holds a sentence to being true. Tuning the constant is a one-character edit in a file no doc guard reads, and the page would go on saying eight. Measured rather than argued: set the constant to 16 and this guard fails twice, once on the width and once on the arithmetic ("the page says 40 workspaces cost 5 batches, but at a width of 16 they cost 3"). The instrument is the sentences the page already writes, so a rewrite that drops the claim fails here rather than passing quietly, and `CHANGELOG.md` is out of scope for the same reason `test_citations_resolve.py` exempts it. The regexes match letters rather than `\w`, because the page also writes "Five batches of 0.45s" and a `\w+` reads that duration as the width. Claude-Session: https://claude.ai/code/session_011BUMqTboYui66dxqxfRCo1 --- test/test_status_width_agrees.py | 128 +++++++++++++++++++++++++++++++ 1 file changed, 128 insertions(+) create mode 100644 test/test_status_width_agrees.py diff --git a/test/test_status_width_agrees.py b/test/test_status_width_agrees.py new file mode 100644 index 00000000..786cbe93 --- /dev/null +++ b/test/test_status_width_agrees.py @@ -0,0 +1,128 @@ +"""`docs/performance.md` spells the listing's fan-out width, so the two are diffed. + +`STATUS_TRIPS_AT_ONCE` in `rust/devlaunch-core/src/flows/listing.rs` decides how +many `devpod status` round trips `dl --ls` has in flight at once. The performance +page states that width in prose, and states two figures derived from it, because +a reader deciding whether a slow `--ls` is worth reporting needs to know the shape +of the wait. That makes the page a second hand-maintained copy of one number, +which this repository allows only with a test beside it that diffs the copies. + +Nothing else would catch it. `test_bench_doc.py` reads this page for the bench +harness rows and `test_docs_prose.py` reads it for em dashes; neither holds a +sentence to being true. Tuning the constant to sixteen is a one-character edit in +a file no doc guard reads, and the page would go on saying eight. + +The instrument is the sentences the page already writes: "batches of eight", "a +pool of eight permits", "eight is a conservative pick". None is a marker added for +this test's benefit, so a rewrite that drops the claim fails here rather than +passing quietly. + +`CHANGELOG.md` states the width too and is deliberately out of scope: it records +what was true when it was written, which is the same reason +`test_citations_resolve.py` exempts it. +""" + +from __future__ import annotations + +import math +import re +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent + +PAGE = REPO_ROOT / "docs" / "performance.md" +SOURCE = REPO_ROOT / "rust" / "devlaunch-core" / "src" / "flows" / "listing.rs" + +WIDTH = re.compile(r"^const STATUS_TRIPS_AT_ONCE: usize = (\d+);", re.MULTILINE) + +# The three ways the page states the width, none of them written for this test. +# Letters only, deliberately: the page also writes "Five batches of 0.45s", and a +# `\w+` here would read that duration as the width. +CLAIMS = re.compile( + r"batches of ([A-Za-z]+)|pool of ([A-Za-z]+) permits" + r"|and ([A-Za-z]+) is a conservative pick" +) + +# "forty workspaces cost five batches", the one worked example on the page. +WORKED = re.compile(r"([A-Za-z]+) workspaces cost ([A-Za-z]+) batches") + +NUMBERS = { + "one": 1, + "two": 2, + "three": 3, + "four": 4, + "five": 5, + "six": 6, + "seven": 7, + "eight": 8, + "nine": 9, + "ten": 10, + "twelve": 12, + "sixteen": 16, + "twenty": 20, + "thirty": 30, + "forty": 40, + "sixty": 60, + "sixty-four": 64, +} + + +def _spelled(word: str) -> int: + assert word.lower() in NUMBERS, ( + f"{PAGE.relative_to(REPO_ROOT)} spells a number this guard cannot read: " + f"{word!r}. Add it to NUMBERS rather than rewording the page around the test" + ) + return NUMBERS[word.lower()] + + +def _configured_width() -> int: + found = WIDTH.search(SOURCE.read_text(encoding="utf-8")) + assert found, ( + "STATUS_TRIPS_AT_ONCE is gone from " + f"{SOURCE.relative_to(REPO_ROOT)}. If the listing no longer batches its " + "status trips, retire this guard with the prose it diffs" + ) + return int(found.group(1)) + + +def _stated_widths() -> list[int]: + text = PAGE.read_text(encoding="utf-8") + return [_spelled(next(filter(None, match))) for match in CLAIMS.findall(text)] + + +def test_the_page_still_states_the_width(): + """A claim that vanished would make the diff below vacuously true.""" + assert _stated_widths(), ( + f"{PAGE.relative_to(REPO_ROOT)} no longer says how many status trips " + "`dl --ls` has in flight at once. Either restore the claim or retire this " + "guard with the copy it diffs" + ) + + +def test_the_page_agrees_with_the_configured_width(): + configured = _configured_width() + stated = set(_stated_widths()) + + assert stated == {configured}, ( + f"STATUS_TRIPS_AT_ONCE is {configured} and " + f"{PAGE.relative_to(REPO_ROOT)} says {sorted(stated)}. The constant is the " + "fact and the page is the copy, so change the page" + ) + + +def test_the_worked_example_divides_by_the_width(): + """The page's "forty workspaces cost five batches" is arithmetic, not a second claim.""" + configured = _configured_width() + worked = WORKED.search(PAGE.read_text(encoding="utf-8")) + assert worked, ( + f"{PAGE.relative_to(REPO_ROOT)} dropped the worked example that shows what " + "the width buys. Restore it or retire this guard with it" + ) + + workspaces, batches = (_spelled(word) for word in worked.groups()) + expected = math.ceil(workspaces / configured) + + assert batches == expected, ( + f"the page says {workspaces} workspaces cost {batches} batches, but at a " + f"width of {configured} they cost {expected}" + ) From 06f47c90d7613593c8e38ae01e6e531370c22dbf Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Tue, 1 Sep 2026 08:51:30 +0000 Subject: [PATCH 05/11] fix: the CHANGELOG said the promised API tier was untouched, and it is not "One row of `devlaunch-runner/public-api.txt` moves; the promised `api` tier is untouched" is true of the *file* and false of the *contract*, which is the opposite of what a reader of a breaking-change note needs. `devlaunch-core/public-api.api.txt` promises `CommandContext::new(&'r dyn Runner)` (line 283), `ColdPath::new`, `Refresh::ask` and `Provision::provision_tools`. Every one of them names a `dyn Runner`, and every one of them now accepts only a `Sync` implementation. The rendered rows are byte-identical, which is exactly why the file did not move -- `cargo public-api` renders the parameter as `&'r dyn devlaunch_runner::Runner` either way, so a supertrait tightening reaches the promised surface with no row to diff and no guard able to see it. The sentence one paragraph earlier already gets this right ("Any out-of-tree implementation holding a `RefCell`, `Rc` or `Cell` no longer compiles"), so this is a wrong reassurance sitting beside a correct warning, which is worse than either alone. Not testable as written: the defect is that the snapshot cannot observe the narrowing, so any guard I added here would assert on prose rather than on the contract. Named in the review report as the design question it actually is. Claude-Session: https://claude.ai/code/session_011BUMqTboYui66dxqxfRCo1 --- CHANGELOG.md | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ff09c570..a51f1450 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -168,8 +168,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 alternative, a `Sync` bound written at each call site that needs one, was rejected because it puts the requirement in the callers rather than in the contract and so permits an implementation that satisfies some callers and not - others. One row of `devlaunch-runner/public-api.txt` moves; the promised `api` - tier is untouched. + others. One row of `devlaunch-runner/public-api.txt` moves. + + `devlaunch-core/public-api.api.txt` does not move at all, and that is the part + to read twice rather than the reassurance it looks like. The promised tier hands + out `CommandContext::new(&'r dyn Runner)`, `ColdPath::new`, `Refresh::ask` and + `Provision::provision_tools`, and every one of them names a `dyn Runner` that + has just narrowed to `dyn Runner + Sync`. They render exactly as before, so the + promised contract tightened without a single row changing. The snapshot guards + compare rendered rows and cannot see a supertrait reach the promised surface + through a `dyn` it names, which is why this paragraph is the migration note and + the diff is not. A timing document for `dl --ls` reports smaller `devpod-up` **stage** seconds as a result, since that stage is now the wall time of the batch loop rather than From 5b8a2a21023bcfd9164cd1e02dd69b3eaa69d70d Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Tue, 1 Sep 2026 08:59:25 +0000 Subject: [PATCH 06/11] fix: a row's state was tied to its workspace by position alone `container_states` returned `Vec>` in input order and `enriched_listing` re-paired it with `.zip()`. Two things that shape permits, both silent, and the batched fan-out is what puts them in reach: - a short vector truncates the listing rather than failing, so `dl --ls --json` prints fewer workspaces than exist and exits 0; - a vector in completion order rather than spawn order gives every row a plausible `state` belonging to a different workspace. Neither was live. The point is that neither was excluded by anything except a `debug_assert_eq!` that is compiled out of the shipped binary and a six-line comment explaining what the positions mean, which is the invariant the type was supposed to carry. The diff's own test comment records the second one being measured slipping through four runs in five against an earlier shape. The worker now carries its workspace back out beside its answer, so the pairing is made where the trip is made and never re-derived. The compiler is the proof: there is no longer a bare vector to zip, mis-pair, or truncate, and `enriched_listing` maps the pairs straight through. The `debug_assert_eq!` and its comment delete with the hazard they described. `a_batch_larger_than_the_width_still_answers_for_every_workspace_in_order` now checks each pair against itself -- the fake echoes the id it was asked about as the state, so a row carrying another row's answer is visible inside the pair -- and keeps the order assertion separately. Verified by reversing each chunk's answers: the order assertion fires and every per-pair assertion still passes, which is the shape of the guarantee that was just added. Claude-Session: https://claude.ai/code/session_011BUMqTboYui66dxqxfRCo1 --- rust/devlaunch-core/src/flows/listing.rs | 68 +++++++++++++++--------- 1 file changed, 43 insertions(+), 25 deletions(-) diff --git a/rust/devlaunch-core/src/flows/listing.rs b/rust/devlaunch-core/src/flows/listing.rs index cc0bf696..a67d06c9 100644 --- a/rust/devlaunch-core/src/flows/listing.rs +++ b/rust/devlaunch-core/src/flows/listing.rs @@ -1023,20 +1023,8 @@ pub fn enriched_listing( let runner = context.runner(); // Every status trip first, together, because they are the only part of a row // that leaves this machine and they do not depend on each other. - let states = container_states(runner, &workspaces); - // `zip` stops at the shorter side, so a `container_states` that ever came back - // short would drop workspaces off the end of `dl --ls` rather than fail. It - // cannot today (one answer is pushed per workspace, and the empty case returns - // an empty vector), which is exactly why the invariant is worth stating where - // it is relied on. - debug_assert_eq!( - states.len(), - workspaces.len(), - "one state per workspace, or the listing loses rows" - ); - Ok(workspaces - .iter() - .zip(states) + Ok(container_states(runner, &workspaces) + .into_iter() .map(|(workspace, state)| enriched_row(&git, view, sizes, workspace, state)) .collect()) } @@ -1051,7 +1039,15 @@ pub fn enriched_listing( /// those the machine will schedule rather than by how many can compute at once. const STATUS_TRIPS_AT_ONCE: usize = 8; -/// The container state of each workspace, in the order they were given. +/// Each workspace with the container state devpod reported for it. +/// +/// The workspace is handed back beside its answer rather than the answers being +/// returned alone in input order. A bare `Vec>` would have +/// left the caller to re-pair the two by position, and position is precisely what +/// a batched fan-out puts at risk: a `zip` truncates in silence if the vector is +/// ever short, and a collector that read completion order rather than spawn order +/// would give every row a plausible state belonging to a different workspace. The +/// pair is built by the worker that made the trip, so neither is expressible. /// /// An answer devpod would not give reads as `None`, whichever way it would not /// give it: Python collapses every unreadable answer to `None` and the wire field @@ -1086,7 +1082,10 @@ const STATUS_TRIPS_AT_ONCE: usize = 8; /// the trips were serial. The stage fails if devpod could not be run at all, for /// exactly the reason the serial version failed it: a stage must not report `ok` /// for a step devpod never ran (P12). -fn container_states(runner: &dyn Runner, workspaces: &[Workspace]) -> Vec> { +fn container_states<'w>( + runner: &dyn Runner, + workspaces: &'w [Workspace], +) -> Vec<(&'w Workspace, Option)> { // Before the stage, not inside it: a listing with nothing to ask about opened // no `devpod-up` stage when the trips were made one at a time, because the // function that opened one was never reached. Opening it here regardless would @@ -1097,7 +1096,8 @@ fn container_states(runner: &dyn Runner, workspaces: &[Workspace]) -> Vec