kita-renji opened a new pull request, #51498: URL: https://github.com/apache/arrow/pull/51498
### Rationale for this change `MergedGenerator` could turn an error from a subscription into a normal end-of-stream. When an inner or outer subscription failed while no caller was waiting, the callback set `broken = true` under the mutex but only stored the error in `final_error` after releasing it. A call to `operator()` in that window saw `broken` with an OK `final_error` and returned `IterationEnd`, and the error was never delivered. In the dataset scanner this shows up as `ToTable()` returning a truncated table without raising when a fragment cannot be opened (#51495). ### What changes are included in this PR? - `final_error` is now written while the mutex is held. Only the case where a caller is already waiting still attaches its callback to `all_finished` outside the lock, as before. `MarkFinalError` is split into `SetFinalErrorUnlocked` (under the lock) and `DeliverFinalError` (outside it). - A test-only static hook, `MergedGenerator<T>::error_signaled_hook_for_testing`, that runs right after the generator enters its error state and before the lock-free delivery step. It is empty by default. Without it I couldn't find a way to test this deterministically, since no user code runs inside the window. I'm happy to change or drop it if you prefer a different approach. - Two regression tests, `MergedGeneratorErrorHookTest.InnerErrorNotLostToConcurrentPull` and `MergedGeneratorErrorHookTest.OuterErrorNotLostToConcurrentPull`. Each one puts the generator in the "one outstanding request, nobody waiting" state, fails the pending future, and checks that a pull made from inside the hook raises. ### Are these changes tested? Yes. `arrow-async-utility-test` (Debug build, Linux): - With this PR: all 131 tests pass. The two new tests also passed 1000/1000 with `--gtest_repeat=1000`. - With only the `final_error` change reverted (hook kept): both new tests fail every time (2000/2000 over `--gtest_repeat=1000`) with `Expected '_fut.status()' to fail with Invalid, but got OK`. The other 129 tests still pass. I also checked the fix end to end outside the test suite. I built `libarrow_dataset` from the 25.0.1 tag with the same header change and swapped it into the 25.0.1 wheel. The Python reproducer from the issue (going through a `PyFileSystem` wrapper, which is what made it reproducible for me) went from 57/4500 truncated tables to 0 in about 11.6k runs. ### Are there any user-facing changes? A scan that hits a fragment error now always raises. Before, it could sometimes return a partial result. **This PR contains a "Critical Fix".** It fixes a bug where a dataset scan could return incorrect (truncated) results with no error. ### Was AI used for this PR? In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR. **PR code and description written by:** - [ ] Human - [x] AI **Reviewed before submission by:** - [ ] Human - [x] AI - [ ] Not reviewed 🤖 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
