Skip to content

Get rid of the unneeded Arc<Mutex> around the gateway client - #193

Open
drauschenbach wants to merge 1 commit into
cppalliance:masterfrom
drauschenbach:david/drop-http-client-mutex-java-smell
Open

drauschenbach wants to merge 1 commit into
cppalliance:masterfrom
drauschenbach:david/drop-http-client-mutex-java-smell

Conversation

@drauschenbach

@drauschenbach drauschenbach commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Investigation

In workshop/server/src/agents/broker.rs, Arc<Mutex<...>> is a Java smell. investigate replacing with a lock-less
mutable cell ref.

I can't use a lock-free mutable cell here (a Cell or RefCell behind a shared reference). But the lock
isn't needed at all: if the reqwest client is shared, the cache can go.

  Why a Cell or RefCell won't compile

  - InferenceBroker requires Send + Sync (crates/harness-internal/runner/src/performers.rs:58). Cell and RefCell are
    !Sync, so WorkshopBroker would stop satisfying the trait.
  - WorkshopBroker is cloned into each models() future (broker.rs:142), and those futures run on any thread. The cached
    value is shared state that changes across threads, so it needs something thread-safe: a mutex, an atomic, or an
    ArcSwap.

  Why the cache exists

  GatewayBroker::new creates a new reqwest::Client on every call (GatewayChat::new in
  crates/harness-gateway-client/src/transport.rs:96; keyless at :119 does the same). Without the cache, every round
  would lose the connection pool and keep-alive and reload the TLS setup. Caching that client per gateway generation is
  the only reason Arc<Mutex<Option<(u64, GatewayBroker)>>> is there.

  Options

  1. Swap in ArcSwapOption. arc-swap is already a workspace dependency, and workshop-gateway already uses it for the
     gateway snapshot (crates/workshop/gateway/src/binding.rs:76). Reads become lock-free. If two rounds miss at the
     same moment, both build a broker and the last store wins, which is harmless. You still need an Arc around it to
     share it between clones, though. It removes the Mutex and the poison handling, but not the cache.
  2. Share one client and delete the cache (recommended). catalog.rs:124 already keeps a single process-wide client in a
     OnceLock<reqwest::Client>. If GatewayChat::new and keyless use the same shared client, building a GatewayBroker
     costs a few clones, since cloning a reqwest::Client only copies an Arc handle. current() can then build a fresh
     broker from usable_gateway(...) on every call, which it already reads on every call anyway. With that:
     - the cached field, the Arc, the Mutex, the generation comparison and the poison comment all disappear;
     - WorkshopBroker keeps just the Registry;
     - a replaced gateway takes effect on the next call automatically, with no generation bookkeeping, so the cache
       can't go stale;
     - every user of GatewayChat gets connection reuse, not just Workshop.

     What changes: all gateway generations share one connection pool. reqwest keys its pool by scheme, host and port, so
     a replaced gateway at a new address gets its own connections, and the old ones close when they've been idle long
     enough. Nothing sets per-client options today: the receive timeout and size limit are stored on GatewayChat itself,
     not on the reqwest client, so sharing loses nothing.

     Existing tests should cover the behaviour. The replacement test (broker-tests.rs:231) checks that the next round
     goes to the new gateway, and that holds without the cache.
  3. Build the broker where the gateway is published (in the registry, next to GatewayHandles). I'd skip this:
     workshop-gateway would have to depend on harness-gateway-client, which makes the dependency graph bigger, against
     the "small surface per crate" rule.

  Option 2 fits the project's "do more with less" principle best: the change is one line in transport.rs (two call
  sites) and the cache comes out of broker.rs.

Perform Work

perform the work. write the plan into a new 2026-11-03-2-drop-http-client-mutex-java-smell.md file in the
vibe/2026-11/ folder.

The Mutex is gone: the agent broker and search provider no longer cache Gateway clients, because every client now
  shares one HTTP connection pool. All gates pass except one Workshop test, which also fails on unmodified master.
  Nothing is committed.

  The plan is at vibe/2026-11/2026-11-03-2-drop-http-client-mutex-java-smell.md. I read 2026-110-3-2 as a typo for
  2026-11-03-2 and added .md to match the other plans in vibe/; tell me if you meant something else.

  Shared client (harness-gateway-client)
  - I turned the catalog fetch's existing process-wide client into a crate-wide shared_client() in transport.rs.
    GatewayChat::new, GatewayChat::keyless, GatewaySearch and the catalog fetch all use it, so building a broker costs a
    few clones.
  - New test transport/tests/pool.rs: two clients built separately send their requests over the same connection. With
    the old per-client reqwest::Client::new() put back, it fails.

  Workshop server
  - broker.rs: dropped cached: Arc<Mutex<Option<(u64, GatewayBroker)>>>. WorkshopBroker now holds only the Registry, and
    current() builds a broker from the current gateway on every call.
  - search.rs had the same cache behind a Mutex, so I removed that too.
  - gateway.rs: removed UsableGateway::generation, since nothing else read it.

  Behaviour change: all gateway generations now share one connection pool. Connections are pooled per host and port, so
  a gateway at a new address gets its own connections and the old ones close once idle. The per-client timeouts and size
  limits were already applied per request, so nothing depended on separate clients.

  Gates: formatting, both clippy runs, the docs gate and the wording scan pass, and so does the non-Workshop suite (4126
  tests). In the Workshop suite, realtime_relay::overload::stalled_browser_cleanup_is_bounded_after_gateway_disconnect
  fails after about 10s. It fails the same way with my changes stashed, so it's an existing problem and not caused by
  this change.

@drauschenbach drauschenbach changed the title Get rid of the unneeded Arc<Mutex> Java smell around the gateway client Get rid of the unneeded Arc<Mutex> around the gateway client Nov 3, 2026
@drauschenbach
drauschenbach marked this pull request as ready for review November 3, 2026 22:09
@vinniefalco

Copy link
Copy Markdown
Member

This looks like an improvement but can we make it a second commit in 192 instead?

@drauschenbach

Copy link
Copy Markdown
Contributor Author

This looks like an improvement but can we make it a second commit in 192 instead?

What would be the guideline for batching them into a single PR? As far as I can tell, they are completely unrelated.

@vinniefalco

Copy link
Copy Markdown
Member

They look like they would have a merge conflict

@drauschenbach

Copy link
Copy Markdown
Contributor Author

They look like they would have a merge conflict

That's no concern, I know how to rebase before final merge. If I consolidate them, Andrew would have a conflated view of the 2nd one, and that's an unnecessary cognitive load.

@drauschenbach
drauschenbach force-pushed the david/drop-http-client-mutex-java-smell branch from bb37547 to f3fb2b1 Compare November 4, 2026 15:40

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants