fix: serialize updateEntitlements() #14974
Conversation
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @spikecurtis and the rest of your teammates on |
|
Just so I understand, and jotting notes as I break for a minute. I am going to play with this after some morning calls.
Is the issue that in the multiple calls in unit tests, the earlier calls do not have a license. This earlier call happens in parallel to the later call (with license). If the later one finishes first, then the earlier call overwrites it back to no license. The fix, is to force serialization such that the order of calls to This looks to be a good solution. The test does not exercise the bug when I remove the channel, but I only briefly looked at it atm. An aside. when I extracted this logic out into this package, I was not really happy with the API it exposed. But it was already used throughout the codebase, and refactoring it everywhere felt like a large lift. Since we only have 1 entitlement per coderd, it could probably be it's own go routine and take feature sets to merge into the existing. Ensuring everything is done in order if multiple threads are working with it. Order would matter still though, as there is no time information to know which |
Yeah, exactly.
Hard to hit the bug consistently, since it's a race over which one writes first. |
There was a problem hiding this comment.
Hard to hit the bug consistently, since it's a race over which one writes first.
I figured it would be.
The entitlements package I refactored out because I had to access it from the idpsync package, and there is an import loop if entitlements is kept in coderd.
I think if done from scratch, the update code would look a lot different.
Regardless, I like this serialized approach. Modify can still get around this, but it's really useful for unit tests.
fixes #14961
Adding the license and updating entitlements is flaky, especially at the start of our
coderdenttesting because, while the actual modifications to theentitlements.Setwere threadsafe, we could have multiple goroutines reading from the database and writing to the set, so we could end up writing stale data.This enforces serialization on updates, so that if you modify the database and kick off an update, you know the state of the
Setis at least as fresh as your database update.