From c7b6147e748f79b7ecca04f0bb7a31153bedae65 Mon Sep 17 00:00:00 2001 From: Markus Hilger Date: Fri, 14 Aug 2026 14:07:49 +0200 Subject: [PATCH 1/8] Report why an ipmi session could not be established A session that failed raised with no message at all, so a failed console read "IpmiException: None". Record the reason wherever a session is marked broken. --- confluent_server/aiohmi/ipmi/private/session.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/confluent_server/aiohmi/ipmi/private/session.py b/confluent_server/aiohmi/ipmi/private/session.py index 2bbb628e..797bf5b8 100644 --- a/confluent_server/aiohmi/ipmi/private/session.py +++ b/confluent_server/aiohmi/ipmi/private/session.py @@ -550,7 +550,9 @@ class Session(object): while self.logging and not self.broken: await Session.wait_for_rsp() if self.broken: - raise exc.IpmiException(self.errormsg) + # Never leave the caller with an exception carrying no reason at all + raise exc.IpmiException( + self.errormsg or 'Unable to establish a session with the bmc') async def _mark_broken(self, error=None): # since our connection has failed retries @@ -781,7 +783,7 @@ class Session(object): waiter({'success': True}) await self.process_pktqueue() if _monotonic_time() > alltimeout: - await self._mark_broken() + await self._mark_broken('Session no longer connected') raise exc.IpmiException('Session no longer connected') await WAITING_SESSIONS.acquire() try: @@ -815,7 +817,7 @@ class Session(object): if not self.logged: if (self.logoutexpiry is not None and _monotonic_time() > self.logoutexpiry): - await self._mark_broken() + await self._mark_broken('Session no longer connected') raise exc.IpmiException('Session no longer connected') await self.atomicop.acquire() try: @@ -1372,7 +1374,7 @@ class Session(object): else: await self.logout() except exc.IpmiException: - await self._mark_broken() + await self._mark_broken('Session keepalive failed') async def process_pktqueue(self): while self.pktqueue: @@ -1810,7 +1812,7 @@ class Session(object): if self.ipmicallback: await self.ipmicallback(response) self.nowait = False - await self._mark_broken() + await self._mark_broken('timeout') return else: self.maxtimeout = 2 @@ -1847,7 +1849,7 @@ class Session(object): if self.ipmicallback: await self.ipmicallback(response) self.nowait = False - await self._mark_broken() + await self._mark_broken('timeout') return else: # in IPMI case, the only recourse is to act as if the packet is # idempotent. SOL has more sophisticated retry handling From ef608005cf8791043a2e713f484e3f087fe57f7d Mon Sep 17 00:00:00 2001 From: Markus Hilger Date: Fri, 14 Aug 2026 14:07:49 +0200 Subject: [PATCH 2/8] Register an ipmi session before establishing it, not after initting_sessions exists so a caller can share a session already on its way, but the entry was added only once the login had finished, leaving the login itself uncovered. Two callers asking at once each built a session, both on the socket the other had not claimed yet, and replies route by bmc address and local port, so only the last to transmit was ever answered. That is the console session failing about one attempt in three. Register before the login and remove the entry in a finally. The two old removals keyed on the encoded credentials while the register is keyed on the caller's strings, so they never matched and an entry outlived its session. A session handed over mid login is now waited for rather than returned as one that answers as though it had been lost. --- .../aiohmi/ipmi/private/session.py | 54 +++++++++++-------- 1 file changed, 33 insertions(+), 21 deletions(-) diff --git a/confluent_server/aiohmi/ipmi/private/session.py b/confluent_server/aiohmi/ipmi/private/session.py index 797bf5b8..d273f046 100644 --- a/confluent_server/aiohmi/ipmi/private/session.py +++ b/confluent_server/aiohmi/ipmi/private/session.py @@ -422,6 +422,24 @@ class Session(object): KEEPALIVE_SESSIONS.release() return not session.broken + @classmethod + async def _await_login(cls, session): + """Wait out a login that another caller has already started + + A session handed out mid login is not usable yet, and the caller + receiving it cannot tell, so wait and give it the same answer as the + caller that built it. + """ + while not (session.logged or session.broken): + # A ceiling, not a wait: with nothing registered to wait for, + # wait_for_rsp returns at once. Traffic still ends it early. + await cls.wait_for_rsp(1) + if session.broken: + raise exc.IpmiException( + getattr(session, 'errormsg', None) + or 'Unable to establish a session with the bmc') + return session + async def __new__(cls, bmc, userid, @@ -432,6 +450,7 @@ class Session(object): keepalive=True): trueself = None forbidsock = [] + sesskey = (bmc, userid, password, port, kg) for res in socket.getaddrinfo(bmc, port, 0, socket.SOCK_DGRAM): sockaddr = res[4] if ipv6support and res[0] == socket.AF_INET: @@ -460,18 +479,23 @@ class Session(object): # id, however it's easier this way forbidsock.append(self.socket) if trueself: - return trueself - i = cls.initting_sessions.get( - (bmc, userid, password, port, kg), False) + return await cls._await_login(trueself) + i = cls.initting_sessions.get(sesskey, False) if i: - i.initialized = True - i.logging = True - return i + return await cls._await_login(i) self = super().__new__(cls) self.forbidsock = forbidsock - await self.__init__( - bmc, userid, password, port, kg, privlevel, keepalive) - cls.initting_sessions[(bmc, userid, password, port, kg)] = self + # Register before establishing, not after: this is where a caller + # asking for a session already on its way finds it, and leaving it + # to the end left the whole login uncovered + cls.initting_sessions[sesskey] = self + try: + await self.__init__( + bmc, userid, password, port, kg, privlevel, keepalive) + finally: + # The only removal, and it has to happen whether the login + # worked or not, or the entry outlives the session + cls.initting_sessions.pop(sesskey, None) return self async def __init__(self, @@ -569,12 +593,6 @@ class Session(object): Session.waiting_sessions.pop(self, None) finally: WAITING_SESSIONS.release() - try: - del Session.initting_sessions[(self.bmc, self.userid, - self.password, self.port, - self.kgo)] - except KeyError: - pass await self.logout(False) # self.logging = False self.errormsg = error @@ -1890,12 +1908,6 @@ class Session(object): Session.bmc_handlers[sockaddr] = {} Session.bmc_handlers[sockaddr][myport] = self _io_sendto(self.socket, self.netpacket, sockaddr) - try: - del Session.initting_sessions[(self.bmc, self.userid, - self.password, self.port, - self.kgo)] - except KeyError: - pass except socket.gaierror: raise exc.IpmiException( "Unable to transmit to specified address") From 9408c5663945ec6582a48b569fb88d0bcf6df11b Mon Sep 17 00:00:00 2001 From: Markus Hilger Date: Fri, 14 Aug 2026 14:07:49 +0200 Subject: [PATCH 3/8] Give back a socket pool count once, not twice logout decremented the count of sessions on a socket twice over, and _mark_broken again for the case logout had not, so it went negative and kept falling. _assignsocket picks the least used socket and refuses one at MAX_BMCS_PER_SOCKET, and both of those read that number. --- .../aiohmi/ipmi/private/session.py | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/confluent_server/aiohmi/ipmi/private/session.py b/confluent_server/aiohmi/ipmi/private/session.py index d273f046..23c42003 100644 --- a/confluent_server/aiohmi/ipmi/private/session.py +++ b/confluent_server/aiohmi/ipmi/private/session.py @@ -532,6 +532,9 @@ class Session(object): self.servermode = False self.initialized = True self.cleaningup = False + # Whether this session still holds the socket pool count it took. + # Set before anything can fail, so releasing is safe either way. + self._socketclaimed = False self.lastpayload = None self._customkeepalives = None # queue of events denoting line to run a cmd @@ -567,6 +570,7 @@ class Session(object): await self.socketchecking.acquire() try: self.socket = await self._assignsocket(forbiddensockets=self.forbidsock) + self._socketclaimed = True finally: self.socketchecking.release() await self.login() @@ -578,6 +582,18 @@ class Session(object): raise exc.IpmiException( self.errormsg or 'Unable to establish a session with the bmc') + def _release_socket(self): + """Give back the socket pool count this session took, exactly once + + More than one path ended a session and decremented, so the count fell + below zero. _assignsocket picks the least used socket and refuses one + at MAX_BMCS_PER_SOCKET, and both read this number. + """ + if not self._socketclaimed: + return + self._socketclaimed = False + self.socketpool[self.socket] -= 1 + async def _mark_broken(self, error=None): # since our connection has failed retries # deregister our keepalive facility @@ -598,8 +614,7 @@ class Session(object): self.errormsg = error if not self.broken: self.broken = True - if self.socket: - self.socketpool[self.socket] -= 1 + self._release_socket() while self.logonwaiters: waiter = self.logonwaiters.pop() try: @@ -1961,7 +1976,6 @@ class Session(object): {'error': 'Session Disconnected'}) self._customkeepalives = None if not self.broken: - self.socketpool[self.socket] -= 1 self.broken = True # since this session is broken, remove it from the handler list # This allows constructor to create a new, functional object to @@ -1974,7 +1988,7 @@ class Session(object): if Session.bmc_handlers[sockaddr] == {}: del Session.bmc_handlers[sockaddr] self.nowait = False - self.socketpool[self.socket] -= 1 + self._release_socket() return {'success': True} From ed21c7634ab5464f41d0ecdbbf3cc425d443a587 Mon Sep 17 00:00:00 2001 From: Markus Hilger Date: Fri, 14 Aug 2026 14:07:49 +0200 Subject: [PATCH 4/8] Drop a guard that never ran and would not have worked __init__ opened by checking for an initialized attribute, meaning it had been handed a session someone else was establishing. That attribute is only ever set further down in the same method, so the check cannot be true, and the port lost the return that made it work upstream. Waiting for someone else's login is done in __new__ now, so this is dead code claiming to protect something. --- confluent_server/aiohmi/ipmi/private/session.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/confluent_server/aiohmi/ipmi/private/session.py b/confluent_server/aiohmi/ipmi/private/session.py index 23c42003..5ce8e9e7 100644 --- a/confluent_server/aiohmi/ipmi/private/session.py +++ b/confluent_server/aiohmi/ipmi/private/session.py @@ -506,10 +506,6 @@ class Session(object): kg=None, privlevel=None, keepalive=True): - if hasattr(self, 'initialized'): - # new found an existing session, do not corrupt it - while self.logging and not self.broken: - await Session.wait_for_rsp() self.awaitingresponse = False self.lastresponse = None self.atomicop = asyncio.Lock() From 9a8fe206a46ec2a2a710a987cc82404fcf07abb4 Mon Sep 17 00:00:00 2001 From: Markus Hilger Date: Fri, 14 Aug 2026 14:07:49 +0200 Subject: [PATCH 5/8] Let a session serve several callers without one closing it One session is now routinely handed to a console and a command at once, and logout closed it for both, leaving whoever was left holding one that answered as though it had been lost. Count the holders and give up a claim instead, unless the session is no longer usable, which logout is told by sessionok. A console had no way to give a claim back: close deactivated its sol payload and left the session alone, which was right when closing meant closing it for everybody and is a leak now. Both of its exits release it. --- confluent_server/aiohmi/ipmi/console.py | 22 +++++++++++++++---- .../aiohmi/ipmi/private/session.py | 21 +++++++++++++++--- 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/confluent_server/aiohmi/ipmi/console.py b/confluent_server/aiohmi/ipmi/console.py index c68ca774..6f3abfeb 100644 --- a/confluent_server/aiohmi/ipmi/console.py +++ b/confluent_server/aiohmi/ipmi/console.py @@ -204,6 +204,21 @@ class Console(object): if not self.awaitingack: await self._sendpendingoutput() + async def _release_session(self): + """Give up this console's claim on the ipmi session + + A console can be sharing one with whatever else is talking to the same + bmc, so let go of it rather than closing it. Clearing the reference + first means it does not matter how many exits end up here. + """ + sess = self.ipmi_session + if sess is None: + return + self.ipmi_session = None + if sess.sol_handler is not None and sess.sol_handler.__self__ is self: + sess.sol_handler = None + await sess.logout() + async def close(self): """Shut down an SOL session""" @@ -217,6 +232,7 @@ class Console(object): # if underlying ipmi session is not working, then # run with the implicit success pass + await self._release_session() async def send_data(self, data): if self.broken: @@ -320,10 +336,8 @@ class Console(object): self.broken = True if self.ipmi_session: self.ipmi_session.unregister_keepalive(self.keepaliveid) - if (self.ipmi_session.sol_handler - and self.ipmi_session.sol_handler.__self__ is self): - self.ipmi_session.sol_handler = None - self.ipmi_session = None + # A console that has given up is finished with the session too + await self._release_session() if type(error) == dict: await self._print_data(error) else: diff --git a/confluent_server/aiohmi/ipmi/private/session.py b/confluent_server/aiohmi/ipmi/private/session.py index 5ce8e9e7..a13d9cb3 100644 --- a/confluent_server/aiohmi/ipmi/private/session.py +++ b/confluent_server/aiohmi/ipmi/private/session.py @@ -479,10 +479,15 @@ class Session(object): # id, however it's easier this way forbidsock.append(self.socket) if trueself: - return await cls._await_login(trueself) + await cls._await_login(trueself) + # Count the caller only once it is really getting the session + trueself.users += 1 + return trueself i = cls.initting_sessions.get(sesskey, False) if i: - return await cls._await_login(i) + await cls._await_login(i) + i.users += 1 + return i self = super().__new__(cls) self.forbidsock = forbidsock # Register before establishing, not after: this is where a caller @@ -531,6 +536,8 @@ class Session(object): # Whether this session still holds the socket pool count it took. # Set before anything can fail, so releasing is safe either way. self._socketclaimed = False + # How many callers hold this session; the last one out closes it + self.users = 1 self.lastpayload = None self._customkeepalives = None # queue of events denoting line to run a cmd @@ -1936,7 +1943,15 @@ class Session(object): WAITING_SESSIONS.release() async def logout(self, sessionok=True): - + if (sessionok and self.users > 1 + and not self.broken and not self.cleaningup): + # A caller finished with a working session gives up its claim and + # leaves it to whoever else holds it; closing it here would hand + # them one that answers as though it had been lost. sessionok is + # false when it is no longer usable, and then it comes down anyway. + self.users -= 1 + return {'success': True} + self.users = 0 if self.cleaningup: self.nowait = True if self.logged: From 5aec0e69f5a9d26cc6c1f98e4252b10fe4449662 Mon Sep 17 00:00:00 2001 From: Markus Hilger Date: Fri, 14 Aug 2026 14:07:49 +0200 Subject: [PATCH 6/8] Recognise a session that is already open to the same bmc The check for sharing a live session compared the credentials a session keeps encoded against the strings every caller passes, so it never matched and each caller built another session beside the one it could not see. Normalise both sides. Three commands to one bmc went from three sessions on three sockets to one, and from five sessions open on the bmc to three. Not from the port to asyncio: upstream compares the same two things the same way. --- .../aiohmi/ipmi/private/session.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/confluent_server/aiohmi/ipmi/private/session.py b/confluent_server/aiohmi/ipmi/private/session.py index a13d9cb3..6dfc0a1a 100644 --- a/confluent_server/aiohmi/ipmi/private/session.py +++ b/confluent_server/aiohmi/ipmi/private/session.py @@ -251,6 +251,18 @@ async def _poller(timeout=0): return sessionqueue +def _credkey(value): + """A credential in the form a session keeps it, so callers can be compared + + A session encodes what it was given, while the checks for sharing one are + built from whatever the caller passed. Put both through here. + """ + try: + return value.encode('utf-8') + except AttributeError: + return value + + def _aespad(data): """ipmi demands a certain pad scheme, per table 13-20 AES-CBC encrypted @@ -450,7 +462,7 @@ class Session(object): keepalive=True): trueself = None forbidsock = [] - sesskey = (bmc, userid, password, port, kg) + sesskey = (bmc, _credkey(userid), _credkey(password), port, kg) for res in socket.getaddrinfo(bmc, port, 0, socket.SOCK_DGRAM): sockaddr = res[4] if ipv6support and res[0] == socket.AF_INET: @@ -466,8 +478,8 @@ class Session(object): del cls.bmc_handlers[sockaddr][portself[0]] continue if (self.bmc == bmc - and self.userid == userid - and self.password == password + and self.userid == _credkey(userid) + and self.password == _credkey(password) and self.kgo == kg): trueself = self break From 8aecc6959a3923db173306c2523daba8cdcc62b9 Mon Sep 17 00:00:00 2001 From: Markus Hilger Date: Fri, 14 Aug 2026 14:07:49 +0200 Subject: [PATCH 7/8] Do not let a logout come back round into its own notification Telling a keepalive that the session is gone can end up back in logout, because reporting it is how a console gives up its claim. The inner pass finished by clearing the register of keepalives while the outer was still walking it, so the next entry was looked up on None. Two entries is what an XCC has, the console's and the oem handler's. Take the callbacks and give up the register before notifying anyone. --- .../aiohmi/ipmi/private/session.py | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/confluent_server/aiohmi/ipmi/private/session.py b/confluent_server/aiohmi/ipmi/private/session.py index 6dfc0a1a..30caa535 100644 --- a/confluent_server/aiohmi/ipmi/private/session.py +++ b/confluent_server/aiohmi/ipmi/private/session.py @@ -1987,16 +1987,19 @@ class Session(object): self.onlogpayload = None self.logging = False if self._customkeepalives: - for ka in list(self._customkeepalives): - # Be thorough and notify parties through their custom - # keepalives. In practice, this *should* be the same, but - # if a code somehow makes duplicate SOL handlers, - # this would notify all the handlers rather than just the - # last one to take ownership - if self._customkeepalives[ka][1] is None: + # Be thorough and notify parties through their custom + # keepalives. In practice, this *should* be the same, but + # if a code somehow makes duplicate SOL handlers, + # this would notify all the handlers rather than just the + # last one to take ownership + # Take the callbacks and give this up first: notifying one can + # come back round through here and clear it mid walk + callbacks = [ka[1] for ka in self._customkeepalives.values()] + self._customkeepalives = None + for callback in callbacks: + if callback is None: continue - await self._customkeepalives[ka][1]( - {'error': 'Session Disconnected'}) + await callback({'error': 'Session Disconnected'}) self._customkeepalives = None if not self.broken: self.broken = True From 0b7f6b13951125578faa65639851b1765a2e4a7c Mon Sep 17 00:00:00 2001 From: Markus Hilger Date: Fri, 14 Aug 2026 14:07:49 +0200 Subject: [PATCH 8/8] Tidy three loose ends around sharing a session Closing a console gives up its claim on the session, and that talks to the bmc, so let it fail the same way the console's own deactivate is already allowed to. kg was left as the caller passed it in both the register key and the reuse check, so the mismatch fixed for the name and password still applied to it. The count for a new socket was taken before binding it and before the io task was known to be up. Take it last, once nothing is left that can still fail. --- confluent_server/aiohmi/ipmi/console.py | 7 ++++++- confluent_server/aiohmi/ipmi/private/session.py | 10 +++++++--- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/confluent_server/aiohmi/ipmi/console.py b/confluent_server/aiohmi/ipmi/console.py index 6f3abfeb..ab463eb5 100644 --- a/confluent_server/aiohmi/ipmi/console.py +++ b/confluent_server/aiohmi/ipmi/console.py @@ -217,7 +217,12 @@ class Console(object): self.ipmi_session = None if sess.sol_handler is not None and sess.sol_handler.__self__ is self: sess.sol_handler = None - await sess.logout() + try: + await sess.logout() + except exc.IpmiException: + # A bmc that has stopped answering must not be what makes + # closing a console fail, as for the deactivate above + pass async def close(self): """Shut down an SOL session""" diff --git a/confluent_server/aiohmi/ipmi/private/session.py b/confluent_server/aiohmi/ipmi/private/session.py index 30caa535..6726a4e6 100644 --- a/confluent_server/aiohmi/ipmi/private/session.py +++ b/confluent_server/aiohmi/ipmi/private/session.py @@ -390,7 +390,6 @@ class Session(object): # Rather than wait until send() to bind, bind now so that we have # a port number allocated no matter what tmpsocket.bind(('', 0)) - cls.socketpool[tmpsocket] = 1 else: tmpsocket.bind(server[4]) iosockets.append(tmpsocket) @@ -413,6 +412,10 @@ class Session(object): initevt = asyncio.Event() iothreadwaiters.append(initevt) await initevt.wait() + if server is None: + # Take the count last: one taken for a socket no session ever + # received is one nobody is left to give back + cls.socketpool[tmpsocket] = 1 return tmpsocket def _sync_login(self, response): @@ -462,7 +465,8 @@ class Session(object): keepalive=True): trueself = None forbidsock = [] - sesskey = (bmc, _credkey(userid), _credkey(password), port, kg) + sesskey = (bmc, _credkey(userid), _credkey(password), port, + _credkey(kg)) for res in socket.getaddrinfo(bmc, port, 0, socket.SOCK_DGRAM): sockaddr = res[4] if ipv6support and res[0] == socket.AF_INET: @@ -480,7 +484,7 @@ class Session(object): if (self.bmc == bmc and self.userid == _credkey(userid) and self.password == _credkey(password) - and self.kgo == kg): + and _credkey(self.kgo) == _credkey(kg)): trueself = self break # ok, the candidate seems to be working, but does not match