Skip to content

Stop the landing test racing the shell's exit #44

Description

@sehkone

Summary

executor::tests::conformance::landing::a_directory_at_the_destination_fails_the_shell_write fails intermittently on CI, and the failure is in the test harness rather than in anything it is testing:

thread '...a_directory_at_the_destination_fails_the_shell_write' panicked at src/executor.rs:2995:22:
write contents: Os { code: 32, kind: BrokenPipe, message: "Broken pipe" }

Seen on run 31291172351 (371 passed, 1 failed); the same commit's code passed the same job on earlier runs, which is what a race looks like.

PUT_FILE_SCRIPT refuses a directory at the destination in its first four lines:

if [ -d "$dest" ]; then
  echo "destination $dest is a directory" >&2
  exit 1
fi

That is before cat > "$tmp", so on this path the shell never reads its stdin and exits with the read end closed. Meanwhile run_landing_script writes the contents and .expect("write contents")s the result. Two outcomes race: the eight bytes reach the pipe buffer before the shell is gone, and the write succeeds; or the shell exits first, and the write gets EPIPE. A loaded runner makes the second more likely, and nothing about the behaviour under test decides which happens.

The refusal being tested is the exit status and the stderr the script produces. Whether the parent managed to hand over bytes nobody was going to read is not part of it.

Scope

In run_landing_script, accept ErrorKind::BrokenPipe from the write_all as one of the script's legitimate outcomes and let the existing assertions on Output deliver the verdict; keep panicking on every other write error. The pipe must still be closed explicitly before wait_with_output, or cat on the paths that do read stdin never sees EOF.

PUT_FILE_SCRIPT itself does not change: refusing before staging anything is the correct order, and reading stdin to be polite about a write that is being refused would be the wrong repair.

Non-goals

Every other test in mod landing. Only the helper they share is at fault, and only on the paths that refuse early.

Acceptance criteria

  • run_landing_script treats a BrokenPipe write as a non-failure and panics on any other write error.
  • The test still asserts the refusal it was written for: a non-zero exit, the directory message on stderr, and nothing staged under the destination.
  • cargo fmt -- --check --config group_imports=StdExternalCrate, cargo clippy --all-targets -- -D warnings, cargo clippy --all-targets --features test-support -- -D warnings, cargo test and cargo test --features test-support all pass.

Note

Found by #41 (issue #37), whose branch touches no part of src/executor.rs. It lands separately for the same reason #42 did: that pull request's diff is fixed at the four files its issue names.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions