fix(installer): name the exit code when the CLI install fails, and guard the class (backend#2906) - #938
fix(installer): name the exit code when the CLI install fails, and guard the class (backend#2906)#938LukasWodka wants to merge 7 commits into
Conversation
…d#2906) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…the k3d path (backend#2906)
Bugbot, Medium: the AST sweep matched only the dotted `.ExitCode -eq/-ne 0`
spelling, so the house idiom after a wait -- copy the code into a local, branch
on the local -- was invisible to it. `$k3dExitCode = $k3dProc.ExitCode` …
`if ($k3dExitCode -ne 0) { Err … }` is exactly that, and it reported a k3d
failure with no code while this guard called the class closed. Restating one
spelling is what went wrong in client#913; restating two is the same mistake
with a longer list. The variable names are now DERIVED from assignments whose
right side reads `.ExitCode`.
Compliance follows ONE HOP through the call arguments, which is what separates
the two sites this newly sees. The GPU branch names no code in its Warn text,
but $GPU_SKIP_REASON is assigned in that branch from Get-GpuBuildFailureReason
-ExitCode $buildExit, whose fallback returns "docker build exit $ExitCode" --
the classifier is deliberately preferred over a bare number and the comment
there says so. The k3d branch has no such hop: the code is read, tested and
dropped. Fixed with the house Format-ExitCode helper, matching the sibling site
at :4086.
Mutation-proved, anchors asserted both times:
revert the k3d fix -> 1 of 2 reddens
-ExitCode $buildExit -> -ExitCode 0 -> 1 of 2 reddens
The second mutation initially did NOT redden: the token regex matched the bare
word ExitCode, which the PARAMETER NAME satisfies with a constant value, so the
one-hop was a blanket pass. Tightened to the property read and the gate variable.
Not widened to $LASTEXITCODE here: 28 uses, and the widening surfaces 7 further
offenders. That is a red gate on arrival, so it is filed separately rather than
landed (CLAUDE.md rule 4 -- arm while green).
Pester: 868 total, 853 passed, 0 failed. make drift: all 38 guards green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Right, and right about the mechanism rather than just the symptom. Fixed in The guard matched one spelling of the gate, so the house idiom after a wait — copy the code into a local, then branch on the local — was structurally invisible to it. That's the same failure as client#913's Compliance follows one hop, and that hop is what separates the two sites the widening newly sees:
Mutation-proved, anchor asserted both times: Worth flagging that the second one initially did not redden. My token regex matched the bare word One thing deliberately not done here. Pester: 868 total, 853 passed, 0 failed. — drafted with Claude Code |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 2779bdc. Configure here.
…ckend#2906) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
…e (backend#2906) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 14f4d70. Configure here.
saadqbal
left a comment
There was a problem hiding this comment.
The guard is one spelling short of the class it claims to close, and there's a live site
behind the gap.
The derivation only follows .ExitCode — install-k8s.Tests.ps1 matches
$a.Right.Extent.Text -match '\.ExitCode' — so $createRc = $LASTEXITCODE at :5785 never
becomes a gate token, and if ($createRc -ne 0) at :5787 is invisible. You added
$LASTEXITCODE as a direct token, which catches if ($LASTEXITCODE -ne 0), but not the
copy-into-a-local idiom — i.e. exactly what Bugbot flagged in round one for .ExitCode,
fixed for one spelling only. Widen it to '\.ExitCode|\$LASTEXITCODE' and your own guard
names the site.
Not a false positive: & tracebloc @createArgs is native, the capture at :5785 is correct
and non-stale, and nothing carries the code out — Print-CreateFailure gets -OutFile and
-Location, never the code, and its line filter can match nothing. So the operator can get
"The client couldn't be provisioned." followed by "Couldn't provision the client. Re-run to
retry." with no code and no output. That is the #2906 shape precisely.
Two lines to fix, and the floor goes 8 -> 9. Worth doing here rather than as a follow-up,
because the floor test passing at 8 is what makes the hole self-concealing.
Everything else holds up, and the harder half is right. Reverting the two GPU edits was the
correct call — under-reporting beats demanding a stale probe code — and the catch/nearest-if
stops are what make that safe. Format-ExitCode is genuinely fail-closed: it uses $null -eq $Code rather than the falsy if ($Code) trap, so a real exit 0 renders as "0"
instead of vanishing, and an absent code renders as "with no code reported" rather than
being dressed up as data.
Non-blocking, same class one layer out: :786 (-not $ok) and :4444 (-not $partialDeleted)
collapse the code to a bool before the branch, so the -(eq|ne) 0 gate can't see those
either.
One nit on the ticket's own goal: powershell.exe -Command "irm | iex" collapses most
terminating errors to exit 1, so the number discriminates network from permissions only as
far as the CLI installer's own exit-code discipline goes — which this PR doesn't control.
Still strictly better than nothing, since the code was the only surviving evidence. Contrast
:2690, where you prefer a classifier over a bare number; that's the better pattern and the
file is now inconsistent about which it uses.
…d (backend#2906) @saadqbal's change-request, and he was right that the floor passing at 8 is what made the hole self-concealing. THE DERIVATION FOLLOWED ONE SPELLING. `$a.Right.Extent.Text -match '\.ExitCode'` never matched `$createRc = $LASTEXITCODE`, so `$createRc` never became a gate token and `if ($createRc -ne 0)` at install-k8s.ps1:5787 was invisible to the walk. `$LASTEXITCODE` had been added as a DIRECT token, which catches `if ($LASTEXITCODE -ne 0)` but not the copy-into-a-local idiom -- the same shape round one flagged for `.ExitCode`, fixed for one spelling. Widened to `'\.ExitCode|\$LASTEXITCODE'`, floor 8 -> 9. AND WIDENING IT IMMEDIATELY NAMED A LIVE SITE, which is the point: line 5789 gated on [$createRc -ne 0]: Err "Couldn't provision the client. Re-run to retry.", but got 1. Exactly the case he described -- `Print-CreateFailure` receives `-OutFile` and `-Location`, never the code, so an operator got "The client couldn't be provisioned." followed by "Couldn't provision the client. Re-run to retry." with no code at all. Fixed with the house pattern: Err "Couldn't provision the client (tracebloc exited $(Format-ExitCode $createRc)). Re-run to retry." So the two-line change is a three-line change: the guard widening is what makes the product fix findable, and leaving the floor at 8 would have let both sit. Pester: 853 passed, 0 failed, 15 skipped. drift 38/38. Manifest regenerated (install-k8s.ps1 moved). A note on my own process, since it bit me twice here: two earlier edits in this pass were silently lost because a later anchor miss in the same script aborted before `write_text`. Both edits are now applied one at a time and each re-read from disk to confirm it landed -- the widening in particular reported "ok" once without ever being written, and the Pester run that followed was measuring the unmodified file. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Fixed on Widened to Exactly your reading: Err "Couldn't provision the client (tracebloc exited $(Format-ExitCode $createRc)). Re-run to retry."So the two-line change is a three-line change — the guard widening is what makes the product fix findable, and a floor left at 8 would have let both sit indefinitely. Pester 853 passed / 0 failed / 15 skipped, drift 38/38, manifest regenerated. One process note, because it nearly cost you a false report: two of my edits in this pass were silently lost when a later anchor miss in the same script aborted before the write — including this widening, which printed "ok" once without being written, after which the Pester run I was reading was measuring the unmodified file and reporting 12/12. Each edit is now applied on its own and re-read from disk to confirm it landed. The numbers above are from a file I verified byte-for-byte first. |
|
bugbot run |
saadqbal
left a comment
There was a problem hiding this comment.
Nothing's moved since my last pass — head is still 14f4d70, which predates that review — so
the blocker stands as written: the derivation is still -match '\.ExitCode', and the
$createRc site is still outside the guard's denominator.
I measured it this time rather than reading it. The guard gates 8 sites and passes clean at 8.
My own sweep of the file finds 9 sites of the class, the ninth being Err "Couldn't provision the client. Re-run to retry." gated on $createRc -ne 0. Widen the derivation to
'\.ExitCode|\$LASTEXITCODE' and the count goes 8 -> 9 and your guard names that line by
itself; with the message fixed it goes back to green. No false positives from the widening —
the GPU probe else and the catch stay excluded, which is the part you got right and I
don't want disturbed.
Base has gone stale too: #937 landed and this now conflicts, on scripts/manifest.sha256
alone — the script and the test file merge clean. Re-run gen-manifest.sh after the merge
rather than picking a side, or Source-of-truth drift will catch it.
One correction to my own non-blocking list: there are three of those bool-collapsed sites, not
two — :786, :4444 and :4729 — and :4729 must stay silent. Its code would be the stale
apply/rollout probe and its catch twin has none at all, which is exactly what you reverted for
the right reason. So if you do extend the gate to bool shapes later, that exclusion has to
come with it, or the guard ends up demanding a false statement — the one outcome its own
comments call worse than not checking.
# Conflicts: # scripts/manifest.sha256
…ed (backend#2906)
Bugbot Medium: the gate derived from `.ExitCode` and `$LASTEXITCODE` only, so
branches on `.Code` were invisible. `.Code` is the HOUSE result shape --
Invoke-BoundedProcess and Invoke-DockerCli return @{ Code; Output }, documented at
install-k8s.ps1:2307-2308 -- so this was the commonest wrapper spelling, not an
edge case.
DERIVED, NOT COUNTED BY HAND. Bugbot named 2 sites. Widening the gate and letting
the walk report found 31 `.Code` gates in the file, 7 of them carrying a
user-facing Warn/Err. The finding was 2 of 7.
TWO OF THE 7 ARE DISJUNCTIONS AND ARE NOW EXCLUDED, for the same reason the ELSE
and the CATCH already are: the guard cannot know WHICH disjunct fired, so
demanding the code demands a possibly-false cause. `if ($res.Code -ne 0 -or $out
-match "FAIL " -or $unconfirmed.Count -gt 0)` fails on a non-code disjunct with a
code of 0, and "exit 0" beside a failure is the wrong-cause-reads-as-information
outcome this Describe exists to refuse. @saadqbal flagged this hazard
pre-emptively for the bool-collapsed sites; it arrived here first.
THE FIVE REAL ONES NOW NAME THE CODE. All five are wrapper calls where 124 IS the
timeout, so the code is precisely what separates the two causes the old text made
the operator guess between -- ":1534 nvidia-smi failed or timed out" being the
clearest case.
AND THE GUARD WAS UNSATISFIABLE FOR ONE ITERATION, which is the finding worth
reading. Widening the GATE alone left compliance matching `.ExitCode` only: all
five sites kept failing with the code sitting in the message text. A guard that
cannot be satisfied is worse than one that does not check -- it trains the reader
to edit the guard instead of the code. The two sides now widen in one place, named
as such.
Floor 9 -> 14, MEASURED by raising it until it failed and reading the number back,
not by counting additions by hand -- which is how a floor and a walk start
disagreeing.
Base merged (#937 had landed; conflict was scripts/manifest.sha256 alone) and the
manifest REGENERATED rather than resolved by picking a side, per @saadqbal.
4 mutations, all reddening: drop the code from a fixed message; narrow the gate
back; narrow compliance back (the unsatisfiable state); drop the disjunction
exclusion. Full Pester suite: 1061 passed, 0 failed. gen-manifest --check clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
All three points closed — and two of them were already on the head when you reviewed, so the first thing worth saying is that you were reading Your blocker was already fixed before your review landed
And your finding generalised one spelling furtherBugbot then raised the third: Derived rather than counted: 31 Your
|
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 75f8a96. Configure here.
Closes tracebloc/backend#2906. Fifth site of the class client#913 fixed for backend#2849 — and the guard here found a sixth.
The defect
install-k8s.ps1, the CLI-install failure branch:The block above it does everything right — redirects both streams, caches
.HandlebeforeWaitForExit()so.ExitCodeis reliable, replays both files into the log. Then the failure branch discards the one number.Measured cost. E2E run 33395912890, Windows Server 2022. The complete record of the failure:
The redirected streams were captured and replayed and hold nothing beyond that banner, so the exit code was the only surviving evidence. And
Warntakes no$Detail, so unlike the k3d path there is no second channel to fall back on.Why #913 missed it
#913 fixed the sites that rendered a code as blank —
wsl exited,installer exited. This one never rendered one at all. Identical information loss, different spelling, so the search that found the first shape (exited $() was structurally blind to the second.That is why this PR does not just fix the line.
The guard, and the sixth site it found
The invariant is checkable directly rather than site-by-site: if you tell the user something failed, from inside a branch a process exit code decided, name the code.
Implemented over the AST, not a regex — the question is "which
Warn/Errcalls are lexically inside a branch gated on.ExitCode", which is a shape in the tree that a text match cannot see.It found two sites, one of which nobody had reported:
75804086Err "Couldn't start the existing '$CLUSTER_NAME' environment…"The second is the lesser instance —
Errdoes receive$startLog, so k3d's own words reach the user — but the code is the cheapest discriminator and it is the same class. Both now name it.A marker per known site could not have done this. It can only be as complete as the enumeration behind it, and the enumeration is what was wrong. Raised on client#931 as well, where the registry has "the exit-code slot can render empty again" (the blank shape) and "the CLI installer child is waited on with no deadline" (the deadline) but nothing for this.
Mutation-proved, each independently
The third is the fail-closed check: a sweep that stops finding sites passes vacuously and reports clean forever, so it asserts at least two are found. A third site appearing is fine; a drop to zero is the finding.
Also in here: the ~350s figure from #934
#934shippedWORST CASE ~350s. Derived fromInvoke-DiagnoseBundleinstead:13 reads x 20s + 3 logs x 30scounts the three log fetches twice — 13 is the number of captures (10 sites + 3 loop iterations), not the number of 20s reads — and omits the twoInvoke-DockerCliwaits, which are bounded on the same sequential path but do not go throughInvoke-DiagnoseCapture. The two errors partly cancel, which is how a wrong number survived a PR written to correct a wrong number.Conclusion unchanged — 330s is still under six minutes. I raised this on #934 as
CHANGES_REQUESTED; it was approved and merged three minutes later, so the correction lands here instead. The comment now shows the sum term by term.Test plan
scripts/testsrunscripts/check-style.shcleanscripts/gen-manifest.shre-run;manifest.sha256updatedinstall-k8s.ps1and the test file both parse clean via[Parser]::ParseFileNote
Low Risk
Changes are limited to installer error messaging, comments, tests, and manifest checksums; no runtime cluster or auth logic changes.
Overview
Improves Windows installer diagnostics by naming process exit codes in
Warn/Errmessages whenever a branch fails on$LASTEXITCODE,.ExitCode, or bounded-result.Code— including the previously silent tracebloc CLI install failure, k3d cluster start/create, kubeconfig merge, GPU CDI probes, sign-in, client create, and Helm steps. nvidia-smi failures now call out that exit 124 means timeout.Adds a Pester AST guard so future
Warn/Errinside exit-code-gated branches cannot drop the code again (with exclusions for catch blocks, OR conditions, and one-hop indirection via branch assignments).Corrects the diagnose-bundle worst-case comment from ~350s to ~330s with an itemized breakdown. Updates
scripts/manifest.sha256forinstall-k8s.ps1.Reviewed by Cursor Bugbot for commit 75f8a96. Bugbot is set up for automated code reviews on this repo. Configure here.