Preserve PVE authentication endings while reading responses (#3136)

## Summary

`TestClientConstructionHonoursContext` (`pkg/proxmox`) has been failing
the required "Backend tests (rest-0)" check on unrelated PRs: #3132
(`json/cancel`) and #3134 (`form/cancel`) on 23 Oct, each with
`authentication failed: invalid authentication response: expected a
complete ticket and CSRF token`.

**Cause.** When a PVE password login ends while its response is being
read (the caller cancels, the caller's deadline passes, or the client's
own login timeout expires), Go's HTTP transport can still return the
server's answer to the abandoned request. The fixture answers a departed
client with an empty 200, and the PVE client reported that answer as a
malformed session instead of the context error. The PBS client got this
fix on 18 Oct (b6e56c5baa); the PVE client never did.
`TestClientConstructionHonoursClientTimeout` hits the same race while
its caller stays live, so checking only the caller's context in
`NewClientWithContext` would not have fixed it.

**Change.**
- `authenticatePassword`, shared by initial login and detached renewal:
when an exchange fails after its login context ended, report the ending;
keep transport errors that already carry it; start no form attempt after
an ended JSON refusal. Complete sessions are still accepted,
construction still discards a client whose caller ended, and renewal's
detached login can only be ended by its own bounded timeout.
- New deterministic control
`TestClientEndedAuthResponseOwnsErrorAndSession` (wrapper in the
registered proof file `host_diagnostics_regression_test.go`). It ends
the login from inside the response read, by caller cancellation and by a
simulated login deadline, for JSON/form × empty 200, invalid 200, 415,
401 and body-read failure, with no wall-clock timers. It also checks
that both encoding attempts carry the one login budget that
`authenticate` derives from the client timeout.
- The TLS fixture now aborts with `http.ErrAbortHandler` instead of
answering a departed client with an empty 200.
- Contract note in `monitoring.md`, next to the PBS note.

## Evidence

pulse-dev, go1.26.10 (the CI toolchain), `go test -race -count=200 -run
<test> ./pkg/proxmox/`:

| Tree | `TestClientConstructionHonoursContext` |
`TestClientConstructionHonoursClientTimeout` |
|---|---|---|
| main at 3b5d789aeb | **4/200 failed** (form/cancel ×2, json/cancel,
json/deadline) | **2/200 failed** (form/client-timeout ×2) |
| fix only, original empty-200 fixture | 0/200 | 0/200 |
| this PR (fix + aborting fixture) | 0/200 | 0/200 |

Every baseline failure was `invalid authentication response: expected a
complete ticket and CSRF token`. The race needs CPU contention (the
first baseline failures all landed while race builds compiled on the
same box), so further loops ran beside niced CPU burners and at
GOMAXPROCS 1 and 2. Across all loops, main failed 19 of 1,400 runs. The
fix with the original empty-200 fixture failed 0 of 1,600. In an
instrumented copy, the new rule caught two live occurrences of that
exact error, and both runs passed. In the one run paired with main under
identical contention, main failed 4 of 200 and the fix 0 of 200.

New control: against main's `client.go`, all 20 cases fail on each of
three runs, including the CI error itself. With the fix, 200 of 200 runs
pass under `-race`. A mutation in which `authenticate` passes the
caller's context instead of its login budget fails all 10 cancellation
cases on the budget check. The full `pkg/proxmox` suite passes under
`-race`.
This commit is contained in:
rcourtman
2026-10-23 14:12:43 +01:00
committed by GitHub
4 changed files with 145 additions and 2 deletions
@@ -4260,6 +4260,26 @@ during response reading for JSON/form, complete and incomplete success,
encoding and credential refusal, and body-read failure. The real TLS
constructor cancellation/deadline and shared-renewal controls remain separate.
### PVE authentication keeps an ended login's outcome
When a PVE password exchange ends while its response is being read (the caller
cancels, the caller's deadline passes, or the client's own login timeout
expires), the login now reports that context error. The server's answer to the
abandoned exchange, such as an empty 200 or a refusal, is no longer reported as
a malformed session or a credential refusal, and an ended JSON refusal does not
start a form attempt. Transport errors that already report the ending keep
their wording. A complete session is still accepted, and construction still
discards a client whose caller has ended. Shared renewal gets the same rule
through the common password exchange, where only its own bounded timeout can
end the detached login. `TestClientEndedAuthResponseOwnsErrorAndSession` ends
the login during response reading, by caller cancellation and by a simulated
login deadline, for JSON/form, empty and invalid success, encoding and
credential refusal, and body-read failure. It also checks that both encoding
attempts carry the one login budget. The real TLS construction fixture now
aborts a request whose client has left instead of answering it with an empty
200; reading that answer as a malformed session is what made
`TestClientConstructionHonoursContext` fail intermittently in CI.
### AI Discovery removed (2026-10-11)
The [AI runtime retirement boundary](ai-runtime.md#ai-discovery-removed-2026-10-11)
+8 -1
View File
@@ -446,11 +446,18 @@ func (c *Client) authenticate(ctx context.Context) error {
return c.authenticatePassword(loginCtx, auth.user+"@"+auth.realm)
}
// A response can finish just as ctx ends. The server's answer to that
// abandoned exchange, such as an empty or refused response, is not a
// credential diagnosis: an ended exchange tries no other encoding and reports
// the ending, keeping a transport error that already carries it.
func (c *Client) authenticatePassword(ctx context.Context, username string) error {
err := c.authenticateJSON(ctx, username, c.config.Password)
if shouldFallbackToForm(err) {
if ctx.Err() == nil && shouldFallbackToForm(err) {
err = c.authenticateForm(ctx, username, c.config.Password)
}
if ended := ctx.Err(); err != nil && ended != nil && !errors.Is(err, ended) {
return ended
}
return err
}
+113 -1
View File
@@ -77,7 +77,8 @@ func testClientConstructionHonoursContext(t *testing.T, endings ...string) {
select {
case <-r.Context().Done():
close(requestEnded)
return
// Abort: returning would answer the departed client with an empty 200.
panic(http.ErrAbortHandler)
case <-release:
}
}
@@ -169,3 +170,114 @@ func testClientConstructionHonoursContext(t *testing.T, endings ...string) {
}
}
}
// End the login while its response is consumed, after HTTP has already
// returned an unusable 200 or a refusal. This reproduces the constructor race
// without scheduler timing.
type endingAuthBody struct {
io.Reader
end func()
}
func (r endingAuthBody) Read(p []byte) (int, error) {
r.end()
return r.Reader.Read(p)
}
// The caller's deadline and the client's own login timeout both reach the
// password exchange as a login context ending with DeadlineExceeded. The
// response read ends this one, so no wall-clock timer races the answer.
type endedLoginDeadline struct {
context.Context
done <-chan struct{}
}
func (c endedLoginDeadline) Done() <-chan struct{} { return c.done }
func (c endedLoginDeadline) Err() error {
select {
case <-c.done:
return context.DeadlineExceeded
default:
return nil
}
}
func testClientEndedAuthResponseOwnsErrorAndSession(t *testing.T) {
const good = `{"data":{"ticket":"recovered-ticket","CSRFPreventionToken":"recovered-csrf"}}`
for _, ending := range []string{"cancel", "deadline"} {
for _, encoding := range []string{"json", "form"} {
for _, outcome := range []struct {
name, body string
status int
readError bool
}{
{"empty-success", ``, http.StatusOK, false},
{"invalid-success", `{}`, http.StatusOK, false},
{"encoding-refusal", `{}`, http.StatusUnsupportedMediaType, false},
{"credential-refusal", `{}`, http.StatusUnauthorized, false},
{"body-read-failure", good, http.StatusOK, true},
} {
t.Run(ending+"/"+encoding+"/"+outcome.name, func(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
deadline := make(chan struct{})
end, want := func() { cancel() }, context.Canceled
if ending == "deadline" {
end, want = sync.OnceFunc(func() { close(deadline) }), context.DeadlineExceeded
}
calls, recovering := 0, false
var budget time.Time
c := &Client{
baseURL: "https://pve.fixture.invalid/api2/json",
config: ClientConfig{Password: "fixture-password"},
auth: auth{user: "fixture", realm: "pam"},
}
c.httpClient = &http.Client{Transport: roundTripperFunc(func(r *http.Request) (*http.Response, error) {
calls++
if ending == "cancel" && !recovering {
// authenticate gives every encoding attempt one login budget.
deadline, ok := r.Context().Deadline()
if !ok || (!budget.IsZero() && !deadline.Equal(budget)) {
t.Error("login attempts lost their shared login budget")
}
budget = deadline
}
if encoding == "form" && r.Header.Get("Content-Type") == "application/json" {
return &http.Response{StatusCode: http.StatusUnsupportedMediaType, Body: io.NopCloser(strings.NewReader(`{}`)), ContentLength: -1}, nil
}
if recovering {
return &http.Response{StatusCode: http.StatusOK, Body: io.NopCloser(strings.NewReader(good)), ContentLength: -1}, nil
}
var body io.Reader = strings.NewReader(outcome.body)
if outcome.readError {
body = sessionAdmissionReadError{body}
}
return &http.Response{StatusCode: outcome.status, Body: io.NopCloser(endingAuthBody{body, end}), ContentLength: -1}, nil
})}
before, generation := c.authSnapshot()
var err error
if ending == "deadline" {
err = c.authenticatePassword(endedLoginDeadline{context.Background(), deadline}, "fixture@pam")
} else {
err = c.authenticate(ctx)
}
if after, afterGeneration := c.authSnapshot(); !errors.Is(err, want) || after != before || afterGeneration != generation {
t.Errorf("ended login response lost its ending or changed the session: error=%v", err)
}
wantCalls := 1
if encoding == "form" {
wantCalls = 2
}
if calls != wantCalls {
t.Errorf("ended login tried another encoding: calls=%d", calls)
}
recovering = true
if err := c.authenticate(context.Background()); err != nil || c.auth.ticket != "recovered-ticket" || c.auth.csrfToken != "recovered-csrf" || calls != 2*wantCalls {
t.Errorf("distinct later login did not recover: error=%v calls=%d", err, calls)
}
})
}
}
}
}
@@ -167,6 +167,10 @@ func TestClientConstructionHonoursClientTimeout(t *testing.T) {
testClientConstructionHonoursContext(t, "client-timeout")
}
func TestClientEndedAuthResponseOwnsErrorAndSession(t *testing.T) {
testClientEndedAuthResponseOwnsErrorAndSession(t)
}
func TestClusterCancelledCallerStartsNoSession(t *testing.T) {
testClusterCancelledCallerStartsNoSession(t)
}