fix(lcm): Add retry with refresh token during upload file#2084
Conversation
📝 WalkthroughWalkthroughThe ChangesStreaming Upload and Test Enablement
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Connection
participant RestClientRequest
Caller->>Connection: do_stream_file
Connection->>Connection: retryable block
Connection->>RestClientRequest: build request (File.new, x_gdc_authtt)
RestClientRequest-->>Connection: 401 error
Connection->>Connection: refresh token
Connection->>RestClientRequest: rebuild request (fresh File.new, updated x_gdc_authtt)
RestClientRequest-->>Connection: success
Connection-->>Caller: result
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/gooddata/rest/connection.rb (1)
530-545: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFile handle leaks on retry.
The fix correctly moves request construction inside the retry block so each retry gets a fresh file handle and refreshed
x_gdc_authtt. However, when a retry attempt fails, theFile.new(filename, 'rb')from that attempt is never explicitly closed — RestClient does not auto-close File payloads. Each failed attempt leaks one file descriptor, which could exhaust the FD limit under sustained retry loops.Using
File.openwith a block aroundrequest.executeensures the handle is closed on both success and failure, while still creating a fresh handle per retry.♻️ Proposed refactor to ensure file handle cleanup
b = proc do - request = RestClient::Request.new(:method => :put, - :url => uri.to_s, - :verify_ssl => verify_ssl, - :headers => `@webdav_headers.merge`(:x_gdc_authtt => headers[:x_gdc_authtt]), - :payload => File.new(filename, 'rb')) - request.execute + File.open(filename, 'rb') do |file| + request = RestClient::Request.new(:method => :put, + :url => uri.to_s, + :verify_ssl => verify_ssl, + :headers => `@webdav_headers.merge`(:x_gdc_authtt => headers[:x_gdc_authtt]), + :payload => file) + request.execute + end end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/gooddata/rest/connection.rb` around lines 530 - 545, The retryable upload path still leaks file descriptors because each call to the upload proc creates a new File handle that is never explicitly closed if RestClient fails. Update the upload logic inside GoodData::Rest::Connection.retryable and the b proc so the payload file is opened in a block around request.execute, ensuring every retry gets a fresh handle and that it is closed on both success and failure while preserving the refreshed x_gdc_authtt behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/gooddata/rest/connection.rb`:
- Around line 530-545: The retryable upload path still leaks file descriptors
because each call to the upload proc creates a new File handle that is never
explicitly closed if RestClient fails. Update the upload logic inside
GoodData::Rest::Connection.retryable and the b proc so the payload file is
opened in a block around request.execute, ensuring every retry gets a fresh
handle and that it is closed on both success and failure while preserving the
refreshed x_gdc_authtt behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e1b6470-544e-460f-a788-e1d5c84ab9a1
📒 Files selected for processing (2)
lib/gooddata/rest/connection.rbspec/project/full_process_schedule_spec.rb
2 participants
JIRA: GRIF-877
Summary by CodeRabbit
Bug Fixes
Tests