Skip to content

Stream Pro rolling-deploy bundle downloads with caps#3435

Open
justin808 wants to merge 2 commits into
mainfrom
justin808/fix-3416
Open

Stream Pro rolling-deploy bundle downloads with caps#3435
justin808 wants to merge 2 commits into
mainfrom
justin808/fix-3416

Conversation

@justin808
Copy link
Copy Markdown
Member

@justin808 justin808 commented May 27, 2026

Summary

  • Stream Pro HTTP rolling-deploy bundle downloads into a Tempfile instead of buffering the response body in memory.
  • Enforce a 50 MB compressed-response cap before tarball extraction, including oversized non-2xx bundle responses that must be drained by Net::HTTP.
  • Keep the existing 200 MB uncompressed tarball extraction cap and warn/degrade through the existing rolling-deploy adapter failure path.

Fixes #3416.

Pull Request checklist

  • Add/update test to cover these changes
  • Update documentation
  • Update CHANGELOG file

Test plan

  • cd react_on_rails_pro && bundle exec rspec spec/react_on_rails_pro/rolling_deploy_adapters/http_spec.rb
  • cd react_on_rails_pro && bundle exec rubocop --ignore-parent-exclusion
  • corepack pnpm exec prettier --check CHANGELOG.md
  • git diff --check

Review notes

  • The review concern about non-2xx /bundles/:hash bodies was valid. The adapter now drains those responses through the same compressed-byte cap while discarding chunks, preventing Net::HTTP from materializing an oversized error body after the request block exits.
  • CodeRabbit's docstring coverage warning is not actionable for this Ruby change. The repo does not require docstrings on these private helper methods, and adding them would not improve the tested behavior here.

Note

Low Risk
Localized refactor of Pro rolling-deploy HTTP streaming with added tests; no auth or public API surface changes beyond safer response handling.

Overview
Pro HTTP rolling-deploy bundle download handling now routes all streamed bodies through a shared each_capped_body_chunk helper so the 50 MB compressed cap applies whether the response is saved or discarded.

On non-success GET /bundles/:hash responses, the adapter drains the body through that cap (via drain_response_body) instead of stopping without consuming the stream—so oversized error payloads still trip the same abort/cleanup path as successful downloads. Success paths still write chunks to the Tempfile through stream_response_body.

CHANGELOG adds the PR attribution for the streaming/cap fix. Specs cover 404 + oversized body draining and cap enforcement.

Reviewed by Cursor Bugbot for commit 01ddf15. Bugbot is set up for automated code reviews on this repo. Configure here.

Summary by CodeRabbit

  • Bug Fixes

    • Rolling-deploy HTTP adapter now streams bundle downloads and enforces a 50 MB compressed-body cap to prevent heap exhaustion; the existing 200 MB uncompressed tarball limit remains and non-success responses are handled with warnings.
  • Tests

    • Added tests verifying streamed downloads, mid-stream abort and cleanup when the compressed cap is exceeded, and associated warning logging.
  • Documentation

    • Added changelog entry describing the fix.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 27, 2026

Walkthrough

Streams rolling-deploy bundle tarballs into a Tempfile with a 50 MB compressed-body cap (COMPRESSED_BODY_CAP), refactors HTTP helper construction to share request building, and adds specs plus a CHANGELOG entry documenting the change.

Changes

Streaming Bundle Downloads with Compressed-Body Cap

Layer / File(s) Summary
Streaming download and compression cap enforcement
react_on_rails_pro/lib/react_on_rails_pro/rolling_deploy_adapters/http.rb
Adds tempfile require, COMPRESSED_BODY_CAP constant, updates fetch to use a block-based download_bundle_tarball flow, implements streaming HTTP response writes into a Tempfile with compressed-byte counting and enforcement, and adapts extract_payload to read from the streamed tempfile. Includes RuboCop suppression around the Http class.
HTTP Request Helper Consolidation
react_on_rails_pro/lib/react_on_rails_pro/rolling_deploy_adapters/http.rb
Introduces http_for and build_request to centralize connection/timeout/SSL setup and routes http_get/http_stream through the shared request builder; http_stream now captures and returns the response object after yielding and sets Accept-Encoding: identity.
Test Suite and Documentation
react_on_rails_pro/spec/react_on_rails_pro/rolling_deploy_adapters/http_spec.rb, CHANGELOG.md
Adds fileutils require; introduces .fetch spec validating streaming to tempfile, extraction return, and mid-stream abort/cleanup with warning log when compressed body exceeds cap; adds compose_tarball_from_strings(entries) helper; updates CHANGELOG.md with the new Pro changelog entry.

Sequence Diagram

sequenceDiagram
  participant fetch as fetch(bundle_hash)
  participant download as download_bundle_tarball
  participant stream as http_stream
  participant tempfile as Tempfile
  participant extract as extract_payload
  participant error as ReactOnRailsPro::Error
  
  fetch->>download: yield block
  download->>stream: stream HTTP response
  stream->>tempfile: write chunks
  tempfile->>tempfile: track compressed bytes
  alt exceeds COMPRESSED_BODY_CAP
    tempfile->>error: raise error
    error->>download: cleanup & return nil
  else within cap
    tempfile->>extract: flush & rewind
    extract->>download: return extracted paths
    download->>fetch: return result
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

  • Issue #3416: Implements streaming to Tempfile with COMPRESSED_BODY_CAP and associated refactor/tests described by that issue.

Suggested labels

full-ci

Poem

🐇 I nibble chunks and write to file,

Counting bytes with careful guile.
Fifty meg cap keeps my memory fine,
Tempfile cradles every bundle line.
Extracted treasures, ready to align.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 7.14% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: implementing streaming for Pro rolling-deploy bundle downloads with size caps.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch justin808/fix-3416

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fef4bdcd61

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@CHANGELOG.md`:
- Line 41: Update the CHANGELOG.md entry that starts with "HTTP rolling-deploy
adapter now streams bundle downloads with a compressed-size cap" to append the
required PR/author suffix in the format "[PR ####](link) by [username](link)";
locate the line containing that exact entry text and add the PR number and
author links at the end of the sentence so the entry matches the repository
changelog formatting guidelines.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1726acaa-2e02-4242-95d5-c2b37f5b009d

📥 Commits

Reviewing files that changed from the base of the PR and between 33695e5 and fef4bdc.

📒 Files selected for processing (3)
  • CHANGELOG.md
  • react_on_rails_pro/lib/react_on_rails_pro/rolling_deploy_adapters/http.rb
  • react_on_rails_pro/spec/react_on_rails_pro/rolling_deploy_adapters/http_spec.rb

Comment thread CHANGELOG.md Outdated
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 27, 2026

Greptile Summary

This PR replaces the buffered response.body download path in the HTTP rolling-deploy adapter with a streaming download into a Tempfile, adding a 50 MB compressed-body cap (COMPRESSED_BODY_CAP) enforced chunk-by-chunk before the existing 200 MB uncompressed-tarball cap runs at extraction time. Two focused specs cover the streaming path and the mid-stream cap rejection.

  • download_bundle_tarball is rewritten to use Tempfile.create + http_stream instead of http_get; Tarball.extract already handles both String and IO sources (line 118 of tarball.rb), so extract_payload works with the new Tempfile argument without changes.
  • stream_response_body accumulates chunk byte counts before the cap check and writes the chunk only when the running total has not exceeded COMPRESSED_BODY_CAP, so at most COMPRESSED_BODY_CAP − 1 bytes are written to the tempfile before an over-cap raise. Tempfile.create's ensure deletes the file even when a non-local return or an exception exits the block.

Confidence Score: 5/5

Safe to merge. The streaming + cap logic is correct, cleanup is guaranteed by Tempfile.create's ensure, and both new code paths are covered by specs.

The buffered-to-streaming migration is well-contained. Tarball.extract already handles both String and IO sources, so the Tempfile argument to extract_payload is a safe drop-in. The COMPRESSED_BODY_CAP is checked before writing each chunk, keeping the tempfile always below the cap. Non-local returns and mid-stream exceptions both trigger Tempfile cleanup correctly.

No files require special attention.

Important Files Changed

Filename Overview
react_on_rails_pro/lib/react_on_rails_pro/rolling_deploy_adapters/http.rb Core change: buffered response.body replaced with http_stream + Tempfile.create + per-chunk COMPRESSED_BODY_CAP guard; cleanup, non-local return, and exception propagation are all handled correctly.
react_on_rails_pro/spec/react_on_rails_pro/rolling_deploy_adapters/http_spec.rb Two new specs added for .fetch: one exercises the chunk-streaming path end-to-end and one verifies mid-stream cap rejection with directory cleanup.
CHANGELOG.md Changelog entry added for the streaming download with compressed-size cap, correctly referencing the issue and describing the behavior change.

Reviews (1): Last reviewed commit: "Prevent rolling deploy download heap exh..." | Re-trigger Greptile

@justin808 justin808 force-pushed the justin808/fix-3416 branch from fef4bdc to da7b7af Compare May 27, 2026 01:54
@coderabbitai coderabbitai Bot added the full-ci label May 27, 2026
@justin808 justin808 changed the title Stream rolling deploy bundle downloads Stream Pro rolling-deploy bundle downloads with caps May 27, 2026
@justin808
Copy link
Copy Markdown
Member Author

Mattered

  • Addressed react_on_rails_pro/lib/react_on_rails_pro/rolling_deploy_adapters/http.rb: non-2xx bundle responses now drain through the same compressed-byte cap before skipping the hash, preventing Net::HTTP from materializing oversized error bodies. Fixed in 01ddf15 and resolved the thread.
  • Addressed CHANGELOG.md: added the required [PR 3435](https://github.com/shakacode/react_on_rails/pull/3435) by [justin808](https://github.com/justin808) suffix. Fixed in 01ddf15 and resolved the thread.

Optional

  • No optional inline review items remained after the fixes.

Discuss

  • CodeRabbit's docstring coverage warning was reviewed as advisory, not a blocker. This PR changes private Ruby helper behavior with focused specs; adding docstrings would be low-value unless a maintainer wants that policy changed.

Skipped

  • Skipped status-only/generated summaries and the later CodeRabbit/Greptile safe-to-merge summaries because they had no actionable change request.

Scan window: full PR history, because no previous <!-- address-review-summary --> marker existed. Future full-PR scans should start after this comment unless check all reviews is requested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stream rolling-deploy tarball download to a Tempfile with a compressed-body cap

1 participant