I think the logic is correct. However, this function is getting more complicated, and I would like you to consider an alternative approach.

My suggestion starts from the assumption that, when draining, we don't need to run n.reload(ts), which will make things easier IMO. Consider the Manager.Run() function from main branch. When the n.stopAndDrainRequested is signaled, we can simply exit the for loop. After the existing for loop, we add something like this, to handle the draining (consider it a pseudo-code, I haven't run it):

if n.opts.DrainTimeout > 0 {
  drainTimedOut := time.After(n.opts.DrainTimeout)

  for n.queueLen() > 0 {
    select {
    case <-drainTimedOut:
      return // TODO would be even cleaner to exit the for loop instead

    default:
      alerts := n.nextBatch()

      if !n.sendAll(alerts...) {
        n.metrics.dropped.Add(float64(len(alerts)))
      }
    }
  }
}

In this way we can keep the draining logically separated from the main loop, and I believe it may be easier to follow the logic.

What about keeping Run() as is (we may split it here) and only delay the call to n.cancel() in Stop(): loop/block until the timeout is reached or the queue is empty, then call n.cancel() which will stop the Run()
This way we don't need to re-implement the Run logic.

What about keeping Run() as is (we may split it here) and only delay the call to n.cancel() in Stop(): loop/block until the timeout is reached or the queue is empty, then call n.cancel() which will stop the Run()
This way we don't need to re-implement the Run logic.

My concern with making this change is that it changes the contract of Stop(), which may cause issues elsewhere. Previously, Stop() signalled that the manager should shutdown, but didn't block, whereas with this suggestion, Stop() will block until the manager has stopped.

I've implemented something along the lines of what @pracucci suggested in e4f5fee. Let me know what you think.

My concern with making this change is that it changes the contract of Stop(), which may cause issues elsewhere. Previously, Stop() signalled that the manager should shutdown, but didn't block, whereas with this suggestion, Stop() will block until the manager has stopped.

I don't see how the contract will change if the default value of DrainTimeout is 0.
(one can argue that the same applies for Run now, before it wasn't blocking, now it does, but again the default DrainTimeout is 0)
Note that we take a similar approach for remote write queues here

(one can argue that the same applies for Run now, before it wasn't blocking, now it does, but again the default DrainTimeout is 0)

Run was blocking before - before this PR, Run wouldn't return until Stop was called, and this PR does not change this behaviour.

I've changed the behaviour of Stop in 086be26.

I'm not convinced it's safe to drain the queue here. As you stated in another comment, the Run() contract is that it blocks until the Manager has stopped executing. Moving the draining here is breaking such contract.