From 033eb8ca28fc322f42128c29c63967e5d9c964a7 Mon Sep 17 00:00:00 2001 From: Rafael Matias Date: Thu, 23 Jul 2026 07:57:24 +0200 Subject: [PATCH] fix(builder): write the eest build sidecar even when the fill fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fill-stateful exits non-zero when any test fails, but it continues through failures and still writes every fixture it did produce. Build returned early on that error, so .benchmarkoor-build.json was never written — a 99/100 fill left fixtures on disk with no fingerprint beside them, and the build markdown summary lost the target's EEST repo, tests, marker and datadir method. Capture the run error, write the sidecar, then return it. That needs a guardrail: when a fill dies before producing anything (filler won't boot, bad args), the output_dir would hold only the sidecar and count as "populated", making the next plain build skip a target that produced nothing. isPopulated now ignores benchmarkoor's own .benchmarkoor-* sidecars, which also closes the same pre-existing hazard from .benchmarkoor-fill.json. The three sidecar filename constants are built from a shared prefix so they can't drift from the check. --- pkg/builder/eest_payloads.go | 20 +++++++++++----- pkg/builder/eest_payloads_test.go | 32 +++++++++++++++++++++++++ pkg/builder/fingerprint.go | 2 +- pkg/builder/util.go | 23 +++++++++++++++--- pkg/builder/util_test.go | 40 +++++++++++++++++++++++++++++++ 5 files changed, 107 insertions(+), 10 deletions(-) diff --git a/pkg/builder/eest_payloads.go b/pkg/builder/eest_payloads.go index d4433aec..4418a479 100644 --- a/pkg/builder/eest_payloads.go +++ b/pkg/builder/eest_payloads.go @@ -255,18 +255,26 @@ func (b *EESTPayloadsBuilder) Build(ctx context.Context, name string, opts Build return false, err } - if err := b.run(ctx, log, target); err != nil { - return false, err - } + runErr := b.run(ctx, log, target) // Record the config fingerprint (computed pre-run) for a later - // --rebuild-on-diff run. Best-effort; a failure must not fail the build. + // --rebuild-on-diff run. Written even when the fill failed: fill-stateful + // continues through individual test failures and still writes every fixture + // it did produce, so a target where 1 of 100 tests failed to fill must still + // be described on disk (the build summary reads this sidecar for the EEST + // repo, tests and marker). An output_dir holding only sidecars still counts + // as unpopulated, so this can't make a later build skip a target that + // produced nothing. Best-effort; a failure here must not mask runErr. if inputsErr != nil { log.WithError(inputsErr).Warn("Failed to compute build fingerprint; sidecar not written") } else if err := writeBuildSidecar(target.OutputDir, EESTPayloadsBuilderName, inputs); err != nil { log.WithError(err).Warn("Failed to write build fingerprint sidecar") } + if runErr != nil { + return false, runErr + } + return false, nil } @@ -828,12 +836,12 @@ func (b *EESTPayloadsBuilder) buildFillImage(ctx context.Context, log logrus.Fie const ( // eestFillResultFile is the sidecar written next to the fixtures recording // how many tests the fill produced/failed (read by the build markdown summary). - eestFillResultFile = ".benchmarkoor-fill.json" + eestFillResultFile = sidecarPrefix + "fill.json" // pytestReportFile is the pytest-json-report output written under output_dir // (via PYTEST_ADDOPTS --json-report); it carries the authoritative // passed/failed tally. Relative to output_dir (= the fill container's /out). - pytestReportFile = ".benchmarkoor-pytest-report.json" + pytestReportFile = sidecarPrefix + "pytest-report.json" ) // dirSize returns the total size in bytes of all regular files under dir. diff --git a/pkg/builder/eest_payloads_test.go b/pkg/builder/eest_payloads_test.go index aa77377b..830d7b23 100644 --- a/pkg/builder/eest_payloads_test.go +++ b/pkg/builder/eest_payloads_test.go @@ -376,6 +376,38 @@ func TestEESTPayloadsBuilder_BuildSkipsPopulatedDir(t *testing.T) { assert.FileExists(t, filepath.Join(dir, eestFillResultFile)) } +// A build that failed before producing any fixture still leaves its sidecars +// behind (the fingerprint is now written even when the fill errors). Those must +// not make the next build skip the target as "already populated". +func TestEESTPayloadsBuilder_BuildDoesNotSkipSidecarOnlyDir(t *testing.T) { + t.Setenv("SCHELK_STATE", filepath.Join(t.TempDir(), "absent.json")) + + dir := t.TempDir() + for _, name := range []string{buildSidecarFile, eestFillResultFile} { + require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte("{}"), 0o600)) + } + + cfg := &config.EESTPayloadsConfig{ + FillImage: "fill:latest", + // A local, non-existent repo path keeps the lazy EEST SHA resolution + // offline: `git ls-remote` fails immediately instead of hitting the network. + EESTRepo: filepath.Join(t.TempDir(), "no-such-repo"), + Targets: []config.EESTPayloadTarget{{ + Name: "compute", FillerClient: "geth", SourceDir: filepath.Join(t.TempDir(), "missing"), + OutputDir: dir, Fork: "Osaka", FillerImage: "geth:master", + Tests: []string{"tests/benchmark/compute"}, + }}, + } + + b := NewEESTPayloadsBuilder(noopLogger(), cfg, "docker", &fakeMgr{}, t.TempDir()) + + // Not skipped: it proceeds into the build and fails on the absent source_dir. + skipped, err := b.Build(context.Background(), "compute", BuildOptions{}) + require.Error(t, err) + assert.False(t, skipped, "a sidecar-only output_dir must not count as built") + assert.Contains(t, err.Error(), "source_dir") +} + func TestMaterializeAddressStubs(t *testing.T) { t.Setenv("TMPDIR", t.TempDir()) diff --git a/pkg/builder/fingerprint.go b/pkg/builder/fingerprint.go index 5721ac9d..a3acfb0e 100644 --- a/pkg/builder/fingerprint.go +++ b/pkg/builder/fingerprint.go @@ -15,7 +15,7 @@ import ( // at an output_dir root after a successful build. When --rebuild-on-diff is set, // the next build compares the current config fingerprint against this file to // decide whether the output is stale and must be rebuilt. -const buildSidecarFile = ".benchmarkoor-build.json" +const buildSidecarFile = sidecarPrefix + "build.json" // buildFingerprintSchema versions the sidecar format so a future change can // invalidate old sidecars deliberately. diff --git a/pkg/builder/util.go b/pkg/builder/util.go index a6a63268..b48b3ec1 100644 --- a/pkg/builder/util.go +++ b/pkg/builder/util.go @@ -8,6 +8,7 @@ import ( "io" "os" "path/filepath" + "strings" "sync" "time" @@ -28,8 +29,18 @@ func mountTempDir() string { return os.TempDir() } -// isPopulated reports whether dir exists and contains at least one -// entry. A missing dir returns (false, nil). +// sidecarPrefix marks the bookkeeping files benchmarkoor itself writes into an +// output_dir (the build fingerprint, the eest fill result, the pytest report). +// They describe a build rather than being its product. +const sidecarPrefix = ".benchmarkoor-" + +// isPopulated reports whether dir exists and holds at least one entry a builder +// actually produced. A missing dir returns (false, nil). +// +// benchmarkoor's own sidecars don't count: they are written even when a build +// fails before producing anything, so a dir holding nothing else must still read +// as empty — otherwise the next build would skip it as "already populated" and +// the target would never be produced. func isPopulated(dir string) (bool, error) { entries, err := os.ReadDir(dir) if err != nil { @@ -40,7 +51,13 @@ func isPopulated(dir string) (bool, error) { return false, fmt.Errorf("reading output_dir %q: %w", dir, err) } - return len(entries) > 0, nil + for _, entry := range entries { + if !strings.HasPrefix(entry.Name(), sidecarPrefix) { + return true, nil + } + } + + return false, nil } // prepareOutputDir ensures dir exists. When force is true the directory is diff --git a/pkg/builder/util_test.go b/pkg/builder/util_test.go index 5f7201d7..ae4942c1 100644 --- a/pkg/builder/util_test.go +++ b/pkg/builder/util_test.go @@ -1,12 +1,52 @@ package builder import ( + "os" + "path/filepath" "sync" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +func TestIsPopulated(t *testing.T) { + t.Run("missing dir is not populated", func(t *testing.T) { + got, err := isPopulated(filepath.Join(t.TempDir(), "absent")) + require.NoError(t, err) + assert.False(t, got) + }) + + t.Run("empty dir is not populated", func(t *testing.T) { + got, err := isPopulated(t.TempDir()) + require.NoError(t, err) + assert.False(t, got) + }) + + // A build that failed before producing anything still writes its sidecars. + // They must not make the dir look built, or the next build would skip it. + t.Run("sidecars alone are not populated", func(t *testing.T) { + dir := t.TempDir() + for _, name := range []string{buildSidecarFile, eestFillResultFile, pytestReportFile} { + require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte("{}"), 0o600)) + } + + got, err := isPopulated(dir) + require.NoError(t, err) + assert.False(t, got) + }) + + t.Run("any produced entry alongside sidecars is populated", func(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, buildSidecarFile), []byte("{}"), 0o600)) + require.NoError(t, os.MkdirAll(filepath.Join(dir, ".meta"), 0o755)) + + got, err := isPopulated(dir) + require.NoError(t, err) + assert.True(t, got) + }) +} + // TestTailBufferConcurrent exercises the case the mutex guards: the container // log-streaming goroutine keeps Write()-ing while the caller reads String() on // the error path (RunInitContainer doesn't join the streaming goroutine). Run