From 0c6fe1f378a052ca5e24b38bec8bf66d1d4c54cd Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Fri, 12 Jun 2026 13:07:57 +0000 Subject: [PATCH 1/3] 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 5c579fa16dce8800377e95f4f6109c77c8287938 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Thu, 18 Jun 2026 10:35:55 +0000 Subject: [PATCH 2/3] refactor(coderd/x/chatd): address chatd review comments --- coderd/x/chatd/auto_archive_internal_test.go | 4 +- coderd/x/chatd/chatd.go | 61 +++++++++----------- coderd/x/chatd/generation_preparer.go | 2 +- coderd/x/chatd/helpers_test.go | 27 ++++++++- coderd/x/chatd/options.go | 28 +++++---- coderd/x/chatd/runner_test.go | 18 +++--- coderd/x/chatd/tasks_test.go | 40 +++++++------ coderd/x/chatd/worker.go | 22 +++++-- coderd/x/chatd/worker_internal_test.go | 43 +++++--------- 9 files changed, 126 insertions(+), 119 deletions(-) diff --git a/coderd/x/chatd/auto_archive_internal_test.go b/coderd/x/chatd/auto_archive_internal_test.go index 8c2e68b924400..951ba66099ca8 100644 --- a/coderd/x/chatd/auto_archive_internal_test.go +++ b/coderd/x/chatd/auto_archive_internal_test.go @@ -293,9 +293,7 @@ func (f *workerTestFixture) newArchiveWorkerWithOptions(t *testing.T, opts chatW if opts.NotificationsEnqueuer == nil { opts.NotificationsEnqueuer = notificationstest.NewFakeEnqueuer() } - worker, err := newChatWorker(nil, opts) - require.NoError(t, err) - return worker + return newChatWorker(nil, opts.WorkerID, opts.Store, opts.Pubsub, opts.MessagePartBuffer, opts) } func mockAuditorPtr(auditor *audit.MockAuditor) *atomic.Pointer[audit.Auditor] { diff --git a/coderd/x/chatd/chatd.go b/coderd/x/chatd/chatd.go index 0e18f600b60cf..18405c36e9a28 100644 --- a/coderd/x/chatd/chatd.go +++ b/coderd/x/chatd/chatd.go @@ -2275,8 +2275,7 @@ type manualTitleGenerationError struct { // read; the title_change pubsub event it publishes remains the source of // truth for clients. type generatedChatTitle struct { - mu sync.RWMutex - title string + title atomic.Value // string } func (t *generatedChatTitle) Store(title string) { @@ -2284,9 +2283,7 @@ func (t *generatedChatTitle) Store(title string) { return } - t.mu.Lock() - t.title = title - t.mu.Unlock() + t.title.Store(title) } func (t *generatedChatTitle) Load() (string, bool) { @@ -2294,12 +2291,11 @@ func (t *generatedChatTitle) Load() (string, bool) { return "", false } - t.mu.RLock() - defer t.mu.RUnlock() - if t.title == "" { + title, ok := t.title.Load().(string) + if !ok || title == "" { return "", false } - return t.title, true + return title, true } func (e *manualTitleGenerationError) Error() string { @@ -3421,20 +3417,9 @@ func New(ps pubsub.Pubsub, cfg Config) *Server { p.metrics = chatloop.NopMetrics() } p.messagePartBuffer = messagepartbuffer.New(messagepartbuffer.Options{Clock: clk}) - localStreamPartsDialer := NewLocalStreamPartsDialer(LocalStreamPartsDialerConfig{ - Buffer: p.messagePartBuffer, - Logger: cfg.Logger, - }) - p.streamPartsDialer = streamPartsDialerForServer(workerID, localStreamPartsDialer, cfg.StreamPartsDialer) - p.streamSyncPoller = newStreamSyncPoller(ctx, cfg.Database, clk, cfg.Logger.Named("chatstream")) - p.streamSyncPoller.Start() - chatWorker, err := newChatWorker(p, chatWorkerOptions{ - WorkerID: workerID, - Store: cfg.Database, - Pubsub: ps, + workerOpts := chatWorkerOptions{ Logger: cfg.Logger.Named("chatworker"), Clock: clk, - MessagePartBuffer: p.messagePartBuffer, AcquisitionInterval: pendingChatAcquireInterval, AcquisitionBatchSize: maxChatsPerAcquire, HeartbeatInterval: chatHeartbeatInterval, @@ -3442,11 +3427,22 @@ func New(ps pubsub.Pubsub, cfg Config) *Server { NotificationsEnqueuer: notificationsEnqueuer, Auditor: cfg.Auditor, AutoArchiveRecords: chatAutoArchiveRecords, - }) - if err != nil { - panic("chatd: create chat worker: " + err.Error()) } - p.chatWorker = chatWorker + localStreamPartsDialer := NewLocalStreamPartsDialer(LocalStreamPartsDialerConfig{ + Buffer: p.messagePartBuffer, + Logger: cfg.Logger, + }) + p.streamPartsDialer = streamPartsDialerForServer(workerID, localStreamPartsDialer, cfg.StreamPartsDialer) + p.streamSyncPoller = newStreamSyncPoller(ctx, cfg.Database, clk, cfg.Logger.Named("chatstream")) + p.streamSyncPoller.Start() + p.chatWorker = newChatWorker( + p, + workerID, + cfg.Database, + ps, + p.messagePartBuffer, + workerOpts, + ) //nolint:gocritic // The chat processor uses a scoped chatd context. ctx = dbauthz.AsChatd(ctx) @@ -4134,7 +4130,7 @@ func (p *Server) loadPersonalSkillBody( return parsed, nil } -func (p *Server) appendRootChatTools( +func (p *Server) appendRootChatToolsWithoutWorkspaceContextPersistence( ctx context.Context, tools []fantasy.AgentTool, opts rootChatToolsOptions, @@ -4145,14 +4141,11 @@ func (p *Server) appendRootChatTools( // build logs before the tool completes. p.publishChatPubsubEvent(updatedChat, codersdk.ChatWatchEventKindStatusChange, nil) - // Note: we intentionally do not insert AGENTS.md / workspace - // context here. Local tool callbacks must not mutate chat - // history while a local-tool generation task is in flight, - // because that advances history_version before the tool - // result is committed and exits the local-tool commit as - // stale. Workspace context is persisted by the - // persist_workspace_context generation action in a later - // pass. + // Do not persist workspace context from this callback. Local + // tool callbacks run while a generation task is fenced by + // history_version; mutating chat history here would make that + // commit stale. The generation state machine runs + // persist_workspace_context after this tool result commits. // Prime the workspace MCP tools cache while the create_workspace // or start_workspace tool is still running. The AgentID guard diff --git a/coderd/x/chatd/generation_preparer.go b/coderd/x/chatd/generation_preparer.go index a0aec403ba279..d18cf06d455bf 100644 --- a/coderd/x/chatd/generation_preparer.go +++ b/coderd/x/chatd/generation_preparer.go @@ -352,7 +352,7 @@ func (server *Server) prepareGeneration( tools = append(tools, chattool.NewAskUserQuestionTool()) } if isRootChat { - tools = server.appendRootChatTools(ctx, tools, rootChatToolsOptions{ + tools = server.appendRootChatToolsWithoutWorkspaceContextPersistence(ctx, tools, rootChatToolsOptions{ chat: chat, modelConfigID: modelConfig.ID, workspaceCtx: &workspaceCtx, diff --git a/coderd/x/chatd/helpers_test.go b/coderd/x/chatd/helpers_test.go index 352392f26c48c..84f7d5a5464e5 100644 --- a/coderd/x/chatd/helpers_test.go +++ b/coderd/x/chatd/helpers_test.go @@ -269,10 +269,31 @@ func testOptions(t *testing.T, f *workerTestFixture, starter chatWorkerTaskStart } } -func startWorker(t *testing.T, opts chatWorkerOptions) *chatWorker { +func testWorkerDeps(f *workerTestFixture, opts chatWorkerOptions) chatWorkerDependencies { + workerID := opts.WorkerID + if workerID == uuid.Nil { + workerID = uuid.New() + } + store := opts.Store + if store == nil { + store = f.db + } + pubsub := opts.Pubsub + if pubsub == nil { + pubsub = f.pubsub + } + return chatWorkerDependencies{ + WorkerID: workerID, + Store: store, + Pubsub: pubsub, + MessagePartBuffer: opts.MessagePartBuffer, + } +} + +func startWorker(t *testing.T, f *workerTestFixture, opts chatWorkerOptions) *chatWorker { t.Helper() - worker, err := newChatWorker(nil, opts) - require.NoError(t, err) + deps := testWorkerDeps(f, opts) + worker := newChatWorker(nil, deps.WorkerID, deps.Store, deps.Pubsub, deps.MessagePartBuffer, opts) require.NoError(t, worker.Start(context.Background())) t.Cleanup(func() { require.NoError(t, worker.Close()) }) return worker diff --git a/coderd/x/chatd/options.go b/coderd/x/chatd/options.go index ff3dbdd3d9a30..3e3e8929eb310 100644 --- a/coderd/x/chatd/options.go +++ b/coderd/x/chatd/options.go @@ -8,7 +8,6 @@ import ( "github.com/google/uuid" "github.com/prometheus/client_golang/prometheus" - "golang.org/x/xerrors" "cdr.dev/slog/v3" "github.com/coder/coder/v2/coderd/audit" @@ -94,19 +93,18 @@ type chatWorkerOptions struct { TaskRetryMaxBackoff time.Duration } -func (o chatWorkerOptions) withDefaults() (chatWorkerOptions, error) { - if o.Store == nil { - return chatWorkerOptions{}, xerrors.New("chatworker: store is required") - } - if o.Pubsub == nil { - return chatWorkerOptions{}, xerrors.New("chatworker: pubsub is required") - } - if o.TaskStarter == nil && o.MessagePartBuffer == nil { - return chatWorkerOptions{}, xerrors.New("chatworker: task starter or message part buffer is required") - } - if o.WorkerID == uuid.Nil { - return chatWorkerOptions{}, xerrors.New("chatworker: worker ID is required") - } +type chatWorkerDependencies struct { + WorkerID uuid.UUID + Store database.Store + Pubsub chatWorkerPubsub + MessagePartBuffer *messagepartbuffer.Buffer +} + +func (o chatWorkerOptions) withDefaults(deps chatWorkerDependencies) chatWorkerOptions { + o.WorkerID = deps.WorkerID + o.Store = deps.Store + o.Pubsub = deps.Pubsub + o.MessagePartBuffer = deps.MessagePartBuffer if o.Clock == nil { o.Clock = quartz.NewReal() } @@ -155,5 +153,5 @@ func (o chatWorkerOptions) withDefaults() (chatWorkerOptions, error) { if o.TaskRetryMaxBackoff < o.TaskRetryInitialBackoff { o.TaskRetryMaxBackoff = o.TaskRetryInitialBackoff } - return o, nil + return o } diff --git a/coderd/x/chatd/runner_test.go b/coderd/x/chatd/runner_test.go index ef95069518632..9a2b829c0c953 100644 --- a/coderd/x/chatd/runner_test.go +++ b/coderd/x/chatd/runner_test.go @@ -18,7 +18,7 @@ func TestRunner_IgnoresDuplicateStateNotifications(t *testing.T) { f := newWorkerTestFixture(t) chat := f.createRunningChat(t) starter := newBlockingTaskStarter(false) - startWorker(t, testOptions(t, f, starter)) + startWorker(t, f, testOptions(t, f, starter)) starter.waitCall(t, taskKindGeneration, chat.ID) latest, err := f.db.GetChatByID(testutil.Context(t, testutil.WaitShort), chat.ID) require.NoError(t, err) @@ -33,7 +33,7 @@ func TestRunner_CancelsActiveTaskWhenHistoryChanges(t *testing.T) { f := newWorkerTestFixture(t) chat := f.createRunningChat(t) starter := newBlockingTaskStarter(false) - startWorker(t, testOptions(t, f, starter)) + startWorker(t, f, testOptions(t, f, starter)) first := starter.waitCall(t, taskKindGeneration, chat.ID) updated := commitAssistantStep(t, f, chat.ID, "first step") @@ -49,7 +49,7 @@ func TestRunner_CancelsActiveTaskWhenStatusChanges(t *testing.T) { f := newWorkerTestFixture(t) chat := f.createRunningChat(t) starter := newBlockingTaskStarter(false) - startWorker(t, testOptions(t, f, starter)) + startWorker(t, f, testOptions(t, f, starter)) first := starter.waitCall(t, taskKindGeneration, chat.ID) updated := interruptChat(t, f, chat.ID) @@ -64,7 +64,7 @@ func TestRunner_CleansUpOnOwnershipTakeover(t *testing.T) { f := newWorkerTestFixture(t) chat := f.createRunningChat(t) starter := newBlockingTaskStarter(false) - startWorker(t, testOptions(t, f, starter)) + startWorker(t, f, testOptions(t, f, starter)) first := starter.waitCall(t, taskKindGeneration, chat.ID) acquireChat(t, f, chat.ID, uuid.New(), uuid.New()) @@ -78,7 +78,7 @@ func TestRunner_SerializesReplacementTasksForSameHistoryAndStatus(t *testing.T) chat := f.createRunningChat(t) starter := newBlockingTaskStarter(true) defer starter.releaseAll() - startWorker(t, testOptions(t, f, starter)) + startWorker(t, f, testOptions(t, f, starter)) first := starter.waitCall(t, taskKindGeneration, chat.ID) forceExecutionStateAndPublish(t, f, chat.ID, database.ChatStatusInterrupting, false) @@ -97,7 +97,7 @@ func TestRunner_AllowsReplacementForDifferentHistoryOrStatus(t *testing.T) { chat := f.createRunningChat(t) starter := newBlockingTaskStarter(true) defer starter.releaseAll() - startWorker(t, testOptions(t, f, starter)) + startWorker(t, f, testOptions(t, f, starter)) first := starter.waitCall(t, taskKindGeneration, chat.ID) updated := commitAssistantStep(t, f, chat.ID, "different history") @@ -117,7 +117,7 @@ func TestRunner_TaskTimeoutRetries(t *testing.T) { opts.Clock = clock opts.TaskRetryInitialBackoff = time.Minute opts.TaskRetryMaxBackoff = time.Minute - startWorker(t, opts) + startWorker(t, f, opts) timeoutTrap.MustWait(testutil.Context(t, testutil.WaitLong)).MustRelease(testutil.Context(t, testutil.WaitLong)) timeoutTrap.Close() @@ -143,7 +143,7 @@ func TestWorker_RoutesDatabaseSyncStateToActiveRunner(t *testing.T) { opts := testOptions(t, f, starter) opts.Clock = clock opts.RunnerSyncInterval = time.Minute - startWorker(t, opts) + startWorker(t, f, opts) first := starter.waitCall(t, taskKindGeneration, chat.ID) forceExecutionState(t, f, chat.ID, database.ChatStatusInterrupting, false) @@ -157,7 +157,7 @@ func TestWorker_CleanupStopsRoutingAndCancelsTasks(t *testing.T) { f := newWorkerTestFixture(t) chat := f.createRunningChat(t) starter := newBlockingTaskStarter(false) - startWorker(t, testOptions(t, f, starter)) + startWorker(t, f, testOptions(t, f, starter)) first := starter.waitCall(t, taskKindGeneration, chat.ID) latest := acquireChat(t, f, chat.ID, uuid.New(), uuid.New()) diff --git a/coderd/x/chatd/tasks_test.go b/coderd/x/chatd/tasks_test.go index 8fcc588c35a6f..96d66fb2859f5 100644 --- a/coderd/x/chatd/tasks_test.go +++ b/coderd/x/chatd/tasks_test.go @@ -1124,25 +1124,27 @@ func startRealTaskWorker(t *testing.T, f *taskTestFixture) *chatWorker { t.Helper() buffer := messagepartbuffer.New(messagepartbuffer.Options{}) t.Cleanup(buffer.Close) - worker, err := newChatWorker(nil, chatWorkerOptions{ - WorkerID: uuid.New(), - Store: f.db, - Pubsub: f.pubsub, - Logger: slog.Make(), - MessagePartBuffer: buffer, - AcquisitionInterval: time.Hour, - AcquisitionBatchSize: 10, - RunnerSyncInterval: time.Hour, - HeartbeatInterval: time.Hour, - HeartbeatCleanupInterval: time.Hour, - HeartbeatStaleSeconds: 30, - StateChannelSize: 16, - RunnerManagerChannelSize: 16, - AcquisitionWakeChannelSize: 1, - TaskRetryInitialBackoff: time.Millisecond, - TaskRetryMaxBackoff: time.Millisecond, - }) - require.NoError(t, err) + worker := newChatWorker( + nil, + uuid.New(), + f.db, + f.pubsub, + buffer, + chatWorkerOptions{ + Logger: slog.Make(), + AcquisitionInterval: time.Hour, + AcquisitionBatchSize: 10, + RunnerSyncInterval: time.Hour, + HeartbeatInterval: time.Hour, + HeartbeatCleanupInterval: time.Hour, + HeartbeatStaleSeconds: 30, + StateChannelSize: 16, + RunnerManagerChannelSize: 16, + AcquisitionWakeChannelSize: 1, + TaskRetryInitialBackoff: time.Millisecond, + TaskRetryMaxBackoff: time.Millisecond, + }, + ) require.NoError(t, worker.Start(context.Background())) t.Cleanup(func() { require.NoError(t, worker.Close()) }) return worker diff --git a/coderd/x/chatd/worker.go b/coderd/x/chatd/worker.go index 7b3e8d5666fc1..7c7c75e029c57 100644 --- a/coderd/x/chatd/worker.go +++ b/coderd/x/chatd/worker.go @@ -13,6 +13,7 @@ import ( "github.com/coder/coder/v2/coderd/database" coderdpubsub "github.com/coder/coder/v2/coderd/pubsub" "github.com/coder/coder/v2/coderd/x/chatd/chatstate" + "github.com/coder/coder/v2/coderd/x/chatd/messagepartbuffer" ) // chatWorker owns chat acquisition and runner lifecycle for one process. @@ -32,12 +33,23 @@ type chatWorker struct { // newChatWorker constructs a chat worker. The worker is idle until Start is // called. -func newChatWorker(server *Server, opts chatWorkerOptions) (*chatWorker, error) { - withDefaults, err := opts.withDefaults() - if err != nil { - return nil, err +func newChatWorker( + server *Server, + workerID uuid.UUID, + store database.Store, + pubsub chatWorkerPubsub, + messagePartBuffer *messagepartbuffer.Buffer, + opts chatWorkerOptions, +) *chatWorker { + return &chatWorker{ + server: server, + opts: opts.withDefaults(chatWorkerDependencies{ + WorkerID: workerID, + Store: store, + Pubsub: pubsub, + MessagePartBuffer: messagePartBuffer, + }), } - return &chatWorker{server: server, opts: withDefaults}, nil } // chatWorkerID returns this worker's configured worker ID. diff --git a/coderd/x/chatd/worker_internal_test.go b/coderd/x/chatd/worker_internal_test.go index f01bb0d69cd71..4d4c646f48987 100644 --- a/coderd/x/chatd/worker_internal_test.go +++ b/coderd/x/chatd/worker_internal_test.go @@ -15,30 +15,13 @@ import ( "github.com/coder/quartz" ) -func TestWorker_NewRequiresTaskStarterOrMessagePartBuffer(t *testing.T) { - t.Parallel() - f := newWorkerTestFixture(t) - _, err := newChatWorker(nil, chatWorkerOptions{WorkerID: uuid.New(), Store: f.db, Pubsub: f.pubsub}) - require.ErrorContains(t, err, "task starter or message part buffer is required") -} - -func TestWorker_NewRequiresWorkerID(t *testing.T) { - t.Parallel() - f := newWorkerTestFixture(t) - opts := testOptions(t, f, newRecordingTaskStarter()) - opts.WorkerID = uuid.Nil - _, err := newChatWorker(nil, opts) - require.ErrorContains(t, err, "worker ID is required") -} - func TestWorker_UsesConfiguredWorkerID(t *testing.T) { t.Parallel() f := newWorkerTestFixture(t) starter := newRecordingTaskStarter() opts := testOptions(t, f, starter) workerID := opts.WorkerID - worker, err := newChatWorker(nil, opts) - require.NoError(t, err) + worker := newChatWorker(nil, workerID, opts.Store, opts.Pubsub, opts.MessagePartBuffer, opts) require.Equal(t, workerID, worker.chatWorkerID()) require.NoError(t, worker.Start(context.Background())) require.Equal(t, workerID, worker.chatWorkerID()) @@ -50,7 +33,7 @@ func TestWorker_AcquiresRunnableChatFromOwnershipHint(t *testing.T) { f := newWorkerTestFixture(t) chat := f.createRunningChat(t) starter := newRecordingTaskStarter() - worker := startWorker(t, testOptions(t, f, starter)) + worker := startWorker(t, f, testOptions(t, f, starter)) call := starter.waitCall(t, taskKindGeneration, chat.ID) require.Equal(t, worker.chatWorkerID(), call.input.WorkerID) @@ -73,7 +56,7 @@ func TestWorker_AcquiresRequiresActionChatFromOwnershipHint(t *testing.T) { f := newWorkerTestFixture(t) chat := f.createRequiresActionChat(t) starter := newRecordingTaskStarter() - startWorker(t, testOptions(t, f, starter)) + startWorker(t, f, testOptions(t, f, starter)) call := starter.waitCall(t, taskKindRequiresActionTimeout, chat.ID) require.Equal(t, database.ChatStatusRequiresAction, call.input.Status) @@ -88,7 +71,7 @@ func TestWorker_SkipsFreshlyOwnedChat(t *testing.T) { otherRunner := uuid.New() acquireChat(t, f, chat.ID, otherWorker, otherRunner) starter := newRecordingTaskStarter() - worker := startWorker(t, testOptions(t, f, starter)) + worker := startWorker(t, f, testOptions(t, f, starter)) worker.Wake() starter.assertNoCall(t) @@ -107,7 +90,7 @@ func TestWorker_ReacquiresStaleOwnedChat(t *testing.T) { acquireChat(t, f, chat.ID, deadWorker, deadRunner) makeHeartbeatStale(t, f, chat.ID, deadRunner) starter := newBlockingTaskStarter(false) - worker := startWorker(t, testOptions(t, f, starter)) + worker := startWorker(t, f, testOptions(t, f, starter)) call := starter.waitCall(t, taskKindGeneration, chat.ID) require.Equal(t, worker.chatWorkerID(), call.input.WorkerID) @@ -133,8 +116,8 @@ func TestWorker_TwoWorkersRaceSingleOwner(t *testing.T) { chat := f.createRunningChat(t) firstStarter := newRecordingTaskStarter() secondStarter := newRecordingTaskStarter() - first := startWorker(t, testOptions(t, f, firstStarter)) - second := startWorker(t, testOptions(t, f, secondStarter)) + first := startWorker(t, f, testOptions(t, f, firstStarter)) + second := startWorker(t, f, testOptions(t, f, secondStarter)) call := waitAnyTaskCall(t, firstStarter, secondStarter, taskKindGeneration, chat.ID) require.Contains(t, []uuid.UUID{first.chatWorkerID(), second.chatWorkerID()}, call.input.WorkerID) @@ -158,7 +141,7 @@ func TestWorker_DrainsMultipleRunnableChatsOnWake(t *testing.T) { starter := newRecordingTaskStarter() opts := testOptions(t, f, starter) opts.AcquisitionBatchSize = 1 - startWorker(t, opts) + startWorker(t, f, opts) want := map[uuid.UUID]bool{first.ID: true, second.ID: true, third.ID: true} for range 3 { @@ -178,7 +161,7 @@ func TestWorker_DoesNotAcquireIdleOrArchivedChats(t *testing.T) { archived := f.createRunningChat(t) forceExecutionStateAndPublish(t, f, archived.ID, database.ChatStatusRunning, true) starter := newRecordingTaskStarter() - worker := startWorker(t, testOptions(t, f, starter)) + worker := startWorker(t, f, testOptions(t, f, starter)) worker.Wake() starter.assertNoCall(t) @@ -195,7 +178,7 @@ func TestWorker_HeartbeatLoopRefreshesActiveRunnerHeartbeat(t *testing.T) { opts := testOptions(t, f, starter) opts.Clock = clock opts.HeartbeatInterval = time.Minute - startWorker(t, opts) + startWorker(t, f, opts) heartbeatTrap.MustWait(testutil.Context(t, testutil.WaitLong)).MustRelease(testutil.Context(t, testutil.WaitLong)) call := starter.waitCall(t, taskKindGeneration, chat.ID) oldHeartbeat := makeHeartbeatStale(t, f, chat.ID, call.input.RunnerID) @@ -228,7 +211,7 @@ func TestWorker_HeartbeatCleanupDeletesStaleRows(t *testing.T) { opts := testOptions(t, f, starter) opts.Clock = clock opts.HeartbeatCleanupInterval = time.Minute - startWorker(t, opts) + startWorker(t, f, opts) cleanupTrap.MustWait(testutil.Context(t, testutil.WaitLong)).MustRelease(testutil.Context(t, testutil.WaitLong)) clock.Advance(time.Minute).MustWait(testutil.Context(t, testutil.WaitLong)) @@ -250,7 +233,7 @@ func TestWorker_CloseDeletesOwnedHeartbeatsAndPublishesOwnershipHints(t *testing pubsub := newRecordingPubsub(f.pubsub) opts := testOptions(t, f, starter) opts.Pubsub = pubsub - worker := startWorker(t, opts) + worker := startWorker(t, f, opts) callsByChat := make(map[uuid.UUID]taskCall) for range 2 { call := starter.waitCall(t, taskKindGeneration, uuid.Nil) @@ -283,7 +266,7 @@ func TestWorker_CloseIsIdempotentAndDoesNotBlock(t *testing.T) { f := newWorkerTestFixture(t) chat := f.createRunningChat(t) starter := newBlockingTaskStarter(false) - worker := startWorker(t, testOptions(t, f, starter)) + worker := startWorker(t, f, testOptions(t, f, starter)) call := starter.waitCall(t, taskKindGeneration, chat.ID) closed := make(chan error, 1) From 0960282817b38a15bc497d3bf9ea089f1e1e8326 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Thu, 18 Jun 2026 10:39:21 +0000 Subject: [PATCH 3/3] chore: remove deferred review plan from branch --- deferred-review-threads-pr26109.md | 152 ----------------------------- 1 file changed, 152 deletions(-) delete mode 100644 deferred-review-threads-pr26109.md diff --git a/deferred-review-threads-pr26109.md b/deferred-review-threads-pr26109.md deleted file mode 100644 index 02063544bfbee..0000000000000 --- a/deferred-review-threads-pr26109.md +++ /dev/null @@ -1,152 +0,0 @@ -# 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)