enabled token encoding by default - #66
Draft
chaptersix wants to merge 7 commits into
Draft
chaptersix wants to merge 7 commits into
chaptersix wants to merge 7 commits into
Conversation
## What changed? - Revert https://github.com/temporalio/temporal/pull/11698/changes since that had introduced a NDE change - The NDE change was that we were returning without decrementing a counter, which controls the CAN'ing rate of a version workflow. - The NDE could occur if some version workflow had already CAN'ed earlier and with this change, it would not CAN again. - Also added some more logs here so that when this does happen, we have alerts + logs for manual repair. ## Why? Provide enough context to investigate terminal task-queue deletion propagation failures without leaving the version workflow open indefinitely. ## How did you test it? - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [x] updated unit test - [ ] added new functional test(s) Focused test: env GOWORK=off go test -tags test_dep ./service/worker/workerdeployment -run TestVersionWorkflowSuite/Test_DeleteVersion_AsyncPropagationFailureCompletesWorkflow <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes terminal-delete failure handling and workflow lifetime; stale task-queue version data may persist, but metrics/logs are the recovery path and the prior keep-open behavior risked NDE/CaN issues. > > **Overview** > Reverts the behavior where a **terminal failure** during async task-queue delete propagation left the worker deployment **version workflow running** by skipping `asyncPropagationsInProgress` decrement. That path now **always decrements** the counter after logging and emitting the failure metric, so delete can finish and the workflow can exit—aligned with **`syncTaskQueuesAsync`**. > > Failure logging now includes **`taskQueues`** and **`revision`** for investigation when task queues may still hold stale version data. The unit test was updated to expect **workflow completion** (no error) instead of an indefinitely open workflow. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit b0ad220. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…11447) ## What changed? `createFirstWorkflowTask` now emits a counter (`child_execution_not_found`) and an Error log when `ScheduleWorkflowTask` returns NotFound for a child. `NamespaceNotFound` and `ErrWorkflowCompleted` are excluded. No behavior change: the error is still returned as-is and the task is still acked. ## Why? Callers arrive only after `ChildWorkflowExecutionStarted` was committed, so the parent asserts the child exists. A potential NotFound on child execution would contradict that, and `isInvalidTaskError` would drop the task, leaving the child created but never scheduled. Publish a metric and monitor it, in case if it happens. ## How did you test it? - [X] built - [X] run locally and tested manually - [ ] covered by existing tests - [X] added new unit test(s) - [X] added new functional test(s) Also simulated NotFound error thrown from the store, resulting in corruption. The error was logged. ## Potential risks There could be some cases I didn't consider. But I explicitly not changing the behavior to monitor it first.
## What changed? - Allow a deployment version at `MaxTaskQueues` to register another task queue type for an existing task queue name. - Preserve the limit error when registration would add a genuinely new task queue family. ## Why? `TaskQueueFamilies` is keyed by task queue name and stores Workflow, Activity, and Nexus types inside each family. The validator previously rejected every new type once the family map reached the limit, even though adding a type to an existing name does not increase the number of families. ## How did you test it? - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [x] added new unit test(s) - [ ] added new functional test(s) Commands: - `go test -tags test_dep ./service/worker/workerdeployment -run '^TestValidateRegisterWorker_AllowsNewTaskQueueTypeAtFamilyLimit$' -count=1` - `go test -tags test_dep ./service/worker/workerdeployment -count=1` The focused test was run before the production change and failed with `errMaxTaskQueuesInVersion`; it passes with this fix. ## Potential risks Low. Duplicate registration of the same task queue type is still rejected, and a new task queue name is still rejected when the family limit has been reached. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Narrow validation change in worker registration; duplicate-type and new-family limits are unchanged, covered by new integration test. > > **Overview** > **Fixes deployment-version registration** so `MaxTaskQueues` counts **task queue families** (by name), not every workflow/activity/nexus type on that name. > > `validateRegisterWorker` in `version_workflow.go` now enforces the cap only when `!familyExists`—registering another type on an existing task queue name is allowed even when the family map is already at the limit. Duplicate type registration and adding a **new** name at the limit still fail as before. > > Adds **`TestDeploymentVersionTaskQueueFamilyLimitAllowsNewType`**: with `MatchingMaxTaskQueuesInDeploymentVersion` set to 1, workflow then activity polls on the same queue succeed (two types, one family); a second queue name is rejected. **`pollActivityFromDeployment`** is exported as a package helper for reuse in tests. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 5cac0cc. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
## What changed? Remove commit and author from notification. Wrap stack trace in code block. ## Why? latest commit and author for the CI run can be irrelevant for data race issues since they are sporadic. ## How did you test it? - [X] built - [X] run locally and tested manually - [X] covered by existing tests - [X] added new unit test(s) - [ ] added new functional test(s)
## What changed? Return no error if child in stale branch ## Why? Return no error if child in stale branch so the standby task verification will mark the task as complete and no retry. ## How did you test it? - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [ ] added new unit test(s) - [ ] added new functional test(s)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed?
Describe what has changed in this PR.
Why?
Tell your future self why have you made these changes.
How did you test it?
Potential risks
Any change is risky. Identify all risks you are aware of. If none, remove this section.