Skip to content

[multistage] Fix flaky RAW_MESSAGES assertion in GrpcSenderBackpressureTest#19020

Merged
Jackie-Jiang merged 1 commit into
apache:masterfrom
gortiz:fix/grpc-backpressure-test-counting-race
Jul 20, 2026
Merged

[multistage] Fix flaky RAW_MESSAGES assertion in GrpcSenderBackpressureTest#19020
Jackie-Jiang merged 1 commit into
apache:masterfrom
gortiz:fix/grpc-backpressure-test-counting-race

Conversation

@gortiz

@gortiz gortiz commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Description

GrpcSenderBackpressureTest.senderObservesBackpressureFromSlowReceiver is flaky. It asserted an exact relationship between the send loop's sendCount and the RAW_MESSAGES stat:

assertTrue(rawMessages == sendCount || rawMessages == sendCount + 1, ...);

That equality does not hold in general. sendCount counts every send() call the loop makes, but RAW_MESSAGES is only incremented once a block passes the terminated / early-terminated guard inside GrpcSendingMailbox.sendInternal — and SendingMailbox.send(MseBlock.Data) returns void, so the loop cannot tell an aborted attempt from a real send. When the tail of the run interleaves with the watchdog's cancel(), one or more attempts bump sendCount without incrementing RAW_MESSAGES, so RAW_MESSAGES legitimately trails sendCount. CI hit rawMessages == sendCount - 1, which the assertion rejected.

Fix

Replace the exact check with two bounds that hold regardless of how the tail interleaves with the cancel:

  • RAW_MESSAGES <= sendCount + 1 — each send() pushes at most one block through processAndSend (one RAW_MESSAGES increment), and the cancel adds at most one error EOS.
  • RAW_MESSAGES >= polledCount — the receiver cannot poll a block the sender never pushed. polledCount is measured independently on the receiver side, so this still guards against RAW_MESSAGES not being tracked at all.

Test-only change; no production code is touched.

Testing

mvn -pl pinot-query-runtime -Dtest=GrpcSenderBackpressureTest test passes; spotless / checkstyle / license checks pass on the module.

…reTest

The `senderObservesBackpressureFromSlowReceiver` test asserted an exact
relationship between the loop's `sendCount` and the `RAW_MESSAGES` stat
(`rawMessages == sendCount || sendCount + 1`). That equality does not hold in
general: `sendCount` counts every `send()` call, but `RAW_MESSAGES` is only
incremented once a block passes the terminated / early-terminated guard inside
`GrpcSendingMailbox.sendInternal` (`send()` returns void, so the loop cannot
tell an aborted attempt from a real send). When the tail of the run
interleaves with the watchdog's cancel, one or more attempts bump `sendCount`
without incrementing `RAW_MESSAGES`, so `RAW_MESSAGES` can legitimately trail
`sendCount`, which tripped the assertion (observed `rawMessages == sendCount - 1`).

Replace the exact check with two bounds that hold regardless of interleaving:
`RAW_MESSAGES <= sendCount + 1` (each send pushes at most one block, cancel adds
at most one error EOS) and `RAW_MESSAGES >= polledCount` (the receiver cannot
poll a block the sender never pushed). The lower bound uses the receiver-side
poll count as an independent witness, so it still guards against the stat not
being tracked at all.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@gortiz
gortiz requested review from xiangfu0 and yashmayya July 20, 2026 16:33
@codecov-commenter

codecov-commenter commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.37%. Comparing base (2892236) to head (cee3be6).

Additional details and impacted files
@@             Coverage Diff              @@
##             master   #19020      +/-   ##
============================================
- Coverage     65.39%   65.37%   -0.02%     
  Complexity     1405     1405              
============================================
  Files          3423     3423              
  Lines        215968   215968              
  Branches      34186    34186              
============================================
- Hits         141228   141185      -43     
- Misses        63380    63418      +38     
- Partials      11360    11365       +5     
Flag Coverage Δ
custom-integration1 100.00% <ø> (ø)
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-25 65.37% <ø> (-0.02%) ⬇️
temurin 65.37% <ø> (-0.02%) ⬇️
unittests 65.37% <ø> (-0.02%) ⬇️
unittests1 57.09% <ø> (-0.03%) ⬇️
unittests2 37.56% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Jackie-Jiang Jackie-Jiang added the flaky-test Tracks a test that intermittently fails label Jul 20, 2026
@Jackie-Jiang
Jackie-Jiang merged commit b9e538a into apache:master Jul 20, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flaky-test Tracks a test that intermittently fails

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants