proxy-io: Reference-count Connection objects #336

This is a documentation-only change meant to make upcoming commits easier to
understand.

This change was written with Claude Opus 4.8 (1M context).

Correct the ThreadContext "Synchronization note", which said
Waiter::m_mutex must not be locked before EventLoop::m_mutex. That is
the reverse of the documented and actual lock order (Waiter::m_mutex
first, as ~ProxyServer<Thread> does). The constraint it was reaching for
is the EventLoop blocking rule now documented on Waiter::m_mutex.

This change was written with Claude Fable 5.1.

Currently, a ListenConnections listener that reaches its max-connection limit
stops accepting new connections permanently if one of its connections is closed
locally instead of by a remote disconnect. Closing a connection locally (e.g.
erasing it from m_incoming_connections) leaves the listener's active-connection
count stuck at the limit, so it never resumes accepting.

This happens because the count is decremented by a callback which only fires on
a remote disconnects, not local disconnects. Fix by moving the decrement to
callback which fires on both local and remote disconnects.

Add a regression test that closes a connection locally and checks the listener
resumes accepting; it fails before this change (the listener never accepts the
waiting client) and passes after.

Co-Authored-By: Enoch Azariah <[email protected]>
This change was written with Claude Opus 4.8 (1M context).

Fix a use-after-free, possible since the destroy_connection option was added
in 2019 (c685fa9): a Connection's disconnect handler could run after the
Connection had already been destroyed, deleting it a second time and
crashing. Reported by enirox001 in
https://github.com/bitcoin-core/libmultiprocess/pull/335#discussion_r3821831654

Give each Connection a shared_ptr "alive" token that disconnect handlers hold
a weak_ptr to and check before running, so a handler is skipped once its
Connection is gone. Having this check also enables the simplifications
described below.

Previously each Connection kept its disconnect handlers in its own
kj::TaskSet, and when the network disconnected it moved a handler onto the
shared event loop TaskSet with kj::evalLater. Destroying the Connection
destroyed that per-connection TaskSet, canceling a still-pending handler --
but a handler already moved onto the shared TaskSet was no longer canceled
and could run after the Connection was gone. (The evalLater step existed only
to avoid a "promise callback destroyed itself" error when a handler deletes
its own Connection, which the per-connection TaskSet made possible.)

With the token doing the cancellation, neither the per-connection TaskSet nor
the evalLater step is needed, and both are removed.

Co-Authored-By: Enoch Azariah <[email protected]>
This change was written with Claude Fable 5.

Fix a race between a thread exiting after making IPC calls and its
connection being destroyed on the event loop thread, which could destroy
the same ProxyClient<Thread> object twice. ~ThreadContext destroyed the
thread-local request_threads/callback_threads maps with no locking while
the SetThread cleanup callback run by ~Connection erased entries from the
same maps. When both ran at once, each side destroyed the entry's
ProxyClient<Thread>, and ~Connection then ran the ProxyClientBase
disconnect callback on the freed map node (heap-use-after-free, then a
glibc "double free or corruption" abort).

Fix by making map entry removal decide which side destroys an entry:
~ThreadContext and the SetThread callback each remove entries under
Waiter::m_mutex before destroying them, and a side that finds an entry
already gone leaves it to the other. See the code comments for why the
entries are destroyed with the mutex released.

Add a regression test, "Thread exiting while its connection is
destroyed", which uses a new testing_hook_thread_client_destroy hook to
interleave the two sides deterministically and fails on every run
without the fix.

The race is long-standing and reachable on master via connections
created by ConnectStream, whose onDisconnect handler deletes the client
Connection on the event loop thread when the peer disconnects.

This change was written with Claude Fable 5.1.

Add an accessor and a Connections type alias for the EventLoop's
list of incoming connections, so future code can be simplified to
locate a specific connection without directly accessing the private
list or embedding a Connection object itself.

This change was written with Claude Sonnet 5.

Split connection teardown out of ~Connection into an idempotent disconnect()
method, with the destructor delegating to it. For existing callers, this is a
behavior-neutral refactor: the same steps run in the same order on destruction.

Having a separate disconnect() method allows severing a connection while
keeping the Connection object alive, which the next commits use to let
shutdown code wait for in-flight server call bodies to finish after a
disconnect (bitcoin/bitcoin#35845). Two details are new in the disconnect()
method which were not present in the destructor method:

- disconnect() expires the m_alive token explicitly, where previously it was
  expired implicitly by member destruction. This keeps onRemoteDisconnect able
  to distinguish a local disconnect from a remote one when a connection is
  severed without destroying the object (see the disconnect() code comment).

- disconnect() explicitly releases m_thread_pool and m_thread_map so worker
  thread teardown happens at disconnect time whether or not the object is
  destroyed right away. Previously this happened implicitly during member
  destruction.

This change was written with Claude Fable 5.

Add a _Serve/ServeStream overload accepting the init object as a
shared_ptr, so callers can transfer ownership instead of always
passing a reference to an object they keep alive themselves. Existing
reference-taking callers keep working through a thin overload that
wraps the reference in a shared_ptr with an empty deleter.

Also return the constructed ProxyServer along with an iterator to its
Connection in loop.m_incoming_connections, so callers can look up or
erase the connection later without embedding a Connection object
themselves.

This change was written with Claude Sonnet 5.

Add a per-connection ServerObjectTracker counting live ProxyServer objects,
incremented in the ProxyServerBase constructor and decremented in its
destructor, with Connection::waitDrained() blocking until the count reaches
zero and Connection::pendingServerObjects() exposing it for logging.

Disconnecting a connection cancels the KJ promise of an in-flight call, but a
C++ server method body already dispatched to a worker thread runs to
completion. Counting live server objects turns Cap'n Proto's object lifetime
rules into a usable quiescence signal: a ProxyServer object is not destroyed
until its outstanding calls finish (the target capability is kept alive for
the duration of a call and pinned by post()/PassField via thisCap()), so
after disconnect() the count drains to zero exactly when no server call body
is still executing. Waiting for that lets shutdown code avoid freeing
application state that a still-running call body dereferences
(bitcoin/bitcoin#35845).

The tracker is held via shared_ptr by the Connection and by every
ProxyServer object because objects kept alive by in-flight calls can outlive
the Connection on some teardown paths (see ~ProxyServerBase), and their
destructors must decrement state that is still valid. It must be declared
before m_rpc_system, whose construction creates the bootstrap server object
that registers itself with the tracker.

This change was written with Claude Fable 5.

Give ServeStream and ConnectStream a destroy_connection parameter,
defaulting to true, so callers can opt out of automatic connection
teardown and manage the Connection's lifetime themselves instead.
ServeStream gates the internal disconnect handler's list erase on the
parameter; ConnectStream just forwards it to the existing
ProxyClientBase parameter of the same name.

This lets callers that need to keep a connection alive past a
disconnect notification (e.g. to let in-flight server calls finish)
use these helpers instead of constructing a Connection manually.

This change was written with Claude Sonnet 5.

Add a deterministic mptest regression test for bitcoin/bitcoin#35845: hold a
server method body in flight on a worker thread, call
Connection::disconnect(), and assert that Connection::waitDrained() blocks
until the body finishes and its server object is destroyed. Also covers
destroying an already-disconnected connection (~Connection noticing
disconnect() has run).

This change was written with Claude Fable 5.
Co-Authored-By: Enoch Azariah <[email protected]>

Replace TestSetup's manual Connection construction with
ServeStream/ConnectStream, following the same pattern already used in
Bitcoin Core's own IPC test and fuzz code. This drops server_on_disconnect
entirely: ServeStream's destroy_connection parameter now controls whether
a remote disconnect erases the server Connection, so the only test that
needed to suppress that (the mp#348 getResults race test) just constructs
TestSetup with server_owns_connection=false instead of overriding a
callback afterward.

This change was written with Claude Sonnet 5.

# Conflicts:
#	include/mp/proxy-io.h
#	test/mp/test/test.cpp

Make Connection objects shared_ptr-owned, created via a new Connection::make()
factory, and have every proxy object share ownership of its connection
(ProxyContext::connection becomes a shared_ptr, populated via
enable_shared_from_this). A Connection now always outlives its proxy objects
and survives disconnect() as an inert husk until its last reference drops.

Sharing ownership removes two workarounds that existed only because a
Connection could previously be destroyed while proxy objects still referenced
it:

- ~ProxyServerBase no longer has to avoid dereferencing m_context.connection;
  the connection is guaranteed to still exist.

- The separate m_alive liveness token is dropped. The deferred disconnect
  handler (renamed onRemoteDisconnect -> afterDisconnect, since it fires on any
  disconnect and not only remote ones) instead runs off a weak_ptr to the
  Connection and passes each handler a Connection* -- the live connection, or
  null if it was already destroyed before the handler ran. Call sites branch on
  that explicitly rather than the framework silently skipping them; handler
  bodies are idempotent disconnect() calls, not deletions.

Two supporting changes fall out of this and are documented at the code rather
than repeated here: connection construction is split into make() and serve()
(see their comments), and because the shared references form a cycle that
reference-dropping can't break, all teardown now routes through disconnect() --
so every teardown path calls it (see the note on Connection::make). The _Serve
disconnect handler's list bookkeeping changed to match (see the comment there).

Also drop the test that checked a queued disconnect handler does not run after
its Connection is destroyed. It reproduced the m_alive race by constructing a
Connection directly and freeing it while the handler was still queued. Both
premises are gone: a Connection can only be built through make() now, and
handler bodies no longer delete anything, so the double deletion the test
guarded against is structurally impossible. afterDisconnect now hands such a
handler a null Connection* instead, which its callers handle explicitly, and
the ordinary disconnect and drain tests exercise that path.

This change was written with Claude Fable 5.

Remove the per-client disconnect tracking from ProxyClientBase: client
objects no longer register a cleanup callback with their Connection, and a
disconnect no longer eagerly releases their m_client capability handles or
nulls their connection pointers.

Neither is necessary now that proxy objects share ownership of their
Connection. The connection pointer stays valid after a disconnect because the
Connection outlives its proxies, and keeping the capability handle is safe:
Cap'n Proto's per-connection state is refcounted and outlives the RPC system as
long as handles reference it, with calls on handles of a disconnected
connection failing cleanly with DISCONNECTED errors. The handle is simply
released (on the event loop thread, since capability refcounts are not thread
safe) whenever the client object is eventually destroyed, and clientInvoke
checks the connection's disconnected() predicate instead of a nulled pointer,
throwing the same 'IPC client method called after disconnect' error as before.

This deletes the detach machinery from ~ProxyClientBase, including the FIXME'd
duplicate-cleanup code path. Connection::onDisconnect (the renamed
addSyncCleanup) remains for its one other user, the per-thread connection maps
(see SetThread), which the next commit converts.

Because the connection pointer is no longer nulled on disconnect, ~ProxyClient
<Thread> can no longer use it to tell whether Connection::disconnect() has
already run and freed its m_disconnect_cb cleanup node. It now keys off the
connection's disconnected() predicate instead: if the connection is
disconnected the node is already gone and must not be passed to
cancelOnDisconnect. Without this, ~ProxyClient<Thread> erased an already-freed
list node -- a heap-use-after-free (see the "Waiting for in-flight server call
to finish after disconnect" test, which this commit re-enables).

That test also has to hold its own shared reference to the server Connection
across the disconnect() + waitDrained() sequence, the way Ipc::disconnect
Incoming does: under shared ownership the last server proxy (destroyed once the
drained body finishes) would otherwise free the Connection while the test is
still observing it.

This change was written with Claude Fable 5.
This change was written with Claude Opus 4.8.

Update comments to reflect that after the previous commit, the sync cleanup
callback list has exactly one remaining purpose: eagerly removing a
disconnected connection's ProxyClient<Thread> entries from the thread_local
per-thread connection maps (ThreadContext::request_threads /
callback_threads) via callbacks registered by SetThread.

Unlike interface clients, these entries cannot simply be left alive across a
disconnect: they are owned by other threads that may never touch their maps
again, and a surviving entry would hold the disconnected Connection object
-- and through its EventLoopRef the event loop -- alive indefinitely,
preventing the loop from ever exiting. (Replacing the callbacks with lazy
garbage collection in SetThread was tried and hangs mptest for exactly this
reason: entries owned by long-lived threads pin the loop after their
connection is gone.) So this per-object disconnect tracking is retained by
design, now clearly documented as thread-map-specific rather than a general
client-object mechanism.

This change was written with Claude Fable 5.

Now that proxy objects hold shared ownership of their Connection, a
ProxyServer object kept alive by an in-flight call can no longer outlive the
Connection, so ~ProxyServerBase can always reach the tracker through
m_context.connection. Drop the shared_ptr indirection that existed to keep
the tracker valid past the Connection's death, and the separate tracker
handle member on ProxyServerBase.

No behavior change; Connection::waitDrained() semantics are identical.

This change was written with Claude Fable 5.

Needed because changing m_incoming_connections type is an API change.