[TE-6274] Fall bktec plan fallback parallelism back to BUILDKITE_PARALLEL_JOB_COUNT#577
Conversation
There was a problem hiding this comment.
I didn't find any code issues in this pass. This reads as an L1 bug fix with focused fallback-path coverage; I'm leaving a comment rather than an approval because the PR is still marked draft.
Want to dig deeper? The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 2452, then answer my questions about the findings.
…LLEL_JOB_COUNT When the Test Engine API is unreachable or returns an error plan, bktec plan substitutes a locally-computed fallback and exits 0. The fallback previously took cfg.MaxParallelism verbatim, so an unset --max-parallelism yielded parallelism 0 — --pipeline-upload uploaded nothing and --json emitted "0", silently skipping the suite (a green build that tested nothing). Resolve the fallback parallelism the way bktec run already does: MaxParallelism -> Parallelism -> 0, falling back to the static BUILDKITE_PARALLEL_JOB_COUNT when --max-parallelism is unset. On-agent this closes the "nothing runs" hole: the suite runs un-optimised instead of being skipped.
… rejects both-unset TE-6275 (merged) makes ValidateForPlan reject the both-unset config that produced this fallback, so the config can no longer reach Plan()'s parallelism-0 guard through validation. The guard remains as defence in depth; drive it directly and drop the now-failing ValidateForPlan call.
34ea2d1 to
b1a51b0
Compare
There was a problem hiding this comment.
Focused fallback-path bug fix: the local fallback now derives parallelism from --max-parallelism or BUILDKITE_PARALLEL_JOB_COUNT, with direct coverage for both --pipeline-upload and --plan-out. I read this as L1: no API contract or config surface change, and CI is green.
Want to dig deeper? The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 2468, then answer my questions about the findings.
There was a problem hiding this comment.
Bug fix with focused fallback-path coverage; no API surface or configuration contract change. I read this as L1 because it only changes the local fallback parallelism resolution from zero to the existing static job count when max-parallelism is unset.
Want to dig deeper? The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 2469, then answer my questions about the findings.
Description
Fall the
bktec planfallback parallelism back to the staticBUILDKITE_PARALLEL_JOB_COUNT(cfg.Parallelism) when--max-parallelism(
BUILDKITE_TEST_ENGINE_MAX_PARALLELISM) is unset, so an unavailable Test Enginedegrades to "runs the full suite un-optimised" instead of "silently runs nothing".
When the API is unreachable or returns an error plan,
bktec plansubstitutes alocal fallback and exits 0. The fallback took
cfg.MaxParallelismverbatim, so anunset
--max-parallelismgave parallelism 0:--pipeline-uploaduploaded nothing,--jsonemitted"0", and the suite was skipped. A green build that testednothing is the dangerous shape; "runs un-optimised" (non-zero parallelism) is the
acceptable fail-open degradation
bktec runalready makes.This resolves the fallback parallelism like
bktec rundoes —MaxParallelism → Parallelism → 0— aligning the two commands. It only addsruns where there were none, so plans with a non-zero parallelism are unchanged.
The
--max-parallelismhelp text already promises "when 0 ... the test planparallelism will be derived from
BUILDKITE_PARALLEL_JOB_COUNT"; this honours thatin the fallback path.
Residual both-unset gap: when both
--max-parallelismandBUILDKITE_PARALLEL_JOB_COUNTare 0 (e.g. off-agentplan --plan-identifier), thefallback would still be parallelism 0. #578 (TE-6275, merged) now catches this up
front with a client-side
parallelism > 0check that rejects the config before anynetwork call, so it no longer reaches this fallback.
Context
TE-6274. Surfaced during review of TE-6254 (
bktec plan --plan-out, merged);independent of it. Companion to #578 (TE-6275), the client-side
parallelism > 0check.
Changes
fallbackParallelism(cfg)and wire it intomakeFallbackPlanand the--plan-outemitLocalFallbackpath ininternal/command/plan.go.Testing
Backed by specs:
--pipeline-uploadand--plan-outcases assert the fallbackuses the static parallelism when
--max-parallelismis unset. Since #578 landed onmain, the parallelism-0 guard test drives
Plan()'s guard directly (ValidateForPlannow rejects the both-unset config before it reaches the guard; the guard stays as
defence in depth). AI assisted.