Skip to content

ci(lint): 193 test-only bodyclose findings in providers/azure (split from #1754) #63

Open

Description

@cristim

Split out of LeanerCloud/cloud-commitments-cli#1754 so it can move independently of that 1291-finding backlog. This subset is one linter, in one module.

Triage result: 193 findings, 0 real leaks

Requested triage was: separate live request paths from error paths from test-only/init-once code, and establish which are reachable in production. The answer is that none are, on two independent grounds, both verified rather than inferred.

1. Production reachability is zero, by Go build semantics

All 193 findings sit at call sites in _test.go files — 10 files, no exceptions:

file findings
services/internal/reservations/purchase_test.go 35
services/compute/client_test.go 27
services/search/client_test.go 22
services/managedredis/client_test.go 22
services/database/client_test.go 22
services/cosmosdb/client_test.go 20
services/cache/client_test.go 20
services/synapse/client_test.go 15
services/savingsplans/client_test.go 5
internal/pricing/retail_prices_test.go 5

Go never compiles _test.go into a production binary, so this is not "probably unreachable" — it is unreachable by language rule.

The one shared helper involved, providers/azure/mocks/azure_mocks.go:138 CreateMockHTTPResponse, is an ordinary non-test file and therefore could in principle ship. It does not: every importer of providers/azure/mocks is a _test.go file (verified across the repo). It is also not itself flagged — bodyclose flags the call sites.

2. There is no resource to leak, by construction

Every flagged response is built by one of two shapes, both in-memory:

func CreateMockHTTPResponse(statusCode int, body string) *http.Response {
	return &http.Response{
		StatusCode: statusCode,
		Body:       io.NopCloser(bytes.NewBufferString(body)),
		Header:     make(http.Header),
	}
}
func fakeResp(status int, body string) *http.Response {
	return &http.Response{
		StatusCode: status,
		Body:       io.NopCloser(bytes.NewBufferString(body)),
		Header:     make(http.Header),
	}
}

plus local r.Body = io.NopCloser(bytes.NewReader(...)) assignments in compute, managedredis and synapse.

io.NopCloser.Close() returns nil and does nothing, wrapping a bytes.Buffer / bytes.Reader held in memory. There is no socket, no connection, no file descriptor. The condition bodyclose exists to detect — an unclosed body pinning a connection so it cannot be reused or released — cannot occur here.

Reachability classes requested: live request path 0, production error path 0, init-once 0, test-only 193.

Corrected severity

An earlier revision of LeanerCloud/cloud-commitments-cli#1754 described these as "a resource-leak class on a provider that makes real API calls", i.e. connection and file-descriptor exhaustion. That was wrong and is corrected there. It was inferred from the linter's name rather than measured. These are 193 true-positive lint findings and 0 true-positive defects.

They still matter, but only because 193 findings will drown a lint gate when providers/azure is added to it (LeanerCloud/cloud-commitments-cli#1754). That is a reviewability problem, not a reliability one — hence priority/p3.

The genuinely production-code findings in this module and its siblings (49 total: errcheck 18, unused 12, staticcheck 10, errorlint 8, gosec 1) stay in LeanerCloud/cloud-commitments-cli#1754 and should be triaged well ahead of this.

Fix shape and sizing

bodyclose reports at the call site, not the helper, so there is no single place to fix and no annotation that would cover the set.

  1. Close the bodies at the 193 call sites. Mechanical, no judgement needed, but it adds ~193 lines that close a no-op closer over a memory buffer — noise that a future reader has to re-derive as pointless. Effort: m, low risk, tedious.
  2. Exclude bodyclose on _test.go in .golangci.yml. Roughly four lines. Defensible on the merits rather than as suppression: the linter's premise is a real network response, and the file already excludes gocyclo on test files for a comparable "this check does not apply here" reason. Effort: xs.

Recommendation: option 2, with the reason written into the config comment so it reads as a scoping decision rather than a silenced gate. It needs a deliberate reviewer sign-off, because the standing rule here is that CI debt does not get gate-suppressed — the distinction being whether a real defect is hidden or a linter is applied outside its domain. The evidence above says the latter, but that call belongs to a reviewer, not to this triage.

Whoever takes it should confirm no production bodyclose findings appear later by re-checking the prod/test split rather than the total.

Verification notes for whoever picks this up

  • Any before/after finding comparison must be a set diff (comm on normalized, sorted findings), not a total. Equal totals hide an offsetting swap.
  • golangci-lint run must end at exit 0 with a non-empty findings block before being recorded as clean. --out-format was removed in v2 and a concurrent invocation exits 3 — both produce zero findings and read exactly like a clean run.

Related

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

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions