diff --git a/docs/adr/006-mcp-surface-v070-breaking-batch.md b/docs/adr/006-mcp-surface-v070-breaking-batch.md index 94db21c..33437dc 100644 --- a/docs/adr/006-mcp-surface-v070-breaking-batch.md +++ b/docs/adr/006-mcp-surface-v070-breaking-batch.md @@ -96,6 +96,15 @@ cancellation on client disconnect (a shutdown `Event` threaded through the handler) is deeper and deferred; the soft-deadline return is the load-bearing mitigation. +> **Update (2026-07-07, #151/#185):** the soft deadline now also bounds +> `merge_recordings` (300 s poll loop) and `upload_recording` (multipart S3 +> loop), covering all three long-blocking handlers. Full cancel-on-disconnect +> was deemed **YAGNI** and closed, not deferred: the ~90 s cap is already below +> the exe-lock contention window the updater fights, so wiring a shutdown +> `Event` through `server.py`'s `asyncio.to_thread` buys nothing measurable. +> The soft-deadline return is the accepted resolution for #151; the `Event` +> path is the documented upgrade if orphan telemetry ever shows 90 s matters. + ### Session-expired remediation strings (§6.2) MCP session-expired errors now append "Tell the user to open the PlaudTools diff --git a/src/plaud_tools/client.py b/src/plaud_tools/client.py index 67619c1..684861a 100644 --- a/src/plaud_tools/client.py +++ b/src/plaud_tools/client.py @@ -140,6 +140,7 @@ def upload_recording( *, start_time: int | None = ..., timezone_offset: float | None = ..., + timeout_s: float | None = ..., ) -> Recording: ... @overload @@ -151,6 +152,7 @@ def upload_recording( *, start_time: int | None = ..., timezone_offset: float | None = ..., + timeout_s: float | None = ..., ) -> Recording: ... def upload_recording( @@ -161,6 +163,7 @@ def upload_recording( *, start_time: int | None = None, timezone_offset: float | None = None, + timeout_s: float | None = None, ) -> Recording: """4-step upload: presign → S3 multipart PUT → merge_multipart → confirm_upload. @@ -176,6 +179,14 @@ def upload_recording( start_time_ms: millisecond epoch for the recording's date. Defaults to now. Plaud respects whatever value the client sends — pass the original recording's timestamp to preserve the date after re-upload. + + timeout_s: (#151) optional overall wall-clock budget for the S3 chunk + loop below. ``None`` (the default) preserves the historical unbounded + behaviour — each individual PUT already has its own 120 s ceiling + (see ``_s3_put``), so this only matters for many-chunk files on a slow + link. Callers that can retry safely (the MCP facade) pass a soft + deadline so a disconnected client doesn't orphan the process for the + full multipart transfer; the CLI leaves it unset. """ if not filename.strip(): raise ValueError("filename cannot be empty") @@ -227,10 +238,20 @@ def upload_recording( # per part, avoiding a full in-memory buffer. For the bytes variant we # slice the existing buffer as before (no behaviour change for callers # that already have bytes in hand). + # (#151) Soft deadline across the whole multipart loop — a many-chunk + # file on a slow link can accumulate a long total even though each + # individual PUT stays under its own 120 s ceiling. ``None`` (the + # CLI's default) skips the check entirely, so this cannot regress + # unbounded callers. + deadline = None if timeout_s is None else time.time() + timeout_s + parts: list[dict[str, Any]] = [] if isinstance(data, Path): with data.open("rb") as fh: for i, url in enumerate(part_urls): + if deadline is not None and time.time() >= deadline: + assert timeout_s is not None # deadline is only ever set from timeout_s + raise PlaudApiError(f"upload timed out after {int(timeout_s)}s") chunk = fh.read(_CHUNK_SIZE) if not chunk: # Presign returned more part URLs than the file has @@ -247,6 +268,9 @@ def upload_recording( parts.append({"Etag": etag, "PartNumber": i + 1}) else: for i, url in enumerate(part_urls): + if deadline is not None and time.time() >= deadline: + assert timeout_s is not None # deadline is only ever set from timeout_s + raise PlaudApiError(f"upload timed out after {int(timeout_s)}s") start_byte = i * _CHUNK_SIZE end_byte = min(start_byte + _CHUNK_SIZE, len(data)) chunk = data[start_byte:end_byte] diff --git a/src/plaud_tools/mcp.py b/src/plaud_tools/mcp.py index 1172425..8938ae2 100644 --- a/src/plaud_tools/mcp.py +++ b/src/plaud_tools/mcp.py @@ -246,24 +246,46 @@ def _count_summary_matches(client: PlaudClient, recording_id: str, find: str) -> # (10 min) each by default — a wait="summary" call can block a handler for # ~20 min total, long enough that a disconnected MCP client orphans the # process holding the exe lock the updater fights. Most MCP clients time out -# long before that (60-120s). Bounding the wait here to a soft deadline and -# reporting "still_processing" on timeout is the minimum viable fix; true -# cancellation on client disconnect is a deeper change tracked as follow-up. +# long before that (60-120s), so waiting any longer server-side is pointless +# regardless of which tool is blocking — the caller already gave up. Bounding +# every long wait here to the same soft deadline and reporting +# "still_processing" on timeout is the accepted resolution for #151 (settled +# 2026-07-07). Reused below for merge_recordings (client.py's poll-loop wait, +# same shape as wait_for_transcription/summary) and upload_recording +# (client.py's multipart S3 loop, bounded by wall-clock rather than a poll +# interval). +# +# ponytail: true cancel-on-disconnect (a shutdown threading.Event threaded +# through server.py's asyncio.to_thread and checked between poll iterations) +# is intentionally NOT implemented — the soft deadline already caps the orphan +# window at ~90s, which is below the exe-lock contention the updater cares +# about. Upgrade path if orphan telemetry ever shows 90s still matters: wire +# that Event; until then it's YAGNI. _WAIT_TIMEOUT_S = 90.0 +def _is_soft_deadline_timeout(exc: PlaudApiError) -> bool: + """True if *exc* is one of client.py's soft-deadline timeouts (#151). + + Those are raised with no ``http_status`` and a message ending + "timed out after Ns" — see ``wait_for_transcription``, ``wait_for_summary``, + ``merge_recordings``, and ``upload_recording``. Any other error (auth + failure, 404, non-retryable API error) is not a timeout and must propagate. + """ + return exc.http_status is None and "timed out" in str(exc) + + def _wait_or_still_processing(wait_fn: Callable[..., None], recording_id: str) -> bool: """Call a client wait_for_* method bounded by ``_WAIT_TIMEOUT_S``. Returns True if the wait completed normally, False if it hit the soft - deadline (surfaced by the client as a ``PlaudApiError`` with no - http_status whose message ends "timed out after Ns"). Any other error - (auth failure, 404, non-retryable API error) propagates unchanged. + deadline. Any other error propagates unchanged (see + ``_is_soft_deadline_timeout``). """ try: wait_fn(recording_id, timeout_s=_WAIT_TIMEOUT_S) except PlaudApiError as exc: - if exc.http_status is None and "timed out" in str(exc): + if _is_soft_deadline_timeout(exc): return False raise return True @@ -603,14 +625,34 @@ def inner(client: PlaudClient) -> dict[str, Any]: # (ffmpeg failure) are intentionally not caught here — they # propagate to _call's except clauses, which already map them to # the correct structured {error_code, retryable} shape (#150). - outcome = upload_with_transcode( - client, - path, - rec_title, - start_time=start_ms, - timezone_offset=timezone_offset, - folder_id=folder_id, - ) + try: + outcome = upload_with_transcode( + client, + path, + rec_title, + start_time=start_ms, + timezone_offset=timezone_offset, + folder_id=folder_id, + timeout_s=_WAIT_TIMEOUT_S, # (#151) bound the S3 multipart wait too + ) + except PlaudApiError as exc: + if _is_soft_deadline_timeout(exc): + # Unlike merge/transcription/summary, there is no + # server-side job to check back on — a timed-out upload + # has to be retried, not polled. We still use the + # "still_processing"-shaped response (rather than an + # error) so a disconnected client's already-abandoned + # call doesn't matter, and a connected caller gets a + # structured, retryable signal instead of a raw timeout. + return _json_result( + { + "title": rec_title, + "filename": path.name, + "status": "still_processing", + "retryable": True, + } + ) + raise payload: dict[str, Any] = { "ok": True, "recording_id": outcome.recording.id, @@ -686,7 +728,21 @@ def merge_recordings( title: str, ) -> dict[str, Any]: def inner(client: PlaudClient) -> dict[str, Any]: - detail = client.merge_recordings(recording_ids, title) + try: + # (#151) merge_recordings' own poll loop defaults to a 300s + # deadline (client.py) — bound it to _WAIT_TIMEOUT_S here for + # the same reason process_recording's waits are bounded. + detail = client.merge_recordings(recording_ids, title, timeout_s=_WAIT_TIMEOUT_S) + except PlaudApiError as exc: + if _is_soft_deadline_timeout(exc): + # The merge task keeps running server-side (it's a + # task_id-backed job, like transcription/summary) — no + # merged recording_id exists yet, so report the source + # ids/title so the caller knows what's still in flight. + return _json_result( + {"recording_ids": recording_ids, "title": title, "status": "still_processing"} + ) + raise # Slim response: a fresh merge's detail dict is all nulls besides # id/filename (no transcript/summary yet), so the full # _summarize_detail() shape is dead weight. diff --git a/src/plaud_tools/transcode.py b/src/plaud_tools/transcode.py index a678527..dd64ed9 100644 --- a/src/plaud_tools/transcode.py +++ b/src/plaud_tools/transcode.py @@ -147,6 +147,7 @@ def upload_with_transcode( start_time: int | None = None, timezone_offset: float | None = None, folder_id: str | None = None, + timeout_s: float | None = None, ) -> UploadOutcome: """Upload *path* to Plaud, transcoding first if the format requires it. @@ -159,6 +160,12 @@ def upload_with_transcode( failure at that step does NOT raise — it is reported via :attr:`UploadOutcome.folder_error` so the caller never loses the already-created recording id (see module docstring / issue #149). + + timeout_s: (#151) forwarded to ``PlaudClient.upload_recording``'s soft + deadline on the S3 multipart loop. ``None`` (the CLI's default) is + unbounded, matching pre-existing behaviour; the MCP facade passes a + bounded value so a disconnected client can't orphan the process for the + whole transfer. """ if not path.exists(): raise ValueError(f"file not found: {path}") @@ -173,7 +180,12 @@ def upload_with_transcode( try: transcode_to_mp3_path(path, tmp_mp3_path) recording = client.upload_recording( - tmp_mp3_path, title, file_type, start_time=start_time, timezone_offset=timezone_offset + tmp_mp3_path, + title, + file_type, + start_time=start_time, + timezone_offset=timezone_offset, + timeout_s=timeout_s, ) finally: try: @@ -182,7 +194,12 @@ def upload_with_transcode( pass else: recording = client.upload_recording( - path, title, file_type, start_time=start_time, timezone_offset=timezone_offset + path, + title, + file_type, + start_time=start_time, + timezone_offset=timezone_offset, + timeout_s=timeout_s, ) folder_error: str | None = None diff --git a/tests/test_interfaces.py b/tests/test_interfaces.py index f9a2e66..199e37c 100644 --- a/tests/test_interfaces.py +++ b/tests/test_interfaces.py @@ -57,7 +57,7 @@ def list_recordings(self, query=None): # return all records so client-side filtering can narrow them. return list(self._ALL_RECORDINGS) - def merge_recordings(self, ids: list[str], filename: str): + def merge_recordings(self, ids: list[str], filename: str, **kwargs): from plaud_tools.models import RecordingDetail return RecordingDetail( @@ -76,7 +76,7 @@ def merge_recordings(self, ids: list[str], filename: str): raw={}, ) - def upload_recording(self, data, filename, file_type, *, start_time=None, timezone_offset=None): + def upload_recording(self, data, filename, file_type, *, start_time=None, timezone_offset=None, **kwargs): from plaud_tools.models import Recording return Recording( @@ -1240,7 +1240,9 @@ def test_mcp_upload_recording_passes_timestamp(tmp_path): captured = {} class TimestampCapturingClient(StubClient): - def upload_recording(self, data, filename, file_type, *, start_time=None, timezone_offset=None): + def upload_recording( + self, data, filename, file_type, *, start_time=None, timezone_offset=None, **kwargs + ): captured["start_time"] = start_time captured["timezone_offset"] = timezone_offset return super().upload_recording(data, filename, file_type) @@ -1300,7 +1302,9 @@ def test_mcp_upload_recording_start_time_as_iso_string(tmp_path): captured = {} class IsoCapturingClient(StubClient): - def upload_recording(self, data, filename, file_type, *, start_time=None, timezone_offset=None): + def upload_recording( + self, data, filename, file_type, *, start_time=None, timezone_offset=None, **kwargs + ): captured["start_time"] = start_time return super().upload_recording(data, filename, file_type) diff --git a/tests/test_mcp_wave4_surface.py b/tests/test_mcp_wave4_surface.py index e64c6e6..617dc7c 100644 --- a/tests/test_mcp_wave4_surface.py +++ b/tests/test_mcp_wave4_surface.py @@ -308,6 +308,24 @@ def test_response_is_slim_ok_recording_id_title(self): payload = json.loads(result["content"][0]["text"]) assert payload == {"ok": True, "recording_id": "merged1", "title": "Combined"} + def test_wait_timeout_returns_still_processing(self): + # (#151) merge_recordings' own poll loop (up to 300s by default) is + # now bounded the same way process_recording's waits are — a soft + # deadline that reports still_processing instead of blocking the + # handler thread for the full window. + mock_client = MagicMock() + mock_client.merge_recordings.side_effect = PlaudApiError("merge timed out after 90s") + handlers = build_handlers(lambda: mock_client) + + result = handlers["merge_recordings"](recording_ids=["r1", "r2"], title="Combined") + + payload = json.loads(result["content"][0]["text"]) + assert payload == {"recording_ids": ["r1", "r2"], "title": "Combined", "status": "still_processing"} + assert "isError" not in result + mock_client.merge_recordings.assert_called_once_with( + ["r1", "r2"], "Combined", timeout_s=_WAIT_TIMEOUT_S + ) + # --------------------------------------------------------------------------- # Compact JSON — no indentation/space tax on every response diff --git a/tests/test_upload.py b/tests/test_upload.py index db94469..3d6b9dd 100644 --- a/tests/test_upload.py +++ b/tests/test_upload.py @@ -864,7 +864,9 @@ def __init__(self, *, folder_move_error: Exception | None = None): self._folder_move_error = folder_move_error self._next_id = 0 - def upload_recording(self, data, filename, file_type, *, start_time=None, timezone_offset=None): + def upload_recording( + self, data, filename, file_type, *, start_time=None, timezone_offset=None, timeout_s=None + ): from plaud_tools.models import Recording self._next_id += 1