chadtmiller.com/knotless

Switch repository

Loading…

What changes

A container or overlay that fails to start now leaves a log. The runner adds the error to the end of the run's log and uploads the log before it files the check as error. The check's message says "see the log", and now there is one to see. If saving the log fails, the start's own error is still the one the check gives.

Docker's own words reach the log. A container is started with Command::output(), and that pipes stderr even when the command was given the log to write to. So what docker said as a container failed to start never reached the log. Now it is written to the log after docker run returns.

Why

The start returned early, so the check was filed error with no log, though its message pointed at one. With the log in place, the missing docker output showed up as the second half of the same problem.

Fixes knotless-run: a container or overlay that fails to start says "see the log", but no log is uploaded

How it was checked

A new test in crates/knotless-run/tests/run.rs runs the docker executor against a fake docker on the path. The fake prints a line to stderr and exits 1. The test reads the filed check, which has status error and a log holding docker's line and "see the log". CI runs it.

Changeset: none

Shortcomings

An error from running the steps after a good start still returns without this note. Nothing reaches it in a test yet. The test puts its fake docker at the front of the whole process's PATH. No other test in run.rs uses docker, so they are not affected.

🤖 Generated with Claude Code

When the executor's start fails, the runner now appends the error to the run's log and uploads it before returning. The check is then filed as error with a log that holds docker's output and "see the log". Every bail! in start already says "see the log", so the new test's assertion holds.

I checked the early return against live::send. It treats a dropped ended sender as the run being over, so the live sender task ends cleanly. The docs page is updated, and Changeset: none is right because only knotless-run changes.

Two notes, neither blocking:

• If reading or uploading the log fails in the new branch, ? returns that error and hides the original start error. Probably acceptable, but you could keep the start error as the one returned.
• The test calls std::env::set_var("PATH", …) process-wide while the other tests in run.rs run in parallel threads. It only prepends a directory, so I expect it to be harmless unless another test starts docker. A fake docker placed in the sandbox's own environment would remove that risk, if the harness allows it.

A failed start now appends its error to the run's log and uploads the log before the check is filed as error. The new test and the docs line cover that. My earlier review asked for nothing blocking, and I see nothing new to block on.

Two notes from last round still apply, and neither blocks the merge:

• If reading or uploading the log fails in the new branch, ? hides the original start error.
• The test sets PATH for the whole process while the other tests in run.rs run in parallel.

This round adds two commits to the test and a merge of main. The test now runs one reconcile so the watch learns the refs, pushes the workflow, then reconciles again. The runner change and the docs line are the same as in my earlier rounds. I found nothing new to block on. The check and test workflows were still running when I looked, so confirm they pass before it lands.

Two notes from earlier rounds still apply, and neither blocks the merge:

• If reading or uploading the log in the new error branch fails, ? hides the original start error.
• The test sets PATH for the whole process while the other tests in run.rs run in parallel.

The latest commit breaks the build. The check workflow fails at clippy with error[E0716]: temporary value dropped while borrowed at crates/knotless-run/src/lib.rs:2986. upload_log returns a future that borrows &check::redact(...), and the temporary is freed before kept.await runs. Because knotless-run doesn't compile, the test workflow can't pass either. The -> &err fix to keep the start error was the point of this round, and it's sound, but it needs to compile.

Bind the redacted bytes to a let first, or await the upload in the same expression. Then rerun check and test, including the new docker test. The runner change, the docs line and the Command::output() stderr fix look right otherwise. The PATH note from earlier rounds still stands and doesn't block.

The E0716 from last round is fixed. The redacted bytes now sit in let text, and the upload is awaited after that. Because the Err arm only returns err, a failed log save can no longer hide the start error. log is mut and Write is imported, so the write_all call compiles as far as I can read it. The runner change, the Command::output() stderr fix, the docs line and the test are the same as in earlier rounds.

I couldn't confirm the build from the checks. check and test were still running on this commit when I looked, so make sure both pass, including the new docker test, before you merge. The note about the test setting PATH for the whole process still stands and doesn't block the merge.