fix(proxy): honor TLSSkipVerify for proxied requests

[s0up4200]

Fix 502 errors for proxied write requests (e.g. POST /api/v2/torrents/add) when the qBittorrent instance has TLSSkipVerify enabled.

The reverse proxy previously always used sharedhttp.Transport, ignoring the instance client's configured HTTP transport where TLSSkipVerify is applied. Now uses a per-request transport selector to pick the instance client's transport when available.

Summary by CodeRabbit

• New Features

  • Proxy now supports flexible, per-request HTTP transport routing based on request context.

• Bug Fixes

  • Enhanced retry logic with improved exponential backoff and connection cleanup.

• Tests

  • Added comprehensive test coverage for transport selection, retry behavior, and request handling.

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

[coderabbitai]

Walkthrough

The pull request implements per-request transport selection in the proxy layer. A new selector-based transport mechanism allows proxy requests to route through instance-specific HTTP client transports obtained from request context, replacing the previous static shared transport approach.

Changes

Cohort / File(s)	Summary
Core Transport Refactoring internal/proxy/retry_transport.go	Adds NewRetryTransportWithSelector() constructor and baseSelector field to enable per-request transport routing. Introduces transportFor() to resolve the effective transport, refactors retry logic into retryBackoff() and waitForRetry() helpers, and updates closeIdleConnections() to accept a base transport parameter.
Proxy Handler Integration internal/proxy/handler.go	Replaces NewRetryTransport() with NewRetryTransportWithSelector(), passing a selector function that inspects the request context for a proxyContext and extracts its HTTP client's Transport, enabling context-aware per-request transport selection.
Test Coverage internal/proxy/handler_test.go, internal/proxy/retry_transport_test.go	Adds TestHandler_ProxyUsesInstanceHTTPClientTransport() to verify instance transport injection. Expands retry transport test suite with TestRetryTransport_SelectorOverridesBase() and additional scenarios for retryable errors, idempotency, backoff behavior, context cancellation, and idle connection management.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Handler as Proxy Handler
    participant RetryTransport as Retry Transport
    participant Selector
    participant ProxyCtx as Request Context
    participant InstanceTransport as Instance Transport
    participant DefaultTransport as Default Transport

    Client->>Handler: Send proxy request
    Handler->>RetryTransport: RoundTrip(request)
    
    rect rgb(240, 248, 255)
        Note over RetryTransport,Selector: Per-Request Transport Selection
        RetryTransport->>Selector: Call selector(request)
        Selector->>ProxyCtx: Extract proxyContext from request
        alt Context has HTTP client with Transport
            ProxyCtx-->>Selector: Return instance HTTP client
            Selector->>InstanceTransport: Use instance transport
            Selector-->>RetryTransport: Return instance transport
        else No valid context
            Selector-->>RetryTransport: Return nil
            RetryTransport->>DefaultTransport: Fall back to base transport
        end
    end
    
    rect rgb(255, 250, 240)
        Note over RetryTransport: Execute Request
        RetryTransport->>InstanceTransport: RoundTrip(request)
        InstanceTransport-->>RetryTransport: Response
    end
    
    RetryTransport-->>Handler: Response
    Handler-->>Client: Response

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

• KyleSanderson
• Audionut

Poem

🐰 A selector hops through request streams,
Per-instance transports fuel our dreams,
Context-aware routing, clean and bright,
No more static paths—dynamic flight!
Retry logic blooms, refactored with care. 🌟

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title 'fix(proxy): honor TLSSkipVerify for proxied requests' directly matches the primary change: enabling per-request transport selection to honor TLSSkipVerify configuration for proxied requests.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/proxy-tls-skipverify

---

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2f27c0d and 4d2cad1.

📒 Files selected for processing (4)

• internal/proxy/handler.go
• internal/proxy/handler_test.go
• internal/proxy/retry_transport.go
• internal/proxy/retry_transport_test.go

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-12-28T18:44:10.496Z

Learnt from: s0up4200
Repo: autobrr/qui PR: 876
File: internal/logstream/hub_test.go:188-192
Timestamp: 2025-12-28T18:44:10.496Z
Learning: In Go 1.25 (Aug 2025), use wg.Go(func()) to spawn a goroutine and automate the Add/Done lifecycle. Replace manual patterns like wg.Add(1); go func(){ defer wg.Done(); ... }() with wg.Go(func(){ ... }). Ensure the codebase builds with Go 1.25+ and apply this in relevant Go files (e.g., internal/logstream/hub_test.go). If targeting older Go versions, maintain the existing pattern.

Applied to files:

• internal/proxy/handler_test.go
• internal/proxy/retry_transport.go
• internal/proxy/handler.go
• internal/proxy/retry_transport_test.go

🧬 Code graph analysis (4)

internal/proxy/handler_test.go (2)

internal/proxy/handler.go (1)

• NewHandler (71-102)

internal/proxy/retry_transport.go (1)

• RetryTransport (29-34)

internal/proxy/retry_transport.go (1)

pkg/redact/redact.go (2)

• String (107-118)
• URLString (28-78)

internal/proxy/handler.go (1)

internal/proxy/retry_transport.go (1)

• NewRetryTransportWithSelector (45-50)

internal/proxy/retry_transport_test.go (1)

internal/proxy/retry_transport.go (2)

• NewRetryTransport (37-41)
• NewRetryTransportWithSelector (45-50)

🔇 Additional comments (7)

internal/proxy/handler_test.go (1)

194-226: LGTM! Excellent test coverage for per-request transport selection.

The test effectively validates that:

• The handler configures the RetryTransport with a selector
• Per-request transports from the proxyContext are used when available
• The base transport is bypassed (0 attempts) when a selector provides an override
• The selected transport is actually invoked (1 attempt)

The test design is clean and uses appropriate assertions to verify the selector mechanism.

internal/proxy/handler.go (1)

87-93: LGTM! Per-request transport selector correctly implements TLS configuration routing.

The selector properly:

• Extracts the proxyContext from the request
• Guards against nil at each level (context, proxyContext, httpClient, Transport)
• Returns the instance-specific transport when available, ensuring TLSSkipVerify is honored
• Falls back to the base transport when no per-request transport is available

This change directly addresses the root cause of 502 errors for proxied requests to instances with TLSSkipVerify enabled.

internal/proxy/retry_transport_test.go (1)

1-569: LGTM! Comprehensive test coverage for retry transport behavior.

The test suite thoroughly validates:

• Per-request transport selection via NewRetryTransportWithSelector (lines 65-94)
• Retry logic for various retryable network errors (ECONNREFUSED, ECONNRESET, EOF, etc.)
• Non-retry behavior for non-retryable errors and non-idempotent methods
• Max retry limits and exponential backoff timing
• Context cancellation handling
• Idle connection management
• Helper function correctness

The tests use appropriate patterns:

• Context-aware request construction with http.NewRequestWithContext
• Proper resource cleanup with t.Cleanup(func() { _ = resp.Body.Close() })
• Clear test case naming and comprehensive assertions

internal/proxy/retry_transport.go (4)

31-60: LGTM! Clean per-request transport selection mechanism.

The new selector-based architecture is well-designed:

• baseSelector field enables per-request transport customization while maintaining backward compatibility
• NewRetryTransportWithSelector provides the new API without breaking existing callers of NewRetryTransport
• transportFor properly handles all nil cases and fallback logic

The approach ensures instance-specific TLS configuration (like TLSSkipVerify) is honored for proxied requests.

---

62-121: LGTM! Well-structured retry logic with proper error handling.

The refactored retry helpers improve code quality:

• retryBackoff consolidates all retry decision logic in one place
• Properly closes idle connections on the resolved per-request transport (not the default base)
• Validates idempotency before retrying
• Enforces max retry limits
• Calculates exponential backoff with appropriate caps
• Uses redact.String() and redact.URLString() for secure logging

waitForRetry correctly respects context cancellation with the appropriate //nolint:wrapcheck directive.

---

126-162: LGTM! RoundTrip correctly delegates to per-request transports.

The updated RoundTrip implementation:

• Resolves the per-request transport once at line 129 using transportFor
• Uses the resolved transport consistently for all retry attempts
• Delegates retry decisions to the new retryBackoff helper
• Properly waits for backoff while respecting context cancellation
• Maintains existing success-case behavior and logging

The refactoring improves maintainability while preserving correctness.

---

166-175: LGTM! Idle connection closing operates on the correct transport.

The updated closeIdleConnections now accepts a base http.RoundTripper parameter and uses it for the type assertion. This ensures idle connections are closed on the per-request transport (which may have instance-specific TLS settings) rather than the default shared transport.

This is a critical correctness fix for the per-request transport selection feature.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[netlify]

✅ Deploy Preview for getqui ready!

Name	Link
🔨 Latest commit	44c8fee
🔍 Latest deploy log	https://app.netlify.com/projects/getqui/deploys/69596e17d3d1e2000898213a
😎 Deploy Preview	https://deploy-preview-1051--getqui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.