Add focused-build cost reporting and high-output conductor diagnostics#521
Open
morluto wants to merge 8 commits into
Open
Add focused-build cost reporting and high-output conductor diagnostics#521morluto wants to merge 8 commits into
morluto wants to merge 8 commits into
Conversation
Adds conductor_diagnostics.py with focused-build parsing and a synthetic high-output child, wires the diagnostics subcommands into the daemon, and includes the new tests in conductor-selftest. Also removes the stray generated testwarn binary and fixes harness bugs: run_high_output now honors an explicit lines=0, and _FocusedBuildParser resolves repo-relative warning/error source paths to modules.
The output summary now includes summaryDurationSeconds and makes WARNING_RE case-insensitive, so the summary version is bumped to 2. Update the help text for the new diagnostics subcommands and add tests for the summary version and duration fields.
…ern. - Add Scripts/conductor_diagnostics.py and the new diagnostic test files (test_conductor_diagnostics.py, test_conductor_high_output.py) to the contribution preflight control-plane pattern so control-plane changes run conductor-selftest. - Correct run_focused_build docstring: remove incorrect `or test` wording. - Add a 30-second deadline to the resource-polling loop in test_conductor_high_output.py. - Document symlink handling in directory_size_bytes docstring.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| "directory": str(build_dir), | ||
| "observedBefore": observed_state, | ||
| "sizeBytes": size, | ||
| "sizeHuman": format_bytes(size), |
There was a problem hiding this comment.
Bug: The run_focused_build function returns the Swift process's exit code, violating its docstring which states it should return 0 if the diagnostic ran, regardless of build success.
Severity: MEDIUM
Suggested Fix
Update run_focused_build to align with its documentation. The function should return 0 if the diagnostic itself ran, and 1 only if an error prevented the diagnostic from running. The exit code of the underlying Swift process can be logged or handled internally but should not be the return value that determines the job's success or failure.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: Scripts/conductor_diagnostics.py#L449
Potential issue: The function `run_focused_build` violates its documented contract. The
docstring states it should return 0 if the diagnostic ran, regardless of the underlying
build/test exit code. However, the implementation returns the exit code from the Swift
process. This causes the parent conductor job to be incorrectly marked as "failed" when
the Swift build fails, even though the diagnostic itself executed successfully. This
misrepresents the outcome of the diagnostic run, as its purpose may be to measure build
failures, not just report successful builds.
Also affects:
Scripts/test_conductor_diagnostics.py:188~190
Did we get this right? 👍 / 👎 to inform future reviews.
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
1 participant
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.
Why
A cold focused test spent 315.49 seconds building and 2.515 seconds running tests. The same run emitted roughly 11,000 warning lines from about 451 source locations and produced a 97,209-line log.
During that run, one
conductor job waitrequest timed out while the child continued and the daemon remained alive. Existing evidence did not identify output handling, daemon scheduling, summary generation, or machine pressure as the cause. Changing the output pipeline before reproducing the failure would risk hiding it.This PR adds the measurements needed to distinguish those explanations.
Supports #519. Investigates #518.
What changes
conductor diagnostics focused-buildfor warning ownership, output size, build/link/XCTest timing, compiler job counts, peak child RSS, and scratch state.conductor diagnostics high-outputfor status, wait, cancellation, raw-log completeness, resources, and terminal-summary behavior.warning:output.Result
A synthetic run exceeding 100,000 lines did not reproduce the
job waittimeout. This PR therefore does not guess at an output-pipeline fix. It leaves a reproducible probe for representative Swift load.Review order
Scripts/conductor_diagnostics.pyScripts/conductor.pyVerification
make conductor-selftestmake guardrails