Repository navigation
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33197ee281
ℹ️ 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".
bfaf813 to
cd21e5b
Compare
This comment has been minimized.
This comment has been minimized.
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. |
This comment has been minimized.
This comment has been minimized.
5193120 to
256b63f
Compare
patbeqo
changed the title
DABH
left a comment
DABH
left a comment
There was a problem hiding this comment.
Ultrareviewed the exact head and the full stacked use in the OpenTelemetry v2 PRs. I found one scale/correctness issue inline.
I also revisited the resolved read-only-draw thread. The failure mode is real, but the final public Javadoc explicitly makes WorkflowUnsafe.isSubjectToReplay() gating a caller precondition, and the downstream ID generator observes it, so I am not treating a query that violates that contract as a separate blocker. A guarded API would still be safer.
Focused random-stream tests and the replay-divergence diagnostic passed locally; the worktree is clean. Current PR checks are green.
| } | ||
|
|
||
| Random get(@Nonnull String runId, @Nonnull String name) { | ||
| return streams.computeIfAbsent(name, key -> new Random(deriveSeed(runId, key))); |
There was a problem hiding this comment.
[P2] Preserve more than 48 bits of generator state
Although the seed is hashed with SHA-256 and reduced to a long, new Random(seed) retains only 48 bits. At roughly 2^24 workflow-run/name streams the birthday probability of at least one duplicate state becomes material, and a collision produces the same entire sequence—not merely one repeated value. The stated consumer is OpenTelemetry IDs, so this can merge otherwise unrelated traces in a large installation; documenting that Random is not cryptographically secure does not address global ID uniqueness.
The earlier implementation on this branch used a full-seed SHA-counter stream. Please retain substantially wider deterministic state (for example through a Random subclass if the return type must remain Random) and pin that sequence in replay tests.
There was a problem hiding this comment.
Agreed a collision is a real risk. It repeats the whole sequence, which matters for trace IDs.
I tried a wider generator (L64X128MixRandom via RandomGenerator), but java.util.random.RandomGenerator is Java 17+ and temporal-sdk still compiles for Java 8. Shipping it would need a multi-release split, so Java 8 workers couldn't use the feature. In mixed-JDK fleets, a run that drew on 17+ would get stuck when it replays on 8.
For now, the 48-bit limit is documented on the ID generator in #3089. I'm open to doing the multi-release version, but I wanted to flag those tradeoffs first.
DABH
left a comment
DABH
left a comment
There was a problem hiding this comment.
Ultrareviewed the exact head d5bb22c a second time with an independent trace and scratch reproductions. Two new findings are inline: unguarded getRandomStream fails the workflow task when called from a query handler and silently desynchronizes streams drawn in read-only or non-replayed contexts (P1), and isSubjectToReplay() throws a raw Error off workflow threads where isReplaying() returns false (P2). I have reopened the earlier Codex read-only thread with the reproductions, since its resolution did not change the code. The 48-bit generator state finding stands, and a Random subclass can host both the wider state and the read-only guard.
Seed derivation, memoization, the reset reseed ordering, continue-as-new and child isolation, and the isSubjectToReplay context flags all trace correctly. Smaller items, not blocking: WorkflowRandomStreams.deriveSeed accepts a null name and hashes the literal "null" instead of failing fast; the isSubjectToReplay Javadoc omits the preferred-version provider, and the getRandomStream Javadoc suggests gating draws on isSubjectToReplay() even though Workflow.await conditions are read-only yet subject to replay; no test pins behavior inside queries, validators, or side effects, there is no WorkflowReplayer history test, and the reset tests only run against an external service; the comments at WorkflowRandomStreams.java:18 and WorkflowRandomStreamTest.java:107-108 do not end with a period. Current PR checks are green.
8d9c915 to
033e6e1
Compare
033e6e1 to
0a8e194
Compare
6b6ebf0 to
0539be3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0539be31ed
ℹ️ 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".
| try { | ||
| readOnly = true; | ||
| subjectToReplay = false; |
There was a problem hiding this comment.
When a custom DataConverter calls WorkflowUnsafe.isSubjectToReplay() while decoding the previously stored mutable-side-effect value, it incorrectly sees true because storedBinary.map(...) runs before this assignment. That decoding occurs only while the mutable-side-effect function is evaluated live; replay instead deserializes the recorded result later. Consequently, converter logic guarded by this API—such as drawing from a named random stream—can execute only on the live path and shift subsequent deterministic results. Set the flag to false before decoding the stored value and restore it around the entire live-only callback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2aca398. The stored value is now decoded inside the live-only window, so a converter sees isSubjectToReplay() as false (and read-only as true) during that decode. MutableSideEffectDecodeSubjectToReplayTest pins this with a recording converter.
There was a problem hiding this comment.
There's a related concern: live execution for a mutable side effect may not run a payload converter, but replay will run it to deserialize the recorded result, thus resulting in differential sampling of the random stream between live execution and replay.
There was a problem hiding this comment.
Nice catch. Sampling the random stream changes Workflow state, and isReadOnly already exists to guard against that, so I extended isReadOnly instead of adding a new flag. It now also returns true when a recorded mutable side effect result is deserialized. That code only runs on replay, because live execution returns the in-memory result early. With this change, guarded code runs the same number of times live as on replay.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
27e595e to
b6b0e65
Compare
patbeqo
changed the title
19e4d2e to
a5c353b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5c353b459
ℹ️ 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".
patbeqo
changed the title
Make code that does not run the same way live and on replay read-only: await conditions including interceptor wrappers, context propagation on Update threads, the final mutableSideEffect decode, and thread unwinding in close. A thread being destroyed still stops with DestroyWorkflowThreadError instead of a catchable ReadOnlyException.
… errors Make await conditions read-only before they reach the interceptors too, so an interceptor that evaluates the condition itself is covered, and restore the previous read-only value when the condition returns. Run context propagation inside the thread's cleanup scope, so a propagator that throws ends the thread right away instead of leaving the runner waiting for deadlock detection.
a5c353b to
a9656fb
Compare
What changed?
Randomstreams throughWorkflow.getRandomStream(String).WorkflowUnsafe.isReadOnly().Why?
OpenTelemetry and other integrations need replay-stable entropy without perturbing a Workflow's application random sequence. This is the prerequisite slice for #3046.
Breaking changes?
None. The public APIs are additive and experimental.
Server PR
None.
Test plan
mise exec -- ./gradlew :temporal-sdk:test --offlinemise exec -- ./gradlew test --offlinemise exec -- ./gradlew spotlessCheckmise exec -- ./gradlew :temporal-sdk:javadoc --offline