Address open review feedback from PR #8263 (TRX streaming store)#8285
Open
Evangelink wants to merge 3 commits into
Open
Address open review feedback from PR #8263 (TRX streaming store)#8285Evangelink wants to merge 3 commits into
Evangelink wants to merge 3 commits into
Conversation
- Fix doc comment in TrxTestResultExtractor: the serializer per-record cap is 64 MiB, not 16 MiB. - Apply the null-forgiving operator consistently on the bare `lastError` rethrows in `TrxResultStreamingStore.WriteRecordWithRetryAsync` so the nullable-flow analysis stays satisfied across all rethrow paths. - Switch the TRX streaming-store writer task from `Task.Run` to `ITask.RunLongRunning` so the `BlockingCollection<T>.TryTake` poll loop no longer blocks a shared threadpool thread. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Youssef1313
reviewed
May 16, 2026
| // BlockingCollection<T>.TryTake blocks the calling thread for up to _flushIntervalMs when | ||
| // the queue is idle. Running the writer on a dedicated long-running thread instead of the | ||
| // shared threadpool keeps it from starving threadpool consumers while it sleeps on the queue. | ||
| _writerTask = task.RunLongRunning(WriteLoopAsync, "TRX streaming store writer", CancellationToken.None); |
Member
There was a problem hiding this comment.
The awaits in WriteLoopAsync might get you back on threadpool so I don't think this is a good fix.
Contributor
There was a problem hiding this comment.
Pull request overview
Follow-up to PR #8263 that tightens documentation/nullable consistency in the TRX streaming store, and attempts to move the streaming-writer loop off the shared threadpool to reduce contention during idle waits.
Changes:
- Corrected
TrxTestResultExtractorcomment to reflect the serializer’s 64 MiB per-record cap. - Made
ExceptionDispatchInfo.Capture(lastError)rethrows nullable-consistent vialastError!. - Switched the streaming-store writer startup from
ITask.RuntoITask.RunLongRunning.
Show a summary per file
| File | Description |
|---|---|
| src/Platform/Microsoft.Testing.Extensions.TrxReport/Streaming/TrxTestResultExtractor.cs | Updates rationale comment to reference the correct serializer record-size cap (64 MiB). |
| src/Platform/Microsoft.Testing.Extensions.TrxReport/Streaming/TrxResultStreamingStore.cs | Starts the writer via RunLongRunning and aligns nullable usage in retry rethrow paths. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 1
Comment on lines
+69
to
+72
| // BlockingCollection<T>.TryTake blocks the calling thread for up to _flushIntervalMs when | ||
| // the queue is idle. Running the writer on a dedicated long-running thread instead of the | ||
| // shared threadpool keeps it from starving threadpool consumers while it sleeps on the queue. | ||
| _writerTask = task.RunLongRunning(WriteLoopAsync, "TRX streaming store writer", CancellationToken.None); |
Evangelink
commented
May 16, 2026
Member
Author
Evangelink
left a comment
Evangelink
left a comment
There was a problem hiding this comment.
✅ 21/21 dimensions clean — no findings.
All three changes are correct:
RunLongRunning: The API exists onITask/SystemTaskand correctly spawns a dedicated background thread, keeping the blockingBlockingCollection.TryTakeoff the shared threadpool. Rationale in the comment is accurate.lastError!: Both null-forgiving operators are safe. The catch sites are only reachable after at least one loop iteration that unconditionally assignslastError; null is impossible there.- Comment
16 MiB→64 MiB: Matches the actualMaxRecordLengthBytes = 64 * 1024 * 1024constant inTrxTestResultSerializer.cs; theMaxCapturedFieldChars = 1 MiBtruncation cap is still well below 64 MiB.
Generated by Expert Code Review (on open) for issue #8285 · ● 8.6M
Member
Author
|
@copilot address review comments |
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Contributor
Addressed in 6fbd731. The TRX streaming writer loop is now await-free/synchronous while running on the long-running background thread, so the |
Comment on lines
+223
to
+225
| _logger.LogErrorAsync( | ||
| $"TRX streaming store writer faulted; intermediate file may be incomplete. {discarded} record(s) were dropped from the in-memory queue and will not appear in the TRX.", | ||
| ex).ConfigureAwait(false); | ||
| ex).GetAwaiter().GetResult(); |
|
|
||
| await _logger.LogErrorAsync( | ||
| _logger.LogErrorAsync( | ||
| $"Failed to write TRX record {i + 1}/{batch.Count} after {MaxWriteRetries} retries; truncating to last good record. {unwritten} record(s) from this batch will not appear in the TRX.", |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #8263 addressing the open review threads on the merged PR.
Changes
64 MiB(TrxTestResultSerializer.MaxRecordLengthBytes), not16 MiBas the comment stated. Updated so the rationale forMaxCapturedFieldCharscites the correct number. (Copilot review threads x2)TrxResultStreamingStore.WriteRecordWithRetryAsyncnullable consistency — the two bareExceptionDispatchInfo.Capture(lastError).Throw()rethrows now use the null-forgiving operator, matching the finalCapture(lastError!).Throw()at the end of the method. (Copilot review thread)ITask.RuntoITask.RunLongRunningso theBlockingCollection<T>.TryTakepoll loop (which blocks for up to_flushIntervalMswhile idle) runs on its own dedicated background thread instead of starving the threadpool. (@Youssef1313 review thread)cc @Youssef1313