Skip to content

test(coderd/x/chatd/chatdebug): table the stream wrapper tests and cover their status rules - #30980

Draft
ibetitsmike wants to merge 5 commits into
mike/chatdebug-begin-runfrom
mike/chatdebug-stream-wrapper-tests
Draft

ibetitsmike wants to merge 5 commits into
mike/chatdebug-begin-runfrom
mike/chatdebug-stream-wrapper-tests

Conversation

@ibetitsmike

@ibetitsmike ibetitsmike commented Oct 18, 2026 •

Copy link
Copy Markdown
Collaborator

Replaces five stream wrapper tests with two tables that run every case against both wrapStreamSeq and wrapObjectStreamSeq. #30981 rewrites both wrappers on one finalize-once lifecycle, and these tests pin what that rewrite must keep. It deletes TestWrapStreamSeq_CompletedNotDowngradedByCtxCancel, TestWrapObjectStreamSeq_CompletedNotDowngradedByCtxCancel, TestWrapStreamSeq_DroppedStreamFinalizedOnCtxCancel and TestDebugModel_StreamObjectRejectsNilSequence.

  • New TestStreamWrapperStatus runs 13 cases on each wrapper: a finished stream, a consumer that stops before or after the finish part, a stream that ends without one, an error part with and without an error, an error on a text part, a consumer that stops after an error, a canceled error part, cancellation before the finish part and while the consumer holds it, a dropped stream, and a consumer that stops right after cancellation, which races the context safety net. Each case writes through a recording store and checks that the step is written exactly once, plus its status, finish reason, stored usage (all six token counts, or NULL), error count, last error, and whether the wrapper or the safety net finalized it.
  • TestDebugModel_StreamRejectsNilSequence is now one table over Stream and StreamObject.
  • TestDebugModel_Stream also checks the streamed text, not only that the response has content.

Why it is safe: the PR changes only a test file, and the tests pass on the production code of the layer below. To show they can fail, I ran 46 mutants of both wrappers and the nil-sequence checks. The whole package kills 17 of them before this PR and all 46 after it. The 29 it missed include the object wrapper's dropped-stream path, the rule that a stream without a finish part is interrupted, both error-part rules, cancellation while the consumer holds the finish part, text that is never materialized, usage that keeps only the input tokens or is stored without a finish part, and summary metadata on the safety net's write. Two more mutants drop the safety net's finalized check, so a step can be written twice. Across repeated batches of 50 runs, the race case kills them in 49 to 50 of 50 without -race. Under -race the kill rate depends on machine load: batches of 50 ranged from 24 to 46 kills. The unchanged code passes 200 runs with and without -race. A copy of model.go with garbage in it fails to build, which shows the mutants replaced the real file.

Review focus: the PR adds 117 more lines than it deletes. It is the test-first layer for #30981, which deletes 65 production lines.

Moved onto the deletion stack: a merge commit moved this layer onto the updated #30934 without a force push; its only conflict was the test file's import block (#30988 dropped time with the heartbeat tests). A second commit adds the audit follow-ups: the recording store, the stored usage check, the race case and the text check.

Validation: go test -race -count=1 ./coderd/x/chatd/chatdebug/... passes, golangci-lint run --fix=false ./coderd/x/chatd/chatdebug/... reports 0 issues, and the pre-commit hook passed.

Stack

This stack deletes the stale debug-run sweep, then simplifies the chatdebug debug-run, stream wrapper and step numbering code, drops the indexes the sweep used, and last deletes a runner debug field that lost its only reader with the heartbeats. The debug-run and stream wrapper refactors each sit on a test-only layer that pins the behavior they keep.

  1. refactor(coderd): delete the chat debug run and step heartbeats #30988 Delete the chat debug run and step heartbeats
  2. refactor: delete chatdebug FinalizeStale and the stale threshold #30989 Delete FinalizeStale and the stale threshold
  3. refactor: delete the FinalizeStaleChatDebugRows query #30995 Delete the FinalizeStaleChatDebugRows query
  4. test(coderd/x/chatd): pin the debug runs that turns, titles and status labels record #30933 Pin the debug runs that turns, titles and status labels record
  5. refactor(coderd/x/chatd): create and finalize debug runs through one helper #30934 Create and finalize debug runs through one helper
  6. This PR: Table the stream wrapper tests and cover their status rules
  7. refactor(coderd/x/chatd/chatdebug): share one finalize-once lifecycle between the stream wrappers #30981 Share one finalize-once lifecycle between the stream wrappers
  8. refactor(coderd): number chat debug steps in the database #31007 Number chat debug steps in the database
  9. refactor(coderd/x/chatd/chatdebug): delete the process-global step counters #31008 Delete the process-global step counters
  10. chore(coderd/database): drop the chat debug stale-sweep indexes #31009 Drop the chat debug stale-sweep indexes
  11. refactor(coderd/x/chatd): drop the unused runnerCtx from the runner debug turn #31010 Drop the unused runnerCtx from the runner debug turn

Xum, an AI agent, opened this PR for @ibetitsmike and wrote this description.

Moves this layer onto the updated #30934, which now sits on the
stale-sweep deletion stack (#30995), without rewriting history. The
only conflict was the test file's import block: the deletion stack
dropped the "time" import with the heartbeat tests, and this layer
adds "testing/synctest". The layer's net line changes are unchanged.
…ace in the stream wrapper tests

Follow-ups from the audit of this layer:

- Each TestStreamWrapperStatus case now writes through a recording
  store and requires exactly one UpdateChatDebugStep call.
- streamRecord compares the stored usage column (all six token counts,
  or nil when NULL) and requires it to match the response usage. It
  compared only the response's input tokens before.
- The safety-net rows (CanceledBeforeFinish, DroppedThenCanceled)
  require that no stream summary was recorded.
- New StoppedRightAfterCancel row for each wrapper: the consumer stops
  right after cancellation, so the context safety net and the
  consumer race to finalize, and only one of them may write.
- TestDebugModel_Stream checks the streamed text, not only that the
  response has content.
ibetitsmike added a commit that referenced this pull request Oct 18, 2026
…finalize-once

Moves this layer onto the updated #30980, which now sits on the
stale-sweep deletion stack (#30995), without rewriting history. The
deletion stack removed the stream heartbeats, so the merge drops them
from the shared lifecycle too: streamStep loses heartbeatDone and
startHeartbeat, finalizeLocked no longer closes the heartbeat channel,
neither wrapper calls step.startHeartbeat(), and launchHeartbeat is
gone. The rest of the layer's diff is unchanged.
@ibetitsmike
ibetitsmike added this pull request to stack #31004 October 18, 2026 18:59
@ibetitsmike

Copy link
Copy Markdown
Collaborator Author

@codex review

Xum, an AI agent, requested this review for @ibetitsmike.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-18T20:28:29.853987Z 6987bed Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: c636e5b868

ℹ️ 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".

@ibetitsmike

Copy link
Copy Markdown
Collaborator Author

@codex review

Xum, an AI agent, requested this review for @ibetitsmike.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Chef's kiss.

Reviewed commit: 6987bed4d4

ℹ️ 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".

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant