servstate: close service ring buffer on process exit during stop - #925
Open
sundeep8967 wants to merge 2 commits into
Open
servstate: close service ring buffer on process exit during stop#925sundeep8967 wants to merge 2 commits into
sundeep8967 wants to merge 2 commits into
Conversation
Member
|
Hi @sundeep8967, Thank-you for the PR. It looks like we are trying to close the ring buffers for services that could still be running. |
sundeep8967
force-pushed
the
fix/ringbuffer-close-on-shutdown
branch
from
September 5, 2026 09:38
5a10147 to
778721c
Compare
When a service is stopped by the service manager, its log ring buffer was left open, meaning any log puller or reader iterators would wait until forced termination deadlines or remain open while the service was inactive. Furthermore, attempting to close ring buffers globally in LogManager.Stop() risks closing buffers for services that may still be running. Close the service's log ring buffer (s.logs.Close()) from within the service manager when the service reaches stopped state once the process has exited (or when stopped during backoff). If the service is subsequently restarted, ensure serviceForStart allocates a fresh ring buffer if the previous one was closed. Also fix the goroutine leak in TestLabels by ensuring m.Stop() is called at the end of the test. Fixes: canonical#681 Signed-off-by: sundeep8967 <sundeep8967@gmail.com>
sundeep8967
force-pushed
the
fix/ringbuffer-close-on-shutdown
branch
from
September 6, 2026 09:02
778721c to
68aec26
Compare
Author
|
Hi @hpidcock, Thank you for the review and guidance! I have updated the implementation according to your feedback:
Please take another look when you have a moment. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a service is stopped by the service manager, its log ring buffer was left open. Any log puller or reader iterators would wait until forced termination deadlines or remain open while the service was inactive. Furthermore, attempting to close ring buffers globally in
LogManager.Stop()risks closing buffers for services that could still be running.Solution
internals/overlord/servstate/handlers.go:s.exited(), closes.logs(_ = s.logs.Close()) when the service reachesstateStoppedafter its process has exited.s.stop(), closes.logswhen the service is stopped while instateBackoff.serviceForStart(), allocate a freshs.logs = servicelog.NewRingBuffer(maxLogBytes)if the service is subsequently restarted and the previous buffer was closed.internals/overlord/servstate/export_test.go:ServiceLogBufferhelper for test verification.internals/overlord/servstate/manager_test.go:TestStartStopServicesandTestStopDuringBackoffto verifys.logs.Closed()istrueafter stopping.internals/overlord/logstate/manager_test.go:TestLabelsby callingm.Stop()at the end of the test.Fixes #681
Testing
GOOS=linux go test -c ./internals/overlord/servstate).TestStartStopServicesandTestStopDuringBackoffwith ring buffer closure assertions.internals/overlord/logstatetests (TestTimelyShutdown,TestLabels,TestPlanChange).