Fix flaky t.TempDir cleanup race in webhook tests (closes #198)
Check / check (pull_request) Successful in 3m33s
Check / check (pull_request) Successful in 3m33s
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
This commit was merged in pull request #201.
This commit is contained in:
@@ -20,6 +20,10 @@ main cannot regress.
|
|||||||
|
|
||||||
# Completed Steps
|
# 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`
|
- 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
|
(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,
|
so the linter always executes), reduced `script/lint` to building it,
|
||||||
|
|||||||
@@ -6,6 +6,7 @@ import (
|
|||||||
"database/sql"
|
"database/sql"
|
||||||
"fmt"
|
"fmt"
|
||||||
"log/slog"
|
"log/slog"
|
||||||
|
"sync"
|
||||||
|
|
||||||
"go.uber.org/fx"
|
"go.uber.org/fx"
|
||||||
|
|
||||||
@@ -31,6 +32,10 @@ type Service struct {
|
|||||||
db *database.Database
|
db *database.Database
|
||||||
deploy *deploy.Service
|
deploy *deploy.Service
|
||||||
params *ServiceParams
|
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.
|
// New creates a new webhook Service.
|
||||||
@@ -108,6 +113,14 @@ func (svc *Service) HandleWebhook(
|
|||||||
return nil
|
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(
|
func (svc *Service) triggerDeployment(
|
||||||
ctx context.Context,
|
ctx context.Context,
|
||||||
app *models.App,
|
app *models.App,
|
||||||
@@ -117,7 +130,7 @@ func (svc *Service) triggerDeployment(
|
|||||||
eventID := event.ID
|
eventID := event.ID
|
||||||
appName := app.Name
|
appName := app.Name
|
||||||
|
|
||||||
go func() {
|
svc.deployments.Go(func() {
|
||||||
// Use context.WithoutCancel to ensure deployment completes
|
// Use context.WithoutCancel to ensure deployment completes
|
||||||
// even if the HTTP request context is cancelled.
|
// even if the HTTP request context is cancelled.
|
||||||
deployCtx := context.WithoutCancel(ctx)
|
deployCtx := context.WithoutCancel(ctx)
|
||||||
@@ -130,5 +143,5 @@ func (svc *Service) triggerDeployment(
|
|||||||
// Mark event as processed
|
// Mark event as processed
|
||||||
event.Processed = true
|
event.Processed = true
|
||||||
_ = event.Save(deployCtx)
|
_ = event.Save(deployCtx)
|
||||||
}()
|
})
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -7,7 +7,6 @@ import (
|
|||||||
"os"
|
"os"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
"testing"
|
"testing"
|
||||||
"time"
|
|
||||||
|
|
||||||
"github.com/stretchr/testify/assert"
|
"github.com/stretchr/testify/assert"
|
||||||
"github.com/stretchr/testify/require"
|
"github.com/stretchr/testify/require"
|
||||||
@@ -828,8 +827,9 @@ func TestExtractBranch(testingT *testing.T) {
|
|||||||
)
|
)
|
||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
|
|
||||||
// Allow async deployment goroutine to complete before test cleanup
|
// Wait for the async deployment goroutine to finish so its
|
||||||
time.Sleep(100 * time.Millisecond)
|
// writes under the temp dir complete before test cleanup.
|
||||||
|
svc.WaitForDeployments()
|
||||||
|
|
||||||
events, err := app.GetWebhookEvents(context.Background(), 10)
|
events, err := app.GetWebhookEvents(context.Background(), 10)
|
||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
@@ -867,8 +867,9 @@ func TestHandleWebhookMatchingBranch(t *testing.T) {
|
|||||||
)
|
)
|
||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
|
|
||||||
// Allow async deployment goroutine to complete before test cleanup
|
// Wait for the async deployment goroutine to finish so its writes
|
||||||
time.Sleep(100 * time.Millisecond)
|
// under the temp dir complete before test cleanup.
|
||||||
|
svc.WaitForDeployments()
|
||||||
|
|
||||||
events, err := app.GetWebhookEvents(context.Background(), 10)
|
events, err := app.GetWebhookEvents(context.Background(), 10)
|
||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
@@ -962,8 +963,9 @@ func assertHandleWebhookDeploys(
|
|||||||
err := svc.HandleWebhook(context.Background(), app, source, pushEventType, payload)
|
err := svc.HandleWebhook(context.Background(), app, source, pushEventType, payload)
|
||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
|
|
||||||
// Allow async deployment goroutine to complete before test cleanup
|
// Wait for the async deployment goroutine to finish so its writes
|
||||||
time.Sleep(100 * time.Millisecond)
|
// under the temp dir complete before test cleanup.
|
||||||
|
svc.WaitForDeployments()
|
||||||
|
|
||||||
events, err := app.GetWebhookEvents(context.Background(), 10)
|
events, err := app.GetWebhookEvents(context.Background(), 10)
|
||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
|
|||||||
Reference in New Issue
Block a user