Siyao Meng created HDDS-17404:
---------------------------------
Summary: LeaseManager monitor can expire a lease that was
re-acquired or renewed after it found the old deadline passed
Key: HDDS-17404
URL: https://issues.apache.org/jira/browse/HDDS-17404
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments:
MC-1-expire-only-the-timed-out-lease-atomically-with-renew.patch,
TestBugMC1ExpiryRemovesReacquiredLease.java, TestBugMC2RenewLostToExpiry.java
h3. Mechanism
{{LeaseManager.LeaseMonitor.run}} decides expiry without the manager lock: it
calls {{get(resource)}}, {{Lease.getRemainingTime}} and {{Lease.getCallback}},
and only then takes the lock in {{release(resource)}}. {{release}} removes
whatever lease is stored under that resource at that moment and invalidates it.
It never checks that this is the lease the monitor judged, or that its deadline
is still in the past.
{{Lease.renew}} takes no lock at all. It checks the non volatile {{expired}}
flag and adds to {{leaseTimeout}}. The monitor does not read the timeout again
before {{release}}.
So two things can change between the monitor's decision and its commit:
* the lease is released and the same resource is acquired again, so {{release}}
removes the new lease;
* the lease is renewed, so {{release}} removes a lease whose new deadline is in
the future.
h3. Trigger
Any caller of {{LeaseManager}} that either releases and re-acquires the same
resource, or renews a lease, close to the moment the monitor finds the old
deadline passed. The window runs from the monitor's unlocked read until it gets
the manager lock, so other callers doing {{acquire}} or {{release}} on the same
manager make it wider.
In tree, no production code does either at this commit.
{{CloseContainerEventHandler}} only acquires; its leases are released only by
the monitor and by {{shutdown}}. {{EventWatcher}} releases and re-acquires by
event ID, but its only main subclass, {{RetriableDatanodeEventWatcher}}, is not
instantiated in production code at this commit. Nothing outside tests calls
{{Lease.renew}}. The defect is in the public API of {{hdds-server-framework}}
and would affect any future caller of this kind.
h3. Impact
Reproduced on unmodified source through the public API:
* *Re-acquired lease removed early.* A lease that is released and acquired
again while the monitor is expiring the old one is removed and invalidated a
few milliseconds after it was acquired, instead of at its 1000 ms deadline.
{{get}} throws {{LeaseNotFoundException}}, {{renew}} throws
{{LeaseExpiredException}}, the callback of the lease that was explicitly
released runs once, and the new lease's own callback never runs.
[^TestBugMC1ExpiryRemovesReacquiredLease.java]
* *Successful renew dropped.* {{renew(60000)}} returns normally, the lifetime
reads 60100 ms, and the monitor then invalidates the lease and runs its expiry
callback; a second {{acquire}} of the same resource succeeds.
[^TestBugMC2RenewLostToExpiry.java]
A lost renew always lands after the monitor found the lease timed out. It
returns normally, although the lease is then expired anyway.
h3. Reproduction
The two attached classes are self contained JUnit tests for
{{hadoop-hdds/framework}} (target path and Maven command are in each class
comment). They pass on unmodified source when the bug is present. Each holds
the manager's own monitor, the lock that {{acquire}} and {{release}} already
take, while the monitor thread is blocked on it to release the timed out lease,
and does the release and re-acquire, or the renew, meanwhile. This only fixes
an order that can also happen without help, so the tests show that the window
exists, not how often it is hit. No state is injected and no code under test is
changed.
h3. Patch
[^MC-1-expire-only-the-timed-out-lease-atomically-with-renew.patch], against
f804bf26867246067017fdd9e5319c2cbdbd5db8. It also applies to master at
a7da4f70195.
The monitor now commits through a new private {{LeaseManager.expire(resource,
lease)}}, which holds the manager lock and the lease's lock. It releases the
lease only if it is still the active lease for the resource, compared by
identity since {{Lease.equals}} compares only the resource, and its remaining
time is still not positive. Otherwise the monitor runs no callback and scans
again at once, so a renewed lease still expires at its new deadline.
{{Lease.renew}} is {{synchronized}}, so it either completes before the
monitor's check or fails after the invalidation. A renew that runs after the
deadline but before the monitor removes the lease still extends it, as it
already does today when it runs before the monitor's check; making it throw
would break renews that work today. {{Lease.expired}} is {{volatile}}, so
{{hasExpired}} and {{renew}} see an invalidation done by another thread.
{{acquire}}, {{release}} and {{shutdown}} are unchanged, and callbacks still
run on the executor, outside any lock.
Covered by two new cases in {{TestLeaseManager}}:
{{testExpiryKeepsLeaseReacquiredMeanwhile}} and
{{testRenewDuringExpiryKeepsLease}}. The first also checks that the released
lease's callback has not run once a later lease on the same resource has
expired; the second checks that the renewed lease still expires at its new
deadline with exactly one callback. Without the change both fail ("lease
acquired after the release" and "lease renewed before it was expired": expected
false but was true). With it {{TestLeaseManager}}, {{TestEventWatcher}},
{{TestCloseContainerEventHandler}} and {{TestBlockManager}} pass (32 tests),
checkstyle is clean, and both attached reproductions no longer show the bug.
Found by TLA+ model checking and code review of the HDDS LeaseManager expiry
and shutdown paths under HDDS-15926, on commit
f804bf26867246067017fdd9e5319c2cbdbd5db8. Checked against HDDS issues and
apache/ozone pull requests for duplicates before filing. Related but not
duplicates: HDDS-8418 (removed an acquire variant that attached the callback
after acquire, a different race) and HDDS-10899 (callback refactoring). The
attached patch is a proposal for review. Generated with Specula (Claude Opus
5.5).
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
