Ask the listing's questions together - #540
Merged
Merged
Conversation
`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.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's Guide
Sequence diagram for concurrent workspace status listingsequenceDiagram
participant CLI as dl --ls
participant Listing as enriched_listing
participant Runner as Runner
participant Devpod as devpod status
CLI->>Listing: enriched_listing()
Listing->>Listing: container_states()
loop Batches of up to 8 workspaces
par Independent status requests
Listing->>Runner: capture(status workspace 1)
Runner->>Devpod: status workspace 1
Devpod-->>Runner: state 1
Runner-->>Listing: answer 1
and
Listing->>Runner: capture(status workspace 2)
Runner->>Devpod: status workspace 2
Devpod-->>Runner: state 2
Runner-->>Listing: answer 2
end
Listing->>Listing: Append answers in workspace order
end
Listing-->>CLI: Enriched listing
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
`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.
Codecov Report❌ Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
Owner
Author
|
A fresh-context adversarial review of this branch found a defect in the two tests I added, plus six smaller things. All are addressed in The tests were corrupting other tests' timing documents
Measured rather than reasoned about. Running either new test beside
The full suite hides it, so this would have surfaced as an unreproducible flake on a narrowed filter or a re-run. It also runs in both directions: the listing's stage can be the one a launch test's The order guard was weak
It is now two full chunks at the full width, so every trip in a chunk is released together. Verified that a build which reverses answers within a chunk fails it, and that the honest version passes. The rest
One open question, now settledThe review could not determine whether any out-of-tree Still not verifiedReal devpod under eight concurrent Validation on Generated by Claude Code |
`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
…s 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
`container_states` returned `Vec<Option<ContainerState>>` 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
…trips `docs/performance.md` opened with "`dl --ls` is the one command whose cost grows with the machine ... because the `STATE` column is reported for every row", and sized it at "about eighteen seconds" on a forty-workspace machine. The CHANGELOG entry and two comments in `listing.rs` said the same. There is no `STATE` column. `dl --ls` renders the human table, whose header is `WORKSPACE TYPE SOURCE LAST USED` (`dl/src/render.rs:125`), built by `workspace_table` from the single `devpod list` and nothing else. `render_table`'s own doc has said "Still one devpod round trip" since before this branch, and `the_table_asks_devpod_for_the_list_and_nothing_else` has pinned it, one screen below the tests this PR added: "the table has no state column, so `dl --ls` is one devpod round trip where `dl --ls --json` is one per workspace." `enriched_listing` has exactly one caller, `render_json` (`dl/src/commands.rs:273`). So every per-workspace trip this PR parallelised belongs to `dl --ls --json`, and a reader sizing `dl --ls` off this page was told to expect eighteen seconds for a command that costs one round trip. I wrote a fresh guard for this before finding the existing one, and deleted it rather than leave a second copy: the repository's standing rule is that a second hand-maintained copy of a fact needs a test diffing it against the first, and here the test already existed and the prose simply contradicted it. It is cited by name from the page now, so the two are read together. Verified the guard is not vacuous rather than trusting it: making `workspace_table` call `container_states` fails it with six devpod calls against the one it demands. Also corrected here, from the same reading: the page and the CHANGELOG now say that `devpod-up` stage seconds are smaller *than the spans inside the stage add up to*, which is the counterintuitive half and the only place the timing document stops being addition. Claude-Session: https://claude.ai/code/session_011BUMqTboYui66dxqxfRCo1
`an_empty_listing_asks_nothing_and_opens_no_stage` asserted that no trips were made and that the answers were empty. Neither can distinguish the early return from its absence: `[].chunks(8)` yields no chunks, so with the `is_empty` guard deleted the loop body never runs, `states` is still empty and `high_water` is still 0. Measured, not argued -- deleting the guard and running the test passes. The named property is "opens no stage", and the test never looked at a stage. Worse, it could not have: `timing::stage` returns a guard with `stage: None` while `RECORDING` is false, and no registry was installed, so the stage guard under test was a no-op in the only run that exercised it. It now installs a document registry the way `lifecycle`'s `a_devpod_that_cannot_be_run_fails_the_stage_but_one_that_refuses_does_not` does, emits, and asserts `devpod-up` is absent from the stages. With the early return deleted it fails with "an empty listing reported a devpod-up stage it never spent: [\"devpod-up\"]", which is the defect the test was written for. Claude-Session: https://claude.ai/code/session_011BUMqTboYui66dxqxfRCo1
…t was inert `Overlap::arrivals` counted arrivals ever and the rendezvous waited for it to reach `want`. `chunks` releases one batch and then starts another, so by the time the second chunk's first trip arrived the total was already 9, `9 < 8` was false, and every trip in that chunk returned without waiting for anything. That makes the comment on `a_batch_larger_than_the_width_still_answers_for_every_workspace_in_order` false for half its input. It says "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", and by its own next clause -- "a trip that never overlaps another cannot reorder anything" -- the second chunk was contributing no reordering coverage at all. The test still caught the defect on chunk one, so this is dead coverage behind a false claim rather than a dead test, which is the kind that rots quietly: the sixteen ids read like twice the coverage of eight. The barrier now re-arms. `waiting` resets when the batch trips it, so each chunk rendezvouses on its own, and `released` counts the trips so the property is asserted rather than described. `high_water` is asserted at exactly `STATUS_TRIPS_AT_ONCE` too, which is deterministic here for the same reason: no trip leaves before the eighth arrives, and the next chunk cannot start until this one is joined. Verified by stopping the barrier re-arming: `released` comes back 9 rather than 2 -- one real trip plus the eight that sailed past -- and the test fails. Ran the fixed version eight times for the two new deterministic assertions; ok every time. Claude-Session: https://claude.ai/code/session_011BUMqTboYui66dxqxfRCo1
…once `FakeDevpodRealGit::new`'s doc had a stray break leaving `/// and` alone on a line, so rustdoc rendered "...a concurrent measured test installed, and its stage guard closes..." with a gap in the middle of the clause. Rewrapped, and the "enriched listing built without the guard" phrasing dropped for "listing", since the sentence was already carrying the qualifier twice. `Overlapping::_serialized` then restated the same paragraph in full -- the process-global registry, the span every worker records, the stage guard closing another test's stage. It now cites `FakeDevpodRealGit::new` and keeps only what is its own: the measurement, 12 failures in 15 without the field and 0 in 25 with it. The other two statements of the staging argument are deliberately left. They are not copies of this one or of each other: `container_states`'s doc block argues why the stage is opened once around the whole batch, and the inline comment above the early return argues why it is opened after the empty check rather than before. Different claims about different lines. Claude-Session: https://claude.ai/code/session_011BUMqTboYui66dxqxfRCo1
…enied Verified against real devpod on a host, which the devcontainer cannot do. Ten workspaces on the local docker provider, devpod 0.26.1, 2026-09-01. Correctness, ordering and attribution across the chunk boundary all hold: five runs byte-identical, and the one `NotFound` row sits at row 9, the first row of the second chunk, which is where a reordering or an off-by-one would move it. Two claims on `docs/performance.md` were wrong, both in the optimistic direction: - "Five batches of 0.45s is therefore the figure to expect." A trip costs about 38% more when eight are in flight: 0.465s asked alone against 0.641s in a batch. The whole command went 5.132s to 1.395s, which is about 3.7x and not the 8x the width implies. The page and the CHANGELOG now carry the measured numbers. - "How that trades against the extra overlap has not been measured here." It has now, at eight. What is still unmeasured is where the curve turns, since nothing has run at four or sixteen, and the page says that instead. The barrier paragraph gets the evidence it was arguing from: the first chunk's eight trips landed between 0.593s and 0.656s, and the second chunk's two took 0.443s and 1.066s, so one outlier is 62% of the stage. The work queue the page names as the better shape would have saved the faster thread's wait, which is now a figure rather than an assertion. Both timing claims held exactly. Span count unchanged at 10; the serial stage equals its span sum to three decimals (4.656s) while the concurrent stage reports 1.724s against 6.554s of spans. `container_states` now records the one path nothing has exercised, with the hazard stated correctly. SIGTTIN is not it: the children stay in dl's own process group, so under a pty they are in the foreground group. The hazard is eight children legitimately reading one terminal with no timeout to escape it. A docker-only host cannot reach it, and this one did not: zero `/dev/tty` opens and no SIGTTIN across the run. It needs a remote provider to close. Claude-Session: https://claude.ai/code/session_011BUMqTboYui66dxqxfRCo1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes the per-workspace
devpod statusround trips ofdl --ls --jsonoverlap instead of running one at a time. Follow-up from the clean-room reconstruction (#539, merged).What it does, and which command it is
dl --ls --json, notdl --ls. I originally credited this todl --lsand sized it at "about eighteen seconds" on a forty-workspace machine. Wrong. The human table has no state column, sodl --lscosts the singledevpod listand nothing per row;enriched_listinghas exactly one caller,render_json. The document is what carries astateper workspace, anddevpod listdoes not answer that, so--jsonasksdevpod statusonce per row.the_table_asks_devpod_for_the_list_and_nothing_elsehad pinned that distinction all along, in the file I was editing.Those trips are required and none is removed. What the listing no longer does is wait for each answer before asking the next: they are independent, so they go out in batches of eight.
Measured, on real devpod
Ten workspaces, local docker provider, devpod 0.26.1, 2026-09-01. This closes the item I had flagged as unverifiable from the devcontainer, and it corrects two more of my claims, both optimistic in the same direction:
devpod-upstageThe win is about 3.7x, not the 8x the width implies. I had written "five batches of 0.45s is therefore the figure to expect", which assumed a trip costs the same under contention — the one assumption a bounded pool exists to question. I had also written that the contention trade "has not been measured here" as though it were a neutral gap; it was the load-bearing unknown in my own headline number.
The barrier now has evidence rather than an assertion behind it: the first chunk's eight trips landed between 0.593s and 0.656s, and the second chunk's two took 0.443s and 1.066s, so one outlier is 62% of the stage. A work queue starting the ninth trip the moment any of the first eight returned would have spent the faster thread instead of idling it. Still unmeasured: where the curve turns, since nothing has run at four or sixteen.
Correctness held. Five runs byte-identical; ordering and attribution hold across the chunk boundary, with the single
NotFoundrow landing at row 9, the first row of the second chunk, which is where a reordering or off-by-one would move it. Both timing claims held to three decimals.RunnergainsSync, and it does reach the promised contractHanding one
&dyn Runnerto several threads means the seam must promise it can be shared. That binds every future implementation: a runner holding aRefCell,RcorCellno longer compiles.Correcting my earlier claim: the promised
apitier is not untouched.public-api.api.txtpromisesCommandContext::new(&'r dyn Runner),ColdPath::new,Refresh::askandProvision::provision_tools, and each now accepts only aSyncimplementation. The rows are byte-identical, which is why no snapshot moved:cargo public-apiprints the parameter the same either way, so the tightening reaches the promised surface with no row to diff and no guard able to see it. I wrote "the promisedapitier is untouched" after finding those four signatures myself; it is true of the file and false of the contract.Rest of the cost: production code nothing (
ProcessRunneris a unit struct); three test wrappers took theRefCelltoMutexchange a shared recorder wants anyway. No out-of-tree implementation breaks today — wayfinder does not linkdevlaunch-coreat all, no devlaunch entry in itsCargo.lock— but that is a fact about today, not a guarantee.State is paired by construction, not by position
container_statesfirst returned states in input order andenriched_listingre-paired with.zip(), which permits a short vector truncating the listing at exit 0, and a completion-order vector giving each row a plausiblestatefrom a different workspace. Neither was live; neither was excluded by anything but adebug_assert_eq!compiled out of shipped builds. The worker now carries its workspace back out beside its answer.Timing
One stage around the whole batch, not one per trip: the registry admits one owner per stage, so per-trip staging would have had one thread close
devpod-upwhile its siblings ran. An empty listing returns before opening one. A worker's panic is carried withresume_unwind.devpod-upstage seconds are now the batch loop's wall time, so the stage reports less than the spans inside it add up to — 1.724s against 6.554s measured. That is the one place the timing document stops being addition.Where my own tests did not work
Four guards I wrote were inert or measured the wrong thing. Each is now verified against the defect it names:
the_status_trips_of_one_listing_overlapexpressed its bar asSTATUS_TRIPS_AT_ONCE, so forcing the width to 1 moved the bar with it and it passed against a deliberately serialised build.a_batch_larger_than_the_width_still_answers_for_every_workspace_in_orderused a rendezvous of one, so nothing ever overlapped and a completion-order collector slipped past it four runs in five. Widened to two chunks; then only the first chunk rendezvoused, so half was still inert.an_empty_listing_asks_nothing_and_opens_no_stagecould not fail against the defect it named.timing::exclusive(), which every other fixture in the module holds, so they wrote into whatever document a concurrent measured test had installed: 12 failures in 15 besidelaunch's timing test under--test-threads=8, 0 in 25 with the guard.test_status_width_agrees.pydiffs the fan-out width against the prose, which was another second copy of a number nothing read.Still open
The
/dev/ttyhazard is not closed, and my framing of it was mechanically wrong: I reached for SIGTTIN, but the children stay in dl's process group, so under a pty they are the foreground group. The real hazard is eight children legitimately reading one terminal with no timeout to escape. A docker-only host cannot reach it and this run did not: zero/dev/ttyopens, no SIGTTIN. It needs a remote provider to close.Not in this PR
reconcile.rsjoins by path and never by id, plus a resumable two-phase migration). Already in flight.every_row_carries_its_own_index_or_marking_cannot_accumulateshows indices already exist.