Conversation
- Job instance page: show only relevant fields - Job page: show output files size next to download links - Batch page: fix PHP warnings - antique_file_deleter: code cleanup (no functional change)
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="html/user/submit.php">
<violation number="1" location="html/user/submit.php:873">
P2: `filesize($path)` is called without checking that the output file exists, which can trigger PHP warnings when the file is missing. Consider guarding or using a fallback size value.</violation>
</file>
<file name="sched/antique_file_deleter.cpp">
<violation number="1" location="sched/antique_file_deleter.cpp:249">
P2: Returning immediately on any non-zero `wu.enumerate()` result causes antique file cleanup to be skipped when there are no workunits (ERR_DB_NOT_FOUND). The previous logic still deleted files older than the 31‑day cutoff in that case. Consider only aborting on real DB errors and allowing the fallback cutoff when no rows are found.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ret = wu.enumerate("where name not like '%nodelete%' order by id limit 1"); | ||
| if (ret) { | ||
| log_messages.printf(MSG_CRITICAL, "No workunits\n"); | ||
| return ret; | ||
| } | ||
| max_mod_time = time(0) - 32*86400; | ||
| if (max_mod_time > wu.create_time) { | ||
| max_mod_time = wu.create_time - 86400; | ||
| } |
There was a problem hiding this comment.
P2: Returning immediately on any non-zero wu.enumerate() result causes antique file cleanup to be skipped when there are no workunits (ERR_DB_NOT_FOUND). The previous logic still deleted files older than the 31‑day cutoff in that case. Consider only aborting on real DB errors and allowing the fallback cutoff when no rows are found.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At sched/antique_file_deleter.cpp, line 249:
<comment>Returning immediately on any non-zero `wu.enumerate()` result causes antique file cleanup to be skipped when there are no workunits (ERR_DB_NOT_FOUND). The previous logic still deleted files older than the 31‑day cutoff in that case. Consider only aborting on real DB errors and allowing the fallback cutoff when no rows are found.</comment>
<file context>
@@ -231,13 +241,19 @@ int delete_antiques_from_dir(char*dirpath, time_t mtime, uid_t uid) {
- t = wu.create_time - 86400;
+ // max_mod_time = min (create_time_of_oldest_wu, 31days_ago)
+
+ ret = wu.enumerate("where name not like '%nodelete%' order by id limit 1");
+ if (ret) {
+ log_messages.printf(MSG_CRITICAL, "No workunits\n");
</file context>
| ret = wu.enumerate("where name not like '%nodelete%' order by id limit 1"); | |
| if (ret) { | |
| log_messages.printf(MSG_CRITICAL, "No workunits\n"); | |
| return ret; | |
| } | |
| max_mod_time = time(0) - 32*86400; | |
| if (max_mod_time > wu.create_time) { | |
| max_mod_time = wu.create_time - 86400; | |
| } | |
| max_mod_time = time(0) - 32*86400; | |
| ret = wu.enumerate("where name not like '%nodelete%' order by id limit 1"); | |
| if (ret && ret != ERR_DB_NOT_FOUND) { | |
| log_messages.printf(MSG_CRITICAL, | |
| "Couldn't enumerate workunits: %d\n", ret | |
| ); | |
| return ret; | |
| } | |
| if (!ret && max_mod_time > wu.create_time) { | |
| max_mod_time = wu.create_time - 86400; | |
| } |
There was a problem hiding this comment.
Pull request overview
This pull request makes several minor improvements to the web interface for job submission and a code cleanup to the antique_file_deleter daemon. The changes include displaying only relevant fields on the job instance page based on job state, showing file sizes next to download links, fixing PHP warnings in batch processing, and refactoring the antique_file_deleter code for better readability.
Changes:
- Job instance page now conditionally displays fields (exit status, run time, CPU time, credit, validate state) based on result state
- Output file sizes are now shown next to download links for assim_move apps
- Fixed PHP warnings by initializing batch job counters that could be undefined in early returns
- Added ability to abort batches in INIT state
- Refactored antique_file_deleter.cpp for better code structure and readability (with one functional bug fix)
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sched/antique_file_deleter.cpp | Refactored if-else chain to separate if statements with early continues; improved error handling for missing workunits; renamed variable for clarity |
| html/user/submit.php | Added BATCH_STATE_INIT to abortable states; added file size display for output files in assim_move apps |
| html/user/get_output3.php | Refactored inline outfile_path function to use shared assim_move_outfile_path function |
| html/inc/submit_util.inc | Added assim_move_outfile_path function; initialized njobs_success and njobs_in_prog to fix PHP warnings; improved comments |
| html/inc/result.inc | Conditionally display result fields based on server state and outcome for better UX |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
html/user/submit.php
Outdated
| // don't show 'view' link if it's a .zip | ||
| $y = "$name: "; | ||
| $y = sprintf('%s (%s): ', | ||
| $name, size_string(filesize($path)) |
There was a problem hiding this comment.
The filesize() function is called without checking if the file exists first. This can generate PHP warnings if the output file doesn't exist. Consider wrapping the filesize call in a file_exists() check, similar to how the non-assim_move case handles this at lines 900-911. If the file doesn't exist, you could display a message like "file missing" as is done in the else branch.
| ret = wu.enumerate("where name not like '%nodelete%' order by id limit 1"); | ||
| if (ret) { | ||
| log_messages.printf(MSG_CRITICAL, "No workunits\n"); | ||
| return ret; |
There was a problem hiding this comment.
This changes the behavior when wu.enumerate() returns an error. The original code would continue execution with max_mod_time set to time(0) - 32*86400, while the new code returns early with an error. This is actually a bug fix (the new behavior is better), but contradicts the PR description's claim of "no functional change" for antique_file_deleter. Consider updating the PR description to note this functional improvement.
Summary by cubic
Improves job and batch pages for clarity, adds output file sizes, and cleans up the antique file deleter. Pages now show only relevant fields and avoid PHP warnings.
New Features
Bug Fixes & Refactors
Written for commit 85e1f8e. Summary will update on new commits.