Skip to content

fix: guard stale call session playback - #139

Draft
Justinabox wants to merge 1 commit into
mainfrom
agent/stale-playback-20260627
Draft

fix: guard stale call session playback#139
Justinabox wants to merge 1 commit into
mainfrom
agent/stale-playback-20260627

Conversation

@Justinabox

Copy link
Copy Markdown
Owner

Summary

  • Fixes Guard CallSession playback against stale or inactive sessions #135 by making CallSession.play, play_sequence, and play_loop fail closed unless the exact session is still active.
  • Adds a session-liveness composite cancel event so playback stops when the call/session becomes stale during a prompt, sequence, or loop.
  • Keeps active play_and_collect tests using a fake service that now models active-call ownership explicitly.

Tests

  • git diff --check
  • PYTHONPATH=. uv run --no-project --with pytest --with pytest-asyncio --with pytest-aiohttp --with pyserial-asyncio --with aiosqlite pytest tests/test_call_service.py tests/test_call_session_play_and_collect.py -q50 passed in 1.73s
  • PYTHONPATH=. uv run --no-project --with pytest --with pytest-asyncio --with pytest-aiohttp --with pyserial-asyncio --with aiosqlite pytest tests/ -q598 passed in 5.75s

Review

  • Spec compliance review: PASS.
  • Quality/security review: APPROVED after adding reviewer-requested mid-playback stale-session cancellation coverage.

Risks

  • CallSession.play_loop(..., cancel=None) now has a natural session-liveness cancel event instead of relying on the lower-level player's missing-cancel ValueError; this aligns the session API with active-call ownership but is a small behavior change.

@Justinabox

Copy link
Copy Markdown
Owner Author

Review/merge-manager classification: needs-fix before autonomous landing. The stale-session playback guard is good direction, but read-only review found a remaining stale play_and_collect() privacy/routing risk: if the session becomes stale while prompt playback is in progress, the liveness cancel event can stop audio playback normally, then play_and_collect() continues into the DTMF wait without rechecking that this CallSession is still the active/current call. That can leave a stale session alive until timeout and potentially collect DTMF intended for a newer call.\n\nRequested TDD fix:\n1. Add a RED regression where service.active_call is replaced while CallSession.play_and_collect(interrupt=True) is running, then emit DTMF and prove the stale session does not collect it or wait until the normal timeout.\n2. After prompt playback completes/stops, recheck self.is_active before entering DTMF collection (or otherwise distinguish normal prompt completion from stale-session cancellation) and fail closed.\n3. Rerun git diff --check plus PYTHONPATH=. uv run --no-project --with pytest --with pytest-asyncio --with pytest-aiohttp --with pyserial-asyncio --with aiosqlite pytest tests/test_call_service.py tests/test_call_session_play_and_collect.py -q and the full suite.\n\nDraft status is not the blocker by itself in this lane; the executable stale-session collection gap is.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release-manager classification update: superseded by #150, which has now landed on main as b63f08b.

This PR remains blocked because it protects stale playback calls but leaves related stale-session control/privacy gaps open:

  • play_and_collect() can continue into DTMF collection after prompt playback is cancelled by a stale-session liveness change.
  • stale hangup() can still affect the current call.
  • stale collect_dtmf() can still subscribe to DTMF from a later/current call.

#150 covered those broader control/DTMF boundaries and passed the release-manager gates before merge, so future work should build on current main rather than this branch.

@Justinabox

Copy link
Copy Markdown
Owner Author

Review/merge-manager classification: needs-fix.

I am not marking this ready or merging yet. The branch currently conflicts with main, so the voice/session lifecycle changes need a refresh before another safety review.

Requested changes:

  • Refresh over current main and resolve conflicts.
  • Preserve stale CallSession privacy/safety boundaries: playback/collect should fail closed for ended or superseded sessions before any audio/modem side effects.
  • Include regression tests for retained stale session handles, then rerun focused call-service/session tests, git diff --check, and the full required pytest gate.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release review: needs-fix.

This draft is currently DIRTY against main, so it is not mergeable as-is. Please rebase or repackage it on current main, resolve conflicts without broadening scope, rerun git diff --check plus the full required Python 3.11 test suite, push the refreshed head, and request a new release review. No behavior was approved or merged in this pass.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release-captain classification: needs-fix. This PR is now DIRTY against the current default branch after later safe merges. Please rebase/repackage it from current origin/main, resolve only the genuine overlap, then rerun git diff --check and the required no-hardware full pytest gate on the new head. Do not attempt to merge the stale head directly.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release lane update: needs-human.

This change affects a sensitive security/privacy, production-deployment, modem/SIM, audio/hardware, or public realtime product boundary. It will not be autonomously merged under this lane. Please obtain an explicit Korgo decision on the intended policy/scope and, where applicable, rebase on current main with current gate evidence before requesting a new release review.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release review: needs-fix.

This PR is now DIRTY against the default branch after recent safe merges. Do not merge the stale head directly. Refresh/rebase it in an isolated worktree, resolve the concrete overlap against current main, run git diff --check plus the required full pytest gate on the refreshed head, then return it for review.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release review: needs-fix. GitHub currently reports this branch DIRTY against current main, so it cannot be safely evaluated or landed as-is. Rebase/repackage it from current main, resolve only the intended scope, inspect the resulting diff for overlap with work already shipped, and run git diff --check plus PYTHONPATH=. uv run --no-project --with pytest --with pytest-asyncio --with pytest-aiohttp --with pyserial-asyncio --with aiosqlite pytest tests/ -q. Request a fresh review on the new head; do not force a merge of the stale head.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release review classification: needs-fix. GitHub now reports this branch DIRTY against the advanced default branch, so it is not mergeable. Please create an isolated rebase/repackage branch from current origin/main, resolve and review the overlap, run git diff --check plus the required full test suite on the rebased head, push it, and request a fresh release review. Do not attempt to merge this stale head directly.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release-lane classification: needs fix/revalidation before merge.

This draft was not independently reviewed or integration-tested in this pass and its GitHub mergeability is stale/unknown after current-main advances. Please rebase or otherwise validate it against current main, run the required full local gate, and request a fresh release review with exact head SHA and gate evidence. Do not treat absence of hosted checks as approval.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release classification: needs-fix for this release pass.

The draft has not received a current exact-head review and prescribed local integration gate over the advancing main baseline. Rebase/repackage as needed against current main, resolve any conflicts, then provide a narrow behavioral review plus passing git diff --check and the prescribed full test suite before it can be reconsidered for landing.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release review: needs-fix. GitHub reports this stale-playback PR DIRTY. Rebase/repackage against the current voice lifecycle and revalidate cancellation/cleanup races with the required full suite before re-review.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Guard CallSession playback against stale or inactive sessions

1 participant