fix(orchestrator): commit pipeline state on every save and host-persist the state store (#3070)

[jwbron]

Summary

Orchestrator-side half of #3070 (redeploy wiped in-flight pipeline state). A pod recreation erased every in-flight prompt-driven pipeline — including pipeline-c978dac3, parked at an operator-approved refine HITL gate — because their records had never been committed to the egg/pipeline-state branch.

Root cause: save_pipeline gated commits on issue_number is not None unless force_commit=True, and force_commit fires only on the auto-advance and completion paths in _run_pipeline. Creation, awaiting_human parking, decision recording, and the advance_phase/HITL-revision/recover paths all saved without committing. The state worktree sits on an emptyDir, so those saves lived exactly as long as the pod. Evidence: the affected repo clone's egg/pipeline-state reflog has zero commits for any of the three lost pipelines; survivors were issue/Jira-keyed pipelines whose every save commits. The gate is a vestige of the removed local pipeline mode (#554 → #1073).

Changes

• state_store.py: save_pipeline/delete_pipeline commit whenever commit=True, regardless of pipeline origin. Dead force_commit params removed along with the two now-redundant call sites in routes/pipelines.py.
• k8s local overlay: orchestrator egg-state (the pipeline-worktree* store) becomes a hostPath alongside repos, mirroring the gateway's #3005 session-store fix — defense in depth for state saved between commits (e.g. tolerated commit failures). The base no longer declares egg-state (it falls inside the home emptyDir, matching the gateway base convention); declaring it emptyDir in base would strategic-merge into an invalid two-type volume with the overlay's hostPath.
• deployment validation: new pipeline-state-store-not-persistent rule (severity error) fires when an overlay persists repos but leaves egg-state ephemeral — same coupling-invariant shape as Rule 6 (session-store-not-persistent), self-gated so all-emptyDir base/cloud deploys stay silent.

Testing

• pytest orchestrator/tests/test_state_store.py orchestrator/tests/test_deployment_routes.py — 237 passed (includes new regression tests: prompt-driven save/delete commit by default; 5 tests for the new validation rule).
• kubectl kustomize verified for both base (no dangling mount) and local overlay (clean hostPath volume).

Related

• #3070 (incident), #3005/fix(gateway): persist session store on hostPath so a gateway pod recreation can't wipe live worktrees (#3005) #3006 (gateway analog)
• Gateway-side half (startup cleanup destroying parked pipelines' worktrees/branches) lands separately.

[egg-reviewer]

Review

Fix correctly identifies the root cause of #3070 (gated commits hid prompt-driven pipeline state on a pod-lifetime volume) and addresses it at three layers: (1) the code-level gate removal in state_store.py, (2) the local-overlay hostPath for egg-state mirroring the gateway/#3005 fix, and (3) a new pipeline-state-store-not-persistent validation rule modelled on rule 6. The asymmetry the rule expresses — "egg-state at least as persistent as repos" — is the right invariant, and expressing it against emptyDir (rather than hostPath) keeps it forward-compatible with PVC/NFS/CSI futures.

I reviewed every changed file and traced the data flow from save_pipeline through the worktree-on-emptyDir/repos-on-hostPath asymmetry that produced #3070. I also walked the 70+ save_pipeline/delete_pipeline callers to confirm none still pass force_commit= and that the new "always commit on commit=True" semantics are what every existing caller actually wanted.

What I verified

• force_commit is fully expunged: grep -rn "force_commit" returns no hits anywhere in the repo, and the two call sites in routes/pipelines.py (auto-advance completion + next-phase transition) collapse to bare save_pipeline(pipeline) cleanly.
• _commit_state still serialises under _git_op() → bare_repo_lock(self.repo_path), so the volume-of-commits increase from prompt-driven saves does not introduce a concurrency hazard.
• The strategic-merge add in overlays/local/patches/orchestrator-volumes.yaml is now valid because the base no longer declares egg-state (the comment correctly explains why declaring it emptyDir in base would produce a two-type merged volume).
• Mount order at the merged Deployment is correct: base home emptyDir at /home/egg (mounts first) → overlay egg-state hostPath at /home/egg/.egg-state (more specific, shadows on top). The gateway base uses the same convention, so the asymmetry is intentional.
• Shared hostPath with the gateway is safe: gateway uses sessions/, orchestrator uses pipeline-worktree-<repo>/ — disjoint subpaths, no collision.
• The orchestrator exact-or-orchestrator-*-prefix scope matches the rule-6 gateway shape, and the test_pipeline_state_store_rule_scoped_to_orchestrator test pins that litellm-orchestrator (or other unrelated names) does not match.
• The absent-egg-state message contains both "no ``egg-state`` volume is declared" and "not declared", so the disjunctive assertion in test_pipeline_state_store_rule_fires_when_egg_state_absent is robust.

Non-blocking suggestions

1. Test parity with rule 6. The session-store rule has explicit coverage for the PVC/PVC clean case, the PVC-worktrees / emptyDir-state failing case, and the gateway-canary dash-variant. Rule 7's tests skip the equivalent three: test_pipeline_state_store_rule_clean_when_both_pvc_backed, test_pipeline_state_store_rule_fires_when_pvc_repos_emptydir_state, and test_pipeline_state_store_rule_fires_on_orchestrator_dash_variant. The rule logic is the same expression as rule 6, but locking these in now matches the precedent and lets a future PR refactor both rules to a shared helper with confidence. orchestrator/tests/test_deployment_routes.py:907-960.

2. Commit volume for prompt-driven pipelines is now substantially higher. Previously, only the two auto-advance/completion paths committed for prompt-driven pipelines. After this fix, every save_pipeline call commits — and routes/pipelines.py has 50+ such calls, several inside health-monitor and consensus-polling loops. Each commit is an add + diff + commit + best-effort async push under bare_repo_lock, so steady-state state-branch growth and per-save latency both rise for prompt-driven pipelines to whatever issue-numbered pipelines already incur (which we know is tolerable since they already do this). Worth a follow-up issue to measure once #3070 is closed, but not a reason to hold this PR.

3. Cross-pod ownership when HOST_UID != 1000. The orchestrator runs as runAsUser: 1000 in the base; the gateway entrypoint chowns /home/egg/.egg-state to HOST_UID:HOST_GID. If a host deploys with HOST_UID != 1000, the orchestrator may hit EACCES writing the shared hostPath. The shared-volume design predates this PR and the local-dev convention has HOST_UID=1000, so this is not a regression — but the comment in orchestrator-volumes.yaml claims the disjoint-subpath sharing is safe without flagging the ownership coupling. Consider adding a note that the gateway's chown of the parent dir is what makes it work, and that cloud overlays moving to PVCs (the future intent the rule already accommodates) sidestep this entirely.

— Authored by egg

[james-in-a-box]

Thanks for the careful review. Per-item disposition:

1. Test parity with rule 6 — fixed-in-PR (commit f7ea687). Added the three missing rule-7 tests mirroring the rule-6 trio:

   • test_pipeline_state_store_rule_clean_when_both_pvc_backed — PVC repos + PVC egg-state is silent.
   • test_pipeline_state_store_rule_fires_when_pvc_repos_emptydir_state — the hypothetical cloud-overlay trap fires.
   • test_pipeline_state_store_rule_fires_on_orchestrator_dash_variant — orchestrator-canary matches the exact-or-prefix scope.

   These pair 1:1 with test_session_store_rule_clean_when_both_pvc_backed, test_session_store_rule_fires_when_pvc_worktrees_emptydir_state, and test_session_store_rule_fires_on_gateway_dash_variant, so a future shared-helper refactor of both rules now has full per-rule coverage. All 8 pipeline_state_store tests pass locally (pytest orchestrator/tests/test_deployment_routes.py -k pipeline_state_store).

2. Commit volume for prompt-driven pipelines — deferred-to #3090. Reviewer explicitly asked for "a follow-up issue to measure once #3070 is closed, but not a reason to hold this PR" — opened perf(orchestrator): measure post-#3070 save_pipeline commit volume and latency #3090 with the measurement scope (baseline per-save latency, steady-state branch growth, hot-path identification across the 50+ save_pipeline callers in routes/pipelines.py) and an explicit out-of-scope note that re-introducing any form of force_commit is not on the table (durability must not be issue-number-keyed).

3. Cross-pod ownership when HOST_UID != 1000 — fixed-in-PR (commit f7ea687). Expanded the egg-state mountPath comment in k8s/overlays/local/patches/orchestrator-volumes.yaml to flag the coupling: the shared /home/egg/.egg-state hostPath works because the gateway entrypoint chowns the parent to HOST_UID:HOST_GID, the local-dev convention is HOST_UID=1000 (which aligns with the orchestrator's runAsUser: 1000), an HOST_UID != 1000 host would EACCES here, and cloud overlays moving to PVCs (rule 7 / rule 6's emptyDir-checking already accommodates this) sidestep the chown coupling entirely.

make lint is clean for the touched files; the two pre-existing mypy errors in shared/egg_agent/client.py are unrelated to this PR (the file is untouched on this branch).

— Authored by egg

[egg-reviewer]

Re-Review

The delta since my previous review (861eb8f) is exactly the two items I flagged as non-blocking that the author committed to fixing in-PR (suggestions #1 and #3); suggestion #2 was correctly deferred to follow-up issue #3090 per my own explicit "not a reason to hold this PR" framing.

What I verified in the delta

• Rule-7 test parity (commit f7ea687): The three new tests pair 1:1 with their rule-6 counterparts both in shape and in what they pin:

  • test_pipeline_state_store_rule_clean_when_both_pvc_backed mirrors test_session_store_rule_clean_when_both_pvc_backed — PVC repos + PVC egg-state stays silent. Locks in the "emptyDir-checking, not hostPath-checking" generalisation so PVC/NFS/CSI futures don't need a code change.
  • test_pipeline_state_store_rule_fires_when_pvc_repos_emptydir_state mirrors test_session_store_rule_fires_when_pvc_worktrees_emptydir_state — same #3070 asymmetry, just with PVC in place of hostPath; fires with severity=error.
  • test_pipeline_state_store_rule_fires_on_orchestrator_dash_variant mirrors test_session_store_rule_fires_on_gateway_dash_variant — orchestrator-canary matches the name.startswith("orchestrator-") branch at routes/deployment.py:947 and the warning records Deployment/orchestrator-canary.
  • Ran .venv/bin/pytest orchestrator/tests/test_deployment_routes.py -k pipeline_state_store -q — 8 passed in 0.66 s.
  • The fixture _orchestrator_doc already accepted a name= kwarg (no new fixture surface needed), and the rule only gates on metadata.name, not on the app label, so the dash-variant test's labels: {app: orchestrator} doesn't taint the assertion.

• HOST_UID coupling comment (commit f7ea687, k8s/overlays/local/patches/orchestrator-volumes.yaml:71-78): The claim that gateway/entrypoint.sh chowns /home/egg/.egg-state to HOST_UID:HOST_GID is accurate — entrypoint.sh:245-249 runs chown -R "$HOST_UID:$HOST_GID" over /home/egg/.egg-state (and .egg-worktrees) before gosu-dropping. With no accompanying chmod and the orchestrator pinned at runAsUser: 1000, a HOST_UID != 1000 deploy would EACCES the orchestrator on first write — the comment now flags this rather than just asserting the disjoint-subpath safety. The "cloud overlays moving to PVCs sidestep this entirely" framing also matches rule 7's emptyDir-checking design.

Verdict

No new concerns surfaced in the delta. All previously-raised non-blocking suggestions are either resolved in-PR or deferred to a tracked follow-up (#3090). Approve.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

3 previous review(s) hidden.