Skip to content

Document batching, PFD, and parallel-queue interactions#172

Draft
samgutentag wants to merge 1 commit into
mainfrom
sam-gutentag/batching-pfd
Draft

Document batching, PFD, and parallel-queue interactions#172
samgutentag wants to merge 1 commit into
mainfrom
sam-gutentag/batching-pfd

Conversation

@samgutentag

Copy link
Copy Markdown
Member

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.

@samgutentag

Copy link
Copy Markdown
Member Author

Code verification (2026-06-01): 5 confirmed / 1 contradicted / 2 ambiguous / 1 unverifiable

Claim Verdict Source
--no-batch flag / noBatch: true via API isolates a PR from batching confirmed service.ts, schema.ts
A batch forms when target batch size fills OR max wait time elapses confirmed in_memory_merge_graph.ts:1066-1071
Bisection splits the batch in half (binary) and recurses, not one-by-one confirmed in_memory_merge_graph.ts:1216-1221
"Bisection Testing Concurrency" is the concurrency limit for sub-batch test runs ambiguous batching-bisection-concurrency.tsx:124-125
PFD waits for predecessor groups (always) and successor groups (up to depth) confirmed in_memory_merge_graph.ts:1761-1796
PFD example: a newly-arrived overlapping successor PR is awaited before failure transition confirmed in_memory_merge_graph.ts:1775-1796
The ALL keyword does not serialize downstream PRs; disjoint downstream PRs test in parallel behind it contradicted parallel_mode_impact_graph.ts:112-125
The pending_failure webhook event fires when a batch enters the hold state confirmed webhook.ts:30
Disabling optimistic merge causes already-passing downstream batches to re-test on upstream removal ambiguous merge_graph.ts:2347-2350
Bazel/Nx and similar impact tools include transitive dependents in impacted targets unverifiable (third-party tool behavior, not Trunk source)

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 IMPACTS_ALL is injected into the impactedBy set of every other queued PR, including PRs with disjoint targets. So there is no such thing as a downstream PR that "doesn't share a target" with an ALL-impacting PR: the ALL PR becomes a prerequisite of all of them. The doc's concrete example ("PR-A impacts ALL, PR-B impacts only docs, PR-B can still test in parallel behind PR-A... if PR-A fails, PR-B's test result is unaffected, they were never sharing a lane") is the opposite of what the impact graph builds. PR-B IS in PR-A's lane and PR-A's failure cascades to PR-B. This needs an eng review before publishing.

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: trunk-io/trunk/trunk/services/public-api/src/schema.ts and trunk-io/trunk/trunk/services/merge/src/service.ts

// 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 (--no-batch, parsed in parse_command) and the API field (noBatch: boolean, carried through service.ts submit and the public-api types) exist and feed the same isolate-from-batching behavior. The frontend merge-queue/CLAUDE.md independently lists noBatch=true among the things that prevent co-batching. Both the casing (noBatch) and the flag spelling (--no-batch) match the doc.

Source #2 — batch admission triggers (confirmed)

File: trunk-io/trunk/trunk/services/merge/src/controller/in_memory_merge_graph.ts#L1066-L1071

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 batch.length >= minimumSize (the "target batch size fills") OR the oldest member's age exceeds maximumWaitTimeMinutes (the "maximum wait time elapses"). The doc's "with at least one PR present" is consistent: the wait-time branch only fires for a non-empty in-progress batch. Names map cleanly: target batch size = minimumSize, maximum wait time = maximumWaitTimeMinutes.

Source #3 — bisection splits in half (confirmed)

File: trunk-io/trunk/trunk/services/merge/src/controller/in_memory_merge_graph.ts#L1216-L1221

// 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 Math.floor(count / 2) PRs (the first half), then the remainder (info.count - info.pastPendingCount) as the second half, and recurses. This is exactly the doc's "splits the batch in half... bisects into 2 sub-batches, retests, and recurses" and "logarithmic in batch size", not a one-PR-at-a-time peel. The frontend merge-queue/CLAUDE.md ("When a batch fails, it splits in half and each half is re-tested. This repeats recursively") corroborates.

Source #4 — "Bisection Testing Concurrency" label (ambiguous)

File: trunk-io/trunk2/ts/apps/frontend/src/components/settings/merge-queue/batching-bisection-concurrency.tsx#L124-L125

<h3 className="text-base font-semibold text-gray-900 dark:text-gray-100">
  Bisection Concurrency

Reasoning: The settings UI heading is "Bisection Concurrency"; the backing field is bisectionDedicatedConcurrency. The doc capitalizes it as "Bisection Testing Concurrency" — the extra word "Testing" is not in the product label. The doc's meaning (a concurrency cap on bisection sub-batch test runs, not a count of individual PRs retested at once) is correct given the binary-split logic in Source #3. Fix: drop "Testing" so the label matches the UI. Marked ambiguous because the semantics are right but the proper-noun label is off.

Source #5 — PFD waits on predecessors (always) and successors (up to depth) (confirmed)

File: trunk-io/trunk/trunk/services/merge/src/controller/in_memory_merge_graph.ts#L1761-L1796

// 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 pendingFailureDepth levels; if any successor within that depth is still testing, the transition is held (shouldAdd = false). That is exactly "waits for predecessor groups (always) and successor groups (up to the configured depth)".

Source #6 — newly-arrived overlapping successor is awaited (confirmed)

File: trunk-io/trunk/trunk/services/merge/src/controller/in_memory_merge_graph.ts#L1775-L1796

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 outgoers.some(isTesting) is true and PR-A is held in PENDING_FAILURE until PR-C concludes. This matches the doc's example exactly, including the "even though PR-C wasn't in the queue when PR-A failed" nuance.

Source #7 — the ALL keyword and downstream serialization (contradicted)

File: trunk-io/trunk/trunk/services/merge/src/controller/parallel_mode_impact_graph.ts#L112-L125 and #L140-L177

// 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 IMPACTS_ALL PR is added to the impactedBy set of every other queued PR, including PRs whose own targets are disjoint (the class comment at L18-19 says so: "Nodes that have impacted targets of IMPACTS_ALL impact all nodes... They are also included in the impacted set of all other nodes"). Since queryPrerequisites === impactedBy, the ALL PR becomes a prerequisite of every downstream PR. So the doc's example is inverted: PR-B (impacts only docs) does NOT have an empty shared-lane relationship with PR-A (impacts ALL) — PR-A is a prerequisite of PR-B, they share a lane, and PR-A failing cascades to PR-B. The corrected statement: an ALL-impacting PR shares a lane with every downstream PR and effectively serializes the queue behind it; it does not let disjoint downstream PRs continue in parallel. IMPACTS_ALL is the internal value for the public-API ALL keyword (schema.ts: IMPACTED_TARGETS_IMPACTS_ALL = "ALL").

Source #8 — `pending_failure` webhook event (confirmed)

File: trunk-io/trunk/trunk/services/merge/src/controller/reporter/webhook.ts#L30

[prisma.MergeItemState.PENDING_FAILURE]: "pending_failure",
...
case "pending_failure":
  ... failure_reason: pendingFailureReason ...

Reasoning: The webhook reporter maps the PENDING_FAILURE merge-item state to the wire event string "pending_failure", and there are explicit case "pending_failure" branches that emit the payload (including failure_reason). This confirms the doc's "the pending_failure event that fires when a batch enters the hold state." Casing and spelling match.

Source #9 — disabling optimistic merge causes re-tests (ambiguous)

File: trunk-io/trunk/trunk/services/merge/src/controller/merge_graph.ts#L2347-L2350

// 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 (canOptimisticallyMerge) is what lets downstream test runs treat upstream batches as passed, so their results can be reused against a projected main. Disabling it removes that reuse, which makes the doc's "downstream batches can't reuse results when an upstream batch changes shape, so they re-test" directionally correct. But there is no single source line stating the exact narrative "an already-passing batch goes back to testing when an upstream batch is removed." Marked ambiguous: accurate as a description of the consequence, not pinned to one code-level guarantee. Not a blocker.

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 samgutentag added the needs eng review verify-docs-against-code: at least one claim contradicts source. label Jun 1, 2026

Copy link
Copy Markdown
Member Author

Verification status: unknown - June 2, 2026

Could not determine rollout state from available signals.

  • Flag state: LaunchDarkly not consulted; no feature flag identified
  • Eng PR links: None. PR body notes this reopens work from Document batching, PFD, and parallel-queue interactions #56 (merged then reverted 2026-06-01); no eng PR reference in current body.
  • Flag: none
  • Signals checked: PR body; no eng PR refs to verify against
  • Suggested next action: Add eng PR references confirming batching, PFD, and parallel-queue interaction behavior is current in production, then re-run sweep.

Generated by Claude Code

Copy link
Copy Markdown
Member Author

Verification status (June 3, 2026): unknown

No engineering PR reference or Linear ticket found to verify feature readiness. Keeping as draft.

  • Flag state: N/A — no LaunchDarkly flag identified
  • Eng PR links: none found
  • Flag: none identified
  • Signals checked: PR body, prior sweep chain, draft status
  • Suggested next action: add eng PR link or Linear ticket to enable automated verification

Generated by Claude Code

Copy link
Copy Markdown
Member Author

Verification status (June 4, 2026): unknown

No engineering PR reference or Linear ticket found to verify feature readiness. Keeping as draft.

  • Flag state: N/A — no LaunchDarkly flag identified
  • Eng PR links: none found
  • Flag: none identified
  • Signals checked: PR body, prior sweep chain, draft status
  • Suggested next action: add eng PR link or Linear ticket to enable automated verification

Generated by Claude Code

Copy link
Copy Markdown
Member Author

Docs PR Verify — 2026-06-05 | Verdict: unknown

No eng PR, feature flag, or Linear ticket found. Unable to determine merge readiness automatically. Manual review required.

Auto-generated by Daily Docs PR Verify Sweep · 2026-06-05


Generated by Claude Code

Copy link
Copy Markdown
Member Author

Docs PR Verdict — 2026-06-08

Verdict unknown
Eng refs
LD flag
Notes Reopened from reverted PR #56 (batching, PFD, parallel-queue interactions). Awaiting eng review before re-merging.

Next sweep: 2026-06-09


Generated by Claude Code

Copy link
Copy Markdown
Member Author

⚪ Verdict: unknown — June 9, 2026

No feature flag reference or engineering PR link was found in this PR's description. Unable to verify whether the documented feature is live in production.

Action needed: Please add a LaunchDarkly flag name or link to the engineering PR in the PR description so this sweep can auto-verify next run.

Verified by Daily Docs Sweep · June 9, 2026


Generated by Claude Code

@samgutentag samgutentag added the unknown label Jun 9, 2026 — with Claude

Copy link
Copy Markdown
Member Author

❓ Verdict: unknown — June 10, 2026

Signal Detail
Eng PR none referenced
LD flag none
Linear none

Could not determine rollout state from available signals. No eng PR, no LD flag, no Linear ticket to verify against.

Verified by Daily Docs Sweep · June 10, 2026


Generated by Claude Code

@samgutentag samgutentag removed the unknown label Jun 11, 2026 — with Claude

Copy link
Copy Markdown
Member Author

❓ Verdict: unknown — June 11, 2026

No LaunchDarkly flag identified. Eng PR dependency on trunk2 not verifiable (private repo). Manual review needed to confirm deployment state.

Verified by Daily Docs Sweep · June 11, 2026


Generated by Claude Code

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

Labels

needs eng review verify-docs-against-code: at least one claim contradicts source.

Development

Successfully merging this pull request may close these issues.

1 participant