From 24979b2ed6ac0f29bf310a0f5f993c6ce6663e9a Mon Sep 17 00:00:00 2001 From: Kadin Bullock Date: Tue, 7 Jul 2026 09:19:48 -0600 Subject: [PATCH 1/2] fix(mcp): bound merge_recordings and upload_recording waits (#151) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit process_recording's transcript/summary waits already got a soft deadline (_WAIT_TIMEOUT_S) so a disconnected MCP client can't orphan the process for the full 600s poll window. merge_recordings (up to 300s poll) and upload_recording (unbounded multipart S3 loop) still had no cap, so they retained the full orphan window client.py:309/300 called out in the issue. - mcp.py: pass timeout_s=_WAIT_TIMEOUT_S into client.merge_recordings (already had a timeout_s kwarg) and catch the soft-deadline PlaudApiError the same way process_recording does, returning a still_processing-shaped result with the source recording_ids/title (no merged id exists yet). - client.py: add an optional timeout_s to upload_recording's multipart S3 loop, checked once per chunk; None (the CLI's default) preserves the historical unbounded behavior exactly. - transcode.py: thread timeout_s through upload_with_transcode so the MCP facade can opt in without touching the CLI's call site. - mcp.py: upload_recording catches the same soft-deadline error and returns a still_processing-shaped result (title/filename, no recording_id yet — nothing was created). Noted in comments that, unlike merge/transcription, there's no background job continuing after the abort; the upload itself is lost and must be retried. - Extracted _is_soft_deadline_timeout() out of _wait_or_still_processing so all three call sites share one timeout-detection check. True cancellation on client disconnect (threading.Event wired through server.py's asyncio.to_thread) remains out of scope, as before — tracked as a follow-up. Co-Authored-By: Claude Opus 4.8 --- src/plaud_tools/client.py | 24 ++++++++++ src/plaud_tools/mcp.py | 81 ++++++++++++++++++++++++++------- src/plaud_tools/transcode.py | 21 ++++++++- tests/test_interfaces.py | 12 +++-- tests/test_mcp_wave4_surface.py | 18 ++++++++ tests/test_upload.py | 4 +- 6 files changed, 137 insertions(+), 23 deletions(-) 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..4066eb4 100644 --- a/src/plaud_tools/mcp.py +++ b/src/plaud_tools/mcp.py @@ -246,24 +246,39 @@ 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 minimum viable fix; true cancellation +# on client disconnect is a deeper change tracked as follow-up. 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). _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 +618,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 +721,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 From 91a9f0e848c2ee743004f7bfbfda47a27f4fc25c Mon Sep 17 00:00:00 2001 From: Kadin Bullock Date: Tue, 7 Jul 2026 09:28:39 -0600 Subject: [PATCH 2/2] docs(#151): settle cancel-on-disconnect as YAGNI, not a follow-up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Maintainer decision (2026-07-07): the 90s soft deadline is the accepted resolution for #151, not a stopgap. Reword the mcp.py comment and the ADR-006 note so neither reads as unfinished work: - mcp.py: replace "true cancellation ... tracked as follow-up" with a ponytail: note stating the soft deadline is the accepted fix and the threading.Event cancel path is intentionally not implemented (YAGNI), naming it only as an upgrade path if orphan telemetry ever shows 90s matters. - ADR-006: append a dated (2026-07-07) update line noting the soft deadline now covers process/merge/upload and full cancellation was deemed YAGNI and closed — not rewriting the original follow-up text, just settling it. Co-Authored-By: Claude Opus 4.8 --- docs/adr/006-mcp-surface-v070-breaking-batch.md | 9 +++++++++ src/plaud_tools/mcp.py | 17 ++++++++++++----- 2 files changed, 21 insertions(+), 5 deletions(-) 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/mcp.py b/src/plaud_tools/mcp.py index 4066eb4..8938ae2 100644 --- a/src/plaud_tools/mcp.py +++ b/src/plaud_tools/mcp.py @@ -249,11 +249,18 @@ def _count_summary_matches(client: PlaudClient, recording_id: str, find: str) -> # 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 minimum viable fix; true cancellation -# on client disconnect is a deeper change tracked as follow-up. 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). +# "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