🧹 chore: Improve byte-range handling for SendFile() #3870
Conversation
Summary of ChangesHello @gaby, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on enhancing the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughPrecomputes file size for Range requests in SendFile, sets Content-Range "bytes */" for 416 responses when known, adds helper to get file size for OS and fs.FS backends, adds comprehensive byte-range tests (skipped on Windows), and centralizes a Windows OS constant. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant H as HTTP Handler
participant S as SendFile
participant G as sendFileContentLength
C->>H: GET /file with Range header
H->>S: call SendFile (cfg.ByteRange=true)
alt Range header present
S->>G: stat file (path, cfg)
G-->>S: file size (n)
S->>S: evaluate requested range vs n
alt satisfiable
S->>C: 206 Partial Content + Content-Range
else unsatisfiable
S->>S: set Header: Content-Range: bytes */n
S->>C: 416 Requested Range Not Satisfiable
end
else no Range or ByteRange disabled
S->>C: 200 OK (full file)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
Tip 📝 Customizable high-level summaries are now available!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
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 |
gaby
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3870 +/- ##
==========================================
- Coverage 91.69% 91.58% -0.12%
==========================================
Files 115 115
Lines 9819 9845 +26
==========================================
+ Hits 9004 9017 +13
- Misses 516 527 +11
- Partials 299 301 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully moves the byte-range tests for SendFile into ctx_test.go and adds a fix to correctly set the Content-Range header for unsatisfiable range requests. The new tests are comprehensive and cover a wide range of scenarios. I've provided a couple of suggestions to improve the maintainability of the new code. One suggestion is to refactor the new test function to use a table-driven approach, which will reduce code duplication and make it easier to add new test cases in the future. The other suggestion simplifies the path cleaning logic in the new sendFileContentLength function.
There was a problem hiding this comment.
Pull Request Overview
This PR moves the RFC 9110 SendFile byte-range regression tests into ctx_test.go and adds proper error wrapping in the sendFileContentLength helper function to satisfy linting requirements.
- Refactors byte-range tests to use
t.TempDir()for fixture management - Adds wrapped errors with context in
sendFileContentLengthfunction - Implements Content-Range header support for 416 Range Not Satisfiable responses
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| res.go | Adds sendFileContentLength helper function with error wrapping and implements Content-Range header handling for unsatisfiable byte-range requests |
| ctx_test.go | Moves comprehensive byte-range test suite inline with temporary directory fixtures covering all RFC 9110 range request scenarios |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
res.go (1)
881-901: Tighten fs.FS path normalization; consider future-proofing size type.
- Use path.Clean on slash paths for fs.FS to avoid ad-hoc trimming and handle oddities like repeated slashes.
@@ -func sendFileContentLength(path string, cfg SendFile) (int, error) { +func sendFileContentLength(path string, cfg SendFile) (int, error) { if cfg.FS != nil { - cleanPath := utils.TrimLeft(path, '/') - for strings.HasPrefix(cleanPath, "./") { - cleanPath = strings.TrimPrefix(cleanPath, "./") - } + // fs.FS expects forward slashes and relative paths + cleanPath := pathpkg.Clean(strings.TrimPrefix(path, "/")) info, err := fs.Stat(cfg.FS, cleanPath) if err != nil { return 0, fmt.Errorf("stat %q: %w", cleanPath, err) } return int(info.Size()), nil }Add import alias for clarity:
@@ -import ( +import ( "bufio" "bytes" + pathpkg "path"
- Optional: if large files are a concern on 32‑bit, returning int64 here (and threading it through) would avoid overflow; current int usage is consistent with existing Range(length int) API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ctx_test.go(1 hunks)res.go(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
ctx_test.go
🧬 Code graph analysis (2)
ctx_test.go (2)
res.go (1)
SendFile(24-60)constants.go (7)
MethodGet(5-5)HeaderRange(236-236)StatusPartialContent(58-58)HeaderAcceptRanges(233-233)HeaderContentRange(234-234)StatusRequestedRangeNotSatisfiable(89-89)StatusOK(52-52)
res.go (1)
constants.go (3)
HeaderRange(236-236)StatusRequestedRangeNotSatisfiable(89-89)HeaderContentRange(234-234)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Agent
- GitHub Check: update_release_draft
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: lint
- GitHub Check: Compare
🔇 Additional comments (3)
ctx_test.go (2)
4658-4845: Solid byte-range test coverage; compliant with repo linting rule.
- Exercises satisfiable/unsatisfiable, open-ended, suffix, and disabled cases.
- No require assertions inside handlers; good. Based on learnings
4666-4673: Add FS-backed range test.
- Add a route using FS to ensure size detection and headers work with fs.FS.
- Note: fasthttp.FS does not support multipart/byteranges responses for multiple byte ranges; multipart/byteranges tests are not applicable unless custom handling is implemented.
@@ - app.Get("/range", func(c Ctx) error { + app.Get("/range", func(c Ctx) error { return c.SendFile(fixture, SendFile{ByteRange: true}) }) + app.Get("/rangefs", func(c Ctx) error { + return c.SendFile("fixture.txt", SendFile{ + ByteRange: true, + FS: os.DirFS(tmpDir), + }) + }) @@ t.Run("suffix range", func(t *testing.T) { req := httptest.NewRequest(http.MethodGet, "/range", http.NoBody) @@ }) + + t.Run("fs-backed suffix range", func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/rangefs", http.NoBody) + req.Header.Set(HeaderRange, "bytes=-3") + resp, err := app.Test(req) + require.NoError(t, err) + defer func() { require.NoError(t, resp.Body.Close()) }() + require.Equal(t, StatusPartialContent, resp.StatusCode) + require.Equal(t, "bytes 7-9/10", resp.Header.Get(HeaderContentRange)) + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.Equal(t, content[len(content)-3:], body) + })Also applies to: 4788-4800
res.go (1)
823-839: Correct 416 Content-Range handling for ranged requests.
- Size is computed only when needed and header set iff absent; aligns with RFC 9110 expectations.
- Good separation from fasthttp FS behavior; no regressions to 200/206 path.
Also applies to: 867-869
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
res.go (1)
12-14: Byte-range size handling and 416Content-Rangebehavior look correct; consider usingint64for file sizeThe new logic to:
- detect Range requests when
ByteRangeis enabled,- compute the file size for FS vs OS backends via
sendFileContentLength, and- set
Content-Range: bytes */sizewhen responding withStatusRequestedRangeNotSatisfiableand no header is present,is consistent and works nicely with both
cfg.FSand direct filesystem paths. SwallowingsendFileContentLengtherrors is acceptable here since failure just means we skip the extra header and defer to the existing behavior.One improvement to consider is avoiding the conversion from
int64(FileInfo.Size()) down toint, which can overflow on 32‑bit platforms for large files. You can keep the size asint64end‑to‑end and format it withstrconv.FormatInt:@@ - var ( - sendFileSize int - hasSendFileSize bool - ) + var ( + sendFileSize int64 + hasSendFileSize bool + ) @@ - if size, err := sendFileContentLength(sizePath, cfg); err == nil { + if size, err := sendFileContentLength(sizePath, cfg); err == nil { sendFileSize = size hasSendFileSize = true } @@ - if cfg.ByteRange && hasSendFileSize && response.StatusCode() == StatusRequestedRangeNotSatisfiable && len(response.Header.Peek(HeaderContentRange)) == 0 { - response.Header.Set(HeaderContentRange, "bytes */"+strconv.Itoa(sendFileSize)) + if cfg.ByteRange && hasSendFileSize && response.StatusCode() == StatusRequestedRangeNotSatisfiable && len(response.Header.Peek(HeaderContentRange)) == 0 { + response.Header.Set(HeaderContentRange, "bytes */"+strconv.FormatInt(sendFileSize, 10)) } @@ -func sendFileContentLength(path string, cfg SendFile) (int, error) { +func sendFileContentLength(path string, cfg SendFile) (int64, error) { if cfg.FS != nil { - cleanPath := pathpkg.Clean(utils.TrimLeft(path, '/')) + cleanPath := pathpkg.Clean(utils.TrimLeft(path, '/')) if cleanPath == "." { cleanPath = "" } info, err := fs.Stat(cfg.FS, cleanPath) if err != nil { return 0, fmt.Errorf("stat %q: %w", cleanPath, err) } - return int(info.Size()), nil + return info.Size(), nil } info, err := os.Stat(filepath.FromSlash(path)) if err != nil { return 0, fmt.Errorf("stat %q: %w", path, err) } - return int(info.Size()), nil + return info.Size(), nil }If you prefer to keep
intfor now, the current behavior is still functionally correct on 64‑bit platforms.Please double-check that this change matches your target Go architectures (32‑bit vs 64‑bit) and that no other callers rely on
sendFileContentLengthreturningint.Also applies to: 824-840, 866-871, 882-901
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ctx_test.go(2 hunks)prefork.go(3 hunks)res.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ctx_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
res.go (1)
constants.go (3)
HeaderRange(236-236)StatusRequestedRangeNotSatisfiable(89-89)HeaderContentRange(234-234)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: repeated
- GitHub Check: Compare
- GitHub Check: update_release_draft
🔇 Additional comments (1)
prefork.go (1)
21-26: Centralized Windows checks viagoosWindowslook goodUsing a single
goosWindowsconstant and switching bothwatchMasteranddummyCmdto compare against it keeps the OS check consistent without changing behavior. No issues spotted.Also applies to: 160-170, 186-195
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
res.go (1)
824-839: Consider logging errors fromsendFileContentLengthfor debugging.The error from
sendFileContentLengthis silently ignored (line 835-838). While this is acceptable behavior since the size precomputation is an optimization for proper 416 responses, silent failures can complicate debugging when byte-range requests don't work as expected.Note: This precomputation causes the file to be stat'ed twice (once here, once by fasthttp when serving), but this trade-off is necessary to support proper Content-Range headers on 416 responses per RFC 7233.
Consider adding debug logging when size computation fails:
if size, err := sendFileContentLength(sizePath, cfg); err == nil { sendFileSize = size hasSendFileSize = true + } else { + // Optionally log for debugging: app.config.Logger.Debug("failed to compute file size for range request", "path", sizePath, "error", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
res.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
res.go (1)
constants.go (3)
HeaderRange(236-236)StatusRequestedRangeNotSatisfiable(89-89)HeaderContentRange(234-234)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
- GitHub Check: lint
- GitHub Check: update_release_draft
🔇 Additional comments (3)
res.go (3)
12-13: LGTM! Clean import additions.The
osandpathpkgimports are necessary for the new file size helper function. Thepathpkgalias appropriately avoids naming conflicts with thepathparameter used throughout the codebase.
868-870: LGTM! RFC 7233 compliant 416 response handling.This correctly sets the
Content-Rangeheader tobytes */sizeformat for 416 Range Not Satisfiable responses when the file size is known. The implementation properly checks all necessary conditions and follows RFC 7233 section 4.4 requirements.
882-901: LGTM! Implementation is correct and consistent with codebase patterns.Verified that the path handling in both branches is sound:
- fs.FS case: Properly cleans paths and handles the root case by converting
"."to""- OS case: Correctly uses
filepath.FromSlashto convert separators, consistent with other uses in the codebase (res.go:804, ctx_test.go:4579-4588)Windows compatibility is already managed in the codebase: SendFile operations skip on Windows where needed (ctx_test.go:4661), and
filepath.FromSlashhandles cross-platform paths appropriately.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: b5a6d87 | Previous: 3bf25e8 | Ratio |
|---|---|---|---|
Benchmark_NewError |
70.39 ns/op 24 B/op 1 allocs/op |
44.92 ns/op 24 B/op 1 allocs/op |
1.57 |
Benchmark_NewError - ns/op |
70.39 ns/op |
44.92 ns/op |
1.57 |
This comment was automatically generated by workflow using github-action-benchmark.
|
Let me fix the conflict |
Summary