Skip to content

Fix scheduler readiness semantics#509

Closed
k82cn wants to merge 4 commits into
xflops:mainfrom
k82cn:codex/fix-scheduler-readiness
Closed

Fix scheduler readiness semantics#509
k82cn wants to merge 4 commits into
xflops:mainfrom
k82cn:codex/fix-scheduler-readiness

Conversation

@k82cn

@k82cn k82cn commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

  • run each scheduling cycle in Allocate -> Dispatch -> Shuffle order
  • keep Context::is_ready and Context::is_fulfilled boolean while allowing individual plugins to abstain with None
  • let PluginManager ignore abstentions, combine concrete opinions, and return true when every plugin abstains
  • make allocation account for incomplete-task demand and reusable Void/Idle executors, while dispatch checks bound/in-flight capacity
  • rename the readiness owner to BatchPlugin, load it implicitly like Shim, and omit it from configurable policy lists
  • preserve the existing scheduler API/state shape and add regressions for the Multiple executors when only one task in the session #498 Binding stall, Priority/DRF policy stacks, batch sizing, max instances, and Idle reuse
  • restore the default-policy and executor-manager test-lock changes that were reverted by Revert "scheduler: fix executor allocation for non-gang policies and gang batch semantics (#500)" #508; the removed gang policy name is intentionally rejected

Root cause

The reverted #500 implementation mixed allocation and dispatch readiness semantics and treated an all-abstaining plugin set as an optional scheduler result. That could leave a session stuck when its executor was already Binding. The revised design keeps optionality inside plugins only: allocation asks whether executor supply is fulfilled, dispatch asks whether ready/bound capacity exists, and Context always receives a boolean decision. If every plugin abstains, the action is considered satisfied. Batch is always loaded so valid Priority/DRF stacks have a concrete readiness owner without listing it in configuration.

Validation

  • cargo fmt --check
  • cargo test -p flame-session-manager -- --test-threads=1 (304 passed)
  • cargo test -p common ctx:: (33 passed)
  • cargo test -p flmadm (39 passed)
  • cargo test -p flame-executor-manager shims:: -- --test-threads=4 (14 passed)
  • git diff --check

Addresses #498 and replaces the scheduler behavior reverted by #508.

@k82cn
k82cn marked this pull request as ready for review July 8, 2026 02:46

@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 implements the redesigned scheduler readiness and fulfillment semantics (RFE498), switching the default policy stack to priority + gang and reordering the cycle actions to Allocate → Dispatch → Shuffle. It replaces the modulo-based gang checks with absolute target calculations and defers idle executor accounting in DRF and Priority policies until dispatch binding. The review feedback suggests optimizing the plugin conjunction loops in PluginManager by short-circuiting on false opinions, and refactoring GangPlugin to extract the duplicated incomplete_tasks calculation into a helper function.

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 +347 to +352
let mut result = None;
for (_, plugin) in plugins.iter() {
if let Some(ready) = plugin.is_ready(ssn) {
result = Some(result.unwrap_or(true) && ready);
}
}

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.

medium

We can optimize the conjunction loop by short-circuiting and returning early if any plugin returns Some(false). Since the final result is an AND of all opinions, a single false opinion guarantees the final result will be false.

        let mut result = None;
        for (_, plugin) in plugins.iter() {
            if let Some(ready) = plugin.is_ready(ssn) {
                if !ready {
                    return Ok(false);
                }
                result = Some(true);
            }
        }

Comment on lines +364 to +369
let mut result = None;
for (_, plugin) in plugins.iter() {
if let Some(fulfilled) = plugin.is_fulfilled(ssn) {
result = Some(result.unwrap_or(true) && fulfilled);
}
}

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.

medium

Similarly to is_ready, we can optimize is_fulfilled by short-circuiting and returning early if any plugin returns Some(false).

        let mut result = None;
        for (_, plugin) in plugins.iter() {
            if let Some(fulfilled) = plugin.is_fulfilled(ssn) {
                if !fulfilled {
                    return Ok(false);
                }
                result = Some(true);
            }
        }

Comment on lines +82 to +91
let incomplete_tasks = [TaskState::Pending, TaskState::Running]
.iter()
.map(|task_state| {
ssn.tasks_status
.get(task_state)
.copied()
.unwrap_or(0)
.max(0) as u64
})
.sum::<u64>();

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.

medium

The logic to calculate incomplete_tasks is duplicated between is_ready and is_fulfilled. We should extract this into a private helper function to improve maintainability and readability.

Here is an example of the helper function you can define in gang.rs:

fn get_incomplete_tasks(ssn: &SessionInfoPtr) -> u64 {
    [TaskState::Pending, TaskState::Running]
        .iter()
        .map(|task_state| {
            ssn.tasks_status
                .get(task_state)
                .copied()
                .unwrap_or(0)
                .max(0) as u64
        })
        .sum::<u64>()
}
        let incomplete_tasks = get_incomplete_tasks(ssn);

Comment on lines +105 to +114
let incomplete_tasks = [TaskState::Pending, TaskState::Running]
.iter()
.map(|task_state| {
ssn.tasks_status
.get(task_state)
.copied()
.unwrap_or(0)
.max(0) as u64
})
.sum::<u64>();

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.

medium

The logic to calculate incomplete_tasks is duplicated between is_ready and is_fulfilled. We should extract this into a private helper function to improve maintainability and readability.

        let incomplete_tasks = get_incomplete_tasks(ssn);

@k82cn k82cn closed this Jul 8, 2026
@k82cn
k82cn deleted the codex/fix-scheduler-readiness branch July 8, 2026 04:51
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