Repository navigation
Add getRandomStream and isSubjectToReplay #3049
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| package io.temporal.internal.statemachines; | ||
|
|
||
| import com.google.common.hash.HashCode; | ||
| import com.google.common.hash.Hashing; | ||
| import java.nio.ByteBuffer; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
| import java.util.Random; | ||
| import javax.annotation.Nonnull; | ||
|
|
||
| final class WorkflowRandomStreams { | ||
| private static final String SEED_VERSION = "temporal.sdk.random.v1"; | ||
|
|
||
| private final Map<String, Random> streams = new HashMap<>(); | ||
|
|
||
| long deriveSeed(@Nonnull String runId, @Nonnull String name) { | ||
| // The separators keep ("ab", "c") from colliding with ("a", "bc") | ||
| String seed = String.join("\0", SEED_VERSION, runId, name); | ||
| HashCode hash = Hashing.sha256().hashString(seed, StandardCharsets.UTF_8); | ||
| return ByteBuffer.wrap(hash.asBytes()).getLong(); | ||
| } | ||
|
|
||
| Random get(@Nonnull String runId, @Nonnull String name) { | ||
| return streams.computeIfAbsent(name, key -> new Random(deriveSeed(runId, key))); | ||
| } | ||
|
|
||
| void reseed(@Nonnull String runId) { | ||
| streams.forEach((name, stream) -> stream.setSeed(deriveSeed(runId, name))); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -735,6 +735,17 @@ public static <T> T readOnly(Functions.Func<T> func) { | |
| } | ||
| } | ||
|
|
||
| public static <T> T notSubjectToReplay(Functions.Func<T> func) { | ||
| SyncWorkflowContext workflowContext = getRootWorkflowContext(); | ||
| boolean previousSubjectToReplay = workflowContext.isSubjectToReplay(); | ||
| workflowContext.setSubjectToReplay(false); | ||
| try { | ||
| return func.apply(); | ||
| } finally { | ||
| workflowContext.setSubjectToReplay(previousSubjectToReplay); | ||
| } | ||
| } | ||
|
|
||
| public static WorkflowInfo getWorkflowInfo() { | ||
| return new WorkflowInfoImpl(getRootWorkflowContext().getReplayContext()); | ||
| } | ||
|
|
@@ -761,6 +772,10 @@ public static Random newRandom() { | |
| return getRootWorkflowContext().newRandom(); | ||
| } | ||
|
|
||
| public static Random getRandomStream(String name) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P1] Guard getRandomStream against read-only contexts Unlike Acquisition from a query handler fails the workflow task. 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 Please add
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding a guard to
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Read-only is the wrong boundary. |
||
| return getRootWorkflowContext().getReplayContext().getRandomStream(name); | ||
| } | ||
|
|
||
| public static Logger getLogger(Class<?> clazz) { | ||
| Logger logger = LoggerFactory.getLogger(clazz); | ||
| return new ReplayAwareLogger( | ||
|
|
@@ -946,6 +961,11 @@ static void assertNotReadOnly(String action) { | |
| } | ||
| } | ||
|
|
||
| public static boolean isSubjectToReplay() { | ||
|
patbeqo marked this conversation as resolved.
|
||
| Optional<WorkflowThread> thread = DeterministicRunnerImpl.currentThreadInternalIfPresent(); | ||
| return thread.isPresent() && getRootWorkflowContext().isSubjectToReplay(); | ||
| } | ||
|
|
||
| static void assertNotInUpdateHandler(String message) { | ||
| if (getCurrentUpdateInfo().isPresent()) { | ||
| throw new UnsupportedContinueAsNewRequest(message); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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 thatRandomis 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
Randomsubclass if the return type must remainRandom) and pin that sequence in replay tests.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed a collision is a real risk. It repeats the whole sequence, which matters for trace IDs.
I tried a wider generator (
L64X128MixRandomviaRandomGenerator), butjava.util.random.RandomGeneratoris 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.