airborne12 opened a new pull request, #66961:
URL: https://github.com/apache/doris/pull/66961
### What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary:
Two cases under `be/test/storage/index/snii/` reach outside their own test
and
break unrelated cases. Both are test-only; no production code changes.
**1. A fixed `/tmp` scratch path (`metered_file_reader_test.cpp`)**
`MakeRampFile()` built its 256-byte ramp file at the constant path
`/tmp/snii_metered_ramp.bin`. On a shared build machine that file belongs to
whoever ran the suite first; every later user gets `EACCES` from `::open`,
and
all seven `SniiMeteredFileReader` cases fail at the first assertion with no
hint
that a foreign file is the cause:
```
metered_file_reader_test.cpp:42: Failure
Value of: w.open(path).ok() Actual: false Expected: true
```
Every other scratch path in this subtree already namespaces itself with the
pid
and a counter, and the nearest neighbour in the same directory
(`file_reader_caller_buffer_test.cpp`) also `std::remove()`s the file
afterwards.
This PR adopts both halves of that convention: the ramp file becomes a
`RampFile`
RAII object whose name carries the pid and a counter and whose destructor
removes
it. Removing matters as much as naming — a leftover file is what causes the
collision in the first place, and pids get recycled.
**2. A permanently lowered `RLIMIT_NOFILE` (`spimi_term_buffer_test.cpp`)**
`SniiSpimiTermBuffer.SpillOpenIoFailureLatched` drives a spill's `::open` to
`EMFILE` by lowering `RLIMIT_NOFILE` to a hardcoded 64 and then opening
`/dev/null` until the table is full. Two things go wrong once the binary
already
holds more than 64 descriptors:
1. The cap assumes the process holds fewer than 64 descriptors, so the hog
loop
can still take one. Sampling `/proc/<pid>/fd` during a run shows the SNII
cases
alone keep the process at **14** descriptors, but adding the CLucene
inverted
index cases to the same binary takes it to **273** — they hold hundreds of
index files open at once. Above the cap the very first `::open` already
fails,
`hogs` stays empty, and the case stops at `ASSERT_FALSE(hogs.empty())`.
What
decides this is *which* cases share the binary, not how many.
2. That assertion is fatal, so it returns from the test body — and the code
that
restores `RLIMIT_NOFILE` sits after it and never runs. The whole process
stays
capped at 64 descriptors for every later case.
The result is not a descriptor leak but a cascade: the next four
`SniiSpimiTermBuffer` cases fail instantly because they cannot open a spill
file,
and the first death test after them aborts the entire run when gtest's
`pipe()`
hits `EMFILE`:
```
CHECK failed: gtest-death-test.cc, line 1400: pipe(pipe_fd) != -1
Aborted
```
The fix holds the tight limit and the hog descriptors in an inner scope whose
`Defer` restores `RLIMIT_NOFILE` and closes the descriptors on every exit
path,
fatal assertion included; and derives the cap from `::dup(STDIN_FILENO)`, the
lowest free descriptor, instead of a fixed 64, so the case no longer depends
on
which tests ran before it. The case still drives the spill open to `EMFILE`
and
still asserts the error is latched.
**Before / after**, with
`--filter='*Snii*:*snii*:*SNII*:CollectionStatisticsTest.*:*InvertedIndex*:*inverted_index*'`:
| | before | after |
|---|---|---|
| `SniiMeteredFileReader` | 7 failed | pass |
| `SniiSpimiTermBuffer*` | 5 failed | pass |
| death test | aborted the run | pass |
| total | run truncated by the abort | 1693 passed, 0 failed |
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
- Behavior changed:
- [x] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
