diff --git a/CHANGELOG.md b/CHANGELOG.md index 591f359..ff09c57 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 7731369..9797716 100644 --- a/docs/performance.md +++ b/docs/performance.md @@ -20,6 +20,47 @@ 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: +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. 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. + ## 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 a218d67..0b9b0d2 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 7dffcbf..cc0bf69 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,126 @@ 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); + // `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() - .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. +/// +/// 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 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 +1181,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 +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. -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 +1516,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,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. @@ -2768,6 +2854,199 @@ 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 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)] + 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, + _serialized: timing::exclusive(), + } + } + + 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 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 + // 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. + 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); + + 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 e897be6..8acd8b2 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 96c5d7d..910ab6f 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 3bb4d92..b7efbfe 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 c892bb8..ab50768 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 603a720..13f3cdd 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!(