Notify connection disconnect during error handling of new connection …

master/buildbot/test/unit/worker/test_protocols_base.py

@@ -43,3 +43,14 @@ def test_notify(self):
  self.assertEqual(cb.call_args_list, [])
  self.conn.notifyDisconnected()
  self.assertNotEqual(cb.call_args_list, [])
+
+ def test_notify_twice(self):
+ cb = mock.Mock()
+
+ self.conn.notifyOnDisconnect(cb)
+ self.assertEqual(cb.call_args_list, [])
+ self.conn.notifyDisconnected()
+ self.assertNotEqual(cb.call_args_list, [])
+ cb.call_args_list = []
+ self.conn.notifyDisconnected()
+ self.assertEqual(cb.call_args_list, [])

master/buildbot/worker/manager.py

@@ -132,6 +132,7 @@ def newConnection(self, conn, workerName):
  raise RuntimeError("rejecting duplicate worker")
  except defer.CancelledError:
  old_conn.loseConnection()
+ old_conn.notifyDisconnected()
  log.msg(
  f"Connected worker '{workerName}' ping timed out after {self.PING_TIMEOUT} "
  "seconds"
@@ -140,6 +141,7 @@ def newConnection(self, conn, workerName):
  raise
  except Exception as e:
  old_conn.loseConnection()
+ old_conn.notifyDisconnected()
  log.msg(f"Got error while trying to ping connected worker {workerName}:{e}")
  log.msg(f"Old connection for '{workerName}' was lost, accepting new")
 
@@ -155,6 +157,9 @@ def newConnection(self, conn, workerName):
  self.connections[workerName] = conn
 
  def remove():
+ assert (
+ self.connections.get(workerName, None) == conn
+ ), f"Attempt to remove non-connection entry for {workerName}"
  del self.connections[workerName]
 
  conn.notifyOnDisconnect(remove)

master/buildbot/worker/protocols/base.py

@@ -107,6 +107,8 @@ def notifyOnDisconnect(self, cb):
 
  def notifyDisconnected(self):
  self._disconnectSubs.deliver()
+ # Ensure that the disconnect notifications are not called twice
+ self._disconnectSubs = subscription.SubscriptionPoint(self._disconnectSubs.name)
 
  def loseConnection(self):
  raise NotImplementedError

master/buildbot/worker/protocols/pb.py

@@ -21,6 +21,7 @@
 
 from buildbot.pbutil import decode
 from buildbot.util import deferwaiter
+from buildbot.worker import AbstractWorker
 from buildbot.worker.protocols import base
 
 
@@ -146,6 +147,12 @@ def loseConnection(self):
  # keepalive handling
 
  def _do_keepalive(self):
+ if not self.worker or (
+ isinstance(self.worker, AbstractWorker) and self.worker.isConnected() != self
+ ):
+ self.loseConnection()
+ self.notifyDisconnected()
+ return defer.fail(None)
  return self.mind.callRemote('print', message="keepalive")
 
  def stopKeepaliveTimer(self):

newsfragments/really-disconnect-unattached-connections.bugfix

@@ -0,0 +1,3 @@
+Notify disconnect of connections that are being unattached when a replacement connection is being set up.
+Additionally, make sure the notification callbacks cannot be called twice, and that a disconnected connection
+cannot send a KeepAlive.