fix(plugin): read the workflow and activity definitions without the plugin mutex - #810
fix(plugin): read the workflow and activity definitions without the plugin mutex#810xepozz wants to merge 1 commit into
Conversation
…lugin mutex Plugin.Stop holds p.mu for the whole shutdown, and pool.Destroy inside it waits for the in-flight PHP workers to come back. An activity that calls Activity::heartbeat on its way out reaches RecordActivityHeartbeat, which takes p.mu.RLock twice to read rrActivityDef and blocks behind that writer, so the worker never returns to the pool, the drain never finishes and the lock is never released. The result is a circular wait that burns the entire endure grace period on every graceful stop that has a heartbeating activity in flight: the process is killed, the activity attempt is lost and is only recovered later by the server-side heartbeat timeout. Both definitions are written once, by initPool under Serve's lock, and are never replaced afterwards - Reset rebuilds the workers, not the definitions. A mutex was never the right tool for a write-once pointer, so store them in an atomic.Pointer, matching apiKey in the same struct, and drop the second RLock in RecordActivityHeartbeat which re-read what getActDef had already returned. The root package had no CI step, so add one - otherwise the regression test never runs.
![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: 762929b918
ℹ️ 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".
| p.temporal.rrWorkflowDef = wfDef | ||
| p.temporal.rrActivityDef = actDef | ||
| p.temporal.rrWorkflowDef.Store(wfDef) | ||
| p.temporal.rrActivityDef.Store(actDef) |
There was a problem hiding this comment.
When an early Temporal worker receives a task immediately after its Start() returns—especially while later workers in this loop are still starting—the activity can call RecordActivityHeartbeat before this store executes. Previously that RPC waited on the Serve mutex until initialization completed; now its atomic load observes nil and returns no activity definition registered, potentially failing or retrying an activity during startup. Store the definitions before enabling any task pollers, or otherwise retain startup synchronization while allowing reads during shutdown.
Useful? React with 👍 / 👎.
1 participant
Reason for This PR
[Author TODO: add issue # or explain reasoning.]Description of Changes
Plugin.Stop holds p.mu for the whole shutdown, and pool.Destroy inside it waits for the in-flight PHP workers to come back. An activity that calls Activity::heartbeat on its way out reaches RecordActivityHeartbeat, which takes p.mu.RLock twice to read rrActivityDef and blocks behind that writer, so the worker never returns to the pool, the drain never finishes and the lock is never released.
The result is a circular wait that burns the entire endure grace period on every graceful stop that has a heartbeating activity in flight: the process is killed, the activity attempt is lost and is only recovered later by the server-side heartbeat timeout.
Both definitions are written once, by initPool under Serve's lock, and are never replaced afterwards - Reset rebuilds the workers, not the definitions. A mutex was never the right tool for a write-once pointer, so store them in an atomic.Pointer, matching apiKey in the same struct, and drop the second RLock in RecordActivityHeartbeat which re-read what getActDef had already returned.
The root package had no CI step, so add one - otherwise the regression test never runs.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.