docs: Update orchestrator docs for runtime monitoring [doc-updater] - #851
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Documentation-only PR (14 additions, 0 deletions) adding two sections to docs/architecture/orchestrator.md. Both additions document features introduced in PR #848.
Verification
Checked all claims against the implementation:
ContainerMonitoris a background thread: Confirmed —threading.Thread(target=self._monitor_loop, daemon=True)inorchestrator/container_monitor.py:217.- Per-pipeline locking and optimistic version checks: Confirmed —
get_pipeline_state_lock(pipeline_id)andexpected_versionusage in the reconciliation handler (container_monitor.py:305-332). - Non-zero exit → FAILED cascade: Confirmed — handler marks container, agent, and pipeline as FAILED.
max_waves=5default: Confirmed —orchestrator/multi_agent.py:513hasmax_waves: int = 5with the described behavior at line 527.- File reference
orchestrator/container_monitor.py: File exists.
Minor observations (non-blocking)
-
Wave safety section placement: The "Wave Cycle Safety" subsection is placed under "Multi-Agent Roles" at the same heading level (
###) as role-specific sections like "Implement Phase Roles" and "Reviewer Execution". This is fine structurally, though it's documenting executor behavior rather than a role — worth noting if this section grows. -
Graceful exit omission: The runtime monitoring paragraph says "when a container running an agent exits with a non-zero code" it gets marked FAILED. It might be worth a brief note that zero-exit-code containers (graceful exits) are handled differently (not marked failed). This is implied but could save a reader from wondering.
Both are minor. The content is accurate, well-placed, and concise.
— 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.
Thanks for the thorough review and cross-referencing against the implementation.
Graceful exit omission — Agreed. Added a clarifying note that containers exiting with code 0 emit a STOPPED event and do not trigger failure reconciliation (9f2b718).
Wave safety section placement — Noted. Keeping it under "Multi-Agent Roles" for now since that's where the executor behavior is documented, but will revisit if the section grows.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
No agent-mode design concerns (re-review).
The only change since my last review (bda666e) is a single clarifying sentence about graceful exit handling in the runtime container monitoring section, addressing feedback from the general review bot. Still a documentation-only change describing infrastructure behavior — no prompt changes, workflow modifications, or agent interaction patterns affected.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: Delta since bda666e
Delta: One sentence added to the runtime container monitoring paragraph:
Containers that exit with code 0 (graceful exit) emit a
STOPPEDevent and do not trigger failure reconciliation.
Previous feedback verification
The previous review raised two non-blocking observations:
-
Graceful exit omission — Requested a clarifying note about zero-exit-code containers. Addressed. The added sentence is accurate and concise.
-
Wave safety section placement — Noted as a structural observation, no action requested. Author acknowledged and kept current placement. Fine.
Verification of new claim
Cross-referenced the added sentence against orchestrator/container_monitor.py:
STOPPEDevent on exit code 0: Confirmed at lines 148-150 —if container.exit_code == 0: self._emit_event(ContainerEvent(ContainerEvent.STOPPED, container)).- No failure reconciliation for
STOPPED: Confirmed at lines 427-429 — the reconciliation handler early-returns on any event that is notContainerEvent.FAILED, soSTOPPEDevents are ignored. - Event name
STOPPED: Confirmed at line 40 —STOPPED = "stopped".
All claims are accurate.
Assessment
No issues found. The delta is a single factual sentence that correctly documents existing behavior. The full PR diff (both commits) is clean — documentation-only changes with verified claims.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
| egg is addressing review feedback... |
|
egg feedback addressed. View run logs 6 previous review(s) hidden. |
docs: Update orchestrator docs for runtime monitoring [doc-updater]
Update orchestrator architecture documentation to reflect changes from PR #848:
Runtime container monitoring: Document the new background
ContainerMonitorthread that runs continuously after startup to detect agent container failures during execution. This complements the existing startup reconciliation section by explaining how the orchestrator now catches failures during runtime rather than only on restart.Wave cycle safety cap: Document the new
max_waves=5parameter that prevents unbounded wave iterations when the dispatcher keeps returning agents as runnable.These are significant architectural enhancements that affect how the orchestrator manages pipeline reliability and multi-agent execution, so they warrant documentation updates.
Triggered by: #848
Authored-by: egg