fix: Use final_info.status in _spawn_and_wait state update - #841
Merged
jwbron merged 1 commit intoFeb 21, 2026
Merged
Conversation
…wait 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.
2 tasks
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: fix container status overwrite
Verdict: Approve (posted as comment — self-authored PR)
Analysis
The one-line change is correct. Before this fix, the state-update block at line 3416 unconditionally set ci.status = ContainerStatus.EXITED, which overwrote the FAILED status synthesized by the exception handler (lines 3380–3392) when a container is lost mid-wait via ContainerNotFoundError or ContainerOperationError. This meant orphaned containers would appear as cleanly exited rather than failed, defeating the reconciliation logic in PR #839.
After the fix, ci.status = final_info.status correctly propagates:
ContainerStatus.EXITEDfrom the normalwait_for_containerreturn path (docker_client.py:476)ContainerStatus.FAILEDfrom the exception handler path (pipelines.py:3389)
Both paths always return a ContainerInfo with a valid ContainerStatus, so there's no risk of final_info.status being None or an unexpected value.
Minor pre-existing observation (not blocking)
Line 3417 sets ci.exited_at = datetime.utcnow() rather than using final_info.exited_at, which is already populated in both the normal path (docker_client.py:478) and the exception path (pipelines.py:3391). The time delta is negligible, but using final_info.exited_at would be more consistent with the pattern this fix establishes. Not in scope for this PR.
PR description note
The PR body mentions that PR #839's description contains a 🤖 Generated with [Claude Code] line that violates project conventions. That's a valid observation — flagging it as a separate concern for the #839 author.
No issues found. Clean fix.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
Contributor
Author
| egg is addressing review feedback... |
Contributor
Author
|
egg feedback addressed. View run logs 1 previous review(s) hidden. |
jwbron
merged commit Feb 21, 2026
a570e84
into
fix/orphaned-container-state-reconciliation
11 checks passed
jwbron
added a commit
that referenced
this pull request
Feb 21, 2026
* 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>
james-in-a-box Bot
pushed a commit
that referenced
this pull request
Feb 21, 2026
When GATEWAY_URL and EGG_SESSION_TOKEN are set (all sandbox containers), the checkpoint CLI uses the gateway HTTP API. The HTTP code path never passed the --checkpoint-repo flag to the gateway, and the gateway endpoints only auto-detected checkpoint_repo from repo config — which silently fails when repositories.yaml isn't accessible from the gateway's perspective. Result: all checkpoint CLI commands return 0 results. Fix both sides: - CLI: pass checkpoint_repo in HTTP params for all five command paths (list, show, browse, context, cost) - Gateway: add _resolve_checkpoint_repo() that accepts an explicit checkpoint_repo query param with fallback to auto-detection; use it in all three checkpoint endpoints (list, show, cost) Closes #841
jwbron
pushed a commit
that referenced
this pull request
Feb 21, 2026
* Fix checkpoint CLI ignoring --checkpoint-repo in HTTP mode When GATEWAY_URL and EGG_SESSION_TOKEN are set (all sandbox containers), the checkpoint CLI uses the gateway HTTP API. The HTTP code path never passed the --checkpoint-repo flag to the gateway, and the gateway endpoints only auto-detected checkpoint_repo from repo config — which silently fails when repositories.yaml isn't accessible from the gateway's perspective. Result: all checkpoint CLI commands return 0 results. Fix both sides: - CLI: pass checkpoint_repo in HTTP params for all five command paths (list, show, browse, context, cost) - Gateway: add _resolve_checkpoint_repo() that accepts an explicit checkpoint_repo query param with fallback to auto-detection; use it in all three checkpoint endpoints (list, show, cost) Closes #841 * Fix checkpoint_repo missing from nested HTTP calls in context --files Address review feedback: pass checkpoint_repo through to per-checkpoint _http_get calls in _cmd_context_http (both JSON and text output paths), add logger.warning on invalid checkpoint_repo format in gateway, and add test coverage for checkpoint_cost endpoint's use of _resolve_checkpoint_repo. --------- Co-authored-by: egg <egg@localhost> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the container status overwrite bug identified in review of PR #839.
The state-update block in
_spawn_and_waitunconditionally setci.status = ContainerStatus.EXITED, overwriting theFAILEDstatus synthesized by the exception handler when a container is lost mid-wait. Changed toci.status = final_info.statusso the persisted state correctly reflectsFAILEDfor lost containers andEXITEDfor normal exits.Also: PR #839 description contains a
🤖 Generated with [Claude Code]line that violates project conventions — should be removed from the PR body.Issue: none (review feedback on #839)
Test plan:
ruff checkclean on the changed fileAuthored-by: egg