From db6d719f24d7e52e2f6e5d9db6ec38ceb663e7cc Mon Sep 17 00:00:00 2001 From: Damilola Edwards Date: Tue, 28 Jul 2026 12:47:32 +0100 Subject: [PATCH] Implement test run retention cleanup cleanupTestHistory was a stub that never removed anything, so every finished test run stayed in the in-memory run map for the life of the process. On a coordinator that runs cron or startup-scheduled tests continuously, this pins an ever-growing set of schedulers, task states, and buffered loggers with no way to bound it, and makes the testRetentionTime setting a no-op. Prune runs whose status is terminal (success, failure, aborted, or skipped) once they are older than the configured retention window, leaving pending and running tests untouched. This only drops the in-memory reference; persisted results are unaffected and continue to be served from the database once a run is no longer in the map, the same fallback path the test history endpoint already relies on. --- pkg/assertoor/testrunner.go | 28 ++++++++++- pkg/assertoor/testrunner_test.go | 80 ++++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 2 deletions(-) create mode 100644 pkg/assertoor/testrunner_test.go diff --git a/pkg/assertoor/testrunner.go b/pkg/assertoor/testrunner.go index 05f54989..e5c471ff 100644 --- a/pkg/assertoor/testrunner.go +++ b/pkg/assertoor/testrunner.go @@ -412,6 +412,30 @@ func (c *TestRunner) RunTestCleanup(ctx context.Context, retentionTime time.Dura } } -func (c *TestRunner) cleanupTestHistory(_ time.Duration) { - // TODO: clean db +// cleanupTestHistory evicts finished test runs from testRunMap once they are +// older than retentionTime. Runs that are still pending or running are never +// touched. Evicting a run here only drops the in-memory reference (and +// everything it pins: its scheduler, task states, buffered loggers); the +// persisted result stays in the database and is served from there via +// test.WrapDBTestRun once the run is no longer in testRunMap. +func (c *TestRunner) cleanupTestHistory(retentionTime time.Duration) { + cutoff := time.Now().Add(-retentionTime) + + c.testRegistryMutex.Lock() + defer c.testRegistryMutex.Unlock() + + for runID, testRef := range c.testRunMap { + switch testRef.Status() { + case types.TestStatusSuccess, types.TestStatusFailure, types.TestStatusAborted, types.TestStatusSkipped: + default: + continue + } + + stopTime := testRef.StopTime() + if stopTime.IsZero() || stopTime.After(cutoff) { + continue + } + + delete(c.testRunMap, runID) + } } diff --git a/pkg/assertoor/testrunner_test.go b/pkg/assertoor/testrunner_test.go new file mode 100644 index 00000000..78088fda --- /dev/null +++ b/pkg/assertoor/testrunner_test.go @@ -0,0 +1,80 @@ +package assertoor + +import ( + "testing" + "time" + + "github.com/ethpandaops/assertoor/pkg/types" +) + +// stubTest is a minimal types.Test double. Only RunID, Status, and StopTime +// matter for cleanupTestHistory; the rest exist to satisfy the interface. +type stubTest struct { + runID uint64 + status types.TestStatus + stopTime time.Time +} + +func (s *stubTest) RunID() uint64 { return s.runID } +func (s *stubTest) TestID() string { return "stub-test" } +func (s *stubTest) Name() string { return "stub test" } +func (s *stubTest) StartTime() time.Time { return s.stopTime.Add(-time.Minute) } +func (s *stubTest) StopTime() time.Time { return s.stopTime } +func (s *stubTest) Timeout() time.Duration { return 0 } +func (s *stubTest) Status() types.TestStatus { return s.status } +func (s *stubTest) GetTaskScheduler() types.TaskScheduler { return nil } +func (s *stubTest) AbortTest(_ bool) {} + +func TestCleanupTestHistory(t *testing.T) { + const retention = time.Hour + + now := time.Now() + + tr := NewTestRunner(nil, 0) + + tr.testRunMap[1] = &stubTest{runID: 1, status: types.TestStatusSuccess, stopTime: now.Add(-2 * retention)} + tr.testRunMap[2] = &stubTest{runID: 2, status: types.TestStatusFailure, stopTime: now.Add(-2 * retention)} + tr.testRunMap[3] = &stubTest{runID: 3, status: types.TestStatusAborted, stopTime: now.Add(-2 * retention)} + tr.testRunMap[4] = &stubTest{runID: 4, status: types.TestStatusSuccess, stopTime: now.Add(-retention / 2)} + tr.testRunMap[5] = &stubTest{runID: 5, status: types.TestStatusRunning, stopTime: time.Time{}} + tr.testRunMap[6] = &stubTest{runID: 6, status: types.TestStatusPending, stopTime: time.Time{}} + + tr.cleanupTestHistory(retention) + + wantEvicted := []uint64{1, 2, 3} + for _, runID := range wantEvicted { + if _, ok := tr.testRunMap[runID]; ok { + t.Errorf("run %d: expected eviction (finished more than retention ago), still present", runID) + } + } + + wantKept := []uint64{4, 5, 6} + for _, runID := range wantKept { + if _, ok := tr.testRunMap[runID]; !ok { + t.Errorf("run %d: expected to be kept, was evicted", runID) + } + } + + if got := len(tr.testRunMap); got != len(wantKept) { + t.Fatalf("testRunMap has %d entries after cleanup, want %d", got, len(wantKept)) + } +} + +func TestCleanupTestHistoryNeverEvictsWithoutRetention(t *testing.T) { + tr := NewTestRunner(nil, 0) + + tr.testRunMap[1] = &stubTest{ + runID: 1, + status: types.TestStatusSuccess, + stopTime: time.Now().Add(-365 * 24 * time.Hour), + } + + // A zero retention window still means "evict anything already finished", + // not "keep everything forever" -- RunTestCleanup is what clamps a + // non-positive config value to the 14 day default before calling in. + tr.cleanupTestHistory(0) + + if _, ok := tr.testRunMap[1]; ok { + t.Fatalf("expected the year-old finished run to be evicted with a zero retention window") + } +}