graham/prop
Watch
1
0
Fork
You've already forked prop
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Fix weather client file handling and lookup bugs #19

Merged
graham merged 2 commits from fix/weather-clients into main 2026-09-22 12:05:12 -05:00
Conversation 6 Commits 2 Files changed 15 +495 -113
graham commented 2026-09-22 11:23:16 -05:00
Owner
Copy link

Audit weather-clients group.

  • HRDPS temp GRIB leak on wgrib2 failure; Sgrid fd leak on bad header; TempFile leaves file on failed write.
  • ScalarFile/MsFootprints prunes now count real deletions via FilePrune and log failures.
  • download_grib_ranges_to_file propagates write failures.
  • NARR lookups snap to the 3-hourly storage grid — pre-2014 contacts now find profiles.
  • HRRR .idx errors no longer memoized for an hour (publish-lag 404s blocked Skew-T).
  • SkewtParams no longer raises on nil dewpoint between 700-400 mb.

Verified: targeted tests green; full suite passes on combined tree.

Audit weather-clients group. - HRDPS temp GRIB leak on wgrib2 failure; Sgrid fd leak on bad header; TempFile leaves file on failed write. - ScalarFile/MsFootprints prunes now count real deletions via FilePrune and log failures. - download_grib_ranges_to_file propagates write failures. - NARR lookups snap to the 3-hourly storage grid — pre-2014 contacts now find profiles. - HRRR .idx errors no longer memoized for an hour (publish-lag 404s blocked Skew-T). - SkewtParams no longer raises on nil dewpoint between 700-400 mb. Verified: targeted tests green; full suite passes on combined tree.
Fix weather client file handling and lookup bugs
All checks were successful
skippy-bot/review Skippy review: clean — no open findings
Details
5b92415b3b
 
- HRDPS temp GRIB no longer leaks when wgrib2 extraction fails
- ScalarFile/MsFootprints prunes count real deletions via FilePrune and log
  failures instead of swallowing File.rm errors
- Sgrid.read_point_from_path closes its fd on a bad header
- download_grib_ranges_to_file propagates write failures instead of :ok
- TempFile.write removes the file on a failed write
- NARR lookups snap to the 3-hourly storage grid so pre-2014 contacts match
- HRRR .idx errors are no longer memoized for an hour (publish-lag 404s)
- SkewtParams no longer raises on a nil dewpoint between 700-400 mb
skippy-bot reviewed 2026-09-22 11:28:49 -05:00
skippy-bot left a comment
Copy link

🤖 Skippy PR review

2 findings — none blocking.

Severity Location Issue
🔵 Suggestion lib/microwaveprop/propagation/pgrid.ex:136 The fd leak fixed in Sgrid.read_point_from_path also exists in Pgrid.read_point/3 (outside this diff)
🔵 Suggestion lib/microwaveprop/canopy.ex:134 Canopy.prune_older_than still counts candidates and swallows delete failures (outside this diff)

Findings outside the commentable diff

  • 🔵 Suggestion — lib/microwaveprop/propagation/pgrid.ex:136 — The fd leak fixed in Sgrid.read_point_from_path also exists in Pgrid.read_point/3 (outside this diff)
    read_header/1 returns the bare atom :error for a short or foreign header, so the else _ -> nil branch returns without :file.close(fd) and leaks one raw fd per call. Identical defect to the one fixed in Sgrid.read_point_from_path/3 here. Reachable from ProfilesFile.read_point/3, which guards with Pgrid.exists?/1 (a plain File.exists?), so one truncated or wrong-version .pgrid left by a killed writer leaks an fd on every point lookup (CurrentConditions calls it per point) until the pod hits EMFILE and every :file.open in the process fails. The try/after shape from the Sgrid fix closes it.
  • 🔵 Suggestion — lib/microwaveprop/canopy.ex:134 — Canopy.prune_older_than still counts candidates and swallows delete failures (outside this diff)
    do_delete/1 discards the File.rm/File.rm_rf result with no log line, and prune_file/4 increments the count regardless, so a canopy store that never shrinks still reports a healthy prune. That is the exact failure mode FilePrune was added for, and PropagationPruneWorker.process/1 calls this three lines below the MsFootprints call this PR fixed, logging canopy_deleted the same way. Files via FilePrune.delete_all/2, directories through the rm_rf-counted path ScalarFile uses here.

Reviewed 5b92415b3b51. Comment skippy review to re-run.

### 🤖 Skippy PR review **2 findings** — none blocking. | Severity | Location | Issue | | --- | --- | --- | | 🔵 Suggestion | `lib/microwaveprop/propagation/pgrid.ex:136` | The fd leak fixed in Sgrid.read_point_from_path also exists in Pgrid.read_point/3 (outside this diff) | | 🔵 Suggestion | `lib/microwaveprop/canopy.ex:134` | Canopy.prune_older_than still counts candidates and swallows delete failures (outside this diff) | #### Findings outside the commentable diff - **🔵 Suggestion — `lib/microwaveprop/propagation/pgrid.ex:136` — The fd leak fixed in Sgrid.read_point_from_path also exists in Pgrid.read_point/3 (outside this diff)** `read_header/1` returns the bare atom `:error` for a short or foreign header, so the `else _ -> nil` branch returns without `:file.close(fd)` and leaks one raw fd per call. Identical defect to the one fixed in `Sgrid.read_point_from_path/3` here. Reachable from `ProfilesFile.read_point/3`, which guards with `Pgrid.exists?/1` (a plain `File.exists?`), so one truncated or wrong-version `.pgrid` left by a killed writer leaks an fd on every point lookup (`CurrentConditions` calls it per point) until the pod hits EMFILE and every `:file.open` in the process fails. The `try/after` shape from the Sgrid fix closes it. - **🔵 Suggestion — `lib/microwaveprop/canopy.ex:134` — Canopy.prune_older_than still counts candidates and swallows delete failures (outside this diff)** `do_delete/1` discards the `File.rm`/`File.rm_rf` result with no log line, and `prune_file/4` increments the count regardless, so a canopy store that never shrinks still reports a healthy prune. That is the exact failure mode `FilePrune` was added for, and `PropagationPruneWorker.process/1` calls this three lines below the `MsFootprints` call this PR fixed, logging `canopy_deleted` the same way. Files via `FilePrune.delete_all/2`, directories through the rm_rf-counted path `ScalarFile` uses here. <sub>Reviewed `5b92415b3b51`. Comment `skippy review` to re-run.</sub> <!-- skippy-pr-review -->
Route Canopy.prune_older_than through FilePrune (review finding)
All checks were successful
skippy-bot/review Skippy review: clean — no open findings
Details
99945e7e6c
 
Same defect class as the ScalarFile/MsFootprints fixes in this PR:
prune_file counted every expired candidate regardless of the File.rm
result and logged nothing, so a canopy store that never shrank still
reported a healthy prune. Files now go through FilePrune.delete_all/2;
directories through an rm_rf path with the same count-and-log contract.
graham commented 2026-09-22 12:01:17 -05:00
Author
Owner
Copy link

Addressed in 99945e7e:

  • Canopy.prune_older_than — routed through FilePrune.delete_all/2 for files and an rm_rf-counted path for dirs, same contract as the ScalarFile fix. Failures now log and are excluded from the count.
  • Pgrid.read_point fd leak — already fixed on #18 (fix/propagation-pipeline), which owns that file; same try/after shape as the Sgrid fix here.
Addressed in 99945e7e: - **Canopy.prune_older_than** — routed through `FilePrune.delete_all/2` for files and an rm_rf-counted path for dirs, same contract as the ScalarFile fix. Failures now log and are excluded from the count. - **Pgrid.read_point fd leak** — already fixed on #18 (`fix/propagation-pipeline`), which owns that file; same try/after shape as the Sgrid fix here.
graham commented 2026-09-22 12:01:23 -05:00
Author
Owner
Copy link

skippy review

skippy review
skippy-bot commented 2026-09-22 12:02:39 -05:00
First-time contributor
Copy link

🤖 Skippy PR review — follow-up on 99945e7e6cdc

No new findings in the new commit.

  • Resolved: Canopy.prune_older_than/1 now sends files through FilePrune.delete_all/2 and directories through a counted rm_rf that logs failures, so the value handed to maybe_rmdir_empty/2 is now real deletions. Checked File.rm_rf/1 against the stdlib: the removed list includes the root path, so path in removed counts a directory correctly (it is not a permanent false, which would have re-created the undercount for dirs).
  • Still open (from the first review, and outside this diff): Pgrid.read_point/3 still leaks an fd when read_header/1 returns :error, lib/microwaveprop/propagation/pgrid.ex:136.

Reviewed 99945e7e6cdc. Comment skippy review to re-run.

### 🤖 Skippy PR review — follow-up on `99945e7e6cdc` No new findings in the new commit. - **Resolved:** `Canopy.prune_older_than/1` now sends files through `FilePrune.delete_all/2` and directories through a counted `rm_rf` that logs failures, so the value handed to `maybe_rmdir_empty/2` is now real deletions. Checked `File.rm_rf/1` against the stdlib: the removed list includes the root path, so `path in removed` counts a directory correctly (it is not a permanent false, which would have re-created the undercount for dirs). - **Still open** (from the first review, and outside this diff): `Pgrid.read_point/3` still leaks an fd when `read_header/1` returns `:error`, `lib/microwaveprop/propagation/pgrid.ex:136`. <sub>Reviewed `99945e7e6cdc`. Comment `skippy review` to re-run.</sub> <!-- skippy-pr-review -->
graham force-pushed fix/weather-clients from 99945e7e6c
All checks were successful
skippy-bot/review Skippy review: clean — no open findings
Details
to b34f48f65a
All checks were successful
skippy-bot/review Skippy review: clean — no open findings
Details
2026-09-22 12:04:19 -05:00 Compare
skippy-bot commented 2026-09-22 12:04:51 -05:00
First-time contributor
Copy link

Reviewed 99945e7: the Canopy finding from the last review is fixed. Files now go through FilePrune (only real deletions counted, failures logged with path and reason) and directories through an rm_rf path with the same count-and-log contract, so a canopy store that cannot shrink is no longer reported as a healthy prune. Nothing new in the commit - the HRDPS/Sgrid/TempFile/HRRR-idx/NARR/SkewtParams changes reviewed earlier all still read correct at this head.

Still open from the last review (suggestion, outside this diff, unchanged at this head): Pgrid.read_point/3 (lib/microwaveprop/propagation/pgrid.ex) leaks its raw fd when read_header/1 fails, the same defect fixed in Sgrid.read_point_from_path/3 here. Suggestions do not hold the check red.

Reviewed `99945e7`: the Canopy finding from the last review is fixed. Files now go through `FilePrune` (only real deletions counted, failures logged with path and reason) and directories through an `rm_rf` path with the same count-and-log contract, so a canopy store that cannot shrink is no longer reported as a healthy prune. Nothing new in the commit - the HRDPS/Sgrid/TempFile/HRRR-idx/NARR/SkewtParams changes reviewed earlier all still read correct at this head. Still open from the last review (suggestion, outside this diff, unchanged at this head): `Pgrid.read_point/3` (`lib/microwaveprop/propagation/pgrid.ex`) leaks its raw fd when `read_header/1` fails, the same defect fixed in `Sgrid.read_point_from_path/3` here. Suggestions do not hold the check red. <!-- skippy-pr-review -->
graham merged commit af2888713d into main 2026-09-22 12:05:12 -05:00
graham deleted branch fix/weather-clients 2026-09-22 12:05:13 -05:00
skippy-bot commented 2026-09-22 12:08:30 -05:00
First-time contributor
Copy link

Rechecked b34f48f6 — no new findings.

Both earlier findings are fixed:

  • Canopy.prune_older_than/1 now counts real deletions through FilePrune and logs rm_rf failures (lib/microwaveprop/canopy.ex:151); fixed in this commit.
  • Pgrid.read_point/3 no longer leaks the fd when read_header/1 fails — it uses the same try/after shape as the Sgrid fix here. That landed on main with #18 and the rebase picked it up, which is also why this ran as a full-diff review: 99945e7e is no longer in the branch history.

No blocking findings left.

Rechecked `b34f48f6` — no new findings. Both earlier findings are fixed: - `Canopy.prune_older_than/1` now counts real deletions through `FilePrune` and logs `rm_rf` failures (`lib/microwaveprop/canopy.ex:151`); fixed in this commit. - `Pgrid.read_point/3` no longer leaks the fd when `read_header/1` fails — it uses the same `try/after` shape as the Sgrid fix here. That landed on main with #18 and the rebase picked it up, which is also why this ran as a full-diff review: `99945e7e` is no longer in the branch history. No blocking findings left. <!-- skippy-pr-review -->
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
graham/prop!19
No description provided.