fix(adoption): name the recovery an interrupted rollback accepts and drain isolated test output

docs/reference/native-compose-adoption.md

@@ -553,7 +553,60 @@ diagnostic; the literal-env message appears only on the first refusal. Plain
 override, so whenever `hack config adopt` refuses as busy over a pending switch
 its message adds the step that state accepts: `--recover --rollback` when the
 bound override holds literal values, otherwise `--recover` to complete or
-`--recover --rollback` to restore (HACK-1414).
+`--recover --rollback` to restore (HACK-1414). Once the switch's rollback is
+journaled (`rolling-back`, an interrupted rollback), plain `--recover` refuses
+whatever the override holds, so the message names only `--recover --rollback`
+to finish restoring the originals (HACK-1427).
+
+#### Rollback releases the generated env override (HACK-1437)
+
+A saved read binds each generated override in `.hack/.internal` by its exact
+bytes and file identity: device, inode, mode, link count, size, owner, mtime
+and ctime. Moving the file aside and back keeps its bytes and inode but
+changes its ctime, so the binding cannot be restored by hand. The binding
+protects two things. Adopted execution and switch completion must run exactly
+the inputs that preparation qualified and whose Compose config hash the
+retained containers carry, so a moved, edited or replaced file is treated as
+tampering and refused. And while an adoption is prepared, active or pending,
+`hack doctor --fix` and the env scrubs leave the bound file in place.
+
+Rollback is different. It restores the two held legacy originals, archives the
+candidate and verifies the original resources. It never reads, runs, rewrites
+or removes the generated env override, which is derived from the managed env
+layers and which the next `hack up` writes again. No data depends on it. So
+rollback (`--rollback` from `active`, and `--recover --rollback` from
+`switching` or `rolling-back`) reads the saved proof with the env override
+released: the override may be missing, moved back or replaced, and rollback
+neither reads it nor requires it. The saved `config_files` path list keeps the
+override's path, so each retained container is still matched against the
+exact label Compose recorded. Every other check stays exact: the managed env
+revision, the runtime override and the `.hack/.internal` directory identity,
+the saved config hashes, the original resource identities and stopped state,
+the held originals and the installed candidate. A change to any of those is
+still refused before anything moves. Execution and completion keep the full
+binding, so they still refuse a moved override.
+
+Before this change, moving the bound override aside left an interrupted
+rollback with no supported way out. `--recover --rollback` journaled
+`rolling-back` and then refused, plain `--recover` refuses a rollback, and the
+busy message named `--recover --rollback` again, so the user looped. Now
+`--recover --rollback` finishes in each pending state, and every step names
+only a command that the state does not refuse for a known reason:
+
+- `rolling-back`: `--recover --rollback`, whatever the override holds.
+- `switching` with a literal override: `--recover --rollback`.
+- `switching` with the bound override moved or changed, while the other
+  generated inputs still match: `--recover --rollback`, with a message that
+  completing refuses.
+- `switching` otherwise: `--recover` to complete it, or `--recover --rollback`
+  to restore the originals.
+
+A refused plain `--recover` adds the same step when the cause is one of these
+known states (`rolling-back`, or a moved or changed override). If rollback
+itself refuses, something it still checks has changed, such as a running
+original container, edited held originals or candidate, changed resources or
+managed env layers, or conflicting bytes at an original path. That refusal
+keeps the journal and recovery evidence for inspection.
 
 `hack doctor` reports the pinned literal override and names the first command the
 pending state accepts, since `hack down` and plain `--rollback` refuse in every
@@ -630,7 +683,10 @@ files, and preserve both legacy originals for rollback. Raw-byte or selection
 drift, unsafe managed-file permissions, generated-file identity changes, and
 original-resource drift refuse before a new
 effect; drift after an effect retains pending evidence. Rechecks do not freeze
-external editors or the engine. Old version 1/2 receipts remain readable; earlier
+external editors or the engine. Rollback is the one exception: it releases the
+generated env override, which it never reads or writes, and rechecks the rest
+(see "Rollback releases the generated env override"). Old version 1/2 receipts
+remain readable; earlier
 upgraded owners refuse version 3 through the existing selector boundary.
 
 The version 3 synthetic publication, saved-readback and raw-drift controls are

packages/runtime-core/src/provider/test_executable.rs

@@ -4,6 +4,7 @@ use std::{
     os::unix::fs::{OpenOptionsExt, PermissionsExt},
     path::Path,
     process::{Child, Command, Stdio},
+    thread::JoinHandle,
     time::{Duration, Instant},
 };
 
@@ -48,12 +49,69 @@ pub(super) fn sleeping_executable(binary: &Path) {
 /// may be heavily loaded, so this bounds a hang rather than the fixture's own timing.
 pub(super) const ISOLATED_HANG_GUARD: Duration = Duration::from_secs(120);
 
+/// Bytes of each child stream kept for a failure message. The readers always drain the
+/// whole stream, so the child is never blocked on a full pipe; only the tail, where libtest
+/// prints the failing assertion and the result line, is retained.
+const RETAINED_TAIL: usize = 16 * 1024;
+
+/// Largest stdout a passing child may print: one isolated test's libtest summary is far
+/// smaller, so more means the child ran something other than the requested fixture.
+const PASSING_STDOUT_LIMIT: usize = 4096;
+
+/// One child stream drained to its end on a reader thread, keeping a bounded tail.
+struct DrainedStream {
+    tail: Vec<u8>,
+    total: usize,
+}
+
+impl DrainedStream {
+    fn spawn(mut source: impl Read + Send + 'static) -> JoinHandle<Self> {
+        std::thread::spawn(move || {
+            let mut drained = Self {
+                tail: Vec::new(),
+                total: 0,
+            };
+            let mut chunk = [0_u8; 8192];
+            loop {
+                match source.read(&mut chunk) {
+                    Ok(0) => break,
+                    Ok(read) => {
+                        drained.total += read;
+                        drained.tail.extend_from_slice(&chunk[..read]);
+                        let excess = drained.tail.len().saturating_sub(RETAINED_TAIL);
+                        drained.tail.drain(..excess);
+                    }
+                    Err(error) if error.kind() == std::io::ErrorKind::Interrupted => {}
+                    Err(_) => break,
+                }
+            }
+            drained
+        })
+    }
+
+    fn text(&self) -> String {
+        let omitted = self.total - self.tail.len();
+        let tail = String::from_utf8_lossy(&self.tail);
+        if omitted == 0 {
+            tail.into_owned()
+        } else {
+            format!("[{omitted} earlier bytes omitted]\n{tail}")
+        }
+    }
+}
+
 /// Run one ignored test alone in a child process of this test binary and require its
 /// complete assertion path. Use it when a fixture's descriptors must not be shared with
 /// unrelated parallel tests: their pre-exec forks (`process::clean_command`) hold copies
 /// of every descriptor of this process until they exec, so a whole-host process scan or
 /// an `flock` release can observe this test's sockets, files or locks in another
 /// process. `bound` is a hang guard for launching the whole binary on a loaded host.
+///
+/// The child's stdout and stderr are drained on reader threads while it runs: a child
+/// that fails with more output than a pipe buffer holds must exit and report its own
+/// failure, not block on a write until the hang guard reports a watchdog instead. The
+/// bound covers both the child's exit and the readers reaching end of stream, so a
+/// descendant that keeps a copy of either pipe open is reported rather than waited on.
 pub(super) fn run_isolated(test: &str, bound: Duration) {
     struct OwnedChild(Child);
     impl Drop for OwnedChild {
@@ -67,11 +125,13 @@ pub(super) fn run_isolated(test: &str, bound: Duration) {
             .args(["--ignored", "--exact", test, "--test-threads=1"])
             .stdin(Stdio::null())
             .stdout(Stdio::piped())
-            .stderr(Stdio::inherit())
+            .stderr(Stdio::piped())
             .spawn()
             .unwrap(),
     );
     let started = Instant::now();
+    let stdout = DrainedStream::spawn(child.0.stdout.take().unwrap());
+    let stderr = DrainedStream::spawn(child.0.stderr.take().unwrap());
     let status = loop {
         if let Some(status) = child.0.try_wait().unwrap() {
             break status;
@@ -83,20 +143,80 @@ pub(super) fn run_isolated(test: &str, bound: Duration) {
         );
         std::thread::sleep(Duration::from_millis(1));
     };
-    let mut output = String::new();
-    child
-        .0
-        .stdout
-        .take()
-        .unwrap()
-        .take(4097)
-        .read_to_string(&mut output)
-        .unwrap();
-    assert!(output.len() <= 4096);
-    assert!(status.success(), "isolated test {test} failed: {output}");
+    while !(stdout.is_finished() && stderr.is_finished()) {
+        assert!(
+            started.elapsed() < bound,
+            "isolated test {test} exited with {status} but its output stayed open past its \
+             watchdog after {:?}",
+            started.elapsed()
+        );
+        std::thread::sleep(Duration::from_millis(1));
+    }
+    let stdout = stdout.join().unwrap();
+    let stderr = stderr.join().unwrap();
+    let (output, errors) = (stdout.text(), stderr.text());
     assert!(
-        output.contains(&format!("test {test} ... ok"))
+        status.success(),
+        "isolated test {test} failed with {status}\n--- stdout ---\n{output}\n--- stderr ---\n{errors}"
+    );
+    assert!(
+        stdout.total <= PASSING_STDOUT_LIMIT
+            && output.contains(&format!("test {test} ... ok"))
             && output.contains("test result: ok. 1 passed"),
-        "isolated test {test} assertions did not run: {output}"
+        "isolated test {test} assertions did not run\n--- stdout ---\n{output}\n--- stderr ---\n{errors}"
     );
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    /// More than a 64 KiB pipe buffer on each stream, so an unread pipe blocks the child.
+    const NOISY_BYTES: usize = 160 * 1024;
+    const NOISY_FAILURE: &str = "noisy isolated child failed on purpose";
+    const NOISY_STDERR_END: &str = "noisy isolated child stderr end";
+
+    #[test]
+    #[ignore = "deliberately failing child launched by isolated_child_failure_with_large_output_is_reported"]
+    fn noisy_failure_child() {
+        let line = "x".repeat(1023);
+        // libtest captures this and prints it with the failure, on stdout, before the panic.
+        for _ in 0..NOISY_BYTES / 1024 {
+            println!("{line}");
+        }
+        // Direct writes bypass libtest's capture and reach the stderr pipe as written.
+        let mut stderr = std::io::stderr().lock();
+        for _ in 0..NOISY_BYTES / 1024 {
+            writeln!(stderr, "{line}").unwrap();
+        }
+        writeln!(stderr, "{NOISY_STDERR_END}").unwrap();
+        drop(stderr);
+        panic!("{NOISY_FAILURE}");
+    }
+
+    /// A child whose failure output overflows both pipes must still exit and report its own
+    /// failure: an unread pipe would block it until the hang guard hid the real failure
+    /// behind a watchdog panic.
+    #[test]
+    fn isolated_child_failure_with_large_output_is_reported() {
+        let test = "provider::test_executable::tests::noisy_failure_child";
+        let started = Instant::now();
+        let panic = std::panic::catch_unwind(|| run_isolated(test, ISOLATED_HANG_GUARD))
+            .expect_err("a failing isolated child must fail its parent");
+        let message = panic.downcast_ref::<String>().cloned().unwrap_or_default();
+        assert!(
+            started.elapsed() < ISOLATED_HANG_GUARD,
+            "{:?}",
+            started.elapsed()
+        );
+        assert!(!message.contains("watchdog"), "{message}");
+        assert!(
+            message.starts_with(&format!("isolated test {test} failed with ")),
+            "{message}"
+        );
+        assert!(message.contains(NOISY_FAILURE), "{message}");
+        assert!(message.contains(NOISY_STDERR_END), "{message}");
+        assert!(message.contains("earlier bytes omitted"), "{message}");
+        assert!(message.len() < 3 * RETAINED_TAIL, "{}", message.len());
+    }
+}

src/commands/config-adopt.ts

@@ -100,26 +100,107 @@ export const PENDING_SWITCH_BUSY_NEXT_STEP =
   "A publication switch is pending. If no other adoption command is running, run 'hack config adopt --recover' to complete it or 'hack config adopt --recover --rollback' to restore the originals.";
 export const PENDING_LITERAL_SWITCH_BUSY_NEXT_STEP =
   "A publication switch is pending and the bound .hack/.internal/compose.env.override.yml holds literal env values, so completing it refuses. If no other adoption command is running, run 'hack config adopt --recover --rollback' to release it.";
+/**
+ * The step for a switch whose rollback is journaled (`rolling-back`): plain
+ * `--recover` completes only a `switching` publication and refuses this one,
+ * whatever the override holds, so only `--recover --rollback` finishes it
+ * (HACK-1427). Fixed text; values omitted.
+ */
+export const PENDING_ROLLBACK_BUSY_NEXT_STEP =
+  "A rollback of the publication switch is pending. If no other adoption command is running, run 'hack config adopt --recover --rollback' to finish restoring the originals.";
+/**
+ * The step for a `switching` publication whose bound env override was moved
+ * or changed since preparation: completing it refuses, because its saved read
+ * binds the override's bytes and identity (ctime included, so moving it back
+ * does not help), while rollback releases the override and restores the
+ * originals (HACK-1437). Fixed text; values omitted.
+ */
+export const PENDING_CHANGED_SWITCH_BUSY_NEXT_STEP =
+  "A publication switch is pending and its bound .hack/.internal/compose.env.override.yml was moved or changed since preparation, so completing it refuses. If no other adoption command is running, run 'hack config adopt --recover --rollback' to restore the originals.";
+
+/**
+ * Whether the pending switch's bound env override changed since preparation.
+ * A hint only: unreadable state answers false, which keeps the generic step.
+ */
+async function pendingSwitchEnvOverrideChanged(opts: {
+  readonly projectRoot: string;
+  readonly branch?: string;
+}): Promise<boolean> {
+  try {
+    const store = await openLegacyComposeAdoptedGenerationStore({
+      projectRoot: opts.projectRoot,
+      requestedBranch: opts.branch,
+      mode: "saved",
+    });
+    try {
+      return await store.pendingSwitchEnvOverrideChanged();
+    } finally {
+      await store.close();
+    }
+  } catch {
+    return false;
+  }
+}
 
-/** The store refusal's fixed message, plus the pending-switch step for BUSY. */
+/**
+ * The fixed step a pending switch accepts. Every step names only a command
+ * that state does not refuse for a known reason: plain `--recover` refuses a
+ * `rolling-back` switch, a bound literal override and a bound override that
+ * was moved or changed, so those name `--recover --rollback` alone. With
+ * `busy` false (a refused completion) only those known steps are returned.
+ */
+async function pendingSwitchNextStep(opts: {
+  readonly projectRoot: string;
+  readonly branch?: string;
+  readonly busy: boolean;
+}): Promise<string | null> {
+  const detail = await inspectLegacyComposeAdoptionSelectionDetail({
+    projectRoot: opts.projectRoot,
+  });
+  if (detail?.pending !== "switch") {
+    return null;
+  }
+  if (detail.switchPhase === "rolling-back") {
+    return PENDING_ROLLBACK_BUSY_NEXT_STEP;
+  }
+  if (
+    await legacyAdoptionEnvOverrideHoldsValues({
+      projectRoot: opts.projectRoot,
+    })
+  ) {
+    // A refused completion's own literal-env message already names it.
+    return opts.busy ? PENDING_LITERAL_SWITCH_BUSY_NEXT_STEP : null;
+  }
+  if (await pendingSwitchEnvOverrideChanged(opts)) {
+    return PENDING_CHANGED_SWITCH_BUSY_NEXT_STEP;
+  }
+  return opts.busy ? PENDING_SWITCH_BUSY_NEXT_STEP : null;
+}
+
+/**
+ * The store refusal's fixed message, plus the step a pending switch accepts:
+ * for BUSY, and for a refused plain `--recover` whose cause is known.
+ */
 async function adoptionRefusalMessage(opts: {
   readonly error: LegacyComposeAdoptedGenerationError;
   readonly projectRoot: string;
+  readonly branch?: string;
+  readonly completing: boolean;
 }): Promise<string> {
-  if (opts.error.code !== "E_LEGACY_ADOPTION_BUSY") {
+  const busy = opts.error.code === "E_LEGACY_ADOPTION_BUSY";
+  if (
+    !(busy || opts.completing) ||
+    opts.error.code === "E_LEGACY_ADOPTION_LITERAL_ENV"
+  ) {
     return opts.error.message;
   }
   try {
-    const detail = await inspectLegacyComposeAdoptionSelectionDetail({
-      projectRoot: opts.projectRoot,
-    });
-    if (detail?.pending !== "switch") {
-      return opts.error.message;
-    }
-    const literal = await legacyAdoptionEnvOverrideHoldsValues({
+    const step = await pendingSwitchNextStep({
       projectRoot: opts.projectRoot,
+      branch: opts.branch,
+      busy,
     });
-    return `${opts.error.message} ${literal ? PENDING_LITERAL_SWITCH_BUSY_NEXT_STEP : PENDING_SWITCH_BUSY_NEXT_STEP}`;
+    return step ? `${opts.error.message} ${step}` : opts.error.message;
   } catch {
     // Unreadable state keeps the store's fixed message alone.
     return opts.error.message;
@@ -337,7 +418,16 @@ export const configAdoptCommand = withHandler(spec, async ({ ctx, args }) => {
       code: "E_CONFIG_INVALID",
       message:
         error instanceof LegacyComposeAdoptedGenerationError
-          ? await adoptionRefusalMessage({ error, projectRoot })
+          ? await adoptionRefusalMessage({
+              error,
+              projectRoot,
+              branch: args.options.branch,
+              completing: Boolean(
+                args.options.recover &&
+                  !args.options.rollback &&
+                  !args.options.stop
+              ),
+            })
           : "Legacy adoption refused; original data and recovery evidence retained. Values omitted.",
       ...(diagnostic
         ? { detail: { legacy_adoption_publication_refusal: diagnostic } }