fix(cli): validate fps upper bound, prune stale sessions, add upload progress, retry desktop store read#1887
Open
ManthanNimodiya wants to merge 3 commits into
Open
Conversation
…progress, retry desktop store read
…ce, and credential retry logic
Contributor
Author
|
@greptileai please re-review |
Comment on lines
+342
to
+353
| let body = reqwest::Body::wrap_stream(stream); | ||
| let response = http | ||
| .put(put_url) | ||
| .header(reqwest::header::CONTENT_LENGTH, total_size) | ||
| .header(reqwest::header::CONTENT_LENGTH, total_bytes) | ||
| .body(body) | ||
| .send() | ||
| .await | ||
| .map_err(|e| format!("Upload failed: {e}"))?; | ||
|
|
||
| if let Some(task) = progress_task { | ||
| task.abort(); | ||
| } |
Contributor
There was a problem hiding this comment.
Progress task not aborted on
send() failure
If http.put(...).send().await returns an Err, the ? on line 349 propagates immediately, skipping the task.abort() call at line 351–353. The background task then continues to wake up every second and emit stale Uploading events after the error JSON has already been written to stdout. A JSON consumer treating the stream as NDJSON would see progress events after the terminal error object. The fix that was intended here is only applied for the success path (HTTP non-2xx) — the network-error path still leaks the task.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/cli/src/upload.rs
Line: 342-353
Comment:
**Progress task not aborted on `send()` failure**
If `http.put(...).send().await` returns an `Err`, the `?` on line 349 propagates immediately, skipping the `task.abort()` call at line 351–353. The background task then continues to wake up every second and emit stale `Uploading` events after the error JSON has already been written to stdout. A JSON consumer treating the stream as NDJSON would see progress events after the terminal error object. The fix that was intended here is only applied for the success path (HTTP non-2xx) — the network-error path still leaks the task.
How can I resolve this? If you propose a fix, please make it concise.
Contributor
Author
|
@greptileai please re-review |
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.
Addresses 4 issues found in #1886:
Greptile Summary
This PR addresses four previously-reported issues: FPS upper-bound validation, stale-session pruning, upload progress events, and desktop store read retries. The infinite-recursion in pruning and the progress-task lifetime issues from the prior round are now correctly fixed.
record.rs): simplefps > 120guard added toRecordParams::validate— correct for theOption<u32>type.session.rs):prune_old_sessionsbuilds paths directly from itsdirargument, eliminating the earlier recursion; pruning is triggered on everysessions_dir()call, so helpers likecleanupandwrite_sessionscan the directory 2–3 times per invocation.upload.rs): atomic counter + background task correctly aborted before both the error path and theUploadedemission, but the terminal event still useswrite_json(pretty-printed, multi-line) while progress events usewrite_json_line(compact), breaking NDJSON stream consumers.credentials.rs): correctly exits early on a successful parse; however,ENOENTis retried with 50 ms sleeps, adding ~200 ms latency per invocation on machines without Cap Desktop.Confidence Score: 3/5
Two regressions in the changed code should be fixed before merging: the file-not-found retry adds 200 ms blocking latency on every CLI invocation for users without Cap Desktop, and the mixed JSON formatting in the upload event stream breaks NDJSON consumers.
The file-not-found retry in load_desktop_store and the mixed JSON formatting in the upload event stream are both present-defect regressions introduced by this PR affecting real usage paths. The previously-flagged bugs are correctly resolved.
apps/cli/src/credentials.rs and apps/cli/src/upload.rs
Important Files Changed
dir. Pruning runs once persessions_dir()call, which is called by every path-helper, so cleanup/write_session scan the directory 2-3 times each.Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "fix(cli): abort upload progress task on ..." | Re-trigger Greptile