Store: Fix panic on nil resp from response heap

[matej-g]

Signed-off-by: Matej Gera matejgera@gmail.com

• I added CHANGELOG entry for this change.
• Change is not relevant to the end user.

Changes

As part of refactoring MultiTSDB to use proxy methods (#5552), I noticed that lazy evaluation strategy is causing the receiver to panic in some cases. Upon closer looker, I realized this is happening when the resp from heap is nil (which can happen if there are no responses, seems like this can happen if e.g. I query for non-existing metric). We still send the nil resp further down the line, causing a panic. This fix checks for nil and skips a resp if this is true.

Verification

• Manual test with receiver from Receiver: Use proxy store for MultiTSDB #5552
• Some E2E tests with receiver also depend on querying for non-existing metrics so after Receiver: Use proxy store for MultiTSDB #5552 is using lazy eval, this will be implicitly tested as well

[fpetkovski]

Hmm, shouldn’t next return false in this case?

[matej-g]

Hmm, shouldn’t next return false in this case?

So I haven't dived too much into that new heap code, I assumed this is a valid case since we can return nil from the At() method on the dedup heap (cc @GiedriusS).

[GiedriusS]

Hmm, shouldn’t next return false in this case?

So I haven't dived too much into that new heap code, I assumed this is a valid case since we can return nil from the At() method on the dedup heap (cc @GiedriusS).

Maybe you could point to exact tests which are failing because of this? It would be interesting to check this out with a debugger.

[matej-g]

You can pull this branch and just change evaluation strategy at here.

For example, this rule E2E test fails as receiver will panic. But I think you can easily trigger this also with just setting up query+receiver and querying for a non-existing metric.

[GiedriusS]

I took a look at this and indeed nil needs to be handled but I think at a different place. I think we could either improve lazyRespSet's Empty() to wait for at least one response to be able to determine whether it is empty or we could handle nil's in Less(). I think the latter isn't enough because we might build an invalid heap? In other words, h.Min().rs might point to some respSet that isn't actually the "smallest" one with lazy proxying if my thinking is correct 🤔

[matej-g]

Not needed, will be addressed by #5746