chadtmiller.com/knotless

Switch repository

Loading…

What changes

Queueing again at an ejected head says so. When a pull goes back into the queue at the very head the queue last ejected, knotless merge --queue adds a line after its place. It says this is the head the queue last ejected, and why, and that unless the failure was not the pull's doing it will be ejected again, and how to take it out. The MCP merge tool answers the same as queued.ejectedBefore, and dev.knotless.queuePull as ejectedBefore, an optional new field. Queueing still goes ahead: the failure may have been a flake, or main may have fixed it since.

Halving ejects at the head that failed. A pull that halving finds broke its batch is now ejected with its head, commit on the record, as every other ejection is. It had none, so the warning above could not see it.

Why

In the queue's history, 23 of 125 repeat ejections of one pull were at the same head as the ejection before: queued again with nothing changed. Most of those were ejected again. Queueing could not tell, as it never compared the head with the ejection's.

How it was checked

The merge queue's run test queues the pull halving ejected again at the same head, checks the line and the command to take it out, and takes it out. Left to CI.

Shortcomings

The pages do not show ejectedBefore yet: they queue from the merge box, which would need its own design.

This branch is built on The merge queue ejects failed pre-flights sooner and holds conflicts and Queueing says to read an ejection's logs before queueing again, whose lines it changes, so it carries their commits until they land.

This pull does two things. When a pull is queued again at the head the queue last ejected, the CLI, the MCP merge tool and queuePull now say so and give the reason. A pull that halving finds broke its batch is now ejected with its head, so that comparison covers it too. I read only the top commit's own diff, since the rest is the two stacked pulls.

The logic looks right:

• ejected_at compares the current head with the ejection's commit and the state, and it only runs when the pull was not already queued.
• failed_heads is rebuilt each time the suspects are set, so a stale head can't carry over.
• The lexicon field is optional, as the style guide asks.
• The docs and the change file are updated.

Things to check once CI finishes. The check and test runs were still running or queued when I looked, so I haven't seen the 100% coverage gate or the new run-test assertion pass.

• The new test queues x again and then runs --leave. Make sure the later steps in a_merge_queue_lands_in_batches don't depend on x still being in the ejected state.
• Nothing in the diff exercises ejectedBefore through the MCP merge tool or the serve queuePull path. If coverage or the answer-vs-lexicon check flags either, add a case.
• A very small point: an ejection with no recorded reason would print "as ." in the CLI line. I don't think that can happen, because ejections always carry a reason.

Carried forward: the change this pull makes at 8bcef63a9d8d62a271875f040521d34fce0459ef is the one reviewed at 5d443dd96a22ff8565339a42d7c90cce604c998b, so that review stands. What is new comes from its base.

The only change since my last review is in cli.rs. The ejected_before message is now built with format! and then written with writeln!, so the ? sits on a short line for the coverage gate. The output is the same. My earlier review stands. The logic is sound and the field is optional in the lexicon. The docs and the change file are updated.

CI is still queued, so I haven't seen check or test pass. Two things to look at when they do:

• The new run-test step queues x again and then runs --leave. Later steps in a_merge_queue_lands_in_batches must not depend on x still being ejected.
• Nothing in the diff exercises ejectedBefore through the MCP merge tool or the serve queuePull path. If coverage or the lexicon check flags either, add a case.

The two stacked pulls have landed, and this round merges main into the branch. The pull's diff against main is now only its own change. It adds the ejectedBefore line when a pull is queued again at the head the queue last ejected. It also ejects a pull that halving finds broke its batch with its head recorded. The conflict resolutions in queue.rs, lib.rs and cli.md look clean. The lexicon field is optional, and the docs, MCP description and change file are updated.

Only docs has passed at this commit. check, test, e2e, web, shots and lexicons were still queued or running, so I haven't seen the coverage gate or the new run-test step pass. Two things to look at when they finish:

• The new run-test step queues x again and then runs --leave. Later steps in a_merge_queue_lands_in_batches must not depend on x still being ejected.
• The MCP merge tool and queuePull paths have no test of their own for ejectedBefore. If coverage or the lexicon check flags either, add a case.