diff --git a/CHANGELOG.md b/CHANGELOG.md index 591f359a..1f392d19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -148,6 +148,52 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **`dl --ls --json` asks its `devpod status` round trips together, and `Runner` + now requires `Sync`.** The `--json` is the whole of which command this is about: + the human table `dl --ls` prints has no state column and costs one `devpod list` + and nothing per row. The document is the surface carrying a `state` for every + workspace, and `devpod list` does not answer that, so the document asks devpod + about every workspace in it, including the ones devlaunch did not make. Those + trips are required and none has been removed. What the document 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. The number of + trips is unchanged, which is why the test that pins that cost reads exactly as + before. + + Measured against real devpod on one docker host: ten workspaces went from 5.13s + to 1.40s. That is about 3.7x rather than the 8x the width suggests, because a + trip costs about 38% more when eight of them are in flight (0.465s alone against + 0.641s in a batch). `docs/performance.md` carries the per-chunk figures. + + **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. + + `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 --json` 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, so the stage now reports less than the spans inside it add + up to. + - **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 7731369a..a19bcf73 100644 --- a/docs/performance.md +++ b/docs/performance.md @@ -20,6 +20,68 @@ 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 --json` is the one command whose cost grows with the machine, and the +`--json` is load-bearing: the human table `dl --ls` prints has no state column, so +it costs the single `devpod list` and nothing per row. The document is the surface +that carries a `state` for every workspace, and a state is a question `devpod list` +does not answer, so the document asks `devpod status` once per workspace, +including about the ones devlaunch did not make. That is one round trip per row +and there is no way around it: a document of forty workspaces asks forty +questions. `the_table_asks_devpod_for_the_list_and_nothing_else` holds the two +apart. + +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: +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. 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 --json` as +something else. + +**A trip also costs more when eight are in flight, so the win is not the width.** +Measured on 2026-09-01, on one host with ten workspaces on the local docker +provider and devpod 0.26.1: asked serially the ten trips averaged 0.465s each and +the `devpod-up` stage took 4.656s. Batched, the same ten averaged 0.641s each, +about 38% more, and the stage took 1.724s. The command went from 5.132s to 1.395s. +So read the win as roughly 3.7x rather than the 8x the width suggests, and read +"forty workspaces cost five batches" as five batches whose trips are each slower +than a lone one would be. + +That run shows the barrier's cost rather than describing it. The first chunk's +eight trips landed between 0.593s and 0.656s, and the second chunk, holding the +two rows left over, took 0.443s and 1.066s. The slower of those two is 62% of the +whole stage, and the work queue above would have spent the faster one's thread on +something instead of idling it. + +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. 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. What the figures above settle is that the contention +is real rather than hypothetical and that eight still pays; what they do not +settle is where the curve turns, since nothing has been measured at four or at +sixteen, 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 --json` 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. So the stage now reports less +than the spans inside it add up to, which is the one place this page's arithmetic +stops being addition. + +This is also why the document is something somebody asks for 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..40d4f498 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,147 @@ pub fn enriched_listing( let workspaces = context.workspaces()?; let git = context.git(); let runner = context.runner(); - Ok(workspaces - .iter() - .map(|workspace| enriched_row(runner, &git, view, sizes, workspace)) + // 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. + Ok(container_states(runner, &workspaces) + .into_iter() + .map(|(workspace, state)| enriched_row(&git, view, sizes, workspace, state)) .collect()) } -fn enriched_row( +/// 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; + +/// 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 +/// 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 +/// 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 document whose answer was +/// ready in two. Only `--json` pays this: the human table has no state column +/// and asks devpod nothing per row +/// (`the_table_asks_devpod_for_the_list_and_nothing_else`). +/// +/// 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). +/// +/// **One path here is still unexercised.** A capture pipes stdout and stderr but +/// not stdin, and `/dev/tty` stays reachable either way, so a provider that +/// prompts (an ssh host-key confirmation, a passphrase, a git credential) prompts +/// from inside one of these trips. The children stay in dl's own process group, +/// so they are in the terminal's foreground group and SIGTTIN is not the hazard; +/// eight of them legitimately reading one terminal is, and +/// [`Patience::AsLongAsItTakes`] means nothing times out of it. A docker-only host +/// cannot reach it: ten workspaces on the local provider opened `/dev/tty` zero +/// times and took no SIGTTIN (2026-09-01, devpod 0.26.1). It needs a listing run +/// against a remote provider to close. +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 + // put an empty stage in the timing document of every `dl --ls --json` 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<(&'w Workspace, Option)> = + 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() + // The worker carries the workspace back out with its answer, so + // the two are married where the trip is made rather than re-paired + // by position after the fact. + .map(|workspace| { + scope.spawn(move || { + ( + workspace, + 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 (workspace, answer) in batched { + never_ran |= matches!(answer, Err(devpod::StatusUnreadable::NotRun(_))); + answers.push((workspace, answer.ok())); + } + } + + if never_ran { + stage.fail(); + } + answers +} + +fn enriched_row( 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 +1202,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, @@ -1123,32 +1254,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. -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 +1537,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, @@ -1463,12 +1569,12 @@ 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 - /// 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. + /// [`container_states`] opens the `devpod-up` stage on the **process-global** + /// registry, once per listing, so a listing built without the guard writes + /// into whatever document a concurrent measured test installed and 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. fn new() -> Self { Self { devpod: FakeRunner::new(), @@ -2768,6 +2874,262 @@ 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, + /// The timing exclusion, for the reason [`FakeDevpodRealGit::new`] gives. + /// Measured rather than feared: without this field, running these tests + /// beside `launch`'s + /// `a_warm_launch_reports_the_devpod_probe_and_the_attach_and_nothing_else` + /// failed 12 runs in 15, and 0 in 25 with it. + /// + /// 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)] + struct Overlap { + in_flight: usize, + high_water: usize, + /// Arrivals at the barrier that has not tripped yet, reset each time one + /// does. Not `in_flight`, because a thread already released decrements + /// that on its way out, so waiting on it lets the last arrival free + /// itself and leave the earlier ones waiting for a number that has just + /// gone back down -- a ten second timeout per run. And not a cumulative + /// count of arrivals ever, because `chunks` releases one batch and then + /// starts another: a total that only goes up is already past `want` when + /// the second batch arrives, so every batch after the first sails through + /// without waiting for anything and never overlaps. + waiting: usize, + /// Barriers tripped, one per batch that actually rendezvoused. What lets + /// a test assert the overlap it is claiming coverage of happened in + /// *every* batch rather than only the first. + released: usize, + } + + impl Overlapping { + fn expecting(want: usize) -> Self { + Self { + state: Mutex::new(Overlap::default()), + arrived: Condvar::new(), + want, + _serialized: timing::exclusive(), + } + } + + fn high_water(&self) -> usize { + self.state.lock().expect("the overlap").high_water + } + + /// How many batches rendezvoused, rather than sailing past the barrier. + fn released(&self) -> usize { + self.state.lock().expect("the overlap").released + } + } + + 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.high_water = state.high_water.max(state.in_flight); + state.waiting += 1; + if state.waiting >= self.want { + // The last arrival trips the barrier and re-arms it for the next + // batch. Re-arming is the whole difference from a cumulative + // count, and it is what makes the second chunk overlap too. + state.waiting = 0; + state.released += 1; + self.arrived.notify_all(); + } else { + let mine = state.released; + while state.released == mine { + 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 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 --json` on an empty machine. + // + // So the document is what this reads. Asserting on the round trips alone + // cannot fail against the defect it names: `[].chunks(8)` yields no chunks, + // so deleting the early return leaves `states` empty and the trip count + // zero either way. It also has to *install* a registry, because + // `timing::stage` is a no-op while `RECORDING` is false, and a test that + // installed none is asserting against a stage guard that does nothing. + let runner = Overlapping::expecting(1); + + timing::install(Some(timing::Registry::start( + timing::Mode::Document, + timing::Seam::default(), + 0.0, + ))); + let states = container_states(&runner, &[]); + let report = timing::emit().expect("a report"); + let document = report.document().expect("a document"); + + assert!(states.is_empty(), "no workspaces, no answers"); + assert_eq!( + runner.high_water(), + 0, + "an empty listing asks devpod nothing" + ); + let staged: Vec<&str> = document.stages.iter().map(|stage| stage.stage).collect(); + assert!( + !staged.contains(&"devpod-up"), + "an empty listing reported a devpod-up stage it never spent: {staged:?}" + ); + } + + #[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. + // + // 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. + // + // `released` is asserted rather than trusted, because the claim above was + // false for half the input until the barrier learned to re-arm: with a + // cumulative arrival count the second chunk was already past `want` when + // it arrived and never waited for anything, so it contributed no overlap + // and no reordering coverage at all. + 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(); + let runner = Overlapping::expecting(STATUS_TRIPS_AT_ONCE); + + let states = container_states(&runner, &workspaces); + + // Each pair is checked against *itself* rather than against a second + // positional vector: 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 does not need the input order to detect. + for (workspace, state) in &states { + match state { + Some(ContainerState::Unknown(word)) => assert_eq!( + word, &workspace.id, + "this row carries another workspace's state" + ), + other => panic!("the fake answers its own id as the state: {other:?}"), + } + } + let answered: Vec<&str> = states + .iter() + .map(|(workspace, _)| workspace.id.as_str()) + .collect(); + assert_eq!( + answered, ids, + "every workspace answered for, in the order it was given" + ); + assert_eq!( + runner.released(), + 2, + "both chunks have to rendezvous, or the second one covers no reordering" + ); + assert_eq!( + runner.high_water(), + STATUS_TRIPS_AT_ONCE, + "a whole chunk is in flight at once, and never more than one chunk" + ); + } + #[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..8acd8b21 100644 --- a/rust/devlaunch-core/src/flows/repo_manager.rs +++ b/rust/devlaunch-core/src/flows/repo_manager.rs @@ -1835,7 +1835,12 @@ pub(crate) mod tests { } /// Something a test wants to happen when a given argv is spawned. - type Effect = Box; + /// `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 { pub(crate) fn new() -> Self { @@ -1869,7 +1874,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; 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!( 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}" + )