Skip to content

Commit ed942e2

Browse files
authored
fix: let healthy Copilot completions wait without a default deadline (#86)
1 parent 6cf56a0 commit ed942e2

15 files changed

Lines changed: 373 additions & 35 deletions

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -325,7 +325,7 @@ Emitted immediately before the SDK call.
325325
| `message_count` | int | Number of messages in the conversation |
326326
| `tool_count` | int | Number of tools available |
327327
| `streaming` | bool | Whether streaming is enabled (default: `true`) |
328-
| `timeout` | float | Request timeout in seconds |
328+
| `timeout` | float or null | Explicit request timeout in seconds; null waits for completion, cancellation or failure |
329329

330330
### `llm:response`
331331

‎amplifier_module_provider_github_copilot/config/_models.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,8 @@
3232
"defaults": {
3333
"model": "auto",
3434
"max_tokens": 4096,
35-
"timeout": 3600,
35+
# Healthy generation has no implicit elapsed-time deadline.
36+
"timeout": None,
3637
"context_window": 128000,
3738
"max_output_tokens": 16384,
3839
},

‎amplifier_module_provider_github_copilot/config/_sdk_protection.py‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,10 @@ class SessionProtectionConfig:
187187
# Contract: streaming-contract:abort-on-capture:MUST:1
188188
idle_timeout_seconds: float = 30.0
189189

190+
# Public SDK ping cadence while waiting for completion. This is not a timeout:
191+
# a slow ping or long generation is never failed because of elapsed time.
192+
connection_check_interval_seconds: float = 1.0
193+
190194
# Timeout for session.disconnect() call.
191195
# Prevents indefinite hang on stubborn sessions during cleanup.
192196
# Contract: sdk-protection:Session:MUST:3

‎amplifier_module_provider_github_copilot/observability.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ async def emit_request(
156156
message_count: int,
157157
tool_count: int,
158158
streaming: bool,
159-
timeout: float,
159+
timeout: float | None,
160160
raw_request: dict[str, Any] | None = None,
161161
) -> None:
162162
"""Emit llm:request event.

‎amplifier_module_provider_github_copilot/provider.py‎

Lines changed: 41 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,9 @@
2525
import logging
2626
import time
2727
import uuid
28+
from contextlib import suppress
2829
from dataclasses import dataclass, field
30+
from inspect import iscoroutinefunction
2931
from typing import Any, cast
3032

3133
from amplifier_core import (
@@ -586,6 +588,8 @@ def get_info(self) -> ProviderInfo:
586588
# are already corrected for the effective model just below.
587589
# Contract: provider-protocol:get_info:MUST:2
588590
defaults["model"] = self._effective_default_model
591+
configured_timeout = self.config.get("timeout", defaults["timeout"])
592+
defaults["timeout"] = None if configured_timeout is None else float(configured_timeout)
589593
# Report the default model's tier-selected PROMPT budget so Amplifier's
590594
# compaction math matches the active context tier. Cold cache (model not
591595
# yet discovered) keeps the static fallback from config/_models.py.
@@ -800,10 +804,11 @@ async def complete(
800804
async with llm_lifecycle(self.coordinator, model, raw=self._raw) as ctx:
801805
# Real SDK path: use client wrapper with STREAMING
802806
# Three-Medium: timeout from YAML config
803-
timeout_seconds: float = kwargs.get(
807+
configured_timeout = kwargs.get(
804808
"_timeout_seconds",
805-
float(self._provider_config.defaults["timeout"]),
809+
self.config.get("timeout", self._provider_config.defaults["timeout"]),
806810
)
811+
timeout_seconds = None if configured_timeout is None else float(configured_timeout)
807812

808813
retry_config = self._retry_config
809814

@@ -973,11 +978,6 @@ async def complete(
973978
)
974979
accumulator = StreamingAccumulator()
975980

976-
# Three-Medium: timeout from YAML config
977-
timeout_seconds_retry: float = kwargs.get(
978-
"_timeout_seconds",
979-
float(self._provider_config.defaults["timeout"]),
980-
)
981981
try:
982982
# Note: attachments=None for correction - the model already saw the image
983983
# Correction responses only need to produce a structured tool call —
@@ -992,7 +992,7 @@ async def complete(
992992
client=self._client,
993993
model=model,
994994
prompt=corrected_prompt,
995-
timeout=timeout_seconds_retry,
995+
timeout=timeout_seconds,
996996
event_config=event_config,
997997
accumulator=accumulator,
998998
tools=internal_request.tools or None,
@@ -1151,7 +1151,7 @@ async def _execute_sdk_completion(
11511151
client: CopilotClientWrapper,
11521152
model: str,
11531153
prompt: str,
1154-
timeout: float,
1154+
timeout: float | None,
11551155
event_config: EventConfig,
11561156
accumulator: StreamingAccumulator,
11571157
tools: list[Any] | None = None,
@@ -1178,7 +1178,7 @@ async def _execute_sdk_completion(
11781178
"""
11791179
# DEBUG: Log entry point with key parameters
11801180
logger.debug(
1181-
"[SDK_COMPLETION] Starting: model=%s, prompt_len=%d, timeout=%.1f, "
1181+
"[SDK_COMPLETION] Starting: model=%s, prompt_len=%d, timeout=%s, "
11821182
"tools=%d, attachments=%d, idle_events=%s, system_message_len=%d",
11831183
model,
11841184
len(prompt),
@@ -1261,6 +1261,21 @@ async def _execute_sdk_completion(
12611261
)
12621262

12631263
unsubscribe = sdk_session.on(event_handler)
1264+
# The SDK's public send() acknowledges submission, so pipe failure
1265+
# afterwards may not emit session.error. A separate public ping
1266+
# watcher detects real connection failure without a generation deadline.
1267+
connection_task: asyncio.Task[None] | None = None
1268+
watch_connection = getattr(sdk_session, "wait_for_disconnect", None)
1269+
if iscoroutinefunction(watch_connection):
1270+
async def watch() -> None:
1271+
try:
1272+
await watch_connection()
1273+
except Exception as exc:
1274+
if not idle_event.is_set():
1275+
error_holder.append(exc)
1276+
idle_event.set()
1277+
1278+
connection_task = asyncio.create_task(watch())
12641279
try:
12651280
# Record TTFT start time before send
12661281
# Contract: behaviors:Streaming:MUST:1
@@ -1391,8 +1406,24 @@ async def _execute_sdk_completion(
13911406
stream_ctx.signal_done()
13921407
if consumer_task is not None:
13931408
await consumer_task
1409+
except asyncio.CancelledError:
1410+
# User cancellation and explicit caller deadlines terminate
1411+
# native work too; cleanup bounds do not limit healthy generation.
1412+
try:
1413+
await asyncio.wait_for(
1414+
sdk_session.abort(),
1415+
timeout=sdk_protection.session.abort_timeout_seconds,
1416+
)
1417+
except Exception:
1418+
logger.debug("[provider] Abort during cancellation failed; disconnecting")
1419+
raise
13941420
finally:
13951421
unsubscribe()
1422+
for task in (connection_task, consumer_task):
1423+
if task is not None:
1424+
task.cancel()
1425+
with suppress(asyncio.CancelledError):
1426+
await task
13961427

13971428
# =========================================================================
13981429
# Progressive Streaming Emission

‎amplifier_module_provider_github_copilot/sdk_adapter/client.py‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -702,7 +702,14 @@ async def session(
702702
from .types import SessionHandle
703703

704704
session_id = str(getattr(sdk_session, "session_id", "unknown"))
705-
yield SessionHandle(sdk_session, session_id)
705+
yield SessionHandle(
706+
sdk_session,
707+
session_id,
708+
ping=getattr(client, "ping", None),
709+
connection_check_interval=(
710+
load_sdk_protection_config().session.connection_check_interval_seconds
711+
),
712+
)
706713
finally:
707714
if sdk_session is not None:
708715
try:

‎amplifier_module_provider_github_copilot/sdk_adapter/types.py‎

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
Contract: contracts/sdk-boundary.md
77
"""
88

9+
import asyncio
910
import logging
1011
from collections.abc import Awaitable, Callable
1112
from dataclasses import dataclass, field
@@ -71,9 +72,16 @@ class SessionHandle:
7172
session_id: Opaque string identifier for the session
7273
"""
7374

74-
__slots__ = ("_raw_session", "session_id")
75+
__slots__ = ("_raw_session", "session_id", "_ping", "_connection_check_interval")
7576

76-
def __init__(self, raw_session: Any, session_id: str | None = None) -> None:
77+
def __init__(
78+
self,
79+
raw_session: Any,
80+
session_id: str | None = None,
81+
*,
82+
ping: Callable[[], Awaitable[Any]] | None = None,
83+
connection_check_interval: float = 1.0,
84+
) -> None:
7785
"""Create a session handle wrapping a raw SDK session.
7886
7987
Args:
@@ -82,6 +90,23 @@ def __init__(self, raw_session: Any, session_id: str | None = None) -> None:
8290
"""
8391
self._raw_session = raw_session
8492
self.session_id = session_id or getattr(raw_session, "session_id", "unknown")
93+
self._ping = ping
94+
self._connection_check_interval = connection_check_interval
95+
96+
async def wait_for_disconnect(self) -> None:
97+
"""Watch real SDK connection errors without imposing any response deadline.
98+
99+
Copilot's public ping uses the JSON-RPC client's unlimited default wait.
100+
A slow ping is not treated as failure. Pipe closure or a failed write
101+
raises; a successful ping merely schedules the next check. Cancellation
102+
removes any pending SDK request through its normal finally cleanup.
103+
"""
104+
if self._ping is None:
105+
await asyncio.Event().wait()
106+
return
107+
while True:
108+
await self._ping()
109+
await asyncio.sleep(self._connection_check_interval)
85110

86111
def on(self, handler: Callable[[Any], None]) -> Callable[[], None]:
87112
"""Subscribe to session events.

‎contracts/sdk-boundary.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ class SessionWrapper:
155155
```python
156156
# ✓ Façade hides the SDK session; raw reference is private
157157
class SessionHandle:
158-
__slots__ = ("_raw_session", "session_id")
158+
__slots__ = ("_raw_session", "session_id", "_ping", "_connection_check_interval")
159159

160160
def __init__(self, raw_session: Any, session_id: str | None = None) -> None:
161161
self._raw_session = raw_session # private — never exposed
@@ -189,7 +189,7 @@ class SessionHandle:
189189
```
190190

191191
- `Types:MUST:4` — **MUST** keep `_raw_session` private: no public attribute, no accessor returning it (`sdk_adapter/types.py:74,83`).
192-
- `Types:MUST:5` — **MUST** expose only `on(handler) → unsubscribe`, `await send(prompt, *, attachments=None)`, `await abort()`, plus the attribute `session_id: str` (`sdk_adapter/types.py:86-120`).
192+
- `Types:MUST:5` — **MUST** expose only `on(handler) → unsubscribe`, `await send(prompt, *, attachments=None)`, `await abort()`, `await wait_for_disconnect()`, plus the attribute `session_id: str`. The connection waiter uses an injected public SDK `ping` callable and raises only for real transport failures; it must never convert elapsed time into failure or expose the raw client/session.
193193
- `Types:MUST:6` — **MUST NOT** expose lifecycle (`connect` / `disconnect` / `destroy` / `close`) on the handle; lifecycle is owned by the `client.session()` async context manager in `sdk_adapter/client.py`.
194194
- `Types:MUST:7` — **MUST** assign `session_id` exactly once in `__init__` from the raw SDK session (`copilot.session.CopilotSession.session_id`); provider code MUST NOT reassign it. Callers **SHOULD** treat it as read-only — `__slots__` constrains attribute names but does not block reassignment of declared slots; a `__setattr__` guard or `frozen` dataclass is tracked as a code-level follow-up.
195195

‎contracts/sdk-protection.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,11 +65,11 @@ The provider MUST bound the abort call by `session.abort_timeout_seconds`. If ab
6565

6666
### SHOULD-2: Idle Timeout (Safety Bound Only)
6767

68-
The provider SHOULD NOT use `session.idle_timeout_seconds` for the main idle wait. SDK API calls can take 60+ seconds for complex operations (e.g., agent delegation). Use the caller's request timeout instead.
68+
The provider MUST NOT impose a default elapsed-time deadline on healthy generation. `defaults.timeout` is `None`; an explicit per-call `_timeout_seconds` or provider `timeout` value still sets a deadline. An explicit per-call `None` disables an inherited deadline. The provider MUST NOT use `session.idle_timeout_seconds` for the main idle wait.
6969

7070
The `idle_timeout_seconds` config is retained for abort operations only.
7171

72-
**Implementation:** `asyncio.wait_for(idle_event.wait(), timeout=timeout)` — uses caller's timeout
72+
**Implementation:** one outer `asyncio.timeout(timeout)` with `None` by default; await `idle_event.wait()` directly. The SDK `send()` RPC has a genuine unlimited default. A public SDK `ping()` watcher detects actual connection errors after submission without assigning a ping deadline. Slow pings are not failures. On explicit cancellation or deadline expiration, abort native work, unsubscribe handlers, cancel/join the connection watcher and stream consumer, then disconnect. Existing bounded abort/disconnect/close cleanup remains in force.
7373

7474
---
7575

‎tests/test_config_driven_timeout.py‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -178,8 +178,8 @@ async def send(
178178
class TestTimeoutConfigValue:
179179
"""Tests for timeout configuration values."""
180180

181-
def test_yaml_timeout_is_3600(self) -> None:
182-
"""YAML timeout value is 3600 (1 hour for reasoning models).
181+
def test_default_completion_timeout_is_unlimited(self) -> None:
182+
"""Healthy generation has no default elapsed-time cutoff.
183183
184184
Contract: behaviors:Config:MUST:2
185185
Three-Medium: YAML is authoritative source.
@@ -190,4 +190,4 @@ def test_yaml_timeout_is_3600(self) -> None:
190190

191191
config = load_models_config()
192192
timeout = config.defaults["timeout"]
193-
assert timeout == 3600
193+
assert timeout is None

0 commit comments

Comments
 (0)