From 764724481345aefa8209caf5a6c374957e9a3b34 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Wed, 24 Jun 2026 18:08:53 -0400 Subject: [PATCH] fix(e2e): make act container start and CLI tar-copy deterministic Two e2e-harness infra flakes hardened at the root. The act runner readiness probe inherited testcontainers' 60s default, too tight for a cold image pull, so a cold runner timed out though the container came up; set an explicit cold-pull-aware startup budget that still fails closed. The CLI tar-copy used CopyFileToContainer, which stats the file for the tar header then streams the bytes separately, a TOCTOU that yielded 'archive/tar: write too long'; cache the built binary bytes once and copy the same immutable slice so header size and payload cannot drift, with a thin fail-closed retry over genuine transient daemon blips. Closes #336, closes #338. Signed-off-by: Joshua Temple --- e2e/harness/act.go | 33 +++++++-- e2e/harness/cli_copy_test.go | 136 +++++++++++++++++++++++++++++++++++ e2e/harness/harness.go | 87 +++++++++++++++++++--- e2e/harness/multi_repo.go | 11 +-- 4 files changed, 250 insertions(+), 17 deletions(-) create mode 100644 e2e/harness/cli_copy_test.go diff --git a/e2e/harness/act.go b/e2e/harness/act.go index b69af485..e635b49e 100644 --- a/e2e/harness/act.go +++ b/e2e/harness/act.go @@ -37,6 +37,19 @@ type RunOpts struct { RepoPath string // Path to cloned repo in container } +// actStartupTimeout bounds how long the container readiness probe waits for the +// act container to become runnable. testcontainers' default is 60s, which is +// ample for the probe itself but offers no slack when a cold first-run image +// pull has already consumed wall-clock and daemon bandwidth. A generous-but- +// finite budget tolerates the one-time cold pull while still failing closed if +// the container never becomes ready. +const actStartupTimeout = 5 * time.Minute + +// actStartupPollInterval is how often the readiness probe re-checks while +// waiting. A brisk interval detects a healthy container promptly once the pull +// completes, without hammering the daemon. +const actStartupPollInterval = 2 * time.Second + // NewActRunner starts a new act container func NewActRunner(ctx context.Context, giteaURL, giteaToken, networkName string, net *testcontainers.DockerNetwork) (*ActRunner, error) { var networks []string @@ -50,10 +63,22 @@ func NewActRunner(ctx context.Context, giteaURL, giteaToken, networkName string, // network alias directly. Job containers are configured separately // in actrc below. req := testcontainers.ContainerRequest{ - Image: "ghcr.io/catthehacker/ubuntu:act-latest", - Cmd: []string{"sleep", "infinity"}, // Keep container running - Networks: networks, - WaitingFor: wait.ForExec([]string{"echo", "ready"}), + Image: "ghcr.io/catthehacker/ubuntu:act-latest", + Cmd: []string{"sleep", "infinity"}, // Keep container running + Networks: networks, + // The readiness exec ("echo ready") only proves the container is up; it + // is near-instant once the container is running. The slow, variable part + // is the FIRST-RUN image pull, which testcontainers performs as part of + // container creation, before this wait strategy's clock starts. The + // strategy's own startup budget defaults to 60s, which is fine for the + // exec itself but leaves no slack when a cold pull has already eaten into + // the daemon's bandwidth. Give the readiness probe an explicit, + // generous budget (still a hard cap: a container that never becomes + // runnable fails after actStartupTimeout) and poll briskly so a healthy + // container is detected promptly. + WaitingFor: wait.ForExec([]string{"echo", "ready"}). + WithStartupTimeout(actStartupTimeout). + WithPollInterval(actStartupPollInterval), HostConfigModifier: func(hc *container.HostConfig) { hc.Mounts = append(hc.Mounts, mount.Mount{ Type: mount.TypeBind, diff --git a/e2e/harness/cli_copy_test.go b/e2e/harness/cli_copy_test.go new file mode 100644 index 00000000..98e81c29 --- /dev/null +++ b/e2e/harness/cli_copy_test.go @@ -0,0 +1,136 @@ +package harness + +import ( + "archive/tar" + "bytes" + "context" + "errors" + "io" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakeCopier records CopyToContainer calls and fails the first failUntil of +// them, letting tests drive copyCLIToContainer's retry path without Docker. +type fakeCopier struct { + failUntil int // number of leading attempts that return err + err error // error returned while failing + calls int // total CopyToContainer invocations + lastBytes []byte + lastPath string +} + +func (f *fakeCopier) CopyToContainer(_ context.Context, content []byte, path string, _ int64) error { + f.calls++ + f.lastBytes = content + f.lastPath = path + if f.calls <= f.failUntil { + return f.err + } + return nil +} + +func TestCopyCLIToContainer_SucceedsFirstTry(t *testing.T) { + t.Parallel() + + fc := &fakeCopier{} + content := []byte("cascade-binary-bytes") + + err := copyCLIToContainer(context.Background(), fc, content, "/usr/local/bin/cascade") + + require.NoError(t, err) + assert.Equal(t, 1, fc.calls, "a clean copy must not retry") + assert.Equal(t, content, fc.lastBytes) + assert.Equal(t, "/usr/local/bin/cascade", fc.lastPath) +} + +func TestCopyCLIToContainer_RetriesTransientThenSucceeds(t *testing.T) { + // Zero the backoff so the retry path runs fast. + orig := cliCopyBackoff + cliCopyBackoff = func(int) time.Duration { return 0 } + t.Cleanup(func() { cliCopyBackoff = orig }) + + fc := &fakeCopier{failUntil: cliCopyMaxAttempts - 1, err: errors.New("docker daemon connection reset")} + + err := copyCLIToContainer(context.Background(), fc, []byte("x"), "/dst") + + require.NoError(t, err, "a copy that succeeds within the budget must not surface an error") + assert.Equal(t, cliCopyMaxAttempts, fc.calls) +} + +func TestCopyCLIToContainer_FailsClosedAfterBudget(t *testing.T) { + orig := cliCopyBackoff + cliCopyBackoff = func(int) time.Duration { return 0 } + t.Cleanup(func() { cliCopyBackoff = orig }) + + sentinel := errors.New("archive/tar: write too long") + fc := &fakeCopier{failUntil: cliCopyMaxAttempts + 5, err: sentinel} + + err := copyCLIToContainer(context.Background(), fc, []byte("x"), "/dst") + + require.Error(t, err, "an exhausted retry budget must surface an error, not swallow it") + assert.ErrorIs(t, err, sentinel, "the underlying copy error must be wrapped, not lost") + assert.Equal(t, cliCopyMaxAttempts, fc.calls, "retries are bounded by cliCopyMaxAttempts") +} + +func TestCopyCLIToContainer_HonorsContextCancellation(t *testing.T) { + // Keep a real backoff so the cancelled context is observed during the wait. + fc := &fakeCopier{failUntil: cliCopyMaxAttempts, err: errors.New("blip")} + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cancelled before the first retry's backoff + + err := copyCLIToContainer(ctx, fc, []byte("x"), "/dst") + + require.Error(t, err) + assert.ErrorIs(t, err, context.Canceled) + assert.Equal(t, 1, fc.calls, "cancellation must stop further attempts") +} + +// TestStableSliceTarSizeMatches proves the invariant the #336 fix relies on: +// when the tar header size and the payload both come from one immutable byte +// slice, archive/tar writes exactly Size bytes with no "write too long" or +// short-write mismatch. This is the deterministic property CopyToContainer +// gives us that CopyFileToContainer (stat-then-stream) does not. +func TestStableSliceTarSizeMatches(t *testing.T) { + t.Parallel() + + content := bytes.Repeat([]byte("cascade"), 4096) // arbitrary stable payload + + var buf bytes.Buffer + tw := tar.NewWriter(&buf) + require.NoError(t, tw.WriteHeader(&tar.Header{ + Name: "usr/local/bin/cascade", + Mode: 0o755, + Size: int64(len(content)), // declared size derives from the SAME slice + })) + n, err := tw.Write(content) + require.NoError(t, err, "writing exactly Size bytes must not error") + require.NoError(t, tw.Close(), "closing after a size-matched write must not error") + assert.Equal(t, len(content), n) + + // Round-trip: read the archive back and confirm the bytes are intact. + tr := tar.NewReader(&buf) + hdr, err := tr.Next() + require.NoError(t, err) + assert.Equal(t, int64(len(content)), hdr.Size) + got, err := io.ReadAll(tr) + require.NoError(t, err) + assert.Equal(t, content, got) +} + +func TestActStartupBudgets(t *testing.T) { + t.Parallel() + + // The readiness probe must carry a cold-pull-aware budget that is still a + // hard cap, and poll briskly. These guard against a silent regression to the + // 60s testcontainers default that let the cold-image start flake (#338). + assert.GreaterOrEqual(t, actStartupTimeout, 3*time.Minute, + "startup budget must tolerate a one-time cold image pull") + assert.Greater(t, actStartupPollInterval, time.Duration(0), + "poll interval must be positive") + assert.Less(t, actStartupPollInterval, actStartupTimeout, + "poll interval must be far smaller than the overall budget") +} diff --git a/e2e/harness/harness.go b/e2e/harness/harness.go index f86b1e76..977a68dc 100644 --- a/e2e/harness/harness.go +++ b/e2e/harness/harness.go @@ -27,10 +27,20 @@ import ( // and downstream orchestrate steps to fail with "No such file or directory". // Build once per process under sync.Once and reuse a stable, package-private // path; all scenarios share the same source so the binary is identical. +// +// We also cache the built binary's BYTES, not just its path. testcontainers' +// CopyFileToContainer re-Stats the file and then streams it, declaring the +// stat'd size in the tar header before writing the bytes. If anything mutates +// the file in the gap between the stat and the stream, the bytes written no +// longer match the declared header size and archive/tar fails the copy with +// "write too long" (issue #336). Copying from a stable in-memory slice via +// CopyToContainer makes the declared size and the written bytes derive from the +// same immutable buffer, so that size-mismatch window cannot exist. var ( - cliBinaryOnce sync.Once - cliBinaryPath string - cliBinaryErr error + cliBinaryOnce sync.Once + cliBinaryPath string + cliBinaryBytes []byte + cliBinaryErr error ) // normalizeCallbackStubPath returns the canonical path where a callback stub @@ -508,14 +518,14 @@ func (h *Harness) GenerateWorkflows(ctx context.Context) error { } // Build CLI once for the whole test process and copy into this scenario's - // act container. - binaryPath, err := h.ensureCLIBinary(ctx) - if err != nil { + // act container. The copy streams the cached binary bytes (not the on-disk + // file) to avoid the archive/tar size-mismatch race (#336). + if _, err := h.ensureCLIBinary(ctx); err != nil { return err } - if err := h.act.Container().CopyFileToContainer(ctx, binaryPath, "/usr/local/bin/cascade", 0755); err != nil { - return fmt.Errorf("failed to copy CLI to container: %w", err) + if err := copyCLIToContainer(ctx, h.act.Container(), cliBinaryBytes, "/usr/local/bin/cascade"); err != nil { + return err } // Also copy the CLI into the repo so job containers can access it @@ -833,11 +843,72 @@ func (h *Harness) ensureCLIBinary(ctx context.Context) (string, error) { cliBinaryErr = fmt.Errorf("failed to build CLI: %w\nOutput: %s", err, output) return } + + // Read the freshly built binary into a stable in-memory slice once, so + // every container copy streams from an immutable buffer rather than + // re-reading (and re-stat'ing) the on-disk file. This is what closes the + // archive/tar size-mismatch race (#336): the tar header size and the + // payload bytes both come from this single slice. + data, err := os.ReadFile(path) + if err != nil { + _ = os.Remove(path) + cliBinaryErr = fmt.Errorf("failed to read built CLI: %w", err) + return + } cliBinaryPath = path + cliBinaryBytes = data }) return cliBinaryPath, cliBinaryErr } +// cliCopyMaxAttempts bounds how many times copyCLIToContainer retries the tar +// copy. The copy itself is deterministic now that it streams from a stable +// in-memory slice, so a retry only absorbs a genuine, transient Docker daemon +// API blip (a dropped connection mid-copy, a momentary daemon stall). A small +// fixed budget clears that; a copy that keeps failing surfaces the real error. +const cliCopyMaxAttempts = 3 + +// cliCopyBackoff returns the pause before the retry that follows a failed copy +// attempt (attempt is 1-based). It is a var so tests can zero the wait. The +// growth (250ms, 500ms) lets a momentary daemon stall clear without stalling +// the suite. +var cliCopyBackoff = func(attempt int) time.Duration { + return time.Duration(attempt) * 250 * time.Millisecond +} + +// containerCopier is the slice of the container surface copyCLIToContainer +// needs: copying a byte slice to an in-container path. The real +// testcontainers.Container satisfies it, and a fake satisfies it in tests, so +// the copy/retry logic is exercisable without Docker. +type containerCopier interface { + CopyToContainer(ctx context.Context, fileContent []byte, containerFilePath string, fileMode int64) error +} + +// copyCLIToContainer copies the cached CLI binary bytes into the container at +// destPath, retrying a transient Docker daemon copy blip up to +// cliCopyMaxAttempts. It streams from the stable cliBinaryBytes slice via +// CopyToContainer, so the archive/tar header size always matches the payload +// (the #336 root cause). The bounded retry only covers a genuine transport +// blip and fails closed, surfacing the last error once the budget is spent. +func copyCLIToContainer(ctx context.Context, dst containerCopier, content []byte, destPath string) error { + var lastErr error + for attempt := 1; attempt <= cliCopyMaxAttempts; attempt++ { + if attempt > 1 { + select { + case <-ctx.Done(): + return fmt.Errorf("CLI copy cancelled after %d attempt(s): %w", attempt-1, ctx.Err()) + case <-time.After(cliCopyBackoff(attempt - 1)): + } + } + err := dst.CopyToContainer(ctx, content, destPath, 0o755) + if err == nil { + return nil + } + lastErr = err + } + return fmt.Errorf("failed to copy CLI to container after %d attempt(s): %w", cliCopyMaxAttempts, lastErr) +} + // localizeWorkflows rewrites generated workflows so external action refs // (`stablekernel/cascade/.github/actions/X@ref`) point at the local // mock actions. A second pass adds the `./` prefix act needs for any bare diff --git a/e2e/harness/multi_repo.go b/e2e/harness/multi_repo.go index e685866b..9c3d9247 100644 --- a/e2e/harness/multi_repo.go +++ b/e2e/harness/multi_repo.go @@ -272,13 +272,14 @@ func (h *MultiRepoHarness) prepareRepoInActContainer(ctx context.Context, repoCt } // Copy the (already-built) CLI binary into the repo so the mock setup-cli - // action can install it onto PATH inside job containers. - binaryPath, err := h.base.ensureCLIBinary(ctx) - if err != nil { + // action can install it onto PATH inside job containers. The copy streams + // the cached binary bytes (not the on-disk file) to avoid the archive/tar + // size-mismatch race (#336). + if _, err := h.base.ensureCLIBinary(ctx); err != nil { return err } - if err := h.act.Container().CopyFileToContainer(ctx, binaryPath, "/usr/local/bin/cascade", 0755); err != nil { - return fmt.Errorf("failed to copy CLI to container: %w", err) + if err := copyCLIToContainer(ctx, h.act.Container(), cliBinaryBytes, "/usr/local/bin/cascade"); err != nil { + return err } copyToRepoCmd := []string{ "bash", "-c",