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
Community-fork copy of !753 (closed). That MR was opened from a personal fork, where the pipeline cannot complete because the danger-review job authenticates to Vault with the CI JWT and the bound claim only allows the canonical project_id. Raised here from gitlab-community/gitlab-org/labkit, on the maintainer suggestion, so the pipeline runs. The change is identical.
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:
-
Run the package:
cd v2 && go test -count=1 ./secret/ -
Confirm the regression test catches the bug. In
v2/secret/rotating_file.go, putclose(p.notify)back at the end ofShutdownand remove thedefer close(p.notify)frompoll, 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 itRestore both lines and it passes. The test is deterministic: it sets
stoppedto a channel that is never closed, standing in for a goroutine still running, soShutdownmust take thectx.Done()branch. It uses the unexported fields, following the existing internal test directly above it.
MR acceptance checklist
-
pollclosesnotify;Shutdowndoes not -
stopOnceremoved,Shutdownstill idempotent - Deterministic regression test, verified to fail without the fix
- Doc comments updated on the type,
ShutdownandOnRotate - Conforms to the code review guidelines