Skip to main content

max / alloy

run: keep a resolver's capture out of the scrollback buffer A Stage::Resolve hands its command's stdout to a closure that builds what runs next. The capture was an offset into the collected output lines, which is meaningful only while nothing is ever removed from the front of them. push_line removes from the front the moment SCROLLBACK is reached. So once 500 lines had gone by, a stage recorded an offset that no longer pointed at its own first line. Past the limit it pointed beyond the end and the resolver was handed the empty string; partway there it was handed the tail of an earlier command spliced onto the head of its own. Neither fails loudly. `root_partition("")` reports "lsblk emitted invalid JSON: EOF while parsing a value", which arrives after `bootc install to-disk --wipe` has already partitioned and deployed, and reads exactly like a disk that genuinely was not deployed. The spliced case is worse: deployment_dir trims a garbage path and the whole configure half is built against it. This is the ordinary case on a real install, not a pathological one. `bootc install to-disk` streams a line per layer and runs first, ahead of every discovery stage. Six test installs stayed under 500 lines, which is a margin rather than a guarantee, and it shrinks as the image gains layers. Each running command now keeps its own lines instead of an index. Only a Resolve fills that buffer, so the long chatty commands, which are all Run, still cost nothing beyond the bounded display buffer; the commands that do capture are lsblk and findmnt, whose output is kilobytes. The regression test asserts the display buffer is still held to SCROLLBACK, so the bound this bug came from is not quietly removed to fix it. Verified to fail against the old code with the capture reading "".
Co-Authored-By
Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-07-22 21:19 UTC
Signed with PGP, not checked
Commit: 4bffd6f26ae0c4c5a959a265722104afbf6ca063
Parent: 1b676e7
1 file changed, +81 insertions, -11 deletions
M crates/alloy/src/run.rs +81 -11
@@ -77,9 +77,25 @@
77 77 /// One running child and the lines it has produced.
78 78 struct Running {
79 79 child: Child,
80 - /// Where this command's own output starts in the collected lines, so a
81 - /// resolver sees only what its command printed.
82 - first_line: usize,
80 + /// This command's own output, kept apart from the display buffer.
81 + ///
82 + /// A resolver has to see exactly what its command printed and nothing
83 + /// else. This used to be an index into the collected lines, which is
84 + /// correct only while nothing is ever removed from the front of them, and
85 + /// [`push_line`] removes from the front as soon as [`SCROLLBACK`] is
86 + /// reached. A stage starting after that point recorded an index that no
87 + /// longer pointed at its own first line, and past 500 lines it pointed
88 + /// beyond the end, so the capture was the empty string. `bootc install
89 + /// to-disk` streams a line per layer and runs first, which puts the
90 + /// installer's discovery stages on exactly that side of the limit.
91 + ///
92 + /// Keeping the lines rather than an offset makes the capture independent
93 + /// of anything the display buffer drops. Only a [`Stage::Resolve`] fills
94 + /// this; a plain [`Stage::Run`] has nothing to read it, so its output goes
95 + /// to the display buffer alone and this stays empty. That matters because
96 + /// the long, chatty commands are all `Run`, and the ones that resolve are
97 + /// short introspection commands whose output is measured in kilobytes.
98 + captured: Vec<String>,
83 99 /// What to do with this command's output, for a [`Stage::Resolve`].
84 100 then: Option<Resolver>,
85 101 /// Lines from stdout and stderr, interleaved in arrival order.
@@ -92,10 +108,10 @@
92 108
93 109 impl Running {
94 110 /// Drain whatever has arrived, without waiting for more.
95 - fn drain(&self, into: &mut Vec<String>) {
111 + fn drain(&mut self, into: &mut Vec<String>) {
96 112 loop {
97 113 match self.lines.try_recv() {
98 - Ok(line) => push_line(into, line),
114 + Ok(line) => self.take(line, into),
99 115 // Disconnected means both readers are done, which is a fact
100 116 // about the pipes rather than about the process: the child may
101 117 // still be exiting. `finished` is what decides that.
@@ -104,6 +120,20 @@
104 120 }
105 121 }
106 122
123 + /// Record one line: into the capture if a resolver will read it, and into
124 + /// the display buffer either way.
125 + fn take(&mut self, line: String, into: &mut Vec<String>) {
126 + if self.then.is_some() {
127 + self.captured.push(line.clone());
128 + }
129 + push_line(into, line);
130 + }
131 +
132 + /// This command's own output, as a resolver wants it.
133 + fn captured(&self) -> String {
134 + self.captured.join("\n")
135 + }
136 +
107 137 /// Drain everything the command will ever produce, blocking until the
108 138 /// readers are done.
109 139 ///
@@ -117,9 +147,9 @@
117 147 /// output has been forwarded: both pump threads reaching EOF and dropping
118 148 /// their senders. The wait is bounded because the child is already dead,
119 149 /// so both pipes are closed and EOF is already on its way.
120 - fn drain_to_end(&self, into: &mut Vec<String>) {
150 + fn drain_to_end(&mut self, into: &mut Vec<String>) {
121 151 while let Ok(line) = self.lines.recv() {
122 - push_line(into, line);
152 + self.take(line, into);
123 153 }
124 154 }
125 155
@@ -237,7 +267,7 @@
237 267 }
238 268
239 269 if let Some(resolve) = finished.then.take() {
240 - let captured = self.output[finished.first_line..].join("\n");
270 + let captured = finished.captured();
241 271 match resolve(&captured) {
242 272 // Pushed to the front: what a discovery decides
243 273 // runs before whatever was queued behind it.
@@ -265,10 +295,10 @@
265 295 return;
266 296 };
267 297
268 - let first_line = self.output.len();
269 298 match spawn(stage.invocation(), log) {
270 299 Ok(mut running) => {
271 - running.first_line = first_line;
300 + // Set before the first drain, because whether a line is
301 + // captured depends on it.
272 302 running.then = match stage {
273 303 Stage::Run(_) => None,
274 304 Stage::Resolve { then, .. } => Some(then),
@@ -306,7 +336,7 @@
306 336
307 337 Ok(Running {
308 338 child,
309 - first_line: 0,
339 + captured: Vec::new(),
310 340 then: None,
311 341 lines: receiver,
312 342 })
@@ -486,6 +516,46 @@
486 516 assert_eq!(sequence.outcome(), Some(&Ok(())));
487 517 }
488 518
519 + // The same rule, once the display buffer has started dropping lines.
520 + //
521 + // The capture used to be an offset into the collected output, which is
522 + // only meaningful while nothing is removed from the front of it. Past
523 + // SCROLLBACK lines the offset pointed beyond the end and the resolver was
524 + // handed an empty string; partway there it was handed the tail of an
525 + // earlier command spliced onto the head of its own. Neither failed
526 + // loudly: the installer reported "lsblk emitted invalid JSON: EOF while
527 + // parsing a value" after the disk had been wiped and deployed, which is
528 + // indistinguishable from a disk that genuinely was not deployed.
529 + //
530 + // `bootc install to-disk` streams a line per layer and runs before every
531 + // discovery stage, so this is the ordinary case on a real install rather
532 + // than a pathological one.
533 + #[test]
534 + fn a_resolver_sees_its_own_output_after_the_scrollback_has_wrapped() {
535 + let mut log = CommandLog::new();
536 + let noisy = format!("seq {}", SCROLLBACK * 2);
537 + let mut sequence = Sequence::new(vec![
538 + Stage::Run(Invocation::new("sh").args(["-c", &noisy])),
539 + Stage::Resolve {
540 + invocation: Invocation::new("echo").arg("mine"),
541 + then: Box::new(|captured| {
542 + assert_eq!(
543 + captured.trim(),
544 + "mine",
545 + "the capture did not survive the scrollback wrapping"
546 + );
547 + Ok(Vec::new())
548 + }),
549 + },
550 + ]);
551 +
552 + drive(&mut sequence, &mut log);
553 +
554 + assert_eq!(sequence.outcome(), Some(&Ok(())));
555 + // The display buffer is still bounded; only the capture is exempt.
556 + assert_eq!(sequence.output().len(), SCROLLBACK);
557 + }
558 +
489 559 // A command handed a secret still shows its diagnostics. `chpasswd` writing
490 560 // "Module is unknown" to stderr is the only way a failing password step
491 561 // says anything at all, and withholding it because the command touched a