nick-boss-tech opened a new pull request, #5027: URL: https://github.com/apache/solr/pull/5027
🤖 *AI text below* 🤖 *(posted on behalf of Nick Shanin)* https://issues.apache.org/jira/browse/SOLR-18391 ## What happens today When collection creation fails after the collection state has been written, whether anything is cleaned up depends on the type of the exception. Only an AssignmentException out of replica assignment runs the delete that removes the half-created collection. Any other failure, for example a NullPointerException when the placement strategy reads a cluster state that does not contain the new collection yet, lands in a catch-all that throws a SolrException with a null message, and the collection stays behind with no replicas. The client cannot tell what failed, and the leftover collection blocks a retry under the same name. The two waits in the create (collection visible, replica entries visible) also leave the collection behind when they time out. ## What this change does CreateCollectionCmd now records a stateWritten flag and runs one catch for the whole method. Any exception thrown after the state write deletes the collection before the error returns, so a failed create leaves nothing behind, and cleanup failures are logged at ERROR and suppressed so the client always sees the original error. Unexpected failures return "Could not create collection <name>: <cause>" instead of a null message, and the message for a core that fails to come up now names the per-node failures. In the non-PRS branch the command checks ZooKeeper directly for the collection's state.json before the flag is set, so a collection that already exists fails fast with BAD_REQUEST "collection already exists: <name>" and can never be deleted by a create that did not make it. The alias write at the end of a create goes through a bounded retry (3 attempts, 200 ms pause, only ZooKeeperException is retried; interrupts are not retried), and the cleanup above runs only when the retries are exhausted. The placement strategy guard from #4997, an AssignmentException naming the missing collection instead of an NPE, is kept and extended to the system-collection branch, so the other callers of the strategy get the clearer error too. The non-PRS cluster state re-read now fills in the waited-for collection when the re-read does not contain it yet, which closes the stale-read window the ticket hit. This is the broader route suggested on #4997; that PR stays as a draft and this branch supersedes it. ## Proof The new cleanup tests are real-cluster tests with failure injection through a delegating placement plugin factory; the retry helper is covered by unit tests. With the production code at the base (c3cdf7b46e8) and the tests from this branch, exactly two tests fail, in the ticket's shapes: testCleanupAfterUnexpectedPlacementFailure (the client gets a SolrException with an empty message) and testAssignForMissingCollection (a NullPointerException instead of an AssignmentException). The guard tests pass on base. Verified 2026-10-05 at fca4235be87 (local gate): CreateCollectionCleanupTest 4/4, PlacementPluginIntegrationTest 8/8, OverseerCollectionConfigSetProcessorTest 26/26, DeleteCoreRemnantsOnCreateTest 5/5, CreateCollectionCmdRetryTest 4/4, and :solr:core:check -x test green, with tidy and the Error Prone compile clean. Fork CI runs of CreateCollectionCleanupTest and CreateCollectionCmdRetryTest at the same head are also green. The only commit after that head removes an internal handoff document. ## Choices to check 1. **Alias failure deletes the collection.** If the alias write still fails after the bounded retry, the cleanup deletes a collection that was created successfully and is otherwise working, because the create as a whole is reported as failed. The alternative is to bound the cleanup at activation, so a failure that late leaves the collection in place and only the alias step is reported. We kept the delete: a create that reports failure should not leave behind a collection the client believes was never created. If you prefer the activation bound, we are willing to make that change in this PR. 2. **The exists guard sits in the non-PRS branch only.** The deletion risk it closes does not exist in the PRS branch, where the state write throws before the flag is set, so PRS was left as it was. Two inconsistencies remain there: a pre-existing collection missed by the up-front check fails with a 500 carrying the NodeExistsException text instead of the new 400, and a lost reply on the PRS state write can leave a full state.json with no replicas. Moving the guard above the PRS branch split closes both. We shipped it as built to keep this change small; we are willing to do the move in this PR if you prefer the branches to behave the same. 3. **How the claims are worded.** The cleanup catch covers Exception, not Error, so this description says "any exception", and an Error still skips the cleanup, as on main. Two timeout messages in the create are unchanged and do not name what timed out. We worded the claims to match the code rather than widen the change; say the word if either belongs in this PR. ## Limits - An interrupt or shutdown mid-create leaves the collection in place; the cleanup does not run on an interrupted thread. - A failed cleanup leaves the collection, logged at ERROR. A cleanup that fails inside DeleteCollectionCmd can leave cores without a collection, because that command still wipes the ZooKeeper tree in its finally block; that matches how a user-issued delete behaves today. - Failures before the state write can leave an empty /collections node and an auto-created configset copy, as on main. - Both create timeouts (collection not visible in 30 s, replica entries not visible in 120 s) now delete the collection, and a failing create waits for that delete before returning, which can add up to 60 s to a failure that used to return at once. - The stale re-read window is closed by construction (the state is still re-read, with the waited collection filled in when the re-read lacks it) and has no dedicated test; the ZkStateReader window itself is not changed. - CreateShardCmd and SplitShardCmd do not get the same treatment. Happy to open a follow-up ticket and PR for them on request. Changelog: `changelog/unreleased/SOLR-18391-collection-creation-failure-cleanup.yml` ### AI assistance AI agents assisted with research, implementation, review, and drafting. Nick Shanin directed the work and takes responsibility for this contribution. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
