mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(agentgit): close subscribe-before-listen race in handleWatch (#22747)
## Problem `TestE2E_WriteFileTriggersGitWatch` and `TestE2E_SubagentAncestorWatch` flake intermittently in `test-go-race-pg` with: ``` agentgit_test.go:1271: timed out waiting for server message ``` ## Root Cause In `handleWatch()`, `GetPaths(chatID)` was called **before** `Subscribe(chatID)` on the PathStore. If `AddPaths()` fired between those two calls: 1. `GetPaths()` returned empty (paths not added yet). 2. `AddPaths()` stored the paths and called `notifySubscribers()` — but the subscription channel didn't exist yet, so the notification was a no-op. 3. `Subscribe()` created the channel, but the notification was already lost. 4. The handler never scanned, and the mock clock never advanced the 30s fallback ticker → timeout. Both failing tests connect the WebSocket with an empty PathStore and immediately call `AddPaths()` from the test goroutine, making them vulnerable to this scheduling interleaving. ## Fix Swap the order: call `Subscribe()` first, then `GetPaths()`. This guarantees: | `AddPaths` fires... | `Subscribe` sees it? | `GetPaths` sees it? | Outcome | |---|---|---|---| | Before `Subscribe` | No | **Yes** | Picked up by `GetPaths` | | Between the two calls | **Yes** (queued) | **Yes** | Redundant but safe (delta dedupes) | | After `GetPaths` | **Yes** | No | Goroutine handles it | No window exists where both miss it. Verified with 10,000 iterations (`-race -count=5000`) — zero failures. Fixes coder/internal#1389
This commit is contained in:
@@ -85,15 +85,21 @@ func (a *API) handleWatch(rw http.ResponseWriter, r *http.Request) {
|
||||
if chatIDStr != "" && a.pathStore != nil {
|
||||
chatID, parseErr := uuid.Parse(chatIDStr)
|
||||
if parseErr == nil {
|
||||
// Subscribe to future path updates BEFORE reading
|
||||
// existing paths. This ordering guarantees no
|
||||
// notification from AddPaths is lost: any call that
|
||||
// lands before Subscribe is picked up by GetPaths
|
||||
// below, and any call after Subscribe delivers a
|
||||
// notification on the channel.
|
||||
notifyCh, unsubscribe := a.pathStore.Subscribe(chatID)
|
||||
defer unsubscribe()
|
||||
|
||||
// Load any paths that are already tracked for this chat.
|
||||
existingPaths := a.pathStore.GetPaths(chatID)
|
||||
if len(existingPaths) > 0 {
|
||||
handler.Subscribe(existingPaths)
|
||||
handler.RequestScan()
|
||||
}
|
||||
// Subscribe to future path updates.
|
||||
notifyCh, unsubscribe := a.pathStore.Subscribe(chatID)
|
||||
defer unsubscribe()
|
||||
|
||||
go func() {
|
||||
for {
|
||||
|
||||
Reference in New Issue
Block a user