fix(mcp): bound merge_recordings and upload_recording waits (#151)

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

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]

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.

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

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)
 

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

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