[thinker0]

Description

When an ALB TSM pool member returns a non-2xx status code (e.g., 400 Bad Request with an error JSON payload), TSM was unconditionally calling prepareGatherContribution to unmarshal the response body as a timeseries dataset. This caused spurious decode failure warnings in logs and hindered clean error propagation.

Root Cause

In pkg/backends/alb/mech/tsm/time_series_merge.go (and merge_plan.go), prepareGatherContribution was invoked without checking whether the member's captured response was a successful 2xx response:

1. When a backend returns an error envelope (such as 400 Bad Request with {"status":"error","errorType":"bad_data",...}), prepareGatherContribution passed the non-timeseries payload into rsc.TSUnmarshaler.
2. The JSON timeseries unmarshaler failed to parse the error envelope, logging:
   WARN tsm gather timeseries decode failure member=N error="..."
3. Furthermore, in time_series_merge.go, the status code sc was calculated after the contribution processing block rather than before it.

Solution

1. Status check guard: Compute sc prior to contribution processing and guard prepareGatherContribution with an is2xx check (sc >= 200 && sc < 300) in both serveStandard (time_series_merge.go) and collectPlanResult (merge_plan.go).
2. Clean error handling: For non-2xx responses, timeseries unmarshaling is bypassed entirely, and the member is recorded as failed: true while preserving its original status code and headers for proper aggregation by aggregateStatus and pickWinner.

Test Coverage

Added two unit tests in pkg/backends/alb/mech/tsm/time_series_merge_test.go:

• TestTSMNon2xxMemberDoesNotAttemptDecode: Verifies that a 400 Bad Request member response does not trigger TSUnmarshaler, and partial failure with a 2xx member aggregates to 200 OK with status=phit.
• TestTSMAllMembers400DoesNotAttemptDecode: Verifies that when all members return 400 Bad Request, TSUnmarshaler is not invoked and the 400 status is properly propagated.

All existing unit tests in pkg/backends/alb/mech/tsm/... and pkg/proxy/engines/... pass.

Type of Change

• Bug fix
• New feature
• Optimization
• Test coverage
• Documentation
• Infrastructure

AI Disclosure

• This contribution DOES NOT include AI-generated changes
• This contribution DOES include AI-generated changes, and I have reviewed the relevant contributing guidelines.

[jranson]

@thinker0 thanks for this. I have done a review and have 2 findings:

• when all fanout requests return >=400, this change causes a Trickster-generated empty 502 error from the internal responder, since the error body remains a nil []byte at time_series_merge.go L584. Trickster must return the same error body/code as one of the fanout requests when all of them fail. For example, if all fanouts return a 400 due to an invalid query syntax, then Trickster must do so as well. If all fanouts return errors but they are different (some 400's, some 502's, some 504's, etc.) then it might make sense to return the lowest error since a 4xx is usually due to user error and we should tell them what was wrong with the original request.
• The spies at time_series_merge_test.go L313 and L357 do not actually work to test for regressions, because they use the parent resources, while a fanout creates fresh resources. Since the spy stubs don't setup TSUnmarshaler or TimeRangeQuery both tests would actually pass as-is regardless of any changes in time_series_merge.go. Make sure the regression tests actually prove the fixed regressions they claim to prove.

[thinker0]

@jranson Thank you for the thoughtful review and catching these edge cases! I have addressed both findings in commit 1415655:

1. Error Response Body & Status Propagation (pickErrorWinner):

   • When all fanouts return errors (!has2xx), Trickster now selects the winning failed request using pickErrorWinner, preferring user client errors (lowest 4xx, e.g. 400 Bad Request) over upstream server errors (5xx).
   • The winning error's status code, custom headers (including Content-Type), aggregated X-Trickster-Result header, and raw error response body are written directly to the client response, avoiding generating an empty 502 or attempting to marshal an unpopulated accumulator.
   • Guarded prepareGatherContribution so that non-2xx responses without timeseries data do not attempt accumulator merges.

2. Regression Test Spies Attached to Child Resources:

   • Updated the unit tests (TestTSMNon2xxMemberDoesNotAttemptDecode, TestTSMAllMembers400DoesNotAttemptDecode) to attach TSUnmarshaler and TimeRangeQuery directly to the child member's request.Resources inside the handler stubs. Verified that timeseries unmarshaling is never invoked for 400 responses.
   • Verified that the exact 400 error JSON payload ({"status":"error","errorType":"bad_data","error":"Forbidden: missing label"}) and Content-Type: application/json are returned to the client when all members fail with 400.
   • Added TestTSMAllMembersFailedMixedErrorsSurfaces4xx to verify that when mixed errors (502, 400, 504) occur, the 400 user error code and body win over the 5xx errors.
   • Added unit test TestPickErrorWinner.

[jranson]

Here's the linter failure:

  Error: pkg/backends/alb/mech/tsm/replica_groups.go:251:36: identical-ifelseif-branches: "if...else if" chain with identical branches (lines 251 and 253) (revive)
  				if isCurr4xx && !isFallback4xx {
  					fallback = result
  				} else if isCurr4xx == isFallback4xx && result.statusCode < fallback.statusCode {
  1 issues:
  * revive: 1

[thinker0]

Fixed the revive linter failure in commit 081b0b1b by merging the identical branches. Thanks!

[jranson]

@thinker0, it looks like the new error-forwarding branch only runs in serveStandard. It needs to be in serveMultiVariantPlan too. Also one more suggestion below for memory consumption.

[jranson]

it looks like rawBody and rawHeader below are being created from Clones unconditionally, even though that data is only used for error responses. Can you gate these Clones behind an error response check, so healthy responses don't get cloned?

[thinker0]

@jranson Thank you for the insightful review! I have updated the PR with both requested changes:

1. Gated memory allocations behind !is2xx:

   • bytes.Clone(fr.Capture.Body()) and fr.Capture.Header().Clone() in both serveStandard and collectPlanResult are now executed strictly when !is2xx. Healthy 2xx responses avoid all clone allocations.

2. Added error-forwarding to serveMultiVariantPlan:

   • When all variants/members in a multi-variant plan return non-2xx errors (!hasAny2xx), serveMultiVariantPlan now selects the winning error via pickErrorWinner and forwards its status code, headers, and body directly to the client instead of attempting accumulator reduction or emitting a synthetic error.
   • Added unit test TestServeWeightedAvg/all_variants_non_2xx_forwards_winning_error verifying this behavior end-to-end.