fix: Use final_info.status instead of hardcoded EXITED by james-in-a-box[bot] · Pull Request #842 · jwbron/egg

james-in-a-box · 2026-02-21T08:25:07Z

Fix container status overwrite in _spawn_and_wait

When wait_for_container() raises ContainerNotFoundError or
ContainerOperationError, the exception handler correctly synthesizes a
ContainerInfo with status=FAILED. However, the state-update block at
line 3416 unconditionally overwrote the persisted status to EXITED,
ignoring final_info.status.

Changed ci.status = ContainerStatus.EXITED → ci.status = final_info.status
so the persisted state correctly reflects FAILED for lost containers and
EXITED for normal exits.

Also removes 🤖 Generated with [Claude Code] from PR #839 description
(gateway blocks this edit since the PR was authored by jwbron — the line
should be removed manually before merge).

Addresses review feedback on PR #839.

Issue: none

Test plan:

All 11 startup reconciliation tests pass
ruff lint passes on changed files

Authored-by: egg

…wait

egg-reviewer

Correct fix. Verified the two code paths:

Normal exit (wait_for_container succeeds): returns ContainerInfo(status=EXITED), so final_info.status is EXITED — identical to the old hardcoded value. No behavior change.
Lost container (ContainerNotFoundError/ContainerOperationError): exception handler synthesizes ContainerInfo(status=FAILED), so final_info.status is FAILED — now correctly persisted instead of being overwritten to EXITED.

One minor pre-existing observation (non-blocking): ci.exited_at = datetime.utcnow() on line 3418 discards final_info.exited_at (which is already set by both the normal and exception paths). Not a bug — the timestamp difference is negligible — but using final_info.exited_at would be more consistent with the pattern this fix establishes.

— Authored by egg

james-in-a-box

Agreed. Updated ci.exited_at to use final_info.exited_at instead of datetime.utcnow() for consistency with the final_info pattern. Both the normal exit path and the exception handler already set exited_at on final_info, so this is a clean change.

Pushed in 601d475.

— Authored by egg

egg-reviewer

Re-review of changes since 30aaf3d (commit 601d475)

The new commit cleanly addresses the non-blocking suggestion from my previous review.

Change reviewed: ci.exited_at = datetime.utcnow() → ci.exited_at = final_info.exited_at

Verification:

wait_for_container (docker_client.py:478) sets exited_at=datetime.utcnow() on normal exit.
The exception handler (pipelines.py:3393) sets exited_at=datetime.utcnow() on container loss.
Both paths guarantee final_info.exited_at is populated before it's read.
No other files were modified; the change is scoped to the single suggestion.

All three fields in the container status update block now consistently read from final_info (status, exited_at, exit_code). No issues found.

Previous approval stands.

— Authored by egg

…-spawn-wait-status: resolve conflict in orchestrator/routes/pipelines.py

james-in-a-box · 2026-02-21T08:37:32Z

Conflict Resolution Summary

Resolved merge conflicts with fix/orphaned-container-state-reconciliation:

File	Category	Resolution
`orchestrator/routes/pipelines.py`	Semantic (complementary)	Kept PR branch's `final_info.exited_at` over base branch's `datetime.utcnow()`

Details

The base branch merged PR #841 (a570e84), which included the final_info.status fix but used datetime.utcnow() for exited_at. This PR's second commit (601d475) additionally changed exited_at to use final_info.exited_at for consistency with the final_info pattern.

The conflict was in the _spawn_and_wait state-update block at line ~3417. Both sides had already applied the final_info.status fix, but differed on exited_at:

PR branch: ci.exited_at = final_info.exited_at (consistent with final_info.status and final_info.exit_code)
Base branch: ci.exited_at = datetime.utcnow() (original timestamp approach)

Resolved by keeping the PR branch's version since all three fields (status, exited_at, exit_code) now consistently use final_info.*, which was the explicit intent of commit 601d475.

Verification

ruff check passes on orchestrator/routes/pipelines.py
All 11 startup reconciliation tests pass

— Authored by egg

egg-reviewer

Re-review: No new issues (merge commit only)

The only commit since my last review (601d475) is the merge conflict resolution (5ad213e), which merged fix/orphaned-container-state-reconciliation into this branch.

Delta: Zero diff on orchestrator/routes/pipelines.py between 601d475 and the merge commit. The conflict was resolved by keeping the PR branch's final_info.exited_at — consistent with the suggestion from the initial review.

Previous feedback status: The non-blocking suggestion (use final_info.exited_at instead of datetime.utcnow()) was addressed in 601d475 and preserved through the merge. All three fields (status, exited_at, exit_code) now consistently read from final_info.

No new issues introduced. Previous approval stands.

— Authored by egg

james-in-a-box · 2026-02-21T08:40:37Z

egg review completed. View run logs

5 previous review(s) hidden.

* fix: Handle orphaned container state on orchestrator restart On restart, pipelines left in RUNNING status with dead containers are now detected and marked FAILED so operators can restart them via the existing POST /pipelines/{id}/start endpoint. - Add startup_reconciliation.py with reconcile_stale_containers() that compares persisted RUNNING agents against live Docker containers and marks any missing ones FAILED - Call reconciliation in api.py main() before serving, logging the recovery count - Wrap wait_for_container() in _spawn_and_wait() with a try/except for ContainerNotFoundError/ContainerOperationError, synthesizing a failed ContainerInfo so the agent is always marked FAILED even if Docker loses the container mid-wait - Add 11 unit tests covering all reconciliation paths Fixes issue-738. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: Use final_info.status instead of hardcoded EXITED in _spawn_and_wait (#841) The state-update block after wait_for_container unconditionally set container status to EXITED, overwriting the FAILED status set by the exception handler for lost containers. Use final_info.status so the persisted state correctly reflects FAILED when a container is lost mid-wait. Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * fix: Use final_info.status instead of hardcoded EXITED (#842) * fix: Use final_info.status instead of hardcoded EXITED in _spawn_and_wait * Use final_info.exited_at for consistency with final_info pattern --------- Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>

fix: Use final_info.status instead of hardcoded EXITED in _spawn_and_…

30aaf3d

…wait

james-in-a-box Bot mentioned this pull request Feb 21, 2026

fix: Handle orphaned container state on orchestrator restart #839

Merged

2 tasks

egg-reviewer Bot approved these changes Feb 21, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

Use final_info.exited_at for consistency with final_info pattern

601d475

james-in-a-box Bot commented Feb 21, 2026

View reviewed changes

jwbron closed this Feb 21, 2026

This comment has been minimized.

Sign in to view

egg-reviewer Bot reviewed Feb 21, 2026

View reviewed changes

Merge origin/fix/orphaned-container-state-reconciliation into egg/fix…

5ad213e

…-spawn-wait-status: resolve conflict in orchestrator/routes/pipelines.py

This comment has been minimized.

Sign in to view

james-in-a-box Bot reopened this Feb 21, 2026

This comment has been minimized.

Sign in to view

jwbron merged commit f18e236 into fix/orphaned-container-state-reconciliation Feb 21, 2026
7 checks passed

egg-reviewer Bot reviewed Feb 21, 2026

View reviewed changes

Navigation Menu

fix: Use final_info.status instead of hardcoded EXITED - #842

fix: Use final_info.status instead of hardcoded EXITED#842
jwbron merged 3 commits into
fix/orphaned-container-state-reconciliationfrom
egg/fix-spawn-wait-status

james-in-a-box Bot commented Feb 21, 2026

Uh oh!

egg-reviewer Bot left a comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

james-in-a-box Bot left a comment

Uh oh!

This comment has been minimized.

egg-reviewer Bot left a comment

Uh oh!

This comment has been minimized.

james-in-a-box Bot commented Feb 21, 2026

Uh oh!

This comment has been minimized.

Uh oh!

egg-reviewer Bot left a comment

Uh oh!

james-in-a-box Bot commented Feb 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

james-in-a-box Bot commented Feb 21, 2026

Uh oh!

egg-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

james-in-a-box Bot left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

egg-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

james-in-a-box Bot commented Feb 21, 2026

Conflict Resolution Summary

Details

Verification

Uh oh!

This comment has been minimized.

Uh oh!

egg-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

james-in-a-box Bot commented Feb 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant