IPC stability bugs

Bug 78

Listener at its connection limit stops accepting after a local disconnect

Status
Open, fix under review
Symptom
service-loss; worst effect seen: hang
Where seen
Libmultiprocess tests (mptest)
Trigger
The process closes its own connection while the peer is still there
First reported
2026-08-20 (first report)
Fixed
Not yet
Fix
mp#361
In released binaries
No
Defect ever on master
Yes

Summary

A ListenConnections listener that had reached its max_connections limit stopped accepting new connections permanently if the server then closed one of its connections locally, because the active-connection counter was only decremented on a remote disconnect. New clients would wait forever. It was found in code review and shown with a regression test; it has never been observed in practice. Bitcoin Core cannot trigger it today, since it only closes IPC connections locally at shutdown, and no release contains the connection limit. A fix is in mp#361, which fixes several disconnect bugs found in review, not yet merged.

What happened

Root cause

_Serve (include/mp/proxy-io.h) called the listener's on_disconnect bookkeeping callback (the counter decrement) inside the it->onDisconnect(...) handler, together with the m_incoming_connections.erase(it) that removes the connection after a remote disconnect. Connection::onDisconnect handlers are cancelled when the Connection is destroyed locally. After a local close the erase is correctly skipped, but the counter decrement is skipped too, leaving ListenConnections at its limit.

The counter was written with only remote disconnects in mind, because that is the only way connections went away in the tests and in Bitcoin Core, which closes clients locally only in Ipc::disconnectIncoming() at shutdown, when accepting again no longer matters. mp#335 makes local disconnection an explicit operation with Connection::disconnect(). Other servers built on the library that close connections themselves could stop accepting.

Fix

No fix merged yet. 33bd6f833f5a in mp#361 (co-authored by enirox001 and Claude) registers the bookkeeping with it->addSyncCleanup(on_disconnect) instead. Sync cleanup functions run from the Connection destructor on both local and remote disconnects, so the slot is always released. The addSyncCleanup comment in src/mp/proxy.cpp is extended to say these callbacks may also update listener bookkeeping. enirox001 ACKed the PR.

Next steps

Timeline

Lessons

What would have prevented this bug, found it sooner, or made it easier to debug. The patterns page groups these lessons across all bugs.

Pattern Error paths that skip cleanup or completion: Cleanup on only one of two exit paths

  • Prevented by a clear ownership model, a different API shape
  • Earliest detector a unit test (did not exist then)
  • Diagnosis easy
  • Fix design easy
  • Rust no

Would Rust have prevented it? No. Choosing the hook that fires on only one of two close paths is a logic error with no Send, Sync or lifetime violation; a Drop guard on the slot would fix it in either language, and nothing in Rust forces that design. More on Rust

Could it have been prevented?

Yes. The listener's slot is a resource acquired at accept time, and the fix moves its release from an event handler to addSyncCleanup, which runs from the Connection destructor on any close. Had the slot been an RAII guard owned by the Connection, there would have been no hook to choose and no path to miss. The API also made the wrong choice easy: 39a10ce8958e (mp#269) put the decrement next to the m_incoming_connections.erase(it) inside onDisconnect, whose comment said it is "run when the peer closes the connection" and cancelled on a local close, so the information was there but the method name does not say "remote". A name like onRemoteDisconnect, or a single "on any close" hook as the documented place for bookkeeping, would have steered the author. The fix's own comment in 33bd6f833f5a now records why the decrement lives where it does.

Could it have been found sooner?

Yes, by a ListenConnections test with max_connections=1 that closes the first connection locally and waits for a second client, which is enirox001's regression test. mp#269's tests covered the limit only through remote disconnects, because that was the only way a connection went away in the tests and in Bitcoin Core, which closes clients locally only in Ipc::disconnectIncoming() at shutdown, when accepting again no longer matters. It was found six weeks after the code landed, by reading the mp#335 commit that made local disconnection an explicit operation, which is the first time the missing path mattered.

What made it hard to debug?

Nothing. The report came with the mechanism, a fix and a test that hangs before and passes after; ryanofsky's reply agreed it was a separate pre-existing bug and not a practical problem for Bitcoin Core, and the 14-line fix went into mp#361 as written. Had it reached a server that does close connections itself, the symptom would have been a listener that silently stops accepting, with nothing logged; a periodic log of the active count against the limit, or a count exposed to tests, would have made that state visible.

Lesson

Release a counted resource from the path that runs on every exit, normally a destructor, not from a handler that fires on one kind of exit; and when an API offers two close hooks, name them for what they exclude.

Classification

The survey's classification of this bug. The explorer charts these fields across all bugs; the method page explains them.

Field Value
Symptom service-loss
Worst effect observed hang
Seen in mptest
Scope production
Trigger Process closes its own connection (local-disconnect)
Phase steady-state
Side server
Component proxy-io
Root cause kind assumption-violated
Fix kind local-fix
Detection review
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see no
Defect ever on master yes
Released no
Confidence high