close(): bound client close so an unclosable HTTP client cannot hang session cleanup

[Brian Krabach (bkrabach)]

Defect

Two unbounded awaits on the mount()-cleanup path, either of which hangs Amplifier's session cleanup for the entire process:

1. CopilotClientWrapper.close() — await self._owned_client.stop(), no ceiling. stop() tears down the SDK's ~500MB Electron subprocess; a wedged or unresponsive subprocess leaves that await pending forever. Three call sites reach it: provider.py:1511 (GitHubCopilotProvider.close()), and both shared-client refcount paths in __init__.py (_acquire_shared_client replacing an unhealthy client, _release_shared_client dropping the last reference).

2. GitHubCopilotProvider.cancel_emit_tasks() — await asyncio.gather(*tasks_to_cancel, ...), no ceiling. Cancelling a task is a request, not a guarantee: a task that swallows CancelledError (or is blocked in a shielded await) never completes and wedges the drain. provider.close() awaits this before touching the client, and __init__.py:578's cleanup calls it directly.

This repo already treats unbounded awaits as a defect class elsewhere — session.abort_timeout_seconds, session.disconnect_timeout_seconds, singleton.lock_timeout_seconds all exist for exactly this reason. Cleanup was the gap.

Root cause

On origin/main, commit bc8d6a9:

# sdk_adapter/client.py:719-730
async def close(self) -> None:
    self._stopped = True
    if self._owned_client is not None:
        try:
            await self._owned_client.stop()        # unbounded — no ceiling
        ...

# provider.py:1493-1496
if tasks_to_cancel:
    await asyncio.gather(*tasks_to_cancel, return_exceptions=True)   # unbounded

The SDK's own graceful-shutdown bound (_RUNTIME_SHUTDOWN_TIMEOUT_SECONDS, added v1.0.2 — see the pyproject.toml dependency note) is the SDK's promise, not this provider's, and it only covers stop(). It does not cover the emit drain, and it is not a guarantee this module can make across SDK versions.

Change

New contract clause sdk-protection:Subprocess:MUST:8 — every await on the cleanup path is bounded by sdk.close_timeout_seconds (new, default 5.0s, added to SdkConfig in config/_sdk_protection.py, the module's policy source of truth).

await asyncio.wait_for(asyncio.shield(<awaitable>), timeout=close_timeout)

• CopilotClientWrapper.close() — bounds stop(). Fixed at the wrapper because it is the single choke point all three call sites funnel through; one bound covers provider.close() and both refcount paths, instead of three drifting copies.
• cancel_emit_tasks() — bounds the drain gather, and reports how many tasks were abandoned.
• shield lets the operation finish even when the enclosing task is cancelled; wait_for caps the wait.
• On timeout: WARNING naming the abandoned resource, then return. Cleanup never raises.
• self._owned_client is cleared before the await, so a client that timed out or raised is never retried on a second close(). Idempotency is preserved (test_close_idempotent passes unchanged).
• CancelledError still propagates exactly as on main — it inherits BaseException, so the existing except Exception never caught it and still does not.
• contracts/sdk-protection.md updated: MUST-8 clause with rationale and implementation, config table row, traceability row, changelog entry.

Gates

uv run pytest -q (exactly what CI runs) → 1563 passed, 12 deselected. Full suite green.

make test equivalent (pytest tests/ -q --tb=short -m "not live") → same 1563 passed.

ruff check . → All checks passed (branch and base).

ruff format --check . → 21 files would be reformatted on this branch, 21 on unmodified origin/main — identical. My edits to provider.py and sdk_adapter/client.py are format-clean; the one pre-existing offender inside tests/test_client_lifecycle.py (line ~1262, untouched by this PR) was deliberately left alone rather than reformatted into diff noise.

pyright amplifier_module_provider_github_copilot tests → exit 0, 0 errors.

New tests:

test	asserts
test_client_lifecycle.py::test_close_is_bounded_when_stop_never_returns	a never-returning stop() returns in <2s, logs the WARNING, drops _owned_client, leaves is_healthy() False, does not raise
test_client_lifecycle.py::test_close_normal_stop_logs_no_warning	well-behaved client stopped exactly once, no warning, released
test_client_lifecycle.py::test_close_timeout_defaults_to_five_seconds	shipped policy default is 5.0s
test_provider_close.py::test_cancel_emit_tasks_is_bounded	a task that swallows CancelledError cannot wedge the drain
test_provider_close.py::test_cancel_emit_tasks_cooperative_logs_no_warning	a cooperative task drains quietly and is actually cancelled

Evidence the tests actually catch the defect

Both new bounded tests were run against the unmodified origin/main client.py and provider.py, each under a 25s wall-clock cap:

timeout 25 pytest -k bounded_when_stop_never_returns    → exit 124   # HUNG
timeout 25 pytest -k cancel_emit_tasks_is_bounded       → exit 124   # HUNG

Both against this branch:

3 passed, 66 deselected in 0.15s

Two independent 124s confirms these are two separate hangs, not one defect seen twice — which is why bounding only stop() would have left the process hangable.

Logs: dtu-artifacts/sweep/prov-others/copilot-{regression-proof-BASE-clientstop,regression-proof-BASE-emitdrain,regression-proof-FIXED,full-FIXED,pyright-FIXED}.log

No live run

Deliberate: the live end-to-end proof for this defect class is carried by the sibling prov-anthropic lane, which exercises the same contract against a real provider session. Live-marked tests here are deselected in CI by design; the hang-vs-bounded reproduction above is stronger for these specific code paths than a live session that happens not to wedge.

Cross-references

• Work items recipes-7nj / recipes-8sr
• Sibling lane: the same bounded-close contract on microsoft/amplifier-module-provider-anthropic
• Family sweep, one PR per repo: chat-completions (close(): bound client close so an unclosable HTTP client cannot hang session cleanup amplifier-module-provider-chat-completions#19), openai (close(): bound client close so an unclosable HTTP client cannot hang session cleanup amplifier-module-provider-openai#85), vllm (close(): bound client close so an unclosable HTTP client cannot hang session cleanup amplifier-module-provider-vllm#42), azure-openai (close(): bound client close so an unclosable HTTP client cannot hang session cleanup amplifier-module-provider-azure-openai#39). ollama, gemini, and openai-chatgpt need no change — none of them awaits a client close at all.
• This repo is the one member of the family whose hang is a subprocess teardown rather than an HTTP transport close, which is why the bound lives in the policy module rather than a provider config key.