Skip to content

search: guard against panic for missing repo broadcast on nil stream

Administrator requested to merge rvt/nil-stream-guard into main

Created by: rvantonder

The function MissingRepoRevStatus() takes a stream and sends status messages if there are missing repos. Some time ago, we maintained the implicit invariant that this stream is non-nil. In a previous commit, I broke this invariant where it became possible to call this function with a nil stream on this line: https://github.com/sourcegraph/sourcegraph/commit/95dba9b3b064dbf1031734ca5aceda0058856abc#diff-81779cda5e11347c5a456ef4c0c0b462ad0cbab0d2a86a261337ff4be277b7cfR626

@coury-clark this is the issue that likely caused your trouble using the GQL API in thread

I've audited the calls for other MissingRepoRevStatus constructors and I'm quite sure they're safe, but to make doubly sure I've put the nil check inside this function, rather than the one caller that broke the invariant. It does make me want to put a panic("broken invariant: non-nil stream") in this function here, but the code calling into is bound to clear up soon. Right now we're tiptoeing around the value of stream in our backend, whether it is nil or not, that's holding for now, and it'll become easier to reason over when we concentrate the logic for stream is nil? in one place.

I could write a test for MissingRepoRevStatus that checks whether this returns a noop function, but it's honestly low value. Open to being persuaded.

CI currently broken, please ignore and stamp.

Merge request reports