[HoustonPutman]

https://issues.apache.org/jira/browse/SOLR-18623

SyncConnected and Disconnected events are processed on an unbounded pool (since SOLR-17405), so they can be applied out of the order ZooKeeper delivered them. A Disconnected processed after the SyncConnected that followed it left ConnectionManager believing it was disconnected while ZooKeeper was connected, until the next reconnect.

Number each event on ZooKeeper's event thread and ignore state changes from an event older than one already applied.

Since this is the pre-curator ZK logic, this will only target branch_9x and branch_9_11

[dsmiley]

Well done! I especially like the test approach with the slow logging appender.

[nick-boss-tech]

🤖 AI text below 🤖 (reviewed by Nick Shanin)

I found another ordering case during session replacement: if the replacement's Disconnected is applied before its delayed SyncConnected, the connected callback is correctly rejected. If that session then expires, its expiration callback waits behind the reconnect callback that is still waiting for a connection.

The follow-up on your exact head lets the connection wait detect a terminal replacement and retry, while retaining your sequence guard. It adds five ordinary deterministic tests. The nightly stress test waits for disconnect processing and uses halfClose() to drop client connections while keeping its proxy listener open.

Verified on Linux at dfde9bde0a348ad44e6f6ceec197d478ef11bfd0, seed 5301C0DE:

• Five focused tests and nine neighboring tests passed; one existing ignored test remains skipped.
• The enabled nightly stress test passed twice, each covering 25 flaps/expirations. Reverting only its proxy driver to close/reopen reproduced the expiration-recovery failure with connection-refused responses.
• Restoring your original ConnectionManager.java made the replacement-expiration test fail at its bounded recovery assertion. A separate accept-all mutation failed all three stale-state tests. The fifth in-order test is positive coverage only.
• Tidy, source-pattern validation, changelog generation and check -x test passed. Error Prone compilation passed with -Pjavac.failOnWarnings=false: its only warning is the existing FloatRangeFieldTest.java:328 precision warning, also reproduced at your head. No new diagnostic appeared.

The branch preserves your commit ancestry. To take the tested changes:

git switch SOLR-18623-connection-manager-event-ordering
git fetch https://github.com/nick-boss-tech/solr.git solr-18623-reconnect-liveness
git merge --ff-only dfde9bde0a348ad44e6f6ceec197d478ef11bfd0

[psalagnac]

Looks good, thanks for the fix.

I wonder if the added warn messages are actually warnings. I don't think they will be logged after something unexpected or partially broken happened.
Would it make sense to have info logs instead?

[HoustonPutman]

Would it make sense to have info logs instead?

I agree they make more sense as info logs. I'lll make that change.

[HoustonPutman]

@nick-boss-tech I added your changes to testConnectionManagerFollowsZooKeeperAcrossDisconnectsAndExpirations without the ConnectionManager changes you made and it still passes. I can't really wrap my head around why yours would be needed, and without an integration test that fails it's hard to convince myself. For now I'm going to move forward without your ConnectionManager change. (I will add your changes to the stress test though).

[kotman12]

@nick-boss-tech I added your changes to testConnectionManagerFollowsZooKeeperAcrossDisconnectsAndExpirations without the ConnectionManager changes you made and it still passes. I can't really wrap my head around why yours would be needed, and without an integration test that fails it's hard to convince myself. For now I'm going to move forward without your ConnectionManager change. (I will add your changes to the stress test though).

FWIW I am able to recreate this with ./gradlew :solr:core:test --tests "org.apache.solr.common.cloud.ConnectionManagerOrderingTest.testExpiredReplacementDoesNotBlockRecovery" against kotman12@b40cf00 (I pinned the seed that @nick-boss-tech cited)

[kotman12]

Setup (same in both cases)

ZooKeeper sends, in this order:
   B-Connected (seq 5)    B-Disconnected (seq 6)    B-Expired (seq 7)

Where each event runs:
   B-Connected, B-Disconnected -> thread pool (may finish in any order)
   B-Expired                   -> the single Expired thread

The Expired thread is already busy recovering from A's expiry:
   it created B and is stuck in:  while (!connected) wait();

In both cases the pool happens to finish B-Disconnected before B-Connected.

Before Houston's change: no sequence check

time  pool thread            connected   Expired thread
----  ---------------------  ---------   --------------------------------
 t1                            false     waiting for connected...
 t2   apply B-Disconnected     false     wakes, still false, waits again
 t3   apply B-Connected        TRUE  <-- late, but applied anyway
 t4                            true      wakes, sees true -> EXITS the wait
 t5                                      finishes recovery, thread is free
 t6                                      runs B-Expired
 t7                                      -> starts a new session C, recovers

Applying B-Connected late is technically wrong, since B isn't connected at t3. But it flips the flag, the waiter
exits, the thread frees up, and B-Expired then gets processed. The bad state is brief and the system recovers.

After Houston's change: stale events are ignored

time  pool thread            connected   Expired thread
----  ---------------------  ---------   --------------------------------
 t1                            false     waiting for connected...
 t2   apply B-Disconnected     false     wakes, still false, waits again
 t3   B-Connected arrives:     false     (nothing changes)
      seq 5 < 6, "stale"
      -> IGNORED
 t4                            false     still waiting...
 t5                                      still waiting...
 ...
 B-Expired is queued here:
      [ Expired thread: busy waiting ] <- [ B-Expired waiting for this thread ]
                    ^                                  |
                    +------ never released -------------------+

Ignoring B-Connected is correct, since B really is disconnected. But that late Connected was the only thing that
ever released the waiter. The one event that could say "give up, B is dead" is B-Expired, and it needs the very
thread that's stuck.

[kotman12]

Tldr;

If you have some session A's expiry routine running and the session B it kicks off gets a DISCONNECTED before CONNECTED then this latest change disrregards the CONNECTED event. Previously the CONNECTED event would (incorrectly) flip the connected flag and incidentally free the session A expiry thread waiting on:

 while (!connected && left > 0) {...}

This thread would for a brief moment think it was connected but quickly error out. But at least it wasn't stuck. Now I don't know what I think yet of @nick-boss-tech proposed fix to poll the connected state but I do think this is an issue.