[biaobiao2233]

Description

This PR now intentionally fixes the REST forwarding half of #491 only.

forwardRequest currently derives its timeout from context.Background(), which discards cancellation from the incoming HTTP request. The outgoing Fleet API request can therefore continue after the client has canceled or disconnected.

This change derives the existing timeout from req.Context() instead:

ctx, cancel := context.WithTimeout(req.Context(), p.Timeout)

That preserves p.Timeout as an upper bound while allowing the forwarded request to observe the incoming request lifecycle.

Go documents that a server-side request context is canceled when the client connection closes, the request is canceled, or the handler returns:
https://pkg.go.dev/net/http#Request.Context

Scope

This PR deliberately leaves handleVehicleCommand unchanged from main.

During independent review of the earlier two-path version, I reproduced a dispatcher startup/cleanup interaction: propagating an already-canceled request context into Vehicle.Connect can make Dispatcher.Start return before the newly started listener is owned by the handler's normal Disconnect defer. Rather than expand this PR into dispatcher lifecycle changes, the vehicle-command half remains deferred for separate treatment.

So this change is limited to the synchronous REST forwarding path.

Addresses the REST-forwarding portion of #491.

Regression coverage

TestForwardRequestPropagatesRequestCancellation uses synchronized channels rather than timing sleeps:

1. wait until the fake upstream transport is in flight;
2. cancel the incoming request;
3. verify the outgoing request context observes context.Canceled;
4. bound the synchronization waits so a routing regression cannot hang the suite indefinitely.

Applied to parent a4b43c1eff0e09d77deb9f2dce97031141fe8c8a without the production fix, the test fails consistently because the upstream context remains uncanceled.

TestForwardRequestCancellationBoundaries adds coverage for cancellation after a 421/Alt-Svc response but before retry, an earlier parent deadline, the proxy's own timeout, and unchanged normal success. Its timeout wait is bounded.

Validation

Exact head: e98c32a05de95a79571bceb4c399b7ccf35cf599

Base: a4b43c1eff0e09d77deb9f2dce97031141fe8c8a

Current local validation with Go 1.23.0 on Linux/amd64:

• Four additional cancellation/deadline cases, three repetitions each: 12/12 passed.
• The same fixture on the pre-fix parent: the cancellation-before-retry and shorter-parent-deadline cases fail as expected (6 failed / 6 passed subcase runs).
• go test -race ./pkg/proxy: 80 tests/subtests passed, with no race report.
• Full-repository tests with coverage, go vet ./..., go build ./..., and go install ./cmd/...: passed.
• Workflow-pinned golangci-lint v1.61.0: passed with no reported issues. The local timeout was increased to 120s for the mounted Windows filesystem; the final run completed in about 20s.
• New test file formatting and git diff --check: passed.

The original implementation commit 7c5e593 is preserved. Previously recorded validation at that commit included the repeated in-flight cancellation test and parent RED check (-count=100), the dispatcher cleanup review probe after narrowing (100/100 plus race), and the original Go 1.23 / pinned-linter validation.

All new upstream requests use the repository's fake transport, synthetic JWT and test key. No Fleet API, BLE or real vehicle was exercised. Stopping a retry does not establish rollback of an already-dispatched external operation. Upstream GitHub Actions are not claimed as passed unless the workflow is authorized and executed.

Contributors

The REST-forwarding fix and original in-flight cancellation regression are by @biaobiao2233 in 7c5e593. The four additional cases were supplied by @gomission in his independent test report, independently verified and incorporated in the separate test-only commit e98c32a, with his author credit and @biaobiao2233 as co-author. The test name, unused parameter and timeout wait were adapted for the project. The shared fixture and integration were AI-assisted.

Type of change

• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Bug fix (non-breaking change which fixes an issue)
• Documentation update

Checklist

• My code follows the style of this project.
• I have performed a self-review of my code.
• No user-facing documentation changes are needed; this does not change the public API or proxy configuration.
• I have added/updated unit tests to cover my changes.

[biaobiao2233]

@patrickdemers6 When convenient, could you take a look at this proxy fix? I picked you because you reviewed #457 in the same pkg/proxy area. The fork workflow is currently awaiting maintainer approval before GitHub Actions can start; on the exact head I have already matched the workflow locally with Go 1.23.0 and golangci-lint v1.61.0, plus race and repeated in-flight cancellation tests. No rush; the branch is frozen unless review finds something substantive.

[gomission]

Independent follow-through on the REST-forwarding scope of #491, with AI assistance. biaobiao2233 owns this patch; I did not change the implementation or expand it into signed vehicle-command/dispatcher lifecycle work.

I ran the same four-case Proxy.ServeHTTP fixture on main a4b43c1eff0e09d77deb9f2dce97031141fe8c8a and this PR's exact head 7c5e593162d1c73820ea8998162740be1347f5ca, three repetitions per case:

Case	main	#492
Parent cancellation after the first 421/Alt-Svc response, before retry	a second fake upstream attempt occurs	one attempt; retry stops with the current 504/context-canceled mapping
Parent deadline shorter than p.Timeout	forwarded context uses the later proxy deadline	forwarded deadline equals the parent deadline
p.Timeout shorter than parent deadline	pass	pass
Normal success, unchanged body and one attempt	pass	pass

Main: 6 failed / 6 passed subcase runs. #492: 12 passed. The complete proxy module with this added fixture also passed under go test -race ./pkg/proxy (80 tests/subtests, no race failure) on Go 1.26.1 darwin/arm64. The first added case extends the existing in-flight cancellation test to the retry-delay boundary.

All upstream requests used the repository's fake roundTripFunc, synthetic JWT and test key. No Fleet API, BLE, vehicle, command effect, Go 1.23 CI parity, full-repository suite or rollback guarantee was exercised. Stopping retries does not establish that an already-dispatched external operation was undone.

Complete additional fixture for pkg/proxy

package proxy

import (
	"context"
	"errors"
	"net/http"
	"net/http/httptest"
	"testing"
	"time"
)

func TestMissionForwardCancellationMatrix(t *testing.T) {
	t.Run("cancel_after_redirect_before_retry", func(t *testing.T) {
		p := newTestProxy(t)
		p.Timeout = 2 * time.Second
		ctx, cancel := context.WithCancel(context.Background())
		defer cancel()
		attempts := 0
		p.client = roundTripFunc(func(req *http.Request) (*http.Response, error) {
			attempts++
			if attempts == 1 {
				cancel()
				header := make(http.Header)
				header.Set("Alt-Svc", h2Prefix+"alt.example.tesla.com")
				return jsonResponse(http.StatusMisdirectedRequest, `{}`, header), nil
			}
			return jsonResponse(http.StatusOK, `{}`, nil), nil
		})
		req := httptest.NewRequest(http.MethodGet, "/api/1/vehicles", nil).WithContext(ctx)
		req.Header.Set("Authorization", authHeader())
		rec := httptest.NewRecorder()
		p.ServeHTTP(rec, req)
		if attempts != 1 {
			t.Fatalf("upstream attempts=%d, want1 after parent cancellation", attempts)
		}
		if rec.Code != http.StatusGatewayTimeout {
			t.Fatalf("status=%d, want504 in current retry-cancellation mapping", rec.Code)
		}
	})
	t.Run("shorter_parent_deadline", func(t *testing.T) {
		p := newTestProxy(t)
		p.Timeout = 20 * time.Second
		deadline := time.Now().Add(5 * time.Second)
		ctx, cancel := context.WithDeadline(context.Background(), deadline)
		defer cancel()
		p.client = roundTripFunc(func(req *http.Request) (*http.Response, error) {
			got, ok := req.Context().Deadline()
			if !ok || !got.Equal(deadline) {
				t.Errorf("forwarded deadline=%v, want parent %v", got, deadline)
			}
			return jsonResponse(http.StatusOK, `{}`, nil), nil
		})
		req := httptest.NewRequest(http.MethodGet, "/api/1/vehicles", nil).WithContext(ctx)
		req.Header.Set("Authorization", authHeader())
		p.ServeHTTP(httptest.NewRecorder(), req)
	})
	t.Run("proxy_timeout_still_bounds_parent", func(t *testing.T) {
		p := newTestProxy(t)
		p.Timeout = 50 * time.Millisecond
		ctx, cancel := context.WithTimeout(context.Background(), time.Second)
		defer cancel()
		p.client = roundTripFunc(func(req *http.Request) (*http.Response, error) {
			parentDeadline, _ := ctx.Deadline()
			forwardedDeadline, ok := req.Context().Deadline()
			if !ok || !forwardedDeadline.Before(parentDeadline) {
				t.Errorf("proxy timeout did not shorten parent deadline")
			}
			<-req.Context().Done()
			if !errors.Is(req.Context().Err(), context.DeadlineExceeded) {
				t.Errorf("error=%v", req.Context().Err())
			}
			return nil, req.Context().Err()
		})
		req := httptest.NewRequest(http.MethodGet, "/api/1/vehicles", nil).WithContext(ctx)
		req.Header.Set("Authorization", authHeader())
		p.ServeHTTP(httptest.NewRecorder(), req)
	})
	t.Run("normal_success", func(t *testing.T) {
		p := newTestProxy(t)
		attempts := 0
		p.client = roundTripFunc(func(req *http.Request) (*http.Response, error) {
			attempts++
			if req.Context().Err() != nil {
				t.Error(req.Context().Err())
			}
			return jsonResponse(http.StatusOK, `{"ok":true}`, nil), nil
		})
		req := httptest.NewRequest(http.MethodGet, "/api/1/vehicles", nil)
		req.Header.Set("Authorization", authHeader())
		rec := httptest.NewRecorder()
		p.ServeHTTP(rec, req)
		if attempts != 1 || rec.Code != http.StatusOK || rec.Body.String() != `{"ok":true}` {
			t.Fatalf("attempts=%d status=%d body=%s", attempts, rec.Code, rec.Body.String())
		}
	})
}

[biaobiao2233]

Thanks, @gomission, for the independent validation and the additional fixture. The cancellation-before-retry and shorter-parent-deadline cases are useful additions to the existing in-flight cancellation test.

I'll independently reproduce the four cases, check them against the pre-fix parent, and incorporate the regression coverage as a separate test-only commit credited to you. I'll keep the PR limited to REST forwarding and preserve the original implementation commit. The validation will remain scoped to synthetic upstream requests; it does not establish rollback of an already-dispatched vehicle operation.

[biaobiao2233]

Thanks again, @gomission. Your four cases are now incorporated in the separate test-only commit e98c32a, with your author credit and my co-author credit. The original implementation commit 7c5e593 is preserved. I aligned the test name, removed an unused transport parameter caught by the project linter, and bounded the timeout wait.

I independently reproduced the matrix with Go 1.23.0 on Linux/amd64: 12/12 subcase runs pass on the PR; the pre-fix parent produces the same expected 6 failures / 6 passes. The complete proxy race suite passes all 80 tests/subtests. Full-repository tests with coverage, vet, build, command installation, and the pinned golangci-lint v1.61.0 also pass.

The PR remains limited to REST forwarding. These are synthetic transport tests, with no real Fleet API or vehicle operations exercised.

[sethterashima]

I don't think this will pick up the cancelled context correctly, will return StatusBadGateway.

[biaobiao2233]

Thanks for catching this. You're right: url.Error.Timeout() does not identify context.Canceled, so cancellation during client.Do can fall through to 502.

I've added explicit errors.Is checks for cancellation and deadline errors, including wrapped errors. This follows the existing retry-path mapping to 504 while preserving 502 for other transport failures.

The regression tests now check the response status as well. They reproduce the issue on the previous head and pass with the fix. Local tests, race tests, vet, build, and the pinned linter all pass.