From a9e18a6ebe7065dec4e157f6d0571738dd43035c Mon Sep 17 00:00:00 2001 From: Rafael Matias Date: Tue, 11 Aug 2026 16:14:28 +0200 Subject: [PATCH] fix(runner): verify the datadir head for a bundle inside the fixtures artifact MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit verifyPreRunBundleHead guards the replay's by-number skip: a datadir at the bundle's height but on a different chain would otherwise skip every line and benchmark the wrong state. It only ran when pre_runs named a local_fixtures_dir, so since #296 — which let a bundle be resolved out of the extracted fixtures artifact instead — it silently no-opped for exactly the configs that rely on that path. Ask the source where its bundle is, via a new optional PreRunBundleLocator, rather than re-deriving the path from config: the bundle may sit at a local directory or inside the artifact, and only the source knows which. EESTSource now resolves it once, for both the replay and this check. The config-derived path stays as a fallback for callers without a live source. ReadPreRunBundleInfoAt reads a bundle directory named outright, which is what a runner-side pre_runs source has; ReadPreRunBundleInfo keeps its parent + PreRunBundleSubdir form. An unlocatable bundle now warns instead of passing quietly, since the skip it guards runs either way. --- pkg/builder/prerun_bundle.go | 15 ++++- pkg/builder/prerun_bundle_info_test.go | 35 ++++++++++++ pkg/executor/eest_source.go | 36 +++++++++--- pkg/executor/eest_source_test.go | 53 +++++++++++++++++ pkg/executor/source.go | 12 ++++ pkg/runner/lifecycle.go | 47 ++++++++++++++- pkg/runner/prerun_head_test.go | 79 ++++++++++++++++++++++++++ 7 files changed, 266 insertions(+), 11 deletions(-) diff --git a/pkg/builder/prerun_bundle.go b/pkg/builder/prerun_bundle.go index cebae27a..a881e457 100644 --- a/pkg/builder/prerun_bundle.go +++ b/pkg/builder/prerun_bundle.go @@ -375,7 +375,20 @@ func ReadPreRunBundleInfo(bundleParentDir string) (*PreRunBundleInfo, error) { return nil, nil } - dir := filepath.Join(bundleParentDir, config.PreRunBundleSubdir) + return ReadPreRunBundleInfoAt(filepath.Join(bundleParentDir, config.PreRunBundleSubdir)) +} + +// ReadPreRunBundleInfoAt is ReadPreRunBundleInfo for a caller that already holds +// the bundle directory itself rather than its parent. A runner-side pre_runs +// source names that directory outright (it may sit anywhere inside an extracted +// fixtures artifact), so it cannot go through the parent + PreRunBundleSubdir +// form above. +func ReadPreRunBundleInfoAt(bundleDir string) (*PreRunBundleInfo, error) { + if bundleDir == "" { + return nil, nil + } + + dir := bundleDir path := filepath.Join(dir, preRunBundleMetaFile) data, err := os.ReadFile(path) diff --git a/pkg/builder/prerun_bundle_info_test.go b/pkg/builder/prerun_bundle_info_test.go index e4d3aa46..6de0a8a9 100644 --- a/pkg/builder/prerun_bundle_info_test.go +++ b/pkg/builder/prerun_bundle_info_test.go @@ -178,3 +178,38 @@ func TestReadPreRunBundleInfoNoBundleAtAll(t *testing.T) { require.NoError(t, err) assert.Nil(t, info) } + +func TestReadPreRunBundleInfoAt(t *testing.T) { + // A runner-side pre_runs source names the bundle directory outright — it + // can sit anywhere inside an extracted fixtures artifact — so it cannot go + // through the parent + PreRunBundleSubdir form. + parent := t.TempDir() + writeBundleFile(t, parent, [][3]string{ + {"0x17466ef", "0xaaa", "0x111"}, + {"0x17466f0", "0xbbb", "0xaaa"}, + }, 4096) + + bundleDir := filepath.Join(parent, config.PreRunBundleSubdir) + + direct, err := ReadPreRunBundleInfoAt(bundleDir) + require.NoError(t, err) + require.NotNil(t, direct) + + // Same bundle reached the old way: the two must agree. + viaParent, err := ReadPreRunBundleInfo(parent) + require.NoError(t, err) + require.NotNil(t, viaParent) + assert.Equal(t, viaParent, direct) + + assert.Equal(t, uint64(0x17466f0), direct.EndBlockNumber) + assert.Equal(t, "0xbbb", direct.EndBlockHash) + + // Empty and absent stay non-errors, as for ReadPreRunBundleInfo. + got, err := ReadPreRunBundleInfoAt("") + require.NoError(t, err) + assert.Nil(t, got) + + got, err = ReadPreRunBundleInfoAt(filepath.Join(t.TempDir(), "nope")) + require.NoError(t, err) + assert.Nil(t, got) +} diff --git a/pkg/executor/eest_source.go b/pkg/executor/eest_source.go index e5cedffb..76d67567 100644 --- a/pkg/executor/eest_source.go +++ b/pkg/executor/eest_source.go @@ -1001,18 +1001,22 @@ func (s *EESTSource) discoverTests() (*PreparedSource, error) { return result, nil } -// loadPreRunBundleSteps returns the configured builder.pre_runs bundle as -// pre-run steps (the runner replays them before the fixtures). Returns nil when -// no pre_runs source is configured. +// PreRunBundleDir resolves the directory holding this source's builder.pre_runs +// bundle, or "" when there is none to resolve. // // The bundle is read from LocalFixturesDir when set, and otherwise from the // already-extracted fixtures artifact — resolved against the same root // FixturesSubdir resolves the fixtures against. A build ships the bundle and // the fixtures in one tarball, so a release consumer can reach both from a // single fixtures_url instead of staging the bundle on every runner host. -func (s *EESTSource) loadPreRunBundleSteps() ([]*StepFile, error) { - if s.cfg.PreRuns == nil { - return nil, nil +// +// This is the one place that resolution happens: the runner reads the bundle's +// metadata to check the datadir is on the bundle's chain, and a second copy of +// this logic derived from config alone would miss the artifact case and skip +// that check. +func (s *EESTSource) PreRunBundleDir() string { + if s.cfg == nil || s.cfg.PreRuns == nil { + return "" } base := s.cfg.PreRuns.LocalFixturesDir @@ -1023,7 +1027,7 @@ func (s *EESTSource) loadPreRunBundleSteps() ([]*StepFile, error) { // Nothing to resolve against: no local directory configured and no // fixtures extracted for this source. if base == "" { - return nil, nil + return "" } subdir := s.cfg.PreRuns.FixturesSubdir @@ -1031,7 +1035,23 @@ func (s *EESTSource) loadPreRunBundleSteps() ([]*StepFile, error) { subdir = config.PreRunBundleSubdir } - bundleDir := filepath.Join(base, subdir) + return filepath.Join(base, subdir) +} + +// loadPreRunBundleSteps returns the configured builder.pre_runs bundle as +// pre-run steps (the runner replays them before the fixtures). Returns nil when +// no pre_runs source is configured. +// +// The bundle is read from LocalFixturesDir when set, and otherwise from the +// already-extracted fixtures artifact — resolved against the same root +// FixturesSubdir resolves the fixtures against. A build ships the bundle and +// the fixtures in one tarball, so a release consumer can reach both from a +// single fixtures_url instead of staging the bundle on every runner host. +func (s *EESTSource) loadPreRunBundleSteps() ([]*StepFile, error) { + bundleDir := s.PreRunBundleDir() + if bundleDir == "" { + return nil, nil + } entries, err := filepath.Glob(filepath.Join(bundleDir, "*.request")) if err != nil { diff --git a/pkg/executor/eest_source_test.go b/pkg/executor/eest_source_test.go index bf9c47c4..5ed31994 100644 --- a/pkg/executor/eest_source_test.go +++ b/pkg/executor/eest_source_test.go @@ -183,3 +183,56 @@ func TestLoadPreRunBundleSteps(t *testing.T) { assert.Contains(t, err.Error(), "no pre-run bundle") }) } + +func TestPreRunBundleDir(t *testing.T) { + tests := []struct { + name string + preRuns *config.EESTPreRunsSource + fixturesDir string + want func(fixtures string) string + }{ + { + name: "no pre_runs source", + preRuns: nil, + want: func(string) string { return "" }, + }, + { + name: "nothing to resolve against", + preRuns: &config.EESTPreRunsSource{}, + want: func(string) string { return "" }, + }, + { + name: "local dir wins over the extracted artifact", + preRuns: &config.EESTPreRunsSource{LocalFixturesDir: "/local"}, + fixturesDir: "/artifact", + want: func(string) string { return filepath.Join("/local", config.PreRunBundleSubdir) }, + }, + { + name: "falls back to the extracted artifact", + preRuns: &config.EESTPreRunsSource{FixturesSubdir: "a/b/pre_run_bundle"}, + fixturesDir: "/artifact", + want: func(string) string { return filepath.Join("/artifact", "a/b/pre_run_bundle") }, + }, + { + name: "subdir defaults under the artifact", + preRuns: &config.EESTPreRunsSource{}, + fixturesDir: "/artifact", + want: func(string) string { return filepath.Join("/artifact", config.PreRunBundleSubdir) }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := &EESTSource{ + log: logrus.New(), + fixturesDir: tt.fixturesDir, + cfg: &config.EESTFixturesSource{PreRuns: tt.preRuns}, + } + assert.Equal(t, tt.want(tt.fixturesDir), s.PreRunBundleDir()) + }) + } + + // The locator is what lets a caller outside this package find the bundle + // without re-deriving it from config. + var _ PreRunBundleLocator = (*EESTSource)(nil) +} diff --git a/pkg/executor/source.go b/pkg/executor/source.go index 6eb8d8e7..56051917 100644 --- a/pkg/executor/source.go +++ b/pkg/executor/source.go @@ -73,6 +73,18 @@ type Source interface { GetSourceInfo() (*SuiteSource, error) } +// PreRunBundleLocator is an optional interface for sources that resolve a +// builder.pre_runs bundle. It exists so a caller can read the bundle's metadata +// without re-deriving the path: the bundle may sit at a configured local +// directory OR inside the fixtures artifact the source extracted, and only the +// source knows which. Re-deriving it from config alone silently misses the +// artifact case. +type PreRunBundleLocator interface { + // PreRunBundleDir returns the directory holding the pre-run bundle, or an + // empty string when this source provides none. + PreRunBundleDir() string +} + // GenesisProvider is an optional interface that sources can implement // to provide genesis files for clients. type GenesisProvider interface { diff --git a/pkg/runner/lifecycle.go b/pkg/runner/lifecycle.go index bdeabcb5..2ece01e0 100644 --- a/pkg/runner/lifecycle.go +++ b/pkg/runner/lifecycle.go @@ -1649,6 +1649,38 @@ func copyStateActorFiles( // Anything in between is a partially applied bundle, which the replay resumes // from. Only a hash mismatch at a block the bundle actually names is an error — // that is the case that cannot be anything but the wrong chain. +// preRunBundleDir locates the pre-run bundle for head verification. +// +// The source is asked first: the bundle may sit at a configured +// local_fixtures_dir OR inside the fixtures artifact the source extracted, and +// only the source knows which. Deriving the path from config alone resolves the +// local case and silently misses the artifact one, which leaves the by-number +// replay skip unguarded — a datadir at the right height on a different chain +// then replays as a no-op and benchmarks the wrong state. +// +// The config-derived path remains as a fallback for callers without a live +// source. +func (r *runner) preRunBundleDir(preRuns *config.EESTPreRunsSource) string { + if r.executor != nil { + if loc, ok := r.executor.GetSource().(executor.PreRunBundleLocator); ok { + if dir := loc.PreRunBundleDir(); dir != "" { + return dir + } + } + } + + if preRuns.LocalFixturesDir == "" { + return "" + } + + subdir := preRuns.FixturesSubdir + if subdir == "" { + subdir = config.PreRunBundleSubdir + } + + return filepath.Join(preRuns.LocalFixturesDir, subdir) +} + func (r *runner) verifyPreRunBundleHead( log logrus.FieldLogger, headNumber uint64, headHash string, ) (bool, error) { @@ -1657,11 +1689,22 @@ func (r *runner) verifyPreRunBundleHead( } src := r.cfg.FullConfig.Runner.Benchmark.Tests.Source.EESTFixtures - if src == nil || src.PreRuns == nil || src.PreRuns.LocalFixturesDir == "" { + if src == nil || src.PreRuns == nil { + return false, nil + } + + bundleDir := r.preRunBundleDir(src.PreRuns) + if bundleDir == "" { + log.Warn( + "A pre_runs source is configured but its bundle could not be located; " + + "skipping head verification — the replay skips by block number alone, " + + "so a datadir at the right height on a different chain will not be caught", + ) + return false, nil } - info, err := builder.ReadPreRunBundleInfo(src.PreRuns.LocalFixturesDir) + info, err := builder.ReadPreRunBundleInfoAt(bundleDir) if err != nil { // Metadata is a convenience; an unreadable sidecar must not block a run // that would otherwise replay correctly. diff --git a/pkg/runner/prerun_head_test.go b/pkg/runner/prerun_head_test.go index 2638ea65..d4a16cec 100644 --- a/pkg/runner/prerun_head_test.go +++ b/pkg/runner/prerun_head_test.go @@ -11,6 +11,7 @@ import ( "github.com/stretchr/testify/require" "github.com/ethpandaops/benchmarkoor/pkg/config" + "github.com/ethpandaops/benchmarkoor/pkg/executor" ) // bundleAt writes a pre-run bundle sidecar describing blocks 100..181. @@ -102,3 +103,81 @@ func TestVerifyPreRunBundleHead(t *testing.T) { assert.False(t, applied) }) } + +// stubLocator is a source that reports where its bundle is, as an EEST source +// resolving one out of an extracted fixtures artifact does. +type stubLocator struct { + executor.Source + dir string +} + +func (s stubLocator) PreRunBundleDir() string { return s.dir } + +// stubExecutor hands back a source; nothing else on the interface is exercised. +type stubExecutor struct { + executor.Executor + src executor.Source +} + +func (e stubExecutor) GetSource() executor.Source { return e.src } + +// The regression behind #305: with the bundle inside the fixtures artifact, +// pre_runs carries no local_fixtures_dir, so a config-derived lookup finds +// nothing and the head check silently no-ops — leaving the by-number replay +// skip free to benchmark a datadir that is on a different chain. +func TestVerifyPreRunBundleHeadFromFixturesArtifact(t *testing.T) { + log := logrus.New() + log.SetOutput(os.NewFile(0, os.DevNull)) + + dir := bundleAt(t) + bundleDir := filepath.Join(dir, config.PreRunBundleSubdir) + + // pre_runs configured WITHOUT local_fixtures_dir, as an artifact-resolved + // source is. + r := &runner{ + cfg: &Config{FullConfig: &config.Config{ + Runner: config.RunnerConfig{ + Benchmark: config.BenchmarkConfig{ + Tests: config.TestsConfig{ + Source: config.SourceConfig{ + EESTFixtures: &config.EESTFixturesSource{ + PreRuns: &config.EESTPreRunsSource{ + FixturesSubdir: "does/not/matter", + }, + }, + }, + }, + }, + }, + }}, + executor: stubExecutor{src: stubLocator{dir: bundleDir}}, + } + + t.Run("right height wrong hash is now rejected", func(t *testing.T) { + _, err := r.verifyPreRunBundleHead(log, 181, "0xdifferent") + require.Error(t, err) + assert.Contains(t, err.Error(), "different chain") + }) + + t.Run("matching head still reports already applied", func(t *testing.T) { + applied, err := r.verifyPreRunBundleHead(log, 181, "0xend") + require.NoError(t, err) + assert.True(t, applied) + }) + + t.Run("no locator and no local dir cannot verify", func(t *testing.T) { + bare := &runner{cfg: &Config{FullConfig: &config.Config{ + Runner: config.RunnerConfig{Benchmark: config.BenchmarkConfig{ + Tests: config.TestsConfig{Source: config.SourceConfig{ + EESTFixtures: &config.EESTFixturesSource{ + PreRuns: &config.EESTPreRunsSource{}, + }, + }}, + }}, + }}} + + applied, err := bare.verifyPreRunBundleHead(log, 181, "0xend") + require.NoError(t, err) + assert.False(t, applied) + }) +}