When a Profiling Archive Includes Nodes It Never Reached

This is my fifth Birdwatcher PR, and the same theme keeps coming up: a tool saying it succeeded when it didn't. The fix is PR #548, fixing #547. It's still open.

What the command does

GetPprofCommand in states/pprof.go is behind Birdwatcher's pprof command. It sends one http.Get to every node in the cluster to collect goroutine and heap dumps, then packs them into a tar.gz. You'd reach for it when something is wrong with the cluster, which is exactly when some nodes might not answer.

Three problems

Failures were archived as empty successes. When a node's fetch failed (connection refused, timeout or a body-read error), the consumer goroutine threw away the error with a bare fmt.Println(). It still wrote a tar header and a 0-byte body for that node, and printed the same "fetched, added into archive file" message as a real success. An unreachable node looked exactly like a node that returned an empty profile.

HTTP status was never checked. A 500 or 404 error page got archived as if it were profile data.

resp.Body was never closed. That leaked a connection on every call. fetchInstanceMetrics in states/metrics.go had the same gap.

The fix

• On a per-node failure, skip the archive entry entirely and print the real error with the node's name and ID.
• After writing the archive, print a summary of every node that failed.
• Treat any non-2xx response as a failure.
• Close resp.Body on every path, in both pprof.go and metrics.go.

Testing with a real dead node

I didn't want to mock the HTTP client, so the new states/pprof_test.go uses a real httptest.Server for one node and points another at an unreachable loopback address.

• TestGetPprofCommandSkipsUnreachableNode: the archive contains only the reachable node's real data, nothing for the dead one, and the failure gets reported
• TestGetPprofCommandTreatsNonOKStatusAsFailure: a fake endpoint returns 500, and nothing is archived for it

Both pass under -race. go build, go vet, gofmt and the full go test ./states were clean.

The pattern

Looking back at all my Birdwatcher fixes, they're mostly the same bug. A repair writes corrupted bytes and says "done." A restore writes nothing and says "done." Now a profiler archives nothing and says "fetched." It's a diagnostic tool, so people use it when things are already broken. That's when partial failure is most likely, and when a fake success costs the most.

For this one, I'd rather hand someone an archive with four nodes and a clear "node 5 failed: connection refused" than five files where one is silently empty.