Conversation
58542ea to
49a89cd
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
02c9bb0 to
df40d1c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df40d1c491
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
df40d1c to
5e7a14c
Compare
5e7a14c to
1a6bd84
Compare
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
e173af5 to
b49dc54
Compare
b49dc54 to
1557bdd
Compare
DABH
left a comment
There was a problem hiding this comment.
Ultrareviewed the exact stacked delta against #3049. I found two independently reproduced replay-compatibility blockers inline. Both generate nondeterministic commands when histories created by the base SDK are replayed at this head, so green same-version tests do not cover them.
The three earlier Codex comments were valid and are correctly addressed: unset propagators are normalized, the workflow test now uses SDKTestWorkflowRule, and standalone activity clients receive plugin propagators. I found no further issues in the activity/Nexus/schedule hooks, option copying, header mutability, plugin extraction, or propagator deduplication. Current PR checks are green.
| handlers.add(() -> proc.apply(resultPromise)); | ||
| // Handlers run in a callback thread created by the runner, which inherits its propagated | ||
| // contexts from the workflow header rather than from the thread registering the handler. | ||
| Map<String, Object> contexts = ContextThreadLocal.getCurrentContextForPropagation(); |
There was a problem hiding this comment.
[P1] Version this workflow-visible continuation change
Capturing the registration context changes what an incomplete-promise continuation observes from the completing thread's context to the registering thread's context. That value is workflow-observable, so existing histories can take a different command branch after an SDK upgrade.
I reproduced this on the exact base/head pair: the base ran an async child under context child, registered a continuation under registration, and completed without a timer; replaying that completed history here made the continuation observe registration, emit START_TIMER, and fail with TMPRL1100 because history contained WORKFLOW_EXECUTION_COMPLETED.
Please gate the new semantics behind an SdkFlag so histories without the flag retain the old completion-thread behavior, and add the generated base history as replay coverage. The current new-execution test cannot detect this compatibility break.
| for (ContextPropagator propagator : workflowClient.getOptions().getContextPropagators()) { | ||
| contextPropagatorsByName.putIfAbsent(propagator.getName(), propagator); | ||
| } | ||
| for (ContextPropagator propagator : factoryOptions.getContextPropagators()) { |
There was a problem hiding this comment.
[P1] Preserve old behavior for existing factory-only histories
SimplePlugin.addContextPropagators(...) and factory-only plugin installation both existed before this PR, but those propagators previously did not reach workflow child threads. Activating them here silently changes workflow-visible behavior during an SDK upgrade.
I reproduced a second cross-version failure: a base workflow with an existing factory-only plugin created an Async.function child, which saw no parent context and completed without a timer. Replaying that history at this head made the child inherit the context and take a timer branch, again failing with WORKFLOW_EXECUTION_COMPLETED versus START_TIMER.
This path also needs SDK-version gating (or another migration that preserves legacy workflow execution) plus a legacy-history replay test. The merge order and name deduplication themselves look correct.
DABH
left a comment
There was a problem hiding this comment.
Ultrareviewed the exact stacked delta against #3049 a second time with an independent trace. Both P1 replay-compatibility findings stand: an incomplete-promise continuation now observes the registering thread's context instead of the completing thread's, and factory-only plugin propagators now reach workflow child threads that previously inherited nothing. Both need SdkFlag gating plus a replay test from a base-generated history; SKIP_YIELD_ON_VERSION and DETERMINISTIC_CANCELLATION_SCOPE_ORDER show the pattern.
Two new items are inline: the data converter customizer is not applied to the activity, Nexus, or schedule clients (P2), and the CompletablePromiseImpl comment misdescribes the threading model while the PR description claims no breaking changes (P2). Smaller items, not blocking: stubs-level ActivityClientPlugins now also wrap the SDK-internal ActivityClients built in TemporalNexusClientImpl, so the comment at line 1118 saying standalone interceptors are unavailable in the Nexus handler lifecycle is no longer accurate for stubs-installed plugins; WorkerFactoryOptions.setContextPropagators is not @Experimental although the sibling setPlugins is, and its Javadoc says "appended" while same-name duplicates are dropped with client precedence; and there is no replay coverage for either behavior change. The three Codex threads remain correctly addressed. Current PR checks are green.
| } | ||
|
|
||
| @Override | ||
| public void configureActivityClient(@Nonnull ActivityClientOptions.Builder builder) { |
There was a problem hiding this comment.
[P2] Apply the data converter customizer to the activity, Nexus, and schedule clients
configureWorkflowClient applies dataConverterCustomizer, but configureScheduleClient, configureActivityClient, and configureNexusClient do not, although all three option builders expose setDataConverter and the schedule client uses its converter for memos and action payloads. Now that SimplePlugin is an ActivityClientPlugin and a NexusClientPlugin, a codec plugin installed on the service stubs reaches ActivityClient.newInstance(stubs) and NexusClient.newInstance(stubs), but those clients keep GlobalDataConverter while the workers decode with the customized converter, so standalone activity or Nexus inputs are encoded differently from what the worker expects. The schedule half is pre-existing, since that hook used to be a no-op, but has the same shape. Please apply the customizer in all three methods and cover it with tests. This does not block the OpenTelemetry stack, which uses no customizer.
| unregisterWithRunner(); | ||
| } else { | ||
| handlers.add(() -> proc.apply(resultPromise)); | ||
| // Handlers run in a callback thread created by the runner, which inherits its propagated |
There was a problem hiding this comment.
[P2] Correct the continuation threading comment and the compatibility claim
This comment says handlers run in a callback thread created by the runner that inherits its contexts from the workflow header. That is only true for SDK-driven completions such as activities, timers, child workflows, and signals, which arrive through executeInWorkflowThread on a new callback thread. invokeHandlers() below runs the handlers inline on whatever thread calls complete(), so a promise completed by an Async.function child runs the continuation on that child thread with its creation-time context. Before this change a continuation therefore observed the completing thread's context; after it, the registering thread's. That is the workflow-visible difference behind the P1 above, and it also makes the PR description's "Breaking changes? None" inaccurate. Please rewrite the comment to describe the inline invocation and update the description once the SdkFlag gating is in place.
What changed?
ActivityClientPluginandNexusClientPlugin, propagated from the service stubs toActivityClientandNexusClientthe same wayScheduleClientPluginreaches the schedule client, withsetPlugins/getPluginsonActivityClientOptionsandNexusClientOptions.SimplePluginimplements both, and its builder gainedaddScheduleClientInterceptors,addActivityClientInterceptors, andaddNexusClientInterceptors.configureScheduleClientnow applies its interceptors instead of being a no-op, so plugins no longer needconfigure*overrides to install interceptors on any client.PluginUtils.extractPlugins, which the two new sites also use.StartNexusOperationExecutionInputnow copies headers into a mutable, case-insensitive map, matchingExecuteNexusOperationInput, so header-adding interceptors can populate it in place as its javadoc describes. Previously it kept the caller-providedCollections.emptyMap().WorkerFactoryOptionssupport for context propagators, so plugins installed only on worker factory options can propagate workflow context into workflow child threads.WorkerFactorynow combines workflow-client and factory propagators, deduplicating by propagator name with client configuration taking precedence.Why?
Standalone activities and client-started Nexus operations had no plugin hook to install an interceptor through, and the schedule hook had no builder helper. This is the second prerequisite slice, stacked on #3049.
The worker-factory additions also make the documented factory-only plugin installation path functional: context propagators configured by a plugin on
WorkerFactoryOptionsare available to workers and workflow child threads, while avoiding duplicate propagators when the same name is configured on both the workflow client and the factory.Breaking changes?
None. The public APIs are additive and experimental.
Server PR
None.
Test plan
./gradlew :temporal-sdk:test --tests 'io.temporal.common.*' --tests 'io.temporal.client.*'./gradlew :temporal-sdk:test -PtestServer=dev-server --tests 'io.temporal.client.nexus.*' --tests 'io.temporal.client.functional.StandaloneActivityTest' --tests 'io.temporal.client.schedules.*' --tests 'io.temporal.common.PluginPropagationTest'./gradlew :temporal-sdk:spotlessCheck./gradlew :temporal-sdk:javadoc