Skip to content

fix(ci): key the lint cache on the toolchain and drop the prefix fallback - #374

Merged
onedr0p merged 2 commits into
mainfrom
fix/sa5011-conflict-test
Jul 30, 2026
Merged

fix(ci): key the lint cache on the toolchain and drop the prefix fallback#374
onedr0p merged 2 commits into
mainfrom
fix/sa5011-conflict-test

Conversation

@onedr0p

@onedr0p onedr0p commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Go Lint failed intermittently on a staticcheck false positive in a test that was already correct:

internal/topology/conflict_test.go:101:6: SA5011(related information): this check suggests that the pointer can be nil (staticcheck)
internal/topology/conflict_test.go:104:18: SA5011: possible nil pointer dereference (staticcheck)

t.Fatalf does terminate, but staticcheck only knows that when the testing package's facts are part of the run, and with a restored ~/.cache/golangci-lint they are not reliably there. The result is nondeterministic on byte-identical trees: on #364's branch the same restored cache key (Linux-golangci-lint-8214272e…) produced 0 issues in a 40s run (job 90625750155) and 2 issues in two 15s runs (job 90580644762, job 90653049615 — the latter blocking the merge queue).

The fix is the cache, not the test

The first pass at this reshaped the assertion, which treats the symptom. The key hashes only go.sum and .golangci.yml, so the poisoned entry survives any commit that does not touch those two files, and the bare-prefix restore-keys hands it to every subsequent run until GitHub's 7-day eviction. A different cache state simply selects a different package to fail on, and there are several other t.Fatal-then-deref sites for it to land on.

So:

  • the key now covers .mise/mise.lock, so a golangci-lint or Go bump starts fresh instead of handing a new binary the old one's facts
  • restore-keys is gone: a miss must start cold rather than fall back to the newest entry under the prefix, which is what let one bad entry outlive its own commit

With the input fixed, the test goes back to the file's own cond == nil || cond.Status != want idiom rather than the else branch. Both are structurally SA5011-proof, but the short-circuit form matches the other condition assertions in the package and needs no rationale comment to survive a future cleanup pass.

Also in this PR

Reviewing the surrounding code turned up four unrelated defects on the same paths.

cancelled did not fail the merge gate (Build Success). if: !cancelled() still runs that job when individual needs were cancelled but the run was not; needs.*.result then contains cancelled, not failure, so Any jobs failed? was skipped and All jobs passed or skipped? exited 0. A required job that never reached a verdict could clear branch protection.

Two Eventf calls passed an already-formatted message as the printf format string. client-go does fmt.Sprintf(note, args...), so every PoolUsageHigh warning reached operators as pool 85%!f(MISSING)ull (warn at 80%!)(MISSING) while the stored condition message was correct.

AddressConflict never set ObservedGeneration, so a stale True on a node whose address was just edited is indistinguishable from a current one. The sibling NodeGroupReconciler sets it on both of its conditions.

One node's failed status update aborted the whole conflict pass, leaving every later node unstamped and so invisible to placement. A 409 is routine here: each agent rewrites its own MiroirNode status every 60s. Per-node errors are now collected and joined, as NodeGroupReconciler already does.

The node group enqueue map func swallowed its List error, dropping the event with no requeue and no trace.

Tests

Two new (TestConflictOneBadNodeDoesNotBlockTheFleet, TestConflictConditionRecordsObservedGeneration) and three strengthened:

  • TestConflictEventFiresOncePerFreshConflict ran both passes over a byte-identical fixture, so SetStatusCondition returned false and the !wasConflicted gate it exists to protect was never reached. It now adds a third node on the shared address, so pass 2 rewrites the peer list of an existing conflict.
  • TestConflictSinglePassCoversAllNodes compared cond.Status == ConditionTrue as a bool, which passes on Unknown as readily as on False. It now compares the status exactly, and uses t.Errorf so a multi-node regression reports all three rather than whichever the randomized map order reached first.
  • TestPoolStatsPublisherRaisesHighDataUsage drained the event channel without inspecting the note; it now asserts the note carries the condition message verbatim.

Each new assertion was checked against a reverted fix to confirm it fails.

Verified: mise run test, and golangci-lint run against a cold GOLANGCI_LINT_CACHE.

#364's branch still carries the else form of the assertion and will want the same treatment when it comes out of draft.

staticcheck only knows t.Fatalf terminates when the testing package
facts are part of the run; with a restored golangci-lint cache they are
not always there, and SA5011 fires on the deref that follows. Moving it
into an else branch makes the guard structural.

Signed-off-by: Devin Buhl <devin@buhl.casa>
@onedr0p
onedr0p added this pull request to the merge queue Jul 30, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 30, 2026
…back

golangci-lint caches analyzer facts, so a stale or partial entry does not
just cost time: it turns staticcheck's SA5011 into a false positive on
untouched code, which is what went red in the merge queue. The key now
covers .mise/mise.lock, so a golangci-lint or Go bump starts fresh instead
of inheriting the previous binary's facts, and restore-keys is gone so a
miss starts cold instead of falling back to the newest entry under a bare
prefix; that fallback is what let one bad entry outlive its own commit.

With the input fixed, the test goes back to the file's own short-circuit
idiom instead of the else branch added in the previous commit: it needs no
rationale comment and matches the other condition assertions in the
package. It also compares the status exactly rather than folding Unknown in
with False, and reports every node instead of aborting on whichever the
randomized map order reached first.

Also in the workflow: cancelled now counts as failed in the Build Success
gate. !cancelled() still runs that job when individual needs were cancelled
but the run was not, so a required job that never reached a verdict could
clear branch protection.

And the surrounding topology and agent code the failure led through:

- Eventf passed an already-formatted message as the printf format string in
  two places. client-go does fmt.Sprintf(note, args...), so every
  PoolUsageHigh warning reached operators as
  "pool 85%!f(MISSING)ull (warn at 80%!)(MISSING)".
- AddressConflict never set ObservedGeneration, leaving a stale True
  indistinguishable from a current one.
- One node's failed status update aborted the whole conflict pass, leaving
  every later node unstamped and so invisible to placement. Per-node errors
  are now collected and joined, as the sibling node group reconciler
  already does.
- The node group enqueue map func swallowed its List error, dropping the
  event with no requeue and no trace.

Signed-off-by: Devin Buhl <devin@buhl.casa>
@onedr0p onedr0p changed the title test(topology): keep the condition deref inside the nil guard fix(ci): key the lint cache on the toolchain and drop the prefix fallback Jul 30, 2026
@onedr0p
onedr0p added this pull request to the merge queue Jul 30, 2026
Merged via the queue into main with commit 789e990 Jul 30, 2026
17 checks passed
@onedr0p
onedr0p deleted the fix/sa5011-conflict-test branch July 30, 2026 15:47
gabrielcosi pushed a commit to gabrielcosi/home-ops that referenced this pull request Jul 31, 2026
….11.13 ➔ 0.11.14) (#197)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [ghcr.io/home-operations/charts/miroir](https://github.com/home-operations/miroir) | patch | `0.11.13` → `0.11.14` |

---

### Release Notes

<details>
<summary>home-operations/miroir (ghcr.io/home-operations/charts/miroir)</summary>

### [`v0.11.14`](https://github.com/home-operations/miroir/blob/HEAD/CHANGELOG.md#01114-2026-07-30)

[Compare Source](home-operations/miroir@0.11.13...0.11.14)

##### Features

- **go:** update module google.golang.org/grpc (v1.82.1 → v1.83.0) ([#&#8203;375](home-operations/miroir#375)) ([572e629](home-operations/miroir@572e629))

##### Bug Fixes

- **ci:** key the lint cache on the toolchain and drop the prefix fallback ([#&#8203;374](home-operations/miroir#374)) ([789e990](home-operations/miroir@789e990))

</details>

---

### Configuration

📅 **Schedule**: (in timezone Europe/Berlin)

- Branch creation
  - At any time (no schedule defined)
- Automerge
  - At any time (no schedule defined)

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Mend Renovate](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0NC4wLjAiLCJ1cGRhdGVkSW5WZXIiOiI0NC4wLjAiLCJ0YXJnZXRCcmFuY2giOiJtYWluIiwibGFiZWxzIjpbInJlbm92YXRlL2NvbnRhaW5lciIsInR5cGUvcGF0Y2giXX0=-->

Reviewed-on: https://git.xcd.dev/gabrielcosi/home-ops/pulls/197
eleboucher pushed a commit to eleboucher/homelab that referenced this pull request Jul 31, 2026
….11.12 ➔ 0.11.15) (#1764)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [ghcr.io/home-operations/charts/miroir](https://github.com/home-operations/miroir) | patch | `0.11.12` → `0.11.15` |

---

### Release Notes

<details>
<summary>home-operations/miroir (ghcr.io/home-operations/charts/miroir)</summary>

### [`v0.11.15`](https://github.com/home-operations/miroir/blob/HEAD/CHANGELOG.md#01115-2026-07-31)

[Compare Source](home-operations/miroir@0.11.14...0.11.15)

##### Bug Fixes

- **csi:** log retryable status codes below error level ([#&#8203;380](home-operations/miroir#380)) ([4774d21](home-operations/miroir@4774d21))
- **snapshot:** lift possibly-set barrier on failed suspend-io and sweep mid-runtime ([#&#8203;382](home-operations/miroir#382)) ([09e3aa7](home-operations/miroir@09e3aa7))

### [`v0.11.14`](https://github.com/home-operations/miroir/blob/HEAD/CHANGELOG.md#01114-2026-07-30)

[Compare Source](home-operations/miroir@0.11.13...0.11.14)

##### Features

- **go:** update module google.golang.org/grpc (v1.82.1 → v1.83.0) ([#&#8203;375](home-operations/miroir#375)) ([572e629](home-operations/miroir@572e629))

##### Bug Fixes

- **ci:** key the lint cache on the toolchain and drop the prefix fallback ([#&#8203;374](home-operations/miroir#374)) ([789e990](home-operations/miroir@789e990))

### [`v0.11.13`](https://github.com/home-operations/miroir/blob/HEAD/CHANGELOG.md#01113-2026-07-29)

[Compare Source](home-operations/miroir@0.11.12...0.11.13)

##### Bug Fixes

- **csi:** refuse to reformat a restore whose device reads blank ([#&#8203;373](home-operations/miroir#373)) ([b7ab14b](home-operations/miroir@b7ab14b))

##### Continuous Integration

- split the conformance leg and build the e2e images once ([#&#8203;371](home-operations/miroir#371)) ([3d58054](home-operations/miroir@3d58054))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4xMDEuMSIsInVwZGF0ZWRJblZlciI6IjQzLjEwMS4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJyZW5vdmF0ZS9jb250YWluZXIiLCJ0eXBlL3BhdGNoIl19-->

Reviewed-on: https://git.erwanleboucher.dev/eleboucher/homelab/pulls/1764
doonga pushed a commit to greyrock-labs/home-ops that referenced this pull request Jul 31, 2026
….11.13 ➔ 0.11.15) (#215)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [ghcr.io/home-operations/charts/miroir](https://github.com/home-operations/miroir) | patch | `0.11.13` → `0.11.15` |

---

### Release Notes

<details>
<summary>home-operations/miroir (ghcr.io/home-operations/charts/miroir)</summary>

### [`v0.11.15`](https://github.com/home-operations/miroir/blob/HEAD/CHANGELOG.md#01115-2026-07-31)

[Compare Source](home-operations/miroir@0.11.14...0.11.15)

##### Bug Fixes

- **csi:** log retryable status codes below error level ([#&#8203;380](home-operations/miroir#380)) ([4774d21](home-operations/miroir@4774d21))
- **snapshot:** lift possibly-set barrier on failed suspend-io and sweep mid-runtime ([#&#8203;382](home-operations/miroir#382)) ([09e3aa7](home-operations/miroir@09e3aa7))

### [`v0.11.14`](https://github.com/home-operations/miroir/blob/HEAD/CHANGELOG.md#01114-2026-07-30)

[Compare Source](home-operations/miroir@0.11.13...0.11.14)

##### Features

- **go:** update module google.golang.org/grpc (v1.82.1 → v1.83.0) ([#&#8203;375](home-operations/miroir#375)) ([572e629](home-operations/miroir@572e629))

##### Bug Fixes

- **ci:** key the lint cache on the toolchain and drop the prefix fallback ([#&#8203;374](home-operations/miroir#374)) ([789e990](home-operations/miroir@789e990))

</details>

---

### Configuration

📅 **Schedule**: (in timezone America/New_York)

- Branch creation
  - At any time (no schedule defined)
- Automerge
  - At any time (no schedule defined)

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Mend Renovate](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0NC4wLjAiLCJ1cGRhdGVkSW5WZXIiOiI0NC4wLjAiLCJ0YXJnZXRCcmFuY2giOiJtYWluIiwibGFiZWxzIjpbInJlbm92YXRlL2NvbnRhaW5lciIsInR5cGUvcGF0Y2giXX0=-->

Reviewed-on: https://git.greyrock.io/greyrock-labs/home-ops/pulls/215
Sign up for free to 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.

1 participant