serhiy-bzhezytskyy opened a new pull request, #16474:
URL: https://github.com/apache/lucene/pull/16474
### Description
`CheckIndex#exorciseIndex` dereferences `Status#newSegments` after checking
only `Status#partial`. When the commit point cannot be read, `checkIndex`
leaves `newSegments` null and sets `missingSegments` instead, so the public
method fails with:
```
java.lang.NullPointerException: Cannot invoke
"org.apache.lucene.index.SegmentInfos.changed()"
because "result.newSegments" is null
at org.apache.lucene.index.CheckIndex.exorciseIndex(CheckIndex.java:4594)
```
Reproduced on `main` with three corruptions, each of them one that users
report on #7820:
| corruption | `DirectoryReader.open` | `checkIndex` | `exorciseIndex` |
|---|---|---|---|
| `_1.si` deleted | `CorruptIndexException` | `clean=false
missingSegments=true` | **NPE** |
| `_1.si` truncated | `CorruptIndexException` | same | **NPE** |
| `segments_N` truncated | `CorruptIndexException` | same | **NPE** |
The command line is not affected — `doCheck` has `if
(result.missingSegments) return 1;` at `CheckIndex.java:4880`. Luke is:
`CheckIndexDialogFactory` enables its Repair button on `if (!st.clean)`
(`:304`) and reads `missingSegments` only when building the result text
(`:329`), while `IndexUtils#tryRepairIndex` (`:318`) calls
`ci.exorciseIndex(st)` with no check of its own. So a Luke user who checks an
index with an unreadable commit point gets an enabled Repair button and an NPE
when they press it.
This is what @mikemccand warned about when `skipMissing` was proposed on
#7820 in 2015:
> `skipMissing` is a spooky option; if accidentally mis-used it can make you
think you have all segments **if you forget to check `missingSegmentInfo`**
This change does not implement the repair that #7820 asks for. It only makes
the failure legible until that exists, and the guard is symmetric with the
`partial` check already in the method.
### The test
@rmuir asked for exactly this on #12530 in August 2023:
> as far as the exorcise stuff, I think a good next step would be to start
writing some unit tests that invoke exorcise? we have a grand total of zero
tests exercising-exorcise, and I think it is part of the problem.
@mikemccand agreed in the same thread — "It's terrifying to have zero test
coverage for such a dangerous feature!". `grep exorciseIndex
lucene/core/src/test/` still returns nothing on `main`, so
`testExorciseUnreadableCommitPoint` is the first test that calls the method at
all. It covers all three corruptions and sits next to
`testPriorBrokenCommitPoint`, which is step 1 of the same issue.
### Relationship to #12872
@gokaai's PR #12872 is open since 2023-12 and its description says it is "a
step towards enabling #7820". Its author also wrote there:
> Will create a new commit (or issue?) to add in fixes and unit tests for
`exorciseIndex`
That is what this PR does, so it is not a competing change — it is the piece
deferred there, and it stands whether or not #12872 lands. If it is easier to
fold this into that PR instead, I will close this one. I am not proposing to
take over step 2.
### Verification
- The test fails without the fix: `AssertionFailedError: Unexpected
exception type, expected IllegalArgumentException but got
java.lang.NullPointerException`
- Removing the guard and re-running fails; restoring it passes
- `./gradlew :lucene:core:test --tests TestCheckIndex` passes
- `./gradlew :lucene:core:check -x test` passes
- `./gradlew tidy` reports no changes
Verified on `main` only; the 10.x branches are not checked.
--
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]
