Skip to content

[fix][evaluation] return not found for missing target callback contexts - #701

Merged
HearyShen merged 2 commits into
mainfrom
fix/report-result-error-classification-20261023
Oct 27, 2026
Merged

HearyShen merged 2 commits into
mainfrom
fix/report-result-error-classification-20261023

Conversation

@HearyShen

@HearyShen HearyShen commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator

Missing asynchronous target contexts currently escape ReportEvalTargetInvokeResult_ as unclassified errors, including its explicit nil-context branch. This PR returns the existing ResourceNotFound business error instead. The callback is still rejected; the change corrects its error classification and stability metadata.

What type of PR is this?

fix

Check the PR title

  • The title follows [<type>][<scope>] <description>.
  • The title describes the resulting user-facing behavior.
  • This PR is written in English.

Before and after: business behavior

Scenario Before After
Redis miss, including raw, wrapped, or joined redis.Nil Return an unclassified error; the evaluation RPC wrapper reports internal error 601200702 Return ResourceNotFound (601203004)
Repository returns a nil context without an error Return unclassified errorx.New("eval async context not found...") Return the same ResourceNotFound business error as a Redis miss
Missing context: result persistence and continuation Reject immediately; do not persist a result or publish the continuation event Same: reject immediately, with no result persistence or continuation event
Redis connection failure, deadline expiry, or another non-miss error Propagate the original error Same original error and failure handling
Valid context with a successful execution result Persist the result, publish the continuation event where applicable, and acknowledge the report Same processing and success response
Valid context reporting status=FAILED Accept the report and record a failed execution; the reporting API can return success Same: acknowledging a failure report does not turn the execution into a success

The production change is confined to this application method. The existing DAO, strong-read retry contract, context TTL, authentication, request validation, and subsequent DB/MQ error paths are not modified. No new error-code definition, IDL field, or configuration is introduced.

Before and after: error response

For the evaluation integration that wraps unclassified errors with 601200702:

Contract Before After
Business status Nonzero error: 601200702 Nonzero error: 601203004
Default message meaning Internal failure; wording can be internal error or a framework-wrapped Redis error The asynchronous context cannot be found for this report
RPC biz_err_affect_stability 1, or absent and treated as stability-affecting Explicitly 0
This error's contribution to server stability metrics Classified as a server failure Classified as a business rejection
Result accepted? No No

HTTP presentation is gateway-dependent. In the inspected gateway configuration, business errors are flattened to HTTP 200 with a nonzero JSON code. That adapter also maps the stability metadata to the tt_stable response header. This PR changes the upstream business error; it does not implement an HTTP status or gateway-format change.

Illustrative responses for that configuration, before localization and omitting unrelated headers/fields:

Before:

HTTP/1.1 200 OK
tt_stable: 0
Content-Type: application/json

{"code":601200702,"msg":"internal error"}

After, using example invocation ID 101:

HTTP/1.1 200 OK
tt_stable: 1
Content-Type: application/json

{"code":601203004,"msg":"resource not found,eval async context not found, invoke_id: 101"}

These are illustrative key fields, not captures from a deployed repaired version. Service/gateway localization can replace msg; a gateway may additionally emit raw_msg when localizing. Neither the exact message nor the presence of the invocation ID in the final text is guaranteed. HTTP 200 and tt_stable=1 do not mean business success, and ResourceNotFound does not by itself mean HTTP 404.

Compatibility and rollout checks

Successful server-side processing is unchanged, and missing-context callbacks remain rejected. This does not establish that every caller's behavior is unchanged:

  • A client that treats every nonzero business code as a failure still sees a failure. A client that retries only 601200702, or branches on message text or stability headers, may behave differently after receiving 601203004.
  • Missing context does not prove an invalid or expired invocation. AsyncExecuteTarget precedes SetEvalAsyncCtx, so an early callback can observe a transient miss. Verify its retry/compensation behavior before rollout; this boundary is not a proven production failure introduced by this PR.
  • A client that checks only HTTP status and ignores the nonzero response code can already miss business failures. This PR does not correct such client implementations.

Deployment acceptance still needed:

  • Capture the repaired endpoint's actual response, business code, and stability metadata in the target environment.
  • Verify early callbacks and the actual callers' retry behavior for the new code.
  • Verify a normal report, a valid failed-execution report, and a genuine dependency-failure control.

Validation

Tested commit: c2a73c659a800f3a811e2d24b78b9b9eac568356 (local Go 1.24.6).

  • Four related package suites passed with -race: 512 top-level tests passed, with one existing template-configuration test skipped.
  • Regression cases cover raw/wrapped/joined misses, nil context, real Redis missing-key and connection-refused behavior, expired request deadlines, stability metadata, and persistence before continuation-event publication.
  • All 3 changed executable lines are covered (100%); the added import is excluded.
  • git diff --check and golangci-lint 2.2.1 with the repository configuration for changed application code passed.
  • Repository-wide backend CI completed successfully, including build, full unit tests, lint, and coverage-upload finalization. CodeQL analysis also passed.
cd backend
go test -p=4 -gcflags="all=-N -l" -race -coverprofile=coverage.out -count=1 \
  ./modules/evaluation/application \
  ./modules/evaluation/infra/repo/experiment \
  ./modules/evaluation/infra/repo/experiment/redis/dao \
  ./infra/redis

@HearyShen
HearyShen merged commit e913af8 into main Oct 27, 2026
14 of 15 checks passed
@HearyShen
HearyShen deleted the fix/report-result-error-classification-20261023 branch October 27, 2026 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants