Skip FileDownloader monkey-patch when native retry support exists

[Jason2866]

Summary

Detects if platformio-core's FileDownloader already has native retry support by checking for the RETRY class attribute. When present, the monkey-patch from #463 is skipped to avoid:

• Double retries: native urllib3.Retry (5×) + monkey-patch (5×) = up to 25 attempts with ~62s+ total wait
• Exception mismatch: native code raises IOError on network failures, not PackageException, so the monkey-patch's except PackageException wouldn't catch them anyway
• Redundant cleanup: closing already-managed session/response objects

How it works

if hasattr(FileDownloader, 'RETRY'):
    logger.debug('FileDownloader has native retry support, skipping monkey-patch')
    return

• Unmodified platformio-core: RETRY attribute absent → monkey-patch applies as before
• Enhanced platformio-core: RETRY attribute present → monkey-patch skipped, native retry handles everything

Checklist

• The pull request is done against the latest develop branch
• Only relevant files were touched
• Only one feature/fix was added per PR
• I accept the CLA

Summary by CodeRabbit

• Chores
  • Optimized internal file download retry mechanism to detect and utilize native platform support when available, streamlining the download handling process.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: efbb8f39-d057-414e-bd92-470ef73b9222

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• ✅ Review completed - (🔄 Check again to review again)

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch skip-monkey-patch-with-native-retry

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Jason2866]

@coderabbitai analyze retry download function together with PR Jason2866/platformio-core#34

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the details of PR `#34` from Jason2866/platformio-core
gh pr view 34 --repo Jason2866/platformio-core --json title,body,files,additions,deletions,state

# Get the diff of PR `#34`
gh pr diff 34 --repo Jason2866/platformio-core

Length of output: 9691

---

🏁 Script executed:

#!/bin/bash
# Read the patch_file_downloader function and surrounding context in platform.py
rg -n "patch_file_downloader\|PackageException\|RETRY\|FileDownloader\|max_retries\|retry" platform.py -A 5 -B 2

# Also get the full function
ast-grep --pattern 'def patch_file_downloader($_) {
  $$$
}'

Length of output: 0

---

🏁 Script executed:

#!/bin/bash
# Find platform.py
fd "platform.py"

# Also search for patch_file_downloader across all files
rg -rn "patch_file_downloader\|FileDownloader\|RETRY" --type py -A 3 -B 2

Length of output: 11

---

🏁 Script executed:

#!/bin/bash
cat platform.py

Length of output: 53602

---

@Jason2866 Here is a detailed analysis of how patch_file_downloader() (PR #467) and the native retry implementation in FileDownloader (PR #34, Jason2866/platformio-core) interact:

---

Combined Analysis: patch_file_downloader() × Native FileDownloader Retry

How the Guard Works

The hasattr(FileDownloader, 'RETRY') check is correct and reliable. In PR #34, RETRY is defined at the class body level:

class FileDownloader:
    RETRY = Retry(total=5, ...)

This means hasattr(FileDownloader, 'RETRY') returns True immediately, and patch_file_downloader() returns early, leaving the native retry untouched. ✅

---

Double-Retry Prevention

Without the guard, if both were simultaneously active, retries could compound:

Layer	Mechanism	Max attempts
urllib3 HTTPAdapter	Status codes [413,429,500,502,503,504]	5
_stream_with_retry	Mid-stream RequestException	5
Monkey-patch __init__	PackageException on init	5

The guard collapses this to just the two native layers (urllib3 connection-level + stream-level), which is the intended design. ✅

---

Exception Type Mismatch — Avoided

Path	Exception raised
Monkey-patch	catches PackageException
Native _stream_with_retry after max retries	raises IOError
Native __init__ on bad status	raises PackageException

If both were active, PackageException from init would be caught by the monkey-patch for a retry, but IOError from _stream_with_retry would not — leading to unexpected propagation. The guard prevents this inconsistency. ✅

---

⚠️ Status Code Coverage Gap (404)

This is the most notable behavioral difference to be aware of:

• Monkey-patch: retries on any PackageException, including those triggered by a 404 response in __init__. This was intentional — GitHub sometimes returns transient 404s.
• Native urllib3 retry: status_forcelist = [413, 429, 500, 502, 503, 504] — 404 is not included.

When the guard skips the monkey-patch, a transient GitHub 404 will not be retried at the urllib3 level. The __init__ will raise PackageException, which nothing now catches for a retry.

Given your previous clarification that packages are verified before download is attempted (so real 404s don't happen in practice), this gap may be acceptable. But if GitHub's transient 404s are still a concern, PR #34 could add 404 to status_forcelist:

RETRY = Retry(
    total=5,
    backoff_factor=1,
    status_forcelist=[404, 413, 429, 500, 502, 503, 504],  # add 404
    raise_on_status=False,
)

---

Backoff Schedule Comparison

Attempt	Monkey-patch (2^(n+1))	Native stream (1 × 2^(n-1))
1	2s	1s
2	4s	2s
3	8s	4s
4	16s	8s
5	32s	16s

Native retry is slightly more aggressive (shorter waits). Both are reasonable.

---

Improvements in Native Retry (PR #34)

• Resumable downloads: Uses Range: bytes=N- and If-Range headers to resume interrupted streams rather than re-downloading from byte 0 — a significant improvement. ✅
• Progress tracking: The _STREAM_RESET sentinel correctly resets progress bars when a restart is forced. ✅
• Separation of concerns: urllib3 handles transport-level retries; _stream_with_retry handles mid-stream failures. Clean architecture. ✅

---

Summary

The two PRs are well-designed to work together. The hasattr(FileDownloader, 'RETRY') guard in PR #467 is the correct and minimal mechanism to enable a smooth transition. The only item worth double-checking is whether the 404 exclusion from status_forcelist in PR #34 could re-introduce the transient GitHub 404 issue that the original monkey-patch (PR #463) was meant to solve.

---

🧠 Learnings used

Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2026-04-12T20:24:06.280Z
Learning: In the pioarduino/platform-espressif32 project, retrying on all HTTP error status codes (including 404) during package downloads is intentional. GitHub sometimes returns incorrect/misleading status codes (including 404) as transient errors. Packages are verified to be available before download is attempted, so real 404 (not found) errors do not occur in practice. The broad `PackageException` catch in the retry logic (e.g., in `patch_file_downloader()`) should not be narrowed to specific status codes like 502/503 only.

---

Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 268
File: builder/penv_setup.py:87-96
Timestamp: 2025-08-26T14:52:29.044Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to trust system executables when falling back to PATH-based command resolution (like "uv"), rather than implementing additional validation checks for security concerns.

---

Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-09-23T12:35:35.508Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 has explicitly instructed to NEVER create issues or PRs outside of his repositories (pioarduino/*). All issue and PR creation must be strictly limited to his own repositories only, never in upstream or other repositories.

---

Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-09-23T12:35:35.508Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 has explicitly instructed to NEVER create issues or PRs outside of his repositories (pioarduino/*). All issue and PR creation must be strictly limited to his own repositories only, never in upstream or other repositories.

---

Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 281
File: builder/frameworks/component_manager.py:1047-1054
Timestamp: 2025-09-04T15:27:18.112Z
Learning: In the pioarduino/platform-espressif32 project, Jason2866 prefers not to add guard checks for missing arduino_libs_mcu in backup operations, preferring to let the operation proceed rather than silently skip when the directory might be missing.

---

Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 296
File: builder/main.py:49-50
Timestamp: 2025-09-22T14:47:08.855Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers that critical setup operations like platform.setup_python_env should crash immediately if they fail rather than having fallback mechanisms, as this ensures problems are detected early and clearly rather than being masked.

---

Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 371
File: builder/spiffsgen.py:620-629
Timestamp: 2025-12-27T16:33:38.710Z
Learning: In the pioarduino/platform-espressif32 project, when extracting SPIFFS files in builder/spiffsgen.py, Jason2866 prefers to check if rel_path is empty after lstrip('/') and skip such files with a warning message, rather than attempting to handle empty parent directories in os.makedirs calls. This treats empty paths as invalid/corrupted data.