Repository navigation
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // per-destination rate limiting, concurrency control and circuit breaking for outgoing HTTP calls. The system endpoint | ||
| // is served by an in-process RPC to the history service, so it has no use for any of that and returns an empty | ||
| // destination, which places the task on the transfer queue instead. | ||
| func taskDestination(endpointName string) string { |
There was a problem hiding this comment.
[med] Behavior: DestinationDownError is returned as a task failure on the transfer queue, even when the attempt was saved successfully
The handlers still wrap retryable call errors unconditionally (operation_tasks.go:208, cancellation_tasks.go:176):
if callErr != nil && isDestinationDown(callErr) {
saveErr = queueserrors.NewDestinationDownError(callErr.Error(), saveErr)
}On the outbound queue, CircuitBreakerExecutable.Execute (service/history/queues/executable.go:1041) unwraps this error, so if saveErr == nil the task completes successfully. The transfer queue has no such wrapper. A system endpoint call that fails with a retryable error then returns a non-nil *DestinationDownError wrapping nil, even though saveInvocationResult already recorded the failed attempt and scheduled the backoff. This affects any retryable HandlerError from the system handler and the raw context.DeadlineExceeded (isDestinationDown returns true for both).
executableImpl.HandleErr treats that as an unexpected error. It increments TaskFailures and unexpectedErrorAttempts, logs Fail to process task (and Critical error processing task after a few attempts), and reschedules the task. The next attempt is discarded by Validate because the attempt number changed. So every retryable system endpoint failure produces a spurious task failure metric, a warning log and an extra execution. This is worse than the PR description says ("treated as a regular retryable error"): the work actually succeeded.
Suggestion: only wrap when the task is on the outbound queue, e.g. if callErr != nil && taskDestination(args.endpointName) != "" && isDestinationDown(callErr). Please also add a unit test for the system endpoint retryable-failure path that asserts Execute returns nil.
There was a problem hiding this comment.
[med] Behavior: system endpoint calls now get about 3s instead of 10s, and nexusoperation.request.timeout is silently capped
On the outbound queue, executeChasmSideEffectTask wraps the task in outboundTaskTimeout (10s), which matches the RequestTimeout default (10s). The transfer queue wraps it in taskTimeout (service/history/transfer_queue_task_executor_base.go:36, 3s). That 3s budget covers loading mutable state, loadStartArgs, the call itself and the final UpdateComponent. callCtx := context.WithTimeout(ctx, callTimeout) can't extend the parent deadline, so:
- Any
nexusoperation.request.timeoutabove ~3s no longer applies to system endpoint calls. Nothing logs or documents this, and the HSM path still gets 10s. - If the system handler (an in-process history RPC, which can still do persistence writes or hop to another shard) runs out the parent deadline,
saveInvocationResult/saveCancellationResultrun on the same expiredctxand fail. The attempt is never recorded and no backoff is scheduled. The queue retries the task on its own policy, and it counts towardunexpectedErrorAttempts/ DLQ. On the outbound queue the call's 10s budget equalled the task budget, so the same race existed in principle, but with 3s it is much easier to hit under load.
Options: run the CHASM side-effect task in the transfer executor with a longer timeout (or one derived from the task), or give the save step its own context that is not limited by the call's deadline. Either way, please mention the effective timeout change in the PR description.
…or off the outbound queue Drop the nexusoperation.request.timeout config: the task executor deadline already bounds each request. Check MinRequestTimeout against the remaining operation timeout (terminal) and the time left on the task deadline (retryable). Only wrap errors in DestinationDownError for tasks on the outbound queue, since the transfer queue doesn't unwrap it.
The request is bound by RequestTimeout, the remaining operation timeout, and the task deadline. RequestTimeout is the knob for leaving time to save the result within the task deadline.
This branch has not been deployed
1 participant
Supersedes #12615.
Before: Every CHASM Nexus operation invocation and cancelation task was given the endpoint name as its destination, so all of them were routed to the outbound queue and grouped by destination. That includes tasks for the system endpoint, which only makes a fast in-process call to the history service and gets nothing from per-destination grouping.
After: CHASM Nexus tasks for the system endpoint have an empty destination, so they are scheduled on the transfer queue. Tasks for external endpoints are unchanged and stay on the outbound queue.
What changed?
taskDestinationhelper inchasm/lib/nexusoperationthat returns an empty string for the system endpoint and the endpoint name otherwise. The invocation and cancelation task transitions (scheduled and rescheduled) use it forTaskAttributes.Destination.nexusoperation.request.timeout, the remaining operation timeout, and the task executor's context deadline. The task deadline also covers loading the component and saving the result: 10s on the outbound queue, 3s on the transfer queue. So system endpoint requests get less than 3s whateverRequestTimeoutis set to.RequestTimeoutis looked up by the operation's endpoint name, because system endpoint tasks have no destination. Its description now notes that it must be set below the task deadline to leave time to save the result. The 10s default equals the outbound task deadline.MinRequestTimeoutis now checked against the actual time available:RequestTimeout) is below it, the request is skipped and the attempt is recorded as a retryable request timeout.DestinationDownErroris only returned for tasks on the outbound queue. That's the only queue whose executor unwraps it. On the transfer queue it would turn an attempt that was already saved into a task failure.Why?
CHASM already categorizes an immediate task with no destination as a transfer task, and the transfer queue executes CHASM side-effect tasks generically, so an empty destination is all that is needed. The transfer queue is a better fit for fast internal RPCs than the outbound queue.
HSM is intentionally not changed. HSM has no transfer task type (
generateSubStateMachineTaskrejects immediate tasks without a destination), and adding one requires a new persisted task type that older history hosts can't deserialize during a rolling upgrade. External endpoints stay on the outbound queue in both HSM and CHASM to keep per-destination concurrency limits, rate limiting and circuit breaking.How did you test it?
built
run locally and tested manually
covered by existing tests
added new unit test(s)
added new functional test(s)
go test -tags test_dep ./chasm/lib/nexusoperation/..., including new cases:Executereturnnil.MinRequestTimeoutskips the request and records a retryable request timeout.RequestTimeoutand at the task deadline (separate cases).TestNexusWorkflowTestSuiteHSM,TestNexusWorkflowTestSuiteCHASM(incl. the system endpoint tests on both code paths) andTestNexusStandaloneTestSuitepass.make lint-code-fastis clean.Potential risks