coder / coder/acp-go-sdk

Data race: Connection.SetLogger writes c.logger while receive() reads it

Open Beginner friendly
#58 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
230
Forks
37
PR merge metrics
No merged PRs in 30d

Description

`Connection.SetLogger` writes `c.logger` with no synchronization:

```go
// connection.go:105
func (c *Connection) SetLogger(l *slog.Logger) { c.logger = l }

// connection.go:107
func (c *Connection) loggerOrDefault() *slog.Logger {
if c.logger != nil {
return c.logger
}
return slog.Default()
}
```

`NewConnection` starts `receive()`, `sendCancelRequests()` and `processNotifications()` before returning (connection.go:97-99), and those goroutines read the field through `loggerOrDefault()`. Since `SetLogger` can only be called *after* the constructor returns, the documented way to install a logger always races with a connection that is already running — any diagnostic logged in that window (a parse error, "connection closed", …) is the read side.

### Reproduction

```go
func TestSetLoggerRace(t *testing.T) {
logger := slog.New(slog.NewTextHandler(io.Discard, nil))
for i := 0; i < 200; i++ {
_, c2aW := io.Pipe()
a2cR, a2cW := io.Pipe()
conn := acp.NewClientSideConnection(&testClient{}, c2aW, a2cR)
go func() {
_, _ = a2cW.Write([]byte("not json\n")) // makes receive() log a parse error
_ = a2cW.Close()
}()
conn.SetLogger(logger)
_ = c2aW.Close()
}
}
```

`go test -race` reports it on the first iteration:

```
WARNING: DATA RACE
Read at 0x00c00042c3a0 by goroutine 26:
acp.(*Connection).loggerOrDefault() connection.go:108
acp.(*Connection).receive() connection.go:364
acp.NewConnection.gowrap2() connection.go:98

Previous write at 0x00c00042c3a0 by goroutine 20:
acp.(*Connection).SetLogger() connection.go:105
acp.(*ClientSideConnection).SetLogger() client.go:27
```

Seen on v0.6.4-0.20260227160919-584abe6abe22; the same code is present on current main.

In our test suite this showed up as an intermittent `-race` failure under parallel load. We worked around it by not calling `SetLogger` at all and routing SDK diagnostics through `slog.Default()` instead, but that gives up per-connection loggers.

### Suggested fix

Store the logger in an `atomic.Pointer[slog.Logger]`:

```go
type Connection struct {
logger atomic.Pointer[slog.Logger]
// …
}

func (c *Connection) SetLogger(l *slog.Logger) { c.logger.Store(l) }

func (c *Connection) loggerOrDefault() *slog.Logger {
if l := c.logger.Load(); l != nil {
return l
}
return slog.Default()
}
```

An optional logger argument (or functional option) on `NewConnection` / `NewClientSideConnection` / `NewAgentSideConnection` would also close the window, and would let a logger be in place for anything the constructor's goroutines log first. Happy to send a PR for either shape.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in connection.go at Connection, SetLogger, loggerOrDefault, and the constructor goroutines identified in the issue; check the forwarding SetLogger methods in client.go. Run the supplied reproduction with go test -race. Done means concurrent logger updates no longer report a race, while per-connection diagnostics still use the configured logger.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend-api-design
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
84/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.