Conversation
Signed-off-by: David Gageot <david.gageot@docker.com>
0ba4334 to
cbd7610
Compare
There was a problem hiding this comment.
Code Review Summary
Found 3 issues in the changed code that should be addressed:
- Path construction portability issue (line 71) - Using string concatenation instead of
filepath.Join - Potential race condition (line 91) - Server goroutine lacks synchronization
- Silent error handling (line 91) - Server errors are ignored in goroutine
These issues could affect test reliability and cross-platform compatibility.
|
|
||
| tmpDir := t.TempDir() | ||
| t.Chdir(tmpDir) | ||
| t.Chdir(tmpDir) // Use relative socket path to avoid Unix socket path length limit |
There was a problem hiding this comment.
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:
- The filepath package is already imported and used correctly elsewhere in the same file (line 40)
- This creates inconsistency within the same file
- 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.
|
|
||
| srv, err := server.New(t.Context(), sessionStore, &config.RuntimeConfig{}, 0, nil) | ||
| require.NoError(t, err) | ||
|
|
There was a problem hiding this comment.
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
| srv, err := server.New(t.Context(), sessionStore, &config.RuntimeConfig{}, 0, nil) | ||
| require.NoError(t, err) | ||
|
|
||
| go func() { |
There was a problem hiding this comment.
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:
- Test output visibility: Errors may not be visible in test output depending on log configuration
- Race condition: No way to detect if server failed to start before the test proceeds
- Error attribution: Test failures will show indirect symptoms rather than root causes
- 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.
No description provided.