Fix startup reconciliation and container monitor never running in production - #852
Conversation
…duction api.main() and cli.cmd_serve were duplicate startup paths. The production entrypoint always uses cli.py, so startup_reconciliation (#839) and ContainerMonitor (#848) were dead code — never executed on any restart. Move the startup logic into cmd_serve and delete api.main() along with its now-unused argparse and waitress imports. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
The diagnosis is correct — api.main() was dead code in production because entrypoint.sh runs python cli.py serve, which imports the Flask app directly and never calls main(). Moving startup reconciliation and the container monitor into cmd_serve is the right fix. The code is a clean lift-and-shift with no logic changes.
One issue worth addressing before merge:
threads=16 silently dropped
The deleted api.main() called:
serve(app, host=host, port=port, threads=16)But the existing cmd_serve (line 137) calls:
serve(app, host=host, port=port)Waitress defaults to 4 threads. This discrepancy existed before this PR — cmd_serve never had threads=16 — but this PR deletes the only place that documented the intended concurrency. The orchestrator handles webhook processing, container lifecycle, and SSE connections concurrently, so 4 threads may be insufficient under load.
Suggested fix — add threads=16 to the serve() call in cli.py:137:
serve(app, host=host, port=port, threads=16)This is low-risk and ensures the intended production configuration isn't lost.
Minor: PR body includes "Claude Code" branding
The repo conventions (CLAUDE.md) say PR descriptions must never include "Claude Code" or claude.ai links. The PR body ends with 🤖 Generated with [Claude Code](https://claude.com/claude-code) — remove that line before merge.
What looks good
- Correct root cause analysis — the
entrypoint.sh→cli.py serve→from api import apppath never touchedapi.main(). - Both
try/except Exceptionblocks around reconciliation and monitor startup are preserved, so failures in either won't prevent the server from starting. if repo_path != "not set"guard is preserved, correctly skipping startup logic whenEGG_REPO_PATHisn't configured.- Existing unit tests (
test_startup_reconciliation.py,test_container_monitor.py) test the functions directly and are unaffected by the move. - No other callers of
api.main()orpython api.pyexist — the deletion is safe. - Import cleanup in
api.py(removingargparse,os,waitress) is correct — none are used outside the deletedmain().
— Authored by egg
This comment has been minimized.
This comment has been minimized.
| egg is addressing review feedback... |
Review feedback addressed
|
|
egg feedback addressed. View run logs 1 previous review(s) hidden. |
Comprehensive architecture analysis for a two-tier pipeline failure detection framework. Recommends unified HealthCheck interface with HealthCheckRunner, migrating existing startup_reconciliation and container_monitor to the new interface, adding Tier 1 programmatic checks (phase output presence, state consistency, repeated failure patterns) and Tier 2 LLM-based agent inspector. Key design features: two-strike FAIL_PIPELINE confirmation, HEALTH_CHECK_MODE kill-switch (enforce/observe/disabled), exception isolation at every layer, per-pipeline AgentCircuitBreaker instances. 13-phase implementation plan across 6 stages with 12 technical decisions documented. Addresses plan reviewer feedback (v2). Analysis confirmed accurate after PRs #852, #854, #855 on main. Issue: #850
1 participant
Summary
api.main()andcli.cmd_servewere duplicate startup paths — the production entrypoint (cli.py serve) never calledapi.main(), sostartup_reconciliation(fix: Handle orphaned container state on orchestrator restart #839) andContainerMonitor(Add runtime container monitor and wave cycle cap #848) were dead code on every orchestrator start/restartcmd_serve, which is the actual production code pathapi.main()along with its now-unusedargparseandwaitressimportsRoot cause
entrypoint.shrunspython cli.py serve, which doesfrom api import appthen callswaitress.serve(app)directly. The reconciliation and monitor startup were added toapi.main()(which is only reachable viapython api.py) — meaning neither mechanism has ever executed in a real deployment.This is why the issue-835 pipeline remained stuck through multiple orchestrator restarts despite #839 and #848 being merged.
Test plan
cd orchestrator && python -m pytest tests/test_startup_reconciliation.py tests/test_container_monitor.py -v🤖 Generated with Claude Code