You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "yashmayya (via GitHub)" <gi...@apache.org> on 2026/04/06 22:28:53 UTC
[PR] Fix multi-controller race conditions in ResponseStoreCleaner cursor cleanup [pinot]
yashmayya opened a new pull request, #18113:
URL: https://github.com/apache/pinot/pull/18113
## Summary
- **Leader coordination**: Only the lead controller runs `ResponseStoreCleaner` by gating `processTables()` on `isLeaderForTable(TASK_NAME)`, preventing all controllers from racing to delete the same expired responses on each broker.
- **Graceful concurrent deletion on broker**: `AbstractResponseStore.deleteResponse()` now catches exceptions from `readResponse()` when files vanish between the `exists()` check and the read (TOCTOU race). `FsResponseStore.deleteResponseImpl()` catches exceptions from `pinotFS.delete()` and treats already-gone directories as success instead of throwing.
- **No batch abort on individual failures**: `ResponseStoreCleaner.deleteExpiredResponses()` logs individual DELETE failures as warnings instead of throwing a `RuntimeException`, so one failed DELETE no longer aborts the entire broker's cleanup batch.
## Root cause
When multiple controllers run the `ResponseStoreCleaner` concurrently (all controllers run it because `processTables()` ignores the table leadership list), they race to delete the same expired cursor responses on each broker. The broker's `deleteResponse()` has a TOCTOU race between `exists()` → `readResponse()` → `deleteResponseImpl()` — when one controller deletes a cursor's files, the others hit `FileNotFoundException` / `IOException`, and the broker returns HTTP 500 instead of 404. The controller's `deleteExpiredResponses()` then throws on any single 500, aborting the remaining successful deletes' logging for that broker.
## Test plan
- [x] Existing `ResponseStoreCleanerTest` tests pass (including `testPartialBrokerFailureDoesNotBlockOthers` and `testCleanupTreats404AsSuccess`)
- [ ] Verify in a multi-controller environment that only the lead controller runs the cleaner
- [ ] Verify that concurrent DELETE requests to the broker no longer cause HTTP 500s
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix multi-controller race conditions in ResponseStoreCleaner cursor cleanup [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18113:
URL: https://github.com/apache/pinot/pull/18113#issuecomment-4195524151
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18113?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `7 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.77%. Comparing base ([`736cbf7`](https://app.codecov.io/gh/apache/pinot/commit/736cbf77fa3ae9ac95129917d5208c8589c8a0bf?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`3e90ba3`](https://app.codecov.io/gh/apache/pinot/commit/3e90ba34ac19fc7648d9869cc386649a712a1532?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 23 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/18113?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...he/pinot/common/cursors/AbstractResponseStore.java](https://app.codecov.io/gh/apache/pinot/pull/18113?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Fcursors%2FAbstractResponseStore.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY3Vyc29ycy9BYnN0cmFjdFJlc3BvbnNlU3RvcmUuamF2YQ==) | 0.00% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18113?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (736cbf7) and HEAD (3e90ba3). Click for more details.
>
> <details><summary>HEAD has 28 uploads less than BASE</summary>
>
>| Flag | BASE (736cbf7) | HEAD (3e90ba3) |
>|------|------|------|
>|unittests1|2|1|
>|unittests|3|1|
>|java-11|5|0|
>|temurin|8|1|
>|java-21|3|1|
>|integration|5|0|
>|integration2|1|0|
>|unittests2|1|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18113 +/- ##
============================================
- Coverage 63.75% 55.77% -7.99%
+ Complexity 1573 799 -774
============================================
Files 3167 2480 -687
Lines 191658 142407 -49251
Branches 29469 22930 -6539
============================================
- Hits 122198 79431 -42767
+ Misses 59851 56159 -3692
+ Partials 9609 6817 -2792
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18113/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/18113/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/18113/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/18113/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/18113/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/18113/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/18113/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.77% <0.00%> (-0.32%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18113/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.77% <0.00%> (-7.99%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18113/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.77% <0.00%> (-7.98%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18113/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.77% <0.00%> (-0.35%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18113/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/18113?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix multi-controller race conditions in ResponseStoreCleaner cursor cleanup [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #18113:
URL: https://github.com/apache/pinot/pull/18113#discussion_r3046265097
##########
pinot-controller/src/main/java/org/apache/pinot/controller/cursors/ResponseStoreCleaner.java:
##########
@@ -114,6 +115,10 @@ private static long getFrequencyInSeconds(ControllerConf config) {
@Override
protected void processTables(List<String> tableNamesWithType, Properties periodicTaskProperties) {
+ // Make it so that only one controller is responsible for cleanup.
+ if (!_leadControllerManager.isLeaderForTable(TASK_NAME)) {
+ return;
+ }
Review Comment:
Good catch, this should be fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix multi-controller race conditions in ResponseStoreCleaner cursor cleanup [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #18113:
URL: https://github.com/apache/pinot/pull/18113#discussion_r3046198365
##########
pinot-controller/src/main/java/org/apache/pinot/controller/cursors/ResponseStoreCleaner.java:
##########
@@ -114,6 +115,10 @@ private static long getFrequencyInSeconds(ControllerConf config) {
@Override
protected void processTables(List<String> tableNamesWithType, Properties periodicTaskProperties) {
+ // Make it so that only one controller is responsible for cleanup.
+ if (!_leadControllerManager.isLeaderForTable(TASK_NAME)) {
+ return;
+ }
Review Comment:
`ControllerPeriodicTask.runTask()` only invokes `processTables()` when this controller is leader for at least one entry returned by `getTablesToProcess()`. With the new `isLeaderForTable(TASK_NAME)` gate inside `processTables()`, it’s possible that the controller that is leader for `TASK_NAME` is not leader for any actual table (e.g., few tables / many controllers), causing `processTables()` to never be called on that controller and the cleaner to never run. Consider overriding `getTablesToProcess()` to return a singleton list containing `TASK_NAME` (or switching this task to `BasePeriodicTask`) so exactly one controller runs cleanup independent of table distribution, and then remove/avoid the additional early-return gate.
##########
pinot-common/src/main/java/org/apache/pinot/common/cursors/AbstractResponseStore.java:
##########
@@ -233,9 +236,18 @@ public boolean deleteResponse(String requestId) throws Exception {
return false;
}
- long bytesWritten = readResponse(requestId).getBytesWritten();
+ // Read bytesWritten for metrics tracking. The response may be deleted concurrently between exists() and
+ // readResponse() (TOCTOU race), so handle that gracefully.
+ long bytesWritten = 0;
+ try {
+ bytesWritten = readResponse(requestId).getBytesWritten();
+ } catch (Exception e) {
+ LOGGER.debug("Could not read response metadata for requestId={} (may have been deleted concurrently)",
Review Comment:
The intent here is to handle the TOCTOU “deleted between exists() and readResponse()” race, but catching a generic `Exception` can also hide non-race failures (e.g., deserialization bugs, permission issues, transient FS errors), and it will silently skip metrics adjustments in those cases. Consider narrowing the catch to the expected “not found”/IO exceptions (or rethrow unexpected exceptions) so real read failures aren’t masked.
```suggestion
// readResponse() (TOCTOU race), so only suppress read failures when the response no longer exists.
long bytesWritten = 0;
try {
bytesWritten = readResponse(requestId).getBytesWritten();
} catch (Exception e) {
if (exists(requestId)) {
throw e;
}
LOGGER.debug("Could not read response metadata for requestId={} because it was deleted concurrently",
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix multi-controller race conditions in ResponseStoreCleaner cursor cleanup [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya merged PR #18113:
URL: https://github.com/apache/pinot/pull/18113
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org