Repository navigation
Conversation
The API's nginx rejects request bodies over 1 MiB, and projects/update sends every file in one form-encoded request. On a 413, update the project metadata without files, then sync the files with files/create, files/update and files/delete (one request per file).
There was a problem hiding this comment.
Thanks for the clear write-up and the measurements. Falling back only on a 413 keeps small projects on the one-request path, and the tests show the fallback is reached and that other errors still re-raise. One blocking problem, and two smaller points.
Blocking: the fallback skips the collaborator write-permission check. This answers the open question in your notes. I checked how the API handles this. The collaboration-lock check (the one behind You do not have write permission ... push with --force) is tied to codeSourceId. projects/update only runs it when files is in the request, and files/create, files/update and files/delete only run it when codeSourceId is sent.
On the fallback path, the metadata-only projects/update has no files, so the check doesn't run. FileClient never sends codeSourceId, so the per-file calls skip it too. As a result, a plain lean cloud push (without --force) of a large project silently overwrites and deletes files while another collaborator holds the write lock, which today's single-request push refuses to do. That can lose someone else's code.
Please add an optional code_source_id to FileClient.create, update and delete, sent as codeSourceId the way ProjectClient.update does. Pass update_args.get("code_source_id") through _push_files_one_by_one so --force keeps working as it does now. Please also extend the 413 test to assert that the per-file calls receive code_source_id="cli" without --force and None with it.
Smaller points (not blocking on their own):
- Partial failure leaves the cloud worse off than before. Deletes run first, so if a later
files/createorfiles/updatefails, the deleted files are already gone. For example, a single file over the 1 MiB body limit still returns 413 per file, and so do validation errors. Before this change, the cloud project was left untouched. You note it isn't atomic, but please at least make the warning say the project was only partly pushed, so users know to fix the problem and push again. You could also make the delete-first order apply only when the project is near its file quota, but that's your call. - I checked encryption: the CLI encrypts with the key's fixed IV, so the ciphertext is the same on every push. Unchanged files in an encrypted project won't be re-uploaded on each push, which answers your second note.
CI: every finished job passed; the last three matrix jobs were still running when I submitted this review.
Other repositories: none need a change. This is client-side only and uses existing files/* endpoints.
Risk: risk:high. This changes what lean cloud push writes to and deletes from users' cloud projects, and as it stands it can overwrite a collaborator's code. Passing codeSourceId with a test for it, plus an end-to-end push of a project over the limit that shows a lock held by another session is still respected, would be the evidence to lower it.
| # The project is too large for a single request, so update the metadata first and push the files one by one | ||
| self._logger.info(f"'{cloud_project.name}' is too large to push in one request, pushing its files one by one") | ||
| self._api_client.projects.update(cloud_project.projectId, | ||
| **{key: value for key, value in update_args.items() if key != "files"}) |
There was a problem hiding this comment.
This request has no files, so the collaboration-lock check that code_source_id="cli" triggers on the single-request path doesn't run here. Together with the per-file calls below, which send no codeSourceId at all, the non---force large-project push no longer respects another collaborator's write lock.
| for file in files: | ||
| cloud_file = cloud_files.get(file["name"]) | ||
| if cloud_file is None: | ||
| self._api_client.files.create(cloud_project.projectId, file["name"], file["content"]) |
There was a problem hiding this comment.
files/create, files/update and files/delete only check the write lock when codeSourceId is sent. Please add an optional code_source_id to these FileClient methods and pass update_args.get("code_source_id") through, so --force keeps its current meaning.
…artial pushes
- Send codeSourceId on files/create, files/update and files/delete ("cli",
or nothing with --force), so the per-file path keeps the write lock.
- Push the files first and the metadata-only projects/update last. The
server doesn't lock-check a projects/update without files.
- Raise PartialPushError when a push stops after some changes were
written, naming the failed request. push_projects raises it after the
loop, so --push commands stop instead of running half-pushed code.
- Compare file contents with rstrip() so leading whitespace still counts.
|
Thanks, the lock point was right. It's fixed, and I tested it end to end against a real lock.
Order changed: files first, metadata last. While testing I found that a metadata-only End-to-end lock test. I used a throwaway project with the same 103-file, 1.1 MB C# project. The web IDE held the edit lock, and I changed Reading the project back afterwards, the cloud description and Both were updated in the cloud. Partial failures.
One more fix. The changed-file check compares with Encryption. Thanks for checking the fixed IV. Since the key id now goes in the last request, I checked that A question for you. When a large project's only change is metadata (description, parameters, libraries), the fallback sends just the metadata-only Tests: the full suite passes (2461 passed, 412 skipped) and |
lean cloud pushfails on big projects.PushManager._push_metadatasends every source file in one form-encodedprojects/updatePOST, and the nginx in front ofwww.quantconnect.com/api/v2rejects any request body over 1 MiB with a 413. The CLI logsCannot push ...and still exits 0, so scripts don't notice the cloud kept the old code.We hit it with a 103-file C# project: 925 KB of source, 1,163,065 bytes once form-encoded. I binary-searched the cap on a throwaway project and it's exactly 1,048,576 bytes; 1,048,577 gets the nginx
413 Request Entity Too Largepage on bothprojects/updateandfiles/update. The docs allow up to 250 files of 256 KB each per project, so a project can sit well inside its quota and still not fit in one request.Related: #573 and #597 (which made the 413 message clearer).
Change
When
projects/updatereturns 413,PushManagernow:files/read,files/delete,files/createandfiles/update,projects/updatecall withoutfileslast (name, description, parameters, libraries, encryption key).Every one of those calls sends
codeSourceIdthe same wayprojects/updatedoes:"cli"normally, nothing with--force.FileClient.create,updateanddeletegot an optionalcode_source_idfor this. The API only checks the collaboration lock onfiles/*whencodeSourceIdis sent, and it doesn't check aprojects/updatewithoutfiles, so the files go first: if another session holds the lock, the first file call is refused and nothing is written.Deletes go first among the file calls, so a rename in a project at its file quota doesn't go over it. Unchanged files are skipped (compared with
rstrip(), so leading whitespace still counts). Small projects keep the single fast request; only a 413 takes this path. Any other error is re-raised as before.I went with "fall back on 413" instead of checking the size up front, so the CLI doesn't hardcode the server's nginx limit.
Partial failures
The per-file path isn't atomic the way
projects/updateis, so failures are reported by how far the push got:nothing was pushedand keeps the server's text, so the--forcehint still shows on a lock.PartialPushErrorsays the project was only partly pushed and names the failing request, for examplecreating 'research.ipynb' failed. If only the final metadata request fails, it says all files were pushed but the project details weren't.push_projectswarns per project and moves on, as before, then raisesPartialPushErrorat the end if any project was only partly pushed. Solean cloud backtest/optimize/live --pushstop instead of running half-pushed code, andlean cloud pushexits 1 in that case.Tests
test_push_projects_pushes_files_one_by_one_when_the_project_is_too_large(with and without--force): checks the create, update and delete calls,code_source_idon each, the exact call order (deletes, file writes, metadata last), and that trailing whitespace is ignored but leading whitespace isn't.test_push_projects_leaves_the_project_unchanged_when_the_first_file_push_fails: a lock refusal on the first call; no metadata update, plain error,--forcehint.test_push_projects_raises_when_a_file_push_fails_after_other_files_were_pushedandtest_push_projects_raises_when_only_the_project_details_fail_after_the_files_were_pushed: the partial-push messages and the raise.test_push_projects_does_not_push_files_one_by_one_on_other_errors: a 400 doesn't trigger the fallback.test_get_cloud_project_does_not_return_a_partly_pushed_project:--pushcommands stop on a partial push.tests/components/api/test_file_client.py:codeSourceIdis sent only when given.python -m pytest: 2461 passed, 412 skipped (master: 2448 passed, 412 skipped).static_analysis.pypasses.Live runs (throwaway project, the 103-file project)
Lock test, with the web IDE holding the edit lock and a changed
Main.csand description:--force: refused on the first file call (updating 'Main.cs' failed, nothing was pushed: The project could not be saved as another collaborator controls the write permission...). The cloud description andMain.cswere unchanged afterwards.--force: pushed; both updated in the cloud.From the first version of this PR (same per-file sync, before the order and
codeSourceIdchanges):files/createand 1files/delete(a stale file); afterwards all 103 cloud files were byte-identical to the local ones. It took about 2.5 minutes with--verboseon.Main.cs: 1files/update, nothing else.Notes
projects/update, which the server doesn't lock-check. The single-request path always includesfiles, so it is checked there. I think this belongs on the server side (check the lock whenevercodeSourceIdis sent).files/createaccepts ciphertext in a project that isn't marked encrypted yet. I haven't run a full--encryptpush of a large project end to end.lean cloud pushexits 0. Happy to open an issue for that.