fix(coderd/x/chatd): resolve inflight race#26460
Conversation
| } | ||
|
|
||
| func (p *Server) clearLastTurnSummaryAsync( | ||
| ctx context.Context, | ||
| chat database.Chat, | ||
| logger slog.Logger, | ||
| ) { | ||
| // This helper runs during processChat cleanup, while processChat is | ||
| // still counted in p.inflight. Do not take inflightMu here because |
There was a problem hiding this comment.
processChat does not exist anymore, so I don't think the scenario described here still applies. But even if it did apply, introducing a possible data race does not seem like the right fix to address the problem.
hugodutka
force-pushed
the
hugodutka/inflight-race
branch
from
08f2a8a to
c7a3c78
Compare
| if server.chatWorker == nil { | ||
| return | ||
| if server.chatWorker != nil { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) |
There was a problem hiding this comment.
Can we instead allow callers to pass in a parent context instead of hard-coding a timeout here?
There was a problem hiding this comment.
We can, but I'd rather keep this PR scoped to the issue at hand. Would you like me to create a Linear issue for this?
There was a problem hiding this comment.
I figure if it's a real problem then it will show up as a flake.
| // new goroutines (via inflight.Add) concurrently with Wait, | ||
| // which would violate sync.WaitGroup's contract. | ||
| func (p *Server) goInflight(f func()) error { | ||
| if p.inflightClosed.Load() { |
There was a problem hiding this comment.
We could just use a RW mutex and do a RLock instead of double check (mutex would protect inflightClosed, not Go per-se, hence read would suffice here.)
There was a problem hiding this comment.
I specifically used an atomic so goInflight can exit early if inflightClosed is true. Otherwise an active drainInflight call - which can be pretty long - would block goInflight calls. I guess you could do that with an RWMutex too if drainInflight also obtained a read lock, but I'm not convinced it's simpler than what we have now.
3 participants
Using
WaitGroup.Gomust be synchronized withWaitGroup.Waitaccording to go docs:There were a couple of places in chatd that violated this principle. This was caught as a data race in coder/internal#1599. This PR ensures that all functions that spawn inflight goroutines synchronize with each other.
I also noticed that inflight goroutines may be spawned after the server is closed, which was surprising and looked like a bug. This PR therefore also introduces a mechanism that disallows spawning inflight goroutines after the server is closed, and ensures that any code that tries doing it logs an error.
Closes coder/internal#1599.