Document scheduler migration handoff counterexamples - #46
Conversation
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. |
![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: 89e4fa767c
ℹ️ 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".
| t.Logf("destination workflow/request=%s/%s; source workflow/request=%s/%s", start.WorkflowId, start.RequestId, | ||
| sourceRequests[0].Request.WorkflowId, sourceRequests[0].Request.RequestId) |
There was a problem hiding this comment.
If a fix preserves the same workflow/request identity on both sides of an ambiguous handoff, History can deduplicate the two start attempts even while the destination still retains its buffered copy. This test only logs those identities and then fails whenever that copy exists, so it would remain red after such a valid exactly-once fix and falsely report duplicate execution; assert that the identities differ or model StartWorkflow deduplication before requiring the destination buffer to be empty.
Useful? React with 👍 / 👎.
| require.True(t, workflow.IsContinueAsNewError(env.GetWorkflowError()), | ||
| "the source must remain active when the destination ID belongs to an unrelated schedule") |
There was a problem hiding this comment.
When AlreadyExists is changed to reject a foreign destination, merely observing a continue-as-new error does not prove the acknowledged manual action remains actionable: an implementation that returns the collision error but accidentally drops BufferedStarts would satisfy this assertion. Count the source StartWorkflow call or decode the continue-as-new input and verify the buffered action is preserved, otherwise this regression test can pass while the data-loss invariant it documents is still violated.
Useful? React with 👍 / 👎.
| The deterministic counterexamples fail as evidence of current behavior: | ||
|
|
||
| ```sh | ||
| TEMPORAL_RUN_MIGRATION_COUNTEREXAMPLES=1 go test -tags test_dep ./chasm/lib/scheduler/migration ./chasm/lib/scheduler ./service/worker/scheduler -run 'TestMigrationCounterexample' -count=1 |
There was a problem hiding this comment.
The documented counterexample command neither includes ./service/frontend nor matches TestMigrationSentinelEvidence, so readers following the canonical reproduction steps never exercise the completed/terminated sentinel defect described above. Add the frontend package and extend the -run expression to include the sentinel test.
Useful? React with 👍 / 👎.
| require.Equal(t, pending.RequestId, imported.InvokerState.BufferedStarts[0].RequestId) | ||
| require.Equal(t, pending.WorkflowId, imported.InvokerState.BufferedStarts[0].WorkflowId) | ||
| for _, b := range imported.Backfillers { |
There was a problem hiding this comment.
If LegacyToCreateFromMigrationStateRequest drops the ongoing backfill entirely, this final range is empty and the test still passes because only the two invoker starts are length-checked. Require exactly one imported backfiller before comparing its watermark so this rollback-progress control detects loss of the backfill it is intended to preserve.
Useful? React with 👍 / 👎.
1 participant
Problem
Scheduler migration currently has deterministic ownership and operation-accounting failures at
the workflow/CHASM handoff. This evidence-only layer adds ordinary passing controls plus opt-in
counterexamples; it changes no production behavior.
Violated invariants
Minimal reproductions
Set
TEMPORAL_RUN_MIGRATION_COUNTEREXAMPLES=1to expose fixed, seed-free sequences for a triggerdelivered after snapshot, a committed create whose response is lost, foreign destination
collisions in both directions, and completed/terminated sentinels. Native controls cover
pre-snapshot triggers, failure before commit, retry boundaries, and running sentinels.
The canonical report and test sources are commit-pinned:
Before/after
This layer intentionally preserves the pre-fix failures behind an environment variable. Safe
rollback fixes land at the top of this stack. The forward response-loss and signal defects are
documented as blocked on the History ingress-fence protocol rather than receiving an unsafe
scheduler-only patch.
Validation
The first command passes. The second deterministically fails on this branch by design.
Stack
chaptersix/temporal:main