feat(task-manager): Make TaskManager conesnsus behavior consistent #25274
Conversation
github-actions
bot
added
area: dds
There was a problem hiding this comment.
Pull Request Overview
This PR modifies TaskManager to use consensus-based behavior consistently across all operations, removing optimistic behavior that was previously implemented. The main goal is to make TaskManager behavior consistent for implementing rollback functionality by ensuring all operations wait for acknowledgment before reflecting state changes.
Key changes include:
- Removing optimistic behavior from
assigned(),queued(), and"completed"event functions - Implementing consensus-based state checking that only reflects acknowledged operations
- Adding a new
queuedOptimistically()method for internal optimistic checks where needed
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| taskManager.ts | Core implementation changes removing optimistic behavior and adding consensus-based state management |
| taskManager.spec.ts | Updated test assertions to reflect new consensus-based behavior expectations |
| taskManager.fuzz.spec.ts | Re-enabled previously skipped fuzz tests that were failing due to eventual consistency issues |
| interfaces.ts | Updated documentation to clarify consensus-based behavior in API comments |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
| if (!this.isAttached()) { | ||
| this.taskQueues.delete(taskId); | ||
| this.completedWatcher.emit("completed", taskId); | ||
| this.emit("completed", taskId); |
There was a problem hiding this comment.
Similar here re: "completed" handlers not having evidence of the completion in the pending ops when they run
There was a problem hiding this comment.
I think the particular block you highlighted is a bit different since this is the detached scenario and we won't actually be sending an op at all (we return one line later).
Co-authored-by: Matt Rakow <[email protected]>
scottn12
changed the title
Description
This PR makes TaskManager more consistently behave as a consensus based DDS. This will be useful when implementing rollback as it will make the behavior consistent across all op types.
This PR changes the following functions/events that previously behaved at least partially optimistically to now behave on a consensus basis:
assigned()- This function had previosly used pending local ops to determine if the local client would soon become unassigned (local abandon/complete ops)."completed"event - This event would previously fire immediately locally aftercomplete()was called locally (without waiting for ack).queued()- This function uses local ops to determine if we will optimistaclly be tryingn to become assigned for a task after all local ops are processed.Misc
AB#44704
Fixes AB#7310
Fixes AB#5185