Skip to content

Fix flaky test #1587

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dgageot merged 1 commit into from
Feb 4, 2026
Merged

Fix flaky test #1587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 18 additions & 13 deletions e2e/cagent_api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
package e2e_test

import (
"bytes"
"context"
"encoding/json"
"fmt"
"io"
"net"
"net/http"
Expand All @@ -15,8 +13,9 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/docker/cagent/cmd/root"
"github.com/docker/cagent/pkg/config"
"github.com/docker/cagent/pkg/server"
"github.com/docker/cagent/pkg/session"
)

type Session struct {
Expand All @@ -38,10 +37,8 @@ func TestCagentAPI_ListSessions(t *testing.T) {
{"desktop.db", 2},
} {
t.Run(tc.db, func(t *testing.T) {
dbPath, err := filepath.Abs("testdata/db/" + tc.db)
require.NoError(t, err)
socketPath := startCagentAPI(t, filepath.Join("testdata", "db", tc.db))

socketPath := startCagentAPI(t, dbPath)
client := &http.Client{
Transport: &http.Transport{
DialContext: func(ctx context.Context, _, _ string) (net.Conn, error) {
Expand All @@ -66,12 +63,18 @@ func TestCagentAPI_ListSessions(t *testing.T) {
func startCagentAPI(t *testing.T, db string) string {
t.Helper()

// Get absolute path to db before changing directory
absDB, err := filepath.Abs(db)
require.NoError(t, err)

tmpDir := t.TempDir()
t.Chdir(tmpDir)
t.Chdir(tmpDir) // Use relative socket path to avoid Unix socket path length limit
Copy link
Contributor

@github-actions github-actions bot Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path construction uses string concatenation instead of filepath.Join

The code uses tmpDir + "/session.db" which hardcodes a forward slash, creating non-portable paths that may fail on Windows where backslashes are expected. This is confirmed as a bug because:

  1. The filepath package is already imported and used correctly elsewhere in the same file (line 40)
  2. This creates inconsistency within the same file
  3. It may cause test failures on Windows CI environments

Recommendation: Use filepath.Join(tmpDir, "session.db") instead to ensure correct path separators on all platforms.


copyFile(t, "session.db", db)
if _, err := os.Stat(db + "-wal"); err == nil {
copyFile(t, "session.db-wal", db+"-wal")
// Copy database files to temp directory
dbCopy := tmpDir + "/session.db"
copyFile(t, dbCopy, absDB)
if _, err := os.Stat(absDB + "-wal"); err == nil {
copyFile(t, dbCopy+"-wal", absDB+"-wal")
}

ln, err := server.Listen(t.Context(), "unix://cagent.sock")
Expand All @@ -80,12 +83,14 @@ func startCagentAPI(t *testing.T, db string) string {
_ = ln.Close()
})

file, err := ln.(*net.UnixListener).File()
sessionStore, err := session.NewSQLiteSessionStore(dbCopy)
require.NoError(t, err)

srv, err := server.New(t.Context(), sessionStore, &config.RuntimeConfig{}, 0, nil)
require.NoError(t, err)

Copy link
Contributor

@github-actions github-actions bot Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Server goroutine lacks synchronization, creating potential race condition

The function starts a goroutine with srv.Serve(t.Context(), ln) and immediately returns "cagent.sock" with no synchronization to ensure the server is ready to accept connections. While the Unix socket is created and bound by Listen() before the goroutine starts, there's still a race window between when the goroutine spawns and when http.Server.Serve() enters its accept loop.

If the test's HTTP client (lines 43-49) attempts to connect during this window, the connection could fail intermittently. This is a classic race condition in test setup that could cause test flakiness—ironically, the issue this PR aims to fix.

Recommendation: Add a readiness check before returning, such as:

  • Attempting to connect in a retry loop
  • Using a channel to signal when the server is ready
  • Using a WaitGroup or similar synchronization primitive

go func() {
Copy link
Contributor

@github-actions github-actions bot Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Server error is silently ignored in goroutine

The goroutine ignores the error from srv.Serve(t.Context(), ln) using _ = srv.Serve(...). If the server fails to start or encounters an error during operation, the test will fail with a confusing "connection refused" error rather than showing the actual server error.

Problems:

  1. Test output visibility: Errors may not be visible in test output depending on log configuration
  2. Race condition: No way to detect if server failed to start before the test proceeds
  3. Error attribution: Test failures will show indirect symptoms rather than root causes
  4. Testing best practice: Errors should be explicitly handled in tests using t.Errorf() or similar

Recommendation: Either send the error through a channel and check it after the test, use t.Errorf() to report the error, or at minimum use t.Logf() to ensure errors are visible in test output.

var stdout, stderr bytes.Buffer
_ = root.Execute(t.Context(), nil, &stdout, &stderr, "api", "-s", "session.db", "--listen", fmt.Sprintf("fd://%d", file.Fd()), "default")
_ = srv.Serve(t.Context(), ln)
}()

return "cagent.sock"
Expand Down
9 changes: 0 additions & 9 deletions pkg/server/listen.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"net"
"os"
"path/filepath"
"strconv"
"strings"
)

Expand All @@ -18,14 +17,6 @@ func Listen(ctx context.Context, addr string) (net.Listener, error) {
return listenNamedPipe(path)
}

if fdStr, ok := strings.CutPrefix(addr, "fd://"); ok {
fd, err := strconv.Atoi(fdStr)
if err != nil {
return nil, err
}
return net.FileListener(os.NewFile(uintptr(fd), ""))
}

return listenTCP(ctx, addr)
}

Expand Down