mirror of
https://github.com/coder/coder.git
synced 2026-10-16 14:48:56 +08:00
fix(tailnet): replace peer node instead of mutating KeepAlive (#30296)
Fixes a `test-go-race-pg` flake ([CLI-53](https://linear.app/codercom/issue/CLI-53)) exposed by the CLI SSH flight recorder (#29752). `configMaps.updatePeerLocked` mutated the stored peer node in place on `READY_FOR_HANDSHAKE` (`lc.node.KeepAlive = ...`) under `c.L`, but that node is shared outside `c.L`: it is attached to log entries as the `node` field, and `fillPeerDiagnostics` returns it as `ReceivedNode`. A flight recorder holds entries unformatted until flush, so when `rawSSHCopier.copy` logged `copy stdin error`, the flush formatted the node from another goroutine and raced the coordination loop. A stored node is now treated as immutable: `READY_FOR_HANDSHAKE` clones it, sets `KeepAlive` on the clone, and swaps it in (only when the value changes). `TestConfigMaps_updatePeers_new_waitForHandshake` now asserts the pre-handshake node still has `KeepAlive` false and the stored node is replaced. It fails against the in-place mutation. <details> <summary>Analysis and decision log</summary> Verified on main: - `configmaps.go:498` was the only in-place write to a stored `lc.node`. The writes at 452 and 473 touch the new node before it is stored. `peerConfigLocked` already clones nodes for the netmap. - slog v3.2.0 `flightRecorder.record` keeps `SinkEntry` fields by reference and formats on flush. `cli/ssh.go:1513` logs at Error, which auto-flushes from the copier goroutine without `c.L`. - `fillPeerDiagnostics` sets `d.ReceivedNode = lc.node`, which callers read after `c.L` is released: a second, latent alias of the same node. - `nodeKeepalive` reads only `node.Key`, and nothing relies on `lc.node` pointer identity (`Equal` compares values), so clone and swap is behavior-preserving. Approaches considered: 1. Snapshot fields inside slog: needs a coder/slog release and bump, and generic deep copy or eager rendering removes the recorder's cheap path. Not needed. 2. Log a `Clone()` of the node at 451 (first revision of this PR): fixes the logging alias only, and clones on every NODE update. 3. Clone and swap at 498 (suggested in review, chosen): fixes every alias, including diagnostics, and clones only on a `KeepAlive` change. The issue's proposed `OneShotStdio -race -count=100` regression test is probabilistic and slow, so the deterministic unit assertion replaces it. Verification: assertion red against the old code and green with the fix; `go test -race ./tailnet -run TestConfigMaps -count=3`; `go test -race ./cli -run '^TestSSH_CoderConnect$/^OneShotStdio$' -count=20`; `make pre-commit`. </details> --- <sub>Generated with Coder Agents.</sub>
This commit is contained in:
@@ -494,9 +494,15 @@ func (c *configMaps) updatePeerLocked(update *proto.CoordinateResponse_PeerUpdat
|
||||
lc.readyForHandshakeTimer.Stop()
|
||||
}
|
||||
if lc.node != nil {
|
||||
old := lc.node.KeepAlive
|
||||
lc.node.KeepAlive = c.nodeKeepalive(lc, status, lc.node)
|
||||
dirty = dirty || (old != lc.node.KeepAlive)
|
||||
// Replace rather than mutate: a stored node is shared outside c.L
|
||||
// (log entries in a flight recorder, peer diagnostics), so it must not
|
||||
// change after it is stored.
|
||||
if keepAlive := c.nodeKeepalive(lc, status, lc.node); keepAlive != lc.node.KeepAlive {
|
||||
next := lc.node.Clone()
|
||||
next.KeepAlive = keepAlive
|
||||
lc.node = next
|
||||
dirty = true
|
||||
}
|
||||
}
|
||||
logger.Debug(context.Background(), "peer ready for handshake")
|
||||
// only force a reconfig if the node populated
|
||||
|
||||
@@ -335,6 +335,11 @@ func TestConfigMaps_updatePeers_new_waitForHandshake(t *testing.T) {
|
||||
|
||||
// it should not send the peer to the netmap yet
|
||||
|
||||
uut.L.Lock()
|
||||
preHandshake := uut.peers[p1ID].node
|
||||
uut.L.Unlock()
|
||||
require.False(t, preHandshake.KeepAlive)
|
||||
|
||||
go func() {
|
||||
<-fEng.status
|
||||
fEng.statusDone <- struct{}{}
|
||||
@@ -359,6 +364,15 @@ func TestConfigMaps_updatePeers_new_waitForHandshake(t *testing.T) {
|
||||
require.Equal(t, p1Node.Endpoints, n1.Endpoints)
|
||||
require.True(t, n1.KeepAlive)
|
||||
|
||||
// The stored node is replaced, not mutated, because it may be shared
|
||||
// outside c.L (log entries, peer diagnostics).
|
||||
require.False(t, preHandshake.KeepAlive, "stored node must not be mutated in place")
|
||||
uut.L.Lock()
|
||||
postHandshake := uut.peers[p1ID].node
|
||||
uut.L.Unlock()
|
||||
require.NotSame(t, preHandshake, postHandshake)
|
||||
require.True(t, postHandshake.KeepAlive)
|
||||
|
||||
// we rely on nmcfg.WGCfg() to convert the netmap to wireguard config, so just
|
||||
// require the right number of peers.
|
||||
require.Len(t, r.wg.Peers, 1)
|
||||
|
||||
Reference in New Issue
Block a user