Skip to content

fix(forge): do not let a killed campaign's leftovers fail the next task - #1564

Merged
BaoYunkai merged 5 commits into
mainfrom
bugfix/yunkai/forge-campaign-leftovers
Sep 18, 2026
Merged

BaoYunkai merged 5 commits into
mainfrom
bugfix/yunkai/forge-campaign-leftovers

Conversation

@BaoYunkai

@BaoYunkai BaoYunkai commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

Problem

Every in-place task in one repository is handed the same forge_experiments
directory. The release archives it, but a run the host killed never reaches
that, and the leftover then does two things:

  1. forge-loop refuses the workspace outright (already contains a Forge campaign; pass --resume to continue it), failing the next task at dispatch.
  2. Recovery reads the stale manifest as that task's own best result, reporting
    the dead campaign's commit as a missing base commit -- a reason with nothing
    to do with the task it lost.

Session 20260914T115354Z-5262e947 lost chunk_gated_delta_rule this way to a
fusion campaign left behind at a timeout an hour earlier.

Fix

Archive a leftover on the way in as well as on the way out, and require a
trusted manifest's commit to still exist in the repository before believing it
describes this task. The leftover is archived rather than deleted: for a run
that published nothing it is the only account of what that run did.

An archive that cannot be made now says so. The log line announcing it sat
inside the contextlib.suppress that guards the move, so it only ever fired on
success; a stale forge_experiments/workspace.lock still held open, an
unwritable operator directory or a cross-device move left the borrow continuing
as though the leftover were gone. forge-loop then failed the task at dispatch
and nothing in the log said an archive had been attempted -- the undiagnosable
failure this change exists to end.

Test plan

  • src/kernelforge/tests/kernel_rewrite_controller/ (275 passed), including
    a new test that a failed archive is reported rather than swallowed
  • ruff check . + ruff format --check .
  • CHANGELOG.md Unreleased / Fixed entry

Every in-place task in one repository is handed the same forge_experiments
directory. The release archives it, but a run the host killed never reaches
that, and the leftover then does two things: forge-loop refuses the workspace
outright ('already contains a Forge campaign; pass --resume to continue it'),
failing the next task at dispatch, and recovery reads the stale manifest as
that task's own best result -- reporting the dead campaign's commit as a
missing base commit, a reason with nothing to do with the task it lost.

Session 20260914T115354Z-5262e947 lost chunk_gated_delta_rule this way to a
fusion campaign left behind at the timeout an hour earlier.

Archive a leftover on the way in as well as on the way out, and require a
trusted manifest's commit to still exist in the repository before believing
it describes this task.
@BaoYunkai
BaoYunkai requested a review from a team as a code owner September 18, 2026 04:45
@xiaofei-zheng

Copy link
Copy Markdown
Collaborator

Reviewed the dispatch/recovery paths against git merge-base origin/main bugfix/yunkai/forge-campaign-leftovers: _borrow_live_repository ordering (reclaim -> checkout -> archive -> _ignore_forge_loop_output), the has_run_artifacts gate in cli.py:1041-1050 the archive is meant to clear, _restore_tree_to_base/remove_foreign_untracked on the release side, and both recovery entry points (dispatcher.py:215/273, controller.py:329) for the new commit_exists gate. Two blocking issues.

1. worktree.py:400-409 — a failed archive is swallowed with no trace.

with contextlib.suppress(OSError, shutil.Error):
    destination.mkdir(parents=True, exist_ok=True)
    target = destination / f"stale_{FORGE_LOOP_OUTPUT_DIRNAME}"
    shutil.rmtree(target, ignore_errors=True)
    shutil.move(str(source), str(target))
    log.warning(...)

The log.warning is inside the suppressed block, so it only fires on success. If mkdir/move raises — a stale forge_experiments/workspace.lock still held open, an unwritable operator directory, a cross-device move that shutil cannot complete — the borrow continues as if the leftover were gone, _ignore_forge_loop_output writes into the leftover, and forge-loop then fails the task at dispatch with "already contains a Forge campaign" and nothing in the log says the archive was attempted. That is exactly the undiagnosable failure this PR exists to remove. Log the failure in an except branch (or let it propagate — the borrow cannot succeed anyway).

2. No CHANGELOG.md entry.

Unreleased / Fixed carries fixes of this kind and scope ("The authored-lane retry state did not survive a resume", "Seven Coordinator-internal enqueues took no lane lease"), and the comparable recent forge fix #1528 added one. A task silently lost to another campaign's leftovers belongs there. Please add an entry in the existing format.

No other blocking issues: the commit_exists gate does not reject a live result on either path (a private worktree's commits stay resolvable from repo_root's shared object store, and the in-place recovery at dispatcher.py:215/273 runs while the campaign branch is still alive), and stale_forge_experiments does not collide with the release archive that _trusted_manifest reads back.

BaoYunkai added 2 commits September 18, 2026 14:38
The log line announcing the archive sat inside the contextlib.suppress that
guards it, so it only ever fired on success. A stale workspace.lock still held
open, an unwritable operator directory or a cross-device move left the borrow
continuing as though the leftover were gone; forge-loop then failed the task at
dispatch with "already contains a Forge campaign" and nothing in the log said an
archive had been attempted -- the undiagnosable failure this archive exists to
end. Report the failure instead, and name the remedy.

CHANGELOG records the fix.
…ampaign-leftovers

# Conflicts:
#	CHANGELOG.md
@xiaofei-zheng

Copy link
Copy Markdown
Collaborator

Re-reviewed at 236b0544b. Both earlier blockers are resolved: _archive_stale_campaign_output now reports the failure and names the remedy, with the success log moved out of the guarded block and test_an_archive_that_failed_says_so covering it; the CHANGELOG entry is there, and the merge conflict was resolved without dropping anything from main (2113 lines = main's 2098 + the 15 added here).

One blocker left.

The PR description still describes the first commit. The diff has since gained a behaviour change and two files the description does not mention:

  • The Fix section names only the two original changes (archive on the way in, require the manifest's commit to exist). It says nothing about the third: an archive that cannot be made now logs a warning naming the manual remedy, instead of leaving the borrow to continue silently. That is the change 32fcadbea exists for and a reviewer reading the description would not know to look for it.
  • Test plan still says 274 passed, which predates the three tests added here and the merge of main.

Please refresh both before merge.

@xiaofei-zheng xiaofei-zheng left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at 236b0544b. Both blockers from the first pass are fixed: _archive_stale_campaign_output reports a failed archive and names the manual remedy instead of swallowing it, with the success log moved out of the guarded block and test_an_archive_that_failed_says_so covering the failure path; the CHANGELOG entry is in Unreleased / Fixed in the existing format.

What I checked:

  • _borrow_live_repository ordering — reclaim, checkout, archive, _ignore_forge_loop_output — and that the archive clears exactly what has_run_artifacts in cli.py:1041-1050 refuses a workspace for.
  • The release side (_archive_campaign_output, _restore_tree_to_base, remove_foreign_untracked) for a collision between stale_forge_experiments and the archive _trusted_manifest reads back: there is none.
  • The commit_exists gate on both recovery entry points against git merge-base origin/main <branch>. It does not reject a live result: a private worktree's commits stay resolvable from repo_root's shared object store, and in-place recovery at dispatcher.py:215/273 runs while the campaign branch is still alive.
  • The CHANGELOG merge conflict resolution: 2113 lines = main's 2098 plus the 15 added here, so nothing from main was dropped.

Tests were not run locally (no environment here); CI covers them.

The description still describes the first commit — it does not mention the archive-failure warning, and Test plan predates the three tests added since. Worth refreshing before merge, but not holding the approval.

BaoYunkai added 2 commits September 18, 2026 15:34
…ampaign-leftovers

# Conflicts:
#	CHANGELOG.md
…ampaign-leftovers

# Conflicts:
#	CHANGELOG.md
@BaoYunkai
BaoYunkai merged commit cfdad16 into main Sep 18, 2026
32 checks passed
@BaoYunkai
BaoYunkai deleted the bugfix/yunkai/forge-campaign-leftovers branch September 18, 2026 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants