fix(providers): execute rclone with non-cancelling context #5040
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where the rclone storage provider was using a cancellable context to execute the rclone subprocess, which could cause premature termination of the rclone server. The fix ensures the rclone process lifecycle is independent of the caller's context cancellation by using context.WithoutCancel().
Key changes:
- Modified
New()to usecontext.WithoutCancel(ctx)when creating the rclone command, preventing premature process termination - Added test coverage to verify storage operations work correctly after the context used in
New()is cancelled
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| repo/blob/rclone/rclone_storage.go | Changed exec.CommandContext to use non-cancellable context for rclone subprocess |
| repo/blob/rclone/rclone_storage_test.go | Added test verifying storage operations work after context cancellation |
91f99e1 to
863c0c7
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5040 +/- ##
==========================================
+ Coverage 75.86% 78.03% +2.16%
==========================================
Files 470 548 +78
Lines 37301 31398 -5903
==========================================
- Hits 28299 24500 -3799
+ Misses 7071 4849 -2222
- Partials 1931 2049 +118 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
863c0c7 to
6b7b254
Compare
|
|
||
| t.Run("PasswordCreds", func(t *testing.T) { | ||
| ctx := testlogging.Context(t) | ||
| newctx, cancel := context.WithCancel(ctx) |
There was a problem hiding this comment.
Variable name newctx should use camelCase as newCtx to be consistent with the naming convention used in the rclone test (line 107 of rclone_storage_test.go).
| newctx, cancel := context.WithCancel(ctx) | ||
|
|
||
| st, err := createSFTPStorage(ctx, t, sftp.Options{ | ||
| st, err := createSFTPStorage(newctx, t, sftp.Options{ |
There was a problem hiding this comment.
Variable reference newctx should be newCtx to match the corrected camelCase naming convention.
Uh oh!
There was an error while loading. Please reload this page.