[JasonWildMe]

Re-derivation of #1743 on the JDO transaction's own completion callback, as recommended by the Codex review of that PR. #1743 stays open as the problem record until this lands.

Closes #1696. That report (GiraffeSpotter, React encounter search) is this bug seen from the search page: the Identity filter "Include only encounters with no assigned Individual ID" listed encounters that already had an ID, because the encounter document's individualId was indexed from the pre-commit row on the match-results Confirm path (iaResultsSetID.jsp). That path never bumps Encounter.modified, so the background reconciler could not repair it and the stale rows persisted. Closes #1548.

The bug

WildbookLifecycleListener.postStore() fires on JDO flush, not commit. It enqueued the OpenSearch reindex there, and the indexing job opens its own Shepherd (its own connection) and reloads the object by id. For a row that already exists that reload succeeds and returns pre-commit state, so the job indexed the old values and reported success. Only a brand-new row raised JDOObjectNotFoundException, which is the one case the retry path handled. The queue's dedupe (if (indexingQueue.contains(id)) return;) then silently dropped the correct post-commit request whenever the pre-commit job was still in flight.

Search index stale, no data loss: Postgres is always right. Affected surfaces are the OpenSearch-backed ones, including the annotation index that feeds the match candidate pool.

What changed

The park. postStore, Encounter.enqueueAclReindex() and MarkedIndividual.enqueueAclReindex() no longer enqueue. They park (id, class, unindex, DataNucleus identity) in the writing PersistenceManager's user-object map. A transient object, or a caller that has already committed, falls through to an immediate enqueue exactly as before.

The release. A javax.transaction.Synchronization (IndexingTransactionSynchronization) is registered once per PM via Transaction.setSynchronization. DataNucleus calls its afterCompletion(status) from inside commit()/rollback() after its own post-commit work. That callback is the only thing that terminalizes a park: STATUS_COMMITTED drains it (evict from L2 by DataNucleus identity, then enqueue by identity), STATUS_ROLLEDBACK drops it, anything else runs index requests (the job re-reads the DB, so that is harmless) and never an ambiguous unindex. Because it hangs off the transaction, a raw pm.currentTransaction().commit() is covered identically.

What that removes. No edits to Shepherd.commitDBTransaction(), commitDBTransactionWithStatus(), updateDBTransaction(), rollbackDBTransaction(), closeDBTransaction() or rollbackAndClose(). No lock around Shepherd's lifecycle. The only Shepherd change is beginDBTransaction(): for a fresh PM it registers the listener and the Synchronization once (main re-added a WildbookLifecycleListener on every begin, so one store event fanned out to N callbacks) and installs the park before Transaction.begin().

The dedupe. A second request for an in-flight id is remembered and runs as exactly one follow-up pass when the job reaches any terminal outcome (success, failure, retry give-up), instead of being dropped.

The reader. The indexing job's PM sets datanucleus.cache.level2.retrieveMode=bypass for its primary lookup. See the boundary below for what that does and does not cover.

Reader setup cleanup. If the indexing reader's transaction initialization throws, ShepherdIndexingWork rolls back and closes its allocated Shepherd before rethrowing the same exception. Regression coverage verifies that the PersistenceManager closes and its Shepherd diagnostic entry is removed. A test through the public Shepherd.beginDBTransaction() method also verifies that repeated begins on the same Shepherd register the lifecycle listener and transaction synchronization only once.

Why this is safe where #1743 was not

• "Commit threw" no longer terminalizes anything. DataNucleus deliberately leaves the transaction active after a NucleusUserException so the caller can fix and retry. fix(indexing): hand OpenSearch reindexing to the queue after commit, not on JDO flush #1743 drained on that path, marking the park terminal while the transaction was still open, after which every later store in that transaction enqueued pre-commit again. Here the park stays live until the transaction actually completes. Tested against real DataNucleus with a one-shot vetoing Synchronization.
• L2 eviction actually evicts. The L2 cache is keyed by DataNucleus's internal identity; JDOHelper.getObjectId() returns a javax.jdo.identity.* object, and evicting with it silently misses. This uses Persistable.dnGetObjectId(), asserted against the real Level2Cache.
• Completion survives failures. The successor park is installed before any external work; each evict+enqueue is isolated; a null IndexingManager is logged, not thrown.
• A wrapped Synchronization cannot lose the park. Capture and successor installation happen before the delegate runs; the delegate may close the PM or throw.

Boundaries (stated on purpose)

• L2 read isolation is partial, and that is a separate problem. DataNucleus publishes an object's new state into the shared L2 cache during preCommit, before the datastore commit, so any reader in Wildbook can briefly see another in-flight transaction's uncommitted copy. That predates this PR and affects web requests as much as the indexer. The per-PM bypass here covers the job's primary getObjectById; in DataNucleus 5.2 it does not cover lazy field loads, bulk loads, or the second Shepherd the encounter serializer opens. Closing that needs either a dedicated L2-less PMF for indexing reads (a second connection pool per install) or a fleet-wide L2 policy decision. Follow-up, not this PR.
• Delivery is best-effort. A handoff that fails at enqueue time (no IndexingManager, executor rejected) is logged and dropped; there is no durable retry. The reconciler re-indexes only when the DB version is strictly greater than the indexed one.
• The Encounter.modified bump from fix(indexing): hand OpenSearch reindexing to the queue after commit, not on JDO flush #1743 is not here. Codex showed the second-resolution setter can decrease the version relative to EncounterForm's millisecond ISO value. A persisted monotonic revision is the right fix; separate PR.
• postDelete() still unindexes pre-commit. Separate fix.
• Deep-index child writes (individual, occurrence, annotations) run on their own executor after the job returns and can still be overtaken by an older cascade. This PR guarantees the top-level document only.
• Transient objects (no PM yet) and PMs Shepherd never prepared still enqueue immediately, as on main.
• Heuristic transaction outcomes are reported by DataNucleus as a rollback attempt; a heuristic-mixed outcome cannot be resolved here.
• logNestedExceptions null-safety refactor from fix(indexing): hand OpenSearch reindexing to the queue after commit, not on JDO flush #1743: unrelated once the drain is out of commitDBTransaction().

Tests

70 unit tests (no datastore) + 9 Testcontainers tests against real Postgres and DataNucleus 5.2.7, with the row oracle being a raw JDBC SELECT (no Shepherd, no DataNucleus cache):

• flush enqueues nothing; Shepherd commit enqueues exactly once, after the transaction ended; the row is committed by then
• raw pm.currentTransaction().commit() behaves identically
• rollback enqueues nothing
• a second transaction begun raw on the same PM parks and drains again
• vetoed commit leaves the transaction active and the park intact; retry enqueues once
• vetoed commit + rollback enqueues nothing
• vetoed commit + pm.close(): under the JDO adapter's default closeActiveTxAction=rollback the close rolls back, the park completes through the normal callback, nothing is enqueued, row unchanged
• committed completion evicts the row from the real L2 by DataNucleus identity
• the indexing job's reader PM has L2 retrieve mode bypass

RED evidence (Testcontainers test run against the pre-change listener): 8 of 9 fail, the first with

NoInteractionsWanted ... But found these interactions on mock 'indexingManager':
-> at org.ecocean.WildbookLifecycleListener.postStore(WildbookLifecycleListener.java:72)

that is, the enqueue at flush time.

Latest full backend run, with the independent membership-fixture correction from #1831 temporarily applied for local validation: 1777 tests, 0 failures, 0 errors, 7 skipped. The final cleanup and public-Shepherd assertions were additionally verified on this branch with IndexingJobWiringTest and ShepherdTest (21 tests, all passed). Claude reviewed the follow-up and found no blocking issues.

CI dependency: #1831 fixes the 23 membership-fixture errors already present on main. Merge it first, then update this branch from main; its fixture changes are kept out of this PR's follow-up commit.

Credits

Written by Claude Fable 5.1 (Claude Code). Codex (codex-cli 0.153.4, gpt-6-astra, reasoning effort high) reviewed #1743 and proposed this design, then reviewed the plan and the diff for this PR. Its #1743 review found the failed-commit terminalization bug this design removes. Its plan review caught four load-bearing problems before any code was written: the L2 eviction key mismatch, the pre-commit L2 publication, the delegate-first callback order that could lose the park, and the need for per-entry failure isolation with the successor installed first. Its diff review established that the per-PM L2 bypass is partial in DataNucleus 5.2, explained the close-on-active-transaction behavior via the JDO adapter default, and pushed the test oracle to raw JDBC.

Review status, stated plainly: Codex's first diff round returned not converged with two Majors, both about the partial L2 bypass. Those are answered with the scope boundary above rather than with code, because the fix they call for (an L2-less PMF or a fleet-wide cache policy) is a separate change with its own blast radius. The convergence round then evaluated the branch against that stated boundary and returned "CONVERGED: no Major findings within scope", accepting the boundary as legitimate ("not a dodge: shared-L2 isolation is a separate, pre-existing problem"). Its two remaining Minors (a DataNucleus pin() quirk in the L2 test fixture, and four comment phrasings that overclaimed recovery or isolation) are fixed in the last commit. Three Codex rounds in total on this PR: plan, diff, convergence.

Earlier rounds of #1743 were reviewed by Codex gpt-5.6-terra.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.89%. Comparing base (06debf4) to head (06e2a60).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1752   +/-   ##
=======================================
  Coverage   55.89%   55.89%           
=======================================
  Files         316      316           
  Lines       12852    12852           
  Branches     4172     4172           
=======================================
  Hits         7184     7184           
  Misses       5367     5367           
  Partials      301      301           

Flag	Coverage Δ
backend	55.89% <ø> (ø)
frontend	55.89% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[UrbanFercec]

Ran it merged into today's main with the Testcontainers tests, all green. Read through the DataNucleus 5.2.7 TransactionImpl too: afterCompletion runs after the L2 put and inactive, so the evict order holds. Two small things inline.

[UrbanFercec]

If begin throws anything other than JDOUserException or NPE, say a JDODataStoreException while the DB is down, the constructor throws, work stays null and the finally in run() never closes this Shepherd. Main assigned bgShepherd before begin, so its rollbackAndClose still ran. A try/catch here that closes and rethrows would keep that.

[UrbanFercec]

Passing null instead of listenerRegisteredOn here brings back one listener per begin, and the Indexing*, ShepherdTest and ShepherdBeginRegistrationTest suites all stay green. A test that begins twice on one Shepherd and checks addInstanceLifecycleListener ran once would pin the fan-out fix.

[UrbanFercec]

Both points are covered now. The failed-begin test catches it when the close in the catch is removed, and the repeated-begin test catches the listener going back to null. Indexing and Shepherd suites pass on today's main.

[naknomum]

lgtm