Report timeout as structured error instead of deserialization failure#75
Report timeout as structured error instead of deserialization failure#75twolff-gh wants to merge 1 commit into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesSubprocess timeout and result handling
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant SpawnProcessToRunTest
participant ChildProcess
participant ResultParser
SpawnProcessToRunTest->>ChildProcess: Start and wait for test subprocess
SpawnProcessToRunTest->>ChildProcess: Send SIGINT, then SIGABRT after escalation
ChildProcess-->>SpawnProcessToRunTest: Exit status and stdout
SpawnProcessToRunTest->>ResultParser: Parse stdout
ResultParser-->>SpawnProcessToRunTest: Parsed result or parse error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/ginkgo/parallel.go (1)
48-65: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSet
timedOutbefore sending SIGINT to close a timing race.
timedOut.Store(true)(Line 55) runs after the SIGINT is sent (Line 49). The main goroutine readstimedOut.Load()(Line 65) immediately aftercommand.Wait()returns. If the subprocess exits in response to SIGINT andWait()returns before the goroutine reaches theStore,timeout_firedreadsfalseand the timeout is misreported as a generic parse/command failure. Move the store ahead of the signal (after theCanceledcheck, which already distinguishes the normal-exit case).🔒️ Proposed reorder to remove the race window
if command.Process != nil { - _ = command.Process.Signal(syscall.SIGINT) + // no-op; signal sent below after recording the timeout } // Canceled means the process exited and the context was cancelled — no need to escalate if timeoutCtx.Err() == context.Canceled { return } timedOut.Store(true) + if command.Process != nil { + _ = command.Process.Signal(syscall.SIGINT) + } // if the process is hung, send SIGABRT after a grace period for a stack dump <-time.After(time.Minute) if command.Process != nil { _ = command.Process.Signal(syscall.SIGABRT) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ginkgo/parallel.go` around lines 48 - 65, The timeout race in the parallel command cleanup logic should be fixed by setting timedOut before sending SIGINT. Update the goroutine in parallel.go so timedOut.Store(true) happens immediately after the timeoutCtx.Err() == context.Canceled check and before command.Process.Signal(syscall.SIGINT), then keep the SIGABRT escalation path unchanged; this ensures the main goroutine’s timedOut.Load() after command.Wait() cannot miss the timeout signal.
🧹 Nitpick comments (2)
pkg/ginkgo/parallel.go (2)
65-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNit: prefer idiomatic camelCase.
timeout_firedis snake_case; Go convention istimeoutFired.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ginkgo/parallel.go` at line 65, The local variable in parallel.go uses snake_case instead of Go’s idiomatic camelCase. Rename timeout_fired to timeoutFired at the assignment and all nearby uses in the same scope so the naming matches Go conventions and remains consistent with the timedOut.Load() logic.
121-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffOptional: reduce duplication with
newTestResult.
newTimeoutResultre-implements the duration/DBTime/Outputconstruction already innewTestResult. Consider delegating the base result tonewTestResultand overriding onlyErrorto keep the two in sync as the struct evolves.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ginkgo/parallel.go` around lines 121 - 142, newTimeoutResult duplicates the base result-building logic already present in newTestResult, so keep the two paths in sync by delegating shared fields to newTestResult and only customizing the timeout-specific Error message. Update the newTimeoutResult function to reuse the Duration, StartTime, EndTime, and Output construction from newTestResult, using the same ExtensionTestResult structure and identifiers so future changes to result shaping only need to be made in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/ginkgo/parallel.go`:
- Around line 48-65: The timeout race in the parallel command cleanup logic
should be fixed by setting timedOut before sending SIGINT. Update the goroutine
in parallel.go so timedOut.Store(true) happens immediately after the
timeoutCtx.Err() == context.Canceled check and before
command.Process.Signal(syscall.SIGINT), then keep the SIGABRT escalation path
unchanged; this ensures the main goroutine’s timedOut.Load() after
command.Wait() cannot miss the timeout signal.
---
Nitpick comments:
In `@pkg/ginkgo/parallel.go`:
- Line 65: The local variable in parallel.go uses snake_case instead of Go’s
idiomatic camelCase. Rename timeout_fired to timeoutFired at the assignment and
all nearby uses in the same scope so the naming matches Go conventions and
remains consistent with the timedOut.Load() logic.
- Around line 121-142: newTimeoutResult duplicates the base result-building
logic already present in newTestResult, so keep the two paths in sync by
delegating shared fields to newTestResult and only customizing the
timeout-specific Error message. Update the newTimeoutResult function to reuse
the Duration, StartTime, EndTime, and Output construction from newTestResult,
using the same ExtensionTestResult structure and identifiers so future changes
to result shaping only need to be made in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 18020dbc-d317-415a-b09d-720051f4b552
📒 Files selected for processing (1)
pkg/ginkgo/parallel.go
1abddee to
6b69cdc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/ginkgo/parallel.go`:
- Around line 66-68: The early return in newTestResultFromOutput currently
bypasses timeout annotation when stdout parses successfully, so a timed-out
subprocess can return an ExtensionTestResult without the expected Error context.
Update newTestResultFromOutput in parallel.go to preserve the timeout
information on the successful parse path by checking the timeout state before
returning subcommandResult and setting ExtensionTestResult.Error consistently so
pkg/ginkgo/util.go can consume the expected contract.
- Around line 57-60: The delayed SIGABRT in the parallel process watcher can
still fire after Wait() returns, since the goroutine is never stopped and
command.Process may remain non-nil. Update the logic in parallel.go around the
goroutine that sends SIGTERM then waits before SIGABRT by adding a done channel
or cancellable context that is closed when Wait() completes, and gate the second
signal send on that cancellation before calling command.Process.Signal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6c52ba71-23d7-417a-a57b-c436ea59e3b9
📒 Files selected for processing (1)
pkg/ginkgo/parallel.go
6b69cdc to
310568b
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: roivaz, twolff-gh The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
not-stbenjam
left a comment
not-stbenjam
left a comment
There was a problem hiding this comment.
Thanks for this.
One thing I noticed while looking at the timing here (pre-existing, not something this PR introduced): the child process gets --timeout passed to it and is supposed to enforce its own deadline, but the parent's escalation timer starts ticking before the child's. That means the parent will always fire first, SIGINT the child, and the child never gets the chance to time out on its own. The child's self-reported timeout is the much nicer outcome, because ginkgo knows exactly which spec timed out and where, and it emits proper JSON that we can parse.
I think the fix is to give the parent a grace period, rather than exactly timeout, so the child gets first crack at reporting its own timeout, and the parent's escalation becomes the backstop for truly hung processes.
Some unit or integration tests around this would be great as well.
|
Claude's comment above ^ I can take a look next week but the feedback looks reasonable |
Sounds good. Ill factor that in. Ill also add tests 👍 |
|
New changes are detected. LGTM label has been removed. |
openshift-ci
Bot
added
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/ginkgo/parallel_test.go (2)
71-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the specific error message to catch a formatting bug.
Consider asserting the exact error string returned here, similar to the empty buffer test. Doing so will expose a bug in the upstream
newTestResultFromOutputfunction (visible in the provided context snippets), whereerrors.New("expected 1 result, got %v results")is incorrectly used instead offmt.Errorf.♻️ Proposed refactor
_, err := newTestResultFromOutput(stdout) if err == nil { t.Fatal("expected error for multi-element array, got nil") } + expectedErr := "expected 1 result, got %v results" // Note: This matches the upstream formatting bug + if err.Error() != expectedErr { + t.Errorf("error = %q, want %q", err.Error(), expectedErr) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ginkgo/parallel_test.go` around lines 71 - 75, Update the multi-element array case in the newTestResultFromOutput test to assert the exact expected error message, including the actual result count, rather than only checking that an error occurred. Use the same error-string assertion pattern as the empty-buffer test so the formatting behavior of newTestResultFromOutput is verified.
187-201: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid testing
fmt.Sprintfwith a duplicated format string.This test verifies the output of
fmt.Sprintfusing a format string copied directly into the test. This approach does not guarantee thatSpawnProcessToRunTestactually uses this exact format. Consider extracting the format string into a shared constant so both the test and the implementation use the same source of truth, or test the output of the function directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ginkgo/parallel_test.go` around lines 187 - 201, Update TestTimeoutErrorMessage to exercise the production timeout-message path, preferably by invoking SpawnProcessToRunTest with controlled timeout and subprocess-error inputs instead of duplicating fmt.Sprintf. Alternatively, extract the format string into a shared named constant used by both SpawnProcessToRunTest and the test, then keep assertions focused on the resulting message contents.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/ginkgo/parallel_test.go`:
- Line 19: Handle the json.Marshal errors in pkg/ginkgo/parallel_test.go at
lines 19, 38, 54, and 68 by capturing each error and failing the corresponding
test immediately with t.Fatalf when marshaling fails; preserve the existing data
flow after successful marshaling.
---
Nitpick comments:
In `@pkg/ginkgo/parallel_test.go`:
- Around line 71-75: Update the multi-element array case in the
newTestResultFromOutput test to assert the exact expected error message,
including the actual result count, rather than only checking that an error
occurred. Use the same error-string assertion pattern as the empty-buffer test
so the formatting behavior of newTestResultFromOutput is verified.
- Around line 187-201: Update TestTimeoutErrorMessage to exercise the production
timeout-message path, preferably by invoking SpawnProcessToRunTest with
controlled timeout and subprocess-error inputs instead of duplicating
fmt.Sprintf. Alternatively, extract the format string into a shared named
constant used by both SpawnProcessToRunTest and the test, then keep assertions
focused on the resulting message contents.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 07590443-6e20-47b6-b11e-1c62b7c072a6
📒 Files selected for processing (2)
pkg/ginkgo/parallel.gopkg/ginkgo/parallel_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/ginkgo/parallel.go
| Name: "test-single", | ||
| Result: extensiontests.ResultPassed, | ||
| } | ||
| data, _ := json.Marshal(result) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not ignore error returns.
The error returns from json.Marshal are being intentionally ignored, which violates the path instructions for Go files. In test code, silently suppressing these errors can lead to confusing test failures down the line (e.g., passing empty buffers into the function under test).
pkg/ginkgo/parallel_test.go#L19-L19: Handle the error fromjson.Marshal(result).pkg/ginkgo/parallel_test.go#L38-L38: Handle the error fromjson.Marshal(results).pkg/ginkgo/parallel_test.go#L54-L54: Handle the error fromjson.Marshal([]extensiontests.ExtensionTestResult{}).pkg/ginkgo/parallel_test.go#L68-L68: Handle the error fromjson.Marshal(results).
As per path instructions, "Never ignore error returns". Please assign the error to a variable and assert that it is nil (e.g., using t.Fatalf("failed to marshal JSON: %v", err)).
📍 Affects 1 file
pkg/ginkgo/parallel_test.go#L19-L19(this comment)pkg/ginkgo/parallel_test.go#L38-L38pkg/ginkgo/parallel_test.go#L54-L54pkg/ginkgo/parallel_test.go#L68-L68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/ginkgo/parallel_test.go` at line 19, Handle the json.Marshal errors in
pkg/ginkgo/parallel_test.go at lines 19, 38, 54, and 68 by capturing each error
and failing the corresponding test immediately with t.Fatalf when marshaling
fails; preserve the existing data flow after successful marshaling.
Source: Path instructions
358f7f1 to
fc864a9
Compare
openshift-ci
Bot
removed
the
needs-rebase
The parent's escalation timer now waits an extra 2 minutes beyond the child's --timeout before sending SIGINT. This lets the child's Ginkgo-level timeout fire first, producing structured per-spec output instead of the parent killing it prematurely. Also reorders the signal goroutine to check context.Canceled before sending SIGINT (avoids spurious signal to already-exited processes) and improves the fallback error wording. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
abe47bc to
5a15ed0
Compare
4 participants
https://redhat.atlassian.net/browse/ARO-25369
When a test subprocess times out and is killed, the parent can't parse its output and reports a generic "Deserialization Error" indistinguishable from a non-timeout crash.
This branch tracks whether the signal escalation goroutine fired due to a timeout using an atomic flag (set before SIGINT to avoid a race), and when it did, replaces the generic error with "test timed out after ; subprocess exit: " so CI tools like Sippy can identify timeout failures.
Summary by CodeRabbit
Summary
Bug Fixes
Tests