Skip to content

refactor(queue): replace a dead store with the reason it is not needed - #139

Closed
rldyourmnd wants to merge 1 commit into
mainfrom
fix/drop-dead-store-in-queue-intent
Closed

refactor(queue): replace a dead store with the reason it is not needed#139
rldyourmnd wants to merge 1 commit into
mainfrom
fix/drop-dead-store-in-queue-intent

Conversation

@rldyourmnd

Copy link
Copy Markdown
Contributor

Code scanning (enabled in #138) flagged go/useless-assignment-to-local at queue_intent.go:673. It is a true positive — the one true positive out of the 18 findings that scan produced.

if exists {
    if !queueIntentCoreIdentityEqual(existing, intent) { return … }
    intent = existing          // <- last statement of the branch
} else { … }

intent is redeclared by := at the top of the next iteration and nothing reads it in between.

Why the line was worse than harmless

It read as though the journal wanted the canonical record afterwards. It does not: the exists branch deliberately performs no write-back, because journal.Intents already holds existing, and that record carries mutable lease state a freshly rebuilt intent does not. Overwriting it would discard progress.

So the assignment implied the opposite of the invariant it sat next to. The comment now states it. Behaviour is unchanged — the store had no reader.

The pin had to move with it

The overlay is digest-pinned in config/garm-derivative.yaml, and build-garm-nddev.sh runs verify_digest against each file before building. Editing the file without updating the pin would have failed later, in a different tool, for whoever built next.

Updated through the repository's own generator rather than by hand:

$ go run ./cmd/gha-fleet render-garm-build
{"current": false, "script": "scripts/build-garm-nddev.sh", "written": true}
$ go run ./cmd/gha-fleet render-garm-build -check
{"current": true,  "script": "scripts/build-garm-nddev.sh", "written": false}

Worth knowing about this directory

./third_party/... matches no packages in this module:

$ go vet ./third_party/...
go: warning: "./third_party/..." matched no packages

The overlay is compiled only inside build-garm-nddev.sh, against a GARM checkout. So go vet and go test -race have never seen this file. This finding came from the scanner precisely because the test suite cannot reach it — which is an argument for having turned scanning on, and possibly an argument for covering the overlay some other way.

Verification

go vet ./... clean · go test -race ./... every package ok · go build -trimpath ./... ok · render-garm-build -check current.

The other 17

Read individually and dismissed with the reasoning recorded on each alert: 12 go/log-injection (structured slog with NewJSONHandler — demonstrated that a newline in a value stays inside the JSON string), 1 go/weak-sensitive-data-hashing (the SigV4 X-Amz-Content-Sha256 payload digest, which the protocol mandates), 3 go/unhandled-writable-file-close (error paths that delete the file, and a flock sentinel never written to), 1 actions/untrusted-checkout (already guarded by git merge-base --is-ancestor before the checkout).

Code scanning flagged `go/useless-assignment-to-local` at
queue_intent.go:673. It is a true positive: `intent = existing` is the last
statement of the `if exists` branch, `intent` is redeclared by `:=` on the next
iteration, and nothing reads it in between.

The line was not harmless, because it read as though the journal wanted the
canonical record afterwards. It does not: the `exists` branch deliberately makes
no write-back, since `journal.Intents` already holds `existing` and that record
carries mutable lease state a freshly rebuilt `intent` does not. Overwriting it
would discard progress. The comment now says that; the assignment said nothing
and implied the opposite.

Behaviour is unchanged — the store had no reader.

The overlay is digest-pinned by `config/garm-derivative.yaml` and
`scripts/build-garm-nddev.sh` verifies each file against it before building, so
editing it without updating the pin would have failed later, in a different
tool, for someone else. The manifest digest is updated and the script's
generated region re-rendered with the repository's own command,
`gha-fleet render-garm-build`, whose `-check` mode then reports `current: true`.

Worth noting for anyone reading this file: `third_party/...` matches no packages
in this module, so `go vet` and `go test` never see the overlay. It is compiled
only inside `build-garm-nddev.sh`, against a GARM checkout. This finding came
from the scanner, not from the test suite, because the test suite cannot reach it.

Verified: `go vet ./...` clean, `go test -race ./...` every package ok,
`go build -trimpath ./...` ok, `render-garm-build -check` current.
@rldyourmnd

Copy link
Copy Markdown
Contributor Author

Closing this. The repository's own design says the change is not worth its price, and that is the right answer.

Reproducible GARM derivative failed:

GARM binary digest mismatch:
  got  9ba6bca8192f11db9c60996c6a1e96d88d282554a7d7afcd9404e4781cb979e7
  want 672a74e29ab7122d163c103a229d9360c5d937b88e8329839283c9f9d24b02e0

Not a build problem — every GARM package compiled and tested, workers/scaleset included. Go embeds a build ID derived from the content of its inputs, so a comment changes the binary, and the derivative binary is pinned twice: scripts/build-garm-nddev.sh:35 and config/garm-derivative.yaml:109.

Landing this would mean re-pinning binary_sha256 — asserting "these are the new authoritative derivative bytes" — for a comment. That inverts what the pin is for. It says these exact reviewed bytes; spending it on a cosmetic edit makes the next reviewer trust it slightly less, and the finding it buys is a dead store with no reader and no behavioural effect.

I had already updated the two overlay-level digests through gha-fleet render-garm-build before hitting this, so the chain is: overlay file → manifest overlay digest → generated script region → derivative binary digest. The fourth link is the one that should only move when behaviour moves.

The alert is dismissed as won't fix with this reasoning recorded on it. If the overlay is edited for a real reason later, folding the comment in then costs nothing extra.

@rldyourmnd rldyourmnd closed this Aug 23, 2026
@rldyourmnd
rldyourmnd deleted the fix/drop-dead-store-in-queue-intent branch August 23, 2026 22:41
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