chore: fix race in cron close behavior (TestAgent_WriteVSCodeConfigs) #11243
Conversation
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
This fix still has a race condition. I do not see a clean solution without modifying the cron libary. The cron library uses a boolean to indicate running, and that boolean needs to be set to "true" before we call "Close()". Or "Close()" should prevent "Run()" from doing anything. In either case, this solves the issue for a niche unit test bug in which the test finishes, calling Close(), before there was an oppertunity to start the go routine. It probably isn't worth a lot of time investment, and this fix will suffice
Emyrk
commented
Dec 15, 2023
Comment on lines
+56
to
+64
| // TestCronClose exists because cron.Run() can happen after cron.Close(). | ||
| // If this happens, there used to be a deadlock. | ||
| func TestCronClose(t *testing.T) { | ||
| t.Parallel() | ||
| runner := agentscripts.New(agentscripts.Options{}) | ||
| runner.StartCron() | ||
| require.NoError(t, runner.Close(), "close runner") | ||
| } | ||
|
|
There was a problem hiding this comment.
Hard to know for sure if this was the bug... I was able to reproduce this way, still feels like it might be more rare intertwined into some setup code that has more chances for the order to be "correct".
johnstcn
approved these changes
Dec 18, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Closes #11226
Problem
The problem is
cmdCloseWaitnever closes, so there is an open agent go routine that will not close. In the unit test in Question,cron.Run()is the only routine started. So I figured it is likely the routine left open. You can exercise the test locally withgo test -run TestAgent_WriteVSCodeConfigs --timeout=8s -count=20. Or I wrote a new test that always exercises what I think the bug is.What is happening
The cron package we are using allows
Run()to be called afterClose(). This essentially allows restarting the cron daemon, which is interesting.Run()is called on a go routine. So in our unit tests, it is possible theClose()gets called first, thenRun()is called. There exists a time period between these 2 lines of code:coder/agent/agentscripts/agentscripts.go
Lines 330 to 331 in 9b343c4
If
Run()is called between there, thecmdCloseWaitwill never terminate.A fix
If the
cronCtxis cancelled, we just never callcron.Run(). The ctx is always cancelled before the stop, so this should be pretty good. Ideally we implement this inside the cron package and hook on theirrunningflag. However this should suffice.