From ee6ba5d445ffde024fd8fec446f57f00535a9585 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Thu, 3 Sep 2026 10:21:38 +0000 Subject: [PATCH] fix: name the agent binary devpod picked from a hostname Section 3 of blooop/devlaunch#560, as far as dl can honestly reach it. devpod chooses which agent binary to inject by globbing `uname -a` for `arm`, and `uname -a` prints the nodename beside the machine. So a container whose hostname holds `arm` reads as an ARM machine, devpod downloads the arm64 agent, the version check cannot execute it, and the launch dies with `exit status 126` -- "not executable", which names neither the architecture nor the word that chose it. dl is one of the ways the word gets there: its setup pass sets the container's hostname to the workspace id, and a workspace id is derived from the branch, so `feature/armature` is enough. A refused `devpod up` of a workspace whose id contains `arm` now carries one line of dl's own, beside devpod's sentence and without changing the exit code. The line is a conditional, and that is the honest limit rather than hedging. dl runs the `up` as a passthrough, because an image build's progress belongs on the user's terminal rather than through a pipe, so dl never reads devpod's message: what it holds is a nonzero exit and a name. That is enough to know the trap is set and not enough to know it fired, so the sentence says what to look for in devpod's own output above it. Reading that output instead would mean piping the build through dl -- changing what devpod renders, and dropping the process group that lets a Ctrl-C tear a build down rather than orphan it holding the launch lock, since `Runner::session` starts its child with `OwnGroup::No` where the `up` needs `Yes` (#304). `clients::devpod::reads_as_arm` transcribes devpod's four globs, uncase-folded because a shell `case` is, and answers for a machine name as readily as for a hostname -- which is what the exemption reads: a host that is itself ARM gets no line, because there devpod's guess is right. The upstream fix is one line (glob `uname -m`) and is not here. Claude-Session: https://claude.ai/code/session_01Dn4qJGhkW4KdwQnMuNSXsN --- CHANGELOG.md | 32 ++++-- docs/cli.md | 18 +++ docs/rust-rewrite-plan.md | 1 + rust/devlaunch-core/public-api.rest.txt | 2 + rust/devlaunch-core/src/clients/devpod.rs | 101 ++++++++++++++++ rust/devlaunch-core/src/flows/launch.rs | 20 +++- rust/dl/src/launch.rs | 9 +- rust/dl/src/render.rs | 133 +++++++++++++++++++++- rust/dl/tests/launch.rs | 73 ++++++++++++ rust/dl/tests/launch_scenario.py | 15 +++ 10 files changed, 390 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ae619585..99ee5e9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,15 +49,29 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 attach that runs no `up` prints neither line, because it asked devpod for nothing. -- **`docs/cli.md` now names the cause of `inject agent … exit status 126`.** devpod - picks its agent binary by globbing `uname -a` for `arm`, and `uname -a` carries - the container's hostname, so a workspace whose branch contains `alarm`, `warm`, - `charm`, `swarm`, `harm` or `armature` gets the arm64 agent on an x86 host and a - launch that dies saying only "not executable". `dl` writes the workspace id into - that hostname itself, so it is one of the ways the name gets there. The entry - carries the one-line check, the way to unblock a container that is already in - that state, and the reason a recreate undoes it. The match itself is devpod's and - is not fixed here. +- **A failed `up` of a workspace whose id contains `arm` names the agent binary + devpod picked.** devpod chooses which agent to inject by globbing `uname -a` for + `arm`, and `uname -a` carries the container's hostname, so a workspace whose + branch contains `alarm`, `warm`, `charm`, `swarm`, `harm` or `armature` gets the + arm64 agent on an x86 host and a launch that dies saying only `exit status 126`, + which means "not executable" and names neither the architecture nor the word that + chose it. `dl` writes the workspace id into that hostname itself, so it is one of + the ways the name gets there. + + **The line is a conditional, and deliberately.** `devpod up` runs with `dl`'s own + terminal, because an image build's progress belongs on your screen rather than + through a pipe, so `dl` never reads devpod's message: what it holds when the + build fails is an exit code and a name. That is enough to know the trap is set + and not enough to know it fired, so the sentence says what to look for in + devpod's own output above it instead of asserting what happened. An `up` that + failed on an image pull for a workspace called `alarm-clock` gets one sentence it + can ignore, and a host whose own architecture is ARM gets none, because there + devpod's guess is the right one. + + `docs/cli.md` carries the whole diagnosis: the one-line check for any running + container, the `docker cp` that unblocks one already in that state, and why a + recreate undoes it. The match itself is devpod's, upstream, and is not fixed + here. ### Changed diff --git a/docs/cli.md b/docs/cli.md index d95e4f72..34ebd81f 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -677,6 +677,24 @@ branch contains `alarm`, `warm`, `charm`, `swarm`, `harm`, `farm` or `armature` a candidate, and `dl`'s own setup pass puts the workspace id into the hostname, so `dl` is one of the ways the name gets there. +`dl` says this itself when an `up` refuses for a workspace whose id contains +`arm`, and says it as a conditional: + +``` +If devpod said 'inject agent' and 'exit status 126' above, this is why: the workspace id +devlaunch-feature-armature-17uu contains 'arm', devpod picks its agent binary by matching +'uname -a' against '*arm*', ... +``` + +**It is a conditional because `dl` never reads devpod's message.** `devpod up` +runs with `dl`'s own terminal, so an image build's progress goes straight to you +rather than through `dl`, and what `dl` holds when the build fails is an exit code +and a workspace id. That is enough to know the trap is set and not enough to know +it fired, so the line tells you what to look for in devpod's output above it. An +`up` that failed on an image pull for a workspace called `alarm-clock` gets one +sentence it can ignore. Nothing is said on a host whose own architecture is ARM, +where devpod's guess is the right one. + ```bash # the mechanism, in any running container docker exec sh -c 'case "$(uname -a)" in *arm*) echo ARM;; *) echo NOT;; esac' diff --git a/docs/rust-rewrite-plan.md b/docs/rust-rewrite-plan.md index 8d1857e8..70f8e7fc 100644 --- a/docs/rust-rewrite-plan.md +++ b/docs/rust-rewrite-plan.md @@ -198,6 +198,7 @@ Nothing diverges silently. | 32 | **`--rm` is docker's `--rm`, and the flag-spelled verbs are retired.** `dl --rm` and `dl --rm -- ` now hand over a session and delete the workspace once it ends — what row 30's `--autorm` did, under the name docker gives it. The word `rm` is unchanged and is the only way to delete one *now*, so `docker rm` / `docker run --rm` is the whole of the grammar and no spelling has to be read twice. Three withdrawals pay for it, each refused at exit 1 rather than reinterpreted: `--autorm` (`--autorm is now spelled --rm: …`); `--stop`, whose only reason to exist was being a flag (rows 15 and 30) and which cannot stay a *cancelling* suffix beside a `--rm` that runs the line; and row 30's suffix override itself, so `aid 'review this pr' --rm` now runs the review and deletes afterwards where it used to delete instead, and `dl prune --rm` is row 31's refusal rather than a removal (with `--force` also on the line it is the `--force`-beside-`--rm` refusal instead, named first because the pair is the more confused half). `--force` still does not compose with `--rm` — docker keeps `-f` on `rm` too — and `dl rm --rm` is refused as the two requests it is. Retired flags are answered *before* anything else the line got wrong, since both moved on account of `--rm`'s new meaning. aid peels the retired spellings so dl refuses them by name instead of joining them into a prompt, and builds no agent command for such a line, so nothing is booted on the way to exit 1. `Overridden`, `pick_target` and the `--rm overrode the rest of the line` notice are gone with the override. | Row 30 bought "and now delete it" as a suffix and paid for it with a flag whose meaning was unguessable from its spelling: `--rm` cancelled the line, `--autorm` ran it, and the two looked like a pair. docker had already split the same problem the other way — a verb for now, a `run` flag for after — and taking that split makes the common line (`aid --rm`: send the agent in, get the disk back) the one the short spelling names, at the cost of the rarer one, which `dl rm` and a pick does in fewer keystrokes than recalling a long prompt to append to it. The withdrawals are recognised rather than deleted, for row 31's reason: a flag dropped from `Cli` is clap's `unexpected argument` at exit 2, naming the spelling and not the replacement — and the spelling is exactly what cannot explain a line that stopped working because a *different* flag changed meaning. Pinned by the `retired_flag` / `rm_` tests in `dl/src/cli.rs`, `the_retired_flag_spellings_name_the_words_that_replaced_them` and `autorm_is_refused_with_the_spelling_that_replaced_it` in `dl/tests/lifecycle.rs`, the `rm_on_exit_` tests in `dl/tests/launch.rs`, and `a_retired_spelling_starts_no_agent_and_is_handed_to_dl_to_refuse` in `aid/src/rewrite.rs`. | | 33 | **A warm launch of a triple says how far behind its checkout is.** `dl owner/repo@branch` against a workspace devpod already knows prints one line before the attach banner when the clone's `HEAD` is behind the `refs/remotes/origin/` that clone last fetched, naming the count both ways (`its checkout is 37 commits behind origin/main … (and 3 of its own it has not pushed)`). Python printed nothing, and neither did earlier Rust. The launch still runs no fetch: the report is one `rev-list` against a local repository, so it says how the checkout stands against a ref of whatever age and never claims to know the remote. Silent when the counts agree, when the checkout is only ahead, when there is no clone on disk, for a bare workspace name (no triple, so no branch to name and no clone path to derive), and for a warm resolution addressed by an id `metadata.json` recorded rather than the one the triple derives (devlaunch#88's arm, where the derived clone path is not this container's source). `reset`'s help line and the README table stopped saying "Clean slate: remove everything, recreate" in the same change, because that promise reads as one about the checkout and `reset` cannot keep it. | The warm arm makes no git call and never will (devlaunch#144, built by #149 and #150), and a launch that looks like it verified new work when it verified neither the commit nor the image invalidates whatever was concluded inside the container. A fetch on attach was the tempting fix and is the wrong trade; the fact was available locally the whole time. Pinned by the `checkout`/`warm_triple` tests in `flows::launch` and by `a_warm_triple_whose_checkout_is_behind_says_how_far_and_still_runs_no_git_fetch` plus `a_bare_workspace_name_reports_no_checkout_however_stale_it_is` in `dl/tests/launch.rs`, whose world is `launch_scenario.py`'s `--stale-checkout`. blooop/devlaunch#560 §1. | | 34 | **Every `devpod up` says whether it forwarded dotfiles.** One line per `up`: the repository and script it passed, or that devpod's context options name none and where to set one. Python read `devpod context options`, silently forwarded `--dotfiles`/`--dotfiles-script` or silently omitted them, and left no way from the terminal to tell which had happened. An attach that runs no `up` prints neither line. | Three plausible causes and no observation to cut between them is what turned "the dotfiles never landed" into a fortnight: `DOTFILES_URL` is read out of `devpod context options` and out of nothing else, not the process environment and not `~/.devpod/config.yaml`, and `context-options.json` is dl's *cache* of that answer rather than an input. The line costs nothing on a path that is already spawning a container build, and it makes a silent policy checkable. Pinned by `an_up_says_which_dotfiles_it_asked_devpod_for_and_the_argv_agrees` in `dl/tests/launch.rs`, which asserts the sentence and the argv together so the two cannot drift. blooop/devlaunch#560 §2. | +| 35 | **A refused `devpod up` of a workspace whose id contains `arm` gains one line of dl's own.** The refusal still renders nothing else and still exits with devpod's status; this is a second line beside it, phrased as a conditional (`If devpod said 'inject agent' and 'exit status 126' above, this is why: …`) naming the workspace id, the `*arm*` glob, the hostname `uname -a` carries, and the `docker cp` that unblocks a container already in that state. Python printed nothing. Silent for every other refusal, for an id with no `arm` in it, and on a host whose own architecture reads as ARM, where devpod's guess is correct. | devpod's `is_arm` globs `uname -a`, which prints the nodename beside the machine, so a branch named `armature` or `alarm` gets the arm64 agent on an x86 host and a launch that dies saying only "not executable" -- and dl is one of the ways the name gets there, since its setup pass sets the container's hostname to the workspace id. **The conditional is not hedging, it is the honest limit**: `devpod up` runs as a passthrough so its progress reaches the terminal directly, dl therefore never reads devpod's message, and what it holds is an exit code and a name. Matching devpod's stderr instead would mean piping the build through dl -- changing what devpod renders and, since `Runner::session` starts its child with `OwnGroup::No`, dropping the process group that lets a Ctrl-C `killpg` a build instead of orphaning it holding the launch lock (row 27, #304). One conditional sentence on a failed launch is the cheaper trade. `clients::devpod::reads_as_arm` is the transcription of devpod's four globs and answers for a machine name as readily as for a hostname, which is what the ARM-host exemption reads. Pinned by the `reads_as_arm` tests in `clients/devpod.rs`, the `arm_agent_hint` tests in `dl/src/render.rs`, and `a_failed_up_of_an_arm_named_workspace_names_the_agent_binary_devpod_picked` plus `an_ordinary_workspace_whose_up_failed_gets_no_arm_explanation` in `dl/tests/launch.rs`, whose world is `launch_scenario.py`'s `--arm-branch`. blooop/devlaunch#560 §3, whose upstream half (glob `uname -m`) is not fixed here. | Additions require a PR that updates this table; the row number is cited by any per-binary harness branch. diff --git a/rust/devlaunch-core/public-api.rest.txt b/rust/devlaunch-core/public-api.rest.txt index ccbc9205..b1954f99 100644 --- a/rust/devlaunch-core/public-api.rest.txt +++ b/rust/devlaunch-core/public-api.rest.txt @@ -118,6 +118,7 @@ pub fn devlaunch_core::clients::devpod::Workspace::eq(&self, &devlaunch_core::cl impl core::fmt::Debug for devlaunch_core::clients::devpod::Workspace pub fn devlaunch_core::clients::devpod::Workspace::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::marker::StructuralPartialEq for devlaunch_core::clients::devpod::Workspace +pub fn devlaunch_core::clients::devpod::reads_as_arm(&str) -> bool pub mod devlaunch_core::clients::devpod_home pub enum devlaunch_core::clients::devpod_home::RepointFailure pub devlaunch_core::clients::devpod_home::RepointFailure::NotADevpodRecord @@ -1514,6 +1515,7 @@ pub devlaunch_core::flows::launch::LaunchRefusal::UnknownWorkspace::name: alloc: pub devlaunch_core::flows::launch::LaunchRefusal::UnsafeSpec(devlaunch_core::domain::workspace_id::UnsafeName) pub devlaunch_core::flows::launch::LaunchRefusal::UpRefused pub devlaunch_core::flows::launch::LaunchRefusal::UpRefused::exit: devlaunch_runner::Exit +pub devlaunch_core::flows::launch::LaunchRefusal::UpRefused::workspace_id: alloc::string::String impl core::clone::Clone for devlaunch_core::flows::launch::LaunchRefusal pub fn devlaunch_core::flows::launch::LaunchRefusal::clone(&self) -> devlaunch_core::flows::launch::LaunchRefusal impl core::cmp::Eq for devlaunch_core::flows::launch::LaunchRefusal diff --git a/rust/devlaunch-core/src/clients/devpod.rs b/rust/devlaunch-core/src/clients/devpod.rs index 3963059b..b1dda8dd 100644 --- a/rust/devlaunch-core/src/clients/devpod.rs +++ b/rust/devlaunch-core/src/clients/devpod.rs @@ -259,6 +259,53 @@ pub(crate) fn says_it_is_blocked(line: &str) -> bool { line.contains("Trying to lock workspace") } +/// The globs devpod's injected shell script tests `uname -a` against to decide +/// it is on an ARM machine. +/// +/// Transcribed from `is_arm` in devpod's `pkg/inject/inject.sh`, whose four +/// patterns are `*arm*`, `*arm64*`, `*aarch*` and `*aarch64*`. Two of those are +/// subsumed by the other two -- anything holding `arm64` holds `arm` -- and they +/// are kept anyway, because this is a mirror of somebody else's list and a +/// mirror that has been tidied is one nobody can diff against the original. +const DEVPOD_ARM_GLOBS: [&str; 4] = ["arm", "arm64", "aarch", "aarch64"]; + +/// Whether devpod would read a container named this way as an ARM machine. +/// +/// **This is a bug in devpod that devlaunch walks into by name** (devlaunch#560). +/// `uname -a` prints the *nodename* along with the machine, so the test above is +/// satisfied by a hostname as readily as by an architecture, and +/// `createBinaryLoader` in `pkg/agent/inject.go` takes the boolean straight +/// through to `arch = "arm64"`. The wrong agent downloads, `performVersionCheck` +/// cannot execute it, and the launch dies with `exit status 126` -- "not +/// executable", which is true and names neither the architecture nor the string +/// that chose it. +/// +/// devlaunch is one of the ways the string gets there: the setup pass runs +/// `sudo hostname ` in every container it opens, and a workspace id +/// is derived from the branch. So `feature/armature` becomes +/// `devlaunch-feature-armature-` becomes a container that reads as ARM, and +/// every branch holding `alarm`, `warm`, `charm`, `swarm`, `harm` or `farm` is the +/// same trap. +/// +/// Asked of the workspace id rather than of a `uname -a` this process cannot see, +/// which is what makes it a *prediction* and the reason its one caller phrases the +/// answer as a possibility. dl runs `devpod up` as a passthrough -- an image build +/// belongs on the user's terminal -- so devpod's own sentence is never read, and a +/// reader has to be told what to look for in it rather than told it happened. +/// +/// The fix is upstream and one line: glob `uname -m`, which prints the machine +/// alone. `blooop/devpod`'s `fix/detect-arch-with-uname-m` carries it. +/// +/// **Case-sensitive, because a shell `case` is.** Nothing is folded here even +/// though a workspace id arrives lowercased by [`slug`](crate::domain::workspace_id) +/// and so could not carry an `ARM` anyway: this predicate answers "would devpod's +/// script match", and a fold would make it answer something devpod does not ask. +pub fn reads_as_arm(container_name: &str) -> bool { + DEVPOD_ARM_GLOBS + .iter() + .any(|glob| container_name.contains(glob)) +} + /// An outcome, split into "it ran, this is what came back" and "it did not". /// /// One function over all four arms rather than a `_ =>` at each call site: the @@ -2370,4 +2417,58 @@ mod tests { StatusUnreadable::NotRun(NotRun::NotInstalled) ); } + + // ------------------------------------------- devpod's own arch detection + + #[test] + fn a_name_holding_arm_anywhere_reads_as_arm_because_the_glob_is_a_substring() { + // devlaunch#560 §3. Not a word match and not a prefix: devpod's script + // globs `*arm*`, so an ordinary English branch name is enough. Every name + // below is one somebody would write without a thought about architecture. + for name in [ + "devlaunch-feature-armature-17uu", + "app-alarm-clock-3j1t", + "dl-warm-start-np10", + "x-charm-1", + "x-swarm-1", + "x-harm-1", + "x-farm-1", + "repo-aarch-notes-1", + ] { + assert!(reads_as_arm(name), "{name}"); + } + } + + #[test] + fn a_name_with_no_arm_in_it_reads_as_nothing() { + for name in [ + "devlaunch-main-3j1t", + "devlaunch-feature-auth-np10", + "", + // The letters, out of order and separated: a substring is a substring. + "mar-ram-amr", + ] { + assert!(!reads_as_arm(name), "{name}"); + } + } + + #[test] + fn the_glob_is_not_folded_because_a_shell_case_is_not() { + // A faithful mirror of somebody else's `case` matters more here than being + // generous: devpod would not match this, so neither may the prediction. A + // workspace id cannot carry it anyway -- `slug` lowercases -- which is + // exactly why a fold would be an untestable kindness. + assert!(!reads_as_arm("devlaunch-ARM-3j1t")); + } + + #[test] + fn the_same_predicate_answers_for_a_machine_as_for_a_hostname() { + // `uname -a` prints both, which is the whole bug, so one glob covers both + // questions and the caller that asks "is this host itself ARM" needs no + // second list. + assert!(reads_as_arm("aarch64")); + assert!(reads_as_arm("arm64")); + assert!(reads_as_arm("armv7l")); + assert!(!reads_as_arm("x86_64")); + } } diff --git a/rust/devlaunch-core/src/flows/launch.rs b/rust/devlaunch-core/src/flows/launch.rs index d21a1d26..63588a43 100644 --- a/rust/devlaunch-core/src/flows/launch.rs +++ b/rust/devlaunch-core/src/flows/launch.rs @@ -3773,7 +3773,14 @@ pub enum LaunchRefusal { error: NotPrepared, }, /// `devpod up` refused. devpod has already said why on the user's stderr. - UpRefused { exit: Exit }, + /// + /// `workspace_id` is carried for the renderer's sake and not for the exit + /// code, which is `exit` alone. dl runs the `up` as a passthrough, so devpod's + /// own sentence is the whole of what a reader gets and dl never sees its text; + /// the id is the one thing dl knows about the failure that devpod's sentence + /// does not spell out, and one failure mode is decided by it -- see + /// [`devpod::reads_as_arm`]. + UpRefused { workspace_id: String, exit: Exit }, /// The stop half of a `restart` refused, so nothing was started. StopRefused { exit: Exit }, /// No session could be composed or handed over. @@ -4273,7 +4280,10 @@ impl<'a, 'r, 'l> Launch<'a, 'r, 'l> { ) .map_err(LaunchAborted::DevpodNotRun)?; if let UpOutcome::Refused { exit } = outcome { - return Ok(Launched::Refused(LaunchRefusal::UpRefused { exit })); + return Ok(Launched::Refused(LaunchRefusal::UpRefused { + workspace_id: placement.workspace_id().to_owned(), + exit, + })); } } let session = SessionContext::new( @@ -4317,7 +4327,10 @@ impl<'a, 'r, 'l> Launch<'a, 'r, 'l> { .map_err(LaunchAborted::DevpodNotRun)?; let refused = match outcome { UpOutcome::Started | UpOutcome::SkippedSiblingWon => None, - UpOutcome::Refused { exit } => Some(LaunchRefusal::UpRefused { exit }), + UpOutcome::Refused { exit } => Some(LaunchRefusal::UpRefused { + workspace_id: placement.workspace_id().to_owned(), + exit, + }), }; // Asked here rather than by whoever renders the refusal, because *when* it is // asked is Python's control flow rather than a rendering decision: for `up` @@ -10312,6 +10325,7 @@ mod tests { assert_eq!( launched, Ok(Launched::Refused(LaunchRefusal::UpRefused { + workspace_id: "myws".to_owned(), exit: Exit::Code(7) })), "{verb:?}" diff --git a/rust/dl/src/launch.rs b/rust/dl/src/launch.rs index d1e74ac2..c1ca9d08 100644 --- a/rust/dl/src/launch.rs +++ b/rust/dl/src/launch.rs @@ -280,6 +280,13 @@ fn ran(outcome: Result, cache: &Path) -> Ran { { eprintln!("{hint}"); } + // The architecture this binary was built for, which is the host's: + // read here rather than inside the renderer, so the renderer stays a + // function of its arguments and a test can ask it about a host it is + // not running on. + if let Some(hint) = render::arm_agent_hint(&refused, std::env::consts::ARCH) { + eprintln!("{hint}"); + } match refused { // devpod's own status back, and nothing printed: its diagnostics // are already on this process's stderr, since the call inherits @@ -291,7 +298,7 @@ fn ran(outcome: Result, cache: &Path) -> Ran { // point: a build that failed in `postCreateCommand` leaves the // container running and the clone cut, so this is exactly the // workspace an unattended `--rm` line is there to collect. - LaunchRefusal::UpRefused { exit } => Ran { + LaunchRefusal::UpRefused { exit, .. } => Ran { ending: Ending::Child(exit), reached: Reached::TheWorkspace, }, diff --git a/rust/dl/src/render.rs b/rust/dl/src/render.rs index 001bdefc..cebd34fa 100644 --- a/rust/dl/src/render.rs +++ b/rust/dl/src/render.rs @@ -10,7 +10,7 @@ use std::io; use std::path::Path; use std::process; -use devlaunch_core::clients::devpod::{ListingUnreadable, NotAListing, NotRun, Workspace}; +use devlaunch_core::clients::devpod::{self, ListingUnreadable, NotAListing, NotRun, Workspace}; use devlaunch_core::clients::devpod_home::RepointFailure; use devlaunch_core::clients::gh::{GhEvent, GhUnavailable}; use devlaunch_core::clients::git::Failure as GitFailure; @@ -3028,6 +3028,50 @@ pub(crate) fn launch_refusal(refused: &LaunchRefusal) -> Option { } } +/// The second line a `devpod up` refused over the wrong agent binary deserves, or +/// nothing to add. +/// +/// **What it can and cannot claim, which is the whole design of the sentence** +/// (devlaunch#560 §3). devpod picks its injected agent by globbing `uname -a` for +/// `arm`, and `uname -a` carries the container's hostname, so a container named +/// after a branch holding `arm` gets the arm64 agent and the launch dies with +/// devpod's `inject agent: … exit status 126`. dl walks into this by name: its +/// setup pass sets the container's hostname to the workspace id. +/// +/// dl does not see that sentence. `devpod up` runs as a passthrough, because an +/// image build's progress belongs on the user's terminal and not through a pipe, +/// so what dl holds is a nonzero exit and the workspace id. That is enough to +/// predict the trap and not enough to confirm it fired, and the line says so in +/// those words: it tells the reader what to look for in devpod's own output above +/// it rather than asserting what happened. An `up` that failed on an image pull +/// for a workspace that happens to be called `alarm-clock` gets one conditional +/// sentence it can discard on sight, which is the price of not piping the build. +/// +/// Silent on every other refusal, and silent on a host whose *own* architecture +/// reads as ARM -- there devpod's guess is right and the failure is something +/// else. Both questions go through [`devpod::reads_as_arm`], since "would that +/// glob match" is one predicate whether it is asked of a hostname or of a machine. +/// +/// Pure like everything else here: the architecture is read by the caller and +/// passed in. +pub(crate) fn arm_agent_hint(refused: &LaunchRefusal, host_arch: &str) -> Option { + let LaunchRefusal::UpRefused { workspace_id, .. } = refused else { + return None; + }; + if !devpod::reads_as_arm(workspace_id) || devpod::reads_as_arm(host_arch) { + return None; + } + Some(format!( + "If devpod said 'inject agent' and 'exit status 126' above, this is why: the workspace id \ + {workspace_id} contains 'arm', devpod picks its agent binary by matching 'uname -a' \ + against '*arm*', and 'uname -a' includes the container's hostname — which dl sets to \ + that id. So devpod fetched the arm64 agent for a container on a host reporting \ + {host_arch}, and could not execute it. Rename the branch, or copy the right binary in \ + with 'docker cp \"$(command -v devpod)\" :/usr/local/bin/devpod' and \ + reconnect; a recreate wipes that." + )) +} + /// The second line a wrong-owner spec deserves, or nothing to add. /// /// The case it answers is a mistyped or half-remembered *owner* — @@ -4051,6 +4095,92 @@ mod tests { assert_eq!(or_list(&three), "'a/r', 'b/r' or 'c/r'"); } + // ------------------------------- the agent binary devpod picked by hostname + + fn up_refused(workspace_id: &str) -> LaunchRefusal { + LaunchRefusal::UpRefused { + workspace_id: workspace_id.to_owned(), + exit: Exit::Code(1), + } + } + + #[test] + fn a_failed_up_of_an_arm_named_workspace_names_the_trap_as_a_possibility() { + // devlaunch#560 §3. The sentence has to be conditional and has to say what + // to look for, because dl runs the `up` as a passthrough and never reads + // devpod's message: what it holds is a nonzero exit and a name. Asserting + // that the failure *was* the 126 would be dl claiming to have read + // something it did not. + let line = arm_agent_hint(&up_refused("devlaunch-feature-armature-17uu"), "x86_64") + .expect("a hint"); + + assert!(line.starts_with("If devpod said"), "{line}"); + assert!(line.contains("exit status 126"), "{line}"); + assert!( + line.contains("devlaunch-feature-armature-17uu"), + "the name that chose the binary: {line}" + ); + assert!( + line.contains("uname -a") && line.contains("hostname"), + "the mechanism, so the reader can check it: {line}" + ); + assert!( + line.contains("docker cp"), + "the way out that does not rebuild: {line}" + ); + assert!( + line.contains("a recreate wipes that"), + "and why not to reach for recreate: {line}" + ); + assert!( + line.contains("a host reporting x86_64"), + "the architecture is the caller's to supply, and it reaches the line: {line}" + ); + } + + #[test] + fn a_host_that_is_itself_arm_gets_no_hint_because_devpods_guess_was_right() { + // On aarch64 the arm64 agent is the correct one, so a failed `up` here is + // some other failure and this line would send the reader after nothing. + for arch in ["aarch64", "arm64", "armv7l"] { + assert_eq!( + arm_agent_hint(&up_refused("devlaunch-feature-armature-17uu"), arch), + None, + "{arch}" + ); + } + } + + #[test] + fn a_failed_up_of_an_ordinary_workspace_keeps_devpods_own_wording_alone() { + // The refusal renders nothing of dl's own (devpod's diagnostics are already + // on this process's stderr), and this must not become the exception. + assert_eq!( + arm_agent_hint(&up_refused("devlaunch-main-3j1t"), "x86_64"), + None + ); + assert_eq!(launch_refusal(&up_refused("devlaunch-main-3j1t")), None); + } + + #[test] + fn no_other_refusal_gains_the_hint_however_the_workspace_is_named() { + // Only a refused `up` injects an agent on the way to failing. A stop that + // refused, or a session that could not be opened, is a different failure + // and an explanation of devpod's arch detection would be a wrong answer + // rather than an unhelpful one. + let refusals = [ + LaunchRefusal::StopRefused { + exit: Exit::Code(9), + }, + LaunchRefusal::UnknownWorkspace { + name: "devlaunch-alarm-1".to_owned(), + }, + ]; + for refused in refusals { + assert_eq!(arm_agent_hint(&refused, "x86_64"), None, "{refused:?}"); + } + } + fn table(rows: Vec) -> WorkspaceTable { WorkspaceTable::Rows(NonEmpty::of(rows).expect("at least one row")) } @@ -4318,6 +4448,7 @@ mod tests { // branch at the call site. assert_eq!( launch_refusal(&LaunchRefusal::UpRefused { + workspace_id: "myws".to_owned(), exit: Exit::Code(7) }), None diff --git a/rust/dl/tests/launch.rs b/rust/dl/tests/launch.rs index 05fae6ca..a4565b57 100644 --- a/rust/dl/tests/launch.rs +++ b/rust/dl/tests/launch.rs @@ -40,6 +40,12 @@ const MAIN: &str = "devlaunch-main-3j1t"; /// The same for `blooop/devlaunch@cold`, which nothing has ever launched. const COLD: &str = "devlaunch-cold-8iyb"; +/// The same for `blooop/devlaunch@armature`, the `--arm-branch` fixture's branch. +/// The `arm` in the middle of it is the whole point: it is there because somebody +/// named a branch after a word, and devpod's agent injection reads it as a +/// processor (blooop/devlaunch#560 §3). +const ARM: &str = "devlaunch-armature-cyi9"; + /// The repository root, from the crate this test is compiled into. fn repo_root() -> PathBuf { Path::new(env!("CARGO_MANIFEST_DIR")) @@ -1649,6 +1655,73 @@ fn a_devpod_up_that_refuses_hands_its_own_status_back_and_adds_nothing() { ); } +#[test] +fn a_failed_up_of_an_arm_named_workspace_names_the_agent_binary_devpod_picked() { + // blooop/devlaunch#560 §3, as far as dl can reach it. devpod globs `uname -a` + // for `arm` to choose which agent binary to inject, `uname -a` prints the + // nodename beside the machine, and dl's setup pass sets that nodename to the + // workspace id — so `armature` is enough to get the arm64 agent on an x86 + // host, and the launch dies with `exit status 126`, which reads as "not + // executable" and names neither the architecture nor the word that chose it. + // + // dl cannot confirm that is what happened: `devpod up` is a passthrough, so + // what dl holds is a nonzero exit and a name. The line is therefore a + // conditional pointing at devpod's own output above it, and it lands after + // that output for exactly that reason. + let world = World::with(&["--arm-branch", "--fail-up"]); + let run = world.dl(&["blooop/devlaunch@armature"]); + // devpod's status, unchanged: the hint is a line, not a different ending. + run.exited(7); + + let lines = run.stderr_lines(); + let devpod = lines + .iter() + .position(|line| *line == "devpod: image pull failed") + .unwrap_or_else(|| panic!("devpod said nothing: {lines:?}")); + // On a host that is itself ARM devpod's guess is right and dl says nothing, so + // the assertion follows the binary rather than assuming the runner. + if devlaunch_core::clients::devpod::reads_as_arm(std::env::consts::ARCH) { + assert_eq!( + lines.len(), + devpod + 1, + "an ARM host was told its own architecture was a mistake: {lines:?}" + ); + return; + } + assert_eq!( + lines.get(devpod + 1).copied(), + Some( + format!( + "If devpod said 'inject agent' and 'exit status 126' above, this is why: the \ + workspace id {ARM} contains 'arm', devpod picks its agent binary by matching \ + 'uname -a' against '*arm*', and 'uname -a' includes the container's hostname — \ + which dl sets to that id. So devpod fetched the arm64 agent for a container on \ + a host reporting {}, and could not execute it. Rename the branch, or copy the \ + right binary in with 'docker cp \"$(command -v devpod)\" \ + :/usr/local/bin/devpod' and reconnect; a recreate wipes that.", + std::env::consts::ARCH + ) + .as_str() + ), + "{lines:?}" + ); +} + +#[test] +fn an_ordinary_workspace_whose_up_failed_gets_no_arm_explanation() { + // The negative that keeps the line from being noise on every failed build. + // `devlaunch-cold-8iyb` holds no `arm`, so the same failure says only what + // devpod said. + let world = World::with(&["--fail-up"]); + let run = world.dl(&["blooop/devlaunch@cold"]); + run.exited(7); + assert!( + !run.err.contains("inject agent"), + "{}", + run.stderr_lines().join("\n") + ); +} + #[test] fn a_session_devpod_could_not_start_ends_with_devpods_own_status() { let world = World::with(&["--warm", "--fail-session"]); diff --git a/rust/dl/tests/launch_scenario.py b/rust/dl/tests/launch_scenario.py index 9ce9c1d0..045a6e93 100755 --- a/rust/dl/tests/launch_scenario.py +++ b/rust/dl/tests/launch_scenario.py @@ -10,6 +10,7 @@ launch_scenario.py [--warm] [--stopped] [--gh] [--no-devpod] [--no-workspaces] [--stale-checkout] [--dotfiles] + [--arm-branch] The base world, under the root it is given: @@ -38,6 +39,12 @@ the clone's `HEAD` is behind its own `refs/remotes/origin/main`, which is a fact the clone holds and no launch of a workspace devpod already knows goes looking for. +- `--arm-branch`: a third branch on `origin.git`, `armature`, with no clone and no + record -- so `dl blooop/devlaunch@armature` is a cold launch whose derived id + contains `arm`. Beside `--fail-up` that is the world blooop/devlaunch#560 §3 is + about: devpod's injected script globs `uname -a` for `arm` and `uname -a` carries + the hostname dl sets from that id, so the name alone decides which agent binary + gets downloaded. The branch name is an ordinary English word on purpose. - `--dotfiles`: `devpod context options` names a dotfiles repository, which is the only place dl reads one from. Without it the fake devpod answers `{}`, so a launch forwards no `--dotfiles` flag -- which is the state a fortnight of #560 @@ -79,6 +86,11 @@ # The branch with no clone and no record: the cold launch's. COLD_BRANCH = "cold" +# The branch whose derived id contains `arm`, for the fixture of the same name. +# An ordinary word: the point of #560 §3 is that nobody writing this name is +# thinking about a processor architecture. +ARM_BRANCH = "armature" + # A token of the shape every GitHub token has, so the fake `gh` is believed. TOKEN = "gho_devlaunchtesttoken0123456789" @@ -181,6 +193,8 @@ def build(root: pathlib.Path, shim: pathlib.Path, wanted: set) -> None: git(seed, "add", "-A") git(seed, "commit", "-q", "-m", "seed") git(seed, "branch", COLD_BRANCH) + if "arm-branch" in wanted: + git(seed, "branch", ARM_BRANCH) origin = root / "origin.git" git(root, "clone", "-q", "--bare", "seed", "origin.git") @@ -296,6 +310,7 @@ def build(root: pathlib.Path, shim: pathlib.Path, wanted: set) -> None: FIXTURES = { "warm", + "arm-branch", "stopped", "stale-checkout", "dotfiles",