Ask the listing's questions together

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

docs/performance.md

@@ -20,6 +20,47 @@ naming it and then the tools probe, rides a single setup pass. So an
 interactive `dl <ws>` and a one-shot
 `dl <ws> -- <cmd>` 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 <ws> -- <cmd>` at a terminal is OpenSSH, over the host

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<Vec<String>>) {
-        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<Vec<Vec<String>>>,
+    calls: &'a Mutex<Vec<Vec<String>>>,
     /// 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")
     );
 }