Change https://golang.org/cl/65475 mentions this issue: x/build/devapp: check for nil Commit field causing panic

This caused the goissues.org server to crash once too. I've since added a nil check with a log (in commit be9a001e), but haven't seen another occurrence yet.

It happened at 5:28:56 am EDT, after the server has been running for many hours. It constantly iterates over every CL of most Gerrit projects. Given I haven't seen another nil Commit for days since then, I suspect this happens pretty rarely and is timing-sensitive (i.e., checking the same CL after time passes, it might no longer have a nil Commit). For example, CL 65473 (from issue report) has a non-nil Commit right now.

Is it a race issue? Could we run maintner with the race detector enabled?

I don't have enough information to say conclusively. It may be either a data race or a non-race (but still timing-related) logic issue.

I tried running my maintner client with race detector a bit, and it didn't find any races.

I just added a test in godata, but I can't reproduce this.

My test was:

func TestGerritMetaNonNil(t *testing.T) {
        c := getGoData(t)
        c.Gerrit().ForeachProjectUnsorted(func(gp *maintner.GerritProject) error {
                var maxCL int32
                gp.ForeachCLUnsorted(func(cl *maintner.GerritCL) error {
                        if cl.Meta == nil {
                                t.Errorf("%s: enumerated CL %d has nil Meta", gp.ServerSlashProject(), cl.Number)
                        }
                        if len(cl.Metas) == 0 {
                                t.Errorf("%s: enumerated CL %d has empty Metas", gp.ServerSlashProject(), cl.Number)
                        }
                        if cl.Commit == nil {
                                t.Errorf("%s: enumerated CL %d has nil Commit", gp.ServerSlashProject(), cl.Number)
                        }
                        if cl.Number > maxCL {
                                maxCL = cl.Number
                        }
                        return nil
                })

                // And test that CL won't yield an incomplete one either:                                                                                                                  
                for n := int32(0); n <= maxCL; n++ {
                        cl := gp.CL(n)
                        if cl == nil {
                                continue
                        }
                        if cl.Meta == nil {
                                t.Errorf("%s: CL(%d) has nil Meta", gp.ServerSlashProject(), cl.Number)
                        }
                        if len(cl.Metas) == 0 {
                                t.Errorf("%s: CL(%d) has empty Metas", gp.ServerSlashProject(), cl.Number)
                        }
                        if cl.Commit == nil {
                                t.Errorf("%s: CL(%d) has nil Commit", gp.ServerSlashProject(), cl.Number)
                        }
                }
                return nil
        })
}

It does find the two deleted CLs from #22060, but no CLs with nil Commits.

So, I'm not sure what to do for this issue.

Change https://golang.org/cl/107296 mentions this issue: maintner: change type/name of GerritCL meta fields, ensure Meta always non-nil

When I played with NewNetworkMutationSource in #24866, I got a better idea of how maintner works. From that, I suspect it's a rare occurrence that has to do with the order and amount in which mutations are processed. Some of the processing happens whenever MutationStreamEvent.End is true. Perhaps what happens is that in some very rare cases, the server ends the set of mutations too early (or sends them in an unexpected order) in such a way, that the post-processing that happens temporarily produces a nil Commit.

This seems to be very rare and hard to reproduce, but the issue is real and I don't have any evidence that it can't happen in the future again.

An idea for how to proceed is the following. Before making GerritCL available for use by users of maintner API, we can add some code that does a sanity check on the GerritCL. If any of its fields have unexpected/invalid values, then it can hide the CL (so the users code would never see it and panic on it), and log to stderr about an "internal error" or inconstancy with information that would be useful for debugging.

Having maintner emit some debugging information in stderr is annoying to users, but it's even more annoying to have to keep writing if cl.Commit == nil { ... } guards because there's no confidence that Commit will always be non-nil in the future.

What do you think of this as a way to move forward (to improve the API quality of the package and eventually its implementation too)?

@shurcooL, that's what I did 4 minutes before your reply. :) See the CL above.

I was just typing that I noticed you did pretty much exactly that (sans debug logging) in Patch Set 3 of that CL. :)

From a quick glance, it looks fantastic. 👍 Sorry, can't review it thoroughly right now because I gotta step away from the computer, but I can look later. Edit: Left a review on CL.