Skip to content

gang: track incomplete_tasks on-demand via bind/unbind hooks#507

Closed
k82cn wants to merge 1 commit into
xflops:mainfrom
k82cn:flm_498_2
Closed

gang: track incomplete_tasks on-demand via bind/unbind hooks#507
k82cn wants to merge 1 commit into
xflops:mainfrom
k82cn:flm_498_2

Conversation

@k82cn

@k82cn k82cn commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Update GangPlugin to track incomplete_tasks dynamically via bind/unbind hooks instead of relying solely on the snapshot value from setup().

Changes

  • Decrement incomplete_tasks in on_session_bind() when a task is bound to an executor
  • Increment incomplete_tasks in on_session_unbind() when an executor is unbound

Motivation

The incomplete_tasks value was previously calculated once in setup() and remained static throughout the scheduling cycle. This could become stale as tasks are bound/unbound during the cycle. By updating it on-demand via hooks, the gang scheduling decisions (is_ready/is_fulfilled) now reflect the current state.

Testing

All 21 gang plugin tests pass.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request attempts to update the number of incomplete tasks when sessions are bound or unbound in the gang scheduler plugin. However, the feedback highlights a critical logic bug where modifying incomplete_tasks during binding and unbinding leads to double-counting and premature fulfillment of gang scheduling requirements, as a bound task is still incomplete. It is recommended to revert these changes.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines 156 to +157
state.bound += 1;
state.incomplete_tasks = state.incomplete_tasks.saturating_sub(1);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

Double-Counting & Premature Fulfillment Bug

Decrementing incomplete_tasks here introduces a critical logic bug:

  1. Conceptual Issue: A task bound to an executor is still incomplete (it is in Running or Binding state, not Succeed/Failed/Cancelled). Therefore, binding a task does not reduce the number of incomplete tasks.
  2. Double-Counting: The gang plugin evaluates fulfillment using total >= needed, where total = allocated + bound and needed = div_ceil(incomplete_tasks, batch_size) * batch_size. If you decrement incomplete_tasks (reducing needed) and increment bound (increasing total) simultaneously, you double-count the progress.

For example, with batch_size = 2 and 4 pending tasks:

  • Initially: needed = 4, total = 0.
  • Bind 1 task: needed = 4, total = 1.
  • Bind 2 tasks: needed = 2, total = 2 -> is_fulfilled becomes true prematurely, leaving the remaining 2 tasks without executors.

Revert this change to keep incomplete_tasks static during the scheduling cycle.

Suggested change
state.bound += 1;
state.incomplete_tasks = state.incomplete_tasks.saturating_sub(1);
state.bound += 1;

Comment on lines 169 to +170
state.bound = state.bound.saturating_sub(1);
state.incomplete_tasks = state.incomplete_tasks.saturating_add(1);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

Double-Counting & Premature Fulfillment Bug

Incrementing incomplete_tasks here is the inverse of the double-counting bug introduced in on_session_bind. Since unbinding a task does not change its completion status (it remains incomplete), incomplete_tasks should not be modified here. Revert this change to keep incomplete_tasks static during the scheduling cycle.

Suggested change
state.bound = state.bound.saturating_sub(1);
state.incomplete_tasks = state.incomplete_tasks.saturating_add(1);
state.bound = state.bound.saturating_sub(1);

@k82cn k82cn closed this Jun 24, 2026
@k82cn
k82cn deleted the flm_498_2 branch June 24, 2026 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant