Follow-up to #113 (merged as b3e3485). Three items from the review were not addressed and are deferred here.
I3 — Over-broad AssertionError suppression during server shutdown (Important)
File: src/conductor/web/server.py:425
_is_proactor_shutdown_race is documented to return True "only when … the traceback (if available) originates from asyncio internals", but the implementation falls through to an unconditional return True when the traceback is missing or the last frame isn't from asyncio. Combined with the DEBUG-level logging in _loop_exception_handler (line 435) and _guarded_serve (line 469), unrelated AssertionErrors raised during shutdown — e.g., from a workflow callback finishing late — get silently swallowed at default log levels.
Scope: Only fires when uvicorn's should_exit is set, narrowing blast radius. Windows + Python 3.14+ is the documented trigger.
Suggested fix (pick one):
- Tighten the gate:
return tb is not None and bool(frames) and 'asyncio' in frames[-1].filename (false otherwise) — matches the docstring contract.
- OR keep the permissive fallback but log it at
WARNING with exc_info=True so operators can see what was suppressed.
S2 — Clear-before-raise rationale comment (Suggestion)
File: src/conductor/engine/workflow.py around _check_interrupt (lines ~1145-1152)
The interrupt event is cleared just before the conditional raise InterruptError for _subworkflow_depth > 0. Correct today (is_set() is the only consumer; the unwind chain doesn't re-check), but brittle if anyone later adds a between-agent recheck after the catch — they'll find the event already gone. A short comment would prevent future foot-guns.
S3 — resolveSlotPath docstring orphaning warning (Suggestion)
File: src/conductor/web/frontend/src/stores/workflow-store.ts (function around line 404)
The implementation correctly matches the newest matching slot for re-runs. The docstring says nothing about the consequence: older iterations of the same slot become unreachable for late-arriving events targeting them. Worth a one-line note in the docstring.
Original review: #113 (review)
Follow-up to #113 (merged as b3e3485). Three items from the review were not addressed and are deferred here.
I3 — Over-broad
AssertionErrorsuppression during server shutdown (Important)File:
src/conductor/web/server.py:425_is_proactor_shutdown_raceis documented to return True "only when … the traceback (if available) originates from asyncio internals", but the implementation falls through to an unconditionalreturn Truewhen the traceback is missing or the last frame isn't from asyncio. Combined with the DEBUG-level logging in_loop_exception_handler(line 435) and_guarded_serve(line 469), unrelatedAssertionErrors raised during shutdown — e.g., from a workflow callback finishing late — get silently swallowed at default log levels.Scope: Only fires when uvicorn's
should_exitis set, narrowing blast radius. Windows + Python 3.14+ is the documented trigger.Suggested fix (pick one):
return tb is not None and bool(frames) and 'asyncio' in frames[-1].filename(false otherwise) — matches the docstring contract.WARNINGwithexc_info=Trueso operators can see what was suppressed.S2 — Clear-before-raise rationale comment (Suggestion)
File:
src/conductor/engine/workflow.pyaround_check_interrupt(lines ~1145-1152)The interrupt event is cleared just before the conditional
raise InterruptErrorfor_subworkflow_depth > 0. Correct today (is_set()is the only consumer; the unwind chain doesn't re-check), but brittle if anyone later adds a between-agent recheck after the catch — they'll find the event already gone. A short comment would prevent future foot-guns.S3 —
resolveSlotPathdocstring orphaning warning (Suggestion)File:
src/conductor/web/frontend/src/stores/workflow-store.ts(function around line 404)The implementation correctly matches the newest matching slot for re-runs. The docstring says nothing about the consequence: older iterations of the same slot become unreachable for late-arriving events targeting them. Worth a one-line note in the docstring.
Original review: #113 (review)