feat: remove unused chat statuses pending, paused, and completed - #27064
Conversation
Docs preview📖 View docs preview for |
|
@codex review |
ibetitsmike
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd4fb9de5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e220380f87
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 028d826410
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8907b366bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbebc8cb53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
hugodutka
left a comment
hugodutka
left a comment
There was a problem hiding this comment.
LGTM except some nits and the bundled frontend logic changes. I'll hold off on approving until I understand if the frontend logic changes are necessary in this PR or not.
| -- Remap any historical rows to 'waiting', the idle resting state and the | ||
| -- column default. The column type is still the original chat_status here. | ||
| UPDATE chats SET status = 'waiting' | ||
| WHERE status IN ('pending', 'paused', 'completed'); |
There was a problem hiding this comment.
To be semantically correct pending should be changed to running. pending used to mean "there's work to be done but no runner has picked up the chat yet"
There was a problem hiding this comment.
Fixed in 581ad2a: the remap is now split, pending goes to running (picked up by the worker acquisition query since worker_id is NULL) and paused/completed go to waiting. Migration comments and the PR description are updated to match.
🤖 Comment by Mux (AI agent) on Mike's behalf.
|
|
||
| // TestClassifyExecutionState_Invalid covers every documented invalid | ||
| // combination: legacy statuses, waiting-with-queue, and archived busy | ||
| // combination: unknown statuses, waiting-with-queue, and archived busy |
There was a problem hiding this comment.
changes in this file seem unnecessary
There was a problem hiding this comment.
Reverted to main in fbb0c1d. The raw legacy status strings still compile after the enum removal, so the original coverage stays intact.
🤖 Comment by Mux (AI agent) on Mike's behalf.
| // running before processOnce calls inflight.Add(1). If we only | ||
| // waited for status != pending, we could call Wait() while Add(1) | ||
| // hasn't happened yet. | ||
| // the running state (meaning processChat has finished its cleanup |
There was a problem hiding this comment.
that comment is invalid, processChat doesn't exist anymore
There was a problem hiding this comment.
Reworded in fbb0c1d to describe the chat worker flow without referencing the removed processChat/processOnce symbols.
🤖 Comment by Mux (AI agent) on Mike's behalf.
| // pending (waiting to be acquired) nor running (being | ||
| // Wait until the chat reaches a settled state (not being | ||
| // processed). This guarantees that inflight.Add(1) has | ||
| // already been called by processOnce. |
There was a problem hiding this comment.
that comment is invalid, processChat doesn't exist anymore
There was a problem hiding this comment.
Reworded in fbb0c1d, same as above: the comment now describes wake pickup and background-work registration without the removed symbols.
🤖 Comment by Mux (AI agent) on Mike's behalf.
| @@ -997,13 +997,11 @@ func TestListChats(t *testing.T) { | |||
| require.Equal(t, modelConfig.ID, chat.LastModelConfigID) | |||
| // The chat may have been picked up by the background | |||
| // processor (via signalWake) before we list, so | |||
There was a problem hiding this comment.
signalWake doesn't exist anymore
There was a problem hiding this comment.
Fixed in fbb0c1d: both signalWake references in this file now say the chat worker may pick up the chat.
🤖 Comment by Mux (AI agent) on Mike's behalf.
| | n/a | `requires_action` | Agent invoked a client-provided tool and is waiting for the result before continuing. | | ||
| | Tasks API status | Chats API status | Notes | | ||
| |------------------|-------------------|---------------------------------------------------------------------------------------------------------------------| | ||
| | `pending` | `running` | Chats have no separate queued state; a picked-up chat reports `running`. | |
There was a problem hiding this comment.
shouldn't this say "a chat that hasn't been picked up yet reports running?
There was a problem hiding this comment.
Yes, fixed in f7d65c6: the row now reads "a chat that hasn't been picked up yet reports running", which also matches the migration remap of pending to running.
🤖 Comment by Mux (AI agent) on Mike's behalf.
| // the running-case promote, where the backend reorders the | ||
| // queued message to the front before auto-promoting it. | ||
| suppressedQueuedMessageIDs: ReadonlySet<number>; | ||
| // Drops incoming message_part events while an optimistic request |
There was a problem hiding this comment.
Could we move fixes that change the functionality of the frontend to a separate PR? I'd rather we kept this PR just to unused status cleanup, so the frontend changes can be reviewed separately.
There was a problem hiding this comment.
Done: the streamPartsSuppressed mechanism is removed from this PR and moved to the stacked follow-up #27098. This PR's frontend diff is now mechanical cleanup only (dropping the removed statuses from guards, status config, and the optimistic promote, which now writes running). Context for the follow-up: the removed pending status implicitly dropped late stream frames during the queued-message promote; #27098 restores that guard explicitly. Until it lands there is a brief window where a late frame from an interrupted turn can repopulate a cleared stream during the promote; the next status event still clears it. Both PR descriptions call this out.
🤖 Comment by Mux (AI agent) on Mike's behalf.
| // stale frames are still expected there, so keep | ||
| // dropping them until the next status. | ||
| if (nextStatus !== "interrupting") { | ||
| store.unsuppressStreamParts(); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Moved to #27098, see #27064 (comment).
🤖 Comment by Mux (AI agent) on Mike's behalf.
| }); | ||
| }); | ||
|
|
||
| it("drops suppressed parts until a status event lifts the suppression", async () => { |
There was a problem hiding this comment.
There was a problem hiding this comment.
Moved to #27098 along with the suppression test, see #27064 (comment).
🤖 Comment by Mux (AI agent) on Mike's behalf.
cbebc8c to
f7d65c6
Compare
| queryClient.getQueryData<TypesGen.Chat>(chatKey(chatId)) ?? | ||
| chatList.find((candidate) => candidate.id === chatId); | ||
| if (chat === undefined || isActiveChat(chat)) { | ||
| if (chat === undefined || chat.status === "running") { |
There was a problem hiding this comment.
not sure if it applies here, but a chat is also active when it's in the interrupting and requires_action states
There was a problem hiding this comment.
Good catch, applies here: this guard decides whether archiving shows the "will interrupt the current run" confirmation, and both statuses have an in-flight run. Fixed in ffa5643 by restoring the isActiveChat helper with running/interrupting/requires_action.
🤖 Comment by Mux (AI agent) on Mike's behalf.
ffa5643 to
c6c329f
Compare
Documentation CheckUpdates Needed
Automated review via Coder Agents |
c6c329f to
cd12a50
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd12a502d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The chatd state machine only recognizes waiting, running, error, requires_action, and interrupting. The pending, paused, and completed enum values were legacy: never written by production code and treated as invalid by ClassifyExecutionState. Remove them from the chat_status enum and every code path that referenced them. Migration 000540 remaps historical rows to waiting (the idle resting state and column default), drops the obsolete idx_chats_pending partial index, and recreates chats_expanded around the enum swap. Also removes the dead AcquireChats query (replaced by the chat worker acquisition flow) and the legacy pending compatibility branches in chatd, the SDK, and the frontend.
Inline the single-comparison isActiveChat helper in AgentsPage, drop redundant clearRetryState calls in the useChatStore status handler (an unconditional clear already runs first), and fix a stale test comment that claimed the accepted status list was active-only.
…history in test comment
…emetry test The child chat seed changed from completed to waiting when the unused chat statuses were removed, but the assertion still expected the old status string.
…igration a no-op Restoring the removed enum values cannot restore the remapped rows, so the rebuild cycle bought nothing. Prior art: 000377, 000384.
…0541 Main merged its own 000540 (workspace_build_orchestrations), which made the migration numbers collide and broke make gen and sqlc-vet.
…ation CreateChat inserts new chats with status running; the walkthrough still told users to expect waiting or the removed pending value.
pending meant queued work that no runner had picked up yet, so waiting was the wrong target: a remapped chat with queued messages would sit idle until stale-chat recovery noticed it. running chats without a worker are picked up by the worker acquisition query and serviced immediately. paused and completed stay remapped to waiting.
…e test comments Revert state_internal_test.go to its pre-branch shape: the raw legacy status strings still compile after the enum removal and the dedicated invalid-status cases keep their coverage. Reword test helper comments in chatd_test.go and exp_chats_test.go that referenced the removed processChat/processOnce/signalWake implementation details.
A chat that has not been picked up by a runner yet reports running, matching the migration remap of legacy pending rows.
…requires_action chats The archive confirmation guard only checked running, but a chat in interrupting or requires_action also has an in-flight run that archiving would interrupt.
919fbfe to
1455e40
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
The chatd state machine only recognizes
waiting,running,error,requires_action, andinterrupting. Remove the unusedpending,paused, andcompletedvalues from the database enum, backend, SDK, frontend, generated queries, and API docs.Migration
000543_chat_status_remove_unusedremaps existingpendingrows torunning, remapspausedandcompletedrows towaiting, drops the obsoleteidx_chats_pendingindex, and recreateschats_expandedaround the enum swap. It also removes the deadAcquireChatsquery and all remaining query literals for the deleted statuses.NOTE: The enum swap can break chat queries from older replicas during a mixed-version rollout because they still reference
'pending'::chat_status. Chats are experimental, so this PR accepts that limited rollout window instead of adding a two-release expand and contract sequence.