Skip to content

Fix #2871: don't mark an empty slice branch COMPLETE on restart - #2874

Merged
jwbron merged 1 commit into
mainfrom
egg/issue-2871-empty-slice-false-complete
May 29, 2026
Merged

Fix #2871: don't mark an empty slice branch COMPLETE on restart#2874
jwbron merged 1 commit into
mainfrom
egg/issue-2871-empty-slice-false-complete

Conversation

@jwbron

@jwbron jwbron commented May 29, 2026

Copy link
Copy Markdown
Owner

Closes #2871.

Problem

On restart_phase implement, bootstrap reconciliation falsely marked a no-work slice COMPLETE when its stale, empty integration branch was an ancestor of an advanced work branch. The slice was then skipped and its dependents ran without their prerequisite (observed on issue-2777-replan: slice-1 died at spawn, committed nothing, yet was marked complete with commit=None, review_cycles=0).

is_slice_branch_merged_into_parent treats "slice tip is an ancestor of parent" as merged → COMPLETE. An empty slice branch's tip is the parent SHA it was forked at, so once the parent advances (parent-slice progress, a main merge, or an operator patch) the empty tip becomes a trivial ancestor — indistinguishable, by ancestry alone, from a genuinely merged branch.

Fix

Git topology alone can't tell the two apart (both are ancestors of the advanced parent), so we record the branch's creation base SHA and require the tip to have moved past it before treating "ancestor of parent" as COMPLETE.

  • Slice.integration_base_sha — new optional contract field: the origin SHA the integration branch was forked at.
  • Recorded once, right after create_slice_integration_branch succeeds — the branch is fresh (tip == base) and no agent has been spawned yet, so the tip is the true base. Only written when unset, so a #2512 restart-recovery over a branch that already carries slice commits keeps its original base.
  • is_slice_branch_merged_into_parent gains an integration_base_sha kwarg: when the branch tip still equals the recorded base, return False (un-started, not merged) before the merge-base call.
  • Both call sites — bootstrap reconciliation and the mid-run race check — pass the recorded base.

The change is purely additive: when the base is unknown (slices provisioned before this field existed), the check falls back to its prior ancestor-only behaviour, so the #2549 already-merged-skip path is preserved. This favours a loud non-fast-forward failure over silent corruption for the narrow legacy-branch window.

Tests

  • New test_empty_branch_at_creation_base_is_not_merged: empty branch (tip == recorded base) → False, and the merge-base call is short-circuited.
  • New test_recorded_base_does_not_block_genuinely_merged_branch: a tip past its base that is an ancestor of parent still reports True (guard is additive).
  • Full test_create_slice_integration_branch.py (26 tests) passes; lint clean. Changeset-aware make test was running at PR-open time.

Part of the 3-bug sequence (#2869, #2870, #2871) gating the issue-2777-replan resume.

is_slice_branch_merged_into_parent treated 'slice tip is an ancestor of
parent' as merged → COMPLETE. An empty (un-started) slice branch's tip
is exactly the parent SHA it was forked at, so when the parent advances
it becomes a trivial ancestor — falsely marking the slice complete and
skipping it, leaving dependents to run without their prerequisite.

Git topology alone can't tell an empty branch from a merged one (both
are ancestors of the advanced parent), so record the branch's creation
base SHA and require the tip to have moved past it.

- Slice.integration_base_sha: new optional contract field, the origin
  SHA the integration branch was forked at.
- Persist it once, right after create_slice_integration_branch succeeds
  (branch fresh, tip == base, no agent spawned yet).
- is_slice_branch_merged_into_parent gains an integration_base_sha kwarg:
  when the tip still equals the recorded base, return False (un-started).
  Purely additive — an unknown base (legacy slices) falls back to the
  prior ancestor-only check, so #2549 behaviour is preserved.
- Both call sites (bootstrap reconciliation + mid-run race check) pass
  the recorded base.
@james-in-a-box

Copy link
Copy Markdown
Contributor

egg is investigating the Test check failure...

  • Integration Tests / Integration Tests

@james-in-a-box

Copy link
Copy Markdown
Contributor

Check Failure: Integration Tests / Integration Tests

What's failing: The Integration Tests tier failed during cluster setup, before any integration test executed. The Cilium CNI install could not reach the k3s API server:

Error: Unable to install Cilium: cluster reachability check failed:
kubernetes cluster unreachable: Get "https://127.0.0.1:6443/version":
dial tcp 127.0.0.1:6443: connect: connection refused
Cilium install failed — restarting k3s and retrying...
Error: Unable to install Cilium: cluster reachability check failed: ...
##[error]Process completed with exit code 1.

Root cause: A transient CI infrastructure race, not a code defect. The cilium install step ran while the freshly-started k3s API server (port 6443) was still coming up. The workflow's one retry restarted k3s and re-attempted Cilium only ~2s later — too soon for the control plane to stabilize — so both attempts hit connection refused.

This is unrelated to this PR. The Test workflow's other tiers all passed (Unit Tests: success, Security Scan: success), and the PR only touches Python orchestrator code (orchestrator/gateway_client.py, orchestrator/routes/pipelines.py, orchestrator/tests/test_create_slice_integration_branch.py, shared/egg_contracts/models.py) — none of which is involved in k3s/Cilium bring-up.

What needs to be done:

  • Re-run the failed Integration Tests job — the k3s/Cilium race usually clears on a fresh runner. (gh run rerun 26621034109 --failed, or re-run from the Actions UI.) gh run rerun is blocked through the agent gateway, so this needs a human.

Suggestion (workflow hardening, separate from this PR): make the integration setup wait for k3s API readiness before installing Cilium, e.g. poll kubectl get --raw='/readyz' (or https://127.0.0.1:6443/version) until it returns 200, and add a short backoff between the k3s restart and the Cilium retry rather than retrying immediately.

— Authored by egg

@james-in-a-box

Copy link
Copy Markdown
Contributor
Autofix tracking
{"Test/Integration Tests / Integration Tests": 1}

@james-in-a-box

Copy link
Copy Markdown
Contributor

egg check fixer completed for Test. CI will re-run to verify. View run logs

— Authored by egg

@jwbron
jwbron merged commit 5636c58 into main May 29, 2026
21 of 24 checks passed
jwbron added a commit that referenced this pull request May 29, 2026
Document the new Slice.integration_base_sha field added in #2874. The
slice-dag architecture doc maintains a table of notable Slice model fields;
the new field should be listed there so readers understand its purpose in
distinguishing empty (un-started) slice branches from genuinely merged ones
on pipeline restart.

Authored-by: egg

Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com>
james-in-a-box Bot pushed a commit that referenced this pull request May 29, 2026
is_slice_branch_merged_into_parent treated 'slice tip is an ancestor of
parent' as merged → COMPLETE. An empty (un-started) slice branch's tip
is exactly the parent SHA it was forked at, so when the parent advances
it becomes a trivial ancestor — falsely marking the slice complete and
skipping it, leaving dependents to run without their prerequisite.

Git topology alone can't tell an empty branch from a merged one (both
are ancestors of the advanced parent), so record the branch's creation
base SHA and require the tip to have moved past it.

- Slice.integration_base_sha: new optional contract field, the origin
  SHA the integration branch was forked at.
- Persist it once, right after create_slice_integration_branch succeeds
  (branch fresh, tip == base, no agent spawned yet).
- is_slice_branch_merged_into_parent gains an integration_base_sha kwarg:
  when the tip still equals the recorded base, return False (un-started).
  Purely additive — an unknown base (legacy slices) falls back to the
  prior ancestor-only check, so #2549 behaviour is preserved.
- Both call sites (bootstrap reconciliation + mid-run race check) pass
  the recorded base.
james-in-a-box Bot added a commit that referenced this pull request May 29, 2026
Document the new Slice.integration_base_sha field added in #2874. The
slice-dag architecture doc maintains a table of notable Slice model fields;
the new field should be listed there so readers understand its purpose in
distinguishing empty (un-started) slice branches from genuinely merged ones
on pipeline restart.

Authored-by: egg

Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant