ref(pool): coordinate demand startup outside the global lock - #169
Open
loocor wants to merge 2 commits into
Open
ref(pool): coordinate demand startup outside the global lock#169loocor wants to merge 2 commits into
loocor wants to merge 2 commits into
Conversation
Move the production demand startup path off the shared pool lock: route preparation and allocation happen under the lock, transport startup runs on a detached worker clone, publication is conditional on the captured route/generation/config facts, and stale results are discarded outside the lock by the owning attempt. Concurrent demands for the same connection identity share one in-flight attempt through a typed outcome channel instead of serializing on the global mutex. All production callers (proxy tools, capability runtime, prompts, resources, Unify broker) and the Inspector now route through the coordinator; the old lock-held ensure_connected* entrypoints are removed. Adds pool startup ownership contract tests covering parallel progress, single-flight outcome sharing, conditional publication across config changes, and stale cleanup isolation from newer instances.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Deploying mcpmate-site with
|
| Latest commit: |
4a91b16
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f7de22d7.mcp-umate.pages.dev |
| Branch Preview URL: | https://ref-pool-lifecycle-ownership.mcp-umate.pages.dev |
There was a problem hiding this comment.
Pull request overview
This PR introduces a coordinated “startup ownership” path for demand-driven upstream connections so transport initialization (DB/secret/process/initialize) happens outside the global pool mutex, while still single-flighting concurrent demands for the same production route identity.
Changes:
- Added a new
core::pool::startupcoordinator with single-flight startup attempts and conditional publication guarded by route/generation/config-fingerprint facts. - Refactored production-demand call sites (proxy tools, capability runtime/prompts/resources, Unify broker, Inspector) to route through
ensure_connected_coordinated. - Added/updated contract tests to validate non-blocking parallel startup, single-flight behavior, stale-attempt supersession, and safe cleanup.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/src/core/pool/startup.rs | New coordinator implementing prepare-under-lock → start-outside-lock → conditional publish → discard-outside-lock, plus contract tests. |
| backend/src/core/pool/mod.rs | Wires the new startup module into the pool module tree. |
| backend/src/core/pool/executor.rs | Removes old lock-held ensure_connected* entrypoints and exposes internal helpers needed by the coordinator. |
| backend/src/core/pool/connection.rs | Adds in-flight startup attempt registry state to the pool; renames failure update helper to a more general pool-level name. |
| backend/src/core/proxy/server/tools.rs | Routes tool-call connection demand through the coordinator (no startup under the global lock). |
| backend/src/core/capability/runtime.rs | Routes capability tool-call startup through the coordinator. |
| backend/src/core/capability/prompts.rs | Releases the pool lock before ensuring a production connection via the coordinator, then reacquires for service lookup. |
| backend/src/core/capability/resources.rs | Same coordinator-based connection ensure for resources reads, avoiding lock-held startup. |
| backend/src/mcper/builtin/broker.rs | Unify broker startup now uses coordinator and default selection fallback. |
| backend/src/inspector/service.rs | Inspector “ensure proxy connection” now uses coordinator with default affinity selection. |
| backend/tests/capability_read_surface.rs | Updates integration test to use the coordinator entrypoint and new pool sharing shape. |
Suppressed comments (1)
backend/src/core/pool/startup.rs:497
- Same as the success path:
pool.lock().await.database.clone()is held as a temporary across the.awaitofpublish_startup_result, keeping the pool mutex locked while awaiting DB name resolution/event publication.
UpstreamConnectionPool::publish_startup_result(
pool.lock().await.database.clone(),
&attempt.server_id,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+480
to
+486
| UpstreamConnectionPool::publish_startup_result( | ||
| pool.lock().await.database.clone(), | ||
| &attempt.server_id, | ||
| true, | ||
| None, | ||
| ) | ||
| .await; |
Owner
Author
There was a problem hiding this comment.
Fixed. The event database clone is now extracted into its own scope before publish_startup_result(...).await in both the Published and Failed branches, so the pool guard no longer lives across the await.
- Publish HTTP/SSE startup results without a pool cancellation token; only stdio transports register one (previously panicked for HTTP). - Extract affinity-bound startup results from client_bound_connections so PerClient/PerSession routes publish instead of panicking. - Publish the startup event exactly once from the coordinator and scope the event database clone outside the await to avoid lock-held events. - Invalidate health reconnect and check backoff under the real pool lock during prepare instead of on a stale worker snapshot. - Clean up the attempt registry when a spawned attempt task panics so joiners fail instead of blocking forever. - Replace the failure-state diff with explicit failure classification at publish time and reuse the transport helper without DB side effects. - Add HTTP/token-less, affinity-bound, single-flight, and parallel-startup contract test coverage.
loocor
force-pushed
the
ref/pool-lifecycle-ownership
branch
from
August 1, 2026 01:34
0266bd9 to
4a91b16
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Concurrent production demands (proxy tool calls, capability runtime, prompts,
resources, Unify broker, Inspector) each acquired the shared pool mutex and
then awaited the full upstream startup path (
ensure_connected*→connect_internal) while holding it. A slow server A's database/secret/process/transport/initialize blocked server B's prepare, status reads, and independent
startup, and concurrent demands did not share a single typed startup outcome.
This PR implements the first slice of the pool lifecycle ownership plan
(Project item:
ref(pool): define lifecycle leases and startup ownership):startup now runs as prepare-under-lock → start-outside-lock →
conditional-publish → discard-outside-lock, with one in-flight attempt per
connection identity.
Changes
backend/src/core/pool/startup.rscoordinator:ensure_connected_coordinated— production demand entry point.prepare_startup_attempt(lock) — resolve/allocate the production route,capture
created_at+config_fingerprintgeneration facts, register anin-flight attempt keyed by
ProductionRouteKey.run_startup_attempt(outside lock) — reuses the existingconnect_internalon a detached worker clone; DB side effects, transport,and protocol initialize never run under the shared lock.
publish_startup_attempt(lock) — conditional publication checks theroute, instance generation, and config fingerprint; stale attempts are
superseded and retried under the current configuration.
discard_startup_result(outside lock) — the owning attempt cancels andshuts down its own temporary transport; it never touches a newer published
instance.
outcome (
watchchannel); joiners never obtain shutdown authority.tools.rs, capabilityruntime.rs,prompts.rs,resources.rs, Unifybroker.rs, and Inspectorservice.rsroute throughthe coordinator. The old lock-held
ensure_connected*entrypoints areremoved.
conditional-publish pattern; no new lease hierarchy was introduced.
Contract tests
core::pool::startuptests (real stdio transports, real production entry):typed outcome (verified via identical error chains on failure)
Out of scope (unchanged)
Validation
cargo test --manifest-path backend/Cargo.toml --lib: 896 passed; 1pre-existing flaky (
durable_commit_survives_projection_failure_..., passesin isolation, present on the base commit too)
cargo test --manifest-path backend/Cargo.toml --features interop: lib 896passed (same flaky);
capability_read_surfaceintegration: 21 passedcargo clippy --manifest-path backend/Cargo.toml --all-targets --all-features -- -D warnings: cleanrefresh=forceconcurrentdemand): slow server 2.05s, fast server 0.04s — fast server startup is not
blocked by slow server initialization; MCP downstream
initialize+tools/listsucceeded as regression signalmodified files 92+/120-)
Review fixes (commit 0266bd9)
Independent read-only review findings addressed before handoff:
cancellation token (only stdio registers one); previously the success
path panicked for HTTP upstreams. Covered by a token-less publication
regression test.
coordinator (worker no longer emits events).
under the real pool lock during prepare instead of on a stale worker
snapshot.
fails joiners instead of stranding the route.
initialize marker; wider parallel-startup budgets).
Final validation:
cargo test --lib897 passed (1 pre-existing flaky),capability_read_surface21 passed, interop 21 passed, clippy-D warningsclean.Copilot review fixes (amended into 4a91b16)
External Copilot review raised 2 issues; both verified against the code and fixed:
run_startup_attemptnow extracts theworker connection from
client_bound_connectionsforPerClient/PerSessionroutes (only
connectionswas checked before). Regression testaffinity_bound_startup_publishes_from_client_bound_instancesadded.the
publish_startup_result(...).awaitin both branches, so the pool guardno longer spans the await.
Final validation:
cargo test --lib -- --test-threads=1899 passed / 0failed;
capability_read_surface21 passed; clippy-D warningsclean;startup contract suite 6 tests green.