kolide / launcher

Osquery launcher, autoupdater, and packager
https://kolide.com/launcher
Other
500 stars 99 forks source link

Remove incorrect use of TryLock for initial delay state in TUF autoupdater #1676

Closed RebeccaMahany closed 2 months ago

RebeccaMahany commented 2 months ago

I didn't add TryLock correctly here before -- this had the consequence that a request to pin a version or autoupdate immediately outside of the initial delay would then block subsequent requests to pin a version or autoupdate immediately.

In this PR, I've pulled it out in favor of a timestamp; we set the timestamp in NewTufAutoupdater rather than in Execute to avoid data races in tests (or a mutex added only to address data races in tests).

RebeccaMahany commented 2 months ago

@directionless I like that idea too, but setting something like ta.initialDelayEnd = time.Now().Add(ta.knapsack.AutoupdateInitialDelay()) at the beginning of Execute will still require a mutex to avoid data races during the tests 😕 -- that's the only reason I preserved the mutex.

directionless commented 2 months ago

@directionless I like that idea too, but setting something like ta.initialDelayEnd = time.Now().Add(ta.knapsack.AutoupdateInitialDelay()) at the beginning of Execute will still require a mutex to avoid data races during the tests 😕 -- that's the only reason I preserved the mutex.

Hrm... If we set it at create time, we could skip the mutex... Might not be worth it. Fun thought exercise