Looking at the commit history, it seems this was introduced by @ibetitsmike. Do you have any thoughts on how to proceed based on @johnstcn comment?

I'm just looking to fix the test race, but happy to hand this over to someone else if another approach is preferred.

@mafredri @johnstcn generally the only scenario in which requestLogger is nil is if the request circumvents our API Logger middleware. It's responsible for attaching RequestLogger to context.

So the only scenario where it fails is if we call the method that tries to log, but doesn't do it through API. An example that was failing for me was provisioner_job_tests which I fixed here: https://github.com/coder/coder/pull/17304/files#diff-a0b103dd31ee53f52c43564220dbada29c0d9147e7210b27cb42e375b08fd947

If we experience flakes I'd try to understand why a given test path fails to run this line: https://github.com/coder/coder/blob/5f7e5d709787ca4e6ec287197f112d56d0eae2c8/coderd/httpmw/loggermw/logger.go#L44C11-L44C28

RequestLogger is useful to debug issues with long lived connections, so it's not critical to have that information 100% of the time, but @deansheather was the one that initially asked me to remove those nil checks (I had them in the first implementation).

Here's a repro:

go test ./coderd -run=Test_logFollower_completeBeforeFollow -v -race -count=10

There was another test as well that failed, also log follower. It's an easy miss to include the request logger when writing a test of this type, so I'm in favor of nil checks tbh as this happens too late and is racy otherwise.

What if we added another middleware only in tests that immediately fails the test if the request logging middleware is not present when any request comes in? That should allow surfacing this issue quickly.

This one seems to be languishing and I'm getting pinged daily for review.
Thus far I've deferred since others have reviewed.
If you need another voice, I'm happy to add it - but either way let's come to a conclusions on this if possible.

I don't really have the bandwidth to refactor the implementation or any tests that touch upon this code path. The problem here is that components are being tested with a http test server and there's nothing enforcing or informing the user/test writer to add certain middleware, leading to occasional flakes.

There is also pre-existing precedent for the != nil check so I don't see why we'd have it in some places but not in others. Either case, we can either merge or close this, either works for me.

Edit: Meanwhile @dannykopping, I've released you from your agony and dropped you as a reviewer. 😜