Files
coder/coderd/database/modelqueries_internal_test.go
T
Michael Suchacz e96d8646e2 fix(coderd): give chat message ids an append-order guarantee (#27495)
Chat message ordering was derived from `created_at`, which is `now()`
and therefore the transaction start time. That makes it unusable as an
append-order column for two independent reasons: every row in one
`InsertChatMessages` batch shares a single timestamp, and two concurrent
transactions can commit in the opposite order to the one they started
in.

This PR gives `chat_messages.id` a real append-order guarantee and moves
the history reads onto it.

## Changes

**`InsertChatMessages` had no input-order guarantee.** Callers index the
returned slice by input position. That only worked because PostgreSQL
happens to evaluate the `BIGSERIAL` default in row order. Ids are now
allocated up front and the k-th smallest is assigned to input index k,
so the pairing does not depend on where the column default is evaluated.
Returned rows are explicitly `ORDER BY id`.

**Three history reads now order by `id`.**

| Query | Was | Now |
|---|---|---|
| `GetChatMessagesByChatID` | `created_at ASC` | `id ASC` |
| `GetChatMessagesByRevisionForStream` | `created_at ASC, id ASC` | `id
ASC` |
| `GetLastChatMessageByRole` | `created_at DESC, id DESC` | `id DESC` |

`GetChatMessagesByChatID` paginated by `id` while ordering by
`created_at`, which is incoherent on its own terms.

The other two matter because of who consumes them. The stream query
supplies incremental updates on the same socket that emits a full
`GetChatMessagesByChatID` snapshot on history reset, so once that
snapshot moved to `id` the two disagreed under timestamp skew.
`GetLastChatMessageByRole` returns an id that is then used as an id
cursor, both as `AfterID` when synthesizing tool cancellations and as
`chats.last_read_message_id`, where a stale anchor leaves later
assistant messages permanently unread.

A tie-breaker would not have fixed either one. It only resolves equal
timestamps; leading with `created_at` is the actual defect.

**`GetLastChatMessageByRole` loses its index, so this adds one.** `ORDER
BY created_at DESC, id DESC` could take an ordered scan of
`idx_chat_messages_chat_created`. Nothing in the schema can supply
`ORDER BY id DESC LIMIT 1` for a given `chat_id` and `role`, so the
planner switches to a backward scan of the primary key and filters every
newer row in the table, scanning all of it when the chat has no message
in that role, which is the routine case for a fresh chat. Migration
`000559` adds `(chat_id, role, id DESC) WHERE deleted = false`, the same
shape as the existing `idx_chat_messages_user_prompts`. This matters
because the query is hot: it runs on every stream connect and
disconnect, and once per turn when synthesizing tool cancellations.

`GetChatMessagesForPromptByChatID` has the same defect and is fixed in
the stacked PR, because its compaction boundary change is semantic and
deserves a separate review. Auto-archive stays timestamp-based
deliberately: it measures activity, not order.

Wrapping the insert in a CTE (needed because `INSERT` cannot take `ORDER
BY`) makes sqlc synthesize `InsertChatMessagesRow`. It is structurally
identical to `ChatMessage`, so the call sites use a direct struct
conversion that stops compiling if the two ever diverge.

## Testing

Behavior tests write `created_at` values inverted against id order, so a
reader that leads with `created_at` returns the batch backwards. All
three queries were verified red by reverting the `ORDER BY` and
regenerating: the stream query returned `[3,2,1]` for `[1,2,3]`, and
`GetLastChatMessageByRole` picked id 1 instead of id 3.

`TestInsertChatMessagesOrderContract` asserts against the generated SQL,
covering what a behavior test cannot: PostgreSQL evaluates the id
default in row order anyway, so a batch still looks ordered once the
guarantee is removed.

`TestChatMessagesSequenceCacheIsOne` guards the cross-batch half of the
invariant. Ids follow chat row lock order only while the sequence hands
out one value at a time; sequence cache blocks are per session, so with
a cache above one a backend holding stale cached values can lock second
and still commit lower ids. Bumping a sequence cache is an ordinary
throughput tweak, and it would silently corrupt history order.

The index was checked on a 200k row fixture. Without it, the zero-match
lookup filters all 200,000 rows over 2763 buffers; with it, the plan is
an index scan with both `chat_id` and `role` in the index condition, no
sort node, and 3 buffers.

Note that the within-batch mapping does not depend on the cache size. It
is established by `ROW_NUMBER() OVER (ORDER BY id)` over the allocated
ids, so it holds regardless of `nextval` evaluation order.

## Note on the deleted subagent hand-sort

The subagent history reader's hand-sort stays deleted, but calling it
redundant was imprecise. It sorted by `created_at` then `id`, so it is
only equivalent to `id` ordering when the two agree. When they disagree
the old code selected a different "latest assistant". This is a
deliberate behavior change to match the new invariant, not dead-code
removal.

> Opened by Mux on behalf of Mike.
2026-07-29 07:17:42 +00:00

220 lines
8.1 KiB
Go

package database
import (
"reflect"
"regexp"
"slices"
"strings"
"testing"
"time"
"github.com/google/go-cmp/cmp"
"github.com/stretchr/testify/require"
"github.com/coder/coder/v2/codersdk"
"github.com/coder/coder/v2/testutil"
)
func TestIsAuthorizedQuery(t *testing.T) {
t.Parallel()
query := `SELECT true;`
_, err := insertAuthorizedFilter(query, "")
require.ErrorContains(t, err, "does not contain authorized replace string", "ensure replace string")
}
// TestWorkspaceTableConvert verifies all workspace fields are converted
// when reducing a `Workspace` to a `WorkspaceTable`.
// This test is a guard rail to prevent developer oversight mistakes.
func TestWorkspaceTableConvert(t *testing.T) {
t.Parallel()
staticRandoms := &testutil.Random{
String: func() string { return "foo" },
Bool: func() bool { return true },
Int: func() int64 { return 500 },
Uint: func() uint64 { return 126 },
Float: func() float64 { return 3.14 },
Complex: func() complex128 { return 6.24 },
Time: func() time.Time {
return time.Date(2020, 5, 2, 5, 19, 21, 30, time.UTC)
},
}
// This feels a bit janky, but it works.
// If you use 'PopulateStruct' to create 2 workspaces, using the same
// "random" values for each type. Then they should be identical.
//
// So if 'workspace.WorkspaceTable()' was missing any fields in its
// conversion, the comparison would fail.
var workspace Workspace
err := testutil.PopulateStruct(&workspace, staticRandoms)
require.NoError(t, err)
var subset WorkspaceTable
err = testutil.PopulateStruct(&subset, staticRandoms)
require.NoError(t, err)
require.Equal(t, workspace.WorkspaceTable(), subset,
"'workspace.WorkspaceTable()' is not missing at least 1 field when converting to 'WorkspaceTable'. "+
"To resolve this, go to the 'func (w Workspace) WorkspaceTable()' and ensure all fields are converted.")
}
// TestTaskTableConvert verifies all task fields are converted
// when reducing a `Task` to a `TaskTable`.
// This test is a guard rail to prevent developer oversight mistakes.
func TestTaskTableConvert(t *testing.T) {
t.Parallel()
staticRandoms := &testutil.Random{
String: func() string { return "foo" },
Bool: func() bool { return true },
Int: func() int64 { return 500 },
Uint: func() uint64 { return 126 },
Float: func() float64 { return 3.14 },
Complex: func() complex128 { return 6.24 },
Time: func() time.Time {
return time.Date(2020, 5, 2, 5, 19, 21, 30, time.UTC)
},
}
// Copies the approach taken by TestWorkspaceTableConvert.
//
// If you use 'PopulateStruct' to create 2 tasks, using the same
// "random" values for each type. Then they should be identical.
//
// So if 'task.TaskTable()' was missing any fields in its
// conversion, the comparison would fail.
var task Task
err := testutil.PopulateStruct(&task, staticRandoms)
require.NoError(t, err)
var subset TaskTable
err = testutil.PopulateStruct(&subset, staticRandoms)
require.NoError(t, err)
require.Equal(t, task.TaskTable(), subset,
"'task.TaskTable()' is not missing at least 1 field when converting to 'TaskTable'. "+
"To resolve this, go to the 'func (t Task) TaskTable()' and ensure all fields are converted.")
}
// TestAuditLogsQueryConsistency ensures that GetAuditLogsOffset and CountAuditLogs
// have identical WHERE clauses to prevent filtering inconsistencies.
// This test is a guard rail to prevent developer oversight mistakes.
func TestAuditLogsQueryConsistency(t *testing.T) {
t.Parallel()
getWhereClause := extractWhereClause(getAuditLogsOffset)
require.NotEmpty(t, getWhereClause, "failed to extract WHERE clause from GetAuditLogsOffset")
countWhereClause := extractWhereClause(countAuditLogs)
require.NotEmpty(t, countWhereClause, "failed to extract WHERE clause from CountAuditLogs")
// Compare the WHERE clauses
if diff := cmp.Diff(getWhereClause, countWhereClause); diff != "" {
t.Errorf("GetAuditLogsOffset and CountAuditLogs WHERE clauses must be identical to ensure consistent filtering.\nDiff:\n%s", diff)
}
}
// Same as TestAuditLogsQueryConsistency, but for connection logs.
func TestConnectionLogsQueryConsistency(t *testing.T) {
t.Parallel()
getWhereClause := extractWhereClause(getConnectionLogsOffset)
require.NotEmpty(t, getWhereClause, "getConnectionLogsOffset query should have a WHERE clause")
countWhereClause := extractWhereClause(countConnectionLogs)
require.NotEmpty(t, countWhereClause, "countConnectionLogs query should have a WHERE clause")
require.Equal(t, getWhereClause, countWhereClause, "getConnectionLogsOffset and countConnectionLogs queries should have the same WHERE clause")
}
// TestFinalizeStaleChatDebugRows_TerminalStatusAlignment asserts that the
// NOT IN ('completed', 'error', 'interrupted') literals in the
// FinalizeStaleChatDebugRows SQL query match the terminal statuses
// defined by ChatDebugTerminalStatuses in codersdk. If a new terminal
// status is added to Go but not to the SQL, this test fails.
func TestFinalizeStaleChatDebugRows_TerminalStatusAlignment(t *testing.T) {
t.Parallel()
// Extract all NOT IN (...) lists from the SQL constant.
re := regexp.MustCompile(`NOT IN\s*\(([^)]+)\)`)
matches := re.FindAllStringSubmatch(finalizeStaleChatDebugRows, -1)
require.NotEmpty(t, matches, "expected at least one NOT IN clause in finalizeStaleChatDebugRows")
// Parse the quoted status literals from each NOT IN clause.
literalRe := regexp.MustCompile(`'([^']+)'`)
goTerminal := codersdk.ChatDebugTerminalStatuses()
for _, match := range matches {
literals := literalRe.FindAllStringSubmatch(match[1], -1)
var sqlStatuses []string
for _, lit := range literals {
sqlStatuses = append(sqlStatuses, lit[1])
}
slices.Sort(sqlStatuses)
var goStatuses []string
for _, s := range goTerminal {
goStatuses = append(goStatuses, string(s))
}
slices.Sort(goStatuses)
require.Equal(t, goStatuses, sqlStatuses,
"terminal statuses in FinalizeStaleChatDebugRows SQL must match "+
"codersdk.ChatDebugTerminalStatuses(); update both when adding "+
"a new terminal status")
}
}
// TestInsertChatMessagesOrderContract guards the input-order guarantee that
// callers rely on when indexing the returned slice. A behavior test cannot:
// Postgres evaluates the id default in row order anyway, so a batch still looks
// ordered once the guarantee is removed.
func TestInsertChatMessagesOrderContract(t *testing.T) {
t.Parallel()
require.Contains(t, insertChatMessages, "nextval('chat_messages_id_seq')",
"ids must be allocated explicitly so they can be correlated to input array position")
require.Contains(t, insertChatMessages, "ROW_NUMBER() OVER (ORDER BY id)",
"the k-th smallest allocated id must be assigned to input index k")
require.Regexp(t, `(?s)ORDER BY id\s*\z`, strings.TrimSpace(insertChatMessages),
"returned rows must be explicitly ordered by id rather than relying on RETURNING order")
// Every parallel input array must be read at the allocated ordinal. A column
// left on UNNEST would be positioned by the executor instead.
subscripted := regexp.MustCompile(`\)\[allocated\.ord\]`).FindAllString(insertChatMessages, -1)
require.Len(t, subscripted, reflect.TypeOf(InsertChatMessagesParams{}).NumField()-1,
"each InsertChatMessagesParams array field, all but ChatID, must be subscripted by allocated.ord")
}
// extractWhereClause extracts the WHERE clause from a SQL query string
func extractWhereClause(query string) string {
// Find WHERE and get everything after it
wherePattern := regexp.MustCompile(`(?is)WHERE\s+(.*)`)
whereMatches := wherePattern.FindStringSubmatch(query)
if len(whereMatches) < 2 {
return ""
}
whereClause := whereMatches[1]
// Remove ORDER BY, LIMIT, OFFSET clauses from the end
whereClause = regexp.MustCompile(`(?is)\s+(ORDER BY|LIMIT|OFFSET).*$`).ReplaceAllString(whereClause, "")
// Remove SQL comments
whereClause = regexp.MustCompile(`(?m)--.*$`).ReplaceAllString(whereClause, "")
// Normalize indentation so subquery wrapping doesn't cause
// mismatches.
lines := strings.Split(whereClause, "\n")
for i, line := range lines {
lines[i] = strings.TrimLeft(line, " \t")
}
whereClause = strings.Join(lines, "\n")
return strings.TrimSpace(whereClause)
}