Repository navigation
test(coderd/x/chatd/chatdebug): table the stream wrapper tests and cover their status rules - #30980
ibetitsmike wants to merge 5 commits into
Conversation
…ver their status rules
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.
…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.
|
@codex review
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Keep them coming! 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
|
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
Replaces five stream wrapper tests with two tables that run every case against both
wrapStreamSeqandwrapObjectStreamSeq. #30981 rewrites both wrappers on one finalize-once lifecycle, and these tests pin what that rewrite must keep. It deletesTestWrapStreamSeq_CompletedNotDowngradedByCtxCancel,TestWrapObjectStreamSeq_CompletedNotDowngradedByCtxCancel,TestWrapStreamSeq_DroppedStreamFinalizedOnCtxCancelandTestDebugModel_StreamObjectRejectsNilSequence.TestStreamWrapperStatusruns 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_StreamRejectsNilSequenceis now one table overStreamandStreamObject.TestDebugModel_Streamalso 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
finalizedcheck, 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-racethe 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 ofmodel.gowith 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
timewith 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.
FinalizeStaleand the stale thresholdFinalizeStaleChatDebugRowsqueryrunnerCtxfrom the runner debug turn