From 2ded9d6a7345611f68d88e69fda460d21964a611 Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Fri, 10 Oct 2025 10:02:03 +0400 Subject: [PATCH] test: remove external compilation of db cleaner entirely (#20240) fixes https://github.com/coder/internal/issues/1026 Thru a (perhaps too-) clever hack of `init()` functions, I've managed to remove the need to separately compile the cleaner binary. This should fix the flakes we are seeing were the binary compilation takes 10s of seconds on macOS. The cleaner is encorporated directly into the test binary and we self-exec as the subprocess. --- Makefile | 12 +---- coderd/database/dbtestutil/cleaner.go | 47 +++++++++++-------- coderd/database/dbtestutil/cleaner_posix.go | 34 -------------- coderd/database/dbtestutil/cleaner_windows.go | 11 ----- coderd/database/dbtestutil/cleanercmd/main.go | 7 --- 5 files changed, 29 insertions(+), 82 deletions(-) delete mode 100644 coderd/database/dbtestutil/cleaner_posix.go delete mode 100644 coderd/database/dbtestutil/cleaner_windows.go delete mode 100644 coderd/database/dbtestutil/cleanercmd/main.go diff --git a/Makefile b/Makefile index 8f7bc9d17e..8b17b88e20 100644 --- a/Makefile +++ b/Makefile @@ -1020,19 +1020,11 @@ endif TEST_PACKAGES ?= ./... -warm-go-cache-db-cleaner: - # ensure Go's build cache for the cleanercmd is fresh so that tests don't have to build from scratch. This - # could take some time and counts against the test's timeout, which can lead to flakes. - # c.f. https://github.com/coder/internal/issues/1026 - mkdir -p build - $(GIT_FLAGS) go build -o ./build/cleaner github.com/coder/coder/v2/coderd/database/dbtestutil/cleanercmd -.PHONY: warm-go-cache-db-cleaner - -test: warm-go-cache-db-cleaner +test: $(GIT_FLAGS) gotestsum --format standard-quiet $(GOTESTSUM_RETRY_FLAGS) --packages="$(TEST_PACKAGES)" -- $(GOTEST_FLAGS) .PHONY: test -test-cli: warm-go-cache-db-cleaner +test-cli: $(MAKE) test TEST_PACKAGES="./cli..." .PHONY: test-cli diff --git a/coderd/database/dbtestutil/cleaner.go b/coderd/database/dbtestutil/cleaner.go index 23da8c8fc7..851f4488f8 100644 --- a/coderd/database/dbtestutil/cleaner.go +++ b/coderd/database/dbtestutil/cleaner.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "os" + "os/exec" "os/signal" "time" @@ -21,36 +22,43 @@ const ( cleanerRespOK = "OK" envCleanerParentUUID = "DB_CLEANER_PARENT_UUID" envCleanerDSN = "DB_CLEANER_DSN" -) - -var ( - originalWorkingDir string - errGettingWorkingDir error + envCleanerMagic = "DB_CLEANER_MAGIC" + envCleanerMagicValue = "XEHdJqWehWek8AaWwopy" // 20 random characters to make this collision resistant ) func init() { - // We expect our tests to run from somewhere in the project tree where `go run` below in `startCleaner` will - // be able to resolve the command package. However, some of the tests modify the working directory during the run. - // So, we grab the working directory during package init, before tests are run, and then set that work dir on the - // subcommand process before it starts. - originalWorkingDir, errGettingWorkingDir = os.Getwd() + // We are hijacking the init() function here to do something very non-standard. + // + // We want to be able to run the cleaner as a subprocess of the test process so that it can outlive the test binary + // and still clean up, even if the test process times out or is killed. So, what we do is in startCleaner() below, + // which is called in the parent process, we exec our own binary and set a collision-resistant environment variable. + // Then here in the init(), which will run before main() and therefore before executing tests, we check for the + // environment variable, and if present we know this is the child process and we exec the cleaner. Instead of + // returning normally from init() we call os.Exit(). This prevents tests from being re-run in the child process (and + // recursion). + // + // If the magic value is not present, we know we are the parent process and init() returns normally. + magicValue := os.Getenv(envCleanerMagic) + if magicValue == envCleanerMagicValue { + RunCleaner() + os.Exit(0) + } } // startCleaner starts the cleaner in a subprocess. holdThis is an opaque reference that needs to be kept from being // garbage collected until we are done with all test databases (e.g. the end of the process). -func startCleaner(ctx context.Context, t TBSubset, parentUUID uuid.UUID, dsn string) (holdThis any, err error) { - cmd := cleanerCmd(t) +func startCleaner(ctx context.Context, _ TBSubset, parentUUID uuid.UUID, dsn string) (holdThis any, err error) { + bin, err := os.Executable() + if err != nil { + return nil, xerrors.Errorf("could not get executable path: %w", err) + } + cmd := exec.Command(bin) cmd.Env = append(os.Environ(), fmt.Sprintf("%s=%s", envCleanerParentUUID, parentUUID.String()), fmt.Sprintf("%s=%s", envCleanerDSN, dsn), + fmt.Sprintf("%s=%s", envCleanerMagic, envCleanerMagicValue), ) - // c.f. comment on `func init()` in this file. - if errGettingWorkingDir != nil { - return nil, xerrors.Errorf("failed to get working directory during init: %w", errGettingWorkingDir) - } - cmd.Dir = originalWorkingDir - // Here we don't actually use the reference to the stdin pipe, because we never write anything to it. When this // process exits, the pipe is closed by the OS and this triggers the cleaner to do its cleaning work. But, we do // need to hang on to a reference to it so that it doesn't get garbage collected and trigger cleanup early. @@ -177,8 +185,7 @@ func (c *cleaner) waitAndClean() { } // RunCleaner runs the test database cleaning process. It takes no arguments but uses stdio and environment variables -// for its operation. It is designed to be launched as the only task of a `main()` process, but is included in this -// package to share constants with the parent code that launches it above. +// for its operation. // // The cleaner is designed to run in a separate process from the main test suite, connected over stdio. If the main test // process ends (panics, times out, or is killed) without explicitly discarding the databases it clones, the cleaner diff --git a/coderd/database/dbtestutil/cleaner_posix.go b/coderd/database/dbtestutil/cleaner_posix.go deleted file mode 100644 index 3c8188e5d8..0000000000 --- a/coderd/database/dbtestutil/cleaner_posix.go +++ /dev/null @@ -1,34 +0,0 @@ -//go:build !windows - -package dbtestutil - -import ( - "os/exec" - "path/filepath" - "time" -) - -const timeFormat = "2006-01-02 15:04:05.000" - -// cleanerCmd builds the cleaner binary in a temporary directory and returns a command to execute it. We can do this on -// POSIX because it's OK to delete the temporary directory after the test: the binary can still run. This is not -// possible on Windows because cleaning the temporary directory will fail if the binary is still running. -// c.f. cleaner_windows.go. -func cleanerCmd(t TBSubset) *exec.Cmd { - start := time.Now() - t.Logf("[%s] starting cleaner binary build", start.Format(timeFormat)) - tempDir := t.TempDir() - cleanerBinary := filepath.Join(tempDir, "cleaner") - - buildCmd := exec.Command("go", "build", "-o", cleanerBinary, "github.com/coder/coder/v2/coderd/database/dbtestutil/cleanercmd") - output, err := buildCmd.CombinedOutput() - if err != nil { - t.Logf("failed to build cleaner binary: %v", err) - t.Logf("output: %s", string(output)) - // Fall back to go run if build fails - return exec.Command("go", "run", "github.com/coder/coder/v2/coderd/database/dbtestutil/cleanercmd") - } - t.Logf("[%s] cleaner binary %s built in %s", time.Now().Format(timeFormat), cleanerBinary, time.Since(start)) - - return exec.Command(cleanerBinary) -} diff --git a/coderd/database/dbtestutil/cleaner_windows.go b/coderd/database/dbtestutil/cleaner_windows.go deleted file mode 100644 index 4c743b90ed..0000000000 --- a/coderd/database/dbtestutil/cleaner_windows.go +++ /dev/null @@ -1,11 +0,0 @@ -//go:build windows - -package dbtestutil - -import "os/exec" - -// cleanerCmd returns a command to execute the cleaner binary. We do this with go run on Windows because we can't -// delete the temporary directory after the test: the binary will still be running. c.f. cleaner_posix.go. -func cleanerCmd(_ TBSubset) *exec.Cmd { - return exec.Command("go", "run", "github.com/coder/coder/v2/coderd/database/dbtestutil/cleanercmd") -} diff --git a/coderd/database/dbtestutil/cleanercmd/main.go b/coderd/database/dbtestutil/cleanercmd/main.go deleted file mode 100644 index 038637320e..0000000000 --- a/coderd/database/dbtestutil/cleanercmd/main.go +++ /dev/null @@ -1,7 +0,0 @@ -package main - -import "github.com/coder/coder/v2/coderd/database/dbtestutil" - -func main() { - dbtestutil.RunCleaner() -}