Conversation
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRefactors many index-based loops to value-based range iterations; replaces ad-hoc host:port parsing with proper parsing/joining (url.Parse, net.SplitHostPort, net.JoinHostPort) and IPv6 bracket handling; updates tests to use test-scoped contexts and adds TLS/address parsing tests. No public API changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Modernizes the codebase (and especially tests) to use newer Go 1.26 idioms/APIs and simplifies a number of loops and string operations.
Changes:
- Update tests/benchmarks to use
t.Context()/b.Context()andb.Loop(). - Refactor multiple index-based loops to
for _, v := range ...for clarity. - Replace some
strings.Split(...)[0]patterns withstrings.Cut(...).
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/uploads_test.go | Use t.Context() for pool creation; fix one errors.Is call ordering. |
| tests/http_plugin_test.go | Use t.Context() for request contexts. |
| tests/http_plugin3_test.go | Remove context import; use t.Context() for request contexts. |
| tests/http_plugin2_test.go | Remove context import; use t.Context() for request contexts and other Go 1.26 idioms. |
| tests/handler_test.go | Use t.Context()/b.Context() and b.Loop() in benchmarks. |
| status.go | Simplify worker iteration using range values. |
| servers/https/https.go | Use strings.Cut in tlsAddr; simplify middleware loop. |
| servers/https/config.go | Refactor SSL address parsing/validation logic. |
| servers/http3/http3.go | Simplify middleware loop. |
| servers/http11/http.go | Simplify middleware loop. |
| servers/fcgi/fcgi.go | Simplify middleware loop. |
| plugin.go | Simplify server shutdown iteration. |
| middleware/redirect.go | Use strings.Cut in TLSAddr. |
| metrics.go | Simplify worker state iteration during Prometheus collection. |
| init.go | Simplify bundled middleware application loop and logging. |
| handler/response.go | Simplify HTTP/2 push iteration. |
| config/uploads.go | Simplify allow/forbid list initialization loops. |
| acme/acme.go | Simplify domain iteration when obtaining certs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/uploads_test.go (1)
32-41:⚠️ Potential issue | 🟡 MinorDestroy each
staticPoolexplicitly.These tests create worker pools with
t.Context(), but none of them callspl.Destroy(...). That leaves subprocess cleanup to test-context cancellation at the very end of the test and can make failures or early exits leak workers longer than intended. Add at.Cleanup(func() { pl.Destroy(t.Context()) })immediately after each successfulNewPool.♻️ Representative cleanup change
pl, err := staticPool.NewPool(t.Context(), ... ) if err != nil { t.Fatal(err) } + t.Cleanup(func() { + pl.Destroy(t.Context()) + })Also applies to: lines 125-134, 219-228, 312-321, 405-414
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/uploads_test.go` around lines 32 - 41, New pools created via staticPool.NewPool (assigned to variable pl) are not explicitly destroyed, risking subprocess leaks; after each successful NewPool call add a cleanup hook like t.Cleanup(func() { pl.Destroy(t.Context()) }) so the pool is torn down promptly; apply this immediately after the NewPool returns (for the occurrences around staticPool.NewPool in tests/uploads_test.go and the other listed blocks) to ensure pl.Destroy is invoked with the test context.servers/https/https.go (1)
189-193:⚠️ Potential issue | 🟠 Major
tlsAddrcorrupts IPv6 listen addresses by splitting on the first colon instead of the port separator.Line 189 uses
strings.Cut(host, ":")which splits at the first:character. For IPv6 addresses like[::1]:8443, this produces[as the host, which then becomes[:8443when the port is appended—an invalid listen address. IPv4 addresses work by accident since they have only one colon.Replace with
net.SplitHostPortandnet.JoinHostPortto properly handle both IPv4 and IPv6 addresses:Suggested fix
+import ( + "net" + "strconv" +) + -func tlsAddr(host string, forcePort bool, sslPort int) string { - // remove current forcePort first - host, _, _ = strings.Cut(host, ":") +func tlsAddr(addr string, forcePort bool, sslPort int) string { + host := addr + if parsedHost, _, err := net.SplitHostPort(addr); err == nil { + host = parsedHost + } if forcePort || sslPort != 443 { - host = fmt.Sprintf("%s:%v", host, sslPort) + return net.JoinHostPort(host, strconv.Itoa(sslPort)) } return host }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@servers/https/https.go` around lines 189 - 193, The code that strips the port uses strings.Cut(host, ":") which breaks IPv6 addresses; update the logic (in the tlsAddr building code around the host variable and sslPort) to call net.SplitHostPort(host) and, if it succeeds, use the returned host part; if SplitHostPort returns an error (no port), keep host as-is. When you need to append/force a port (when forcePort || sslPort != 443), use net.JoinHostPort(host, strconv.Itoa(sslPort)) to produce a correct IPv4/IPv6 listen address. Ensure imports for net and strconv are added and handle SplitHostPort errors by falling back to the original host.
🧹 Nitpick comments (1)
tests/handler_test.go (1)
88-103: Use the test/benchmark context for pool teardown too.Most of these tests now create the pool with
t.Context()/b.Context(), but teardown still usesDestroy(context.Background()). That drops the deadline/cancellation semantics you just introduced and makes stuck worker shutdown harder to abort. Prefert.Cleanup(func() { p.Destroy(t.Context()) })and theb.Context()equivalent.♻️ Representative teardown update
- defer func() { - p.Destroy(context.Background()) - }() + t.Cleanup(func() { + p.Destroy(t.Context()) + })Also applies to: 163-178, 237-252, 311-326, 387-402, 461-476, 531-546, 601-616, 693-708, 786-801, 883-898, 976-991, 1070-1085, 1163-1178, 1298-1313, 1433-1448, 1571-1586, 1627-1642, 1683-1698, 1839-1854, 1896-1911
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/handler_test.go` around lines 88 - 103, Tests create pools with t.Context()/b.Context() but call p.Destroy(context.Background()), losing cancellation/deadline; update teardowns to register cleanup with the test/benchmark context instead. Replace direct defer or explicit Destroy calls that pass context.Background() with t.Cleanup(func() { p.Destroy(t.Context()) }) (or b.Cleanup with p.Destroy(b.Context()) in benchmarks), targeting the pool variable (p) returned by staticPool.NewPool and its Destroy method so teardown uses the same context used for creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@middleware/redirect.go`:
- Around line 30-33: The current use of strings.Cut on host corrupts IPv6
bracketed literals; update the parsing/rewriting in middleware/redirect.go (the
block handling host, forcePort, sslPort) to use net.SplitHostPort to extract
host and port when present and net.JoinHostPort to reconstruct host:port
(preserving [::1] form) when forcing a port or sslPort != 443, and apply the
same change to the tlsAddr logic in servers/https/https.go (lines around
function tlsAddr) so both IPv6 and no-port hosts are handled correctly; ensure
you handle the “no port present” error path from SplitHostPort by treating the
whole input as host and only joining a port when needed.
In `@servers/https/config.go`:
- Around line 100-113: The address-parsing in the config initialization
currently uses strings.Cut on s.Address and strconv.Atoi for port, which breaks
IPv6 addresses and doesn't enforce 0–65535; replace that logic to use
net.SplitHostPort(s.Address) to correctly extract host and port (handling
bracketed IPv6), set s.host to the returned host (defaulting to "127.0.0.1" if
empty), and parse the port with strconv.ParseUint(portStr, 10, 16) (or
equivalent) to validate the port range before assigning s.Port; ensure all
errors are wrapped/returned the same way as existing code (e.g., via
errors.E(op, ...)) and update any related variables in the same function where
host, portStr, and s.Port are set.
In `@tests/uploads_test.go`:
- Around line 68-70: Several upload test functions still call errors.Is with
reversed arguments (errors.Is(http.ErrServerClosed, err)), which prevents
detection of wrapped http.ErrServerClosed; locate each occurrence inside the
upload tests (the goroutine handling server shutdown where errL or err is
compared) and swap the arguments to errors.Is(err, http.ErrServerClosed).
Specifically update the remaining calls that use the wrong order so they match
the corrected pattern used earlier (errors.Is(errL, http.ErrServerClosed) /
errors.Is(err, http.ErrServerClosed)), ensuring any variable names (errL, err)
are used as the first argument to errors.Is.
---
Outside diff comments:
In `@servers/https/https.go`:
- Around line 189-193: The code that strips the port uses strings.Cut(host, ":")
which breaks IPv6 addresses; update the logic (in the tlsAddr building code
around the host variable and sslPort) to call net.SplitHostPort(host) and, if it
succeeds, use the returned host part; if SplitHostPort returns an error (no
port), keep host as-is. When you need to append/force a port (when forcePort ||
sslPort != 443), use net.JoinHostPort(host, strconv.Itoa(sslPort)) to produce a
correct IPv4/IPv6 listen address. Ensure imports for net and strconv are added
and handle SplitHostPort errors by falling back to the original host.
In `@tests/uploads_test.go`:
- Around line 32-41: New pools created via staticPool.NewPool (assigned to
variable pl) are not explicitly destroyed, risking subprocess leaks; after each
successful NewPool call add a cleanup hook like t.Cleanup(func() {
pl.Destroy(t.Context()) }) so the pool is torn down promptly; apply this
immediately after the NewPool returns (for the occurrences around
staticPool.NewPool in tests/uploads_test.go and the other listed blocks) to
ensure pl.Destroy is invoked with the test context.
---
Nitpick comments:
In `@tests/handler_test.go`:
- Around line 88-103: Tests create pools with t.Context()/b.Context() but call
p.Destroy(context.Background()), losing cancellation/deadline; update teardowns
to register cleanup with the test/benchmark context instead. Replace direct
defer or explicit Destroy calls that pass context.Background() with
t.Cleanup(func() { p.Destroy(t.Context()) }) (or b.Cleanup with
p.Destroy(b.Context()) in benchmarks), targeting the pool variable (p) returned
by staticPool.NewPool and its Destroy method so teardown uses the same context
used for creation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4e3fbf1-646a-46a4-ab30-69694005a704
📒 Files selected for processing (18)
acme/acme.goconfig/uploads.gohandler/response.goinit.gometrics.gomiddleware/redirect.goplugin.goservers/fcgi/fcgi.goservers/http11/http.goservers/http3/http3.goservers/https/config.goservers/https/https.gostatus.gotests/handler_test.gotests/http_plugin2_test.gotests/http_plugin3_test.gotests/http_plugin_test.gotests/uploads_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #226 +/- ##
=======================================
Coverage 61.20% 61.20%
=======================================
Files 6 6
Lines 250 250
=======================================
Hits 153 153
Misses 81 81
Partials 16 16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/uploads_test.go (2)
173-179:⚠️ Potential issue | 🟡 MinorResource leak: opened file
fis never closed; misleading error message.The file
fopened at line 173 is never closed (unlike inTestHandler_Upload_Filewhich hasdefer f.Close()). Additionally, the error message says "failed to close a file" but this defer is closing the HTTP serverhs, not the file.🔧 Proposed fix
f := mustOpen(testFile) defer func() { - errC := hs.Close() + errC := f.Close() if errC != nil { t.Errorf("failed to close a file: error %v", errC) } }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/uploads_test.go` around lines 173 - 179, The opened file f from mustOpen(testFile) is never closed and the defer that closes hs logs a misleading message; add a defer f.Close() immediately after f := mustOpen(testFile) to avoid the resource leak, and update the defer that closes hs (the func that calls hs.Close()) to log a message referring to the HTTP server (e.g., "failed to close server" or "failed to close http server hs") instead of "failed to close a file" while keeping the existing errC error handling.
266-272:⚠️ Potential issue | 🟡 MinorSame resource leak pattern: opened file never closed.
Same issue as
TestHandler_Upload_NestedFile: the filefopened at line 266 is never closed, and the defer block closeshs(server) while the error message incorrectly references "a file". This pattern repeats inTestHandler_Upload_File_Forbids(lines 359-365) andTestHandler_Upload_File_NotAllowed(lines 452-458).🔧 Proposed fix for all three functions
Replace
hs.Close()withf.Close()in the defer blocks at lines 268, 361, and 454.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/uploads_test.go` around lines 266 - 272, The defer blocks in the tests are closing the HTTP server variable hs instead of the opened file f, causing a file handle leak and incorrect error messages; in each affected test (TestHandler_Upload_NestedFile, TestHandler_Upload_File_Forbids, TestHandler_Upload_File_NotAllowed) change the defer to close f (call f.Close()) and update the error message to reference the file close failure, leaving mustOpen(...) and the existing error handling otherwise unchanged.
🧹 Nitpick comments (1)
middleware/redirect_test.go (1)
27-27: Test expectations may need adjustment for bare IPv6.If the
TLSAddrfunction is updated to wrap bare IPv6 addresses in brackets (as suggested in theredirect.goreview), these test expectations should be updated from"::1"to"[::1]"to reflect valid URI syntax.Also applies to: 32-32
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/redirect_test.go` at line 27, Update the test expectations in middleware/redirect_test.go to match the TLSAddr behavior for bare IPv6 by changing the expected host output from "::1" to "[::1]" in the test cases referencing "ipv6 with port, default ssl port" (and the other similar case on the later line); locate the table of test cases used by TestTLSAddr and replace the raw IPv6 expectation with the bracketed form so the assertions align with TLSAddr's canonicalized IPv6 output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@middleware/redirect.go`:
- Around line 27-37: TLSAddr currently returns bare IPv6 hosts when sslPort==443
and forcePort==false, causing invalid URLs when assigned to url.URL.Host in
Redirect; update TLSAddr to detect IPv6 literals (use strings.Contains(host,
":") or net.ParseIP) and wrap them in brackets (e.g., "[::1]") when not
returning a port, re-add the "strings" import if used, and update tests that
assert bare IPv6 (lines referencing TLSAddr/Redirect expectations) to expect
bracketed IPv6 instead.
---
Outside diff comments:
In `@tests/uploads_test.go`:
- Around line 173-179: The opened file f from mustOpen(testFile) is never closed
and the defer that closes hs logs a misleading message; add a defer f.Close()
immediately after f := mustOpen(testFile) to avoid the resource leak, and update
the defer that closes hs (the func that calls hs.Close()) to log a message
referring to the HTTP server (e.g., "failed to close server" or "failed to close
http server hs") instead of "failed to close a file" while keeping the existing
errC error handling.
- Around line 266-272: The defer blocks in the tests are closing the HTTP server
variable hs instead of the opened file f, causing a file handle leak and
incorrect error messages; in each affected test (TestHandler_Upload_NestedFile,
TestHandler_Upload_File_Forbids, TestHandler_Upload_File_NotAllowed) change the
defer to close f (call f.Close()) and update the error message to reference the
file close failure, leaving mustOpen(...) and the existing error handling
otherwise unchanged.
---
Nitpick comments:
In `@middleware/redirect_test.go`:
- Line 27: Update the test expectations in middleware/redirect_test.go to match
the TLSAddr behavior for bare IPv6 by changing the expected host output from
"::1" to "[::1]" in the test cases referencing "ipv6 with port, default ssl
port" (and the other similar case on the later line); locate the table of test
cases used by TestTLSAddr and replace the raw IPv6 expectation with the
bracketed form so the assertions align with TLSAddr's canonicalized IPv6 output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef993399-bb98-4640-9b00-55bb24f695aa
📒 Files selected for processing (6)
middleware/redirect.gomiddleware/redirect_test.goservers/https/config.goservers/https/config_test.goservers/https/https.gotests/uploads_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- servers/https/https.go
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/http_plugin_test.go (1)
176-176: Consider completing context modernization across all test functions.Several helper functions in this file still use
http.NewRequestwithout context (e.g.,echoAccessLogs,sslNoRedirect,middleware,echoError,echoHTTP, etc.), while others have been updated. Some have//nolint:noctxcomments.For consistency with the modernization effort, consider updating the remaining functions to use
http.NewRequestWithContext(t.Context(), ...)and removing the nolint directives. This would require passing*testing.Tto these helper functions or restructuring them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/http_plugin_test.go` at line 176, Several test helpers still call http.NewRequest without context; update each helper (echoAccessLogs, sslNoRedirect, middleware, echoError, echoHTTP, etc.) to accept *testing.T (or otherwise obtain a context) and call http.NewRequestWithContext(t.Context(), ...) instead of http.NewRequest, then remove the corresponding //nolint:noctx comments; ensure any call sites are updated to pass the *testing.T or propagate the context so tests compile and use the test context consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/http_plugin_test.go`:
- Line 176: Several test helpers still call http.NewRequest without context;
update each helper (echoAccessLogs, sslNoRedirect, middleware, echoError,
echoHTTP, etc.) to accept *testing.T (or otherwise obtain a context) and call
http.NewRequestWithContext(t.Context(), ...) instead of http.NewRequest, then
remove the corresponding //nolint:noctx comments; ensure any call sites are
updated to pass the *testing.T or propagate the context so tests compile and use
the test context consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3255cd36-e843-445f-a691-134992cf78dc
📒 Files selected for processing (2)
handler/handler_test.gotests/http_plugin_test.go
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Reason for This PR
Description of Changes
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
Refactor
Tests
Chores