Skip to content

[SPARK-59439][CORE] Release the HistoryServerDiskManager lease reservation exactly once on a failed commit - #58741

Open
uros-b wants to merge 2 commits into
apache:masterfrom
uros-b:fix-history-disk-manager-lease-double-release
Open

[SPARK-59439][CORE] Release the HistoryServerDiskManager lease reservation exactly once on a failed commit#58741
uros-b wants to merge 2 commits into
apache:masterfrom
uros-b:fix-history-disk-manager-lease-double-release

Conversation

@uros-b

@uros-b uros-b commented Sep 11, 2026

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

HistoryServerDiskManager.Lease now releases its reserved (uncommitted) usage exactly once, whether the lease is committed, fails to commit, or is rolled back. The two updateUsage(-leased) calls in commit() and rollback() are replaced by an idempotent releaseLease() helper guarded by a released flag.

A regression test forces the rename in commit() to fail, then calls rollback() (as the caller does) and asserts the usage tracker returns to zero rather than going negative, and that a subsequent lease/commit still succeeds.

Why are the changes needed?

Lease.commit() releases the reservation with updateUsage(-leased) before renaming the temporary store into place. SPARK-58985 made that rename throw an IOException on failure, which happens after the reservation has already been released. The caller then rolls the lease back -- e.g. FsHistoryProvider.createDiskStore() calls lease.rollback() on IOException -- and rollback() releases the reservation a second time.

The reservation is added once (in lease()) but subtracted twice, so the current-usage tracker is under-counted by the leased amount and can go negative, throwing:

java.lang.IllegalStateException: Disk usage tracker went negative (now = ..., delta = ...)

This is the same crash SPARK-58985 aimed to prevent; it is reachable whenever renameTo fails (I/O error, full disk, destination parent removed out of band). In createDiskStore()'s retry loop the exception also escapes the loop, so the store is never rebuilt.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

New unit test in HistoryServerDiskManagerSuite (runs for both the LevelDB and RocksDB backends). It fails on the current code with IllegalStateException: Disk usage tracker went negative and passes with this change.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Opus 4.8

…ation exactly once on a failed commit

Lease.commit() releases the reservation before the fallible rename; when the rename fails and the caller rolls back, the reservation is released a second time, driving the usage tracker negative. Funnel both commit() and rollback() through an idempotent releaseLease() so it is returned exactly once.
@uros-b
uros-b requested a review from pan3793 September 11, 2026 13:56

@uros-b uros-b left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@pan3793 could you please review, as a followup to #58312

@dongjoon-hyun dongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for the fix. The change looks correct to me: the reservation is now returned exactly once across all failure paths in commit() (before and after releaseLease()), and it also covers FsHistoryProvider.createHybridStore(), where onSwitchToDiskStoreFail calls lease.rollback() after a failed (or even a successful) commit() on the background thread.

A minor thing in the PR description: could you record the actual model name and version in Generated-by (e.g., Generated-by: Claude Opus 4.8) instead of just Claude?

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.

2 participants