x/tools/gopls: nil deref in evaluatePackageHandle (telemetry)

[adonovan]

This stack kRRIIg was reported by telemetry:

We don't have a program counter, so we can't tell if it is succ or succ.ph that is nil. (We know n.ph.depKeys is non-nil.)

	reachable := b.s.pkgIndex.NewSet()
	for depID, succ := range n.succs {
		n.ph.depKeys[depID] = succ.ph.key // panic
		reachable.Add(succ.idxID)

crash/crash
runtime.gopanic:+69
runtime.panicmem:=262
runtime.sigpanic:+19
golang.org/x/tools/gopls/internal/cache.(*packageHandleBuilder).evaluatePackageHandle:+26
golang.org/x/tools/gopls/internal/cache.(*packageHandleBuilder).buildPackageHandle:+35
golang.org/x/tools/gopls/internal/cache.(*Snapshot).getPackageHandles.func2.1:+8
golang.org/x/sync/errgroup.(*Group).Go.func1:+3
runtime.goexit:+0

golang.org/x/tools/gopls@v0.16.1 go1.23.0 linux/amd64 vscode (1)
golang.org/x/tools/gopls@v0.16.1 go1.23.0 darwin/arm64 other,vscode (1)

Issue created by golang.org/x/tools/gopls/internal/telemetry/cmd/stacks.

[gabyhelp]

Related Issues and Documentation

• x/tools/gopls: "nil metadata for %q" panic in Snapshot.getPackageHandles (reported by telemetry) #66144
• x/tools/gopls: crash in packageReferences (reported by telemetry) #66250 (closed)
• x/tools/gopls/internal/lsp/cache: nil pointer dereference in snapshot.getPackageHandles (v0.12.4) #61420 (closed)
• x/tools/gopls: crash in buildPackageHandle (nil packageHandle) #66414 (closed)
• gopls: automated issue report (crash) #63702 (closed)
• x/tools/gopls: bug reported by telemetry [qoVPwQ] #64243 (closed)
• x/tools/gopls: nil panic loading analysisNode.summary #66732
• x/tools/gopls: nil panic loading actionSummary.Err #66788
• x/tools/gopls: toGobDiagnostic bug reported by telemetry #65695 (closed)
• gopls: automated issue report (crash) #63700 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[findleyr]

I assert that I understand how this was possible, and that it has now been fixed (coincidentally) by refactoring.

Previously, we did not check handleNode.err before reading from succs. This meant that on context cancellation, there was a race in which we could try to read succ.ph even though succ.err was non-nil.

As part of a refactoring I performed recently, I changed evaluatePackageHandle to return an error, and that err terminates the errgroup. So this should no longer be a problem.
https://go-review.git.corp.google.com/c/tools/+/614164/12/gopls/internal/cache/check.go#1009