From 0f65fd960f937711353465e4a7bfadd871bb5c9c Mon Sep 17 00:00:00 2001
From: andig <andi@evcc.io>
Date: Tue, 14 Apr 2026 19:14:03 +0200
Subject: [PATCH] EEBus: fix deadlock on save after validate (#29119)

---
 server/eebus/eebus.go      | 18 ++++++++-----
 server/eebus/eebus_test.go | 52 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 7 deletions(-)

diff --git a/server/eebus/eebus.go b/server/eebus/eebus.go
index 8cf5696a2..38a66dfc0 100644
--- a/server/eebus/eebus.go
+++ b/server/eebus/eebus.go
@@ -237,17 +237,21 @@ func (c *EEBus) UnregisterDevice(ski string, device Device) {
 	c.log.TRACE.Printf("unregistering ski: %s", ski)
 
 	c.mux.Lock()
-	defer c.mux.Unlock()
-
 	if idx := slices.Index(c.clients[ski], device); idx != -1 {
 		c.clients[ski] = slices.Delete(c.clients[ski], idx, idx+1)
-	}
 
-	// only tear down SHIP connection when no more clients need it
-	if len(c.clients[ski]) == 0 {
-		delete(c.clients, ski)
-		c.service.UnregisterRemoteSKI(ski)
+		if len(c.clients[ski]) == 0 {
+			delete(c.clients, ski)
+
+			// Tear down the SHIP session after releasing the mutex: ship-go's CloseConnection
+			// on a non-Complete state synchronously invokes HandleConnectionClosed,
+			// which calls back into evcc's connect(ski, false) — and that needs to
+			// acquire c.mux. Holding c.mux across this cross-layer call would
+			// deadlock the same goroutine on its own non-reentrant mutex. See #28942.
+			defer c.service.UnregisterRemoteSKI(ski)
+		}
 	}
+	c.mux.Unlock()
 }
 
 func (c *EEBus) CustomerEnergyManagement() *CustomerEnergyManagement {
diff --git a/server/eebus/eebus_test.go b/server/eebus/eebus_test.go
index 12ae581e1..c9d3a3957 100644
--- a/server/eebus/eebus_test.go
+++ b/server/eebus/eebus_test.go
@@ -2,7 +2,12 @@ package eebus
 
 import (
 	"testing"
+	"time"
 
+	eebusapi "github.com/enbility/eebus-go/api"
+	eebusmocks "github.com/enbility/eebus-go/mocks"
+	spineapi "github.com/enbility/spine-go/api"
+	"github.com/evcc-io/evcc/util"
 	"github.com/stretchr/testify/require"
 	"go.yaml.in/yaml/v4"
 )
@@ -23,3 +28,50 @@ certificate:
 	var res Config
 	require.NoError(t, yaml.Unmarshal([]byte(conf), &res))
 }
+
+// mockDevice implements Device for testing
+type mockDevice struct{}
+
+func (d *mockDevice) Connect(connected bool) {}
+func (d *mockDevice) UseCaseEvent(_ spineapi.DeviceRemoteInterface, entity spineapi.EntityRemoteInterface, event eebusapi.EventType) {
+}
+
+var _ Device = (*mockDevice)(nil)
+
+// TestUnregisterDevice_MutexNotHeldDuringShipCall is the regression guard
+// for issue #28942. It asserts that c.mux is NOT held at the point
+// UnregisterRemoteSKI is called. The pre-fix code held c.mux across that
+// cross-layer call, and ship-go's synchronous HandleConnectionClosed
+// callback chain re-entered connect(ski, false) on the same goroutine,
+// which then deadlocked on c.mux.Lock() (Go mutexes are non-reentrant).
+//
+// The assertion uses a goroutine that tries to briefly acquire c.mux from
+// inside the mock's UnregisterRemoteSKI implementation; if the lock is
+// held, the acquisition times out and the test fails.
+func TestUnregisterDevice_MutexNotHeldDuringShipCall(t *testing.T) {
+	dev := &mockDevice{}
+	c := &EEBus{
+		log:     util.NewLogger("test"),
+		clients: map[string][]Device{"aabbcc": {dev}},
+	}
+
+	service := eebusmocks.NewServiceInterface(t)
+	service.EXPECT().UnregisterRemoteSKI("aabbcc").Run(func(string) {
+		acquired := make(chan struct{})
+		go func() {
+			c.mux.Lock()
+			defer c.mux.Unlock()
+			close(acquired)
+		}()
+		select {
+		case <-acquired:
+			// good — mutex was free
+		case <-time.After(100 * time.Millisecond):
+			t.Errorf("c.mux was held while UnregisterRemoteSKI was called — " +
+				"regression to the cross-layer lock hold that caused #28942")
+		}
+	}).Once()
+	c.service = service
+
+	c.UnregisterDevice("aabbcc", dev)
+}