[PR] [fix][broker] Fix NPE in ManagedLedgerInterceptorImpl when AppendIndexMetadataInterceptor isn't configured [pulsar]

["lhotari (via GitHub)" <gi...@apache.org>]

lhotari opened a new pull request, #26497:
URL: https://github.com/apache/pulsar/pull/26497

   Fixes #26496
   
   ### Motivation
   
   `ManagedLedgerInterceptorImpl.onManagedLedgerLastLedgerInitialize()` dereferences the nullable field `appendIndexMetadataInterceptor` without a null check:
   
   https://github.com/apache/pulsar/blob/4998cd9e4f66ade6a7972da4afcd0f1546ddfc16/pulsar-broker/src/main/java/org/apache/pulsar/broker/intercept/ManagedLedgerInterceptorImpl.java#L113-L128
   
   The field is null whenever the constructor's interceptor set contains no `AppendIndexMetadataInterceptor`. Every other method of the class that touches the field (`getIndex`, `afterFailedAddEntry`, `onManagedLedgerPropertiesInitialize`, `onUpdateManagedLedgerInfo`) null-checks it first; this one does not.
   
   `BrokerService` installs `ManagedLedgerInterceptorImpl` on every persistent topic when `isBrokerEntryMetadataEnabled() || isBrokerPayloadProcessorEnabled()`, and those predicates are just `!brokerEntryMetadataInterceptors.isEmpty()` / `!brokerEntryPayloadProcessors.isEmpty()`. So the field is null while the interceptor is still installed in two supported configurations:
   
   1. `brokerEntryMetadataInterceptors` contains only `AppendBrokerTimestampMetadataInterceptor`.
   2. `brokerEntryMetadataInterceptors` is empty and `brokerEntryPayloadProcessors` is configured — the interceptor set passed to the constructor is then empty.
   
   In either configuration, loading a topic whose last ledger's last entry still carries a `BrokerEntryMetadata` with `hasIndex() == true` throws an NPE inside the `thenAccept` callback. `ManagedLedgerImpl.initialize()` wraps it in `ManagedLedgerInterceptException` and calls `callback.initializeFailed(...)`, so the topic fails to load:
   
   ```
   org.apache.bookkeeper.mledger.ManagedLedgerException$ManagedLedgerInterceptException: java.lang.NullPointerException: Cannot invoke "org.apache.pulsar.common.intercept.AppendIndexMetadataInterceptor.recoveryIndexGenerator(long)" because "this.appendIndexMetadataInterceptor" is null
   Caused by: java.lang.NullPointerException: Cannot invoke "org.apache.pulsar.common.intercept.AppendIndexMetadataInterceptor.recoveryIndexGenerator(long)" because "this.appendIndexMetadataInterceptor" is null
   	at org.apache.pulsar.broker.intercept.ManagedLedgerInterceptorImpl.lambda$onManagedLedgerLastLedgerInitialize$0(ManagedLedgerInterceptorImpl.java:121)
   	at org.apache.pulsar.common.protocol.Commands.peekBrokerEntryMetadataAndConsume(Commands.java:2244)
   	at org.apache.pulsar.broker.intercept.ManagedLedgerInterceptorImpl.lambda$onManagedLedgerLastLedgerInitialize$1(ManagedLedgerInterceptorImpl.java:119)
   ```
   
   The failure is deterministic and self-repeating rather than transient: the topic cannot be loaded, so no broker can write a non-indexed entry to it, and every retry re-reads the same last entry. The topic stays unavailable until the configuration is reverted.
   
   The realistic trigger is a configuration change over time: an operator removes `AppendIndexMetadataInterceptor` from `brokerEntryMetadataInterceptors` (keeping the timestamp interceptor, or having payload processors configured) and restarts. Every topic written under the old configuration then fails to load. The same happens transiently during a rolling restart, or with a heterogeneous `broker.conf` across the broker pool, where brokers still on the old configuration keep writing indexed entries while topics move to brokers on the new configuration.
   
   This is a regression. The guard existed from #10706 and was preserved by #20112 as `boolean hasAppendIndexMetadataInterceptor = appendIndexMetadataInterceptor != null;`. It was dropped by #23311 (commit 9ebd97941d8e), which rewrote the method onto the new `LastEntryHandle.readLastEntryAsync()` API: the `lh.getLastAddConfirmed() >= 0` half of the old condition was relocated into `ManagedLedgerImpl.createLastEntryHandle` (where it is preserved as `Optional.isPresent()`), but the `appendIndexMetadataInterceptor != null` half was relocated nowhere. `git log -S hasAppendIndexMetadataInterceptor` on the file returns exactly two commits: the one that introduced the guard and #23311 which removed it.
   
   Affected branches, verified against the source on GitHub: `master`, `branch-4.2`, `branch-4.1` and `branch-4.0` are unguarded; `branch-3.3` and `branch-3.0` still carry the guard, so no 3.x backport is needed. The first release containing #23311 is 4.0.0.
   
   ### Modifications
   
   - `ManagedLedgerInterceptorImpl.onManagedLedgerLastLedgerInitialize` returns a completed future immediately when `appendIndexMetadataInterceptor` is null, restoring the pre-#23311 behaviour. Reading the last entry is skipped entirely rather than guarded inside the callback: with no index generator to recover into, the read has no possible effect, and skipping it also removes a pointless last-entry read from the topic recovery path for every topic on brokers configured that way. The path where the interceptor is present is untouched.
   - Annotated the field with JSpecify `@Nullable` to document the invariant that was violated.
   
   The same guard also covers the shadow-topic path: `ShadowManagedLedgerImpl.doInitialize` calls the same method on the source ledger.
   
   ### Verifying this change
   
   - [x] Make sure that the change passes the CI checks.
   
   This change added tests and can be verified as follows:
   
   Three tests were added to `ManagedLedgerInterceptorImplTest`:
   
   - `testLastLedgerInitializeSkipsIndexRecoveryWithoutIndexInterceptor` — a `@DataProvider` covers both null-field configurations (timestamp interceptor only, and an empty interceptor set). It stubs `LastEntryHandle` with an entry carrying a `BrokerEntryMetadata` index and asserts that the returned future completes normally and that the last entry was not read at all, which pins the guard's placement before `readLastEntryAsync()` rather than merely the absence of an NPE.
   - `testLastLedgerInitializeRecoversIndexWithIndexInterceptor` — the positive control: with `AppendIndexMetadataInterceptor` configured, the last entry is still read and the index is still recovered.
   - `testRecoveryIndexAfterIndexInterceptorRemovedFromConfiguration` — reproduces the production scenario end to end through `ManagedLedgerImpl.initialize`: write entries with the index interceptor configured, close, then reopen with it removed.
   
   All three fail on the unpatched code for the real reason. Reverting only the guard makes `testRecoveryIndexAfterIndexInterceptorRemovedFromConfiguration` fail with the `ManagedLedgerInterceptException` shown above, and both data-provider cases of `testLastLedgerInitializeSkipsIndexRecoveryWithoutIndexInterceptor` fail with the NPE.
   
   With the fix, `ManagedLedgerInterceptorImplTest` (12 tests), `MangedLedgerInterceptorImpl2Test` and `BrokerEntryMetadataE2ETest` (15 tests) all pass.
   
   ### Does this pull request potentially affect one of the following parts:
   
   *If the box was checked, please highlight the changes*
   
   - [ ] Dependencies (add or upgrade a dependency)
   - [ ] The public API
   - [ ] The schema
   - [ ] The default values of configurations
   - [ ] The threading model
   - [ ] The binary protocol
   - [ ] The REST endpoints
   - [ ] The admin CLI options
   - [ ] The metrics
   - [ ] Anything that affects deployment
   


-- 
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@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [fix][broker] Fix NPE in ManagedLedgerInterceptorImpl when AppendIndexMetadataInterceptor isn't configured [pulsar]

["lhotari (via GitHub)" <gi...@apache.org>]

lhotari merged PR #26497:
URL: https://github.com/apache/pulsar/pull/26497


-- 
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@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org