Skip to content

Commit 5e6dde0

Browse files
frahlgclaude
andauthored
fix(ocpp): a port Core cannot open fails Start and shows on each charger (#1843)
* fix(ocpp): a port Core cannot open fails Start and shows on each charger ws.Server.Start opens its own socket in a goroutine and drops a listen error into a logger Core does not read, so a taken port left the OCPP server not listening while Start reported success. Start now opens and closes each enabled port first, loads the TLS pair that ServeTLS would load later, and on any failure records the error on every charger the site names. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ocpp): a taken 2.0.1 port leaves the 1.6 listener running Review note: failing all of Start for a taken 2.0.1 port also stopped the 1.6 listener that came up before, leaving 1.6 chargers unsteered. Start now fails only for the 1.6 port. A taken 2.0.1 port is logged, that listener is skipped, and the reason goes on each named charger, which its first 1.6 message clears. The Start comment no longer says it returns once the listener is up, and startOCPP retries when a 2.0.1 listener it asked for did not start. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 53b5a87 commit 5e6dde0

6 files changed

Lines changed: 132 additions & 14 deletions

File tree

‎.changeset/ocpp-port-taken.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"ftw": patch
3+
---
4+
5+
If Core cannot open its OCPP port, because another program holds it or the TLS certificate and key do not load, the OCPP server now fails to start with that error instead of saying it started. If only the OCPP 2.0.1 port is taken, the 1.6 server still starts for the chargers that use it. The error appears in the log and in the help report, and each charger the site names shows it as its last error until it connects, so a charger that never connects has a reason beside it.

‎go/internal/ocpp/auth_test.go‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -238,17 +238,28 @@ func TestTLSMisconfigurationRefusesToStart(t *testing.T) {
238238
{name: "cert without key", tls: &TLSConfig{CertFile: "cert.pem"}},
239239
{name: "key without cert", tls: &TLSConfig{KeyFile: "key.pem"}},
240240
{name: "cert file missing", tls: &TLSConfig{CertFile: "no-such-cert.pem", KeyFile: "no-such-key.pem"}},
241+
{name: "cert does not load", tls: &TLSConfig{CertFile: unreadablePEM(t, "cert.pem"), KeyFile: unreadablePEM(t, "key.pem")}},
241242
}
242243
for _, tc := range tests {
243244
t.Run(tc.name, func(t *testing.T) {
244245
cfg := &Config{Enabled: true, Bind: "127.0.0.1", Port: freePort(t), TLS: tc.tls}
245246
if _, err := Start(context.Background(), cfg, telemetry.NewStore()); err == nil {
246-
t.Fatal("a broken TLS config started anyway, serving plaintext")
247+
t.Fatal("a broken TLS config started anyway")
247248
}
248249
})
249250
}
250251
}
251252

253+
// unreadablePEM writes a file that exists but holds no PEM block.
254+
func unreadablePEM(t *testing.T, name string) string {
255+
t.Helper()
256+
path := filepath.Join(t.TempDir(), name)
257+
if err := os.WriteFile(path, []byte("not a certificate"), 0o600); err != nil {
258+
t.Fatal(err)
259+
}
260+
return path
261+
}
262+
252263
func TestSchemeFollowsTLS(t *testing.T) {
253264
if got := (&Config{}).Scheme(); got != "ws" {
254265
t.Errorf("plaintext scheme: got %q, want ws", got)

‎go/internal/ocpp/charger_socket_test.go‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -156,9 +156,11 @@ func dialChargingStation201(t *testing.T, url, id string, setup func(ocpp201.Cha
156156
// after the chargers a test dials later.
157157
//
158158
// freePort hands back a port the kernel just released, and another socket can
159-
// take it before the library binds it. The library then gives up on that
160-
// listener without an error, so a listener goroutine that exits early means
161-
// the port was lost: try fresh ones.
159+
// take it before Start checks it, or between that check and the library's
160+
// bind. In the first case Start fails, or for 2.0.1 starts without that
161+
// listener; in the second the library gives up on the listener without an
162+
// error, so a listener goroutine that exits early means the port was lost.
163+
// Either way, try fresh ones.
162164
func startOCPP(t *testing.T, cfg Config, tel *telemetry.Store, v201 bool) *Server {
163165
t.Helper()
164166
const attempts = 5
@@ -169,10 +171,14 @@ func startOCPP(t *testing.T, cfg Config, tel *telemetry.Store, v201 bool) *Serve
169171
c.PortV201 = freePort(t)
170172
}
171173
srv, err := Start(context.Background(), &c, tel)
174+
var taken *net.OpError
175+
if errors.As(err, &taken) && attempt < attempts {
176+
continue // another test took the port after freePort gave it back
177+
}
172178
if err != nil {
173179
t.Fatalf("start: %v", err)
174180
}
175-
if listening(t, srv) {
181+
if (!v201 || srv.csms != nil) && listening(t, srv) {
176182
t.Cleanup(srv.Stop)
177183
return srv
178184
}

‎go/internal/ocpp/config.go‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -94,11 +94,10 @@ func (t *TLSConfig) serverTLS() (*tls.Config, error) {
9494
if !t.Enabled() {
9595
return nil, errors.New("ocpp: tls needs both cert_file and key_file")
9696
}
97-
if _, err := os.Stat(t.CertFile); err != nil {
98-
return nil, fmt.Errorf("ocpp: tls cert_file: %w", err)
99-
}
100-
if _, err := os.Stat(t.KeyFile); err != nil {
101-
return nil, fmt.Errorf("ocpp: tls key_file: %w", err)
97+
// ServeTLS loads the pair again inside the listener goroutine, where a
98+
// failure stops the listener with no error anyone sees.
99+
if _, err := tls.LoadX509KeyPair(t.CertFile, t.KeyFile); err != nil {
100+
return nil, fmt.Errorf("ocpp: tls cert_file and key_file: %w", err)
102101
}
103102
cfg := &tls.Config{MinVersion: tls.VersionTLS12}
104103
if t.ClientCAFile == "" {

‎go/internal/ocpp/server.go‎

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ import (
3636
"errors"
3737
"fmt"
3838
"log/slog"
39+
"net"
3940
"net/http"
4041
"sync"
4142
"time"
@@ -66,20 +67,47 @@ type Server struct {
6667
stopOnce sync.Once
6768
}
6869

69-
// Start brings up the OCPP CS on the configured bind:port. Returns
70-
// immediately once the listener is up; the WebSocket loop runs in its own
70+
// Start brings up the OCPP CS on the configured bind:port. It returns once
71+
// the ports have been found free; each WebSocket loop runs in its own
7172
// goroutine until ctx is cancelled or Stop() is called.
7273
//
74+
// An error means no charger can connect. A 2.0.1 port that cannot be opened
75+
// is logged instead, and the 1.6 listener still runs for the chargers that
76+
// dial it. Either way the reason is recorded as the last error of each
77+
// charger the site names, where its device status is read; a charger's first
78+
// message on the 1.6 listener clears it.
79+
//
7380
// The returned Server is the handle for shutdown — main.go is expected to
7481
// call Stop() during graceful drain.
75-
func Start(ctx context.Context, cfg *Config, tel *telemetry.Store) (*Server, error) {
82+
func Start(ctx context.Context, cfg *Config, tel *telemetry.Store) (_ *Server, err error) {
7683
if cfg == nil {
7784
return nil, errors.New("ocpp: nil config")
7885
}
7986
if tel == nil {
8087
return nil, errors.New("ocpp: nil telemetry store")
8188
}
8289
cfg.Defaults()
90+
unreachable := func(err error) {
91+
for _, id := range cfg.ApprovedIDs {
92+
tel.RecordDriverError(id, err.Error())
93+
}
94+
}
95+
defer func() {
96+
if err != nil {
97+
unreachable(err)
98+
}
99+
}()
100+
if err := checkPort(cfg.Port); err != nil {
101+
return nil, err
102+
}
103+
v201 := cfg.PortV201 > 0
104+
if v201 {
105+
if err := checkPort(cfg.PortV201); err != nil {
106+
slog.Error("ocpp: 2.0.1 listener not started; chargers that dial it cannot connect", "port", cfg.PortV201, "err", err)
107+
unreachable(err)
108+
v201 = false
109+
}
110+
}
83111

84112
auth := newAuthorizer(cfg)
85113
sockets := newSocketSessions()
@@ -107,7 +135,7 @@ func Start(ctx context.Context, cfg *Config, tel *telemetry.Store) (*Server, err
107135

108136
// OCPP 2.0.1 on its own port, when configured. Same handler and therefore
109137
// the same charger state and telemetry — only the message encoding differs.
110-
if cfg.PortV201 > 0 {
138+
if v201 {
111139
wsServer201, err := newListener(cfg, auth, sockets)
112140
if err != nil {
113141
return nil, err
@@ -184,6 +212,18 @@ func Start(ctx context.Context, cfg *Config, tel *telemetry.Store) (*Server, err
184212
return s, nil
185213
}
186214

215+
// checkPort opens and closes port on every interface, as ws.Server.Start
216+
// does. That Start runs in a goroutine and, when its own listen fails,
217+
// returns with the error sent to a logger nobody reads, so a taken port has
218+
// to be found here or the chargers stay away with no reason given.
219+
func checkPort(port int) error {
220+
l, err := net.Listen("tcp", fmt.Sprintf(":%d", port))
221+
if err != nil {
222+
return fmt.Errorf("ocpp: %w", err)
223+
}
224+
return l.Close()
225+
}
226+
187227
// newListener builds one version's WebSocket server — TLS when configured —
188228
// with both authorization gates wired.
189229
//

‎go/internal/ocpp/server_test.go‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
11
package ocpp
22

33
import (
4+
"context"
5+
"fmt"
46
"net"
7+
"strings"
58
"sync"
69
"testing"
710
"time"
@@ -35,6 +38,60 @@ func startServer(t *testing.T, tel *telemetry.Store, approved ...string) (int, *
3538
return srv.cfg.Port, srv
3639
}
3740

41+
// A port another process holds fails Start, and each charger the site names
42+
// carries the reason in its health. The library gives up on the port inside
43+
// a goroutine and logs nowhere we read, so without this the chargers stay
44+
// away and nothing says why.
45+
func TestStartFailsWhenItsPortIsTaken(t *testing.T) {
46+
taken := holdPort(t)
47+
tel := telemetry.NewStore()
48+
cfg := &Config{Enabled: true, Bind: "127.0.0.1", Port: taken, ApprovedIDs: []string{"garage"}}
49+
if _, err := Start(context.Background(), cfg, tel); err == nil {
50+
t.Fatalf("Start succeeded with port %d taken", taken)
51+
} else if !strings.Contains(err.Error(), fmt.Sprint(taken)) {
52+
t.Errorf("error %q does not name port %d", err, taken)
53+
}
54+
if h := tel.DriverHealth("garage"); h == nil || !strings.Contains(h.LastError, fmt.Sprint(taken)) {
55+
t.Fatalf("the charger's health does not say why it cannot connect: %+v", h)
56+
}
57+
}
58+
59+
// A taken 2.0.1 port leaves the 1.6 listener running, so the chargers that
60+
// dial 1.6 are still steered. The reason stays on each charger that has not
61+
// connected.
62+
func TestATaken201PortLeavesThe16ListenerRunning(t *testing.T) {
63+
taken := holdPort(t)
64+
tel := telemetry.NewStore()
65+
srv := startOCPP(t, Config{Enabled: true, Bind: "127.0.0.1", PortV201: taken, HeartbeatIntervalS: 60, ApprovedIDs: []string{"garage", "carport"}}, tel, false)
66+
if srv.csms != nil {
67+
t.Fatalf("a 2.0.1 listener started on taken port %d", taken)
68+
}
69+
cp, _, err := dialChargePoint16(t, wsURL(srv.cfg.Port), "garage", nil, nil)
70+
if err != nil {
71+
t.Fatalf("a 1.6 charger could not connect: %v", err)
72+
}
73+
if _, err := cp.BootNotification("Home", "Easee"); err != nil {
74+
t.Fatal(err)
75+
}
76+
if h := tel.DriverHealth("garage"); h == nil || h.LastError != "" {
77+
t.Errorf("the connected 1.6 charger still shows an error: %+v", h)
78+
}
79+
if h := tel.DriverHealth("carport"); h == nil || !strings.Contains(h.LastError, fmt.Sprint(taken)) {
80+
t.Errorf("the charger that has not connected does not say why: %+v", h)
81+
}
82+
}
83+
84+
// holdPort takes a port on every interface until the test ends.
85+
func holdPort(t *testing.T) int {
86+
t.Helper()
87+
l, err := net.Listen("tcp", ":0")
88+
if err != nil {
89+
t.Fatal(err)
90+
}
91+
t.Cleanup(func() { l.Close() })
92+
return l.Addr().(*net.TCPAddr).Port
93+
}
94+
3895
func TestStopIsConcurrentAndIdempotent(t *testing.T) {
3996
_, srv := startServer(t, telemetry.NewStore())
4097

0 commit comments

Comments
 (0)