[#807] Do not drop persistent search notifications through search-phase dedup

[vharseko]

Fixes #807

Problem

SearchOperationBasis keeps the DNs of the entries already returned, so that an entry reached both directly and through an alias is only sent once. endSearchPhase() is what stops this de-duplication from applying to persistent search notifications, but it runs after the search has been registered with the backend (LocalBackendSearchOperation.java:251 → :263). A notification delivered inside that window for an entry which was already reported is silently dropped, and the client waits forever: no SEARCH RES, no ABANDON, nothing in the error log. A real client running a persistent search with derefAliases: always (or in-searching) can miss a change the same way — with changesOnly=false the window is the whole search phase, not a few instructions.

The same window leaves notifications subject to the size and time limits of the search: both are only lifted in the finally block of SearchOperationBasis.run(), which runs after the persistent search is already registered and receiving changes.

Fix

Persistent search notifications report changes, not search results, so they no longer go through returnEntry():

• new SearchOperation.returnPersistentSearchEntry(entry, controls), used by PersistentSearch.sendEntry(). It skips the search-phase de-duplication and the size/time limits, so the timing of endSearchPhase() no longer affects delivery at all;
• entriesSent is now atomic — notifications are counted by the threads applying the changes, several at a time;
• every entry returnEntry() drops silently (access control, de-duplication, a search result entry plugin) is now traced, so the next occurrence is diagnosable.

Independently, a persistent search cancelled because its backend was finalized used to be dropped without a word to the client, which then waited forever on a search which no longer existed. LocalBackend.finalizeBackend() now sends a search result done with UNAVAILABLE.

Tests

• AliasTestCase.test_persistent_search_reports_repeated_changes now checks which change each notification reports (description: change N) instead of only the DN — with identical DNs a lost notification could not be told from a duplicated one. On failure it reports whether the persistent search is still registered and what is left in the queue, and it waits for a persistent search on its own base DN rather than for a non-empty registry, which another test could satisfy.
• New AliasTestCase.test_persistent_search_notification_ignores_search_phase_state drives the notification path directly: the search phase de-duplicates the entry, notifications are reported both while that phase is open and after it is over, and they are not bound by the limits which still bound the search phase. Reverting the fix makes it fail with Expected size: 3 but was: 2.

Ran locally under -P precommit: AliasTestCase 24/24, and ModifyOperationTestCase, SearchOperationTestCase, TestModifyDNOperation, InternalSearchOperationTestCase, PersistentSearchControlTest, ControlsTestCase green (1203 tests).

[maximthomas]

The fix is correct and I verified it end-to-end against a real server — it is worth more than the description claims. Two things should change before merge, and #807 should not be auto-closed.

This is not just a CI flake, it is production data loss. Standalone instance, 5003 entries, changesOnly=false persistent search with a slow reader (search phase open ~40 s), three modifications to an entry the search phase had already returned:

Build	derefAliases	Entries streamed	Changes 1–3
master	always	5003	all three silently lost
master	never (control)	5006	all three delivered
this PR	always	5006	all three delivered

Any persistent search with changesOnly=false and derefAliases: always/in-searching drops every change to an already-returned entry for as long as the initial content takes to stream. Worth leading the PR description with this instead of the flaky test.

Only one of the two flakes in #807 is fixed (major)

From the two CI access logs:

• 2026-07-30 — MODIFY op=18, op=19 only; notification 2 lost after 1 was delivered. This is exactly the dedup window. Fixed.
• 2026-07-31 — MODIFY op=18, op=19, op=20; notifications 1 and 2 delivered, 3 lost. Not explainable by this PR: searchPhaseOver is monotonic, so if notification 2 passed the check, 3 must too. Limits are excluded as well — opendj-server-legacy/tests/unit-tests-testng/resource/configForTests/config-small.ldif:31-32 sets size-limit 1000 / time-limit 60 seconds, and a limit hit returns false → cancel() + sendSearchResultDone() → a SEARCH RES that is absent from the log.

I also ruled out the reactive transport by experiment (30/30 notifications at 300 ms spacing, 200/200 back-to-back), plus cross-test leakage (reuseForks=false → one JVM per test class), the admin psearch limit, client abandon/timeout, and the ACI/subentry/alias paths.

Please keep #807 open, scoped to the residual loss.

The new tracing produces nothing in CI (major)

opendj-server-legacy/src/test/java/org/opends/server/TestListener.java:170-175 matches org.opends.test.trace.pattern against the test class name, and only then calls TestCaseUtils.setupTrace(). The precommit pattern in opendj-server-legacy/pom.xml lists only two replication classes and HostPortTest, so AliasTestCase never enables tracing — which is why the Debug Log Messages: section is missing from both failure dumps. One line makes the added logger.trace calls pay off:

<org.opends.test.trace.pattern>(org\.opends\.server\.replication\.service\..*)|(org\.opends\.server\.replication\.GenerationIdTest)|(org\.opends\.server\.types.\HostPortTest)|(org\.openidentityplatform\.opendj\.AliasTestCase)</org.opends.test.trace.pattern>

"Every entry returnEntry() drops silently is now traced" is inaccurate (medium)

Four silent return true paths remain in opendj-server-legacy/src/main/java/org/opends/server/core/SearchOperationBasis.java:

• :551 — subentry not returned
• :557 — isReturnSubentriesOnly() and the entry is not a subentry
• :799 — alias points at a non-existent entry
• :815 — alias loop

The first two are on the notification path too. And since the surviving 07-31 loss is not in the dedup branch, the place that most needs instrumenting has none: the modify post-response callback, opendj-server-legacy/src/main/java/org/opends/server/core/ModifyOperationBasis.java:365 → opendj-server-legacy/src/main/java/org/opends/server/workflowelement/localbackend/LocalBackendModifyOperation.java:305-317.

finalizeBackend() notification is mostly inert on shutdown, and untested (minor)

DirectoryServer.shutDown() finalizes connection handlers at :4137-4148 and the plugin manager at :4164-4168, then backends via backendConfigManager.shutdownLocalBackends() around :4296. At shutdown the sockets are already closed, so sendSearchResultDone() no-ops or throws into the swallow-all catch (Exception e). The real value is on backend disable / re-initialization — worth saying so, since this half is unrelated to #807 and changes what every backend teardown sends to clients (including ChangelogBackend, so ECL clients now get unavailable). It also has no test.

Nits

• Dead line in the new test: search.setTimeLimitExpiration(Long.MAX_VALUE) — Requests.newSearchRequest(DN, SearchScope) leaves timeLimit at 0, so getTimeLimit() > 0 short-circuits and the expiration is never read.
• Last assertion proves less than the comment says: by then entriesSent == 4, so returnEntry trips the size limit first and the search-phase time-limit branch is never reached. Split it if you want both pinned.
• Boolean soup: returnEntry(entry, controls, true, true, null) — two adjacent unnamed booleans invite a transposition. A small enum (SEARCH_PHASE / PSEARCH_NOTIFICATION) would read better.
• searchPhaseOver is now near-dead: nothing calls plain returnEntry() after endSearchPhase() any more, so the flag only guards returnedDNs against a caller that no longer exists. Harmless, but endSearchPhase() could reduce to returnedDNs.clear().
• Inaccurate test comment: "Other tests may leave persistent searches behind on this backend" — the precommit profile sets reuseForks=false, so each test class gets its own JVM. Only a rerun of AliasTestCase itself could. The new isPersistentSearchRegistered() helper is still the right barrier, just not for that reason.
• WARN_PSEARCH_BACKEND_UNAVAILABLE is never logged: it only reaches the client as a diagnostic string. Given the PR's "no silent drops" theme, a logger.warn at the finalizeBackend() call site would be consistent.
• entriesSent → AtomicInteger reverts a deliberate decision: commit 339d387276 moved it the other way ("as pointed out by Ludo"). The revert is justified here — the plain int was a real data race once psearch threads increment it — but a line in the commit message would preempt the question. Note referencesSent stays a plain int, which is fine.
• Latent leak in a neighbouring test: opendj-server-legacy/src/test/java/org/opends/server/controls/PersistentSearchControlTest.java:562 calls search.cancel(new CancelRequest(...)), which is Operation.cancel — it never invokes PersistentSearch.cancel(), so the psearch is never deregistered from the backend and ds-cfg-max-psearches is left at 1. Harmless across classes given fork-per-class, but worth a follow-up.