Repository navigation
fix: avoid race in IngestionSmokeTest.test_streamLogs_ofCancelledTask - #20549
rishi-rana wants to merge 3 commits into
Conversation
The peon pushes its task log after the process exits, so TaskLogStreamer#streamTaskLog can return a present-but-empty stream before the expected content is written. Poll until the log contains the expected line instead of just until the stream is present. Fixes apache#20435. Assisted with AI Claude.
|
Left a comment here: #20435 (comment) I'll investigate that more. |
| taskId, runDurationMillis | ||
| ); | ||
|
|
||
| // The peon pushes its log after the process exits, so the stream can become present before it |
There was a problem hiding this comment.
This comment isn't accurate, no peon in this setup.
There was a problem hiding this comment.
Good catch, thanks — you're right, there's no peon here. EmbeddedIndexer runs the task in-process and the log is served over HTTP straight from its own process log file (HttpRemoteTaskRunner#streamTaskLog). Updated the comment to describe that instead.
There was a problem hiding this comment.
Sorry that was a oversight on my side
@ykisana pointed out this test's setup has no peon — EmbeddedIndexer runs tasks in-process and the log is served over HTTP from its own process log file (HttpRemoteTaskRunner#streamTaskLog). Fix the comment to describe that mechanism instead of a peon process-exit push. Assisted with AI Claude.
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
🟡 Changes recommended
The retry fixes the log-visibility race, but each retry opens a new task-log stream without closing it. File-backed streams can leak descriptors across retries and make embedded test runs flaky.
Reviewed 1 of 1 changed files; finding in 1 file.
Validation: git diff --check passed.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
This is an automated review by Codex GPT-5.6-Luna(max)
After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.
Each retry of the waitForResult poll opened a new InputStream from streamTaskLog without closing it. Close it with try-with-resources after reading, so retries don't leak file/HTTP response descriptors. Assisted with AI Claude.
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
🟢 Approval recommended
No actionable issues found in this review. The current head closes each present task-log InputStream after every polling read, so retries no longer accumulate file or HTTP response descriptors while preserving the wait-until-content behavior.
Reviewed 1 of 1 changed files, including the full diff, the incremental follow-up change, the embedded test setup, the retry waiter, and the task-log streaming implementations and call sites.
Validation: git diff --check FETCH_HEAD HEAD passed.
This is an automated review by Codex GPT-5.6 Luna(Max)
3 participants
Fixes #20435.
Description
test_streamLogs_ofCancelledTaskpollsTaskLogStreamer#streamTaskLogwithwaitForResult(..., Optional::isPresent). In the Docker-based embedded test setup, the peon pushes its log after the process exits, so the stream can become present (e.g. an empty file/handle) before the expected content has actually been written. This makes the test'sassertFalse(logs.isEmpty())fail intermittently.This PR moves the log read into the polling supplier itself and changes the match condition to wait until the log content contains the expected line (
Running task[%s] for [%d] millis), rather than just waiting for the stream to be present.Fixed the race in
test_streamLogs_ofCancelledTaskPolls on log content instead of stream presence, so the test waits out the delayed log push instead of racing it.
Release note
N/A — test-only change, no user-facing impact.
Key changed/added classes in this PR
IngestionSmokeTestThis PR has:
Assisted with AI Claude.