feat: implement the chat state machine by hugodutka · Pull Request #25908 · coder/coder

hugodutka · 2026-06-01T14:16:31Z

PR 1 of the chatd refactor. It serves 2 purposes:

it implements the chat state machine; and
it contains all the database changes in the chatd refactor PR stack.

I kept the database changes in a single PR to make rebasing easier. However, because of it, this PR doesn't compile and it doesn't pass tests - that's intentional. Only the tip of the stack will pass all tests.

ibetitsmike · 2026-06-02T10:18:33Z

@hugodutka I think it would be beneficial to use grill-with-docs to create a CONTEXT.md style file to describe all the concepts

hugodutka · 2026-06-02T10:29:09Z

@ibetitsmike I'm planning to include that file in the final PR - it'll be a lightly edited version of the RFC we have in Notion.

johnstcn · 2026-06-02T09:52:43Z

+	_ = f
+	_ = seeded
+	_ = result


drive-by nit: underscore the func params instead

johnstcn · 2026-06-02T11:11:51Z

+// Store exposes the active transaction store so callers can perform
+// validation reads (for example loading the messages affected by an
+// EditMessage transition) and metadata writes (for example updating
+// title or labels) that must be atomic with the transition.
+//
+// Callers MUST NOT use Store to mutate execution-state tables
+// (chats.status, chat_messages, chat_queued_messages, chat_heartbeats,
+// or the version fields on chats). Those mutations belong to the
+// transition methods and are validated against the state machine
+// matrix.
+func (tx *Tx) Store() database.Store { return tx.store }


concern, non-blocking: My biggest architectural suggestion: could we instead expose only the set of read-only 'safe' methods that cannot mutate state here? This could be a follow-up meta-refactor.

We talked about it on Slack: we can't expose only read-only methods, since metadata updates, like the workspace binding, are still handled by direct DB calls rather than transitions. We can however expose only a subset of Store to make direct DB calls inside machine.Update more intentional.

I'm going to do this in a follow up PR after we merge the refactor. Then we'll be sure what subset is needed.

johnstcn · 2026-06-02T11:32:54Z

review: this is replaced by SetFamilyArchived. We need chatd to handle the state transition so we can't just go around mutating it with a bulk query.

^ refers to AutoArchiveInactiveChats

I'm going to remove AutoArchiveInactiveChats in a follow up PR. I had to keep it here because it was causing problems during rebase.

github-actions · 2026-06-04T18:52:11Z

Docs preview

📖 View docs preview for docs/admin/security/audit-logs.md

mafredri

Submitting part 1 of my review, looking at chatstate next.

mafredri · 2026-06-05T09:04:36Z

@@ -1999,6 +2032,13 @@ func (q *querier) DeleteChatDebugDataByChatID(ctx context.Context, arg database.
 	return q.db.DeleteChatDebugDataByChatID(ctx, arg)
 }

+func (q *querier) DeleteChatHeartbeats(ctx context.Context, arg database.DeleteChatHeartbeatsParams) (int64, error) {


This seems to be a batch method, although the naming suggests its for a single chat, we should make the name reflect what it does (applies to multiple methods).

Renamed DeleteChatHeartbeats to BatchDeleteChatHeartbeats and UpsertChatHeartbeats to BatchUpsertChatHeartbeats. I didn't find any other queries that needed renaming.

mafredri · 2026-06-05T09:06:44Z

@@ -0,0 +1,346 @@
+-- Foundation for the chatd core state machine refactor (PR 1).


I'd prefer to remove self-referential references like "PR 1" from these comments as the comments will live on without that context.

mafredri · 2026-06-05T09:07:49Z

+
+-- 2. Add new versioning, ownership, retry, and pending-action fields to chats.
+ALTER TABLE chats
+    ADD COLUMN snapshot_version bigint NOT NULL DEFAULT 1,


Readers will wonder why snapshots begin at 1 while other versions at 0. I suggest encoding reasoning from the RFC as COMMENT ON on tables, columns, etc so that the knowledge doesn't just live hidden away in the migration file and RFC.

Added COMMENT ON entries to explain it.

mafredri · 2026-06-05T09:15:16Z

+
+-- 11. Index for queue-order reads and head selection.
+CREATE INDEX IF NOT EXISTS idx_chat_queued_messages_chat_position_id
+    ON chat_queued_messages(chat_id, position, id);


This index requires a bit more explanation about its purpose and what it's targeting. Is the chat_id, position, and id known when querying? Is it for joining efficiency? Index-only lookups? Depending on table size it could even slow things down so worth going through the EXPLAIN (ANALYZE, COSTS, VERBOSE, BUFFERS, FORMAT JSON) of it.

Removed the index altogether. I don't think it's needed at this point.

mafredri · 2026-06-05T09:17:22Z

+
+-- 14. chat_heartbeats: unlogged table for ownership leases. Keyed by
+-- (chat_id, runner_id) so a single chat can briefly have entries from
+-- multiple runners during failover.


When adding a comment on, worth mentioning why it's safe that it's unlogged as well.

Updated the comment.

mafredri · 2026-06-05T09:24:00Z

+-- current runner heartbeat is stale. The runner_id IS NULL predicate is
+-- a robustness extension for inconsistent rows where a worker_id exists
+-- without a runner_id; normal missing ownership is worker_id IS NULL or
+-- a missing or stale heartbeat row.


I wonder if we should encode more about the inclusion criteria in the doc comment. Just seeing this comment doesn't really leave enough clues to verify the implementation and thus trusting that its correct. Making changes here becomes slightly dangerous. I'm guessing test coverage will prove it behaves the right way but that may also change as time goes on.

Updated the comment to describe the inclusion criteria.

mafredri · 2026-06-05T09:26:21Z

+-- are intentionally kept side-by-side with the legacy chatd queries
+-- above so the existing runtime keeps working while the state machine
+-- lands behind it.
+-- =====================================================================


I think these belong in their own file rather than having a custom form header comment with === separation. Just put this doc at the top. Also, referring to previous comment about query comments having enough high-level detail that a reader can verify the query implementation, I think it's worth going through the queries below through that lens as well.

I think chats.sql is the right place for these queries. State machine queries aren't special in any way - they should be in the same place as all the other queries that modify and read from chat-related tables.

I think it's worth going through the queries below through that lens as well

Could you point out queries that should have clearer comments? I think they are pretty clear already.

mafredri · 2026-06-05T09:29:19Z

+	HistoryVersion           int64                 `db:"history_version" json:"history_version"`
+	QueueVersion             int64                 `db:"queue_version" json:"queue_version"`
+	GenerationAttempt        int64                 `db:"generation_attempt" json:"generation_attempt"`
+	RetryState               pqtype.NullRawMessage `db:"retry_state" json:"retry_state"`


What data type is this, curious why it's a raw message rather than e.g. json if the content is json?

The content is jsonb. sqlc uses pqtype.NullRawMessage to represent it.

mafredri

Since there was so much to go through, I've mainly focused on trying to review the production code with spot checks that the allowable states matches what the code claims. I have not done in-depth analysis of the tests and what they prove. From what I've gathered so far, the state machine seems sound.

mafredri · 2026-06-05T09:35:45Z

+// stillBlocked returns true if c has NOT received a value yet. The
+// caller must already have established a happens-before ordering via
+// some other channel so this check is meaningful.
+func stillBlocked(c <-chan struct{}) bool {


"Received a value" can be expressed as len(c) == 0 which is simpler. Does this also check if c is closed? Docs could be updated and len would remove disambiguity.

It also checks if c is closed. Updated the comment.

mafredri · 2026-06-05T09:40:59Z

+	lockWG.Go(func() {
+		lockErr = m.Lock(ctx, func(_ database.Store) error {
+			close(lockEntered)
+			<-releaseLock


We should try to avoid naked selects that block forever. If release doesn't happen we'll leave this goroutine running forever, lockWG.Wait blocks forever, etc. Channels won't et closed when require short-circuits the test. These types of setups have typically lead to flakes or hard-to-diagnose issues.

Addressed by also selecting on ctx.Done.

mafredri · 2026-06-05T09:43:48Z

+		// Force a sync round-trip through the DB. This serves the
+		// same role as a Sleep but is deterministic: by the time
+		// this read completes, the scheduler has had a chance to
+		// run goroutine B if it could make progress.


This is not necessarily true on a highly congested low-resource system. Ideally we could test this with true determinism. Can't we hook into calls done by m.Update to know for sure?

Addressed by hooking into LockChatAndBumpSnapshotVersion. It's still not a deterministic solution, but it should be a bit more reliable.

mafredri · 2026-06-05T09:46:04Z

+// state classification helpers live in `state.go` and `transition.go`
+// alongside unit-testable classifiers; the SQL is in
+// `coderd/database/queries/chats.sql` (e.g. `LockChatAndBumpSnapshotVersion`,
+// `UpdateChatExecutionState`).


Referencing functio names and files is ripe to go stale. I'd like this doc to stand the test of time (barring a refactor).

mafredri · 2026-06-05T09:46:50Z

+
+	// ErrInvalidState is returned when the chat row, queue, and
+	// archive flag together produce a combination outside the 13
+	// valid execution states described in the RFC.


"The RFC" will not stand the test of time. These comments need to be updated and terminology established and kept consistent.

Removed mentions of the RFC from the comments.

mafredri · 2026-06-05T10:55:21Z

+		return PromoteQueuedMessageResult{}, xerrors.Errorf("set running: %w", err)
+	}
+	cancellations := inserted[:len(inserted)-1]
+	insertedUserMsg := inserted[len(inserted)-1]


This looks safe today, but we never check len(inserted) as a guard, should we?

Added a guard.

mafredri · 2026-06-05T10:57:53Z

+	}
+	return InterruptResult{}, newTransitionError(TransitionInterrupt, from, "unhandled state in Interrupt")


Suggested change

}

return InterruptResult{}, newTransitionError(TransitionInterrupt, from, "unhandled state in Interrupt")

default:

return InterruptResult{}, newTransitionError(TransitionInterrupt, from, "unhandled state in Interrupt")

}

Suggestion: This structure protects against unintended logic changes in earlier cases where the return wouldn't happen. Also makes it more explicit that the earlier cases are the only allowed states.

mafredri · 2026-06-05T10:59:50Z

+	if err != nil {
+		return EnterRequiresActionResult{}, xerrors.Errorf("get db now: %w", err)
+	}
+	deadline := sql.NullTime{Time: now.Add(5 * time.Minute), Valid: true}


Nit: Move 5 * time.Minute to a constant with accompanying documentation.

mafredri · 2026-06-05T11:01:13Z

+			"no pending dynamic tool calls",
+		)
+	}
+	now, err := tx.store.GetDatabaseNow(tx.ctx)


Inside a transaction, this always returns the same value. Do we really need to do this round-trip? This seems only used once, should we bake it into the update query and just pass in the offset (5 min)?.

We could, but I think it's clearer this way. I'd like to avoid premature optimization: if this ever proves to be a bottleneck, let's optimize it.

mafredri · 2026-06-05T11:05:50Z

+	}); err != nil {
+		return FinishInterruptionResult{}, xerrors.Errorf("set running: %w", err)
+	}
+	all := append([]database.ChatMessage{}, insertedPartial...)


Small inconsistency, above we return insertedPartial directly (no copy), here we make a copy for all. If we don't need a copy because we're guarding the underlying array, we can just all = append(insertedPartial, insertedHead...).

github-actions Bot assigned hugodutka Jun 1, 2026

hugodutka requested review from johnstcn and mafredri June 1, 2026 14:18

hugodutka mentioned this pull request Jun 1, 2026

feat: use the state machine in chatd http endpoints #25923

Merged

hugodutka force-pushed the chatd-pr1-state-machine branch from 5d6ca0a to 723e393 Compare June 3, 2026 12:45

johnstcn reviewed Jun 3, 2026

View reviewed changes

hugodutka force-pushed the chatd-pr1-state-machine branch 2 times, most recently from 779754a to f2c32c2 Compare June 4, 2026 18:51

mafredri reviewed Jun 5, 2026

View reviewed changes

hugodutka force-pushed the chatd-pr1-state-machine branch 8 times, most recently from 93e4119 to bb090c5 Compare June 11, 2026 17:03

hugodutka added 5 commits June 11, 2026 17:03

source files

19018b7

make gen

5c63479

make gen tmp files

4e96b35

source files post gen

86e0068

post gen

0bc334f

hugodutka force-pushed the chatd-pr1-state-machine branch from bb090c5 to 0bc334f Compare June 11, 2026 17:04

hugodutka mentioned this pull request Jun 11, 2026

fix: chatd refactor #26270

Merged

hugodutka changed the base branch from main to hugodutka/chatd-refactor June 11, 2026 17:07

hugodutka merged commit 079e31e into hugodutka/chatd-refactor Jun 11, 2026
17 of 29 checks passed

hugodutka deleted the chatd-pr1-state-machine branch June 11, 2026 17:08

github-actions Bot locked and limited conversation to collaborators Jun 11, 2026

		@@ -0,0 +1,346 @@
		-- Foundation for the chatd core state machine refactor (PR 1).

		}
		return InterruptResult{}, newTransitionError(TransitionInterrupt, from, "unhandled state in Interrupt")

Uh oh!

Conversation

hugodutka commented Jun 1, 2026

Uh oh!

ibetitsmike commented Jun 2, 2026

Uh oh!

hugodutka commented Jun 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnstcn Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 4, 2026

Docs preview

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

johnstcn Jun 2, 2026 •

edited

Loading