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".
| public void nextBytes(byte[] bytes) { | ||
| Objects.requireNonNull(bytes, "bytes"); |
There was a problem hiding this comment.
When workflow code retains a stream and later calls nextBytes or nextLong from a query, update validator, or another read-only callback, this implementation advances the stream without any read-only check; the only guard is when Workflow.getRandomStream initially acquires it. Such a query therefore mutates workflow state, so subsequent workflow draws depend on whether and how often the query ran and can produce nondeterministic results during replay. Enforce the read-only restriction on every draw, not only during stream lookup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The caller should check if they are in a read only context before drawing.
There was a problem hiding this comment.
Reopening this with reproductions on the exact head d5bb22c, because the current code has no guard at all and the reply relies on a check that callers cannot make. A held stream drawn inside a query, a fresh getRandomStream(...) inside an update validator, and a draw inside Workflow.sideEffect each produced live sequences that differ from a cache-miss replay of the same run (for example [e0, e2] live against [e0, e1] on replay) with no exception, whereas Workflow.newRandom() throws ReadOnlyException in the validator and side-effect cases. There is no public read-only predicate, since WorkflowInternal.isReadOnly() is package-private, and isSubjectToReplay() is an opt-in and different predicate. Acquisition from a query is worse still: it fails the workflow task with a raw Error, detailed in the new [P1] comment on WorkflowInternal.getRandomStream. A guarded Random subclass fixes both the held-stream and the acquisition cases.
bfaf813 to
cd21e5b
Compare
|
To use Codex here, create a Codex account and connect to github. |
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. What shall we delve into next? 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". |
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.
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.
| return getRootWorkflowContext().newRandom(); | ||
| } | ||
|
|
||
| public static Random getRandomStream(String name) { |
There was a problem hiding this comment.
[P1] Guard getRandomStream against read-only contexts
Unlike randomUUID() and newRandom() directly above, this method has no assertNotReadOnly, and both resulting failure modes are worse than a ReadOnlyException.
Acquisition from a query handler fails the workflow task. WorkflowStateMachines.getRandomStream calls checkEventLoopExecuting(), which calls WorkflowThread.await(...) and therefore currentThreadInternal(); on the query thread that throws a raw java.lang.Error("Called from non workflow or workflow callback thread"). ReplayWorkflowRunTaskHandler.executeQueries catches only Exception, so the Error escapes. Reproduced on this head with a query handler that calls Workflow.getRandomStream("x"): history recorded WORKFLOW_TASK_FAILED_CAUSE_WORKFLOW_WORKER_UNHANDLED_FAILURE, the task retried, and the client query failed. On the real-server direct-query path the same Error fails the query and resets the sticky cache.
Draws in read-only or non-replayed contexts silently desynchronize the stream. A held stream drawn inside a query, a fresh stream drawn inside an @UpdateValidatorMethod, and a draw inside Workflow.sideEffect each produced live sequences that differ from what a cache-miss replay reconstructs (for example live [e0, e2] against replay [e0, e1]), with no exception, while newRandom() throws ReadOnlyException in the validator and side-effect cases. Callers also have no public read-only predicate to check: WorkflowInternal.isReadOnly() is package-private, and isSubjectToReplay() is a different, opt-in predicate.
Please add assertNotReadOnly("random stream") here and return a Random subclass whose next(int) asserts the same, so held streams are guarded as well. That subclass is also the natural home for the wider generator state requested in the other thread. A test that pins query, validator, and side-effect behavior would keep this from regressing.
| } | ||
| } | ||
|
|
||
| public static boolean isSubjectToReplay() { |
There was a problem hiding this comment.
[P2] Match isReplaying() off workflow threads
isReplaying() uses currentThreadInternalIfPresent() and documents that it returns false outside workflow code, but isSubjectToReplay() goes through getRootWorkflowContext(), so WorkflowUnsafe.isSubjectToReplay() called from an activity, Nexus, or arbitrary thread throws a raw java.lang.Error("Called from non workflow or workflow callback thread"). The WorkflowUnsafe Javadoc does say "Must be called from Workflow code", but the sibling predicate this one is documented against behaves differently, and the downstream ReplaySafeIdGenerator in #3089 only avoids the Error by checking isWorkflowThread() first. Please either return false through currentThreadInternalIfPresent() like isReplaying() or throw a descriptive exception, and document the off-thread result.
2 participants
What changed?
Randomstreams throughWorkflow.getRandomStream(String).WorkflowUnsafe.isSubjectToReplay().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