fix(v2/secret): close notify from the poll goroutine

What does this MR do and why?

RotatingFileProvider.Shutdown closed the notify channel on both of its exit paths, including the one taken when its own context expires while the poll goroutine is still running. The poll goroutine is the channel's only sender, so a send racing that close panics with "send on closed channel", and nothing recovers it. A service shutting down cleanly under a deadline crashed instead.

The guard in poll, if ctx.Err() != nil { return }, narrows the window but cannot close it, because it is a check-then-act on state another goroutine mutates. The sends use select { case p.notify <- event: default: }, and that default guards a full buffer, not a closed channel.

This gives the channel back to its sender. poll closes notify as it exits, and Shutdown no longer does.

References

Closes #160 (closed)

Related, the flaky test in the same function that led to this: !707 (merged)

Differences

Before: Shutdown called close(p.notify) on both branches of its select, guarded by stopOnce. On the ctx.Done() branch the poll goroutine still owned the channel.

After: poll closes notify as it exits. The defer is registered after defer close(stopped), so LIFO ordering closes notify first and stopped second. Once Shutdown observes stopped closed, notify is closed too.

stopOnce is removed. It existed only to make the close happen once, and Shutdown is naturally idempotent without it: cancel is idempotent and a receive from a closed stopped returns immediately.

Behaviour change worth calling out

On the deadline path notify now closes slightly later, when the goroutine actually finishes, rather than immediately when Shutdown returns. A consumer using for event := range p.OnRotate(), the pattern in v2/secret/README.md, blocks marginally longer in that case. That seemed preferable to a panic.

notify now also closes when the context passed to Start is cancelled, even if Shutdown is never called; before, it stayed open until Shutdown. A range over OnRotate() ends when polling does, which is the better behaviour.

Doc comments on the type, Shutdown and OnRotate, and the RotatingProvider interface doc, are updated to say the poll goroutine owns the close.

How to set up and validate locally

From the v2 module:

  1. Run the package:

    cd v2 && go test -count=1 ./secret/

  2. Confirm the regression test catches the bug. In v2/secret/rotating_file.go, put close(p.notify) back at the end of Shutdown and remove the defer close(p.notify) from poll, then run:

    go test -count=1 -run TestRotatingFileProvider_Shutdown_LeavesNotifyOpenWhilePollRuns ./secret/

    It fails with:

    Shutdown closed notify while the poll goroutine could still send on it

    Restore both lines and it passes. The test is deterministic: it sets stopped to a channel that is never closed, standing in for a goroutine still running, so Shutdown must take the ctx.Done() branch. It uses the unexported fields, following the existing internal test directly above it.

MR acceptance checklist

  • poll closes notify; Shutdown does not
  • stopOnce removed, Shutdown still idempotent
  • Deterministic regression test, verified to fail without the fix
  • Doc comments updated on the type, Shutdown and OnRotate
  • Conforms to the code review guidelines
Edited by Jeston Singh

Merge request reports

Loading
Loading