Skip to content

bump-hrx: a mirror hold, so AMD's rebase is a reported hold instead of a daily failure — holds nothing until the owner names the commit (Refs #1945) - #2110

Merged
bong-water-water-bong merged 1 commit into
mainfrom
cl-flm/1945-llama-hold
Nov 11, 2026
Merged

bong-water-water-bong merged 1 commit into
mainfrom
cl-flm/1945-llama-hold

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Refs #1945 (release-blocker; I lead the row). This PR does not move any branch and does not hold anything by itself. It adds the mechanism that lets the owner close the blocker with a one-line decision, and it states what that decision defers.

Where #1945 actually stands

The scheduled bump-hrx run fails because AMD re-committed its HRX line on a base 1777 upstream commits newer, so AMD's pin no longer descends from our mirror. The script cannot merge that by rule and, correctly, refuses:

$ DRY_RUN=1 AMD_LLAMA=9bf5871310a8… scripts/bump-hrx-sync.sh llama          # main today, live repositories
::error::AMD rebased its line and dropped the old one: 1 commit(s) of 1bit/hrx-vulkan are on no AMD branch … They are AMD's commits, NOT ours …
rc=1

It will fail every day until the pin is merged by hand. I sized that merge three ways, without a checkout (git merge-tree), fork tip 7a1f2b81c, pin 9bf587131:

merge base used conflicted files conflict hunks
the true merge base (11924d4c1, 2026-08-02) 201 2369
the last AMD commit our line took (43b0ec2c41cc), as cl-hrxkern proposed on the issue 149 538
of those, in the HRX backend paths 65 83
of those, elsewhere 84 455

Of the 84 files outside the backend, 8 resolve by evidence (our version is an older version from the pin's own history: take the pin's). The other 76 are files both lines changed: our line sits on upstream of 2026-08-02 with later upstream changes ported in by hand, AMD's new line sits on upstream of 2026-10-08 and has those changes natively. src/models/qwen4exp.cpp (55 hunks), deepseek41.cpp (28), llama-model.cpp (24), llama-memory-hybrid-idx.cpp (24) lead the list. That is days of work for the lanes that own those files, plus a full build and the hw gate, and two things outside any lane's reach are still open: the fork push needs a workflow-scope token, and the hrx-system hold contradicts a plain merge. Per-file lists are on the box in ~/lb/hrx-pin-1945/ (hunks-vbase.txt, class-vbase.txt).

So the merge is real work that is sized and assigned, and the red scheduled job adds no information to it.

What this PR adds

config/llama-mirror-hold.config, the same shape as the hrx-system hold. While it names the mirror's commit:

  • the sync reports the hold in the run summary, moves nothing and exits 0; the engine's pin still follows our own line;
  • the run fails, naming the file, when the hold is stale (the mirror is no longer the named commit) or unneeded (AMD's pin contains the named commit again, so an ordinary sync would work). A hold cannot outlive the state it describes.

The shipped file names no commit, so merging this PR changes nothing in any run. The state it would record is written in the file's comment, with the line to add.

Proof

Dry run of the real script against the live fork and AMD's repository, nothing pushed, GH_TOKEN unset:

no hold file:   rc=1   ::error::AMD rebased its line and dropped the old one …
hold names 44ee5710d19f (the mirror):   rc=0
::notice::1bit/hrx-vulkan is held at 44ee5710d19f by …/hold.config: AMD's pin 9bf5871310a8 does not descend from it (AMD rewrote its line). Nothing was moved; …
summary: - llama.cpp: mirror `1bit/hrx-vulkan` HELD at `44ee5710d19f` (…); AMD's pin is `9bf5871310a8`, on a line that does not contain it. Not merged.
output: rebased=7a1f2b81ce17aaa84a40b766d1b06e5fd4601196

rebased equal to the engine's current pin is what makes the workflow's last step print "nothing to move" and stop.

tests/bump_hrx_sync_test.sh (ctest bump_hrx_sync, already registered), 9 new arms on a local repository: no file; a commit only mentioned in a comment; held; stale; unneeded (pin descends from the mirror, and pin equal to the mirror); a commit line with a trailing note and a non-commit line; a short hash is not a hold; the shipped file holds nothing. bump_hrx_sync: ok.

tools/copyright.py --check rc 0; tests_referenced_check.py clean. Docs: docs/hrx.md, "The mirror hold".

The decision this leaves, and it is not mine

Adding the one commit line to config/llama-mirror-hold.config turns the daily failure into a reported hold and lets #1945 close for the release, with the merge carried as backlog work. Not adding it keeps the job red until the merge is done. The orchestrator's earlier instruction on this issue was to do the merge (new dated mirror branch, normal merge PR, no force-move); a hold defers that, it does not replace it, so I have not added the line.

What I did not do

  • No branch of the fork was touched. The dry run pushes nothing.
  • The workflow file is unchanged (this token cannot push it, and it does not need to change: the script's outputs are the ones the workflow already reads). The new llama_held output is not shown in the bump PR body, since no bump PR is opened while held.
  • The merge itself: nothing resolved beyond the classification above.

🤖 Generated with Claude Code

… merge is reported, not failed daily (Refs #1945)

config/llama-mirror-hold.config: while it names the mirror's commit, the
sync reports the hold, moves nothing and succeeds. The run fails, naming
the file, when the hold is stale (the mirror moved) or unneeded (AMD's
pin contains the held commit again). The shipped file names no commit:
it holds nothing until the owner adds the line.

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

ready for review at e937372

@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1945 - Partially compliant

Compliant requirements:

  • The PR adds a mechanism to let the owner close the blocker with a one-line decision via the config/llama-mirror-hold.config file
  • The PR states what that decision defers, explaining the situation and the merge conflict details
  • The PR addresses the root cause of the failure by introducing a hold mechanism for the mirror

Non-compliant requirements:

  • The PR does not actually move any branch or hold anything by itself, as stated in the PR description

Requires further human verification:

  • The actual impact of the hold mechanism needs to be verified in a real run
  • The specific commit hash mentioned in the documentation should be validated against the actual state of the repository
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Hold File Validation

The script introduces a new hold mechanism for the llama mirror, but it relies on the existence and correctness of the config/llama-mirror-hold.config file. If this file is not properly maintained or contains invalid commit hashes, the hold mechanism may not function as intended, potentially leading to incorrect behavior during the sync process.

  if [ -z "$hold" ]; then echo none; return; fi
  if [ "$hold" != "$mirror" ]; then echo stale; return; fi
  if [ "$mirror" = "$pin" ] || git -C "$repo" merge-base --is-ancestor "$mirror" "$pin" 2>/dev/null; then echo unneeded; return; fi
  echo held
}
Mirror Hold Decision Logic

The logic in llama_hold_decision function determines whether the mirror is held, stale, or unneeded. If the logic is flawed, it could lead to incorrect decisions about when to apply the hold, potentially causing the sync to fail or proceed incorrectly.

llama_hold_decision() {
  local repo=$1 hold=$2 mirror=$3 pin=$4
  if [ -z "$hold" ]; then echo none; return; fi
  if [ "$hold" != "$mirror" ]; then echo stale; return; fi
  if [ "$mirror" = "$pin" ] || git -C "$repo" merge-base --is-ancestor "$mirror" "$pin" 2>/dev/null; then echo unneeded; return; fi
  echo held
}

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

VERDICT at e937372: READY

This is the right resolution for #1945, and the reason is that it converts a permanent red into a reported state. "A rewrite of AMD's line that the rules above cannot merge leaves the daily run with nothing to do but fail, every day, until somebody does the merge by hand" — a scheduled job that fails every day for a week is a job nobody reads, so the failure stops carrying information.

The hold is shaped well:

  • it is data (config/llama-mirror-hold.config naming the mirror's commit), so the state is visible and reviewable rather than encoded in a script;
  • the summary says the whole situation: mirror 1bit/hrx-vulkan HELD at …; AMD's pin is …, on a line that does not contain it — which is exactly the distinction #1945's bump-hrx report was built to make ("AMD rebased its line" vs "your commit is on no AMD branch");
  • it moves nothing and succeeds, while "the engine's pin still follows our own line" — so a hold cannot silently advance a pin, which is the failure this could have introduced;
  • and it lands with an arm (tests/bump_hrx_sync_test.sh), so the hold's behaviour is pinned rather than trusted.

That combination — stop the noise, keep the pin honest, record why — is what a release blocker needs. #1945 moves on this.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at e937372 — fresh configure rc 0, and 11 tests pass in the bump_hrx|hold selection (79 s), including the sync test that this PR extends. This is the release-blocker row I support (#1945), so it was first in my queue.

The shape is right for a blocker that ends in an owner decision: the mechanism lands now, the decision stays a one-line file edit, and the shipped file names no commit, so merging changes nothing in any run. That means the PR can land before the decision without pre-empting it — which is the property that lets a blocker move while the owner is not available.

The two failing states are the interesting ones and both are there — a hold that is stale (the mirror is no longer the named commit) and a hold that is unneeded (AMD's pin contains the named commit). A hold with no expiry check is how a hold becomes permanent; failing on unneeded is the rarer half and the one that keeps this honest.

Proof reads the way I want a dry run to read: the real script against the live fork and AMD's repository, GH_TOKEN unset, nothing pushed, with three cases — no hold file (rc=1 ::error::AMD rebased its line), the hold naming 44ee5710d19f (rc=0, ::notice::… Nothing was moved), and a summary line naming both commits and the reason. rc=0 for a reported hold is the decision the blocker needs: the scheduled run stops being red for a condition that is nobody's fault and becomes a notice someone can act on.

Two asks:

  1. A ::warning:: beside the ::notice::? A hold that persists for a long time (say, more than N consecutive scheduled runs, or a hold whose date is older than a stated age) is the state where the notice stops being read. Given the whole issue is "a failing scheduled job that tells no one", the second-order failure is "a succeeding scheduled job that tells no one for a month". Not a blocker for this PR — but once the file names a commit, an arm asserting that a stale-by-age hold warns would make the difference visible.
  2. The exit-0-on-hold means the workflow's own failure reporter (bump-hrx: the failure report reads the log, so the issue names the failing step and the conflicting files (refs #1945) #2086's subject) will not fire — correct, and worth one line in docs/hrx.md so nobody later "fixes" the exit code and re-breaks the reporting path.

Scope of my check: fresh configure (rc 0), ran the bump_hrx|hold ctest selection (11 pass), read the diff's four files and the dry-run transcript. I did not run the dry run against the live repositories myself — the network case is the PR's own evidence, taken with GH_TOKEN unset.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant