Fix Rosim JAR download failing in every cold container - #366
Open
jarsarasty wants to merge 3 commits into
Open
Conversation
Every cold container raised KeyError: 'assets' because the response body from the unauthenticated api.github.com releases endpoint had no "assets" key. That endpoint is rate-limited to 60 requests/hour per IP, which on shared CI runner IPs is the near-certain cause; the code discarded the status code, so this was never confirmed from a log. A cached JAR short-circuits the download, which is why this passed locally and failed everywhere else. Fetch from a pinned release download URL, which is not rate-limited, and verify the download against a known SHA-256. The file is staged through a temporary name that deliberately does not match the rosim*.jar cache glob, so a process killed mid-download cannot leave a truncated file to be picked up as a valid cached JAR on the next run. The pinned tag is decoupled from this package's version, which means upload_rosim_to_release.yml's per-release upload is not what this code reads. Bumping the JAR without bumping the constants would otherwise serve the previous version with a still-matching checksum, so tests assert the pinned name and digest against the JAR committed in bin.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
It introduces process-global os.umask() mutation in library code (thread-safety risk) and contains misleading documentation about how pinned-constant upgrades interact with existing cached rosim*.jar files.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens Rosim JAR acquisition in PyjniusLoader to avoid unauthenticated GitHub API rate limits in cold CI containers by switching from “latest release via API” to a pinned release-asset URL and verifying the downloaded artifact via SHA-256 before use.
Changes:
- Replace GitHub API “latest release” lookup with a pinned release asset download URL (
ROSIM_RELEASE_TAG/ROSIM_JAR_NAME) plus SHA-256 verification and atomic move into place. - Ensure partial/interrupted downloads can’t be mistaken for cached JARs by downloading to a temp name that never matches the
rosim*.jarcache glob. - Add a comprehensive unittest suite covering cache hits, successful downloads, failures/cleanup, checksum mismatch, temp-name invariants, and drift guards against the committed
bin/JAR (including git-LFS pointer handling).
File summaries
| File | Description |
|---|---|
src/omotes_simulator_core/entities/assets/pyjnius_loader.py |
Pin Rosim JAR download to a specific release asset URL, add checksum verification, and stage downloads safely via a temp file + atomic rename. |
unit_test/entities/test_pyjnius_loader.py |
Add new unit tests validating cache/download behavior, cleanup paths, checksum enforcement, and guarding pinned constants vs the repository JAR. |
Review details
Suppressed comments (1)
src/omotes_simulator_core/entities/assets/pyjnius_loader.py:156
- This debug log uses an f-string, which is formatted even when debug logging is disabled; prefer logger parameterization for lower overhead.
logger.debug(f"Using Rosim JAR file: {final_path}")
- Files reviewed: 2/2 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+103
to
+105
| If a `rosim*.jar` is already present in the `bin` folder, no download happens and | ||
| that file is used. To pick up a newer Rosim JAR, bump the module-level constants; | ||
| deleting the local copy only re-fetches the same pinned asset. |
Comment on lines
118
to
+120
| logger.debug("Rosim JAR files already present, skipping download.") | ||
| logger.debug(f"Using Rosim JAR file: {jar_files[0]}") | ||
| return jar_files[0] | ||
|
|
||
| repo = "Project-OMOTES/simulator-core" | ||
| api_url = f"https://api.github.com/repos/{repo}/releases/latest" | ||
|
|
||
| response = requests.get(api_url) | ||
| release_data = response.json() | ||
| assets = release_data["assets"] | ||
| for asset in assets: | ||
| if "rosim" in asset["name"]: | ||
| # downloading the jar file from github releeases | ||
| logger.debug("Downloading Rosim JAR files from GitHub") | ||
| url = asset["browser_download_url"] | ||
| jar_file = os.path.join(bin_path, asset["name"]) | ||
| response_url = urllib.request.urlretrieve( | ||
| url, | ||
| jar_file, | ||
| return os.path.basename(jar_files[0]) |
Correct the docstring's upgrade instructions: the cache glob matches any rosim*.jar, so bumping the constants alone keeps using an older local copy -- both a bump and a delete are needed. Replace the umask read with copying the mode of a sibling shipped JAR. Reading the umask means setting it to 0 and restoring it, which mutates process-global state that a library should not touch. Parameterize the debug logs so their arguments are not formatted when the level is disabled, and keep the traceback when temporary-file cleanup fails.
Copying the mode from jfxrt.jar silently did nothing in the one case the neighbouring os.makedirs call exists for: a bin folder that is absent or empty holds no sibling to copy from. No OMOTES deployment writes the JAR as one user and reads it as another, so the mode is left alone and the reason recorded.
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
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.
Problem.
download_rosim_jar()looked up the latest release viaapi.github.com, which is rate-limited to 60 req/hour per IP unauthenticated. On shared CI runner IPs the response had noassetskey, sorelease_data["assets"]raisedKeyError: 'assets'. It passed locally because a cached JAR short-circuits the download before the API is called.Change. Download from a pinned release asset URL (not rate-limited) and verify it against a known SHA-256 before use. The download is staged through a temporary name that deliberately doesn't match the
rosim*.jarcache glob, so a process killed mid-download can't leave a truncated file to be cached as valid.Trade-off. The pinned tag is decoupled from the package version, so
upload_rosim_to_release.yml's per-release upload isn't what this code reads. Bumping the JAR without bumpingROSIM_JAR_NAMEandROSIM_JAR_SHA256would serve the old version with a matching checksum — tests assert both against the JAR inbin/so that fails loudly. Tag drift isn't caught; it surfaces as a download failure at runtime.Tests. 13 new (this file had none): cache hit, successful download, checksum mismatch, download error, the temp-name invariant, LFS pointer parsing, drift guards.
Needs a simulator-core release, then a
omotes-simulator-corepin bump in simulator-worker (currently pinned at0.0.30).