Skip to content

[Do not Submit] [Test PR] Fix testbench and disable flaky tests#13371

Open
Dhriti07 wants to merge 12 commits into
mainfrom
fix-testbench-and-disable-flaky-tests
Open

[Do not Submit] [Test PR] Fix testbench and disable flaky tests#13371
Dhriti07 wants to merge 12 commits into
mainfrom
fix-testbench-and-disable-flaky-tests

Conversation

@Dhriti07
Copy link
Copy Markdown
Contributor

@Dhriti07 Dhriti07 commented Jun 5, 2026

No description provided.

@Dhriti07 Dhriti07 requested review from a team as code owners June 5, 2026 06:47
@Dhriti07 Dhriti07 changed the title Fix testbench and disable flaky tests [Do not Submit] [Test PR] Fix testbench and disable flaky tests Jun 5, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a CumulativeHasher to accumulate and validate checksums for full object reads, updates exception constructor visibilities, disables several hanging integration tests, and improves docker container management in TestBench.java. Feedback highlights two key issues: first, CumulativeHasher may throw false-positive mismatch exceptions if some reads bypass validation or return null hashes, which can be resolved with a tracking flag; second, the docker cleanup process in TestBench.java should forcibly destroy timed-out processes and avoid swallowing InterruptedException to prevent resource leaks.

Comment on lines +104 to +110
boolean qualifiesForVerification(Object metadata) {
return startOffset == 0
&& metadata != null
&& metadata.hasChecksums()
&& metadata.getChecksums().hasCrc32C()
&& (!limit.isPresent() || limit.getAsLong() >= metadata.getSize());
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

If any chunk read bypasses the validate methods (by calling hash directly) or if delegate.hash returns null (e.g., if hashing is disabled or skipped for a chunk), the cumulative hash will be incomplete or remain at zero(). If qualifiesForVerification still returns true, validateCumulativeChecksum will compare this incomplete hash with the expected full-object checksum and throw a false-positive UncheckedCumulativeChecksumMismatchException (which is a DataLossException).

To prevent this, we should introduce a private boolean canVerify = true; flag:

  • Set canVerify = false if hash(ByteBuffer) or hash(ByteString) is called directly.
  • Set canVerify = false if delegate.hash returns null in any of the validate or validateUnchecked methods.
  • Check canVerify in qualifiesForVerification.
Suggested change
boolean qualifiesForVerification(Object metadata) {
return startOffset == 0
&& metadata != null
&& metadata.hasChecksums()
&& metadata.getChecksums().hasCrc32C()
&& (!limit.isPresent() || limit.getAsLong() >= metadata.getSize());
}
boolean qualifiesForVerification(Object metadata) {
return canVerify
&& startOffset == 0
&& metadata != null
&& metadata.hasChecksums()
&& metadata.getChecksums().hasCrc32C()
&& (!limit.isPresent() || limit.getAsLong() >= metadata.getSize());
}

Comment on lines +222 to +224
try {
new ProcessBuilder("docker", "rm", "-f", containerName).start().waitFor(5, TimeUnit.SECONDS);
} catch (Exception ignore) {}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

When starting a process with ProcessBuilder and waiting for it with a timeout, if the timeout expires, the process is not automatically terminated. This can lead to leaked system resources or orphaned processes. It is recommended to check the return value of waitFor and forcibly destroy the process if it times out. Also, ensure that InterruptedException is not swallowed; restore the thread's interrupted status by calling Thread.currentThread().interrupt().

try {
  Process process = new ProcessBuilder("docker", "rm", "-f", containerName).start();
  if (!process.waitFor(5, TimeUnit.SECONDS)) {
    process.destroyForcibly();
  }
} catch (InterruptedException e) {
  Thread.currentThread().interrupt();
} catch (Exception ignore) {}
References
  1. In Java, do not swallow InterruptedException. When catching it, restore the thread's interrupted status by calling Thread.currentThread().interrupt() and handle the interruption appropriately.

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.

1 participant