feat: time firmware downloads and split their failures by cause#3407
Open
chet wants to merge 1 commit into
Open
feat: time firmware downloads and split their failures by cause#3407chet wants to merge 1 commit into
chet wants to merge 1 commit into
Conversation
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
(=^･ω･^=) ✅ Action performedFull review finished. |
Contributor
Summary by CodeRabbit
WalkthroughFirmware downloads now classify failures, emit structured completion events with duration metrics, clean up staging files, and use deterministic tests for outcome reporting, labels, logging, and timing. ChangesFirmware download reporting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FirmwareDownloader
participant download_and_publish
participant DownloadFinished
FirmwareDownloader->>download_and_publish: download, verify, and publish firmware
download_and_publish-->>FirmwareDownloader: categorized outcome and error
FirmwareDownloader->>DownloadFinished: emit completion event with duration and context
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@crates/firmware/src/downloader.rs`:
- Around line 196-198: Update the cleanup call in the result error branch to use
the filesystem operation’s `.ok()` form instead of assigning its result to `_`,
while preserving the existing behavior of removing dst_filename and ignoring
cleanup failures.
- Around line 306-309: Update the File::create call in the downloader flow to
pass dst_filename directly instead of borrowing it again. Preserve the existing
error wrapping and DownloadOutcome::Io mapping.
- Around line 66-78: Update DownloadFinished construction and event emission to
sanitize FirmwareFileArtifact.url before assigning it to the #[context] url
field. Ensure query parameters and other embedded credentials or tokens are
removed while preserving the usable URL location, and apply the same redacted
value on both success and failure paths.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a874f290-4e45-4103-8ac9-10b82d1c79ba
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/firmware/Cargo.tomlcrates/firmware/src/downloader.rscrates/firmware/src/tests/downloader.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
chet
force-pushed
the
gh-issue-3174-downloader
branch
from
dcadefe to
1af83db
Compare
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
(=^･ω･^=) ✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/firmware/src/tests/downloader.rs (1)
272-360: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffStatus/Transfer/Io outcomes are only verified via hand-built events, not the real
download()path.
download_finished_records_duration_and_owns_the_completion_lineconstructsDownloadFinishedevents directly rather than driving them throughdownload()/download_and_publish(). Combined with the e2e tests, onlyOk,Checksum, andFetchare exercised through the real code path — the HTTP-status, stream-transfer, and local-IO branches indownload()are covered only by the enum/label mapping, not by actual failing transfers. A small mock HTTP server (e.g. a boundTcpListenerreturning a fixed 404, or a body that closes mid-stream) would close this gap, though it's a moderate lift for a codebase where the branch logic itself is simplematch-driven classification.🤖 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 `@crates/firmware/src/tests/downloader.rs` around lines 272 - 360, Extend the real download-path tests around download() or download_and_publish() to trigger HTTP status, stream-transfer, and local-IO failures, rather than validating Status, Transfer, and Io only through hand-built DownloadFinished events. Use a small controlled mock HTTP server or equivalent fixtures to produce a 404, mid-stream disconnect, and local write failure, then assert each path emits the expected outcome, duration metric, and completion log.
🤖 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 `@crates/firmware/src/downloader.rs`:
- Around line 226-250: Update download and download_and_publish so every
diagnostic that includes the URL uses loggable_url(url) instead of the raw URL
before being wrapped or returned in DownloadFinished.error. Add a regression
test covering a failed download with a query-string URL and verify the raw query
is absent while the sanitized URL remains.
---
Nitpick comments:
In `@crates/firmware/src/tests/downloader.rs`:
- Around line 272-360: Extend the real download-path tests around download() or
download_and_publish() to trigger HTTP status, stream-transfer, and local-IO
failures, rather than validating Status, Transfer, and Io only through
hand-built DownloadFinished events. Use a small controlled mock HTTP server or
equivalent fixtures to produce a 404, mid-stream disconnect, and local write
failure, then assert each path emits the expected outcome, duration metric, and
completion log.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c0919bc7-794e-4be0-bd7a-9c29b105e7d9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/firmware/Cargo.tomlcrates/firmware/src/downloader.rscrates/firmware/src/tests/downloader.rs
Firmware artifact downloads are the api process's longest-running background HTTP work -- hundreds of megabytes per artifact -- and until now their only trace was log lines. `carbide_firmware_download_duration_seconds{outcome}` times every attempt, and its `_count` series split by outcome is the fetch/status/checksum failure-counter set in one metric.
- `outcome` distinguishes what actually broke: `fetch` (the request never got a response), `status` (non-2xx), `transfer` (the body broke mid-stream), `checksum` (SHA-256 mismatch), `io` (staging file, segment write, rename-into-place) -- the download path is retyped to tag its errors with the cause instead of flattening everything into one report.
- The event owns the four historical lines at their levels (the completion INFO and the failure/checksum/rename ERRORs), with the full error chain as a structured field -- richer than the old lines, which printed only the outermost message. One line per attempt now: a failed-verify attempt used to log the completion INFO and then the checksum ERROR; it logs only the ERROR. The spawned task has a single emit-and-cleanup site: `download_and_publish` chains download, verification, and rename.
- The no-URL config-error path stays log-only: it re-fires every availability poll without running a download, so counting it would flood zero-duration samples.
- No catalogue row: the integration environment never exercises a download (empty firmware directory, mocks already at their desired versions), and the scrape-derived catalogue lists only what the regen mints -- the same treatment the machine-update Failed-state histogram got.
Tests added!
This supports NVIDIA#3174
Signed-off-by: Chet Nichols III <chetn@nvidia.com>
chet
force-pushed
the
gh-issue-3174-downloader
branch
from
1af83db to
5e23261
Compare
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/firmware/src/downloader.rs (2)
198-224: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the
filename_stringclone by reordering.
clear_download_stateonly needs&filename_string, but it runs afterfilename_string.clone()is moved into the event. Swapping the order letsemittake ownership of the original string, dropping the clone.As per coding guidelines: "Struct initialization ordering: Sometimes just moving the order of parameters to a struct literal can avoid a clone."♻️ Proposed reorder
- emit(DownloadFinished { - outcome, - took: started.elapsed(), - url: loggable_url(&url), - filename: filename_string.clone(), - error, - }); - actual - .lock() - .unwrap() - .clear_download_state(&filename_string); + actual + .lock() + .unwrap() + .clear_download_state(&filename_string); + emit(DownloadFinished { + outcome, + took: started.elapsed(), + url: loggable_url(&url), + filename: filename_string, + error, + });🤖 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 `@crates/firmware/src/downloader.rs` around lines 198 - 224, Reorder the cleanup and event construction in the download task so clear_download_state(&filename_string) runs before filename_string is moved into DownloadFinished. Then pass the original filename_string to the event instead of cloning it, preserving the existing cleanup and emission behavior.Source: Coding guidelines
330-378: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a direct
DownloadErrorliteral overfail(outcome)(report)at standalone construction sites.
fail()is documented as a.map_err()helper, but it's also called and immediately invoked (e.g.fail(DownloadOutcome::Status)(eyre!(...))here and at the dirname-lookup site around line 304) to build aDownloadErrordirectly. A plain struct literal (DownloadError { outcome: DownloadOutcome::Status, report: eyre!(...) }) is more direct at those call sites and keepsfail()'s use scoped tomap_errchaining as documented.As per coding guidelines: "Prefer struct literals for 'plain old data' types."
🤖 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 `@crates/firmware/src/downloader.rs` around lines 330 - 378, The standalone error constructions in the downloader, including the non-success response branch and the dirname-lookup failure, should use direct DownloadError struct literals instead of invoking fail(outcome)(report). Set the corresponding outcome and eyre report fields explicitly, while keeping fail() for map_err chaining only.Source: Coding guidelines
crates/firmware/src/tests/downloader.rs (1)
32-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConvert to a table-driven test with
check_values.
loggable_urlis a pure conversion (URL in, sanitized URL out) with two cases, which is exactly whatvalue_scenarios!/check_valuesis for — and the pattern is already used elsewhere in this file (download_outcome_labels_render_snake_case).As per coding guidelines: "prefer table-driven tests using grouped `carbide-test-support` scenarios (`scenarios!`/`value_scenarios!`) or explicit cases (`check_cases`/`check_values`) for parsers, validators, conversions, and similar logic."♻️ Proposed table-driven test
-#[test] -fn loggable_url_drops_query_parameters() { - assert_eq!( - loggable_url("https://firmware.example/bmc.fwpkg?X-Amz-Signature=secret"), - "https://firmware.example/bmc.fwpkg", - ); - assert_eq!( - loggable_url("file:///tmp/bmc.fwpkg"), - "file:///tmp/bmc.fwpkg" - ); -} +#[test] +fn loggable_url_drops_query_parameters() { + check_values( + [ + Check { + scenario: "presigned URL with query string", + input: "https://firmware.example/bmc.fwpkg?X-Amz-Signature=secret", + expect: "https://firmware.example/bmc.fwpkg".to_string(), + }, + Check { + scenario: "file URL without query string", + input: "file:///tmp/bmc.fwpkg", + expect: "file:///tmp/bmc.fwpkg".to_string(), + }, + ], + loggable_url, + ); +}🤖 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 `@crates/firmware/src/tests/downloader.rs` around lines 32 - 42, Convert loggable_url_drops_query_parameters into a table-driven value test using the existing value_scenarios!/check_values pattern demonstrated by download_outcome_labels_render_snake_case. Include both the signed URL sanitization case and the file URL unchanged case, preserving their expected outputs.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@crates/firmware/src/downloader.rs`:
- Around line 198-224: Reorder the cleanup and event construction in the
download task so clear_download_state(&filename_string) runs before
filename_string is moved into DownloadFinished. Then pass the original
filename_string to the event instead of cloning it, preserving the existing
cleanup and emission behavior.
- Around line 330-378: The standalone error constructions in the downloader,
including the non-success response branch and the dirname-lookup failure, should
use direct DownloadError struct literals instead of invoking
fail(outcome)(report). Set the corresponding outcome and eyre report fields
explicitly, while keeping fail() for map_err chaining only.
In `@crates/firmware/src/tests/downloader.rs`:
- Around line 32-42: Convert loggable_url_drops_query_parameters into a
table-driven value test using the existing value_scenarios!/check_values pattern
demonstrated by download_outcome_labels_render_snake_case. Include both the
signed URL sanitization case and the file URL unchanged case, preserving their
expected outputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 128570bd-9c63-4ddb-9818-c75db091e339
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/firmware/Cargo.tomlcrates/firmware/src/downloader.rscrates/firmware/src/tests/downloader.rs
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.
Firmware artifact downloads are the api process's longest-running background HTTP work, and until now their only trace was log lines.
carbide_firmware_download_duration_seconds{outcome}times every attempt; its_countseries split by outcome is the fetch/status/checksum failure-counter set in one metric.outcomenames what actually broke:fetch(no response),status(non-2xx),transfer(mid-stream break),checksum(SHA-256 mismatch),io(staging/rename) -- the download path now tags its errors with the cause instead of flattening everything into one report.okattempt's duration spans fetch, verification, and publish (the description says so), so each outcome series reads consistently.Tests added!
This supports #3174