From 1d385854314ba2d62978686fa945726828a252d3 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 22 Sep 2026 10:00:50 +0200 Subject: [PATCH 1/5] Add .prettierignore for vendored minified JS (closes #185) script/fmt ran prettier over static/js/*.js, which rewrote the vendored minified static/js/alpine.min.js. A root .prettierignore with *.min.js excludes vendored bundles: make fmt on a clean tree now yields no changes and alpine.min.js stays byte-identical, while first-party JS still formats. Model: opus-4-8 --- .prettierignore | 2 ++ TODO.md | 2 ++ 2 files changed, 4 insertions(+) create mode 100644 .prettierignore diff --git a/.prettierignore b/.prettierignore new file mode 100644 index 0000000..d87c685 --- /dev/null +++ b/.prettierignore @@ -0,0 +1,2 @@ +# Vendored, minified third-party bundles must never be reformatted. +*.min.js diff --git a/TODO.md b/TODO.md index 2f051a0..a3d6bf5 100644 --- a/TODO.md +++ b/TODO.md @@ -20,6 +20,8 @@ main cannot regress. # Completed Steps +- 2026-09-22: Added `.prettierignore` so `make fmt` no longer rewrites + the vendored `static/js/alpine.min.js` bundle (#185). - 2026-09-09: Fixed four deployability blockers found by QA: CSRF origin check over plain HTTP (`UPAAS_PLAINTEXT_HTTP`, #189), pulling the git image when absent (#190), the env-var editor CSRF token lookup (#191), From f1dfd382a4a2102da1f5ad7fbea433f9ba7556fe Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 22 Sep 2026 11:01:07 +0200 Subject: [PATCH 2/5] Reject path traversal in deploy log download handler (closes #177) The deploy-log download handler passed a request-derived path to http.ServeFile, which gosec flags as G703 (path traversal via taint). The handler now opens the log through an os.Root confined to the deploy log directory, so any escaping path is rejected at runtime (404) and the file is streamed with http.ServeContent. A regression test plants a sentinel outside the log dir and asserts the traversal is refused and its contents never served; removing the guard makes that test fail. No //nolint used. Model: opus-4-8 --- TODO.md | 3 + internal/handlers/app.go | 37 ++++++-- internal/handlers/handlers_test.go | 2 + internal/handlers/log_download_test.go | 121 +++++++++++++++++++++++++ internal/service/deploy/deploy.go | 6 ++ 5 files changed, 161 insertions(+), 8 deletions(-) create mode 100644 internal/handlers/log_download_test.go diff --git a/TODO.md b/TODO.md index a3d6bf5..06257f2 100644 --- a/TODO.md +++ b/TODO.md @@ -22,6 +22,9 @@ main cannot regress. - 2026-09-22: Added `.prettierignore` so `make fmt` no longer rewrites the vendored `static/js/alpine.min.js` bundle (#185). +- 2026-09-22: Fixed the gosec G703 path-traversal finding in the deploy + log download handler by verifying the resolved path stays within the + deploy log directory before serving, returning 404 on escape (#177). - 2026-09-09: Fixed four deployability blockers found by QA: CSRF origin check over plain HTTP (`UPAAS_PLAINTEXT_HTTP`, #189), pulling the git image when absent (#190), the env-var editor CSRF token lookup (#191), diff --git a/internal/handlers/app.go b/internal/handlers/app.go index aa83aed..a985c11 100644 --- a/internal/handlers/app.go +++ b/internal/handlers/app.go @@ -611,7 +611,13 @@ func (h *Handlers) HandleDeploymentLogDownload() http.HandlerFunc { return } - // Get the log file path from deploy service + // The log path is derived from request data (the app is looked + // up by a URL parameter), so open it through an os.Root confined + // to the deploy log directory. Root.Open rejects any path that + // escapes the root, so a traversal attempt fails rather than + // serving an arbitrary file. + logDir := h.deploy.GetLogDir() + logPath := h.deploy.GetLogFilePath(application, deployment) if logPath == "" { http.NotFound(writer, request) @@ -619,28 +625,43 @@ func (h *Handlers) HandleDeploymentLogDownload() http.HandlerFunc { return } - // Check if file exists — logPath is constructed internally, not from user input - _, err := os.Stat(logPath) // #nosec G703 -- internal path, not user input - if os.IsNotExist(err) { + relPath, relErr := filepath.Rel(logDir, logPath) + if relErr != nil { http.NotFound(writer, request) return } - if err != nil { - h.log.Error("failed to stat log file", "error", err, "path", logPath) + root, rootErr := os.OpenRoot(logDir) + if rootErr != nil { + http.NotFound(writer, request) + + return + } + defer func() { _ = root.Close() }() + + file, openErr := root.Open(relPath) + if openErr != nil { + http.NotFound(writer, request) + + return + } + defer func() { _ = file.Close() }() + + info, statErr := file.Stat() + if statErr != nil { + h.log.Error("failed to stat log file", "error", statErr, "path", logPath) http.Error(writer, "Internal Server Error", http.StatusInternalServerError) return } - // Extract filename for Content-Disposition header filename := filepath.Base(logPath) writer.Header().Set("Content-Type", "text/plain; charset=utf-8") writer.Header().Set("Content-Disposition", "attachment; filename=\""+filename+"\"") - http.ServeFile(writer, request, logPath) // #nosec G703 -- internal path + http.ServeContent(writer, request, filename, info.ModTime(), file) } } diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index 6e0624a..68e116e 100644 --- a/internal/handlers/handlers_test.go +++ b/internal/handlers/handlers_test.go @@ -42,6 +42,7 @@ type testContext struct { database *database.Database authSvc *auth.Service appSvc *app.Service + deploySvc *deploy.Service middleware *middleware.Middleware } @@ -186,6 +187,7 @@ func setupTestHandlers(t *testing.T) *testContext { database: dbInstance, authSvc: authSvc, appSvc: appSvc, + deploySvc: deploySvc, middleware: mw, } } diff --git a/internal/handlers/log_download_test.go b/internal/handlers/log_download_test.go new file mode 100644 index 0000000..cbbfd08 --- /dev/null +++ b/internal/handlers/log_download_test.go @@ -0,0 +1,121 @@ +package handlers_test + +import ( + "context" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strconv" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "sneak.berlin/go/upaas/internal/models" +) + +// doLogDownload issues a log-download request for the given app and +// deployment and returns the recorder. +func doLogDownload( + t *testing.T, + testCtx *testContext, + appID string, + deploymentID int64, +) *httptest.ResponseRecorder { + t.Helper() + + idStr := strconv.FormatInt(deploymentID, 10) + + request := httptest.NewRequestWithContext( + t.Context(), + http.MethodGet, + "/apps/"+appID+"/deployments/"+idStr+"/log", + nil, + ) + request = addChiURLParams(request, map[string]string{ + "id": appID, + "deploymentID": idStr, + }) + + recorder := httptest.NewRecorder() + testCtx.handlers.HandleDeploymentLogDownload().ServeHTTP(recorder, request) + + return recorder +} + +// TestHandleDeploymentLogDownloadServesLegitimateFile verifies a normal +// log file is served for download. +func TestHandleDeploymentLogDownloadServesLegitimateFile(t *testing.T) { + t.Parallel() + + testCtx := setupTestHandlers(t) + createdApp := createTestApp(t, testCtx, "log-download-app") + + deployment := models.NewDeployment(testCtx.database) + deployment.AppID = createdApp.ID + deployment.Status = models.DeploymentStatusSuccess + require.NoError(t, deployment.Save(context.Background())) + + // Write the log file where the handler will look for it. + logPath := testCtx.deploySvc.GetLogFilePath(createdApp, deployment) + require.NoError(t, os.MkdirAll(filepath.Dir(logPath), 0o750)) + require.NoError(t, os.WriteFile(logPath, []byte("deploy log contents"), 0o600)) + + recorder := doLogDownload(t, testCtx, createdApp.ID, deployment.ID) + + assert.Equal(t, http.StatusOK, recorder.Code) + assert.Contains(t, recorder.Body.String(), "deploy log contents") +} + +// TestHandleDeploymentLogDownloadRejectsPathTraversal verifies the +// os.Root containment guard. A traversal-shaped app name drives the +// resolved log path out of the deploy log directory onto a sentinel +// file that really exists. The handler must refuse to serve it (404) +// rather than leak its contents. Removing the guard makes this test +// fail, which the earlier version — pointed at a non-existent path that +// 404s either way — did not. +func TestHandleDeploymentLogDownloadRejectsPathTraversal(t *testing.T) { + t.Parallel() + + testCtx := setupTestHandlers(t) + createdApp := createTestApp(t, testCtx, "log-traversal-app") + + createdApp.Name = "../.." + require.NoError(t, createdApp.Save(context.Background())) + + // The log root must exist so os.OpenRoot succeeds and the rejection + // comes from the containment check, not a missing directory. + logDir := testCtx.deploySvc.GetLogDir() + require.NoError(t, os.MkdirAll(logDir, 0o750)) + + deployment := models.NewDeployment(testCtx.database) + deployment.AppID = createdApp.ID + deployment.Status = models.DeploymentStatusSuccess + require.NoError(t, deployment.Save(context.Background())) + + // Where the handler resolves the log path to. The traversal name + // makes this land outside logDir; require that it truly escapes so + // the test cannot silently stop covering the guard. + escapedPath := testCtx.deploySvc.GetLogFilePath(createdApp, deployment) + relPath, relErr := filepath.Rel(logDir, escapedPath) + require.NoError(t, relErr) + require.True(t, strings.HasPrefix(relPath, ".."), + "resolved path must escape the log dir, got %q", relPath) + + // Plant a sentinel where the traversal points; a missing guard would + // open and serve it. + require.NoError(t, os.MkdirAll(filepath.Dir(escapedPath), 0o750)) + + const sentinel = "SENTINEL-outside-log-dir-must-not-be-served" + + require.NoError(t, os.WriteFile(escapedPath, []byte(sentinel), 0o600)) + t.Cleanup(func() { _ = os.Remove(escapedPath) }) + + recorder := doLogDownload(t, testCtx, createdApp.ID, deployment.ID) + + assert.Equal(t, http.StatusNotFound, recorder.Code) + assert.NotContains(t, recorder.Body.String(), sentinel, + "containment guard must not serve a file outside the log dir") +} diff --git a/internal/service/deploy/deploy.go b/internal/service/deploy/deploy.go index 887a094..ab1f2c8 100644 --- a/internal/service/deploy/deploy.go +++ b/internal/service/deploy/deploy.go @@ -294,6 +294,12 @@ func (svc *Service) GetLogFilePath( return filepath.Join(svc.config.DataDir, "logs", hostname, app.Name, filename) } +// GetLogDir returns the root directory under which all deployment log +// files live. Paths returned by GetLogFilePath are always inside it. +func (svc *Service) GetLogDir() string { + return filepath.Join(svc.config.DataDir, "logs") +} + // HasActiveDeploy returns true if there is an active deployment for the given app. func (svc *Service) HasActiveDeploy(appID string) bool { _, ok := svc.activeDeploys.Load(appID) From 727bd50935616d5c2eaa861ccbcef5bd0eb1fcaa Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 22 Sep 2026 11:11:15 +0200 Subject: [PATCH 3/5] Install pinned goimports in script/bootstrap (closes #184) script/fmt runs goimports, but script/bootstrap did not install it, so make fmt failed with goimports: not found on a fresh machine. bootstrap now installs goimports v0.49.0 (pinned; compatible with the repo Go 1.25, so no toolchain download) into /usr/local/bin, guarded to skip when it is already present. Node/prettier pinning is left to a separate issue; the check gate runs only gofmt, so main is unaffected. Model: opus-4-8 --- TODO.md | 3 +++ script/bootstrap | 21 ++++++++++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/TODO.md b/TODO.md index 06257f2..0f7610e 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,9 @@ main cannot regress. - 2026-09-22: Fixed the gosec G703 path-traversal finding in the deploy log download handler by verifying the resolved path stays within the deploy log directory before serving, returning 404 on escape (#177). +- 2026-09-22: `script/bootstrap` now installs a pinned `goimports` + (`golang.org/x/tools` v0.49.0) into `/usr/local/bin`, so `make fmt` + succeeds on a fresh machine after `make bootstrap` (#184). - 2026-09-09: Fixed four deployability blockers found by QA: CSRF origin check over plain HTTP (`UPAAS_PLAINTEXT_HTTP`, #189), pulling the git image when absent (#190), the env-var editor CSRF token lookup (#191), diff --git a/script/bootstrap b/script/bootstrap index f1b3e30..34b2b13 100755 --- a/script/bootstrap +++ b/script/bootstrap @@ -5,7 +5,9 @@ # or apk (detected in that order); assumes NOTHING is present (not git, # make, or go). golangci-lint is packaged in nix, brew, and apk; on apt # it is installed from a hash-verified GitHub release archive (never -# curl | sh). +# curl | sh). goimports is installed with `go install` at a pinned +# version (integrity via the Go module checksum database) into +# /usr/local/bin so it is on PATH. set -eu ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" @@ -15,6 +17,9 @@ GOLANGCI_LINT_VERSION="2.12.2" # sha256 of golangci-lint-2.12.2-linux-.tar.gz release archives GOLANGCI_LINT_SHA256_AMD64="8df580d2670fed8fa984aac0507099af8df275e665215f5c7a2ae3943893a553" GOLANGCI_LINT_SHA256_ARM64="44cd40a8c76c86755375adfeea52cfd3533cb43d7bd647771e0ae065e166df3a" +# golang.org/x/tools goimports, 2026-08-13. v0.49.0 requires Go 1.25 (matches +# go.mod); v0.50.0 needs Go 1.26. Integrity via the Go module checksum database. +GOIMPORTS_VERSION="v0.49.0" PKGMGR="" SUDO="" @@ -102,6 +107,19 @@ ensure_golangci_lint() { esac } +# goimports is not packaged uniformly across nix/apt/brew/apk, so install it +# with `go install` at a pinned version and place the binary in /usr/local/bin +# so it is on PATH regardless of shell config, as the golangci-lint release +# install does. Requires go, which main installs first. +ensure_goimports() { + if ! missing goimports; then return 0; fi + detect_pkgmgr + tmp="$(mktemp -d)" + GOBIN="$tmp" go install "golang.org/x/tools/cmd/goimports@${GOIMPORTS_VERSION}" + $SUDO install -m 0755 "$tmp/goimports" /usr/local/bin/goimports + rm -rf "$tmp" +} + main() { cd "$ROOT" @@ -112,6 +130,7 @@ main() { # Go toolchain and linter if missing go; then pkg_install go golang go go; fi ensure_golangci_lint + ensure_goimports go mod download From d946fa68f926b4102206aa448e21aa5f92dea12c Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 22 Sep 2026 12:11:19 +0200 Subject: [PATCH 4/5] Run all linting in Docker via Dockerfile.lint (closes #188) Per the owner ruling, linting now runs only inside Docker with the pinned golangci-lint (v2.12.2). A root Dockerfile.lint runs the linter as a build step; script/lint just builds it. A GATE_RUN build arg forces the lint layer to execute every run so a cached build cannot report a false clean. script/bootstrap no longer installs golangci-lint (the goimports install stays). The main Dockerfile lint stage calls golangci-lint directly (no docker-in-docker) and still gates the build. config verify is omitted because it fetches its schema over an unpinned HTTPS call. Model: opus-4-8 --- Dockerfile | 6 +++- Dockerfile.lint | 23 +++++++++++++++ TODO.md | 6 ++++ script/bootstrap | 74 ++++++++++-------------------------------------- script/lint | 16 +++++++++-- 5 files changed, 63 insertions(+), 62 deletions(-) create mode 100644 Dockerfile.lint diff --git a/Dockerfile b/Dockerfile index f31025e..1a996e8 100644 --- a/Dockerfile +++ b/Dockerfile @@ -8,8 +8,12 @@ RUN go mod download COPY . . +# golangci-lint is invoked directly here, not via `make lint`: script/lint +# now runs the linter by building Dockerfile.lint, and shelling out to +# `docker build` from inside this image build would be docker-in-docker. +# This image is golangci/golangci-lint, so the pinned linter is on PATH. RUN make fmt-check -RUN make lint +RUN golangci-lint run --config .golangci.yml ./... # Build stage — tests and compilation # golang:1.25-alpine diff --git a/Dockerfile.lint b/Dockerfile.lint new file mode 100644 index 0000000..9836aef --- /dev/null +++ b/Dockerfile.lint @@ -0,0 +1,23 @@ +# Lint image — runs golangci-lint inside a container so every lint uses +# the pinned linter, never a host binary. Linting is a build step, so a +# successful build is a clean lint. Built by script/lint. +# golangci/golangci-lint:v2.12.2 (Debian-based), 2026-08-07 +FROM golangci/golangci-lint:v2.12.2@sha256:5cceeef04e53efe1470638d4b4b4f5ceefd574955ab3941b2d9a68a8c9ad5240 + +WORKDIR /src + +COPY go.mod go.sum ./ +RUN go mod download + +COPY . . + +# Caching is waived for linting: on an unchanged tree a cached build runs +# no linter and still exits 0 in under a second. script/lint passes a +# fresh GATE_RUN every time, and referencing it here forces this step to +# re-run, so the linter always executes. +# +# `golangci-lint config verify` is deliberately NOT run: it fetches its +# JSON schema over an unpinned live HTTPS call, which REPO_POLICIES.md +# forbids (all external references must be pinned by hash). +ARG GATE_RUN +RUN echo "lint run: ${GATE_RUN}"; golangci-lint run --config .golangci.yml ./... diff --git a/TODO.md b/TODO.md index 0f7610e..1f8ab6a 100644 --- a/TODO.md +++ b/TODO.md @@ -20,6 +20,12 @@ main cannot regress. # Completed Steps +- 2026-09-22: Linting now runs only in Docker. Added `Dockerfile.lint` + (pinned golangci-lint v2.12.2, cache-busted via a `GATE_RUN` build arg + so the linter always executes), reduced `script/lint` to building it, + dropped the golangci-lint install from `script/bootstrap`, and switched + the `Dockerfile` lint stage to invoke `golangci-lint` directly instead + of `make lint` to avoid docker-in-docker (#188). - 2026-09-22: Added `.prettierignore` so `make fmt` no longer rewrites the vendored `static/js/alpine.min.js` bundle (#185). - 2026-09-22: Fixed the gosec G703 path-traversal finding in the deploy diff --git a/script/bootstrap b/script/bootstrap index 34b2b13..095afa8 100755 --- a/script/bootstrap +++ b/script/bootstrap @@ -3,20 +3,15 @@ # this repo. Idempotent: every install is guarded by a check so already # installed tools are skipped. Base tooling comes from nix, apt, brew, # or apk (detected in that order); assumes NOTHING is present (not git, -# make, or go). golangci-lint is packaged in nix, brew, and apk; on apt -# it is installed from a hash-verified GitHub release archive (never -# curl | sh). goimports is installed with `go install` at a pinned +# make, or go). goimports is installed with `go install` at a pinned # version (integrity via the Go module checksum database) into -# /usr/local/bin so it is on PATH. +# /usr/local/bin so it is on PATH. The linter is not installed here: it +# runs only in Docker via script/lint, so docker is its sole prerequisite. set -eu ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" -# Pinned versions, 2026-08-07. Never "latest"; exact versions only. -GOLANGCI_LINT_VERSION="2.12.2" -# sha256 of golangci-lint-2.12.2-linux-.tar.gz release archives -GOLANGCI_LINT_SHA256_AMD64="8df580d2670fed8fa984aac0507099af8df275e665215f5c7a2ae3943893a553" -GOLANGCI_LINT_SHA256_ARM64="44cd40a8c76c86755375adfeea52cfd3533cb43d7bd647771e0ae065e166df3a" +# Pinned versions. Never "latest"; exact versions only. # golang.org/x/tools goimports, 2026-08-13. v0.49.0 requires Go 1.25 (matches # go.mod); v0.50.0 needs Go 1.26. Integrity via the Go module checksum database. GOIMPORTS_VERSION="v0.49.0" @@ -61,56 +56,10 @@ missing() { ! command -v "$1" >/dev/null 2>&1 } -# verify_sha256 -verify_sha256() { - if command -v sha256sum >/dev/null 2>&1; then - actual="$(sha256sum "$1" | cut -d' ' -f1)" - else - actual="$(shasum -a 256 "$1" | cut -d' ' -f1)" - fi - if [ "$actual" != "$2" ]; then - echo "bootstrap: sha256 mismatch for $1" >&2 - echo " expected: $2" >&2 - echo " actual: $actual" >&2 - exit 1 - fi -} - -# apt has no golangci-lint package: install a pinned release archive -# from GitHub, verified by hardcoded sha256 (never curl | sh). -install_golangci_lint_release() { - case "$(uname -m)" in - x86_64) goarch="amd64"; sha="$GOLANGCI_LINT_SHA256_AMD64" ;; - aarch64|arm64) goarch="arm64"; sha="$GOLANGCI_LINT_SHA256_ARM64" ;; - *) - echo "bootstrap: unsupported architecture $(uname -m)" >&2 - exit 1 - ;; - esac - if missing curl; then pkg_install curl curl curl curl; fi - name="golangci-lint-${GOLANGCI_LINT_VERSION}-linux-${goarch}" - tmp="$(mktemp -d)" - curl -fsSL -o "$tmp/$name.tar.gz" \ - "https://github.com/golangci/golangci-lint/releases/download/v${GOLANGCI_LINT_VERSION}/${name}.tar.gz" - verify_sha256 "$tmp/$name.tar.gz" "$sha" - tar -xzf "$tmp/$name.tar.gz" -C "$tmp" - $SUDO install -m 0755 "$tmp/$name/golangci-lint" /usr/local/bin/golangci-lint - rm -rf "$tmp" -} - -ensure_golangci_lint() { - if ! missing golangci-lint; then return 0; fi - detect_pkgmgr - case "$PKGMGR" in - apt) install_golangci_lint_release ;; - *) pkg_install golangci-lint golangci-lint golangci-lint golangci-lint ;; - esac -} - # goimports is not packaged uniformly across nix/apt/brew/apk, so install it # with `go install` at a pinned version and place the binary in /usr/local/bin -# so it is on PATH regardless of shell config, as the golangci-lint release -# install does. Requires go, which main installs first. +# so it is on PATH regardless of shell config. Requires go, which main +# installs first. ensure_goimports() { if ! missing goimports; then return 0; fi detect_pkgmgr @@ -127,11 +76,18 @@ main() { if missing git; then pkg_install git git git git; fi if missing make; then pkg_install gnumake make make make; fi - # Go toolchain and linter + # Go toolchain if missing go; then pkg_install go golang go go; fi - ensure_golangci_lint ensure_goimports + # The linter runs only in Docker (script/lint). Warn, don't fail: the + # rest of the repo works without it. + if missing docker; then + echo "bootstrap: WARNING: docker not found; make lint and" >&2 + echo "bootstrap: make check require it. Install docker to run" >&2 + echo "bootstrap: the linter." >&2 + fi + go mod download echo "bootstrap complete" diff --git a/script/lint b/script/lint index 8017180..1d56b58 100755 --- a/script/lint +++ b/script/lint @@ -1,12 +1,24 @@ #!/bin/sh -# script/lint: run the linter. +# script/lint: run golangci-lint. The linter is never installed on the +# host; it runs only inside Docker, from the pinned image in +# Dockerfile.lint, so every run uses the same linter version everywhere. +# Linting is a build step there, so a successful build is a clean lint. +# +# GATE_RUN differs every run so the lint layer always executes; a cached +# build would otherwise exit 0 in under a second having linted nothing. +# --output=type=cacheonly discards the image and keeps only build cache, +# so no tagged image is left behind. set -eu ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" main() { cd "$ROOT" - golangci-lint run --config .golangci.yml ./... + docker build \ + --build-arg GATE_RUN="$(date +%s)-$$" \ + --output=type=cacheonly \ + -f Dockerfile.lint \ + . } main "$@" From f2e4be5eedb1c2be920a7e04189c7bb5c23b5843 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 22 Sep 2026 12:28:23 +0200 Subject: [PATCH 5/5] Fix flaky t.TempDir cleanup race in webhook tests (closes #198) HandleWebhook starts a deployment in a detached goroutine that writes under the app data directory, which is the tests t.TempDir; the tests slept 100ms and returned, racing Go automatic TempDir cleanup and intermittently failing with RemoveAll: directory not empty. The webhook Service now tracks those goroutines in a sync.WaitGroup and exposes WaitForDeployments; the tests wait on it instead of sleeping. Production behavior is unchanged apart from making completion observable. Model: opus-4-8 --- TODO.md | 4 ++++ internal/service/webhook/webhook.go | 17 +++++++++++++++-- internal/service/webhook/webhook_test.go | 16 +++++++++------- 3 files changed, 28 insertions(+), 9 deletions(-) diff --git a/TODO.md b/TODO.md index 1f8ab6a..ae65dff 100644 --- a/TODO.md +++ b/TODO.md @@ -20,6 +20,10 @@ main cannot regress. # Completed Steps +- 2026-09-22: Fixed the flaky `t.TempDir` cleanup race in + `internal/service/webhook` by tracking the async deployment goroutine + in a `sync.WaitGroup` and exposing `WaitForDeployments`; tests now + synchronize on completion instead of sleeping (#198). - 2026-09-22: Linting now runs only in Docker. Added `Dockerfile.lint` (pinned golangci-lint v2.12.2, cache-busted via a `GATE_RUN` build arg so the linter always executes), reduced `script/lint` to building it, diff --git a/internal/service/webhook/webhook.go b/internal/service/webhook/webhook.go index 69c1f6c..02a51ee 100644 --- a/internal/service/webhook/webhook.go +++ b/internal/service/webhook/webhook.go @@ -6,6 +6,7 @@ import ( "database/sql" "fmt" "log/slog" + "sync" "go.uber.org/fx" @@ -31,6 +32,10 @@ type Service struct { db *database.Database deploy *deploy.Service params *ServiceParams + + // deployments tracks the deployment goroutines started by + // triggerDeployment so callers can wait for them to finish. + deployments sync.WaitGroup } // New creates a new webhook Service. @@ -108,6 +113,14 @@ func (svc *Service) HandleWebhook( return nil } +// WaitForDeployments blocks until every deployment goroutine started by +// HandleWebhook has finished, including all writes under the data +// directory. It exists so callers and tests can synchronize on async +// deployment completion instead of polling or sleeping. +func (svc *Service) WaitForDeployments() { + svc.deployments.Wait() +} + func (svc *Service) triggerDeployment( ctx context.Context, app *models.App, @@ -117,7 +130,7 @@ func (svc *Service) triggerDeployment( eventID := event.ID appName := app.Name - go func() { + svc.deployments.Go(func() { // Use context.WithoutCancel to ensure deployment completes // even if the HTTP request context is cancelled. deployCtx := context.WithoutCancel(ctx) @@ -130,5 +143,5 @@ func (svc *Service) triggerDeployment( // Mark event as processed event.Processed = true _ = event.Save(deployCtx) - }() + }) } diff --git a/internal/service/webhook/webhook_test.go b/internal/service/webhook/webhook_test.go index bda7a98..619b271 100644 --- a/internal/service/webhook/webhook_test.go +++ b/internal/service/webhook/webhook_test.go @@ -7,7 +7,6 @@ import ( "os" "path/filepath" "testing" - "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -828,8 +827,9 @@ func TestExtractBranch(testingT *testing.T) { ) require.NoError(t, err) - // Allow async deployment goroutine to complete before test cleanup - time.Sleep(100 * time.Millisecond) + // Wait for the async deployment goroutine to finish so its + // writes under the temp dir complete before test cleanup. + svc.WaitForDeployments() events, err := app.GetWebhookEvents(context.Background(), 10) require.NoError(t, err) @@ -867,8 +867,9 @@ func TestHandleWebhookMatchingBranch(t *testing.T) { ) require.NoError(t, err) - // Allow async deployment goroutine to complete before test cleanup - time.Sleep(100 * time.Millisecond) + // Wait for the async deployment goroutine to finish so its writes + // under the temp dir complete before test cleanup. + svc.WaitForDeployments() events, err := app.GetWebhookEvents(context.Background(), 10) require.NoError(t, err) @@ -962,8 +963,9 @@ func assertHandleWebhookDeploys( err := svc.HandleWebhook(context.Background(), app, source, pushEventType, payload) require.NoError(t, err) - // Allow async deployment goroutine to complete before test cleanup - time.Sleep(100 * time.Millisecond) + // Wait for the async deployment goroutine to finish so its writes + // under the temp dir complete before test cleanup. + svc.WaitForDeployments() events, err := app.GetWebhookEvents(context.Background(), 10) require.NoError(t, err)