reliability: fix audited lifecycle and state edge cases - #962
Merged
Conversation
|
Warning Review limit reached
Next review available in: 16 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughThis PR adds stricter configuration and data validation. It centralizes safe agent-state serialization. It adds bounded, retryable cleanup for journals, sessions, STT providers, WebSockets, listeners, and transports. It also hardens telephony transitions and expands regression coverage. ChangesLifecycle hardening and validation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Session
participant STTCommitter
participant RuntimeSupervisor
participant Provider
Session->>STTCommitter: cancel or end stream
STTCommitter->>Provider: run bounded provider cleanup
STTCommitter->>RuntimeSupervisor: retain unfinished work
RuntimeSupervisor->>STTCommitter: retry retained cleanup
STTCommitter-->>Session: return cleanup status
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
yisding
marked this pull request as ready for review
August 7, 2026 14:07
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b0659c7f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Resolve ratchet manifest conflicts by recounting merged entries. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n errors Address PR 962 review comments: the runtime-copy guard now matches validate_for_build's isinstance semantics, and the WebRTC SDP failure response no longer echoes exception details to the client. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- bound the rejected-start STT cleanup join after caller cancellation so an unresponsive provider cannot make stop(force=True) uncancellable - report cooperatively-cancelled listener waits as force-timeout failures and retain the listener for retry instead of discarding it - validate bridge event text only for text-bearing kinds; duck-typed tool events may carry text=None - restore substring credential-name filtering for the generic workflow __dict__ fallback snapshot (authtoken-style names leaked) - let observer-spawned tasks transition after the owning call-state transition settles; only an active transition rejects them - classify the new teardown budgets and refresh source ratchets Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/easycat/debugger/server.py`:
- Around line 705-708: Remove the duplicated “an” in the comment near the
state-changing request handling, so it reads “can pass an explicit same-origin
value.”
In `@src/easycat/integrations/agents/_agent_runner.py`:
- Around line 208-213: Update the validation branches in AgentRunnerConfig for
preemptive_generation and preemptive_max_retries so every line stays within
Ruff’s 99-character limit. Preserve the existing validation behavior and noqa
suppression while splitting the error statements or extracting their messages
into local constants.
- Around line 65-67: Update the text extraction in the event validation flow
around _require_agent_text() to use None or a sentinel instead of an
empty-string default when the event lacks text, ensuring text-bearing events
without a text attribute are rejected before forwarding.
In `@src/easycat/integrations/agents/_state_serialization.py`:
- Around line 74-77: Update _remove_secret_shaped_keys to canonicalize unordered
state before serialization: recursively scrub set items, sort them using a safe
canonical JSON key, and serialize scrubbed mappings with sort_keys=True so
equivalent states produce identical bytes. Preserve sequence handling, and add a
regression test covering a multi-item set with varying iteration order.
In `@src/easycat/runtime/scope.py`:
- Around line 584-591: Update prune_empty_child around
_close_admission_recursive so its contract reflects that a refused prune may
still close admission and leave the child subtree in CLOSING, or move the
emptiness check ahead of admission closure to preserve a no-side-effect False
result. Ensure the docstring and return behavior consistently describe whichever
contract is implemented.
In `@src/easycat/server/voice_server.py`:
- Around line 640-646: Guard the completed-task result in the sweep-success path
before calling _record_incomplete_hard_sweep: catch exceptions re-raised by
sweep_task.result(), append the exception to cleanup_errors, and pass None as
report when retrieval fails. Preserve the existing successful result behavior
and ensure _record_incomplete_hard_sweep still executes so
_finalize_stop_cleanup remains retry-safe.
In `@src/easycat/session/_stt_committer.py`:
- Line 244: Promote the repeated "stt_segment_commit" identifier to a class
constant alongside FINAL_CLOSE_TASK_NAME, PROVIDER_END_TASK_NAME, and
PROVIDER_CLOSE_TASK_NAME. Replace every occurrence in lifecycle task lookups,
cancellation targets, and journal payloads with that constant, including the
references in the relevant session methods.
- Around line 1016-1049: Bound the retry loop in
_finish_transferred_provider_close with an explicit attempt cap or wall-clock
deadline. When the bound is reached, stop retrying while preserving
_provider_close_error, allow the owned task to settle, and leave
_provider_close_pending consistent so retry_transferred_provider_close can
perform any later attempt.
In `@src/easycat/teardown_budgets.py`:
- Around line 39-44: Adjust the rejected-start STT cleanup timing constants used
by _cleanup_rejected_stt_start() so their combined join and cancel-grace
duration fits within SESSION_FORCE_START_LOCK_TIMEOUT_S when invoked from the
force-cancellation path, or consistently increase that force-path cap; preserve
bounded cleanup before the remaining stop path proceeds.
In `@src/easycat/telephony/call_state.py`:
- Around line 426-432: Move _transition_context out of
OutboundCallStateMachine.__init__ and define one module-level ContextVar shared
by all instances. Store both the owning state machine and task in its value,
updating all transition-context reads and writes to use that shared pair, while
retaining _active_transition_owner as the current task.
In `@src/easycat/telephony/retry.py`:
- Around line 128-135: Update get_delay() to handle a zero base_delay_s when
state.attempts is still zero before evaluating the exponentiation, returning the
defined zero delay (or otherwise applying the established pre-attempt contract).
Preserve the existing overflow and non-finite delay clamping for recorded
attempts.
In `@tests/debugger/test_server_replay_export.py`:
- Around line 211-215: Use the canonical _SAFE_HEADERS from
tests/debugger/_server_helpers.py across both modules: in
tests/debugger/test_server_replay_export.py, import it and replace the local
mappings at lines 193-197, 211-215, 246-250, 268-272, 301-305, 321-325, 352-356,
386-390, and 404-408; in tests/debugger/test_aec_diagnostics.py, remove the
module-level definition at lines 276-280 and import _SAFE_HEADERS instead.
In `@tests/debugger/test_server_rest_api.py`:
- Around line 523-547: Add a test for
test_api_audio_waveform_rejects_pcm_over_memory_limit where the raw audio blobs
remain within _WAVEFORM_MAX_PCM_BYTES but _coerce_frames_to_format expands the
coerced PCM beyond that limit. Assert the post-coercion request still returns
HTTP 413 with the existing WAVEFORM_AUDIO_TOO_LARGE payload, covering the guard
after format conversion rather than the raw accumulation guard.
In `@tests/server/test_capacity_gate_drain.py`:
- Around line 247-268: Strengthen
test_timed_safe_await_observes_cancelled_gather_result by directly verifying
that _observe_future_result was attached to the gathered future after
_safe_await returns. Inspect the future’s registered callbacks before deleting
gathered, while preserving the existing exception-handler assertion.
In `@tests/server/test_shutdown_draining.py`:
- Around line 411-415: Update the shutdown test around server.stop() to expect
and assert the intended cleanup exception for the cooperative cancellation path.
Remove the assertion that server._ws_server is None, while preserving assertions
that the hanging listener is closed and cleanup task tracking is cleared.
In `@tests/server/test_websocket_runtime_drain.py`:
- Around line 294-299: Update the assertions in the test around the retry flow
to inspect runtime._connection_cleanup_retry directly instead of using
runtime._connections as a proxy. Keep the existing setup and drain calls
unchanged, and assert the ledger’s expected state before and after the retry
cleanup.
In `@tests/session/test_stt_committer.py`:
- Around line 737-748: Update the test surrounding _HangingSTT and
committer.end_stream to drain the runtime/provider task scope after the timeout,
matching the cleanup pattern used by the sibling tests. Ensure the cleanup runs
in a finally block so the parked hanging task is released even if an assertion
fails, while preserving the existing assertions.
In `@tests/stages/test_stages.py`:
- Around line 1407-1427: Update
test_turn_stage_materializes_generator_input_once_for_detection to pass a
journal and artifact store using the existing test pattern, then assert the
stage_start artifact contains exactly chunk.data. Keep the existing detect-input
assertion and result checks so the test verifies both capture and detection
receive identical audio.
In `@tests/telephony/test_retry_strategy.py`:
- Around line 117-130: Add the return annotation -> None to both test function
definitions in this test module, including
test_backoff_overflow_caps_at_max_delay and the additional test around the
referenced second location; leave their bodies and behavior unchanged.
In `@tests/turns/test_turn_manager.py`:
- Around line 237-243: Set detector.release before calling manager.shutdown() in
the test cleanup flow, so the cancelled detect task can finish while shutdown
awaits replaced. Preserve the existing assertions and retain the finally block’s
cleanup and exception-gathering behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 72ddc6af-7e4b-449a-87d4-6a5250e8d67a
📒 Files selected for processing (87)
src/easycat/_concurrency.pysrc/easycat/config/_factory.pysrc/easycat/config/easy.pysrc/easycat/debugger/server.pysrc/easycat/integrations/agents/_agent_runner.pysrc/easycat/integrations/agents/_state_serialization.pysrc/easycat/integrations/agents/generic_workflow.pysrc/easycat/integrations/agents/langchain.pysrc/easycat/integrations/agents/langgraph.pysrc/easycat/integrations/agents/llama_agents.pysrc/easycat/integrations/agents/openai_agents.pysrc/easycat/integrations/agents/pydantic_ai.pysrc/easycat/integrations/agents/responses_api.pysrc/easycat/integrations/agents/template.pysrc/easycat/runtime/journal_sql.pysrc/easycat/runtime/scope.pysrc/easycat/server/transports.pysrc/easycat/server/voice_server.pysrc/easycat/session/_audio_router.pysrc/easycat/session/_debug_backends.pysrc/easycat/session/_journal_sink.pysrc/easycat/session/_session.pysrc/easycat/session/_stt_committer.pysrc/easycat/session/_turn_runner.pysrc/easycat/session_manager.pysrc/easycat/stages/turn.pysrc/easycat/teardown_budgets.pysrc/easycat/telephony/call_state.pysrc/easycat/telephony/retry.pysrc/easycat/telephony/voicemail.pysrc/easycat/timeouts.pysrc/easycat/transports/_webrtc_stats.pysrc/easycat/transports/local.pysrc/easycat/transports/webrtc.pysrc/easycat/turn_manager.pysrc/easycat/validation/_latency_artifacts.pysrc/easycat/validation/_latency_baseline.pysrc/easycat/validation/_latency_budgets.pysrc/easycat/validation/_latency_models.pysrc/easycat/validation/redaction.pytests/cli/test_doctor.pytests/config/test_session_creation.pytests/core/test_timeouts.pytests/debugger/_server_helpers.pytests/debugger/test_aec_diagnostics.pytests/debugger/test_server_replay_export.pytests/debugger/test_server_rest_api.pytests/debugger/test_server_route_controller.pytests/debugger/test_server_security_origin.pytests/integration/test_session_pipeline.pytests/integrations/agents/test_agent_runner.pytests/integrations/agents/test_bridge_template.pytests/integrations/agents/test_generic_workflow_bridge.pytests/integrations/agents/test_pydantic_ai_v2.pytests/ratchets/pause-generation-manifest.jsontests/ratchets/source-baseline.jsontests/ratchets/teardown-budget-manifest.jsontests/ratchets/turn-commit-manifest.jsontests/ratchets/turn-lifecycle-manifest.jsontests/runtime/test_scope.pytests/runtime/test_sqlite_journal.pytests/server/test_capacity_gate_drain.pytests/server/test_shutdown_draining.pytests/server/test_voice_server_lifecycle.pytests/server/test_websocket_runtime_drain.pytests/session/test_audio_router.pytests/session/test_journal_sink.pytests/session/test_session_journal_accounting.pytests/session/test_session_lifecycle_teardown.pytests/session/test_session_manager.pytests/session/test_session_stop_ordering.pytests/session/test_session_streaming_behavior.pytests/session/test_stt_committer.pytests/session/test_turn_runner.pytests/stages/test_stages.pytests/teaching/test_chapter_15_doctor_contract.pytests/telephony/test_call_state_basics.pytests/telephony/test_call_state_gate.pytests/telephony/test_outbound_config.pytests/telephony/test_retry_strategy.pytests/telephony/test_voicemail.pytests/transports/test_local_transport.pytests/transports/test_webrtc_lifecycle_server.pytests/transports/test_webrtc_stats_artifacts.pytests/turns/test_turn_manager.pytests/validation/test_latency_boundaries.pytests/validation/test_latency_percentiles.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integrations/agents/test_bridge_template.py`:
- Around line 361-362: Update the _OrderedSet.__iter__ method to declare an
Iterator[str] return annotation while preserving its existing iteration over
self._items.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e4932586-e9af-461c-af28-64a375bef734
📒 Files selected for processing (25)
src/easycat/debugger/server.pysrc/easycat/integrations/agents/_agent_runner.pysrc/easycat/integrations/agents/_state_serialization.pysrc/easycat/runtime/scope.pysrc/easycat/server/voice_server.pysrc/easycat/session/_stt_committer.pysrc/easycat/teardown_budgets.pysrc/easycat/telephony/call_state.pysrc/easycat/telephony/retry.pytests/debugger/test_aec_diagnostics.pytests/debugger/test_server_replay_export.pytests/debugger/test_server_rest_api.pytests/integrations/agents/test_agent_runner.pytests/integrations/agents/test_bridge_template.pytests/ratchets/source-baseline.jsontests/ratchets/teardown-budget-manifest.jsontests/server/test_capacity_gate_drain.pytests/server/test_shutdown_draining.pytests/server/test_voice_server_lifecycle.pytests/server/test_websocket_runtime_drain.pytests/session/test_stt_committer.pytests/session/test_turn_runner.pytests/stages/test_stages.pytests/telephony/test_retry_strategy.pytests/turns/test_turn_manager.py
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
2 participants
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.
Problem
A broad reliability audit found lifecycle races, dropped task failures, inconsistent state serialization, and persistence/validation edge cases that could surface under cancellation, shutdown, parallel execution, or malformed inputs.
Changes
Impact
Shutdown remains bounded while retaining failed work for an explicit retry, background failures are observed instead of leaked, and persisted/validated state is deterministic across concurrent and degraded paths.
Validation
Summary by CodeRabbit
Bug Fixes
Security
Validation
New Features