From 0c6fe1f378a052ca5e24b38bec8bf66d1d4c54cd Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Fri, 12 Jun 2026 13:07:57 +0000 Subject: [PATCH 1/2] deferred review threads 26109 plan --- deferred-review-threads-pr26109.md | 152 +++++++++++++++++++++++++++++ 1 file changed, 152 insertions(+) create mode 100644 deferred-review-threads-pr26109.md diff --git a/deferred-review-threads-pr26109.md b/deferred-review-threads-pr26109.md new file mode 100644 index 0000000000000..02063544bfbee --- /dev/null +++ b/deferred-review-threads-pr26109.md @@ -0,0 +1,152 @@ +# Deferred Review Threads from PR #26109 + +Source: https://github.com/coder/coder/pull/26109 + +This file lists every review comment thread that hugodutka marked as deferred. It serves as a template for a plan to address these issues. Each entry describes what the thread was about, with a link to the original discussion. + +## Threads + +### coderd/database/dbpurge/dbpurge.go + +1. [DONE] **Stale comment removal** (line 61) https://github.com/coder/coder/pull/26109#discussion_r3379072397 mafredri flagged an LLM-generated comment/reasoning that is no longer relevant and should be removed if unused. + +2. [DONE] **Error reuse across long distance** (line 258) https://github.com/coder/coder/pull/26109#discussion_r3379093655 The code reuses an `err` variable far from where it was set, which is unusual and clever in a bad way. A boolean flag would be clearer than reusing the error value. + +3. [SKIPPED] **InTx assertion support in database/Store** (line 340) https://github.com/coder/coder/pull/26109#discussion_r3379104920 Side note/future work: add support to the database Store to assert that code is running inside a transaction (InTx) and error out if not. + +### coderd/x/chatd/chatdebug/service.go + +4. [DONE] **Undocumented silent failure** (line 543) https://github.com/coder/coder/pull/26109#discussion_r3379151284 Behavior needs to either be documented on the method or return an error. Same applies to `TouchRun`. + +5. [DONE] **Rename to maybeResetTicker** (line 551) https://github.com/coder/coder/pull/26109#discussion_r3379164243 Naming suggestion: rename the function to `maybeResetTicker`. + +### coderd/x/chatd/messagepartbuffer/message_part_buffer.go + +6. **Extract getEpisode helper** (line 192) https://github.com/coder/coder/pull/26109#discussion_r3379915812 Extract a helper method (e.g. `getEpisode`) that handles the not-found check, instead of repeating it. + +7. **Package needs much more documentation** (file-level) https://github.com/coder/coder/pull/26109#discussion_r3379988015 The package is hard to review because intent is not documented. Add package and method documentation explaining why the code does what it does. + +8. **Extract episode finalization helper** (line 300) https://github.com/coder/coder/pull/26109#discussion_r3380010948 Repeated logic in several places could be extracted into a method like `episode.markCreated()` or `episode.finalize()`. + +9. **Document channel buffering decisions** (line 262) https://github.com/coder/coder/pull/26109#discussion_r3380029684 Add comments explaining why a channel is buffered, and why the unbuffered channels are safe to be unbuffered. + +### coderd/x/chatd/messagepartbuffer/message_part_buffer_test.go + +10. [DONE] **Add goleak (HIGH PRIORITY)** (file-level) https://github.com/coder/coder/pull/26109#discussion_r3380039964 mafredri suspects goleak would surface straggling goroutines since teardown appears to be "eventual". hugodutka deferred with high priority: add goleak to all packages affected by the refactor, and consider whether code paths should wait for teardown. + +### coderd/x/chatd/auto_archive.go + +11. **Archival loop ticker behavior** (line 42) https://github.com/coder/coder/pull/26109#discussion_r3380197310 A slow `archiveOnce` or congested DB could create a constant archival loop. Suggested restructure: archiveOnce -> createTicker -> select -> archiveOnce + ticker.Reset(interval). + +12. **Document the UTC 00:00 cutoff choice** (line 68) https://github.com/coder/coder/pull/26109#discussion_r3380219922 Add docs explaining why UTC midnight minus N days was selected as the archival cutoff. + +13. **Postgres trigger for updated_at** (line 85) https://github.com/coder/coder/pull/26109#discussion_r3380232630 Concern that an archived chat could be wiped by dbpurge almost immediately because `updated_at` reflected last chat activity. hugodutka confirmed every chat state transition (including archival) already bumps `updated_at` via `UpdateChatExecutionState`, so nothing is broken, but a Postgres trigger that auto-bumps `updated_at` would be more robust. Deferred. + +### coderd/x/chatd/chatd.go + +14. **Use atomic value** (line 2429) https://github.com/coder/coder/pull/26109#discussion_r3380251082 An atomic value seems more appropriate than the current approach. + +15. **Make newChatWorker/withDefaults dependencies explicit** (line 3591) https://github.com/coder/coder/pull/26109#discussion_r3380270527 Make the implicit dependencies explicit (e.g. `withDefaults(db, ...)`) so the runtime panic for missing options is unnecessary. + +16. **Workspace context gathering refactor** (line 5042) https://github.com/coder/coder/pull/26109#discussion_r3386806941 A comment warns about easy misuse of `appendRootChatTools` regarding workspace context. Suggested renaming it to something like `appendRootChatToolsWithoutWorkspaceContext` or guarding against the mistake in the implementation. hugodutka: workspace context gathering in general should be refactored. + +### coderd/x/chatd/generation.go + +17. **Runtime checks for required options** (line 336) https://github.com/coder/coder/pull/26109#discussion_r3380311853 Question whether all the runtime checks for required options are needed; make dependencies explicit instead. + +18. **Too many juggled variables** (line 370) https://github.com/coder/coder/pull/26109#discussion_r3387161874 Too many variables in scope (`locked`, etc.) remain referenceable after they stop being useful. Prefer a single name like `chat` and override when appropriate. + +19. **Error handling structure prevents misuse** (line 482) https://github.com/coder/coder/pull/26109#discussion_r3387191468 Suggestion to restructure error handling so all error cases are handled in one `if err != nil` block (sql.ErrNoRows -> errTaskExpectedExit, else wrap), preferring structures that prevent misuse. + +20. **Unnamed return signature hard to decipher** (line 817) https://github.com/coder/coder/pull/26109#discussion_r3387251382 A function returns many unnamed values. Use named returns at minimum, or return a struct. + +21. **Unify fence verification query** (line 828) https://github.com/coder/coder/pull/26109#discussion_r3387288234 All call sites of fence verification require the running state and history; this could be unified via something like `tx.GetChatForTask`. + +22. **Machine update failure does not record outcome** (line 1049) https://github.com/coder/coder/pull/26109#discussion_r3387544273 If the machine update fails, no outcome is recorded, possibly leaving untracked work in chatdebug. hugodutka: chatdebug should be removed. + +### coderd/x/chatd/generation_preparer.go + +23. **Magic value should be a documented const** (line 100) https://github.com/coder/coder/pull/26109#discussion_r3387640179 + +24. **Reuse earlier err variable** (line 124) https://github.com/coder/coder/pull/26109#discussion_r3387645148 Nit: could just use the `err` defined earlier. + +25. **Dangerous cleanup pattern** (line 162) https://github.com/coder/coder/pull/26109#discussion_r3387675842 The function is dangerous to edit. Suggested a named `err` return coupled with a `defer func() { if err != nil { cleanup() } }()` so cleanup happens on any error return. + +### coderd/x/chatd/runner.go + +26. **Multiple calls should be an error state** (line 77) https://github.com/coder/coder/pull/26109#discussion_r3387720344 Allowing multiple calls feels like it should be an error state instead. + +### coderd/x/chatd/runner_manager.go + +27. **Noise when ctx cancelled** (line 413) https://github.com/coder/coder/pull/26109#discussion_r3380581402 Skip logging this error when the context is cancelled. + +28. **Potential wg.Wait/mu.Lock deadlock (concurrency)** (line 301) https://github.com/coder/coder/pull/26109#discussion_r3380592305 Caution about potential deadlocks between `m.wg.Wait` and `m.mu.Lock`. hugodutka: ensure this is corrected from a concurrency perspective. + +29. **Skip logging context canceled errors** (line 458) https://github.com/coder/coder/pull/26109#discussion_r3387355724 Same as thread 27, applied to another log site. + +30. **Document stateCh buffering semantics** (line 180) https://github.com/coder/coder/pull/26109#discussion_r3387788957 A target whose stateCh is full gets no state update and must process all previous states. Add comments explaining why this is fine: why a gap at the tail is preferred over the head, expectations around the default 64 buffer size, etc. + +### coderd/x/chatd/testhooks.go + +31. **Hard-coded timeout** (line 19) https://github.com/coder/coder/pull/26109#discussion_r3382365135 Accept a `context.Context` parameter instead of a hard-coded timeout. + +### coderd/x/chatd/tasks.go and tasks_test.go + +32. **Extract side effects to an interface** (tasks.go line 124) https://github.com/coder/coder/pull/26109#discussion_r3382554277 Extracting side-effecting dependencies to an interface would make the seam clearer and easier to mock or spy on in tests. + +33. **taskStarter test spy / gomock** (tasks_test.go line 1040) https://github.com/coder/coder/pull/26109#discussion_r3382564035 Related to thread 32: `taskStarter` has many side-effecting dependencies. An interface would allow using gomock for assertions. + +34. **Required options as newTaskStarter args** (tasks.go line 141) https://github.com/coder/coder/pull/26109#discussion_r3387867697 Required options should be explicit arguments of `newTaskStarter`. + +35. **State invariant should be enforced by the machine** (tasks.go line 434) https://github.com/coder/coder/pull/26109#discussion_r3387919402 An invariant is verified in each `Update` call; it should instead be enforced by the state machine itself. + +### coderd/x/chatd/worker.go + +36. **Rename ctx to parentCtx** (line 49) https://github.com/coder/coder/pull/26109#discussion_r3387954876 Rename the outer ctx to `parentCtx` and use `ctx` inline to avoid bugs where the wrong context is referenced. + +37. **Magic number** (line 120) https://github.com/coder/coder/pull/26109#discussion_r3387973304 Replace magic number with a documented const. + +### coderd/x/chatd/quickgen.go + +38. **Separate timeout bound** (line 171) https://github.com/coder/coder/pull/26109#discussion_r3387365392 Question whether this operation should still be bounded by a separate timeout. + +## Todos + +- [x] 1. dbpurge.go: remove stale LLM-generated comment (r3379072397) +- [x] 2. dbpurge.go: replace distant err reuse with a flag (r3379093655) +- [ ] 3. database/Store: support asserting InTx (r3379104920) +- [x] 4. chatdebug/service.go: document or return error, incl. TouchRun (r3379151284) +- [x] 5. chatdebug/service.go: rename to maybeResetTicker (r3379164243) +- [ ] 6. messagepartbuffer: extract getEpisode helper (r3379915812) +- [ ] 7. messagepartbuffer: add package and method documentation (r3379988015) +- [ ] 8. messagepartbuffer: extract episode.markCreated/finalize helper (r3380010948) +- [ ] 9. messagepartbuffer: document channel buffering decisions (r3380029684) +- [x] 10. HIGH PRIORITY: add goleak to all packages affected by the refactor; fix straggling goroutines (r3380039964) +- [ ] 11. auto_archive.go: restructure archival loop ticker (r3380197310) +- [ ] 12. auto_archive.go: document UTC 00:00 cutoff choice (r3380219922) +- [ ] 13. auto_archive.go/DB: add Postgres trigger to bump updated_at (r3380232630) +- [ ] 14. chatd.go: use an atomic value (r3380251082) +- [ ] 15. chatd.go: explicit deps for newChatWorker/withDefaults, remove panic (r3380270527) +- [ ] 16. chatd.go: refactor workspace context gathering; rename appendRootChatTools (r3386806941) +- [ ] 17. generation.go: remove runtime checks via explicit required deps (r3380311853) +- [ ] 18. generation.go: reduce juggled variables around locked/chat (r3387161874) +- [ ] 19. generation.go: restructure error handling to prevent misuse (r3387191468) +- [ ] 20. generation.go: named returns or struct for multi-value return (r3387251382) +- [ ] 21. generation.go: unify fence verification via tx.GetChatForTask (r3387288234) +- [ ] 22. generation.go/chatdebug: handle machine update failure outcome; consider removing chatdebug (r3387544273) +- [ ] 23. generation_preparer.go: move magic value to documented const (r3387640179) +- [ ] 24. generation_preparer.go: reuse earlier err variable (r3387645148) +- [ ] 25. generation_preparer.go: named err return + deferred cleanup on error (r3387675842) +- [ ] 26. runner.go: treat multiple calls as an error state (r3387720344) +- [ ] 27. runner_manager.go: skip logging on ctx cancellation, line 413 (r3380581402) +- [ ] 28. runner_manager.go: fix wg.Wait/mu.Lock concurrency concern (r3380592305) +- [ ] 29. runner_manager.go: skip logging context canceled errors, line 458 (r3387355724) +- [ ] 30. runner_manager.go: document stateCh buffering semantics (r3387788957) +- [ ] 31. testhooks.go: accept context.Context instead of hard-coded timeout (r3382365135) +- [ ] 32. tasks.go: extract side-effecting deps to interface (r3382554277) +- [ ] 33. tasks_test.go: use interface and gomock for taskStarter spy (r3382564035) +- [ ] 34. tasks.go: required options as newTaskStarter args (r3387867697) +- [ ] 35. tasks.go: enforce invariant in state machine, not each Update (r3387919402) +- [ ] 36. worker.go: rename ctx to parentCtx (r3387954876) +- [ ] 37. worker.go: replace magic number with documented const (r3387973304) +- [ ] 38. quickgen.go: consider separate timeout bound (r3387365392) From 063179e847827f5a67bc7235056ab03092f667a5 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Thu, 18 Jun 2026 10:17:56 +0000 Subject: [PATCH 2/2] test: accept context in chatd idle test hook --- coderd/coderdtest/chat.go | 2 +- coderd/x/chatd/chatd_test.go | 24 +++++++++++------------ coderd/x/chatd/recording_internal_test.go | 6 +++--- coderd/x/chatd/subagent_internal_test.go | 6 +++--- coderd/x/chatd/testhooks.go | 14 ++++++------- deferred-review-threads-pr26109.md | 6 ++++-- 6 files changed, 29 insertions(+), 29 deletions(-) diff --git a/coderd/coderdtest/chat.go b/coderd/coderdtest/chat.go index bf460a5ff0c20..0fb9c28966a95 100644 --- a/coderd/coderdtest/chat.go +++ b/coderd/coderdtest/chat.go @@ -98,7 +98,7 @@ func WaitForChatSettled( server := api.ChatDaemonForTest() require.NotNil(t, server) - chatd.WaitUntilIdleForTest(server) + require.NoError(t, chatd.WaitUntilIdleForTest(ctx, server)) chat, err := getChatByIDAsSystem(ctx, api.Database, chatID) require.NoError(t, err) diff --git a/coderd/x/chatd/chatd_test.go b/coderd/x/chatd/chatd_test.go index 03ff68b34a1d2..1d2ddfb4c6696 100644 --- a/coderd/x/chatd/chatd_test.go +++ b/coderd/x/chatd/chatd_test.go @@ -2206,7 +2206,7 @@ func TestAutoPromoteQueuedMessagesPreservesPerTurnModelOrder(t *testing.T) { testutil.TryReceive(ctx, t, secondRunStarted) testutil.TryReceive(ctx, t, thirdRunStarted) require.GreaterOrEqual(t, requestCount.Load(), int32(3)) - chatd.WaitUntilIdleForTest(server) + require.NoError(t, chatd.WaitUntilIdleForTest(ctx, server)) queuedMessages, err := db.GetChatQueuedMessages(ctx, chat.ID) require.NoError(t, err) @@ -2397,7 +2397,7 @@ func TestInterruptAutoPromotionIgnoresLaterUsageLimitIncrease(t *testing.T) { require.GreaterOrEqual(t, requestCount.Load(), int32(3)) close(allowThirdRequestFinish) - chatd.WaitUntilIdleForTest(server) + require.NoError(t, chatd.WaitUntilIdleForTest(ctx, server)) queued, err := db.GetChatQueuedMessages(ctx, chat.ID) require.NoError(t, err) @@ -2646,7 +2646,7 @@ func TestEditMessageDebugCleanupDeletesPreEditRuns(t *testing.T) { }) require.NoError(t, err) - chatd.WaitUntilIdleForTest(replica) + require.NoError(t, chatd.WaitUntilIdleForTest(ctx, replica)) // ErrNoRows on staleRun proves the fast-retry path DELETED the // row: FinalizeStale (the only other debug-row writer on the @@ -2737,7 +2737,7 @@ func TestEditMessageDebugCleanupPreservesRecentRuns(t *testing.T) { }) require.NoError(t, err) - chatd.WaitUntilIdleForTest(replica) + require.NoError(t, chatd.WaitUntilIdleForTest(ctx, replica)) remaining, err := db.GetChatDebugRunByID(ctx, recentRun.ID) require.NoError(t, err, @@ -3323,7 +3323,7 @@ func TestRequiresActionChatPersistsWaitingStatusLabel(t *testing.T) { got.LastTurnSummary.Valid && got.LastTurnSummary.String == "Waiting for user input" }, testutil.IntervalFast) - chatd.WaitUntilIdleForTest(server) + require.NoError(t, chatd.WaitUntilIdleForTest(ctx, server)) require.Equal(t, database.ChatStatusRequiresAction, fromDB.Status, "expected requires_action, got %s (last_error=%q)", @@ -5329,7 +5329,7 @@ func waitForChatProcessed( return c.Status != database.ChatStatusPending && c.Status != database.ChatStatusRunning }, testutil.WaitShort, testutil.IntervalFast) - chatd.WaitUntilIdleForTest(server) + require.NoError(t, chatd.WaitUntilIdleForTest(ctx, server)) } // newTestServer creates a passive server that never calls @@ -8194,7 +8194,7 @@ func TestPassiveServerDoesNotProcess(t *testing.T) { }) require.NoError(t, err) - chatd.WaitUntilIdleForTest(server) + require.NoError(t, chatd.WaitUntilIdleForTest(ctx, server)) // Re-read from DB to catch any unexpected processing. stored, err := db.GetChatByID(ctx, chat.ID) @@ -8806,7 +8806,7 @@ func TestInterruptChatDoesNotSendWebPushNotification(t *testing.T) { } return fromDB.Status == database.ChatStatusWaiting && !fromDB.WorkerID.Valid }, testutil.IntervalFast) - chatd.WaitUntilIdleForTest(server) + require.NoError(t, chatd.WaitUntilIdleForTest(ctx, server)) fromDB, err := db.GetChatByID(ctx, chat.ID) require.NoError(t, err) @@ -9267,7 +9267,7 @@ func TestErroredChatClearsLastTurnSummaryAndSendsWebPush(t *testing.T) { fromDB.Status == database.ChatStatusError && mockPush.dispatchCount.Load() >= 1 }, testutil.IntervalFast) - chatd.WaitUntilIdleForTest(server) + require.NoError(t, chatd.WaitUntilIdleForTest(ctx, server)) fromDB, err := db.GetChatByID(ctx, chat.ID) require.NoError(t, err) @@ -11272,7 +11272,7 @@ func TestCreateChatImmediatelyProcessesNewChat(t *testing.T) { // this receive would time out. testutil.TryReceive(ctx, t, processed) - chatd.WaitUntilIdleForTest(server) + require.NoError(t, chatd.WaitUntilIdleForTest(ctx, server)) // Verify the chat was fully processed. fromDB, err := db.GetChatByID(ctx, chat.ID) @@ -11335,7 +11335,7 @@ func TestSendMessageImmediatelyProcessesWaitingChat(t *testing.T) { // wait for the processing goroutine to finish so the chat // transitions to "waiting" status. testutil.TryReceive(ctx, t, firstProcessed) - chatd.WaitUntilIdleForTest(server) + require.NoError(t, chatd.WaitUntilIdleForTest(ctx, server)) // Now send a follow-up message, which should also be // processed immediately without waiting for the acquire ticker. @@ -11347,7 +11347,7 @@ func TestSendMessageImmediatelyProcessesWaitingChat(t *testing.T) { require.NoError(t, err) testutil.TryReceive(ctx, t, secondProcessed) - chatd.WaitUntilIdleForTest(server) + require.NoError(t, chatd.WaitUntilIdleForTest(ctx, server)) // Both turns processed. Verify the second request reached the LLM. require.GreaterOrEqual(t, requestCount.Load(), int32(2), diff --git a/coderd/x/chatd/recording_internal_test.go b/coderd/x/chatd/recording_internal_test.go index 23d4f2ff2a002..210d723b9fb08 100644 --- a/coderd/x/chatd/recording_internal_test.go +++ b/coderd/x/chatd/recording_internal_test.go @@ -188,7 +188,7 @@ func TestWaitAgentComputerUseRecording(t *testing.T) { // Wait for background processing triggered by CreateChat to // settle before setting up the mock agent connection. - WaitUntilIdleForTest(server) + require.NoError(t, WaitUntilIdleForTest(ctx, server)) // Now wire up the mock agent connection. server.agentConnFn = func(_ context.Context, agentID uuid.UUID) (workspacesdk.AgentConn, func(), error) { @@ -274,7 +274,7 @@ func TestWaitAgentComputerUseRecordingWithThumbnail(t *testing.T) { "parent-recording-thumb", "computer-use-child-thumb", ) - WaitUntilIdleForTest(server) + require.NoError(t, WaitUntilIdleForTest(ctx, server)) server.agentConnFn = func(_ context.Context, agentID uuid.UUID) (workspacesdk.AgentConn, func(), error) { require.Equal(t, agent.ID, agentID) @@ -368,7 +368,7 @@ func TestWaitAgentNonComputerUseNoRecording(t *testing.T) { // Wait for background processing triggered by CreateChat to // settle before setting up the mock agent connection. - WaitUntilIdleForTest(server) + require.NoError(t, WaitUntilIdleForTest(ctx, server)) // Wire up the mock agent connection. The mock has zero // expectations — gomock will fail if StartDesktopRecording diff --git a/coderd/x/chatd/subagent_internal_test.go b/coderd/x/chatd/subagent_internal_test.go index bb02bf66cd954..d8b37fa39839e 100644 --- a/coderd/x/chatd/subagent_internal_test.go +++ b/coderd/x/chatd/subagent_internal_test.go @@ -3315,7 +3315,7 @@ func TestWaitAgentDoesNotRelayRegularSubagentAttachments(t *testing.T) { server := newInternalTestServer(t, db, ps, chatprovider.ProviderAPIKeys{}) parent, child := createParentChildChats(ctx, t, server, user, org, model) - WaitUntilIdleForTest(server) + require.NoError(t, WaitUntilIdleForTest(ctx, server)) insertedFile := insertLinkedChatFile( ctx, @@ -3503,7 +3503,7 @@ func TestAwaitSubagentCompletion(t *testing.T) { // signalWake from CreateChat may trigger immediate processing. // Wait for it to settle, then reset chats to the state we need. - WaitUntilIdleForTest(server) + require.NoError(t, WaitUntilIdleForTest(ctx, server)) setChatStatus(ctx, t, db, parent.ID, database.ChatStatusRunning, "") setChatStatus(ctx, t, db, child.ID, database.ChatStatusRunning, "") @@ -3582,7 +3582,7 @@ func TestAwaitSubagentCompletion(t *testing.T) { // This case should return immediately, so use the shared // real-clock server instead of a mock clock. - WaitUntilIdleForTest(server) + require.NoError(t, WaitUntilIdleForTest(ctx, server)) setChatStatus(ctx, t, db, child.ID, database.ChatStatusWaiting, "") gotChat, report, err := server.awaitSubagentCompletion( diff --git a/coderd/x/chatd/testhooks.go b/coderd/x/chatd/testhooks.go index 4598bbeec60cf..9e62ad9a19fc4 100644 --- a/coderd/x/chatd/testhooks.go +++ b/coderd/x/chatd/testhooks.go @@ -1,19 +1,17 @@ package chatd -import ( - "context" - "time" -) +import "context" // WaitUntilIdleForTest waits for background chat work tracked by the server to // finish without shutting the server down. Tests use this to assert final // database state only after asynchronous chat processing has completed. // Close waits for the same tracked work, but also stops the server. -func WaitUntilIdleForTest(server *Server) { +func WaitUntilIdleForTest(ctx context.Context, server *Server) error { if server.chatWorker != nil { - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - _ = server.chatWorker.WaitIdle(ctx) + if err := server.chatWorker.WaitIdle(ctx); err != nil { + return err + } } server.drainInflight() + return nil } diff --git a/deferred-review-threads-pr26109.md b/deferred-review-threads-pr26109.md index 02063544bfbee..87a7138e8cc54 100644 --- a/deferred-review-threads-pr26109.md +++ b/deferred-review-threads-pr26109.md @@ -1,5 +1,7 @@ # Deferred Review Threads from PR #26109 + + Source: https://github.com/coder/coder/pull/26109 This file lists every review comment thread that hugodutka marked as deferred. It serves as a template for a plan to address these issues. Each entry describes what the thread was about, with a link to the original discussion. @@ -88,7 +90,7 @@ This file lists every review comment thread that hugodutka marked as deferred. I ### coderd/x/chatd/testhooks.go -31. **Hard-coded timeout** (line 19) https://github.com/coder/coder/pull/26109#discussion_r3382365135 Accept a `context.Context` parameter instead of a hard-coded timeout. +31. [DONE] **Hard-coded timeout** (line 19) https://github.com/coder/coder/pull/26109#discussion_r3382365135 Accept a `context.Context` parameter instead of a hard-coded timeout. ### coderd/x/chatd/tasks.go and tasks_test.go @@ -142,7 +144,7 @@ This file lists every review comment thread that hugodutka marked as deferred. I - [ ] 28. runner_manager.go: fix wg.Wait/mu.Lock concurrency concern (r3380592305) - [ ] 29. runner_manager.go: skip logging context canceled errors, line 458 (r3387355724) - [ ] 30. runner_manager.go: document stateCh buffering semantics (r3387788957) -- [ ] 31. testhooks.go: accept context.Context instead of hard-coded timeout (r3382365135) +- [x] 31. testhooks.go: accept context.Context instead of hard-coded timeout (r3382365135) - [ ] 32. tasks.go: extract side-effecting deps to interface (r3382554277) - [ ] 33. tasks_test.go: use interface and gomock for taskStarter spy (r3382564035) - [ ] 34. tasks.go: required options as newTaskStarter args (r3387867697)