Skip to content

fix(#488): log voice adapter and ffmpeg disconnect failures at WARNING#556

Open
drewdrewthis wants to merge 2 commits into
mainfrom
fix/issue488-log-disconnect-failures
Open

fix(#488): log voice adapter and ffmpeg disconnect failures at WARNING#556
drewdrewthis wants to merge 2 commits into
mainfrom
fix/issue488-log-disconnect-failures

Conversation

@drewdrewthis

Copy link
Copy Markdown
Collaborator

Summary

  • Replaces two silent except Exception: pass blocks in _voice_disconnect_all() with logger.warning(..., exc_info=True) calls, honouring the existing docstring contract: "failures are logged but do not mask the primary scenario result."
  • Voice adapter disconnect failures now emit WARNING: voice adapter {ClassName} disconnect failed with full traceback.
  • ffmpeg playback stop failures now emit WARNING: ffmpeg playback stop failed with full traceback.

Acceptance criteria (from #488)

  • _voice_disconnect_all logs every caught exception via logger.warning("voice adapter %s disconnect failed", agent_name, exc_info=True)
  • ffmpeg playback .stop() swallow also logs (logger.warning("ffmpeg playback stop failed", exc_info=True))
  • Test: failing disconnect() produces a WARNING record with exc_info attached
  • Test: scenario result is unaffected — no exception propagates out of _voice_disconnect_all
  • Docstring at :750-751 remains accurate (unchanged; logging now matches the documented intent)

Test plan

  • pytest tests/test_voice_disconnect_logging.py -v — 4 tests, all green
  • Full suite: pytest tests/ -m "not integration" — no regressions
  • pyright . — 0 errors

🤖 Generated with Claude Code

Both except-pass blocks in _voice_disconnect_all() silently swallowed
exceptions, violating the docstring's "failures are logged" contract.
Each failure now emits a WARNING record with exc_info so operators can
see transport-cleanup errors (e.g. Twilio voice_url restore failures,
ffmpeg process hangs) without the scenario result being affected.

Four new unit tests cover: disconnect failure logged, no propagation,
ffmpeg stop failure logged, clean disconnect produces no warnings.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@drewdrewthis drewdrewthis added the grinding Grinder is actively managing this PR label May 25, 2026
@github-actions github-actions Bot added the low-risk-change PR qualifies as low-risk per policy and can be merged without manual review label May 25, 2026
github-actions[bot]
github-actions Bot previously approved these changes May 25, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved by automation: PR qualifies as low-risk-change under the documented policy.


import logging
import pytest
from typing import Any
import logging
import pytest
from typing import Any
from unittest.mock import MagicMock, AsyncMock, patch
@drewdrewthis

Copy link
Copy Markdown
Collaborator Author

[grinder] READY for human review

CI: green (zero failing, zero pending)
ACs: met — both silent except Exception: pass blocks in _voice_disconnect_all() now emit logger.warning(..., exc_info=True); 4 new tests verify logging behaviour
Threads: zero unresolved

Verified by:
command gh pr checks 556 → all 13 checks pass (test 3.12: pass 4m58s, python-complete: pass, Analyze: pass)
command gh pr view 556 --json reviewDecision → APPROVED by github-actions (low-risk-change)

@drewdrewthis drewdrewthis added pr-ready and removed grinding Grinder is actively managing this PR labels May 25, 2026
…onnect-failures

# Conflicts:
#	python/scenario/scenario_executor.py
@github-actions github-actions Bot added low-risk-change PR qualifies as low-risk per policy and can be merged without manual review and removed low-risk-change PR qualifies as low-risk per policy and can be merged without manual review labels Jun 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Automated low-risk assessment

This PR was evaluated against the repository's Low-Risk Pull Requests procedure.

  • Scope: Adds tests (python/tests/test_voice_disconnect_logging.py) that verify _voice_disconnect_all and ffmpeg playback.stop() log WARNING with exc_info and that exceptions are not propagated; introduces mock failing/ok VoiceAgentAdapter classes.
  • Exclusions confirmed: no changes to auth, security settings, database schema, business-critical logic, or external integrations.
  • Classification: low-risk-change under the documented policy.

The diff only adds a new test file (python/tests/test_voice_disconnect_logging.py) that asserts logging behavior for _voice_disconnect_all and ffmpeg.stop(); it does not modify production code, authentication/authorization, secrets, database schemas, business logic, or external integrations. Adding tests falls under the allowed low-risk categories (test configuration/fixtures), so the PR meets the low-risk criteria.

An approving review has been submitted by automation. The PR may merge once required CI checks pass.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved by automation: PR qualifies as low-risk-change under the documented policy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

low-risk-change PR qualifies as low-risk per policy and can be merged without manual review pr-ready

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant