docs: update BRC timeout wait for event-driven polling [doc-updater] - #1934
Conversation
There was a problem hiding this comment.
Doc-only change, single line in docs/guides/concurrent-execution.md. Verified every claim against the merged implementation in orchestrator/routes/pipelines.py (from #1929 / 7e2f999):
- "event-driven polling loop": confirmed —
while remaining:loop at:8443 - "30-second intervals": matches
post_timeout_poll_interval = 30at:8440 - "up to 3600s total budget": matches
post_timeout_budget = 3600at:8439 - "rechecks consensus proactively on each iteration":
executor.check_consensus()at:8452 - "immediately stops remaining containers":
_stop_running_containers()at:8481 - "marks agents complete, restores pipeline, returns success":
:8480-8482 - "replaces the former per-container blocking wait":
ThreadPoolExecutor+wait_for_containerremoved in #1929 - Issue references: #1921 (polling refactor) added, #1693 (HITL escalation) preserved — both correct
No issues.
One non-blocking note: line 704 is now ~580 characters of unbroken text. Consider splitting it into a short intro sentence followed by a bulleted list of the polling mechanics for readability. Not blocking on this.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Agreed. Split the ~580-character paragraph on line 704 into a short intro sentence followed by a bulleted list (polling mechanics, consensus-complete behavior, unresolved-NACKs escalation). The historical context note about #1921 is now a standalone sentence after the list.
— Authored by egg
There was a problem hiding this comment.
Re-reviewed the delta since 745bdead (one new commit: 7c3184b).
The feedback-addressing commit correctly splits the ~580-character paragraph into a bulleted list as suggested. Verified:
- No content lost: all claims from the first commit preserved — polling mechanics (30s / 3600s budget), consensus-complete recovery path, NACK escalation path, and historical context about #1921.
- Formatting clean: intro sentence with colon, three bullets, standalone closing paragraph. Integrates well with surrounding sections (lines 702–712).
- Issue references intact: #1921 and #1693 both preserved and correctly placed.
No issues.
— Authored by egg
|
egg review completed. View run logs 2 previous review(s) hidden. |
…1934) * docs: update BRC timeout wait description for event-driven polling * docs: split long timeout-wait paragraph into bulleted list for readability --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
Update documentation to reflect changes from 7e2f999 (#1929):
execution guide described the post-timeout fallback as waiting for
containers to exit before checking consensus. This was the old
ThreadPoolExecutor behavior.
3600s budget) that rechecks consensus proactively on each iteration
and returns success immediately when consensus is confirmed — without
waiting for all containers to exit naturally.
references issue Implement phase fails after 90 min even when BRC consensus is actively converging (consensus_timeout_minutes=30 + blind 1h container-wait window) #1921 so operators understand why late-arriving
consensus is now reliably detected.
Triggered by: #1929
Issue: none
Test Plan
docs/guides/concurrent-execution.mdaccurately describes the event-driven polling behavior
Authored-by: egg