Document batching, PFD, and parallel-queue interactions#172
Conversation
samgutentag
force-pushed
the
sam-gutentag/batching-pfd
branch
from
d2ff6cb to
1a974a9
Compare
|
Code verification (2026-06-01): 5 confirmed / 1 contradicted / 2 ambiguous / 1 unverifiable
One contradiction blocks publishing. The section "The ALL keyword does not serialize downstream PRs" is wrong as written. In parallel mode, a PR whose impacted targets are Two ambiguous items to resolve (not blockers): (1) the setting is labeled "Bisection Concurrency" in the UI, not "Bisection Testing Concurrency" (the word "Testing" should be dropped to match the product); the semantics the doc describes are correct. (2) The optimistic-merge restart narrative is directionally consistent with source (optimistic merging transitions predecessors to TESTS_PASSED so downstream results can be reused; disabling it removes that reuse), but I found no single source line that states the specific "already-passing batch goes back to testing when an upstream batch is removed" sequence. It reads as an accurate description of a customer report rather than a code-level guarantee. Source #1 — `--no-batch` / `noBatch: true` isolates a PR (confirmed)File: // service.ts — submit path destructures the field
const { key, prNumber, creatorTrunkId, priorityValue, priorityName, noBatch, forceEnqueue } = ...
// reporter/types/index.ts
noBatch?: boolean;
// parse_command.test.ts confirms the CLI surface
it("parses --no-batch flag", () => { ... });Reasoning: Both the CLI flag ( Source #2 — batch admission triggers (confirmed)File: if (!shouldAdd && batch.length >= minimumSize) {
shouldAdd = true;
}
if (!shouldAdd && shouldCheckWaitTimeout && maximumWaitTimeMinutes) {
const age = ...;
if (age >= maximumWaitTimeMinutes * 60 * 1000) {
shouldAdd = true;
}
}Reasoning: A batch is admitted when Source #3 — bisection splits in half (confirmed)File: // Cut bisection in half if nothing is in testing. Otherwise, start everything else in the batch.
if (!info.pastPendingCount) {
// Use floor to decrease the chance of a logical merge conflict. PFD will help with flakiness.
const halfSize = Math.max(Math.floor(info.count / 2), 1);
batchingParameters = { ...batchingParameters, minimumSize: halfSize };
info.pastPendingCount = halfSize;
}Reasoning: On a failed batch the bisection re-batches Source #4 — "Bisection Testing Concurrency" label (ambiguous)<h3 className="text-base font-semibold text-gray-900 dark:text-gray-100">
Bisection ConcurrencyReasoning: The settings UI heading is "Bisection Concurrency"; the backing field is Source #5 — PFD waits on predecessors (always) and successors (up to depth) (confirmed)File: // Include PENDING_FAILURE items for which all predecessors have item state TESTS_PASSED.
return predecessors.every(
(predecessor) => predecessor.itemState === prisma.MergeItemState.TESTS_PASSED,
);
...
if (pendingFailureDepth) {
let outgoers = testRunGraph.outgoers(candidateTestRunId);
for (let i = 0; i < pendingFailureDepth; i += 1) {
if (outgoers.some((testRun) => testRun.isTesting)) {
shouldAdd = false; // do not transition out of PENDING_FAILURE yet
break;
}
outgoers = [ ...successors of successors... ];
}
}Reasoning: A PENDING_FAILURE batch only becomes eligible to transition once every predecessor is TESTS_PASSED (the "always wait for predecessors" half). Then the depth loop walks successor test runs up to Source #6 — newly-arrived overlapping successor is awaited (confirmed)File: let outgoers = testRunGraph.outgoers(candidateTestRunId);
for (let i = 0; i < pendingFailureDepth; i += 1) {
if (outgoers.some((testRun) => testRun.isTesting)) {
shouldAdd = false;
break;
}
...
}Reasoning: The successor check is evaluated against the live test-run graph's current outgoers, with no filter on when a successor entered the queue. With PFD=1, the loop inspects immediate successors once; if PR-C arrived after PR-A failed, shares targets with PR-A (making it a successor in the impact graph), and is testing, then Source #7 — the ALL keyword and downstream serialization (contradicted)File: // if there are any IMPACTS_ALL items then they are added to every single impactedBy set
if (impactsAllMergeItemIds.length > 0) {
this.mergeItemIdToImpactedByMergeItemIdsMap.forEach((impactedByMergeItemIds) => {
impactsAllMergeItemIds.forEach((impactsAllId) => {
impactedByMergeItemIds.add(impactsAllId);
});
});
}
...
public queryPrerequisites(itemId: string): Array<ImpactRecord> {
return this.impactedBy(itemId);
}Reasoning: In parallel mode an Source #8 — `pending_failure` webhook event (confirmed)File: [prisma.MergeItemState.PENDING_FAILURE]: "pending_failure",
...
case "pending_failure":
... failure_reason: pendingFailureReason ...Reasoning: The webhook reporter maps the Source #9 — disabling optimistic merge causes re-tests (ambiguous)File: // If optimistic merging is enabled, we need to transition all predecessors of the test run's items
// ... to TESTS_PASSED
if (this.mergeInstance.canOptimisticallyMerge) {
...
}Reasoning: Source confirms that optimistic merging ( Source #10 — transitive dependents in impacted targets (unverifiable)File: n/a (third-party tool behavior) Reasoning: The claim "most impact-detection tools (Bazel, Nx, and similar) include transitive dependents when computing impacted targets" is about the behavior of external build tools, not Trunk source. Trunk consumes whatever impacted-target set the customer uploads; the parallel-mode lane logic (Source #7) operates on that set but does not itself compute transitive dependents. The doc's framing (false-parallel merges are a signal to widen the impact graph, not to disable parallel mode) is sound advice, but the underlying tool-behavior claim cannot be verified against the Trunk codebase. |
samgutentag
added
the
needs eng review
|
Verification status: unknown - June 2, 2026 Could not determine rollout state from available signals.
Generated by Claude Code |
1 participant
Reopens the work from #56, which was merged then reverted on 2026-06-01 (it was not ready to merge). Content is unchanged from the original branch; needs content verification against source before re-merging. Reverted merge commit removed from main.