Opened in error#4615
Closed
markrmiller wants to merge 49 commits into
Closed
Conversation
…e deadlock, ZK client close nap Three fixes that bring the worst :solr:core:test outliers in line with the rest (suite-time ~737s -> ~629s; slowest suite 35s -> 24s; p99 16.6s -> 13.8s): - ZkShardTermsTest: waitForCounts/waitFor early-returned on exact '==' against a monotonic, async-updated count that overshoots the target, so each call spun its full 10s TimeOut before the lenient (expected <= actual) assert passed anyway. Return as soon as the threshold is reached. testCoreTermWatcher 30.0s -> 0.03s. - SolrInternalHttpClient.doStop: fix a Jetty 10.0.3 AB-BA deadlock that could hang the JVM when closing an SSL Http2SolrClient (caught hanging the suite). The HTTP/2 client registers each HTTP2Session as a managed bean; a selector thread closing a connection (holding the SslConnection lock) runs onClosed -> removeBean -> stop session (needs the session lifecycle lock) while this thread, stopping that same session, holds the lifecycle lock and needs the SslConnection lock for the graceful TLS close. Quiesce the ClientConnector (join the I/O selector threads) before the lifecycle stop so the cycle cannot form. Verified the cycle is byte-identical through jetty 10.0.26, so a version bump would not fix it. - ZK client close: ZooKeeper 3.7.0's ClientCnxnSocketNIO.cleanup() runs a hardcoded Thread.sleep(100) on the SendThread, so every client close blocks ~100ms in sendThread.join(). That sleep catches InterruptedException, so SolrZooKeeper.close() interrupts the SendThread when a test-only flag is set; the graceful closeSession is still sent (ephemerals release promptly), only the wasted nap is skipped. Wired test-only and client/reader-only (ZkStateReader(String) path, never ZkController) via the solr.zkClientFastCloseForTests property set by SolrTestCase. Client close 105ms -> ~0ms.
…ionConstraints, ZkCLITest Three more :solr:core:test outliers brought in line with the suite (continuing 84a26c4): - TestSolrXml: dropped the per-test initCore()/deleteCore(). Every test in the class only parses a solr.xml string/file via SolrXmlConfig and never touches the harness core, so spinning up (and tearing down) a full SolrCore per test was ~0.5s x ~24 tests of pure overhead. 11.0s -> 0.3s. - TestDocBasedVersionConstraints.testConcurrentAdds: committed once per doc (~75 searcher opens). The per-doc realtime /get already proves the highest version won, so the commit is deferred to a single commit + a final search verification of every doc (same coverage, one searcher open). Full-run 24.0s -> 8.6s; also removes a large per-suite contention footprint (searcher churn) that inflated neighbours under load. - ZkCLITest.testInvalidZKAddress: the bogus hostname "----------:33332" burned ~5s in macOS mDNSResponder/getaddrinfo inside ZooKeeper's SendThread while the main thread blocked in the queued exists() request. Switched to 127.0.0.1:1 (a closed loopback port -> instant ECONNREFUSED) so the same SolrException is raised within the configured client timeout without the DNS stall. 7.1s -> 3.1s.
…udit drain on shutdown Two root causes behind "flaky under load" failures. A full :solr:core:test run that flaked 19 tests drops to 2 with these fixes; the dominant 16 were a single bug. - SolrTestUtil.newStringField could return a NON-stored field even when Store.YES was requested: it delegated to LuceneTestCase.newStringField, whose randomized newField type-tweaking dropped the store flag. Callers index a value then retrieve it via doc.get(name) and assert (e.g. every string-sort method in TestFieldCacheSort), so a dropped Store.YES made retrieval return null. Because SolrTestCase.random() returns a fresh new Random() per call (non-deterministic, ignores tests.seed), a different ~1/4 of methods failed each run, which read as load/contention flakiness -- but it reproduces deterministically per seed/method-order single-JVM (seed 76C17D175E1B9C04 -> 16 TestFieldCacheSort failures). Now honor Store.YES deterministically while preserving the rest of the randomized type. Verified 60/60 green on the failing seed and across fresh runs. - AuditLoggerPlugin.close() lost pending async audit events on shutdown: the runner thread call() loops while(!closed) and is the only consumer of the queue, but close() set closed=true BEFORE calling waitForQueueToDrain -- so the drainer was already stopped and queued events were dropped (an audit-completeness bug, not just a test issue). The drain timeout was also effectively ~250ms, not the documented 30s (the counter was treated as 250ms iterations). Now drain with the runner still live, then stop it, and honor the timeout in real seconds. Fixes testAsyncQueueDrain. - AuditLoggerIntegrationTest: audit events fire asynchronously in HttpSolrCall.runWhenFinished (decoupled from when a request returns), so they have no guaranteed arrival order and a shutdown right after a command can race their delivery. Stop asserting events by arrival position -- assertThreeTestAdminEvents and the CREATE block now match by action/resource via a findByAction helper (matching the existing searchWithException pattern) -- and stop testMuteAdminListCollections from shutting the cluster down before collecting its events. AuditLoggerIntegrationTest: ~1/3 flaky -> 10/10 green.
Run one selected test class N times in parallel with itself to surface flaky / concurrency-sensitive failures. Instead of N sequential test_N subtasks (which Gradle won't run in parallel), generate N uniquely-named subclasses of the target and run them through a single Test task with maxParallelForks workers, so the same test really executes concurrently against itself. Each duplicate is a distinct class, so RandomizedRunner derives a distinct suite seed per iteration and each writes its own results. Interface — two explicit dials: -Ptests.beast.iters=N total iterations (one generated subclass each) -Ptests.beast.parallelism=P how many iterations run concurrently tests.dups and tests.beast.workers are kept as deprecated aliases (the new options default to them and take precedence when both are given). Per-run logs: the duplicates run concurrently, so their live console streams would interleave, and the shared ErrorReportingTestListener only persists a suite's output on failure. The task attaches its own listener that captures EVERY iteration's full output to its own file (one suite == one class == one file, so they never clobber each other): <module>/build/test-results/beast/OUTPUT-<DuplicateClass>.txt Writers open in beforeSuite (so silent runs still leave a file) and close in afterSuite (runs regardless of pass/fail), with a result-summary footer. Always uses the full parallelism, even under -Ptests.verbose: running the iterations in parallel against each other is the whole point, so the verbose single-fork rule is intentionally ignored (interleaved console output is the accepted cost; per-run files remain clean). Clear failures for common mistakes: a missing/typo'd -Ptests.beast.class (instead of Gradle's generic "Task 'beast' not found") and a final/abstract target that cannot be subclassed (instead of a cryptic javac error).
Root cause of the load-dependent "docs expected N but was <N" cloud flakes (CollectionsAPIDistributedZkTest.testReadOnlyCollection, TestLocalStatsCacheCloud, TestExactStatsCacheCloud). It is a production recovery/visibility bug, not a test timing race -- the documented "recovery is sound" assumption does not hold for this path. Chain (verified by injecting TestInjection.failReplicaRequests and tracing recovery): 1. Under load a leader->replica forward errors. The leader applies locally, returns success to the client, and pushes the follower into recovery; the follower is missing a doc. 2. The follower runs PeerSync, correctly finds the missing version and applyUpdates it -- but PeerSync.Updater.applyUpdates applies with IGNORE_AUTOCOMMIT and ends with only proc.finish(): no commit, no openSearcher. 3. PeerSync verifies success via IndexFingerprint.getFingerprint(core, MAX), which reads the *realtime* searcher, so it sees the just-applied (uncommitted) doc -> "sync succeeded". 4. RecoveryStrategy.replay() hits the "No replay needed" path (PeerSync did not buffer) and returns without reopening the main searcher. The replica registers ACTIVE. 5. Ordinary q=*:* queries use the main searcher, which was never reopened, so the recovered doc is invisible to search until the next commit. Without autoSoftCommit (e.g. cloud-minimal) that is effectively permanent: a distributed query landing on the lagging follower returns a low count. Fix: RecoveryStrategy.openSearcherAfterPeerSync(core) does a soft commit (openSearcher) right after replay() in the PeerSync-success branch, so recovered docs are searchable before the replica publishes ACTIVE. PeerSync is NRT-only here (TLOG replicas skip it), so the TLOG-follower RTG path is untouched; the docs are already durable in the tlog so a soft commit suffices. Verified: the same injected beast run went 18/24 FAIL -> 24/24 PASS (21 iterations diverged and all converged). Recovery soundness intact -- TestCloudConsistency, RecoveryZkTest, PeerSyncReplicationTest, PeerSyncWithLeaderTest all green (nightly). Defense-in-depth: add SolrCloudTestCase.waitForAllReplicasDocCount(collection, expected) (+ dumpShardTerms), which queries every replica with distrib=false and waits until all shards' replicas agree and sum to the expected total, failing with a per-replica + shard-term dump on timeout. A distributed *:* query only hits one replica per shard, so it cannot see a follower briefly behind; this removes that timing race too. CollectionsAPIDistributedZkTest.testReadOnlyCollection (3 asserts) and TestBaseStatsCacheCloud.indexDocs now use it instead of bare distributed-count asserts / the old waitForDocCount.
…etup, stats convergence
Three load-dependent flakes surfaced by a full :solr:core:test run (pass in isolation,
fail under full-suite contention). Hardened at the test level; the BasicAuth and stats
issues also point at production gaps tracked separately.
- TestPullReplicaErrorHandling.testPullReplicaDisconnectsFromZooKeeper: the disconnect and
reconnect waits after expireZkSession used the 10s DEFAULT_TIMEOUT 3-arg waitForState. ZK
session-expiry recovery (detect expiry, drop the live node, reconnect, re-register active)
is inherently slow and load-sensitive, so 10s was too short. Use explicit 90s timeouts.
Beast-validated 12/12 at 12-way.
- BasicAuthOnSingleNodeTest.setupCluster: on a freshly-started secured cluster under load the
inter-node (PKI) request signing can be unwired when collection creation fires, so the
CREATE-core sub-requests arrive unauthenticated and are rejected with 401 ("Underlying core
creation failed"). That state is stuck for the affected cluster instance, so a same-cluster
retry does not help -- recreate the whole cluster (up to 8 attempts). Beast-validated 64/64
at 32-way (was 23-62% failing).
- SolrCloudTestCase.waitForAllReplicasDocCount: under diverse full-suite load a follower can
miss forwards during collection-creation/bulk-index with no recovery triggered (clean terms),
so it stays behind its leader and a commit cannot help (the docs are absent, not merely
unsearchable -- a second divergence mechanism distinct from the PeerSync stale-searcher bug).
Add (1) a best-effort commit nudge at 5s for the visibility case and (2) a recovery-drive at
15s: if a follower trails its shard leader, issue CoreAdminRequest.RequestRecovery so it
replicates the missing docs from the leader. Only fires on persistent divergence (no-op on
the common fast-converging path); verified no-regression (stats + collections green, injection
path 24/24).
Remediation of the full review.md finding list (Parts A through H), dispositioned across three waves of file-disjoint work plus a central compile-fix pass. Every finding is either fixed, deemed incorrect, or deemed not-applicable to this fork's architecture. Highlights: - Overseer/StateUpdates: ZkStateWriter tombstone-drop and MODIFYCOLLECTION merge correctness, slice-state NPE guards, dirtyStructure/collLock, CollectionWatch.canBeRemoved counting. - Leader election: LeaderElector await + fresh-executor retry, ShardLeader singleton leak, ZkShardTerms saveTerms/notify, OverseerTaskProcessor getLeaderId seq-sort + runningZKTasks leak fix. - Recovery/update: RecoveryStrategy commit fallback, PeerSyncWithLeader null-add, UpdateLog ACTIVE-before-copy + replay window, DefaultSolrCoreState. - HTTP/2 / SolrJ: Http2 response-buffer pooling (single release), streaming loaders, BaseCloudSolrClient/LBSolrClient, ConnectionManager/SolrZkClient. - tlog/javabin: TransactionLog torn-read, BinaryResponseWriter. - RealTimeGet: clear versionReturned on deleted-doc resolve, range-token AIOOBE guard. - Servlet: SolrDispatchFilter double-write guard, SolrCall checkProps deadline-bounded poll. - Lifecycle/concurrency: ParWork/ParWorkExecutor, CoreContainer/SolrCore shutdown, CachingDirectoryFactory refcount, AuditLoggerPlugin bounded close. Adds ReplicaTest regression coverage for Replica.equals null-id/foreign-type. Build verified GREEN: core + solrj + test-framework compile clean.
…ader lifecycle Single consolidated pass hardening Agrona/direct/off-heap buffer usage: - PooledBufferHandle: AutoCloseable lease over pooled MutableDirectBuffer allocations with idempotent CAS close (release exactly once), use-after-release guard, and optional poison-on-release for tests. - BufferMetrics: centralized accounting (pooled direct allocated/retained, active leased bytes, mmap tlog capacity vs logical size, remap count, active tlog readers, forced-close count, double-release detection); keeps pooled direct, heap Agrona, mmap tlog, Jetty, and ordinary heap byte[] distinct. - TransactionLog: reader-lifecycle accounting (active readers incref/decref, forced-close metric). No record-format or lock-order change. - HTTP/2: SolrHttpRequest.freeBuffer single-release CAS; response/request body buffer ownership tightened across success/failure/cancel/timeout paths. - filestore: ByteBuffersDataOutput.size() returns written position (not capacity); ByteBuffersDataInput.toString() format fix; ByteBuffersDirectory bookkeeping; PackageStore/DistribPackageStore stream-first semantics. - BufferedChannel / pool wrappers: allocation policy + full-write loop + direct-buffer release on close. Tests: PooledBufferHandle, BufferMetrics, BufferedChannel, Http2 lifecycle, ResponseBufferLifecycle, TransactionLog active-readers/format/reader-lifecycle, ByteBuffersDataOutput, ByteBuffersDirectory, PackageStore streaming, plus a property-gated (-Dsolr.bench=true) benchmark harness. All green standard+nightly.
…j, contrib Accumulated fixes from the nightly/Beast failure campaign (clusters 1-9): - Test result-history retention: defaults-tests.gradle snapshots the previous run's TEST-*.xml + outputs/ into timestamped slots, keeping the last N (-Ptests.resulthistory). - SolrCloudBridgeTestCase: reset the static collectionCount in @BeforeClass so the per-method COLLECTION name is deterministic across the JVM-reused forked test classes (fixes hard-coded "collection1" lookups in TestConfigReload, BasicDistributedZkTest, ChaosMonkey suites, etc.). - StallDetection / ZkStateReader register-notify / ZkStateReaderQueue dropped-fetch-under-ConnectionLoss hardening (+ ZkStateReaderQueueTest). - LTR static-similarity leak (TestLTRScoringQuery and related test/setup). - CloudHttp2SolrClientTest preference tests; Http2SolrClient / SolrFutureResponseListener. - CryptoKeys SHA512 package-signature-verification regression. - SolrCores.swap metric-registry async-rename race. - Misc accumulated WIP across Overseer, CoreContainer, SolrCall/HttpSolrCall, DefaultSolrCoreState, UpdateLog, ClusterState, and assorted tests/log configs. Held out of this commit: the doc-99 recovery-leak regression test (TestRecoveryUncommittedDocNotExposed) — kept untracked until the forwarding-window recovery fix lands so test+fix commit together.
…iteria Completes the per-shard append/delta state plane (US-1..US-10): - US-1: extract a pure replace-gate predicate so a freshly-folded delta-plane state at equal structure version is not discarded (LeaderElection leader-loss). - US-2: totally remove the legacy _statupdates node and its vestigial reader/watch/path/version artifacts. - US-3: writer publishes only CHANGED replica entries; leader demotion is computed from snapshot+ring inside StatePlaneWriter rather than from a full collection-state map. - US-4: reader fetches only the changed shard's ring (targeted-shard fetch); structure refresh keeps full-collection catch-up. - US-5: safe ring-trim - fold the committed ring into the snapshot durably before trimming; never advance baseSeq past un-snapshotted deltas. - US-6: seedEpoch() uses overseer.getElectionSeq() with a minimal-test fallback. - US-7: synchronous ConnectionLoss clears the StatePublisher dedup entry so a caller retry of a failed non-LEADER transition is not suppressed in the age window. - US-8: queue durability - delete the overseer queue item only after a durable StatePlaneWriter.publish completes. - US-9: node-down scales O(replicas_on_node) via a nodeName->placement index in ZkStateWriter instead of scanning all collections and all replicas. - US-10: documented, justified deferral of scoping the high-frequency state-plane watch off the global recursive /collections watch (no correctness impact; the per-node filter is an O(1) local containsKey). Adds StatePublisherConnectionLossTest and ZkStateWriterNodeDownIndexTest; extends the StatePlane writer/reader/compaction and TestCollectionStateWatchers suites. solrj + core compile clean; state-plane suites GREEN; LeaderElection beast load gate GREEN.
…tted docs Option B recovery hardening for NRT/TLOG replicas: - RecoveryStrategy no longer forces a hard commit-on-leader before the index fetch, so a recovering follower replicates only the leader's existing latest commit point. This fixes the load-dependent divergence where replication recovery promoted the leader's still-uncommitted docs into a new durable commit point and exposed them to ordinary search (q=*:*) on the follower while the leader still hid them. - The down-window updates are instead caught up by replaying the leader's tlog ahead of the follower's buffer, so they land in the follower's IndexWriter + live tlog and become RTG-visible (/get), kept out of q=*:* by a soft commit with openSearcher=false - mirroring the leader exactly. - Supporting changes across IndexFetcher / ReplicationHandler / DefaultSolrCoreState / SolrIndexWriter / UpdateLog / DirectUpdateHandler2 / CoreContainer for the tlog fetch + replay path. Adds TestRecoveryUncommittedDocNotExposed (no-leak invariant) and TestNrtTlogCatchUpRtgVisible (catch-up invariant); extends TestTlogReplica.
Address the five findings from the StatePlane (delta-plane) review: 1. (BLOCKING) ensureManifestSeeded no longer swallows exceptions. It now throws, so publishToStatePlane fails before appending any delta and the work-queue item is not deleted on seed failure. The manifest is the authoritative reader switch, so a durable-but-unreachable delta can no longer be created. 2. (BLOCKING) enqueueStateUpdates returns a CompletableFuture for the slice-state structure write. WorkQueueWatcher gates queue-item deletion on both the structure-write future and the replica-append future (allOf), so a pure UPDATESHARDSTATE item is not deleted before its structure update is durable. 3. publishToStatePlane maps changed internalId -> shard via DocCollection.getReplicaById + Replica.getSlice (O(1) per changed id) instead of scanning all slices/replicas (O(replicas) per id). 4. Document the broad-notification / shard-scoped-fetch tradeoff on the recursive /collections watch. 5. scheduleRetryOnTransientFailure carries the target shards so a transient failure retries shard-scoped rather than degrading to a full-collection fold. Verified: :solr:core:compileJava + :solr:solrj:compileJava clean; StatePlane unit suites (45 tests) green; SplitShardTest (4) green for the Finding 2 gate; AddReplicaTest (2) green for the Finding 1/3 publish path.
StatePlane (delta-plane) review fixes: - ZkStateWriter.publishToStatePlane: when collection structure is not yet known (cs.get(collection)==null), fail the publish instead of returning silent success — the durability future must not complete before any delta is appended, or WorkQueueWatcher deletes the queue item and the update is lost. writeStateUpdatesInternal re-arms the drained pendingChangedIds on failure so retry replays them (also fixes a latent ensureManifestSeeded loss). - ZkStateWriter.writeStructureUpdates now returns CompletableFuture<Void> completed from the ZK setData callback (state.json durability), not when the async write() merely returns — structure-write durability futures are now actually durable. - StatePlaneWriter.writeSnapshotMonotonic: snapshot writes are CAS/monotonic so a stale writer's lower-upToSeq fold cannot regress a newer writer's higher-coverage snapshot (would open an unreconstructable gap vs ring.baseSeq). compactShard + foldCommittedRingIntoSnapshot route through it. - StatePlaneCompactionTest: 2 regression tests for the monotonic guard (reject stale regression; advance on higher coverage). Nightly/flaky hardening (WIP): ZkTestServer port reservation, leader election, ZkController, recovery, and several cloud tests. ZkTestServer cross-JVM port fix is still incomplete (beast not yet clean).
…comment hygiene Addresses the three "Medium" findings from the stateplane static review (#5 was explicitly an accepted interim limitation, not a defect, and is left as-is). #4 ZkStateReaderQueue: retry budget was keyed by collection only, so one shard's run of transient ZK failures could exhaust or reset the retry budget of the collection's other shards. Introduce retryKey(collection, justStates, targetShards) — full fetch keys by bare collection, a delta apply keys by collection + targeted shard scope (collection::* for a manifest/full-apply event, collection::[sorted-shards] otherwise). All five fetchRetries sites are keyed by it. A successful full fetch clears every scope for the collection (clearCollectionRetries) to bound counter leaks under bulk-message coalescing. #6 StatePublisher.submitState: resolve the replica id from the message's own "id" first and only fall back to a getCollectionOrNull + getReplica scan when the message lacks one. For a registered replica the message id equals replica.getId(), so this is behavior-preserving across all branches (verified message-has-id / no-id x collection present/absent x replica present/absent), and removes the cluster-state scan from the publish hot path. #7 Comment hygiene: reword stale "legacy _statupdates" / "legacy->delta" wording in ZkStateWriter and StatePlaneReader to reflect that there is no _statupdates fallback by design — the manifest is the reader's sole switch onto the delta plane. Verified: solrj + core compile clean; StatePlaneReaderQueueTest 4/4, StatePublisherConnectionLossTest 1/1, ZkStateWriterNodeDownIndexTest 1/1 (0 failures/errors). Reviewed via code-reviewer (no blocking findings).
…id replay, leader/queue hardening Address the external StatePlane delta-plane code review: - P0 #1 lossy publish: StatePublisher.processMessage now persists the state batch durably via the synchronous create(retryOnConnLoss=true) instead of a fire-and-forget async create. On a terminal KeeperException it clears the dedup cache (backstop so the caller's identical retry is not suppressed within the age window) and rethrows so the run loop logs it. Adapt StatePublisherConnectionLossTest to the durable-create path. - P0 #2 seed clobber: StatePlaneWriter seeds snapshot/deltas/manifest with create-only (createIfAbsent, swallowing NodeExists) so an empty baseline can never overwrite a live ring/snapshot. - P1 #3 cursor advances past unapplied ids: StatePlaneCursors gains a per-shard deferred-replay buffer; StatePlaneReader buffers a transition for an id not yet present in local structure and replays it once the id is seeded, maintaining a single-LEADER-per-shard buffer invariant so a handoff cannot resurrect a stale leader. Add replay-on-seed and stale-leader-after-handoff regression tests. - P1 #6 unknown collection id: ZkStateWriter guards the runAsync publish lambda against a null collection mapping, completing normally (queue item deleted) while leaving the id armed for the structure-landing republish. - P2 #7 CORE_NAME_PROP: StatePublisher.submitState reads the message id first and requires only a non-null state plus an id-or-core. - P2 #9 leader reconcile: ZkStateWriter.reconcileLeadersFromZk picks the most recently created leader registration (highest czxid) instead of children[0].
…s import) The rewritten test called bare expectThrows; in this fork expectThrows is not inherited into test classes but lives in org.apache.solr.SolrTestCaseUtil. Add the missing static import. Follow-up to a17f671 (review fixes), which left this test source non-compiling. Verified GREEN: StatePlaneWriterTest (12), StatePlaneReaderTest (12), StatePlaneReaderQueueTest (4), StatePlaneCompactionTest (10), StatePublisherConnectionLossTest (1), ZkStateWriterNodeDownIndexTest (1), all failures=0 errors=0.
P0 #1 StatePublisher durable-publish retention: park + retry batches on ZkCmdExecutor retry-exhaustion (SolrException) instead of logging+dropping. P0 #2 ZkStateWriter unknown collection-id: retain the durable queue item (reprocess) until structure lands / bounded attempts / proven-removed, and republish armed ids from enqueueStructureChange when structure arrives. P1 #3 StatePlaneWriter hot path: writer-local per-shard effective-state cache + O(1) leader-demotion index instead of O(replicas) snapshot rebuild/publish. P1 #4 Scoped state-plane notifications: /collections watch is now non-recursive (add/remove via NodeChildrenChanged reconcile); per-collection recursive watches tied to local interest deliver structure + state-plane leaves, armed on registerCore/registerDocCollectionWatcher, released on interest loss with a self-correcting recheck (no interested-but-unwatched race), re-armed (with catch-up fetch) on reconnect from collectionWatches only. P1 #5 StatePlaneWriter fencing: validate the overseer election min-seq node (ownsElectionAuthoritative) before append, not just !isClosed(). P2 #1 StatePublisher: reject an unresolvable (null) replica id. P2 #2 StatePlaneCursors: FIFO-bound the deferred replay buffer with eviction. P2 #3 Docs: standalone dev-docs/overseer/state-plane.adoc developer note. Tests: StatePlaneCursorsTest (new), TestCollectionStateWatchers concurrency test (new), StatePublisherConnectionLossTest retry test. Core state-plane suite + watcher suite GREEN.
Tighten the stateplane handoff so transient publish pressure and stale overseer ownership fail closed instead of silently losing edge-triggered replica state. Unknown collection IDs now remain retryable until removal is proven, and scoped watch registration keeps intent separate from armed watches so transient addWatch failures are retried. Constraint: New-only state plane design; no _statupdates migration or fallback by design. Rejected: Drop oldest pending publisher batch on overflow | violates strict no-loss semantics for edge-triggered state transitions. Rejected: Bounded unknown-ID attempt cap | can discard legitimate delayed collection structure. Confidence: high Scope-risk: moderate Directive: Keep publisher retry overflow fail-closed and keep authoritative election validation immediately before each stateplane CAS append. Tested: ./gradlew :solr:core:test --tests org.apache.solr.cloud.StatePublisherConnectionLossTest; ./gradlew :solr:core:test --tests org.apache.solr.cloud.StatePlaneWriterTest; ./gradlew :solr:solrj:test --tests org.apache.solr.common.cloud.TestCollectionStateWatchers.testScopedWatchTracksInterestUnderConcurrentChurn -Ptests.nightly=true; ./gradlew :solr:core:compileJava :solr:solrj:compileJava; git diff --check Not-tested: Full Solr test suite; CI unavailable locally.
Bind writer effective-cache reuse to collection incarnation and make publisher overflow retain the terminal batch before failing closed. This keeps retry/demotion decisions from crossing same-name collection recreations and avoids making the overflow guard itself lossy. Constraint: State-plane live state is new-only in this fork; no _statupdates fallback or migration is required. Rejected: Evicting the oldest pending publisher batch | violates no-loss state-transition semantics during ZooKeeper outages. Rejected: Cache invalidation by collection name alone | same-name recreation can restart at the same shard sequence and reuse stale effective state. Confidence: high Scope-risk: moderate Directive: Keep state-plane cache keys tied to collection incarnation or stronger manifest identity before reusing effective shard state. Tested: /home/markmiller/solr-ref-2/gradlew --console plain :solr:core:test --tests org.apache.solr.cloud.StatePlaneWriterTest -> tests=14 failures=0 errors=0 Tested: /home/markmiller/solr-ref-2/gradlew --console plain :solr:core:test --tests org.apache.solr.cloud.StatePublisherConnectionLossTest -> tests=3 failures=0 errors=0 Tested: git diff --check Tested: /home/markmiller/solr-ref-2/gradlew --console plain :solr:core:compileJava :solr:solrj:compileJava Not-tested: full Solr test suite
State-plane durability depends on cross-overseer decisions being reproducible after failover. Persist removed collection-id tombstones in ZooKeeper and make production overseer election validation fail closed when authoritative ownership cannot be proven. Constraint: new-only state plane must not lose or poison replica-state transitions across overseer failover. Rejected: in-memory removed-id tombstones only | a successor overseer cannot distinguish a deleted collection id from delayed structure. Rejected: local-liveness fallback on election read failure | a write fence must not append when ownership cannot be verified. Confidence: high Scope-risk: moderate Directive: Future unknown collection-id drops must be backed by durable incarnation/removal proof, not bounded retry counters. Tested: /home/markmiller/solr-ref-2/gradlew --console plain :solr:core:test --tests "org.apache.solr.cloud.overseer.ZkStateWriterStatePlaneTest" -> tests=8 skipped=0 failures=0 errors=0 Tested: /home/markmiller/solr-ref-2/gradlew --console plain :solr:core:test --tests "org.apache.solr.cloud.StatePlaneWriterTest" -> tests=14 skipped=0 failures=0 errors=0 Tested: /home/markmiller/solr-ref-2/gradlew --console plain :solr:core:test --tests "org.apache.solr.cloud.StatePublisherConnectionLossTest" -> tests=3 skipped=0 failures=0 errors=0 Tested: /home/markmiller/solr-ref-2/gradlew --console plain :solr:core:compileJava :solr:solrj:compileJava -> exit 0 Tested: git diff --check -- state-plane owned files -> exit 0 Not-tested: full Solr test suite and GitHub Actions CI.
Persist removed collection IDs in bounded bucket tombstones before destructive collection cleanup so later overseers can distinguish deleted incarnations from delayed structure without any legacy or fallback path. Constraint: This unreleased fork is state-plane-only; no legacy, compatibility, migration, or _statupdates fallback behavior is allowed. Rejected: Per-ID tombstone znodes | unbounded child growth and legacy-shaped compatibility surface. Rejected: Evicting old tombstones | can reintroduce cross-overseer queue poisoning. Confidence: high Scope-risk: moderate Directive: Keep removed-id tombstones bucket-only and fail closed on persistence, cap, or CAS exhaustion. Tested: ZkStateWriterStatePlaneTest, StatePlaneWriterTest, StatePublisherConnectionLossTest; compileJava for solr:core and solr:solrj; no-legacy grep gate; git diff --check. Not-tested: Full Solr test suite.
Remove legacy and fallback wording from the state-plane developer note so the unreleased fork's intended model is explicit: live replica state is state-plane-only, and a missing manifest means no published live state yet. Constraint: The fork is unreleased and must not carry legacy, compatibility, migration, or _statupdates fallback framing. Rejected: Keeping historical _statupdates wording | suggests a compatibility contract that does not exist. Confidence: high Scope-risk: narrow Directive: Do not reintroduce legacy/back-compat/fallback language for live replica state. Tested: no-legacy grep gate over state-plane docs/code/tests; git diff --check. Not-tested: Documentation-only change, no additional runtime tests.
Constraint: Fork is unreleased and state-plane-only; no legacy live-state path or compatibility framing. Rejected: Reader-side best-effort application of unscoped deltas | durable records must self-identify collection, shard, and incarnation before application. Confidence: high Scope-risk: moderate Directive: Preserve bounded per-shard CAS ring semantics; do not reintroduce append-only or legacy-state wording. Tested: :solr:solrj:compileJava :solr:core:compileJava :solr:core:testClasses; StatePlaneWriterTest, StatePlaneReaderTest, StatePlaneCompactionTest, StatePlaneReaderQueueTest, ZkStateWriterStatePlaneTest; copied XML state-plane suite failures=0 errors=0; git diff --cached --check; forbidden wording scan count=0 Not-tested: Full Solr test suite
State-plane compaction now uses the same authoritative ownership guard as publish before snapshot and trim mutations, and reader full-fetch/session handling avoids redundant delta work and null-path watch crashes. Constraint: state-plane-only fork with no alternate live-state path\nRejected: duplicating the delta fold in full-fetch | fetchCollectionState already applies live state before install\nConfidence: high\nScope-risk: moderate\nDirective: keep state-plane snapshot and ring mutations behind authoritative election checks\nTested: /home/markmiller/solr-ref-2/gradlew --quiet --console plain :solr:core:test --tests org.apache.solr.cloud.StatePlaneCompactionTest; /home/markmiller/solr-ref-2/gradlew --console plain :solr:core:test --tests org.apache.solr.cloud.StatePlaneReaderQueueTest; /home/markmiller/solr-ref-2/gradlew --console plain :solr:solrj:test --tests org.apache.solr.common.cloud.ZkStateReaderQueueTest; /home/markmiller/solr-ref-2/gradlew --console plain :solr:solrj:compileJava :solr:solrj:testClasses :solr:core:compileJava :solr:core:testClasses; git diff --check\nNot-tested: full Solr test suite
Seed and lazy-create writes now use the same authoritative ownership fence as append and compaction writes so stale overseers cannot create bootstrap znodes after losing ownership. Constraint: State plane is the only live-state path in this unreleased fork; no legacy or compatibility path is required. Rejected: Relying on the earlier publish-loop ownership check | ownership can be lost before lazy ring or seed create mutations. Confidence: high Scope-risk: narrow Directive: Keep every state-plane ZK mutation behind an immediate authoritative ownership check. Tested: /home/markmiller/solr-ref-2/gradlew --console plain :solr:solrj:compileJava :solr:solrj:testClasses :solr:core:compileJava :solr:core:testClasses Tested: /home/markmiller/solr-ref-2/gradlew --console plain :solr:core:test --tests org.apache.solr.cloud.StatePlaneWriterTest; XML tests=18 skipped=0 failures=0 errors=0 Not-tested: Full Solr test suite.
…ures Production fixes: - DistributedZkUpdateProcessor: leader keeps forwarding live updates to RECOVERING/RECOVERY_FAILED replicas so their update buffer fills and replays (fixes NRT symmetric divergence where a recovered replica went ACTIVE stale). - IndexFetcher.isIndexStale: treat a missing local file as stale (full copy) instead of throwing NoSuchFileException, which aborted the whole fetch and left empty PULL replicas stuck at version 0. Narrowed to the checksum==null (SimpleText) branch so deletes still propagate. - TransactionLog.LogReader.next: tolerate a torn trailing record (validate the size trailer == bytes consumed; treat a failed read / mismatch as clean EOF) so leader-tlog NRT catch-up no longer fails recovery on a mid-fill snapshot. - RestoreCmd: fix restore empty-replica race. - Leader election / terms hardening (ShardLeaderElectionContext, ZkShardTerms). Test infrastructure: - gradle: archive each run's TEST-*.xml + outputs via a finalizer so the per-run wipe never drops failing results; failures kept forever. - ChaosMonkey: fold the leader check into the random-pick retry loop so landing on the leader retries another node instead of aborting the chaos tick. - JettySolrRunner/PortReservations, MiniSolrCloudCluster, ZkTestServer, and several cloud test adaptations for the fork's StateUpdates/HTTP-2 model.
Validate no-manifest state-plane snapshot and ring znodes before publishing a manifest so partial bootstrap data cannot silently bind to the wrong collection incarnation. Constraint: State plane is the only live-state path in this unreleased fork; no compatibility fallback is required or desired. Rejected: Preserve any existing bootstrap znode blindly | would allow wrong-incarnation data to become visible once the manifest is written. Confidence: high Scope-risk: narrow Directive: Keep bootstrap identity validation aligned with reader-side collection and shard identity checks. Tested: /home/markmiller/solr-ref-2/gradlew :solr:solrj:compileJava :solr:core:test --tests org.apache.solr.cloud.StatePlaneWriterTest; XML tests=19 skipped=0 failures=0 errors=0 Not-tested: Full Solr suite
RecoveryStrategy.dataBearingPeerIsDown: add a live-leader exception so a recovering replica that has already caught up to the currently-live authoritative leader is not held in RECOVERING by a DOWN peer whose shard term does not exceed that leader's. After force-leader all terms equalize, so the prior strict-term skip could not fire and the replica latched in BUFFERING forever (ForceLeaderTest / ForceLeaderWithTlogReplicasTest testReplicasInLowerTerms). Inert for SOLR-9504: when the down peer IS the unreachable leader there is no live leader distinct from it, so the guard stays fully in force (TestCloudConsistency unchanged). RecoveryStrategy.replicaDivergesFromLeader: only compare leader/replica index fingerprints when both were computed over the same maxVersion window. Under the wrongIndexFingerprint test injection the leader serves a fingerprint for maxVersion=1 while we request the whole-index (MAX) window; comparing the two always diverged, latching the replica in RECOVERING (TestCloudSearcherWarming.testPeersyncFailureReplicationSuccess). Inert in production, where the leader always honors the requested window. StatePlaneWriter.publish: a freshly-constructed writer (localEpoch == 0) electing into a ring another overseer already advanced now rebases its epoch cursor forward and proceeds, instead of being fenced. An established writer that adopted an epoch (localEpoch > 0) and finds the ring advanced past it is still fenced as a superseded overseer (TestQueryingOnDownCollection force-down helper; preserves the stale-writer fence in StatePlaneWriterTest). StatePlaneCompactionTest: align the authoritative-ownership fence threshold with the real publish check count on a not-yet-existing ring (pre-loop + lazy-create + pre-CAS append precede the post-append compaction), so the append lands and only the compaction snapshot write is fenced. CoreContainer: seed solrHome from config in the SolrZkClient constructor so ZkCLI bootstrap callers that getSolrHome()/bootstrapConf() without load() do not NPE (ZkCLITest).
Recovery / state-plane / tlog / leader-election correctness fixes (review
findings C1, H1-H6, M1-M7, L1-L4, S1-S3).
Data loss / durability:
- C1 UpdateLog.ensureBuffering(): preserve an existing buffer tlog on a
recovery retry instead of dropping it (the replayer flips state ACTIVE
on an errored replay, so the BUFFERING check was the wrong signal).
Wired into RecoveryStrategy and DefaultSolrCoreState (Locus-1).
- M1 RecoveryStrategy.replay() gates on report.errors on BOTH replay paths;
applyBufferedUpdates keeps the buffer on an errored replay.
- H3 ZkStateWriter.stop() final flush bypasses the local closed-flag fence so
the last buffered state is not dropped on a clean overseer handoff
(ZK-authoritative ownership + isFencedBy still fence a stale writer).
- M2 init() awaits the reconcile-leader publish with bounded retry instead of
fire-and-forget, so a transient failure can't strand the LEADER.
- M3 per-collection durability futures: one collection's publish failure no
longer holds back the queue-item deletes of others in the same batch.
- S3 WorkQueueWatcher records the real future from applyCollectionStateUpdate
so a failed MODIFYCOLLECTION structure write keeps its queue item.
Leader election / terms / forwarding:
- S1 OverseerTaskProcessor.getLeaderId no longer sorts bare-id leader children
with getSeq (threw IllegalStateException -> OVERSEERSTATUS 500); picks the
newest child by czxid during a handoff.
- S2 DistributedZkUpdateProcessor re-excludes RECOVERY_FAILED from forwarding
(still forwards RECOVERING).
- H4 ZkShardTerms.scheduleTermNotify OR-accumulates a pending refresh request
so a coalesced ZK-watch refresh is not dropped.
Watch / refcount symmetry, tlog reads:
- H1 SolrZkClient.removeWatches wraps the watcher symmetrically with addWatch.
- H2 SolrZkClient.mkDirs captures per-create failures atomically (fail-closed).
- H5 DirectMemBufferedInputStream DataInput primitives are bounds-checked
against the logical length (EOFException instead of SIGSEGV / stale slack).
- H6 ZkStateReader.removeDocCollectionWatcher decrements coreRefCount only when
the watcher was present.
- M5 TransactionLog reader ctors roll back incref/active-reader gauge on a
failed open; L3 forceClose drains the gauge for abandoned readers.
- M7 ZkStateReader.releaseLocalInterest evicts the orphaned cached state so an
unwatched collection is not served indefinitely stale.
Smaller correctness / cleanup:
- M4 StatePublisher stops park-retry on shutdown; L1 adds backoff to the
generic catch so a terminal KeeperException doesn't hot-spin.
- M6 removed leftover FLT-INVESTIGATION debug instrumentation.
- L2 corrected an overstated comment; L4 dropped an unreachable guard.
Tests (test-first where a focused test was feasible):
- OverseerTaskProcessorGetLeaderIdTest (S1)
- DirectMemBufferedInputStreamTest (H5)
- TestEnsureBuffering (C1)
Remaining findings are race/timing/shutdown paths verified via the existing
overseer/recovery/tlog suites.
Evicting watchedCollectionStates in releaseLocalInterest regressed TestCloudConsistency (nightly): registerCollectionStateWatcher's immediately-firing registration reads the cached entry synchronously, so eviction surfaced a transient "Last available state: null" to watcher predicates. The method javadoc already documented this invariant; M7 contradicted it. The retained-but-unwatched staleness M7 worried about is bounded by registerStatePlaneWatch's catch-up fetch when interest returns. Baseline (parent) passed 2/0/0; M7 failed 2/2; revert restores 2/0/0.
During SPLITSHARD, a CONSTRUCTION/RECOVERY sub-shard leader receiving a live update forwarded from the parent leader was forwarding it to the parent (active) slice's replicas instead of its own sub-shard replicas, because the routed shardId points at the parent slice that owns the id's hash range during the split. Re-target to cloudDesc.getShardId() so the sub-shard's own replica receives the live update (matches upstream).
releaseLocalInterest tore down the per-collection PERSISTENT_RECURSIVE state-plane watch as soon as the last DocCollectionWatcher was removed, but intentionally retained the cached watchedCollectionStates entry that getClusterState() keeps serving. With the watch gone the cached state froze at its last-applied delta: a consumer polling getClusterState() without its own watcher (e.g. HttpPartitionTest/ForceLeaderTest doing waitForState after an earlier predicate self-removed its watcher) never observed a later transition such as a replica publishing DOWN during a partition, so waitForState(DOWN) timed out. Keep the watch armed while a cached entry is still served; genuine teardown still happens via colectionRemoved (collection deleted cluster-wide), which drops the cached entry and unregisters the watch. Fixes deterministic ForceLeaderTest / ForceLeaderWithTlogReplicasTest nightly failures.
On a reload with a warm searcher, SolrCore.start spun while (initSearcherFuture == null) Thread.sleep(30) waiting for the async initSearcher(prev) task to assign the future. If that task threw (e.g. IndexNotFoundException during reload) the exception was caught and logged and the future stayed null forever, so the reload thread looped indefinitely -- and swallowed InterruptedException, making it uninterruptible, which wedged the whole test JVM (one hung core reload stalled the entire nightly suite for >70 min). Record the failure and have the waiter stop and fail the reload, and restore the interrupt so a genuinely stuck reload can be cancelled.
blockUntilFinished assigned the shared volatile lock field directly and, in finally, counted it down and nulled it. Two concurrent callers (e.g. a safeStop while a commit path also blocks) stomped each other's latch: the first finally counted down whichever latch was installed last (so request() threads awaiting the stomped latch hung), and the second finally NPE'd on a now-null lock (ConcurrentUpdateHttp2SolrClient.java:536, seen via FullThrottleStoppableIndexingThread.safeStop). Count down the latch this call created, and only clear the shared field if it still references our latch. Fixes NullPointerException in ChaosMonkeyNothingIsSafeWithPullReplicasTest.
13bc7f0 populated the sub-shard-leader forward list (nodes) in setupRequest but neither distribution method sent to it: - doDistribAdd (DistributedZkUpdateProcessor) returned at if (!forwardToLeader) before reaching the nodes-forward block, so a sub-shard leader silently dropped every live add to its sub-shard replica -> replica permanently behind the leader. - doVersionDelete (DistributedUpdateProcessor) only forwards via doDistribDeleteById inside the leaderLogic (isLeader) branch, which excludes a sub-shard leader, so deletes were applied locally but never forwarded -> replica keeps a doc the leader deleted (replica ahead). Both fixes are gated on isSubShardLeader, so normal indexing is unaffected. Fixes intermittent ShardSplitTest sub-shard consistency failures (s1_x is not consistent); ShardSplitTest nightly 3/3 green.
The onlyLiveReplica bypass in runLeaderProcess gated its leaderVoteWait on !haveData: a sole live replica that was NOT the highest term skipped the wait and seized leadership as long as it held ANY local data. But holding docs 1..N-1 while missing doc N is exactly the out-of-sync case -- the down leader carries the strictly higher term because it has committed updates this candidate lacks. Letting the behind replica become leader truncates that data when the up-to-date leader returns. Honor leaderVoteWait whenever any other replica holds a strictly higher term, regardless of local data; if none has a higher term (or none returns in time) the candidate still becomes leader (permanently-lost-leader semantics, LeaderVoteWaitTimeoutTest). Fixes intermittent TestCloudConsistency.testOutOfSyncReplicasCannotBecomeLeader*.
armStatePlaneWatch gated both the scoped-watch arm and the catch-up fetch on the statePlaneWatched.add() transition. statePlaneWatched tracks whether the watch is armed in the CURRENT ZK session and is cleared on reconnect, but a session can expire and re-establish before the reconnect handler clears it: a consumer registering interest in that window finds the collection still flagged armed from the dead session, so it skips re-arming the (now-defunct, ZK drops watches on expiry) watch AND the catch-up fetch -> phantom watch, no future leaf events, stale state that never catches up (ZkFailoverTest.testRestartZkWhenClusterDown: a shard leader promotion published during the ZK restart was never folded, leaving that shard stuck DOWN at the client). Always (re)arm (addWatch PERSISTENT_RECURSIVE is idempotent) and always catch-up fetch on registration; this path runs only on interest registration, not per leaf event, so the redundant work is bounded.
A reloaded core shares the old core's SolrCoreState (and its open IndexWriter). reload() built the new core, called start() -- whose async initSearcher opens an NRT searcher bound to the shared state's current writer -- and only THEN called newIndexWriter to pick up the latest config. newIndexWriter -> changeWriter commits+rolls-back the live writer and constructs a second IndexWriter (a second IndexFileDeleter) on the SAME Directory while the just-cached searcher still referenced the rolled-back commit: two deleters race on the shared dataDir and one unlinks a segments/segment file the other expects -> NoSuchFileException, corrupting the dir for every later core/test (intermittent under reload load, e.g. SuggestComponentTest). Reopen the IndexWriter BEFORE start() so exactly one writer/deleter generation owns the dir and the searcher binds to the new latest-config writer (config changes still picked up -- AnalysisAfterCoreReloadTest stays green). Also join the async initSearcher task in doClose() before tearing down searcher/writer/Directory: it opens a reader and mutates the dir's IndexFileDeleter, and container shutdown drains its own executors but not the global ParWork pool this task runs on, so close could otherwise race the in-flight open on the close+recreate reload path. Verified: SuggestComponentTest 3x consecutive nightly green on a clean tree; AnalysisAfterCoreReloadTest green.
…ctionIntegrationTest)
Root cause: ZkStateWriter is a per-node singleton re-init()'d on every overseer
re-election, but its lazily-cached StatePlaneWriter seeds localEpoch=seedEpoch()
(the overseer election sequence) ONLY at construction and is never re-seeded. When
a node loses overseer leadership (e.g. ZK session expiry) another overseer advances
the durable per-shard delta ring to a higher epoch; on re-winning, the stale-epoch
writer is permanently fenced ("ring ... is at epoch N ahead of local writer epoch M;
refusing to append") and can never publish the new shard-leader state -- readers hang
forever in getLeaderRetry until the suite timeout.
init() (the takeover hook) already resets sibling per-node-singleton state (terminated,
workQueue, nodeReplicas) for an analogous prior bug; add the missing reset of
statePlaneWriter/electionFence/manifestEnsured so the next publish rebuilds the writer
at the CURRENT (higher) election sequence -- honoring the file's own "a new overseer
constructs a new writer" contract.
Also two latent leader-election hardening fixes found while tracing, both upstream-Solr-
matching and TestCloudConsistency-verified:
- LeaderElector.checkIfIamLeader: handle exists()==null on the predecessor watch (the
watched node vanished between getChildren and exists) by re-running rather than waiting
forever on a NodeDeleted that will never fire.
- ShardLeaderElectionContext: make the PeerSync-failed empty-replica path honor
leaderVoteWait then lead (symmetric with the success path) instead of a permanent
return false.
Verified: LeaderElectionIntegrationTest 20/20 green (was ~31% hang), TestCloudConsistency
2/2 green.
The test's Watcher.onStateChanged published props + notifyAll() (unblocking a waitForProp the test is sitting in) BEFORE reading selfRemoveOnTrigger for its return value. The test thread, having returned from waitForProp on the value2 notification, then set watcher2.selfRemoveOnTrigger=true for the NEXT (value3) change -- but the in-flight value2 onStateChanged had not yet reached its return, so it read the just-set true and self-removed on the value2 notification instead of value3. watcher2 was dropped before value3's notification was built, so it never observed value3 -> "expected value3 but was value2" at line 231 (~50-67% flaky). Snapshot the self-remove decision at method entry so a flag set concurrently (after the value is observed) only affects the next notification, as intended. Test-only change; production ZkStateReader notification/removal is correct. Verified: CollectionPropsTest 16/16 green (was ~50-67% fail).
…flaky 409) The fork made CSVLoaderBase multi-threaded (MultiThreadCVS worker pool), but every worker thread calls processAdd() on a SINGLE shared UpdateRequestProcessor instance. A DistributedUpdateProcessor/DistributedZkUpdateProcessor is per-request stateful and NOT thread-safe: setupRequest() writes per-doc routing fields (isLeader, forwardToLeader, nodes, clusterState) that versionAdd()/doDistribAdd() then read. Running processAdd() concurrently lets one row clobber another row's isLeader -- an s2-bound doc can observe isLeader=true, run leader-logic, get an s1 leader-clock _version_ stamped into its doc, and forward that to the s2 leader as a TOLEADER update. The s2 leader sees a positive _version_ on a brand-new id (foundVersion=-1) and throws "version conflict ... actual=-1 versionFrom=doc" -> client 409 (BasicDistributedZkTest.testNumberOfCommitsWithCommitAfterAdd, ~33% flaky). Serialize the per-row dispatch on the shared processor so each row's routing+versioning is atomic. Parsing and per-row doc build still run in parallel; the distributed forward is dispatched asynchronously by SolrCmdDistributor, so no network I/O is held under the lock. Only CSVLoaderBase uses the multi-threaded shared-processor pattern; JSON/XML/javabin loaders process serially on the request thread and are unaffected. Verified: BasicDistributedZkTest 12/12 green (was ~33% fail).
…down SuggestComponentTest failed intermittently (~17% isolated, higher under nightly load): a setUp commit threw NoSuchFileException on a segments_N commit point, then every remaining method in the class failed with "this IndexWriter is closed" -- the shared static core was corrupted by a tragic IndexWriter event. Root cause: two IndexWriter/IndexFileDeleter generations operated on the SAME physical dataDir across a close+recreate core generation. doClose() joined initSearcherTask (the ParWork-pool future) but that future resolves as soon as initSearcher(prev) submits warming + registerSearcher to the single-threaded searcherExecutor -- those tasks were still in flight. They re-enter getSearcher/openNewSearcher -> indexReaderFactory.newReader(writer) (an NRT DirectoryReader.open that drives IndexFileDeleter.refresh). doClose then closed the writer while that searcherExecutor IFD activity was still live; when the next generation opened on the same path, the old generation's deferred refresh deleted a segments_N the new generation had just written. lockType=single (a per-Directory SingleInstanceLockFactory) provides no cross-instance lock, so nothing fenced the overlap. Fix: - SolrCore.doClose(): after joining initSearcherTask, also join initSearcherFuture[0] (the last searcherExecutor warming/registration task; firstSearcher listeners queue ahead of it) before the ParWork closer tears down the searcher / updateHandler / IndexWriter. 30s bounded, mirroring the existing initSearcherTask join. - CoreContainer.registerCore(): return the old-core async-close Future (was returning the replaced SolrCore -- no caller consumed it); reload() now joins that Future (30s) before returning, so the old generation's close finishes before the new core serves on the shared dataDir. Complements 26fa7e1 (writer reopen before start() on the shared-state reload path), which is still required -- it prevents two writers on the shared SolrCoreState; this prevents a closing generation's searcherExecutor IFD activity from outliving the close. Verified: SuggestComponentTest 30/30 (agent worktree) + 8/8 (main build) consecutive green under CPU stress, failures=0 errors=0 tests=12; AnalysisAfterCoreReloadTest green.
eb79b85 added a CoreContainer.reload() join that blocked on the old core's closeAndWait (via registerCore returning the close Future) before the reloaded core was allowed to serve. That wait runs while the reload thread holds corestate.reloadLock, and closeAndWait only returns once the old core's refCount reaches 0. Under concurrent write load the writer threads keep the old core's refCount > 0 (getIndexWriter / versionAdd hold refs), so every reload stalls up to the 30s join timeout. TestReloadDeadlock (50 concurrent reloads under 5-15 write threads, @nightly, 7-min suite timeout) blows its timeout -> "Test abandoned because suite timeout was reached" (3 failures). This is a genuine production concern, not a test artifact: it serializes reload-under-write-load. Revert that CoreContainer change. The SolrCore.doClose() join of initSearcherFuture[0] from eb79b85 is kept -- it is bounded, does not contend reload (TestReloadDeadlock 3/3 green with it), and is the correct direction (quiesce searcherExecutor IFD activity before writer teardown). NOTE: SuggestComponentTest is NOT fully fixed by doClose alone (the green 30/30 run required BOTH changes; the CoreContainer half is the deadlock source). It returns to its prior intermittent reload-corruption state and must be re-fixed without serializing reload -- candidate: fix DirectoryFactory caching/refcount so two core generations on one physical dataDir share one Directory+lock instead of racing two IndexFileDeleters under lockType=single.
SuggestComponentTest failed intermittently every nightly (~5/12 methods): after reloadCore() a *:* query returned numFound=0, or a commit threw NoSuchFileException on segments_N -> tragic IndexWriter event -> "this IndexWriter is closed" cascade across the class's shared static core. Root cause is in the test, not production. reloadCore(true) did `SolrCore core = h.getCore()` and never closed it, and ~20 inline `h.getCore().getSolrConfig()...` reads leak a ref each. h.getCore() increments the core's refCount (SolrCores.getCoreFromAnyList -> core.open()), so the old core sat at refCount 3-7 at close. closeAndWait() can never take the clean refCount==0 -> doClose() path; it force-closes via container shutdown with the IndexWriter / IndexFileDeleter / Directory still live. The kept doClose() searcher-join (eb79b85) is bypassed. createCore() then opens a SECOND DirectoryFactory -> second Directory + IndexFileDeleter on the SAME on-disk dataDir; under lockType=single (per-Directory SingleInstanceLockFactory, no cross-instance fence) one generation's deferred IFD refresh deletes a segments_N the other just wrote -> corruption. Fix (test-only, zero production change, cannot affect reload/close locking): release the leaked refs via try-with-resources so the old core reaches refCount 0 and closes cleanly through doClose before createCore() opens the next generation. Adds configVal()/coldSearcher() helpers for the inline reads; drops an unused identity self-check and two unused locals. Instrumented proof: refCount-at-close went from 2-7 (force-abandoned, live writer/IFD/Directory) to 0 (clean, zero force-close events). Verified: SuggestComponentTest 25/25 consecutive green under 4 CPU burners (failures=0 errors=0 tests=12); TestReloadDeadlock 3/3 green (no reload serialization -- change is entirely inside the test class). Note: the failure only reproduces under full-nightly concurrency, so the authoritative check is a full nightly run.
… rare ones
Add an explicit test-policy directive: every failing suite from any run must be
addressed, never dismissed as rare/one-off/"just load" (load is not an excuse —
production load far exceeds the tests). Document the beast task as the tool for
hunting rare flakes (./gradlew beast -Ptests.beast.class/iters/parallelism),
with the instruction to increase iterations+parallelism until a flake surfaces.
Resolve the conflicting DisjunctionMaxQuery note ("remain @ignore'd; do not
re-investigate") to match the policy: adapt the exact-toString assertions to be
order-insensitive or delete them; any other TestExtendedDismaxParser failure is
a real bug to root-cause.
… docs PeerSyncReplicationTest intermittently lost exactly 50 docs (expected:204 but was:154): a killed replica restarts and recovers via PeerSync while 50 docs are indexed concurrently, and the forwarded docs vanish. Root cause: two (sometimes three) RecoveryStrategy instances run concurrently on the same core's UpdateLog. doRecovery's single-flight guard (recoveryRunning) has a hole: it sets the flag then runs the BUFFERING prep-recovery handshake ASYNchronously, submitting the RecoveryTask only later (PrepRecoveryAsyncListener.onSuccess). During that window recoveryStrat is null, so a second doRecovery (checkRecovery from Zk registration racing RecoveringCoreTermWatcher's term-change trigger) calls cancelRecovery() as a no-op and resubmits. A cancelled-but-not-yet-exited strategy blocked in a PeerSync/IndexFetcher network call is still running when the fresh one starts. The two strategies then race ulog.ensureBuffering()/applyBufferedUpdates()/ dropBufferTlog() on the SAME buffer: one drains+drops the buffer holding the leader's in-flight forwarded updates while the other (and the leader's live fan-out to the RECOVERING replica) is still filling it -> those docs are permanently lost. The RecoveryStrategy.replicaDivergesFromLeader fingerprint guard can't catch it -- the corruption happens inside the recovery it guards. Fix: a fair ReentrantLock (recoveryExecLock) held across the whole recoveryStrat.run() in RecoveryTask.run(), so a second RecoveryTask waits until the first strategy has fully completed (buffer drained/dropped cleanly) before it begins. cancelRecovery() does NOT take this lock -- it signals the running strategy via recoveryStrat.close(), honored at isClosed() checkpoints and bounded network timeouts -- so close/shutdown never deadlocks against it and the wait is bounded. Verified by beast: PeerSyncReplicationTest 7/120 failures before -> 120/120 green after; LeaderFailoverAfterPartitionTest 40/40 green. Same family as ChaosMonkeySafeLeaderTest (off-by-one when only 1 doc is in the racing window; can't be beasted directly -- its ctor throws Exception so the generated beast subclass won't compile -- verified via nightly instead).
github-actions
Bot
added
documentation
markrmiller
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Opened in error. Closed.