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",