Flaky pkg/server tests: port reused between test servers, requests can hit the wrong instance
- Dominant language
- Go
- Stars
- 1
- Forks
- 0
- Avg merge
- 2h 20m
- Merged PRs (30d)
- 78
Description
`pkg/server` tests fail intermittently on unrelated changes. The clearest case is `TestRunQuery_DTQL_RunsForAdmin`, which failed and then passed on the **same commit** with no change.
## Evidence
| Run | Ref | Failed test |
|---|---|---|
| [34350984261](https://github.com/datatug/datatug-cli/actions/runs/34350984261) | `main` @ `337eaf1a` | `TestRunQuery_DTQL_RunsForAdmin` |
| [34356039231](https://github.com/datatug/datatug-cli/actions/runs/34356039231) | PR #208 @ `33f0b58b` | `TestRunQuery_DTQL_RunsForAdmin` |
| [34354476734](https://github.com/datatug/datatug-cli/actions/runs/34354476734) | `main` @ `4327903c` | `TestServeHTTP_CORS_DatatugApp`, `TestServeHTTP_CreateProject_AuthGate` |
PR #208 changed **only `.gitignore`**. Re-running the identical commit turned it green with no edit, which is what makes this a flake rather than a real failure. `main` was green two minutes before that run, on the same base.
Failure output:
```
validation error: invalid request: bad value for field [project]: missing required field
--- FAIL: TestRunQuery_DTQL_RunsForAdmin (5.04s)
```
That error is the interesting part. The test *does* set `ProjectID`, so the server that answered did not recognise it.
## Probable mechanism
This is a hypothesis, but it explains the exact error rather than just "flaky".
`freeTCPPort` in `pkg/server/http_server_test.go:84` picks a port by binding `127.0.0.1:0`, reading the assigned port, then **closing the listener** and returning the bare number:
```go
l, err := net.Listen("tcp", "127.0.0.1:0")
port := l.Addr().(*net.TCPAddr).Port
if err := l.Close(); err != nil { ... }
return port
```
`startServeHTTPWithSession` (`pkg/server/http_server_test.go:143`) then starts the real server on that number. Between the close and the bind there is a window where the port is unowned.
The window matters more than usual here because every such test shuts its server down gracefully in `t.Cleanup`, and graceful shutdown only completes once tracked connections drain. So a previous test's server can still be listening on a port the OS has already handed back out.
`waitForServer` (line 117) cannot tell the difference. It polls `/datatug/ping` and accepts **any** 200 `pong`. A leftover server from an earlier test answers that ping happily. The test then posts `/datatug/exec/run_query` to a server wired with a *different* project set, which rejects the unknown project exactly as observed.
That also fits the other two failures: they are different tests in the same package using the same helper, failing the same way for the same reason.
## Suggested fix
Never let go of the port. Bind once and hand the listener to the server, rather than passing a port number and rebinding:
```go
l, _ := net.Listen("tcp", "127.0.0.1:0")
go srv.Serve(l) // or ServeHTTP taking a net.Listener
```
That closes the window entirely and needs no retry logic.
If `ServeHTTP`'s signature should stay port-based for production, a test-only entry point taking a `net.Listener` would do. A cheaper partial mitigation is to make `waitForServer` assert an identity unique to *this* server instance instead of accepting any `pong`, which would turn a silent wrong-server hit into a clear failure. That detects the collision rather than preventing it, so it is worth doing in addition, not instead.
## Why it is worth fixing now
`main` is protected and requires `strongo_workflow / Build & test`, so a flake here blocks merges until someone notices and re-runs. Two of the three failures above were on `main` itself.
---
Filed by Claude Code at the maintainer's request, from evidence gathered while landing unrelated CI changes. I have not attempted the fix.
Contributor guide
Research direction
Start with pkg/server/http_server_test.go, especially freeTCPPort, waitForServer, and startServeHTTPWithSession, then trace the ServeHTTP entry point and run the named flaky tests. The work is done when each test keeps its listener associated with its server instance, requests cannot reach a leftover server, and the affected pkg/server tests pass repeatedly.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend, testing
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100