fix(mcp): bound merge_recordings and upload_recording waits (#151)#185
Merged
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #151
What
Extends the soft-deadline pattern already added for
process_recording's transcript/summary waits (mcp.py's_WAIT_TIMEOUT_S/_wait_or_still_processing, see the prior partial fix) to the two remaining unbounded blocking waits the issue calls out:merge_recordings:client.merge_recordingsalready had atimeout_skwarg (default 300s) — the MCP handler now passestimeout_s=_WAIT_TIMEOUT_Sand, on the resulting soft-deadlinePlaudApiError, returns{"recording_ids": ..., "title": ..., "status": "still_processing"}instead of blocking the handler thread for the full window. The merge task keeps running server-side (it's task_id-backed, same shape as transcription/summary), so "still_processing" is an accurate status here.upload_recording: the multipart S3 chunk loop inclient.pyhad no overall bound (only a per-part 120s ceiling). Added an optionaltimeout_sparam, checked once per chunk, threaded throughtranscode.upload_with_transcodeso the MCP facade can opt in (timeout_s=_WAIT_TIMEOUT_S) without changing the CLI's call site —None(the CLI default) is fully unbounded, preserving existing behavior exactly.Also extracted
_is_soft_deadline_timeout()out of_wait_or_still_processingso all three call sites (transcript/summary wait, merge, upload) share one "is this our soft-deadline timeout" check instead of duplicating thehttp_status is None and "timed out" in str(exc)test.Important caveat (called out in code comments)
Unlike merge/transcription/summary, an aborted
upload_recordinghas no background job to check back on — nothing was created on Plaud's side yet, so a timed-out upload is genuinely lost and must be retried, not polled. The response still uses thestill_processing-shaped envelope for consistency with the other bounded waits, but this is a real semantic difference worth flagging for review.Scope: soft deadline is the accepted resolution (cancel-on-disconnect is YAGNI)
The 90s soft deadline is the settled fix for #151, so this PR fully closes the issue on merge. True cancellation on client disconnect (wiring a
threading.Eventthroughserver.py'sasyncio.to_threadso an orphaned handler stops mid-poll/mid-upload) is a deliberate YAGNI decision (settled 2026-07-07), not a pending follow-up: the ~90s cap is already below the exe-lock contention window the updater fights, so the Event path buys nothing measurable. It's documented in the code (ponytail:note atmcp.py's_WAIT_TIMEOUT_S) and ADR-006 purely as an upgrade path should orphan telemetry ever show 90s still matters.Test plan
python -m pytest tests/— 1025 passed, 2 skippedpython -m mypy src/plaud_tools— cleanpython -m ruff check ./ruff format --check .— cleanTestMergeRecordingsSlimResponse.test_wait_timeout_returns_still_processingmirroring the existingTestProcessRecordingBoundedWaitpattern