Fix windows logging tests and guard against runtime panic#6105
Open
EvanDorsky wants to merge 1 commit into
Open
Fix windows logging tests and guard against runtime panic#6105EvanDorsky wants to merge 1 commit into
EvanDorsky wants to merge 1 commit into
Conversation
Member
Author
|
Hm, macOS tests are failing because of bad dependencies? Did something happen to our runners? @cheukt From this PR's CI run just now: |
dgottlieb
approved these changes
Jun 12, 2026
dgottlieb
left a comment
Member
There was a problem hiding this comment.
lgtm -- though I expect you'll look into how modules are working.
I expect if there is a module's problem -- that could be more involved and you'll want to open a separate PR.
But happy to re-review here if that time comes also.
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.
Fix for CI failure: https://github.com/viamrobotics/rdk/actions/runs/27287348888/job/80598208096
Background
ETW logging operates with a provider / session model, where a provider is like a publisher and a session is like a subscriber in a pub/sub model.
viam-server now calls out to
logmanto start a logging session when it opens, and to kill that session before it exits. Provider registration is handled by thego-winiolibrary.There is a bug in the
go-winiolibrary that can lead to a nil pointer dereference when attempting to close a log provider if a logging session remains active after the provider is closed. This scenario shouldn't even be reached in prod becauseIn CI, multiple logging providers are being created in parallel all with the same name and ID. Multiple loggers are also trying to create logging sessions with the same name, so if test A and test B start in sequence and each test starts a logging provider and a logging session, the session from test B can be active while the provider from test A is being closed.
None of this would be a problem, though, if not for the bug PLUS a quirk of how providers and sessions interact. When a session closes, it sends a notification to all active providers informing them of the state change. This notification is caught by a callback in
go-winio, and that callback assumes that the provider is non-nil:permalink to
go-winiocodeSo if the callback fires after the provider has already been closed (which makes it
nil), then we get the nil pointer panic at runtime.Fix
The fix (suggested by Claude) seems to be to just never explicitly call
provider.Close. Windows already unregisters providers on its own, and if we never callprovider.Closethen the provider will never benilwhen the callback fires.So this branch just removes the calls to
provider.Closeand also changes the tests a little bit so they don't unnecessarily leak sessions.