[YUNIKORN-3509] Fix placeholder accounting on allocation rollback

[rjgoyln]

Summary

When the shim fails to bind a placeholder pod, it sends a SCHEDULING_FAILED_ON_RM release. The core rollback incorrectly treats the placeholder as a normal allocation, subtracting it from allocatedResource instead of allocatedPlaceholder and leaving the partition placeholder count unchanged. Reallocating it then double-counts the placeholder.

For a gang with two allocated 1-vcore placeholders, rolling one back gives:

Variable	Before rollback	Current (incorrect)	Expected after rollback
allocatedResource	0	-1	0
allocatedPlaceholder	2	2	1
Partition placeholderAllocations	2	2	1

Queue and node usage remain correct, but the application may report incorrect resource usage, enter Running before all placeholders are allocated, or get stuck in Completing.

Change

• Subtract rolled-back placeholders from allocatedPlaceholder.
• Decrement the partition placeholder count.
• Release only placeholders on placeholder timeout once the application has started.

The placeholder timer keeps running after a rollback: the gang deadline still counts from the first placeholder allocation and covers placeholders allocated again. A started application can then time out with no placeholders allocated, which must not release its real allocations.

Tests

• TestRollbackAllocationPlaceholder: verifies application accounting, user usage, and timer state.
• TestRemoveAllocationSchedulingFailedOnRMPlaceholder: verifies partition counts and that the application enters Running only after both placeholders are reallocated.
• TestTimeoutPlaceholderAfterRollback: verifies that the timeout still resumes an Accepted gang and keeps a Running application's real allocations.

All three tests fail without the fix; race and deadlock tests pass.

Type of change

• Bug Fix

Jira issue

Jira ID : https://issues.apache.org/jira/browse/YUNIKORN-3509

• I have created a Jira issue for this pull request.
• The Jira ID is part of the title of this pull request.

AI Tooling

Generated by Claude Code (Opus 5.5)

• The PR includes the phrase "Generated by <tool>", where <tool> is the name of the AI tool used.
• My use of AI contributions follows the ASF legal policy.

How has this been tested?

• New unit tests were added to cover new or changed code paths.
• make test_all was run, and no failures reported.

[codecov]

Codecov Report

❌ Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.39%. Comparing base (0ef9fd7) to head (79fb1ed).

Files with missing lines	Patch %	Lines
pkg/scheduler/partition.go	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1190      +/-   ##
==========================================
+ Coverage   83.31%   83.39%   +0.07%     
==========================================
  Files         104      104              
  Lines       14652    14660       +8     
==========================================
+ Hits        12208    12225      +17     
+ Misses       2146     2139       -7     
+ Partials      298      296       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[HuangTing-Yao]

Thanks for the fix. The direction looks right, and I can reproduce the symptoms from the JIRA on the base commit. Three things I'd like to confirm:

1. No timer for a placeholder allocated again while Running
   The JIRA calls this out as a known cost, but is it acceptable? On a long-running app the leftover placeholders hold resources until the app ends. Could the timer be restarted when the placeholder is allocated again? Also worth adding this trade-off to the code comment, which currently only explains why the timer is cleared.

2. Timeout is reset while Accepted
   Once a rollback brings allocatedPlaceholder to zero the timer is cleared, and the next allocation starts a new one with the full timeout. If binds keep failing and it drops to zero each time, the gang timeout may never fire and the app stays in Accepted. A timeout in Accepted already takes the incomplete-gang path, so could we leave the timer alone in that state?

3. Rollback of a placeholder with a replacement in flight
   If the placeholder was already picked by tryPlaceholderAllocate, the rollback does not undo its released flag or the link to the real ask. The real ask stays allocated without being an allocation of the app, and the placeholder cannot be replaced or timed out after it is allocated again. removeNodeAllocations reverses the same case and could be used as a reference. This is an existing gap: handle it here or in a follow-up JIRA?

[HuangTing-Yao]

Hi @wilfred-s , one design question.

YUNIKORN-3316 excludes placeholders from the rollback in the shim: on an AssumePod failure a placeholder is failed, not rolled back.
YUNIKORN-2884 reuses that rollback for bind failures without the placeholder check, which is what triggers this issue.

Is rescheduling a placeholder after a bind failure intended, or should it be failed like on the AssumePod path? If it should be failed, a guard in the shim would be the simpler fix.

[rjgoyln]

Thanks for the fix. The direction looks right, and I can reproduce the symptoms from the JIRA on the base commit. Three things I'd like to confirm:

Thanks for the review! I’ll address the first two points by updating the timeout logic directly, rather than clearing the timer during rollback. This should preserve the original timeout while preventing real allocations from being released when no placeholders remain.

For the third point, I agree it’s an existing gap. I’ll track it in a follow-up JIRA to keep this PR focused.