From 703629f5e9d860414edae95876f6fd79f5c67af3 Mon Sep 17 00:00:00 2001
From: Hugo Dutka <hugo@coder.com>
Date: Sat, 7 Mar 2026 15:36:43 +0100
Subject: [PATCH] fix(agentgit): close subscribe-before-listen race in
 handleWatch (#22747)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

## 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
---
 agent/agentgit/api.go | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/agent/agentgit/api.go b/agent/agentgit/api.go
index 6666112028..80513bce0d 100644
--- a/agent/agentgit/api.go
+++ b/agent/agentgit/api.go
@@ -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 {