Fix flaky t.TempDir cleanup race in webhook tests (closes #198)
Check / check (pull_request) Skipped
Check / check (pull_request) Skipped
HandleWebhook starts a deployment in a detached goroutine that keeps writing under the app data dir (the test's t.TempDir). The webhook tests slept 100ms and returned, racing Go's automatic TempDir cleanup and intermittently failing with "RemoveAll: directory not empty". Track those goroutines in a sync.WaitGroup on the webhook Service and expose WaitForDeployments; the tests now wait on it instead of sleeping. Deploy joins its own sub-goroutines before returning, so the wait covers every temp-dir write. Production behavior is unchanged apart from making completion observable. Model: opus-4-8
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: Added `.prettierignore` so `make fmt` no longer rewrites
|
- 2026-09-22: Added `.prettierignore` so `make fmt` no longer rewrites
|
||||||
the vendored `static/js/alpine.min.js` bundle (#185).
|
the vendored `static/js/alpine.min.js` bundle (#185).
|
||||||
- 2026-09-22: Fixed the gosec G703 path-traversal finding in the deploy
|
- 2026-09-22: Fixed the gosec G703 path-traversal finding in the deploy
|
||||||
|
|||||||
@@ -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