shell OOM #93

2026-01-10T23:04:57+08:00

sky22333 commented

2026-01-10 23:04:57 +08:00

(Migrated from github.com)

Summary by CodeRabbit

Bug Fixes
- Improved error handling for script file processing—now returns appropriate error responses on failure instead of attempting fallback processing.
Improvements
- Added 10 MB size limit for shell file processing to protect against excessively large files.
- Enhanced compressed content handling with improved gzip support for better compatibility.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

## Summary by CodeRabbit * **Bug Fixes** * Improved error handling for script file processing—now returns appropriate error responses on failure instead of attempting fallback processing. * **Improvements** * Added 10 MB size limit for shell file processing to protect against excessively large files. * Enhanced compressed content handling with improved gzip support for better compatibility. ✏️ Tip: You can customize this high-level summary in your review settings.

coderabbitai[bot] commented

2026-01-10 23:05:06 +08:00

(Migrated from github.com)

📝 Walkthrough

Walkthrough

The changes modify error handling in the GitHub proxy flow for shell scripts—when smart processing fails, the handler now returns a Bad Gateway error instead of falling back to original content. Additionally, shell processing utilities are refactored to operate on byte slices, accept io.Reader instead of io.ReadCloser, introduce a 10 MB size limit, and improve error propagation.

Changes

Cohort / File(s)	Summary
Error Handling & Early Termination `src/handlers/github.go`	Modifies proxy flow to return Bad Gateway with formatted error on smart processing failure instead of falling back to original response body
Shell Processing Refactoring `src/utils/proxy_shell.go`	Adds exported constant `MaxShellSize` (10 MB); changes `ProcessSmart` and `readShellContent` signatures to accept `io.Reader` instead of `io.ReadCloser`; refactors content handling to operate on byte slices; introduces size-limiting logic; converts content type checks from string-based to `bytes.Contains`; enhances gzip decompression handling

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 A shell-bound rabbit hops with glee,
Ten megabytes is plenty, don't you see?
With bytes instead of strings, we read and flow,
When smart bits stumble, errors steal the show,
No fallback tricks—just truth, direct and clear!

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'shell OOM' is vague and does not clearly convey the main changes: adding a size limit (MaxShellSize) to prevent out-of-memory issues and refactoring error handling in shell processing.	Consider a more descriptive title such as 'Add MaxShellSize limit for shell script processing' or 'Prevent OOM by limiting shell content size' that better explains the specific changes made.

✅ Passed checks (1 passed)

Check name	Status	Explanation
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

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

<details> <summary>📝 Walkthrough</summary> ## Walkthrough The changes modify error handling in the GitHub proxy flow for shell scripts—when smart processing fails, the handler now returns a Bad Gateway error instead of falling back to original content. Additionally, shell processing utilities are refactored to operate on byte slices, accept `io.Reader` instead of `io.ReadCloser`, introduce a 10 MB size limit, and improve error propagation. ## Changes | Cohort / File(s) | Summary | |---|---| | **Error Handling & Early Termination** `src/handlers/github.go` | Modifies proxy flow to return Bad Gateway with formatted error on smart processing failure instead of falling back to original response body | | **Shell Processing Refactoring** `src/utils/proxy_shell.go` | Adds exported constant `MaxShellSize` (10 MB); changes `ProcessSmart` and `readShellContent` signatures to accept `io.Reader` instead of `io.ReadCloser`; refactors content handling to operate on byte slices; introduces size-limiting logic; converts content type checks from string-based to `bytes.Contains`; enhances gzip decompression handling | ## Estimated code review effort 🎯 3 (Moderate) | ⏱️ ~22 minutes ## Poem > 🐰 A shell-bound rabbit hops with glee,\ > Ten megabytes is plenty, don't you see?\ > With bytes instead of strings, we read and flow,\ > When smart bits stumble, errors steal the show,\ > No fallback tricks—just truth, direct and clear! </details>   <details> <summary>🚥 Pre-merge checks | ✅ 1 | ❌ 2</summary> <details> <summary>❌ Failed checks (1 warning, 1 inconclusive)</summary> | Check name | Status | Explanation | Resolution | | :----------------: | :------------- | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | :--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Docstring Coverage | ⚠️ Warning | Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. | | Title check | ❓ Inconclusive | The title 'shell OOM' is vague and does not clearly convey the main changes: adding a size limit (MaxShellSize) to prevent out-of-memory issues and refactoring error handling in shell processing. | Consider a more descriptive title such as 'Add MaxShellSize limit for shell script processing' or 'Prevent OOM by limiting shell content size' that better explains the specific changes made. | </details> <details> <summary>✅ Passed checks (1 passed)</summary> | Check name | Status | Explanation | | :---------------: | :------- | :---------------------------------------------------------- | | Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. | </details> ✏️ Tip: You can configure your own custom pre-merge checks in the settings. </details>   <details> <summary>✨ Finishing touches</summary> - [ ]  📝 Generate docstrings </details>   --- Thanks for using [CodeRabbit](https://coderabbit.ai?utm_source=oss&utm_medium=github&utm_campaign=sky22333/hubproxy&utm_content=93)! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. <details> <summary>❤️ Share</summary> - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code) </details> Comment `@coderabbitai help` to get the list of available commands and usage tips.

coderabbitai[bot] (Migrated from github.com) reviewed 2026-01-10 23:07:45 +08:00

coderabbitai[bot] (Migrated from github.com) left a comment

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/handlers/github.go (1)
206-208: Consider logging the copy error.

When io.Copy fails, the error is silently discarded. While returning early is appropriate, logging the error would help with debugging and observability.
♻️ Suggested improvement
 // 输出处理后的内容
 if _, err := io.Copy(c.Writer, processedBody); err != nil {
+	fmt.Printf("复制处理后内容失败: %v\n", err)
 	return
 }

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c6d95e683f and 685388fff9.

📒 Files selected for processing (2)

src/handlers/github.go
src/utils/proxy_shell.go

🔇 Additional comments (4)

src/utils/proxy_shell.go (4)

15-16: LGTM: Size limit addresses the OOM issue.

The 10 MB limit is a reasonable safeguard against out-of-memory conditions when processing shell scripts. This directly addresses the PR objective ("shell OOM").

19-35: Improved signature and efficiency.

The changes improve the function in several ways:

Accepting io.Reader instead of io.ReadCloser is cleaner; the caller handles resource cleanup

Byte-based content checks (line 29) are more efficient than string-based checks

Direct error propagation (line 22) provides clearer error semantics

The function correctly handles the common case (non-GitHub content) by early-returning with the original bytes.

61-71: Size limiting implementation is correct.

The size limiting logic is well-implemented:

Line 61-62: Read up to MaxShellSize + 1 bytes using LimitReader

Line 64-67: Read all available data within the limit

Line 69-71: Check if data exceeds MaxShellSize

This approach correctly detects oversized content without reading the entire file into memory, which effectively prevents the OOM issue mentioned in the PR title.

41-59: Compression handling is robust.

The peek-and-restore pattern correctly handles both gzip-compressed and uncompressed content:

Lines 42-46: Safely peek at the first 2 bytes to detect gzip magic numbers

Lines 48-55: Create a gzip reader when compression is detected

Line 49: Use io.MultiReader to restore peeked bytes, ensuring no data is lost

Line 54: Properly defer cleanup of the gzip reader

Line 57: Fallback handles the case where isCompressed is true but content isn't actually gzipped

The implementation handles edge cases well (e.g., fewer than 2 bytes available, EOF during peek).

**Actionable comments posted: 1** <details> <summary>🧹 Nitpick comments (1)</summary><blockquote> <details> <summary>src/handlers/github.go (1)</summary><blockquote> `206-208`: **Consider logging the copy error.** When `io.Copy` fails, the error is silently discarded. While returning early is appropriate, logging the error would help with debugging and observability. <details> <summary>♻️ Suggested improvement</summary> ```diff // 输出处理后的内容 if _, err := io.Copy(c.Writer, processedBody); err != nil { + fmt.Printf("复制处理后内容失败: %v\n", err) return } ``` </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between c6d95e683fe327495fed7f4f41b16277a92371c7 and 685388fff9bec0f41647798cf048223df6cd2c4e. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `src/handlers/github.go` * `src/utils/proxy_shell.go` </details> <details> <summary>🔇 Additional comments (4)</summary><blockquote> <details> <summary>src/utils/proxy_shell.go (4)</summary><blockquote> `15-16`: **LGTM: Size limit addresses the OOM issue.** The 10 MB limit is a reasonable safeguard against out-of-memory conditions when processing shell scripts. This directly addresses the PR objective ("shell OOM"). --- `19-35`: **Improved signature and efficiency.** The changes improve the function in several ways: - Accepting `io.Reader` instead of `io.ReadCloser` is cleaner; the caller handles resource cleanup - Byte-based content checks (line 29) are more efficient than string-based checks - Direct error propagation (line 22) provides clearer error semantics The function correctly handles the common case (non-GitHub content) by early-returning with the original bytes. --- `61-71`: **Size limiting implementation is correct.** The size limiting logic is well-implemented: 1. Line 61-62: Read up to `MaxShellSize + 1` bytes using `LimitReader` 2. Line 64-67: Read all available data within the limit 3. Line 69-71: Check if data exceeds `MaxShellSize` This approach correctly detects oversized content without reading the entire file into memory, which effectively prevents the OOM issue mentioned in the PR title. --- `41-59`: **Compression handling is robust.** The peek-and-restore pattern correctly handles both gzip-compressed and uncompressed content: - Lines 42-46: Safely peek at the first 2 bytes to detect gzip magic numbers - Lines 48-55: Create a gzip reader when compression is detected - Line 49: Use `io.MultiReader` to restore peeked bytes, ensuring no data is lost - Line 54: Properly defer cleanup of the gzip reader - Line 57: Fallback handles the case where `isCompressed` is true but content isn't actually gzipped The implementation handles edge cases well (e.g., fewer than 2 bytes available, EOF during peek). </blockquote></details> </blockquote></details> </details>

src/handlers/github.go

						
				@@ -177,3 +176,4 @@

							return

						}

						// 智能设置响应头

coderabbitai[bot] (Migrated from github.com) commented

2026-01-10 23:07:44 +08:00

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

cat -n src/handlers/github.go | sed -n '160,190p'

Repository: sky22333/hubproxy

Length of output: 1228

🏁 Script executed:

# Find the ProcessSmart function definition
rg -n "ProcessSmart" src/

Repository: sky22333/hubproxy

Length of output: 365

🏁 Script executed:

# Check the file size first to see if we need to read selectively
wc -l src/handlers/github.go

Repository: sky22333/hubproxy

Length of output: 87

🏁 Script executed:

cat -n src/utils/proxy_shell.go

Repository: sky22333/hubproxy

Length of output: 3571

🏁 Script executed:

# Check if there's a git log showing the change
git log --oneline -20 src/handlers/github.go 2>/dev/null || echo "Git history not available"

Repository: sky22333/hubproxy

Length of output: 78

🏁 Script executed:

# Look for any fallback patterns or conditional error handling in github.go
cat -n src/handlers/github.go | sed -n '165,210p'

Repository: sky22333/hubproxy

Length of output: 1526

🏁 Script executed:

# Check if there's a fallback behavior in the else branch (for non-.sh/.ps1 files)
cat -n src/handlers/github.go | sed -n '209,232p'

Repository: sky22333/hubproxy

Length of output: 680

🏁 Script executed:

# Look for any recent changes or comments about previous behavior
git show HEAD src/handlers/github.go 2>/dev/null | head -100 || echo "Git show not available"

Repository: sky22333/hubproxy

Length of output: 3210

Consider differentiating error handling by error type.

The error handling treats all ProcessSmart errors uniformly (returning 502). Since different failures have different implications—size limit exceeded (the OOM prevention goal) versus transient decompression or read errors—consider whether transient errors warrant different treatment such as retry logic or fallback behavior, while size-limit errors appropriately trigger 502.

_⚠️ Potential issue_ | _🟡 Minor_ <details> <summary>🧩 Analysis chain</summary> 🏁 Script executed: ```shell cat -n src/handlers/github.go | sed -n '160,190p' ``` Repository: sky22333/hubproxy Length of output: 1228 --- 🏁 Script executed: ```shell # Find the ProcessSmart function definition rg -n "ProcessSmart" src/ ``` Repository: sky22333/hubproxy Length of output: 365 --- 🏁 Script executed: ```shell # Check the file size first to see if we need to read selectively wc -l src/handlers/github.go ``` Repository: sky22333/hubproxy Length of output: 87 --- 🏁 Script executed: ```shell cat -n src/utils/proxy_shell.go ``` Repository: sky22333/hubproxy Length of output: 3571 --- 🏁 Script executed: ```shell # Check if there's a git log showing the change git log --oneline -20 src/handlers/github.go 2>/dev/null || echo "Git history not available" ``` Repository: sky22333/hubproxy Length of output: 78 --- 🏁 Script executed: ```shell # Look for any fallback patterns or conditional error handling in github.go cat -n src/handlers/github.go | sed -n '165,210p' ``` Repository: sky22333/hubproxy Length of output: 1526 --- 🏁 Script executed: ```shell # Check if there's a fallback behavior in the else branch (for non-.sh/.ps1 files) cat -n src/handlers/github.go | sed -n '209,232p' ``` Repository: sky22333/hubproxy Length of output: 680 --- 🏁 Script executed: ```shell # Look for any recent changes or comments about previous behavior git show HEAD src/handlers/github.go 2>/dev/null | head -100 || echo "Git show not available" ``` Repository: sky22333/hubproxy Length of output: 3210 --- </details> **Consider differentiating error handling by error type.** The error handling treats all ProcessSmart errors uniformly (returning 502). Since different failures have different implications—size limit exceeded (the OOM prevention goal) versus transient decompression or read errors—consider whether transient errors warrant different treatment such as retry logic or fallback behavior, while size-limit errors appropriately trigger 502.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: 3344/hubproxy#93