CI: gofmt/lint/tidy/vulncheck gates, enable -race, bump to go1.26.8 - #7
Merged
Conversation
Unchecked error returns (errcheck), deprecated/simplifiable expressions (staticcheck), and dead code (unused), surfaced while wiring golangci-lint into CI for the first time. Mostly mechanical discards (hash.Hash.Write and http.ResponseWriter.Write never meaningfully fail; resp.Body.Close/os.Remove in tests are best-effort cleanup), plus two real test-correctness fixes: gift_wrap_test.go and nipAZ_more_test.go both silently ignored a Sign()/comparison result that could have masked a real failure. Refs #6
Unchecked SetReadDeadline/SetWriteDeadline/Close/WriteControl/ WriteMessage return values, mechanically discarded — best-effort on a possibly-already-closing connection, matching the existing pattern in relay/session.go. Refs #6
Unchecked error returns (mostly best-effort Close/Delete/View/Fetch calls, mechanically discarded), deprecated/simplifiable expressions, an ineffectual assignment, and a dead method, surfaced while wiring golangci-lint into CI for the first time. Two real fixes among the mechanical noise: - handler_cache.go silently ignored a malformed cache-options JSON payload instead of rejecting it, unlike its sibling five lines above which already handled the same case correctly. - store.go's event-delete path had an empty if-err-nil branch on a Put() failure (the error was checked, then discarded) instead of propagating it — the write could fail leaving the expiration index only partially updated, with nothing surfacing that. startHousekeeper now also logs pruneExpiredEvents' error instead of discarding it. Refs #6
…tines in tests go test -race ./relay/... intermittently failed with a genuine WARNING: DATA RACE between a Session's read/write/pingLoop goroutines (relay/session.go) from one test and a completely unrelated later test's fresh allocations (e.g. NewEventStore's migration write) -- caught only now because -race was never part of CI before this PR. Root cause: gorilla/websocket hijacks the underlying TCP connection on upgrade. httptest.Server tracks connection lifecycle via ConnState and treats a hijacked connection as already accounted for the instant it's hijacked, so srv.Close()'s own WaitGroup does not wait for that session's own goroutines to actually exit. t.Cleanup(srv.Close) alone therefore never guaranteed a prior test's session had finished before the next test started, letting its goroutines keep running -- and occasionally race -- against whatever memory a later test's next allocation happened to reuse. Added waitForSessionCount, polling SessionHandler.SessionCount() down to the expected value, and wired it into every place a test dials a websocket and needs the server-side session to be fully torn down before moving on: the two CORS tests and TestSession_GettersAndSessionCount in cors_test.go, the shared createWS test helper (used package-wide), and TestNew_StillUpgradesWebSocket in relay_test.go. Verified with 4 independent, uncached (-count=1) full go test -race ./relay/... runs post-fix, plus one full go test -race ./... run across every package -- all clean. Refs #6
Clears 13 stdlib CVEs (crypto/tls, crypto/x509, net/http) that govulncheck flags against 1.25.5 as reachable through relay/client and nipB7/client -- not nmilat code bugs, just an outdated toolchain floor. Refs #6
Brings ci.yml up to this org's own established pattern (ncli already uses go-version-file and -race) plus general Go-library practice: - go-version-file: go.mod instead of a hardcoded "1.25", so CI and go.mod can't silently drift apart. - go test -race: nmilat has already shipped one real goroutine-leak bug (0.2.5) and this PR fixes a second one that -race caught for the first time -- this class of bug needs the gate, not just luck. - New gates: gofmt -l (build fails the same job, not a separate one, so a failure is immediately visible alongside build/vet/test), go mod tidy diff check, golangci-lint (separate job, full repo, blocking -- the 155 pre-existing findings it surfaced are already fixed on this branch), govulncheck (separate job, blocking -- now clean after the 1.26.8 bump). - Explicit permissions: contents: read (least privilege; release.yml already scoped its own permissions correctly, ci.yml hadn't). - concurrency: cancel-in-progress, so superseded pushes don't queue. Refs #6
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #6
Summary
Brings
ci.ymlup to general Go-library practice and this org's own established pattern (nclialready usesgo-version-fileand-race):go-version-file: go.modinstead of a hardcoded"1.25"go test -race— nmilat has already shipped one real goroutine-leak bug (0.2.5); this PR's third commit fixes a second one that-racecaught for the first time, which is exactly the class of bug this gate exists forgofmt -l,go mod tidydiff check,golangci-lint(separate job, full repo, blocking),govulncheck(separate job, blocking)permissions: contents: read,concurrency: cancel-in-progressTo make those gates land clean rather than immediately red:
golangci-lintfindings repo-wide (verified with--max-issues-per-linter=0 --max-same-issues=0— the default caps were silently hiding the true count, which is more than double what I originally quoted)httptest.Server.Close()doesn't wait for a websocket session's own goroutines to exit (gorilla/websocket hijacks the connection, sohttptest's own connection-tracking WaitGroup considers it done the instant it's hijacked) — a leftover session from one test could still be running when the next test started. AddedwaitForSessionCountand wired it into every test that dials a websocket. Verified with 4 independent uncached-raceruns post-fix.go.modto1.26.8, clearing all 13 stdlib CVEsgovulncheckwas flagging (toolchain-version issues, not nmilat bugs)Also caught and fixed two real (non-cosmetic) bugs while fixing lint findings:
handler_cache.gosilently accepted malformed cache-options JSON instead of rejecting it, andstore.go's event-delete path had an emptyif err != nil {}branch on aPut()failure instead of propagating it.Commit-by-commit breakdown is in the individual commit messages.
Test plan
gofmt -l .cleangolangci-lint run --max-issues-per-linter=0 --max-same-issues=0 ./...— 0 issuesgovulncheck ./...— 0 vulnerabilities (reachable)go build ./...,go vet ./...cleango test -race ./...clean — full repo, 42 packages, allokgo test -race -count=1 ./relay/...run independently 4 times post-fix (no cache hits), all clean