feat: automatically download error diagnostic files for failed jobs - #628
Conversation
Add try_download_error_info() method that downloads the _last_err_file
(containing last 1000 bytes of stderr) from the remote root to local
root for any job that did not finish successfully.
This is called in run_submission() after try_download_result() and before
clean_jobs(), ensuring error diagnostics survive remote workdir cleanup.
Previously, error information was only available via get_last_error_message()
during the run (in retry logic), but was lost after clean_jobs() deleted
the remote workdir. Now the error content is:
1. Written to local_root/{job_hash}_last_err_file
2. Logged as WARNING for immediate visibility in the output
Gracefully handles missing error files and context exceptions.
dosubot
Bot
added
size:M
|
Warning Review limit reachedNext included review available in 41 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthrough
ChangesError diagnostic handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Submission
participant Job
participant MachineContext
participant RemoteStorage
Submission->>Submission: run_submission()
Submission->>Submission: enter finally cleanup flow
Submission->>Job: inspect unfinished state
Submission->>MachineContext: request remote last-error file
MachineContext->>RemoteStorage: download diagnostic content
RemoteStorage-->>MachineContext: return file content
MachineContext-->>Submission: save diagnostic locally and log content
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
for more information, see https://pre-commit.ci
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #628 +/- ##
==========================================
+ Coverage 48.33% 49.62% +1.29%
==========================================
Files 40 40
Lines 3958 4022 +64
==========================================
+ Hits 1913 1996 +83
+ Misses 2045 2026 -19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
The helper works in isolation, but its integration point does not run on the terminal-failure path, so the advertised diagnostic preservation is not achieved.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
| pass | ||
| self.handle_unexpected_submission_state() | ||
| self.try_download_result() | ||
| self.try_download_error_info() |
There was a problem hiding this comment.
[P1] Run diagnostic persistence on the terminal-failure path
This line is reached only after every preceding handle_unexpected_submission_state() succeeds. When a terminated job exhausts retries, that method raises and run_submission() exits before this call; I reproduced zero calls to try_download_error_info() in that path. The ratio_unfinished path also rewrites killed jobs to JobStatus.finished, so this helper skips them. Move the download into the exception path before re-raising (or use a carefully structured finally that preserves the original exception), and add an integration test that exhausts retries and verifies the local error file is written. The fix spans the failure-handling control flow, so a one-line suggestion here would not be complete.
Wrap the main execution block (while-loop, handle_unexpected, try_download_result) in try/finally so that try_download_error_info() is always called, even when handle_unexpected_submission_state() raises RuntimeError after exhausting retries. Previously, the error diagnostic download was only reachable on the success path, defeating the purpose of preserving error info for debugging. Add integration tests verifying error files are downloaded when retries exhaust. Addresses reviewer feedback from njzjz-bot (P1).
dosubot
Bot
added
size:L
for more information, see https://pre-commit.ci
dosubot
Bot
added
size:M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
dpdispatcher/submission.py (3)
262-263: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the no-op
finally: pass.The
finally: passblock at lines 262-263 does nothing. Delete it to keep the control flow readable.♻️ Proposed cleanup
else: self.update_submission_state() self.handle_unexpected_submission_state() - finally: - pass🤖 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 `@dpdispatcher/submission.py` around lines 262 - 263, Remove the no-op finally: pass block from the surrounding exception-handling flow in submission handling, leaving the existing try/except behavior unchanged.
266-273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the suppressed diagnostic-download failure.
The bare
except Exception: passhides all failures oftry_download_error_info().try_download_error_info()already suppresses per-job errors, so this handler only catches setup errors such as a missinglocal_root. Log at debug level so the failure is traceable. Ruff also flags this as S110/BLE001.♻️ Proposed fix
try: self.try_download_error_info() - except Exception: - pass + except Exception as e: + dlog.debug(f"Failed to download error diagnostics: {e}")🤖 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 `@dpdispatcher/submission.py` around lines 266 - 273, Update the exception handler around try_download_error_info() in the finally block to log suppressed setup failures at debug level, including the exception details. Preserve the existing best-effort behavior so diagnostic-download errors do not escape, while replacing the bare silent handler with Ruff-compliant exception logging.Source: Linters/SAST tools
299-304: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle a relative
local_rootand set an explicit encoding.If
context.local_rootis a bare relative name,os.path.dirname(local_err_path)returns an empty string andos.makedirs("")raisesFileNotFoundError. The outerexceptthen discards the diagnostic silently. Also pass an explicit encoding so the write does not depend on the platform default.♻️ Proposed fix
local_err_path = os.path.join( self.machine.context.local_root, err_file_name ) - os.makedirs(os.path.dirname(local_err_path), exist_ok=True) - with open(local_err_path, "w") as f: + os.makedirs( + self.machine.context.local_root, exist_ok=True + ) + with open(local_err_path, "w", encoding="utf-8") as f: f.write(err_content)🤖 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 `@dpdispatcher/submission.py` around lines 299 - 304, Update the error-file writing logic around local_err_path to create the parent directory only when its derived path is non-empty, so a bare relative context.local_root remains valid. Open the file with an explicit encoding while preserving the existing err_content write behavior.tests/test_download_error_info.py (1)
15-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the
exit_on_submitpath.The new
try/finallyinrun_submissionalso runstry_download_error_info()whenexit_on_submit=Truereturns early. Add a test that asserts no failure warning and no local error file for jobs inrunningstate on that path. This locks in the behavior discussed indpdispatcher/submission.py.Also applies to: 125-131
🤖 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 `@tests/test_download_error_info.py` around lines 15 - 16, Add a unit test in TestDownloadErrorInfo covering run_submission with exit_on_submit=True for a job in the running state. Assert that the path emits no failure warning and does not create a local error file, while preserving the existing try_download_error_info coverage.
🤖 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 `@dpdispatcher/submission.py`:
- Line 279: Add type annotations to the try_download_error_info method,
including its return type and any parameter annotations required by the
repository’s conventions. Keep the method’s existing behavior unchanged and
follow the annotation style used by nearby methods in
dpdispatcher/submission.py.
- Around line 236-241: Prevent the exit_on_submit success path in the submission
loop from triggering failure diagnostics in the try/finally cleanup. Update
try_download_error_info() to inspect job states and download or warn only for
genuine failure states, excluding running, waiting, and other nonterminal
states; preserve normal diagnostics for failed jobs.
In `@tests/test_download_error_info.py`:
- Around line 188-200: Update the comments in fake_check_all_finished to reflect
that the first call returns True, all subsequent calls also return True, the
while loop is skipped, and the later handle_unexpected_submission_state call
raises. Remove the contradictory description of a False second call and loop
entry while leaving the function behavior unchanged.
---
Nitpick comments:
In `@dpdispatcher/submission.py`:
- Around line 262-263: Remove the no-op finally: pass block from the surrounding
exception-handling flow in submission handling, leaving the existing try/except
behavior unchanged.
- Around line 266-273: Update the exception handler around
try_download_error_info() in the finally block to log suppressed setup failures
at debug level, including the exception details. Preserve the existing
best-effort behavior so diagnostic-download errors do not escape, while
replacing the bare silent handler with Ruff-compliant exception logging.
- Around line 299-304: Update the error-file writing logic around local_err_path
to create the parent directory only when its derived path is non-empty, so a
bare relative context.local_root remains valid. Open the file with an explicit
encoding while preserving the existing err_content write behavior.
In `@tests/test_download_error_info.py`:
- Around line 15-16: Add a unit test in TestDownloadErrorInfo covering
run_submission with exit_on_submit=True for a job in the running state. Assert
that the path emits no failure warning and does not create a local error file,
while preserving the existing try_download_error_info coverage.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d8666c9-fc17-4996-9641-3245e9473c3e
📒 Files selected for processing (2)
dpdispatcher/submission.pytests/test_download_error_info.py
- Add -> None return annotation to try_download_error_info() - Only download error diagnostics for terminated/unknown jobs, not running/waiting (fixes false 'failed' warnings when exit_on_submit=True triggers finally block) - Remove no-op 'finally: pass' block - Fix stale comments in test fake_check_all_finished Addresses coderabbit review comments.
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
The diagnostic download is still skipped on two real exhausted-retry paths because the new outer try/finally starts too late. The focused tests cover failures inside or after the polling loop, but not the two earlier calls.
Review context: Codex's quota is close to resetting, so I'm deliberately spending the remaining tokens on reviewing the open PR queue now.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
| if ratio_unfinished > 0.0 and self.check_ratio_unfinished(ratio_unfinished): | ||
| self.remove_unfinished_tasks() | ||
| break | ||
| try: |
There was a problem hiding this comment.
[P2] Start the diagnostic guard before the initial failure handling
The calls at lines 228 and 233 run before this try. A recovered job that is already terminated with exhausted retries can raise at the first call; a newly submitted job that fails quickly can raise at the second. In both cases try_download_error_info() is never reached, despite the stated goal of preserving diagnostics on exhausted retries.
Move the enclosing try/finally above the first recovery/status handling path, and add a regression where a recovered terminated job raises from the first handle_unexpected_submission_state() call.
dosubot
Bot
added
size:L
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
One inline correctness finding.
Coding agent: Codex
Codex version: codex-cli 0.149.0
Model: gpt-5.6-sol
Reasoning effort: xhigh
| if job.job_state in (JobStatus.terminated, JobStatus.unknown): | ||
| err_file_name = job.job_hash + "_last_err_file" | ||
| try: | ||
| if self.machine.context.check_file_exists(err_file_name): |
There was a problem hiding this comment.
[P2] Retrieve diagnostics through the cloud job interface
This generic path cannot find diagnostics for Bohrium/OpenAPI jobs. OpenAPIContext.check_file_exists()/read_file() and DPCloudServerContext.check_file_exists()/read_file() inspect the client-side ~/.dpdispatcher/dp_cloud_server metadata directory, while *_last_err_file is created inside the remote job workspace and is not included in the cloud output-file list. An exhausted cloud job therefore silently skips the advertised diagnostic persistence. Please expose a context-level diagnostic download operation or include and retrieve this artifact through the cloud result/log API.
Coding agent: Codex
Codex version: codex-cli 0.149.0
Model: gpt-5.6-sol
Reasoning effort: xhigh
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
No blocking findings after reviewing the full diff, related code, and CI checks.
Coding agent: Codex
Codex version: codex-cli 0.151.0
Model: gpt-5.6-sol
Reasoning effort: xhigh
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
Reviewed the complete diff and relevant surrounding code. No blocking findings. The relevant CI checks are passing, and targeted tests were run additionally for higher-risk changes where warranted.
Coding agent: Codex
Codex version: codex-cli 0.151.0
Model: gpt-5.6-sol
Reasoning effort: xhigh
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
Independent review completed. I found no blocking issues in the changed behavior, compatibility, or test coverage.
Coding agent: Codex
Codex version: codex-cli 0.151.0
Model: gpt-5.6-sol
Reasoning effort: xhigh
3 participants
Problem
When a job fails (terminated), the
_last_err_filecontaining the last 1000 bytes of stderr exists on the remote workdir but is never downloaded to local. Afterclean_jobs()runs, this diagnostic info is permanently lost.The existing
get_last_error_message()method only reads error info during the run (in retry logic), but does not persist it locally.Solution
Add
try_download_error_info()method called inrun_submission()aftertry_download_result()and beforeclean_jobs():{job_hash}_last_err_fileexists on remotelocal_root/{job_hash}_last_err_fileThis ensures error diagnostics survive remote workdir cleanup.
Tests
6 unit tests in
test_download_error_info.py.Summary by CodeRabbit