Ask the listing's questions together - #540
Open
blooop wants to merge 3 commits into
Open
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 |
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.
The
--lsfollow-up from the clean-room reconstruction (#539's "deliberately not fixed here"). Independent of that PR and based onmain.What was wrong
dl --lsreads the workspace list once and then asksdevpod statusabout every workspace in it, including the ones devlaunch did not make, because theSTATEcolumn is reported for every row.devpod listcarries no state, so those round trips are genuinely required and this PR does not remove any of them.What it did not have to do was wait for each answer before asking the next question. The trips are independent: 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 indl --ls. They now go out in batches of eight, so the same forty cost five batches.The count is unchanged, which is why
the_listing_costs_one_list_and_one_status_per_workspacereads exactly as it did. Only the waiting overlaps.Note this is a barrier, not a pool: a batch costs its slowest trip and the next does not start until the last returns. Five batches of 0.45s is the figure to expect when the trips are alike, and it is never worse than asking serially; a work queue starting the ninth trip the moment any of the first eight returned would be better on a machine where one workspace answers much more slowly.
docs/performance.mdsays so rather than claiming the ideal.The decision worth reviewing:
RunnergainsSyncHanding one
&dyn Runnerto several threads means the seam has to promise it can be shared. This is the part to push back on if you are going to, because it binds every future implementation: a runner that wants aRefCellinside it is no longer writable.The case for it is that the alternative is worse. A seam that can only be driven from one thread makes every concurrent flow above it impossible, and spelling the bound at each call site instead puts the requirement in the callers rather than in the contract, which permits an implementation that satisfies some callers and not others.
The cost, measured rather than estimated:
ProcessRunneris a unit struct. The workspace compiled with the supertrait added and no other change.RefCelltoMutexthat a shared recorder needs anyway (agent_worktrees/tests.rs,workspace_clone.rs, and one boxed closure inrepo_manager.rs).pub trait devlaunch_runner::Runner: core::marker::Syncindevlaunch-runner/public-api.txt. The promisedapitier is untouched.devlaunch-coreat all. ItsCargo.lockholds no devlaunch entry and itsCargo.tomlno devlaunch dependency; the only mentions in its tree are issue references and fixture strings. The frozenapitier is a prepared surfacewfhas not yet consumed.Two details not obvious from the diff
The stage is opened once around the whole batch, not once per trip. The registry admits one owner per stage, so per-trip staging would have had whichever thread opened
devpod-upclose it while its siblings were still running, and the spans they then recorded would have landed outside any stage. Opening it on the calling thread keeps the timing document reporting the shape it reported when the trips were serial.An empty listing returns before opening a stage at all. The serial version never reached the function that opened one, so a machine with no workspaces had no
devpod-upstage in its timing document. Opening one unconditionally would have added a reported step that did not happen.A worker's panic is carried through with
resume_unwindrather than replaced by a generic message, so a listing that panics still says why. Thedevpod-upstage seconds fordl --lsdo shrink, since that stage is now the batch loop's wall time rather than the sum of the rows; the spans and their count are unchanged.Testing
Three tests, each verified to fail against the defect it names rather than merely to pass:
the_status_trips_of_one_listing_overlappins the overlap with a rendezvous rather than a sleep: every trip announces itself and waits for one more, so overlap returns immediately and a serial build fails on the high-water mark after the timeout. It expects a literal2rather thanSTATUS_TRIPS_AT_ONCE, 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. Fails withhigh water 1when the width is forced to 1.a_batch_larger_than_the_width_still_answers_for_every_workspace_in_ordercovers the chunking. Two full chunks at the full width, so every trip in a chunk is released together and a collector reading completion order sees a shuffled chunk on essentially every run. The earlier shape (nine ids, a rendezvous of one) never overlapped anything and let a completion-order regression through about four runs in five. Verified: a build that reverses answers within a chunk fails it.an_empty_listing_asks_nothing_and_opens_no_stagepins the early return.All three hold
timing::exclusive()through the fake, as every other fixture in the module does. Without it they wrote into whatever document a concurrent measured test had installed: besidelaunch'sa_warm_launch_reports_the_devpod_probe_and_the_attach_and_nothing_elseunder--test-threads=8that failed 12 runs in 15, and 0 in 25 with the guard.cargo test --workspace: 1,560 pass.cargo clippy --all-targets -- -D warningsandcargo fmt --checkclean. Python suite 659 pass. The only failure is the pre-existinga_write_that_cannot_start_leaves_the_previous_file_readable, which relies onchmod 0o500blocking a write and so cannot pass as root; it fails identically on an unmodified tree in this container.Not verified here: real devpod under eight concurrent
statuscalls, there being no devpod in this container. CI'se2epasses with real devpod and real containers, which is partial evidence; if a provider can prompt on/dev/ttyduring a capture then eight children prompting one terminal at once is a path nothing has exercised. One manualDEVLAUNCH_TIMING=1 dl --lsagainst a remote provider would close it.Not in this PR
The other two follow-ups from the same reconstruction are design questions rather than defects, and neither is a code change I would make without a decision first:
reconcile.rsjoins by path and never by id, plus a resumable two-phase migration). Already in flight in recent commits.every_row_carries_its_own_index_or_marking_cannot_accumulateshows indices already exist.Also still open: roughly thirty tests pin behaviour byte-for-byte against the retired Python build, an oracle nothing can run any more.