Skip to content

Commit 03f0d26

Browse files
committed
fix(flowcontrol): map pre-admission TTL/cancel to eviction statuses instead of 500
A TTL expiry or client disconnect before an item reaches a queue (buffered in the enqueue channel, blocked in SubmitOrBlock, or racing managedQueue.Add) finalizes as QueueOutcomeRejectedOther, which the translator mapped to 500. Once the item is queued, the same event maps to 503 with a dropped-reason header. These pre-admission windows are widest under overload, so a saturation stall with TTL expiries showed up as a spike of 500s that was really backpressure. The translator now recognizes ErrTTLExpired and ErrContextCancelled in the RejectedOther/EvictedOther arm (after the shutdown check, which keeps precedence) and delegates to the matching eviction mapping, so the two paths cannot drift apart. The same branches cover EvictedOther for symmetry; nothing currently emits it with these sentinels. Separately, tryDistribution finalized a ManagedQueue lookup failure for a leased flow as QueueOutcomeRejectedCapacity, reporting an internal invariant violation as 429 with the Saturated header. It now finalizes as RejectedOther: a 500 with no saturation header. This also keeps the RejectedCapacity metric label to genuine capacity rejections. The registry error is flattened with %v because wrapping it would let ErrPriorityBandNotFound flow back through the connection closure and trip the priority-0 fallback in withConnectionWithFallback. Fixes #2097 Part of #1187 Signed-off-by: Luke Van Drie <lukevandrie@google.com>
1 parent 34478d1 commit 03f0d26

4 files changed

Lines changed: 68 additions & 7 deletions

File tree

pkg/epp/flowcontrol/controller/controller.go

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -355,7 +355,13 @@ func (fc *FlowController) tryDistribution(
355355
fc.logger.Error(err,
356356
"Invariant violation. Failed to get ManagedQueue for a leased flow.",
357357
"flowKey", conn.FlowKey())
358-
item.FinalizeWithOutcome(types.QueueOutcomeRejectedCapacity, types.ErrRejected)
358+
// An internal invariant violation, not a capacity condition: finalize as RejectedOther so it
359+
// surfaces as an internal error rather than as saturation backpressure. The registry error is
360+
// flattened with %v because this finalized error is returned through the connection closure in
361+
// EnqueueAndWait: a %w-preserved ErrPriorityBandNotFound would be misread by
362+
// withConnectionWithFallback as a lease-acquisition failure and silently retried at priority 0.
363+
item.FinalizeWithOutcome(types.QueueOutcomeRejectedOther,
364+
fmt.Errorf("%w: failed to get ManagedQueue for leased flow: %v", types.ErrRejected, err))
359365
return item, err
360366
}
361367

pkg/epp/flowcontrol/controller/controller_test.go

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -397,16 +397,21 @@ func TestFlowController_EnqueueAndWait(t *testing.T) {
397397
h := newUnitHarness(t.Context(), t, &Config{}, mockRegistry, nil)
398398

399399
// Create a faulty setup that successfully leases the flow but fails to return the
400-
// ManagedQueue. This setup should be considered as unavailable.
400+
// ManagedQueue. The error wraps ErrPriorityBandNotFound (the realistic registry failure) to
401+
// prove the sentinel does not leak into the finalized error, where
402+
// withConnectionWithFallback would misread it as a lease-acquisition failure and retry at
403+
// priority 0.
401404
faultyRegistry := &mocks.MockRegistryDataPlane{
402405
ManagedQueueFunc: func(_ flowcontrol.FlowKey) (contracts.ManagedQueue, error) {
403-
return nil, errors.New("invariant violation: queue retrieval failed")
406+
return nil, fmt.Errorf("invariant violation: %w", contracts.ErrPriorityBandNotFound)
404407
},
405408
}
409+
var connectionAttempts int
406410
mockRegistry.WithConnectionFunc = func(
407411
key flowcontrol.FlowKey,
408412
fn func(conn contracts.ActiveFlowConnection) error,
409413
) error {
414+
connectionAttempts++
410415
return fn(&mockActiveFlowConnection{
411416
RegistryV: faultyRegistry,
412417
FlowKeyV: key,
@@ -417,8 +422,12 @@ func TestFlowController_EnqueueAndWait(t *testing.T) {
417422
outcome, err := h.fc.EnqueueAndWait(context.Background(), req)
418423
require.Error(t, err, "EnqueueAndWait must reject requests if queue doesn't exist for flow")
419424
assert.ErrorIs(t, err, types.ErrRejected, "error should wrap ErrRejected")
420-
assert.Equal(t, types.QueueOutcomeRejectedCapacity, outcome,
421-
"outcome should be QueueOutcomeRejectedCapacity when queue doesn't exist for the flow")
425+
assert.NotErrorIs(t, err, contracts.ErrPriorityBandNotFound,
426+
"registry sentinels must not leak into the finalized error")
427+
assert.Equal(t, types.QueueOutcomeRejectedOther, outcome,
428+
"outcome should be QueueOutcomeRejectedOther when queue doesn't exist for the flow")
429+
assert.Equal(t, 1, connectionAttempts,
430+
"an invariant violation on a leased flow must not trigger the priority-0 fallback retry")
422431
})
423432
})
424433

pkg/epp/requestcontrol/admission.go

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -242,10 +242,20 @@ func translateFlowControlOutcome(outcome types.QueueOutcome, err error) error {
242242
case types.QueueOutcomeEvictedContextCancelled:
243243
return errcommon.Error{Code: errcommon.ServiceUnavailable, Msg: "client disconnected: " + msg, Headers: map[string]string{errcommon.RequestDroppedReasonHeaderKey: string(errcommon.RequestDroppedReasonContextCancelled)}}
244244
case types.QueueOutcomeRejectedOther, types.QueueOutcomeEvictedOther:
245-
if errors.Is(err, types.ErrFlowControllerNotRunning) {
245+
switch {
246+
case errors.Is(err, types.ErrFlowControllerNotRunning):
246247
return errcommon.Error{Code: errcommon.ServiceUnavailable, Msg: "flow controller shutting down: " + msg, Headers: map[string]string{errcommon.RequestDroppedReasonHeaderKey: string(errcommon.RequestDroppedReasonShuttingDown)}}
248+
// A TTL expiry or client disconnect that fires before the item is admitted to a queue (e.g. while
249+
// buffered in the enqueue channel or blocked in submission) surfaces as RejectedOther/EvictedOther
250+
// rather than as a dedicated eviction outcome. These are client-caused terminations, so delegate
251+
// to the mapping of the post-admission equivalent; the two paths then agree by construction.
252+
case errors.Is(err, types.ErrTTLExpired):
253+
return translateFlowControlOutcome(types.QueueOutcomeEvictedTTL, err)
254+
case errors.Is(err, types.ErrContextCancelled):
255+
return translateFlowControlOutcome(types.QueueOutcomeEvictedContextCancelled, err)
256+
default:
257+
return errcommon.Error{Code: errcommon.Internal, Msg: "internal flow control error: " + msg}
247258
}
248-
return errcommon.Error{Code: errcommon.Internal, Msg: "internal flow control error: " + msg}
249259
default:
250260
return errcommon.Error{Code: errcommon.Internal, Msg: "unhandled flow control outcome: " + msg}
251261
}

pkg/epp/requestcontrol/admission_test.go

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package requestcontrol
1919
import (
2020
"context"
2121
"errors"
22+
"fmt"
2223
"testing"
2324

2425
"github.com/stretchr/testify/assert"
@@ -364,6 +365,41 @@ func TestTranslateFlowControlOutcome(t *testing.T) {
364365
wantCode: errcommon.ServiceUnavailable,
365366
wantReason: string(errcommon.RequestDroppedReasonShuttingDown),
366367
},
368+
{
369+
name: "pre-admission TTL rejection maps like TTL eviction",
370+
outcome: fctypes.QueueOutcomeRejectedOther,
371+
err: fmt.Errorf("%w: %w", fctypes.ErrRejected, fctypes.ErrTTLExpired),
372+
wantCode: errcommon.ServiceUnavailable,
373+
wantReason: string(errcommon.RequestDroppedReasonTTLExpired),
374+
},
375+
{
376+
name: "pre-admission cancellation rejection maps like cancellation eviction",
377+
outcome: fctypes.QueueOutcomeRejectedOther,
378+
err: fmt.Errorf("%w: %w", fctypes.ErrRejected, fctypes.ErrContextCancelled),
379+
wantCode: errcommon.ServiceUnavailable,
380+
wantReason: string(errcommon.RequestDroppedReasonContextCancelled),
381+
},
382+
{
383+
name: "other TTL eviction maps like TTL eviction",
384+
outcome: fctypes.QueueOutcomeEvictedOther,
385+
err: fmt.Errorf("%w: %w", fctypes.ErrEvicted, fctypes.ErrTTLExpired),
386+
wantCode: errcommon.ServiceUnavailable,
387+
wantReason: string(errcommon.RequestDroppedReasonTTLExpired),
388+
},
389+
{
390+
name: "other cancellation eviction maps like cancellation eviction",
391+
outcome: fctypes.QueueOutcomeEvictedOther,
392+
err: fmt.Errorf("%w: %w", fctypes.ErrEvicted, fctypes.ErrContextCancelled),
393+
wantCode: errcommon.ServiceUnavailable,
394+
wantReason: string(errcommon.RequestDroppedReasonContextCancelled),
395+
},
396+
{
397+
name: "shutdown takes precedence over TTL",
398+
outcome: fctypes.QueueOutcomeRejectedOther,
399+
err: fmt.Errorf("%w: %w", fctypes.ErrFlowControllerNotRunning, fctypes.ErrTTLExpired),
400+
wantCode: errcommon.ServiceUnavailable,
401+
wantReason: string(errcommon.RequestDroppedReasonShuttingDown),
402+
},
367403
{
368404
name: "internal error returns 500",
369405
outcome: fctypes.QueueOutcomeRejectedOther,

0 commit comments

Comments
 (0)