Repository navigation
fix: keep the modules cache when the downloaded JSON is malformed or has no modules - #708
Conversation
_download_modules_json opened the cache with mode w before decoding the response, so an HTTP 200 with a malformed body truncated the cache and the CLI could not start. Decode first and write the replacement to a temporary file that replaces the cache only once complete. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
pathlib's replace and unlink bypass pyfakefs on Python 3.10, which made test_force_refresh_modules_json_downloads_file fail on the 3.10 CI job. Use os.replace and os.remove instead. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Thanks for the CI run. The only real failure was This comment was drafted with Claude Code. |
quantconnect-kronos
Bot
added
kronos:develop:queued
|
The remaining red check on Checked locally on this branch merged with current master:
The fix needs no further changes. |
|
The re-run (attempt 2 of Build run 41852272197) failed the same way. In This change touches only |
quantconnect-kronos-reviewer
Bot
added
kronos:review:queued
![quantconnect-kronos-reviewer[bot]](https://avatars.githubusercontent.com/u/3912814?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: the full test matrix hasn't run on this head commit yet. The code change looks correct to me. Nothing in it needs changing.
CI
- The one failing step on
4932d67is Check README intest (ubuntu-22.04, 3.12). This PR didn't cause it. Its diff dropsInteractiveBrokersFixBrokerageand--ib-fix-*fromlean cloud live deploy, and master's Build atd812d26fails the same step with the same diff. The README is out of sync with the hosted module catalog. - Fail-fast then cancelled Run tests in every other job: all 3.9, 3.10 and Windows jobs, plus most of the macOS ones. Only ubuntu-22.04/3.12 ran the suite (2796 passed) on this head.
- That gap matters for this PR in particular. On the first commit,
test_force_refresh_modules_json_downloads_filefailed on ubuntu 3.10 because of how pyfakefs handlesPath.replace/Path.unlink. The second commit switches toos.replace/os.removeto fix that, but no 3.10 job has finished running tests since. Windows, whereos.replacehas its own semantics, hasn't run any tests on either commit. The author's local 3.10 run is the only evidence for those, and I don't run code from outside contributors. - To unblock: once master's README check is green, merge or rebase master and let the whole matrix finish. The branch merges cleanly with master, including #704's empty-placeholder handling in
lean/models/__init__.py. Nothing else should be needed.
Code review (read only)
- The fix does what #707 asks. It decodes before touching the cache, writes to a sibling temp file, does an atomic
os.replace, and cleans up infinally. Both callers already swallow exceptions: the import-time refresh andUpdateManager.force_refresh_modules_json. So a malformed 200 response or a failed write now keeps the previous cache, with no new failure path. - Combined with #704 (
setup.pyships an empty placeholder that pip records):os.replacekeeps the same path, so uninstall still removes the file. - The tests reproduce the bug: the malformed-response test fails against the old code. They also check that no temp file is left behind.
Non-blocking notes (inline)
- The temp file's name is fixed. Two CLI processes refreshing at the same moment share it, and the second one's
os.replacecan raiseFileNotFoundError. The caller swallows that and the cache stays valid, so nothing breaks. A per-process name, e.g.tempfile.NamedTemporaryFile(dir=file_path.parent, delete=False), would avoid the collision. - The new test file is missing the license header that the other test files have.
- Out of scope, for reference: a 200 response with valid JSON but no
moduleskey would still replace the cache and fail ondata['modules']. #707 only covers malformed JSON, so that belongs in its own issue if anyone cares about it.
Risk: I set risk:high for now. The change itself is small and easy to revert, but risk:low still needs evidence: the full CI matrix green on this head, above all 3.10 and Windows, which this PR already broke once.
Other repositories: none affected.
| file_path.parent.mkdir(parents=True, exist_ok=True) | ||
| with open(file_path, 'w', encoding='utf-8') as f: | ||
| dump(res.json(), f, ensure_ascii=False, indent=4) | ||
| temporary_file_path = file_path.with_name(file_path.name + ".tmp") |
There was a problem hiding this comment.
Non-blocking: this name is the same for every process. If two CLI processes refresh at the same moment, both write to the same .tmp file, and whichever runs replace second gets FileNotFoundError. Callers swallow that and the cache stays valid, so it isn't a bug. A per-process temp file in the same directory, e.g. tempfile.NamedTemporaryFile(dir=file_path.parent, delete=False, ...), avoids the collision and still lets os.replace stay atomic.
There was a problem hiding this comment.
Done in 0468910. The temporary file is now <name>.<pid>.tmp, so concurrent refreshes no longer share it. I used the process id rather than NamedTemporaryFile, which creates the file with mode 0600. After os.replace, that mode would carry over to the cache, and other users of a shared install could no longer read it. open(..., 'w') keeps the usual umask-based permissions.
| @@ -0,0 +1,44 @@ | |||
| import json | |||
There was a problem hiding this comment.
Nit: add the license header the other test files start with (see tests/models/test_modules.py).
quantconnect-kronos-reviewer
Bot
added
risk:high
Two CLI processes refreshing the modules cache at once shared the same temporary file, so one replace could fail. Name it after the process id. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed the review on
Local results on this head:
I couldn't run Windows locally. Master's Build is still red at Check README ( |
|
CI on
Nothing on this branch needs to change for the README check. Once the run completes, a re-run of the failed and cancelled jobs may get past it, given that the step already passed in three of five jobs. |
quantconnect-kronos-reviewer
Bot
added
kronos:review:queued
|
Attempt 2 of Build run 41868165952 on
|
There was a problem hiding this comment.
Approving 2df8f3d. The ownership and permissions regression raised on 0468910 is fixed, and nothing in the change is unrelated to #707.
Since the last reviews
- When a cache already exists, the temporary file now gets its owner, group and mode (
chown/chmod) beforeos.replace. If that fails, the exception propagates,finallyremoves the temporary file, and the old cache stays. Both callers (the import-time refresh andUpdateManager.force_refresh_modules_json) already catch it, so a failed copy leaves the user on the previous cache. It doesn't break startup. - Two new POSIX-only tests cover this. One is a root refresh under
umask 077that must keep1000:1000 0644, and that test fails against0468910. The other haschownraise, and the cache must stay unchanged with no temporary file left. They patchos.chowncorrectly, because the function importschownat call time. - The author's end-to-end table covers the case the earlier review asked for: a root
--updatefollowed by a normal user's run. On0468910the next run fails at startup withPermissionError. On2df8f3dit starts normally, the same as on master.
CI
- Regression Testing passed on ubuntu-24.04, macos-15 and windows-latest (3.14). On Windows that is 2805 passed: the two original cache tests ran there and the two POSIX tests were skipped as intended.
- Build: Run tests passed on ubuntu-22.04 3.9–3.14, ubuntu-24.04 3.9/3.11/3.13 and macos-15 3.11–3.14, which includes the new owner/mode tests on Linux and macOS. The only failed step is Check README on ubuntu-22.04/3.9 and ubuntu-24.04/3.9. Its diff is the
--brokeragechoice list oflean cloud live deploy, which is the module-catalog drift that also fails on master (#705). Fail-fast cancelled the remaining jobs, including the Windows Build jobs. This change didn't cause the failure.
Non-blocking
- Inline:
chownruns on every POSIX refresh, even when the owner already matches. See the comment for the case where that adds a failure. - The PR description still describes only the first commit. Please add the owner/mode preservation and the end-to-end outcome, since the description is what people read after merge.
Risk: I kept risk:high. The change is small and easy to revert, and it has tests and an end-to-end run. But on every POSIX refresh it now needs a chown to succeed, which wasn't needed before, on the path that fills a fresh install's empty cache. When that path fails, the CLI won't start. Skipping chown when the owner and group already match (inline) would remove that new failure path for the normal case. With that, I'd consider it low risk.
Other repositories: none affected. The cache path and JSON contract are unchanged.
| # doesn't leave a file that later runs without it can't read | ||
| from os import chown | ||
| existing = stat(file_path) | ||
| chown(temporary_file_path, existing.st_uid, existing.st_gid) |
There was a problem hiding this comment.
Non-blocking: this chown now runs on every POSIX refresh, including the normal case where the same user owns the cache. A non-owner can only change a file's group to the file's current group or to a group they belong to. So if the cache's group differs from the temporary file's group (for example a placeholder installed under another group, or a shared install), this raises. A file the user could rewrite in place before is then never refreshed. If that file is the empty placeholder a fresh install ships, the CLI won't start. Calling chown only when (existing.st_uid, existing.st_gid) differ from the temporary file's would keep the sudo fix and avoid this extra failure in the normal case.
There was a problem hiding this comment.
Done in 2d85f0e, slightly beyond the suggestion so it also covers the case you described:
- Owner differs (a sudo refresh):
chownto the cache's owner and group as before. If that fails, the old cache is kept. - Only the group differs: the group is copied on a best-effort basis, and a
PermissionErroris ignored. Comparing(uid, gid)alone would still callchownfor a cache the user owns under a group they aren't in, and that call fails for a non-root user. The owner can read the file whatever its group. - Both match: no
chowncall.
Tested end to end with a cache owned by the user under nogroup. On 2df8f3d, --update silently didn't refresh it, and with the empty placeholder the CLI stopped with FileNotFoundError. On 2d85f0e both are refreshed and the CLI starts.
quantconnect-kronos-reviewer
Bot
removed
the
kronos:review:working
chown ran on every refresh, so a user whose cache had a group they are not a member of could no longer refresh it, and an empty placeholder then stopped the CLI from starting. Change the owner only when it differs, and keep the group on a best-effort basis: the owner can read the file whatever its group. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed the non-blocking review notes on
Tests: End to end: CLI builds installed into virtualenvs owned by an ordinary user.
Local checks
PR description updated to cover the temporary-file, owner/mode and group handling, and the end-to-end results. |
|
CI on
Nothing on this branch needs to change for this. |
quantconnect-kronos-reviewer
Bot
added
kronos:review:queued
There was a problem hiding this comment.
Approving 2d85f0e. This addresses the non-blocking chown note from my last review. The only chown that can still fail and block a refresh is the one a sudo refresh needs.
Since my last review
- If the temporary file's owner differs from the cache's owner (a sudo refresh), the temporary file gets the cache's owner and group. If that fails, the old cache stays, as before.
- If only the group differs, the group is copied on a best-effort basis and
PermissionErroris ignored. The owner can read the file whatever its group, so ignoring it is safe. - If both match, there's no
chownat all. A user refreshing their own cache, including the empty placeholder a fresh install ships, no longer needschownto succeed. That removes the new startup failure I flagged on2df8f3d. - I checked the non-root
chownbehavior this branching relies on with a standalone standard-library script (not the PR's code). As a non-root user,chown(path, -1, <own group>)succeeds.chown(path, -1, <group not a member of>)andchown(path, <other uid>, ...)both raisePermissionError. So the group branch can only hit the suppressed error, and the owner branch still keeps the old cache when it can't copy the owner. - Tests:
..._replaces_cache_when_only_its_group_cannot_be_keptcovers both the matching group (nochowncall) and a differing group (chownraises and is ignored)...._keeps_cache_when_its_owner_cannot_be_keptnow uses a cache owned by another uid, so it runs the path wherechownis required. One gap, non-blocking: no test checks that a differing group is actually copied whenchownsucceeds. - The description now covers the whole change and the end-to-end outcomes: a root refresh under
umask 077, a user-owned cache with a foreign group, and an empty placeholder with a foreign group. In each case the next run as the user starts normally.
CI
- Regression Testing passed on ubuntu-24.04, macos-15 and windows-latest (3.14). On Windows that's 2805 passed, with the POSIX-only tests skipped as intended.
- Build: Run tests passed in every job that reached the end of the step: ubuntu-22.04 3.10/3.14, ubuntu-24.04 3.13/3.14 and macos-15 3.10/3.12–3.14. The only failed step is Check README (ubuntu-24.04 3.11/3.12, ubuntu-22.04 3.12/3.13, ubuntu-24.04 3.10). Its diff is the
--brokeragechoices and--ib-fix-*options oflean cloud live deploy, the same module-catalog drift that fails master's Build atd812d26(#705). This change doesn't touch the README or any command. Fail-fast cancelled the remaining jobs. Earlier heads ran the full 3.9–3.14 suite green, and this commit only changes thechownbranching inside_download_modules_json.
Code (read only): a malformed 200 response no longer touches the cache, which is what #707 asks for. The replacement keeps the cache's owner and mode, and both callers already catch refresh errors.
Risk: I set risk:low and removed risk:high. The change is a small, self-contained bug fix to a local cache write, it's easy to revert, and it changes no command, option or output. Tests reproduce each failure mode, CI is green across operating systems apart from the unrelated README check, and the end-to-end runs cover the sudo and non-sudo sequences that were the risk. With the chown skipped when ownership already matches, the default path adds no new way to fail.
Other repositories: none affected. The cache path and JSON contract are unchanged.
quantconnect-kronos-reviewer
Bot
added
risk:low
quantconnect-kronos-reviewer
Bot
added
kronos:review:queued
quantconnect-kronos-reviewer-2
Bot
added
kronos:review:working
There was a problem hiding this comment.
Requesting changes on 2d85f0e: replacement still breaks cache access in some shared installations.
The malformed-response fix addresses #707, and owner/mode preservation fixes my earlier sudo/umask 077 example. The updated description and tests cover that sequence. Three access regressions remain, detailed inline:
- Previously writable placeholders can no longer be initialized (
lean/models/__init__.py:44,53). A user can populate a root-owned, group-writable cache in place on master. With a non-writable package directory, sibling staging creation now fails; even with a group-writable directory, transferring staging ownership back to root fails. When the cache is the empty packaged placeholder, keeping it unchanged is not a usable fallback: startup raisesFileNotFoundError. Please preserve startup for these previously permitted writers without requiring new filesystem privileges, and add coverage for an empty placeholder rather than only a populated fallback. - Failed group preservation (
lean/models/__init__.py:57-58). For a mode-0640cache in a shared group, the owner can refresh it even after leaving that group. Suppressing the failed group change then replaces it with the owner's current group. Other members of the original group lose read access, so their next CLI invocation fails at the unguarded cache read. The new group tests and reported end-to-end runs check only the owner, not another reader. Retain the old cache when this fallback would revoke existing read access. - Lost file ACLs (
lean/models/__init__.py:59-60). Copying uid, gid and mode does not preserve a file-specific POSIX ACL. A reader granted access only by such an ACL loses it after a successful refresh, even when uid, gid and mode all match the original file. Preserve that access metadata before replacing the cache, or retain the usable cache if preservation cannot succeed.
I verified the filesystem behavior independently, without executing PR code: the group writer could write the original file but could not create staging or restore root ownership, respectively; each non-owner reader could read the original cache and got PermissionError after replacement. For the ACL case, uid/gid/mode were identical before and after. Please add regression coverage and report end-to-end startup outcomes for the empty-placeholder writer and the existing readers after a refresh. Checking only the refreshing owner's access cannot expose these failures.
CI: Regression Testing passed on Linux, macOS and Windows (Python 3.14). The six new cache-test cases pass on Linux/macOS; the four POSIX cases are skipped on Windows. Build has no observed pytest failure, though fail-fast cancelled some matrix jobs. The two failed jobs fail only Check README. I checked their README diff against master's failing job: the removed IB FIX brokerage/options match the pre-existing failure tracked in #705. That unrelated check is not the reason for this request.
Risk: I set risk:high because these remaining access regressions can prevent users from starting the CLI. Preserving the previously working initialization and reader-access sequences, with coverage for them, would address this concern.
Other repositories: no companion update identified; the cache path and JSON contract are unchanged. The separately tracked market-hours cache issue (#709) remains outside this PR.
🤖 Generated with Claude Code
| existing = stat(file_path) | ||
| temporary = stat(temporary_file_path) | ||
| if temporary.st_uid != existing.st_uid: | ||
| chown(temporary_file_path, existing.st_uid, existing.st_gid) |
There was a problem hiding this comment.
[P2] Preserve initialization by an existing cache's group writer
A non-root member of the cache's group can populate a root-owned 0664 placeholder on master. With a group-writable package directory, this version writes staging successfully but necessarily fails here when transferring ownership to root. The import-time handler swallows that error, leaves the zero-byte placeholder unchanged, and then raises FileNotFoundError on lines 79-82. If only the cache is writable and its directory remains 0755, staging creation on line 44 fails even earlier. I verified both permission cases independently: the existing-file write succeeds, whereas sibling creation or owner assignment fails. Keeping a populated old cache is a safe fallback, but keeping an empty placeholder prevents startup. Please retain startup for these previously permitted writers without requiring additional filesystem privileges, and add coverage for the empty-placeholder case.
| elif temporary.st_gid != existing.st_gid: | ||
| # The owner can read the file whatever its group, and changing the group to one | ||
| # the owner isn't a member of needs root | ||
| with suppress(PermissionError): |
There was a problem hiding this comment.
[P2] Keep existing group readers' access when chown fails
For a cache owned by user A, group shared, mode 0640, A can still rewrite the file after leaving shared, and user B in shared can still read it. Here A's temporary file gets A's current group; restoring shared raises PermissionError, which is suppressed, and os.replace installs that different group with mode 0640. B's next CLI invocation then raises PermissionError at the cache read on line 84. Keeping A's access does not preserve B's. I verified this with an independent OS-only reproduction. Please retain the existing cache when this fallback would remove group readers' access, and cover a shared-group reader rather than asserting only that the owner sees new JSON.
| # the owner isn't a member of needs root | ||
| with suppress(PermissionError): | ||
| chown(temporary_file_path, -1, existing.st_gid) | ||
| chmod(temporary_file_path, S_IMODE(existing.st_mode)) |
There was a problem hiding this comment.
[P2] Preserve file-specific ACLs before replacing the cache
chown and chmod do not copy a file-specific POSIX access ACL to the new inode. For example, a root-owned cache can grant another user read access through a named-user ACL, without a corresponding default ACL on its directory. After root refreshes it, this replacement retains uid/gid/mode but drops that entry; the other user's next CLI invocation fails with PermissionError at line 84. The previous in-place write preserved the ACL. I reproduced the read-success/read-failure transition independently while asserting identical uid/gid/mode before and after. Please preserve the existing ACL (or leave the usable cache untouched when that cannot be done), and add coverage for a reader whose access comes only from the ACL.
quantconnect-kronos-reviewer-2
Bot
removed
the
kronos:review:working
… modules The response is decoded and checked for a `modules` key before the file is opened, since opening it for writing empties it. A malformed or empty body, or JSON that isn't a modules file, now raises before the file is touched, and the callers keep the previous modules. Writing in place, as before, keeps the file's owner, permissions and ACLs, which the earlier temporary-file replacement lost, so the owner and mode copying and their tests are gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Martin-Molinero
changed the title
Fixes #707
_download_modules_json()opened the modules file for writing, which empties it, before decoding the response. A 200 response with a malformed body therefore left an empty file, and the CLI failed at startup instead of using the modules it already had.Fix (
lean/models/__init__.py)moduleskey before the file is opened. A malformed or empty body, or JSON that isn't a modules file ({},[],null), now raises before the file is touched. Both callers, the startup refresh andUpdateManager.force_refresh_modules_json, already catch the error, so the previous modules stay in use and the next run tries the refresh again.Tests (
tests/models/test_download_modules_json.py){},[]andnulleach keep the existing file unchanged. All five fail against master.Verification
python static_analysis.pypasses.python -m pytest -s -rson Python 3.11 (Windows): 2811 passed, 72 skipped.lean live deploy --helplists the brokerages as before.{},[],nullor an HTML page loads the previous modules and leaves the file and its timestamp unchanged. The next normal run refreshes it from the CDN.🤖 Generated with Claude Code