fix: delegated child never leases a same-provider pool entry for another endpoint
A delegated child with an explicit base_url (e.g. an Azure OpenAI resource under provider "openai") shared the parent's "openai" credential pool on bare provider equality, and _lease_child_credential bound whatever entry acquire_lease() picked. _swap_credential adopts the entry's base_url as well, so the child was silently rebound to https://api.openai.com/v1, sent the Azure key there, got HTTP 401 and only then fell back (#68237). _resolve_child_credential_pool now requires endpoint coherence on top of provider identity (credential_pool_matches_provider + at least one entry whose base_url matches the child's) for both the shared parent pool and the provider's loaded pool; a mismatched pool is not attached and the child keeps its fixed credential. _lease_child_credential validates the leased entry against the child's base_url and, on a mixed pool, releases the wrong-host lease and leases an endpoint-matching entry by id instead. Entries and adapters without endpoint metadata cannot rebind and are accepted unchanged. Salvaged from #68240 (@oferlaor); the acquire_lease/leased_entry filter parameters and the extra pool-level helper were reduced to the two small delegate-side predicates.
This commit is contained in:
1
contributors/emails/ofer@openclaw.ai
Normal file
1
contributors/emails/ofer@openclaw.ai
Normal file
@@ -0,0 +1 @@
|
||||
oferlaor
|
||||
@@ -1281,6 +1281,26 @@ class TestChildCredentialPoolResolution(unittest.TestCase):
|
||||
result = _resolve_child_credential_pool("openrouter", parent)
|
||||
self.assertIs(result, mock_pool)
|
||||
|
||||
def test_same_provider_pool_for_another_endpoint_is_not_shared(self):
|
||||
"""#68237: an Azure child must not lease the parent's public-OpenAI ``openai`` pool — the lease swaps the
|
||||
child's base_url too, sending the pooled key to the wrong host. A pool with an entry for the child's endpoint
|
||||
is still shared."""
|
||||
from agent.credential_pool import CredentialPool, PooledCredential
|
||||
|
||||
azure = "https://res.cognitiveservices.azure.com/openai/v1"
|
||||
def _pool(url):
|
||||
return CredentialPool("openai", [PooledCredential(
|
||||
provider="openai", id=url, label=url, auth_type="api_key", priority=0, source="env:X",
|
||||
access_token="k", base_url=url)])
|
||||
parent = _make_mock_parent()
|
||||
parent.provider, parent.base_url = "openai", azure
|
||||
|
||||
parent._credential_pool = _pool("https://api.openai.com/v1")
|
||||
with patch("tools.delegate_tool_config._loaded_pool", return_value=None):
|
||||
self.assertIsNone(_resolve_child_credential_pool("openai", parent, azure))
|
||||
parent._credential_pool = _pool(azure)
|
||||
self.assertIs(_resolve_child_credential_pool("openai", parent, azure), parent._credential_pool)
|
||||
|
||||
# --- Custom-endpoint identity resolution (issue #7833) ---
|
||||
|
||||
|
||||
@@ -1363,6 +1383,26 @@ class TestChildCredentialLeasing(unittest.TestCase):
|
||||
self.assertEqual(result["status"], "error")
|
||||
child._credential_pool.release_lease.assert_called_once_with("cred-a")
|
||||
|
||||
def test_lease_binds_only_an_entry_for_the_child_endpoint(self):
|
||||
"""#68237: on a mixed same-provider pool the least-leased pick may target another host; the child must end up
|
||||
bound to the entry for its own base_url, with the wrong-host lease released."""
|
||||
from agent.credential_pool import CredentialPool, PooledCredential
|
||||
from tools.delegate_tool_child_run import _lease_child_credential
|
||||
|
||||
azure = "https://res.cognitiveservices.azure.com/openai/v1"
|
||||
def _entry(eid, url):
|
||||
return PooledCredential(provider="openai", id=eid, label=eid, auth_type="api_key", priority=0,
|
||||
source=f"env:{eid}", access_token=f"key-{eid}", base_url=url)
|
||||
pool = CredentialPool("openai", [_entry("pub", "https://api.openai.com/v1"), _entry("az", azure)])
|
||||
pool.acquire_lease("az") # tilt least-leased selection toward the public entry
|
||||
child = MagicMock(provider="openai", base_url=azure, _credential_pool=pool)
|
||||
|
||||
_pool, lease_id = _lease_child_credential(child)
|
||||
|
||||
self.assertEqual(lease_id, "az")
|
||||
self.assertEqual(child._swap_credential.call_args[0][0].base_url, azure)
|
||||
self.assertEqual(pool._active_leases, {"az": 2})
|
||||
|
||||
|
||||
class TestDelegateHeartbeat(unittest.TestCase):
|
||||
"""Heartbeat propagates child activity to parent during delegation.
|
||||
|
||||
@@ -390,14 +390,25 @@ def _defer_close_after_timeout(child: Any, child_future: Any) -> None:
|
||||
_resweep_timer.start()
|
||||
|
||||
def _lease_child_credential(child: Any) -> tuple[Any, Optional[str]]:
|
||||
"""Lease a credential from the child's pool (if any) and bind it; ``(pool, lease_id)``."""
|
||||
"""Lease a credential from the child's pool (if any) and bind it; ``(pool, lease_id)``. The bound entry must
|
||||
serve the child's endpoint: on a mixed same-provider pool the least-leased pick may target another host, so it is
|
||||
released and an endpoint-matching entry is leased by id instead (#68237)."""
|
||||
child_pool = getattr(child, "_credential_pool", None)
|
||||
if child_pool is None:
|
||||
return None, None
|
||||
from tools.delegate_tool_config import _entry_serves_endpoint
|
||||
base_url = getattr(child, "base_url", None)
|
||||
leased_cred_id = child_pool.acquire_lease()
|
||||
if leased_cred_id is not None:
|
||||
with _quiet("Failed to bind child to leased credential: %s"):
|
||||
leased_entry = child_pool.current()
|
||||
if not _entry_serves_endpoint(leased_entry, base_url):
|
||||
child_pool.release_lease(leased_cred_id)
|
||||
leased_entry = next(
|
||||
(e for e in child_pool.entries() if e.last_status != "dead" and _entry_serves_endpoint(e, base_url)),
|
||||
None,
|
||||
)
|
||||
leased_cred_id = child_pool.acquire_lease(leased_entry.id) if leased_entry is not None else None
|
||||
if leased_entry is not None and hasattr(child, "_swap_credential"):
|
||||
child._swap_credential(leased_entry)
|
||||
return child_pool, leased_cred_id
|
||||
|
||||
@@ -214,6 +214,30 @@ def _loaded_pool(key: Any):
|
||||
pool = load_pool(key)
|
||||
return pool if pool is not None and pool.has_credentials() else None
|
||||
|
||||
def _entry_serves_endpoint(entry: Any, base_url: Any) -> bool:
|
||||
"""Whether a pooled credential may be bound to a child running at ``base_url``. ``_swap_credential`` adopts the
|
||||
entry's base_url too, so a same-provider entry for another endpoint (public OpenAI vs. an Azure resource) would
|
||||
send the child's request — and the entry's key — to the wrong host (#68237). Entries or children without
|
||||
endpoint metadata (legacy adapters, test doubles) cannot rebind and are accepted."""
|
||||
if not isinstance(base_url, str) or not base_url:
|
||||
return True
|
||||
entry_url = getattr(entry, "runtime_base_url", None) or getattr(entry, "base_url", None)
|
||||
if not isinstance(entry_url, str) or not entry_url:
|
||||
return True
|
||||
from hermes_cli.route_identity import normalize_route_base_url
|
||||
return normalize_route_base_url(entry_url) == normalize_route_base_url(base_url)
|
||||
|
||||
def _pool_serves_endpoint(pool: Any, provider: Optional[str], base_url: Optional[str]) -> bool:
|
||||
"""Provider identity AND at least one entry for the child's endpoint; pools without entry metadata pass."""
|
||||
from agent.credential_pool import credential_pool_matches_provider
|
||||
if not credential_pool_matches_provider(pool, provider, base_url=base_url):
|
||||
return False
|
||||
entries_fn = getattr(pool, "entries", None)
|
||||
if not callable(entries_fn):
|
||||
return True
|
||||
entries = entries_fn()
|
||||
return not isinstance(entries, list) or any(_entry_serves_endpoint(entry, base_url) for entry in entries)
|
||||
|
||||
def _resolve_child_credential_pool(
|
||||
effective_provider: Optional[str], parent_agent, effective_base_url: Optional[str] = None,
|
||||
):
|
||||
@@ -244,8 +268,14 @@ def _resolve_child_credential_pool(
|
||||
return parent_pool
|
||||
return _loaded_pool(child_key)
|
||||
if parent_pool is not None and effective_provider == parent_provider:
|
||||
return parent_pool
|
||||
return _loaded_pool(effective_provider)
|
||||
if not effective_base_url or _pool_serves_endpoint(parent_pool, effective_provider, effective_base_url):
|
||||
return parent_pool
|
||||
logger.debug("Parent %s pool has no entry for child endpoint %s; not sharing it",
|
||||
effective_provider, effective_base_url)
|
||||
pool = _loaded_pool(effective_provider)
|
||||
if pool is not None and effective_base_url and not _pool_serves_endpoint(pool, effective_provider, effective_base_url):
|
||||
return None # child keeps its fixed credential
|
||||
return pool
|
||||
except Exception as exc:
|
||||
if effective_provider == "custom":
|
||||
logger.debug("Could not resolve custom credential pool for child endpoint '%s': %s", effective_base_url, exc)
|
||||
|
||||
Reference in New Issue
Block a user