Honor configured session timeout for Copilot dialog turns - #325
Honor configured session timeout for Copilot dialog turns#325PolyphonyRequiem wants to merge 1 commit into
Conversation
jrob5756
left a comment
There was a problem hiding this comment.
Nice to see your name again 😄. I like the change. Just two minor inline notes worth a look before merging: the is_retryable flip doesn't seem to reach any code that checks it, and reusing max_session_seconds for a single flat wait removes the fast-failure behavior without putting anything back in its place. Lint, types, and the full suite all pass, and the new regression test is solid (I reverted the fix locally and confirmed it fails).
| session.on(on_event) | ||
| await session.send(full_prompt) | ||
|
|
||
| dialog_timeout = self._idle_recovery_config.max_session_seconds |
There was a problem hiding this comment.
This bumps the default dialog-turn timeout from 120s to 1800s for anyone who hasn't set runtime.max_session_seconds, and it's a single flat wait_for with none of the periodic idle-check/recovery logic the main agent loop has around this same config value. If a dialog session genuinely hangs, a console or web user could be staring at nothing for 30 minutes before seeing an error.
Would a shorter, dialog-specific default (or at least a log line once idle_timeout_seconds has elapsed) be worth adding here, so a stall isn't completely silent until the full deadline hits?
| "Dialog turn timed out after 120s", | ||
| is_retryable=False, | ||
| f"Dialog turn timed out after {dialog_timeout:g}s", | ||
| is_retryable=True, |
There was a problem hiding this comment.
Neither gates/dialog.py nor dialog_evaluator.py look at .is_retryable on this error — both just catch a bare Exception and move on. So this flip doesn't change any observable behavior today, and it puts this raise at odds with the main execution loop, which treats the identical max_session_seconds timeout as non-retryable on purpose (see the # Don't retry — same root cause will recur comment a bit further up in this file).
If there's a future retry path planned for dialog turns, a short comment here explaining the divergence would help the next person who reads this. Otherwise I'd lean toward keeping this False to match the main loop.
| is_retryable=True, | |
| is_retryable=False, |
| - **Copilot dialog turns no longer fail after a hidden 120-second deadline** — | ||
| lightweight dialog sessions now honor `runtime.max_session_seconds` (or the | ||
| Copilot provider's 1800-second default), and timeout errors are classified as | ||
| retryable so console and web dialogs can continue without losing history. |
There was a problem hiding this comment.
Minor: the "so console and web dialogs can continue without losing history" part already happens today regardless of is_retryable — both dialog call sites roll back the user turn and continue on any exception, not just retryable ones. Might be worth tightening this to just describe the timeout fix itself.
Summary
max_session_secondsProblem
CopilotProvider.execute_dialog_turn()ignoredruntime.max_session_secondsand always timed out after 120 seconds. Long reasoning responses could therefore fail even when the workflow explicitly allowed a longer session.Validation
PYTHONPATH= ./.venv/Scripts/pytest.exe tests/test_providers/test_copilot.py— 128 passedty check src/conductor/providers/copilot.pypassed