pam: Fix races handling in VHS tests and fix bubbletea models implementations #846
+304
−195
Conversation
This file contains 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
3v1n0
force-pushed
the
races-handling-fixes
branch
2 times, most recently
from
eaefbdc to
5f17a7b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #846 +/- ##
==========================================
- Coverage 83.43% 79.13% -4.31%
==========================================
Files 83 104 +21
Lines 8689 10448 +1759
Branches 74 75 +1
==========================================
+ Hits 7250 8268 +1018
- Misses 1111 1719 +608
- Partials 328 461 +133 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
adombeck
reviewed
Mar 19, 2025
3v1n0
force-pushed
the
races-handling-fixes
branch
2 times, most recently
from
531db15 to
8be4cb5
Compare
3v1n0
force-pushed
the
races-handling-fixes
branch
from
8be4cb5 to
5d936e8
Compare
adombeck
approved these changes
Mar 20, 2025
We were not reading the proper files and so the races were handled but no artifact was exposed or read in logs. Fix this by looking for data races files to check if a race has happened (instead of relying on the exit status that would make the golden files check to fail)
Fixes a race that we've incredibly missed so far: Write at 0x00c0001dc030 by goroutine 3408: runtime.mapassign_faststr() /home/marco/go/pkg/mod/golang.org/[email protected]/src/runtime/map_faststr.go:223 +0x0 github.com/ubuntu/authd/examplebroker.(*Broker).NewSession() /DEV/authd/examplebroker/broker.go:294 +0xee8 github.com/ubuntu/authd/examplebroker.(*Bus).NewSession() /DEV/authd/examplebroker/dbus.go:80 +0x196 runtime.call64() /home/marco/go/pkg/mod/golang.org/[email protected]/src/runtime/asm_amd64.s:777 +0x42 reflect.Value.Call() /home/marco/go/pkg/mod/golang.org/[email protected]/src/reflect/value.go:365 +0x144 github.com/godbus/dbus/v5.exportedMethod.Call() /home/marco/go/pkg/mod/github.com/godbus/dbus/[email protected]/default_handler.go:128 +0x384 github.com/godbus/dbus/v5.(*exportedMethod).Call() <autogenerated>:1 +0x127 github.com/godbus/dbus/v5.(*Conn).handleCall() /home/marco/go/pkg/mod/github.com/godbus/dbus/[email protected]/export.go:193 +0x1084 github.com/godbus/dbus/v5.(*Conn).inWorker.gowrap1() /home/marco/go/pkg/mod/github.com/godbus/dbus/[email protected]/conn.go:435 +0x50 Previous read at 0x00c0001dc030 by goroutine 3405: runtime.mapaccess2_faststr() /home/marco/go/pkg/mod/golang.org/[email protected]/src/runtime/map_faststr.go:117 +0x0 github.com/ubuntu/authd/examplebroker.(*Broker).UserPreCheck() /DEV/authd/examplebroker/broker.go:846 +0x376 github.com/ubuntu/authd/examplebroker.(*Bus).UserPreCheck() /DEV/authd/examplebroker/dbus.go:131 +0xf5 runtime.call32() /home/marco/go/pkg/mod/golang.org/[email protected]/src/runtime/asm_amd64.s:776 +0x42 reflect.Value.Call() /home/marco/go/pkg/mod/golang.org/[email protected]/src/reflect/value.go:365 +0x144 github.com/godbus/dbus/v5.exportedMethod.Call() /home/marco/go/pkg/mod/github.com/godbus/dbus/[email protected]/default_handler.go:128 +0x384 github.com/godbus/dbus/v5.(*exportedMethod).Call() <autogenerated>:1 +0x127 github.com/godbus/dbus/v5.(*Conn).handleCall() /home/marco/go/pkg/mod/github.com/godbus/dbus/[email protected]/export.go:193 +0x1084 github.com/godbus/dbus/v5.(*Conn).inWorker.gowrap1() /home/marco/go/pkg/mod/github.com/godbus/dbus/[email protected]/conn.go:435 +0x50 Goroutine 3408 (running) created at: github.com/godbus/dbus/v5.(*Conn).inWorker() /home/marco/go/pkg/mod/github.com/godbus/dbus/[email protected]/conn.go:435 +0x7aa github.com/godbus/dbus/v5.(*Conn).Auth.gowrap1() /home/marco/go/pkg/mod/github.com/godbus/dbus/[email protected]/auth.go:118 +0x38 Goroutine 3405 (finished) created at: github.com/godbus/dbus/v5.(*Conn).inWorker() /home/marco/go/pkg/mod/github.com/godbus/dbus/[email protected]/conn.go:435 +0x7aa github.com/godbus/dbus/v5.(*Conn).Auth.gowrap1() /home/marco/go/pkg/mod/github.com/godbus/dbus/[email protected]/auth.go:118 +0x38
It's easier to read when it's not merged with the rest of the output.
If we're using a single shared daemon for all the tests the t* instance may refer to another test that has been over, so don't use it once the daemon has been started or we may end up in a race when logging. In such case we use then other simpler logger and error checking functions.
And don't initialize them twice (the Init function was already called) for them given that they're focusable entries too.
There's no much need to limit the tests duration, as they may just be slow in CI or in builder will still work as expected
Adapt timeout to the running environment
Env variables should always override arguments as golden rule, so let's do it.
While we're trying to do this smartly checking the build information for the binaries we're testing, this doesn't work particularly well in some test binaries (debug.BuildInfo Settings are inconsistently exposed). So, let's just be consitent in CI, ensuring that wait times are multiplied as expected.
Be consistent with the model interface and never use pointer receiver functions since they're not needed in the model at all
The basic idea in the bubbletea model is that the Update function changes the model and returns a copy of it, now we were not doing it in the authentication model and in some authenticationComponent implementations and this was leading to modifying some values in a racy way, and indeed bubbletea was not ready for that as per the issue [1]. So, be consistent and don't use pointer receivers for Update functions ever, so that we don't end up modifying the current model but rather a copy of it. As it's expected to happen. This required some extra love in newpasswordmodel because there we were keeping track of the password entries pointers in a list and this list now requires to be updated whenever one of the password entry that it references has been udpated or its content won't never be exposed. [1] charmbracelet/bubbletea#909
Like on the previous change we should not use pointer receiver for the update functions, but rather act on a copy that we eventually return. So that when the update is done the changes are applied. This was not the case before and it was triggering some bubble tea races so switch to use non-pointer receiver for the Update function. This has the implication that the final exit status is never updated on the model that gets initialized in the pam module, but we can now handle this case by setting exit status on the program model just before quitting.
We used to return the exit status from the model, but that was leading to making the quit phase more complex since the model is not meant to be updated, so move it to the caller instead by making it passing a pointer that indeed gets updated when required
There's no need to use pointers in most of cases, so avoid it. We can also avoid atomic logic here given that there's no risk of races
Now that the models gets updated in the right way we should have not anymore bubbletea cursor related races so drop the code that was ignoring them
These two phases should be split so that the creation sets up the internal module data while the Init only sends commands without touching the actual model. As per this is not anymore pointer-receiver, implementing a tea.Model
We used a pointer UIModel as the program model, but this was not how a tea.Model was meant to be, as the whole struct was actually expected for such purpose. This was due to the fact that Init() had an input receiver, but again this was not the way it should be, since Init is not meant to initialize the content but rather to return messages to do that. So cleanup the whole UIModel to split the creation with the init phases
Now that the model can be created with a specific function and that it's properly implementing the tea.Model interface we can just expose it externally as such, since there's no point to mess with its internal structure from the module side. This allows us to also not to export all the private variables. A required change due to this was that now we need to rely again on Update to stop the GDM conversations, but this is just cleaner as it avoids again messing with the model in an unexpected way
And other functions where we don't write to the models
During Clear we were focusing again the focus widget, however the Focus call may return a command that should still be delivered and not ignored So do it.
3v1n0
force-pushed
the
races-handling-fixes
branch
from
5d936e8 to
dd3f797
Compare
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.
See commits for details, but basically lots of cleanups to ensure that the races are properly monitored and that the artifacts saved in CI, then I decided to tackle the root cause of charmbracelet/bubbletea#909 that was mostly due to the fact that we were, in various cases, updating the models with pointer receivers, instead of relying on the returned and modified value.
Coming with various cleanups.
UDENG-6474