Skip to content

fix(installer): split existing-release from existing-datadir in the arch gate - #752

Merged
LukasWodka merged 4 commits into
developfrom
fix/2151-arch-gate-existing-release
Aug 19, 2026
Merged

fix(installer): split existing-release from existing-datadir in the arch gate#752
LukasWodka merged 4 commits into
developfrom
fix/2151-arch-gate-existing-release

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Resolves the three unresolved threads on the `client` develop→staging mirror (#748) — 1 Bugbot Medium + 2 from @saadqbal — on `scripts/lib/install-client-helm.sh`.

The bug

existing_id (a live Helm release, from detect_installed_clienthelm list -A) was folded into the existing-datadir reason. On arm64 the arch gate then:

  • claimed "This host holds existing MySQL 5.7 data" — which can be false: on TB_STORAGE_MODE=node-local both _mysql_dir_has_content probes are empty, so existing_id is the only trigger and there may be no host datadir at all; and the release could even be an 8.4 one whose sticky marker was lost;
  • offered --data-dir as a fresh start — but that cannot clear a release helm list reports, so the next run refuses identically.

The fix

Split the trigger into two reasons, same 5.7 engine choice, different remedy:

  • existing-release (from existing_id): 5.7 kept as a data-safety default, no host-data claim, remedy is emulation or uninstalling the release — not --data-dir.
  • existing-datadir (real host files): unchanged; --data-dir is a valid fresh start there.

Also corrected the header comment that implied assert_amd64_emulation fully "owns" macOS — it refuses whenever Rosetta amd64 is off without consulting _mysql_engine_decision, so a fresh arm64 Mac that 8.4 would serve natively is still turned away. Named as a separate backend#2047 follow-up rather than silently implied covered (@saadqbal’s second point).

Tests

  • existing-release distinguished in the reason-distinguishability and purity suites.
  • New arch-gate test pins the corrected remedy: no false host-data claim, no --data-dir.
  • Both halves mutation-proved; 157/157 bats green; shellcheck -x clean.

On develop so the staging hop re-prepare picks it up.


Note

Low Risk
Installer messaging and engine-reason ordering only; behavior still pins 5.7 for safety, with expanded tests and manifest hash update.

Overview
Fixes misleading arm64 Linux installer errors when MySQL stays on 5.7 only because a Helm release exists, not because host datadir files were found.

_mysql_engine_decision now returns 5.7 existing-release when existing_id is set and host mysql dirs are empty, and 5.7 existing-datadir only when _mysql_dir_has_content fires; datadir is checked first so release + files still report datadir.

_assert_engine_runs_on_this_arch adds an existing-release branch: it does not claim the host holds 5.7 data or suggest --data-dir; fresh-start guidance covers uninstall plus retained MySQL PVC (helm.sh/resource-policy: keep). Comments note macOS Rosetta gating is still a separate follow-up.

Bats cover the new reason, precedence, and arch-gate messaging; scripts/manifest.sha256 is updated for install-client-helm.sh.

Reviewed by Cursor Bugbot for commit d84f90f. Bugbot is set up for automated code reviews on this repo. Configure here.

…rch gate
A live Helm release (existing_id, from 'helm list -A') was folded into the
existing-datadir reason, so on arm64 the gate claimed "This host holds existing
MySQL 5.7 data" and offered --data-dir as a fresh start. That claim can be false
(on TB_STORAGE_MODE=node-local both datadir probes are empty, and the release's
data format is unknown from here), and --data-dir cannot clear a release that
'helm list' reports — the next run refuses identically.
Split the reason: existing_id -> 'existing-release' (5.7 as a data-safety default,
no host-data claim, remedy is emulation or uninstalling the release), real datadir
content -> 'existing-datadir' (unchanged, --data-dir is a valid fresh start there).
Also correct the header comment that claimed assert_amd64_emulation 'owns' macOS:
it refuses whenever Rosetta amd64 is off without consulting the engine rule, so a
fresh arm64 Mac that 8.4 would serve natively is still turned away — named as a
separate backend#2047 follow-up rather than implied covered (Asad, client#748).
Tests: existing-release distinguished in the reason and purity suites; a new arch-
gate test pins the corrected remedy (no false data claim, no --data-dir). Both
halves mutation-proved; 157/157 bats green, shellcheck clean.
Bugbot (Medium) + review (Asad), client#748 staging mirror.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodkaLukasWodka self-assigned this Aug 19, 2026
Comment threadscripts/lib/install-client-helm.sh
Comment threadscripts/lib/install-client-helm.sh
…manifest
Two Bugbot findings on the first push:
- HIGH: install-client-helm.sh is a bootstrap-fetched lib verified against
scripts/manifest.sha256; the edit was made without regenerating the digest, so
install.sh's signed fetch (R8) would fail closed. Regenerated (gen-manifest.sh
--check now clean).
- Medium: with existing_id-first, a host that has BOTH a live release AND 5.7
datadir files got reason existing-release, whose 'just uninstall' remedy leaves
the files to re-pin 5.7 next run. Reorder to datadir-first: files present ->
existing-datadir (true data claim, remedy now also says uninstall any release);
existing-release fires only with NO files, where 'uninstall the release' is a
complete fresh start. New test pins the both-exist -> existing-datadir ordering.
157/157 bats, shellcheck clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 6710d36. Configure here.

Comment threadscripts/lib/install-client-helm.sh

@saadqbalsaadqbal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The four-quadrant split itself reads correctly: datadir-first, then existing_id, keeps the engine at 5.7 in all three "something exists" quadrants exactly as the old OR did, so nothing new becomes reachable engine-wise — only the reason string and the remedy change. Both probes fail closed (INSTALLED_CLIENT_UNKNOWN hard-errors before _resolve_mysql_engine; unlistable datadir counts as content), and TB_NAMESPACE is switched to existing_ns before the decision runs, so the per-release probe looks at the right path. manifest.sha256 matches the new blob (d0a72ba7…). No values.yaml or template change, so no --reuse-values nil-guard exposure.

Holding this on Bugbot's still-open thread rather than restating it inline: the existing-release "just uninstall the release" remedy is wrong in the one case the reason exists for. node-local is exactly where both host probes are empty, and the mysql PVC carries "helm.sh/resource-policy": keep (client/templates/mysql-storage-pvc.yaml, outside the hostPath.enabled block), so helm uninstall keeps the 5.7 datadir. The next run then sees no release and empty host dirs, resolves 8.4 fresh, and the format guard in mysql-deployment.yaml refuses to start — after the operator has torn down a working install on the installer's own advice. What actually clears in-node data is k3d cluster delete or deleting the PVC. The comment at :903 asserting "no files remain to re-trigger the gate" needs the same correction.

Two smaller ones, both outside this diff so no inline: _pf_arch's existing-datadir hint in preflight.sh never got this PR's "uninstall any existing release" correction, and preflight hard-fails before the corrected copy here can print — so the datadir case still sees the old --data-dir-only advice. And _resolve_mysql_engine's case "$reason" still narrates only explicit/sticky/fresh, so existing-release (like existing-datadir) pins 5.7 with nothing in the log saying why — invisible on amd64, where the gate never fires.

Test coverage of the four quadrants is good, and adding existing-release to the mutation loop is the right instinct.

Comment threadscripts/lib/install-client-helm.sh Outdated
LukasWodkaand others added 2 commits August 19, 2026 09:34
… MySQL PVC
Bugbot (Medium) on the prior push: the existing-release fresh-start remedy said
'uninstall the release' and treated it as complete. In the node-local case that
is exactly wrong — MySQL lives on an in-cluster PVC annotated
helm.sh/resource-policy: keep, so 'helm uninstall' leaves the 5.7 data behind;
the next run sees no existing_id, resolves to native 8.4, and fails the format
guard against the leftover volume (or loops). Make the remedy complete: to start
fresh, remove BOTH the release AND the retained MySQL PVC (and any host data
dir). Test asserts the PVC/keep wording. Manifest regenerated.
157/157 bats, shellcheck clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…sad)
_mysql_dir_has_content tests only that the dir is non-empty, never the on-disk
format — so 'real 5.7-format datadir content' overstates exactly the way this PR
removes from the existing_id path. Revert the datadir reason wording to
format-neutral ('mysql datadir content, non-empty'); the engine still keeps 5.7
because the format is UNKNOWN and 8.4 cannot open a 5.7 dir if that is what it is.
Manifest regenerated; 157/157 bats, shellcheck clean.
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Fixed — you're right, _mysql_dir_has_content only tests non-emptiness, so "5.7-format datadir content" was the same unbacked claim this PR strips from the existing_id path. Reverted the datadir reason to format-neutral wording ("real mysql datadir content, non-empty"); the engine still keeps 5.7 precisely because the format is unknown and 8.4 cannot open a 5.7 dir if that is what it is. Also softened the one "that 5.7-format data" in the existing-release remedy to "may be". Pushed.

@saadqbalsaadqbal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified the manifest by hand rather than trusting the resolved thread: recomputed all 18 digests in scripts/manifest.sha256 against the d84f90f blobs — every one matches, and no fetched lib under scripts/lib is missing from the manifest. The signed fetch is intact at this ref.

The remedy now covers both storage modes. node-local is named explicitly (retained PVC under resource-policy: keep), and hostpath cannot reach existing-release at all — HOST_DATA_DIR is bind-mounted at /tracebloc, so the per-release probe sees those files and the reason is existing-datadir, whose remedy already says to uninstall the release and point at an empty data dir. The annotation is outside the hostPath.enabled conditional in mysql-storage-pvc.yaml, so the claim in the new hint holds in both modes.

The invariant from my last pass still holds: the decision block itself (:908-920) is untouched by these two commits, so the engine is 5.7 in all three "something exists" quadrants exactly as the old OR was, and 8.4 is still never selected where the old condition selected 5.7. _mysql_dir_has_content still fails closed on unlistable dirs, INSTALLED_CLIENT_UNKNOWN still hard-errors before _resolve_mysql_engine, and the PR still touches no values.yaml or template, so the --reuse-values nil-guard rule stays N/A.

Tests: 137 of 157 ran green (0 failures) before I capped the run locally — the whole engine and arch-gate block passes, including the updated existing-release assertions.

Two nits inline, neither blocking. The preflight.shexisting-datadir hint and the missing existing-release log line from my last pass are both still outstanding and still outside this diff — follow-up material, not this PR.

Comment threadscripts/lib/install-client-helm.sh
Comment threadscripts/lib/install-client-helm.sh
@LukasWodka
LukasWodka merged commit 910b39e into developAug 19, 2026
70 of 74 checks passed
@LukasWodka
LukasWodka deleted the fix/2151-arch-gate-existing-release branch August 19, 2026 10:23
LukasWodka added a commit that referenced this pull request Aug 19, 2026
…#755)
`Static analysis` (installer-tests.yaml's `static` job) is a required status
check on no branch — measured against develop, staging, main and both rulesets.
Four guards lived there: the R8 installer-manifest check, cross-OS facts
single-sourcing, style/terminology, and PSScriptAnalyzer. A PR could change an
installer sub-script, leave scripts/manifest.sha256 stale, and merge on its
required checks — shipping a manifest that rejects the scripts it covers.
They could also be silenced. On five runs of #752 the job's ShellCheck step ate
the whole `timeout-minutes: 10` on `apt-get install shellcheck`; all four guards
reported `skipped` while every required check went green. The apt-get was never
needed — shellcheck is on the ubuntu-latest image.
No branch-protection change and no new required context: `Source-of-truth drift`
is already required on develop and main, and its header already declares itself
the home for guards that must gate. The Makefile's `drift` target already ran
R8 + check-facts + check-style while that job ran check-drift + env-vocabulary,
so each side gated what the other did not. One list now, in the Makefile.
- drift-checks.yaml runs `make drift` and holds no guard list of its own
- make drift gains check-drift.sh (five guards), and runs ALL of them before
failing, so one red guard no longer hides the rest — it still blocks
- Lint (required) absorbs the deleted job's bash -n + shellcheck, which restated
Lint's own 19-file SHELLCHECK_FILES sweep at the same severity, plus its
PSScriptAnalyzer; `make lint-warnings` keeps the advisory SC2034 pass
- apt-get in Lint is now conditional on shellcheck being absent
- the k3s-cuda paths move to drift-checks.yaml with the check-facts guard (#616)
- docs/SUPPLY_CHAIN.md and .cursor/BUGBOT.md both asserted the gate existed;
corrected to name the required job
Mutation-proved: changing cluster.sh without regenerating the manifest turns
`make drift` red (anchor asserted: blob 87080bd -> 0b74919c); a stale manifest
plus a style violation reports both and exits non-zero. bats 1125/1125 green.
Refs tracebloc/backend#2183
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

/fr-pass

Best-effort functional review passed (triage: merged clean, non-interactive; behavioral evidence limited while e2e journey is red — backend#2206). Advancing to Ready for prod.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@LukasWodka@saadqbal