[frahlg]

Found on 25 October 2026 while fixing flaky OCPP tests (#1836, "Not fixed here").

Problem

ocpp-go v0.19.0's WebSocket client races with itself on close. #1836 lists the three ways, each seen in CI: a reply sent while the charger stops can send on a closed channel and panic; ocppj.Client.Stop swaps the disconnect handler while the write pump reads it; and Stop can wait forever for a disconnect the pump has already reported. #1836 wrapped the client as chargerSocket, but only inside internal/ocpp's tests. Three other places still dialed with the plain client:

• cmd/sim-ocpp/ocppcp/sim.go, the OCPP simulator behind make sim-ocpp and test/e2e/ocpp_evify_test.go, for both 1.6 and 2.0.1;
• cmd/ftw/ev_send_test.go and cmd/ftw/ev_ocpp_reconnect_test.go.

Core's own paths

Core is the OCPP server and never dials. go list -deps ./cmd/ftw does not contain the new package, before or after, and no non-test file in cmd/ftw or internal/ builds a client. There is no self-test that dials. The Core binary does not change.

Fix

• chargerSocket moves unchanged, apart from its name and one error text, from internal/ocpp/charger_socket_test.go to a package of its own, internal/ocpp/chargersocket, as Socket. It adds ChargePoint16(id, client) and ChargingStation201(id, client), which build the charge point on a Socket.
• The simulator, the two cmd/ftw tests and internal/ocpp's dial helpers use them.
• TestEveryChargerDialsThroughASocket parses every Go file in the module and fails if one uses ocpp-go's NewChargePoint (1.6) or NewChargingStation (2.0.1) outside the package, under any import name or a dot import. It skips what the go tool skips and the nested modules under go/. The races need a few instructions of bad luck, so a charger built on the plain client passes its own tests and fails CI weeks later. This test finds it the day it is written. It does not look for a charger built one level lower, on ocppj.NewClient or a bare ws.Client; nothing in the module does that.

Evidence

The races themselves cannot be made to happen on demand: #1836 measured the data race at about 1 in 3,000 runs. The new guard is the test that fails first. On master, with the package added:

--- FAIL: TestEveryChargerDialsThroughASocket (0.30s)
    socket_test.go:99: chargers built on ocpp-go's plain client, use ChargePoint16 or ChargingStation201:
        cmd/ftw/ev_ocpp_reconnect_test.go:59
        cmd/ftw/ev_send_test.go:163
        cmd/sim-ocpp/ocppcp/sim.go:90
        cmd/sim-ocpp/ocppcp/sim.go:103
        internal/ocpp/charger_socket_test.go:129
        internal/ocpp/charger_socket_test.go:143

The last two already used the wrapper, through the old local type. On this branch the guard passes. TestSocketPassesAMissedDisconnectToStop moves with the type and still covers the missed-disconnect path.

• go test -race ./internal/ocpp/... ./cmd/sim-ocpp/... passes.
• go test -race -count=2 -run 'OCPP|EVSend|Ocpp|Charger' ./cmd/ftw/ passes.
• FTW_E2E=1 go test -race -count=2 -run Evify ./test/e2e/ passes (without FTW_E2E=1 these tests skip).
• go vet ./... passes.
• go test ./internal/... ./cmd/ftw/... passes.

Not changed: the server's StopConnection

ocpp-go's ws.Server.StopConnection reads the socket under its lock, lets go, and then sends on the socket's close channel. If the socket cleans itself up in between, the send panics. Core calls it in one place, guardedServer.SetNewClientHandler (socket_sessions.go:166), to refuse a socket whose handshake reservation has gone. That happens when the request's context ended during the handshake, which in practice means the charger hung up while its socket was opening. The library's read pump then sees that close and cleans up at the same moment.

It is reachable. The panic runs on the HTTP handler's goroutine, and net/http recovers it, so Core keeps running. But Core holds that charger's slot.mu across the call and unlocks without defer, so the lock stays held, and every later handshake for that charger id waits on it. The charger cannot connect again until Core restarts. Any peer that can reach the port can try this for a known id when basic auth is off.

How likely it is:

• The review's probe sent 6,000 upgrade requests for one id and closed each at once, under -race, with a println inside the window. In one of two runs the send panicked, and 3,372 handshake goroutines were then stuck on slot.mu (socket_sessions.go:88).
• My probe without the println (16 × 400 requests, mixed RST and FIN) reached line 166 3 to 17 times per run and locked the slot in 0 of 30 runs.

So the mechanism is confirmed, but the window is a few instructions wide, and no test can make it fail on demand: the branch needs the request context to end between two library steps, and StopConnection is a method on the library's concrete type, with no seam for a test. I have not changed it, because the fix would have no test that fails first.

My first suggestion here, to unlock slot.mu before the call, is wrong: StopConnection looks up the socket by charger id, and the lock is what keeps a newer socket for that id out of the table meanwhile. Without it, Core could close a healthy new socket. A fix that keeps the lock: recover the library's panic around that one call, or move the unlock into a defer. The library fix is to send under its lock. Both are left for a decision.

Also not changed

• The simulator can still panic in Socket.Write if Core closes the socket while a write is queued; it cannot stop its chargers before Core restarts under it. Tests stop chargers first. Not seen; no test can make it happen on demand.
• Stop during a reconnect still races inside ocpp-go (ws.Client.Start against Stop on reconnectC). With the default 10–25 s backoff it is unlikely in tests; it is on master too.
• Socket inherits StartWithRetries from ws.Client, which skips Socket.Start. Nothing calls it.

Review

An independent review (Sonnet) found the move clean: the code matches master apart from the name, one error text and the nil-client default, basic auth carries over, 60 reconnect rounds never hung Stop, and cmd/ftw does not link the package. Its findings:

• The e2e line in the first version of this description ran nothing: the tests skip without FTW_E2E=1. Now run with it, above.
• The guard named an unaliased import after its path (ocpp1.6), but the package is ocpp16 (and ocpp2), and it missed dot imports; it also failed on any file that did not parse and walked into the nested modules. Fixed in the second commit. With temporary files, it now reports an unaliased and a dot import and ignores a _ directory and a file that does not parse.
• The StopConnection risk is higher than the first version said, and the suggested unlock was unsafe. Rewritten above.
• The Write, reconnect and StartWithRetries notes, listed above as not changed.

No changeset: this PR changes tests and a simulator that is not in any release (no-changeset label).

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-25T13:21:21.829550Z	902c8ca	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.