Siyao Meng created HDDS-17667:
---------------------------------
Summary: Hardening: AbstractCommitWatcher keeps the cached future
of a failed watch for commit uncompleted, so a repeated watch of that index
would hang
Key: HDDS-17667
URL: https://issues.apache.org/jira/browse/HDDS-17667
Project: Apache Ozone
Issue Type: Bug
Reporter: Siyao Meng
Attachments: CR-2-complete-cached-watch-future-on-failure.patch,
TestBugCR2FailedWatchStaleFuture.java
h3. Mechanism
{{AbstractCommitWatcher.watchForCommitAsync}} caches one future per log index
in {{replies}} so that concurrent watches of the same index share one request.
Only the success path completes the cached future and removes it. When the
client's watch fails, the future returned to the first caller fails, but the
cached future stays in {{replies}} and is never completed. A concurrent caller
that joined it, or any later watch of the same index on the same watcher, gets
that future and waits on it (untimed). The later watch never reaches the
datanode client. {{cleanup()}} clears the map but does not complete the future.
The gap exists since the cache was added in HDDS-2146 (synchronous then, with
{{join()}}), and HDDS-11174 kept it.
h3. Trigger
None through a supported write path today; the cache is never hit there.
* Ratis keys: {{BlockOutputStream.watchForCommit}} is chained once to each
PutBlock, and each PutBlock has its own Raft log index. An hsync or flush
without new data reuses the last flush future and does not call the watcher.
After a failed watch the block stream keeps the error, so later hsync, write
and close calls fail in {{checkOpen()}} before any watch.
* Streaming keys: {{watchOnFirstIndex}} and {{watchOnLastIndex}} run on the
writer thread. Two callers could only join one pending watch if two threads
used one {{KeyDataStreamOutput}} at once, which is not a supported usage (it
has no synchronization). The only index that repeats is 0, used for buffers of
a PutBlock sent without Raft, and {{XceiverClientRatis.watchForCommit(0)}}
completes at once without failing.
h3. Impact
No user visible effect today. This is a latent defect: if a future caller
watched an index twice on one watcher (for example two threads on one streaming
key), it would hang forever after a failed watch instead of seeing the error.
h3. Reproduction
PASS on unmodified source, deterministic, no cluster.
[^TestBugCR2FailedWatchStaleFuture.java] ({{hadoop-hdds/client}}) uses a stub
datanode client whose watches fail.
* {{failedWatchLeavesStaleFutureForSameIndex}} calls {{CommitWatcher}}
directly: after the watch for index 7 fails, the joined future and a later
watch of index 7 (the same object) are not completed, and the client received
one watch request.
* {{failedStreamDoesNotWatchSameIndexAgain}} goes through the public
{{RatisBlockOutputStream}} API and shows why it is not reached: after the
failed hsync, a second hsync, a write and {{close()}} throw the saved error and
send no watch.
A passing test means the defect is present.
h3. Patch
[^CR-2-complete-cached-watch-future-on-failure.patch], against
ec5ed5f7ea0e4b406aee53aae4906616819cba2b. It also applies to master at
b683fd3f294.
When the client's watch future fails, {{watchForCommitAsync}} removes the
cached future with a conditional {{remove(commitIndex, f)}} and completes it
exceptionally with the watch error, so joined callers fail with that error and
a later watch sends a new request. The success path and the future returned to
the first caller are unchanged. Covered by
{{testFailedWatchForCommitCompletesCachedFuture}} in the existing
{{TestBlockOutputStreamCorrectness}}, which checks that the joined future fails
with the watch error and that a later watch is sent again. It fails without the
change because the joined future stays incomplete. With it, the hdds-client
storage and ozone-client unit tests and the mini cluster suites
{{TestCommitWatcher}} and {{TestBlockOutputStreamWithFailures}} (22 tests)
pass, and checkstyle is clean. With the patch the first reproduction test fails
(all three futures complete exceptionally and the client receives two
requests). The second still passes.
Since the cache is never hit on a supported path, removing it would be the
alternative. This patch keeps the existing design and only makes its failure
path correct.
Related: HDDS-17648 (a watch future that never completes when the Ratis client
is closed), still under review.
Found by code review of the Ozone client Ratis block write and failure handling
paths, as part of the TLA+ verification effort under HDDS-15926, on commit
ec5ed5f7ea0e4b406aee53aae4906616819cba2b. Checked against HDDS issues and
apache/ozone pull requests for duplicates before filing. 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]
