Fix orchestrator startup crash, spawn blocking, and cancel state sync

[jwbron]

Summary

• Add missing COPY shared/egg_agent/ ./egg_agent/ to the orchestrator Dockerfile — the module was added in Migrate claude --print to Agent SDK #1088 but the Dockerfile wasn't updated, causing an ImportError crash loop on startup
• Remove the spawn dependency enforcement added in Enforce spawn dependency ordering and fix coordinator bugs #1085 — agents should run concurrently and communicate via messaging, not be blocked from spawning until dependencies complete
• Fix stale container/agent status after pipeline cancellation — cleanup_pipeline() removed Docker containers but left the pipeline state showing them as "running". Adds _mark_pipeline_records_cancelled() to sync persisted state: containers marked REMOVED, agents marked FAILED, coordinator spawn records marked cancelled

Test plan

• Rebuild orchestrator image (docker compose build orchestrator)
• Verify orchestrator starts without import errors
• Spawn multiple agents concurrently (e.g. refiner + reviewers) and confirm none are blocked by dependency checks
• Cancel a running pipeline and verify the response shows all containers/agents as stopped (not "running")
• pytest orchestrator/tests/test_pipelines_api.py — new tests for cancel state sync pass
• Coordinator route tests pass (pytest orchestrator/tests/test_coordinator_routes_functional.py)

🤖 Generated with Claude Code

[egg-reviewer]

Correct fix. The orchestrator imports build_agent_command from egg_agent at routes/pipelines.py:145, the module exists at shared/egg_agent/, and the COPY destination (./egg_agent/) follows the same pattern as the other shared modules. PYTHONPATH is /app, so the import will resolve.

Non-blocking nit: The comment on line 19 (# Copy shared modules (egg_logging, egg_config, egg_contracts, egg_container)) should be updated to include egg_agent now that five modules are copied.

— Authored by egg

[egg-reviewer]

Re-review: Changes since ce33549

Two new commits since last review:

• 9f3fe94 — Updated Dockerfile comment to include egg_agent (addresses previous nit)
• a97d400 — Removed spawn dependency enforcement from coordinator

Previous feedback addressed

The non-blocking nit about the Dockerfile comment has been fixed. The comment now correctly lists all five shared modules.

New changes: dependency enforcement removal

The removal of spawn-time dependency enforcement from coordinator.py is architecturally sound. In concurrent mode, all agents start simultaneously and coordinate via messaging — the enforcement was actively harmful there. In non-concurrent mode, the coordinator agent is responsible for spawn ordering — hardcoded API enforcement is redundant. The dependency metadata in agent_roles.py is preserved and still used by dependency_graph.py, orchestration.py, and other systems for planning purposes.

No security issues, no correctness bugs, no resource leaks.

Non-blocking suggestions

1. Orphaned section header in test file

orchestrator/tests/test_coordinator_routes_functional.py:1452 — The section header # ── Dependency validation tests ──── is left with no content below it. The test class was removed but the header was not. Should be cleaned up:

# Before (current):
# ── Dependency validation tests ────────────────────────────────────


# ── Spawn contract enforcement tests ──────────────────────────────

# After (suggested):
# ── Spawn contract enforcement tests ──────────────────────────────

2. Stale documentation

Multiple doc files still describe the removed dependency enforcement behavior. These should be updated in a follow-up:

• docs/guides/coordinator.md:146 — Lists "Role's dependencies have not yet completed" as a 409 cause
• docs/guides/coordinator.md:170 — Full paragraph describing the removed dependency enforcement at spawn time
• docs/guides/coordinator.md:400 — Troubleshooting entry for "missing dependencies" 409 error that can no longer occur

— Authored by egg

[egg-reviewer]

Re-review: Changes since a97d400

The branch was squashed into a single commit (fa090185). The new change adds _mark_pipeline_records_cancelled() to sync persisted pipeline state after container cleanup during cancellation. Previous changes (Dockerfile fix, dependency enforcement removal) are carried forward.

Previous feedback status

1. Dockerfile comment nit — Fixed.
2. Orphaned section header in test file — Not addressed. orchestrator/tests/test_coordinator_routes_functional.py:1452 still has a bare # ── Dependency validation tests ──── header with no content below it.
3. Stale documentation — Not addressed. docs/guides/coordinator.md still describes the removed dependency enforcement at lines 146, 170, and 400.

Items 2 and 3 are non-blocking and can be addressed in a follow-up, but they should be tracked.

New code: _mark_pipeline_records_cancelled()

Blocking: Incorrect error message for FAILED pipelines

orchestrator/routes/pipelines.py:596 — The agent error is hardcoded to "Pipeline cancelled", but this function is also called when pipeline.status == PipelineStatus.FAILED (the call site at line 654 triggers for both CANCELLED and FAILED). When a pipeline fails (e.g., max retries exceeded), agents will incorrectly report "Pipeline cancelled" as their error. Fix:

agent.error = f"Pipeline {pipeline.status.value}"

The function name _mark_pipeline_records_cancelled is also misleading since it handles both statuses. Consider _mark_pipeline_records_terminated or similar.

The tests only cover the CANCELLED path — add a test for FAILED to verify the error message is correct.

Non-blocking observations

1. save_pipeline called without the pipeline state lock

After store.update_pipeline() returns (releasing the per-pipeline RLock), _mark_pipeline_records_cancelled mutates the pipeline and calls store.save_pipeline() without re-acquiring the lock. A concurrent writer between these calls would have its changes silently overwritten. In practice this is unlikely for terminal states, but wrapping the mutation + save in with get_pipeline_state_lock(pipeline_id): would be more correct.

2. Lazy import is unnecessary

orchestrator/routes/pipelines.py:569-571 — AgentExecutionStatus and ContainerStatus could be added to the top-level import block (lines 39-47 / 69-82) instead of using a try/except import inside the function. The file already follows this pattern for other model types. Not a correctness issue but inconsistent style.

— Authored by egg

[egg-reviewer]

Re-review: Changes since fa090185

One new commit (fa6577a) refactors _mark_pipeline_records_cancelled to reload the pipeline from the store before mutating, avoiding stale-state overwrites. Good change — the reload-before-mutate pattern correctly handles the case where the coordinator writes updates between the status change and container cleanup.

Previous feedback status

1. Dockerfile comment nit — Fixed (earlier commit).
2. Blocking: Incorrect error message for FAILED pipelines — Still present. See below.
3. Non-blocking: Orphaned section header in test file — Not addressed. test_coordinator_routes_functional.py:1452 still has a bare # ── Dependency validation tests ──── header with no content.
4. Non-blocking: Stale documentation — Not addressed. docs/guides/coordinator.md lines 146, 170, and 400 still describe the removed dependency enforcement.
5. Non-blocking: Lazy import — Not addressed. Lines 570-573 still use try/except import inside the function.

Blocking: Hardcoded "Pipeline cancelled" error for FAILED pipelines

orchestrator/routes/pipelines.py:597 — agent.error = "Pipeline cancelled" is still hardcoded. The function is called for both CANCELLED and FAILED pipelines (the call site at line 652 triggers when pipeline.status in (PipelineStatus.CANCELLED, PipelineStatus.FAILED)).

When a pipeline fails (e.g., max retries exceeded), agents will incorrectly report "Pipeline cancelled". This is misleading during incident debugging. The function now reloads the pipeline and has access to pipeline.status, so the fix is straightforward:

agent.error = f"Pipeline {pipeline.status.value}"

The function name _mark_pipeline_records_cancelled is also misleading since it handles both statuses. Consider _mark_pipeline_records_terminated or similar.

The tests only cover the CANCELLED path — add a test for FAILED to verify the error message is correct for both statuses.

New code assessment

The refactoring itself is clean:

• Reload-before-mutate: store.load_pipeline(pipeline_id) ensures we operate on the latest state. Correct.
• Return value: The caller uses the returned pipeline for the response (pipeline = _mark_pipeline_records_cancelled(...) at line 699), so the API response reflects the synced state. Correct.
• Error handling at caller: Moving the try/except to the call site (lines 698-705) is the right layering. If the sync fails, the response still returns the pre-sync pipeline. Consistent, since the persisted state also wasn't updated.
• Test updates: Both tests now mock store.load_pipeline.return_value. Correct.

No new issues introduced by this commit.

— Authored by egg

[james-in-a-box]

Autofix tracking

{"Test/Unit Tests": 1}

[egg-reviewer]

Re-review: Changes since fa6577a

Five new commits since last review:

• c193e39 — Address review feedback (error message, function rename, docs cleanup)
• 4ad3f9a — Merge
• 153583f — Fix spawned agents missing EGG_ORCHESTRATOR_URL env var
• d138871 — Increase cancel_task MCP endpoint timeout from 30s to 120s
• d86961e — Fix test to expect timeout=120

Previous feedback status

1. Blocking: Hardcoded "Pipeline cancelled" error — Fixed. Now uses f"Pipeline {pipeline.status.value}", producing status-specific messages. New test test_failed_pipeline_uses_correct_error_message verifies the FAILED path.
2. Blocking: Misleading function name — Fixed. Renamed to _mark_pipeline_records_terminated.
3. Non-blocking: Orphaned section header in test file — Fixed. Header removed.
4. Non-blocking: Stale documentation — Fixed. All three references to dependency enforcement removed from docs/guides/coordinator.md.
5. Non-blocking: Lazy import — Fixed. AgentExecutionStatus and ContainerStatus moved to top-level import blocks.

All previously identified issues have been properly addressed.

New code: EGG_ORCHESTRATOR_URL in container_spawner.py

The fix correctly adds EGG_ORCHESTRATOR_URL to the spawner env, using hostname for public mode and IP for private mode. The mode parameter is properly in scope. This was the only spawn path missing it — sandbox_template.py (line 133) and routes/pipelines.py (line 6119) both already set it.

New code: cancel_task timeout increase

The 30s → 120s timeout for _handle_cancel_task is appropriate — cancellation involves stopping containers and syncing state, which regularly exceeds 30s. Other _make_request callers use the 30s default for lightweight GET/POST operations, which is correct.

Non-blocking suggestions

1. Inconsistent orchestrator URL construction across spawn paths

container_spawner.py:380-383 uses ORCHESTRATOR_CONTAINER_NAME (hostname) for public mode, while routes/pipelines.py:6110-6114 uses ORCHESTRATOR_EXTERNAL_IP (static IP) for the same mode. Both work because Docker resolves hostnames on attached networks, but the inconsistency could confuse future readers. Consider aligning on one approach.

2. PR body contains prohibited text

The PR body includes 🤖 Generated with [Claude Code](https://claude.com/claude-code) — per CLAUDE.md, PR descriptions should never include "Claude Code" or claude.ai links.

— Authored by egg

[james-in-a-box]

Addressed:

1. Inconsistent orchestrator URL construction — Fixed in 4c2b4c5. container_spawner.py now uses ORCHESTRATOR_EXTERNAL_IP (static IP) for public mode, matching the pattern in routes/pipelines.py.

2. PR body contains prohibited text — Cannot fix: the gateway blocks PR body edits since this PR was authored by jwbron, not james-in-a-box. The 🤖 Generated with Claude Code line in the PR body needs to be removed manually by the PR author.

— Authored by egg

[egg-reviewer]

Re-review: Changes since d86961e

One new commit since last review:

• 4c2b4c5 — Align orchestrator URL construction across spawn paths

Previous feedback status

1. Non-blocking: Inconsistent orchestrator URL construction — Fixed. container_spawner.py:382 now uses ORCHESTRATOR_EXTERNAL_IP (static IP) for public mode, matching the pattern in routes/pipelines.py:6113. The import of ORCHESTRATOR_CONTAINER_NAME from sandbox_template was cleanly removed and replaced with ORCHESTRATOR_EXTERNAL_IP from egg_config.constants, with an appropriate fallback value (172.33.0.3) in the except ImportError block.

2. Non-blocking: PR body contains prohibited text — Not addressed (correctly noted as requiring manual fix by PR author since gateway blocks bot edits to other users' PRs).

New code assessment

The change is correct and minimal:

• Import cleanup: ORCHESTRATOR_CONTAINER_NAME removed from sandbox_template import, ORCHESTRATOR_EXTERNAL_IP added to egg_config.constants import. No orphaned imports.
• Fallback value: ORCHESTRATOR_EXTERNAL_IP = "172.33.0.3" in the except ImportError block matches the canonical value in shared/egg_config/constants.py:34.
• Behavioral alignment: Both spawn paths (container_spawner.py:382 and routes/pipelines.py:6113) now use the same logic: ORCHESTRATOR_ISOLATED_IP for private mode, ORCHESTRATOR_EXTERNAL_IP for public mode. Consistent.
• No stale references: ORCHESTRATOR_CONTAINER_NAME is fully removed from container_spawner.py. It remains defined in sandbox_template.py where other consumers still use it.

No issues found.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

All review feedback has been addressed across prior commits:

1. Dockerfile comment — Fixed in 9f3fe94
2. Hardcoded error message — Fixed in c193e39 (uses f"Pipeline {pipeline.status.value}")
3. Function rename — Fixed in c193e39 (_mark_pipeline_records_terminated)
4. Orphaned test header — Fixed in c193e39
5. Stale docs — Fixed in c193e39 (removed 3 references in coordinator.md)
6. Lazy import — Fixed in c193e39 (moved to top-level imports)
7. Inconsistent URL construction — Fixed in 4c2b4c5 (aligned on ORCHESTRATOR_EXTERNAL_IP)
8. PR body prohibited text — Cannot fix: gateway blocks edits to PRs authored by jwbron. The 🤖 Generated with Claude Code line needs to be removed manually by the PR author.

No new code changes required.

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

16 previous review(s) hidden.