Revert "scheduler: fix executor allocation for non-gang policies and gang batch semantics (#500)"

[k82cn]

Reverts #500 (merge commit f89eeca).

This restores the scheduler code to its state immediately prior to #500. The resulting tree is byte-identical to the pre-#500 commit (2a372d1).

Reverted changes

• common/src/ctx.rs
• executor_manager/src/shims/grpc_shim.rs, executor_manager/src/shims/mod.rs
• flmadm/src/managers/config.rs
• session_manager/src/scheduler/actions/allocate.rs, dispatch.rs
• session_manager/src/scheduler/ctx.rs, mod.rs
• session_manager/src/scheduler/plugins/gang.rs, mod.rs

Notes

Reverting because #500's scheduler changes need re-evaluation. Re-apply once the underlying issues (#498) are addressed with a validated fix.

[gemini-code-assist]

Code Review

This pull request reverts several previous scheduling changes, simplifying the gang plugin state and modifying how executor readiness and fulfillment are evaluated. However, the feedback highlights several critical regressions introduced by these changes. Specifically, defaulting to true when evaluating readiness and fulfillment for non-gang configurations completely breaks scheduling and dispatching by causing the scheduler to skip sessions. Additionally, reverting the gang plugin's logic to simple modulo arithmetic reintroduces duplicate executor allocation and binding bugs. Finally, replacing the shared process-wide lock with local locks in the shim tests introduces race conditions since these tests concurrently mutate global process state.

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.

[gemini-code-assist]

Reverting the changes to is_ready reintroduces a critical bug where the scheduler completely fails to allocate executors if the gang plugin is disabled (e.g., when only priority or drf is used).

Detailed Analysis:

1. If the gang plugin is not loaded, no active plugin overrides is_ready.
2. Therefore, plugin.is_ready(ssn) returns None for all loaded plugins.
3. Due to .unwrap_or(true), is_ready evaluates to true.
4. In AllocateAction::execute, the pre-check checks:

   let ready = ctx.is_ready(&ssn)?;
   if fulfilled || ready {
       continue;
   }

5. Since ready is true, AllocateAction immediately skips the session and continues to the next one, without ever allocating any executors.

This completely breaks scheduling for any non-gang policy configurations. To fix this, is_ready and is_fulfilled should return Option<bool> (or handle None as "no opinion" rather than defaulting to true when no opinionated plugin is loaded).

[gemini-code-assist]

Reverting the changes to is_fulfilled reintroduces a critical bug where the scheduler completely fails to dispatch bindings if the gang plugin is disabled.

Detailed Analysis:

1. If the gang plugin is not loaded, no active plugin overrides is_fulfilled.
2. Therefore, plugin.is_fulfilled(ssn) returns None for all loaded plugins.
3. Due to .unwrap_or(true), is_fulfilled evaluates to true.
4. In DispatchAction::execute, the check:

   if ctx.is_fulfilled(&ssn)? {
       // ...
   }

   will evaluate to true immediately, skipping or misbehaving during dispatch because it assumes the session is already fully satisfied before any bindings are actually committed.

To fix this, is_fulfilled should return Option<bool> (or handle None as "no opinion" rather than defaulting to true when no opinionated plugin is loaded).

[gemini-code-assist]

Reverting the shared SHIMS_TEST_LOCK and replacing it with a local TEST_LOCK introduces a race condition between tests in grpc_shim.rs and shims/mod.rs.

Both test modules modify the same global process-wide state:

1. They set the environment variable FLAME_SOCKET_DIR via std::env::set_var.
2. They change the current working directory via std::env::set_current_dir.

Since Cargo runs tests in parallel across multiple threads by default, these tests will run concurrently and interfere with each other's environment and working directory, leading to flaky test failures. A single process-wide lock should be shared across all shim tests to prevent parallel execution of tests that mutate global state.

[gemini-code-assist]

Reverting the shared SHIMS_TEST_LOCK and replacing it with a local TEST_LOCK introduces a race condition between tests in shims/mod.rs and grpc_shim.rs.

Both test modules modify the same global process-wide state:

1. They set the environment variable FLAME_SOCKET_DIR via std::env::set_var.
2. They change the current working directory via std::env::set_current_dir.

Since Cargo runs tests in parallel across multiple threads by default, these tests will run concurrently and interfere with each other's environment and working directory, leading to flaky test failures. A single process-wide lock should be shared across all shim tests to prevent parallel execution of tests that mutate global state.

[gemini-code-assist]

Reverting the gang plugin's is_ready logic to Some(state.pipelined > 0 && total % state.batch_size == 0) reintroduces a duplicate executor allocation bug.

Detailed Analysis:

1. If a session already has enough executors allocated in previous cycles (e.g., state.allocated is already a multiple of batch_size and satisfies the pending tasks), but state.pipelined is 0, is_ready returns Some(false).
2. In AllocateAction::execute, the pre-check if fulfilled || ready { continue; } will evaluate to false because ready is Some(false).
3. The scheduler does not skip the session and enters the allocation loop:

   while ctx.is_allocatable(node, &ssn)? && !ctx.is_ready(&ssn)? {
       stmt.allocate(node, &ssn)?;
   }

4. The loop will allocate an entire additional batch of executors (until total % batch_size == 0 is satisfied again), leading to duplicate/over-allocation of executors and resource waste.

The gang plugin should track the actual demand (e.g., based on incomplete tasks) rather than relying solely on modulo arithmetic of the current cycle's pipelined count.

[gemini-code-assist]

Reverting the gang plugin's is_fulfilled logic to Some(state.bound > 0 && total % state.batch_size == 0) reintroduces a duplicate binding/allocation bug.

Detailed Analysis:

1. If a session already has enough executors bound in previous cycles (e.g., state.allocated is already a multiple of batch_size and satisfies the pending tasks), but state.bound is 0, is_fulfilled returns Some(false).
2. In AllocateAction::execute, the pre-check if fulfilled || ready { continue; } will evaluate to false because fulfilled is Some(false).
3. This prevents the scheduler from skipping the session, leading to redundant allocations or bindings being triggered.

The gang plugin should track the actual demand (e.g., based on incomplete tasks) rather than relying solely on modulo arithmetic of the current cycle's bound count.