Fix parser crash on unrecognized lftp status lines (#253)#258
Fix parser crash on unrecognized lftp status lines (#253)#258nitrobass24 merged 1 commit intomasterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChanged LftpJobStatusParser.__parse_jobs from a staticmethod to an instance method and made the parser tolerant of "orphan" progress lines by detecting and skipping them (logging a warning) instead of raising on unrecognized lines. Added tests covering orphan progress and truly unrecognized lines. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/python/lftp/job_status_parser.py`:
- Around line 229-231: Replace direct
logging.getLogger("LftpJobStatusParser").warning calls with the parser instance
logger so warnings follow the caller's logger hierarchy: in the __parse_jobs
method (and the similar occurrence at the block around lines 497-499), change
those warnings to self.logger.warning(...) and ensure set_base_logger() provided
logger continues to be used by the class (i.e., reference the instance attribute
self.logger rather than creating a new logger).
- Around line 220-232: The current check in LftpJobStatusParser (around prev_job
/ pget_header_m / mirror_header_m / mirror_fl_header_m) is too broad and
silently skips any unrecognized line; restrict this to only known orphan
progress-line formats by introducing/using a dedicated matcher (e.g.,
orphan_progress_m) and skip only when that regex matches the line, otherwise
treat it as a real parse failure (raise or return an error so the caller
increments __consecutive_status_errors). Apply the same narrowing fix to the
other identical branch around lines 494-499 so only the recognized orphan
progress pattern is suppressed and all other unknown lines propagate as errors;
reference prev_job, pget_header_m, mirror_header_m, mirror_fl_header_m, and the
logger "LftpJobStatusParser" when locating the spots to modify.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7dedebc9-d52a-41c2-b175-3df4629f2712
📒 Files selected for processing (2)
src/python/lftp/job_status_parser.pysrc/python/tests/unittests/test_lftp/test_job_status_parser.py
The lftp job status parser crashed with a ValueError on unrecognized lines like "3.0K/s eta:3m [Receiving data]", which propagated as LftpJobStatusParserError and killed the Docker container after 3 consecutive failures. Changed both crash points in __parse_jobs to log a warning and skip unrecognized lines instead of raising ValueError. This makes the parser resilient to unexpected lftp output formats while still parsing all recognized job statuses. Fixes #253 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fc296e1 to
63ea792
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Summary
3.0K/s eta:3m [Receiving data], which lftp emits during initial connection/data reception__parse_jobsto log a warning and skip unrecognized lines instead of throwingValueErrorFixes #253
Test plan
test_unrecognized_status_line_skipped— unrecognized line after a valid job is skipped, valid job still parsedtest_bare_unrecognized_line_skipped— bare unrecognized line with no valid jobs returns empty list[Receiving data]status lines🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests