diff --git a/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java b/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java index f3bf6771296..d9e3ff74529 100644 --- a/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java +++ b/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java @@ -1043,7 +1043,7 @@ public class InternalEngine extends Engine { currentVersion = versionValue.version; currentNotFoundOrDeleted = versionValue.isDelete(); } - if (index.getIfSeqNo() != SequenceNumbers.UNASSIGNED_SEQ_NO && versionValue == null) { + if (index.getIfSeqNo() != SequenceNumbers.UNASSIGNED_SEQ_NO && currentNotFoundOrDeleted) { final VersionConflictEngineException e = new VersionConflictEngineException(shardId, index.id(), index.getIfSeqNo(), index.getIfPrimaryTerm(), SequenceNumbers.UNASSIGNED_SEQ_NO, SequenceNumbers.UNASSIGNED_PRIMARY_TERM); @@ -1376,7 +1376,7 @@ public class InternalEngine extends Engine { currentlyDeleted = versionValue.isDelete(); } final DeletionStrategy plan; - if (delete.getIfSeqNo() != SequenceNumbers.UNASSIGNED_SEQ_NO && versionValue == null) { + if (delete.getIfSeqNo() != SequenceNumbers.UNASSIGNED_SEQ_NO && currentlyDeleted) { final VersionConflictEngineException e = new VersionConflictEngineException(shardId, delete.id(), delete.getIfSeqNo(), delete.getIfPrimaryTerm(), SequenceNumbers.UNASSIGNED_SEQ_NO, SequenceNumbers.UNASSIGNED_PRIMARY_TERM); plan = DeletionStrategy.skipDueToVersionConflict(e, currentVersion, true); diff --git a/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java b/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java index 5f57bdbb708..49d93d3da0f 100644 --- a/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java +++ b/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java @@ -143,6 +143,7 @@ import org.elasticsearch.test.IndexSettingsModule; import org.elasticsearch.test.VersionUtils; import org.elasticsearch.threadpool.ThreadPool; import org.hamcrest.MatcherAssert; +import org.hamcrest.Matchers; import java.io.Closeable; import java.io.IOException; @@ -1979,7 +1980,7 @@ public class InternalEngineTests extends EngineTestCase { currentTerm.set(currentTerm.get() + 1L); engine.rollTranslogGeneration(); } - final long correctVersion = docDeleted && randomBoolean() ? Versions.MATCH_DELETED : lastOpVersion; + final long correctVersion = docDeleted ? Versions.MATCH_DELETED : lastOpVersion; logger.info("performing [{}]{}{}", op.operationType().name().charAt(0), versionConflict ? " (conflict " + conflictingVersion + ")" : "", @@ -2002,7 +2003,7 @@ public class InternalEngineTests extends EngineTestCase { final Engine.IndexResult result; if (versionedOp) { // TODO: add support for non-existing docs - if (randomBoolean() && lastOpSeqNo != SequenceNumbers.UNASSIGNED_SEQ_NO) { + if (randomBoolean() && lastOpSeqNo != SequenceNumbers.UNASSIGNED_SEQ_NO && docDeleted == false) { result = engine.index(indexWithSeq.apply(lastOpSeqNo, lastOpTerm, index)); } else { result = engine.index(indexWithVersion.apply(correctVersion, index)); @@ -2037,8 +2038,9 @@ public class InternalEngineTests extends EngineTestCase { assertThat(result.getFailure(), instanceOf(VersionConflictEngineException.class)); } else { final Engine.DeleteResult result; + long correctSeqNo = docDeleted ? UNASSIGNED_SEQ_NO : lastOpSeqNo; if (versionedOp && lastOpSeqNo != UNASSIGNED_SEQ_NO && randomBoolean()) { - result = engine.delete(delWithSeq.apply(lastOpSeqNo, lastOpTerm, delete)); + result = engine.delete(delWithSeq.apply(correctSeqNo, lastOpTerm, delete)); } else if (versionedOp) { result = engine.delete(delWithVersion.apply(correctVersion, delete)); } else { @@ -4312,6 +4314,36 @@ public class InternalEngineTests extends EngineTestCase { } } + /** + * Test that we do not leak out information on a deleted doc due to it existing in version map. There are at least 2 cases: + * + */ + public void testVersionConflictIgnoreDeletedDoc() throws IOException { + ParsedDocument doc = testParsedDocument("1", null, testDocument(), + new BytesArray("{}".getBytes(Charset.defaultCharset())), null); + engine.delete(new Engine.Delete("test", "1", newUid("1"), 1)); + for (long seqNo : new long[]{0, 1, randomNonNegativeLong()}) { + assertDeletedVersionConflict(engine.index(new Engine.Index(newUid("1"), doc, UNASSIGNED_SEQ_NO, 1, + Versions.MATCH_ANY, VersionType.INTERNAL, + PRIMARY, randomNonNegativeLong(), IndexRequest.UNSET_AUTO_GENERATED_TIMESTAMP, false, seqNo, 1)), + "update: " + seqNo); + + assertDeletedVersionConflict(engine.delete(new Engine.Delete("test", "1", newUid("1"), UNASSIGNED_SEQ_NO, 1, + Versions.MATCH_ANY, VersionType.INTERNAL, PRIMARY, randomNonNegativeLong(), seqNo, 1)), + "delete: " + seqNo); + } + } + + private void assertDeletedVersionConflict(Engine.Result result, String operation) { + assertNotNull("Must have failure for " + operation, result.getFailure()); + assertThat(operation, result.getFailure(), Matchers.instanceOf(VersionConflictEngineException.class)); + VersionConflictEngineException exception = (VersionConflictEngineException) result.getFailure(); + assertThat(operation, exception.getMessage(), containsString("but no document was found")); + } + /* * This test tests that a no-op does not generate a new sequence number, that no-ops can advance the local checkpoint, and that no-ops * are correctly added to the translog. diff --git a/server/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java b/server/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java index 8f43990ad6a..0d5b23f1c6c 100644 --- a/server/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java +++ b/server/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java @@ -21,13 +21,13 @@ package org.elasticsearch.indices.settings; import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse; import org.elasticsearch.action.admin.indices.settings.get.GetSettingsResponse; -import org.elasticsearch.action.delete.DeleteResponse; import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.common.Priority; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.IndexModule; import org.elasticsearch.index.IndexService; +import org.elasticsearch.index.VersionType; import org.elasticsearch.index.engine.VersionConflictEngineException; import org.elasticsearch.indices.IndicesService; import org.elasticsearch.plugins.Plugin; @@ -449,16 +449,14 @@ public class UpdateSettingsIT extends ESIntegTestCase { public void testEngineGCDeletesSetting() throws Exception { createIndex("test"); - client().prepareIndex("test", "type", "1").setSource("f", 1).get(); - DeleteResponse response = client().prepareDelete("test", "type", "1").get(); - long seqNo = response.getSeqNo(); - long primaryTerm = response.getPrimaryTerm(); - // delete is still in cache this should work - client().prepareIndex("test", "type", "1").setSource("f", 2).setIfSeqNo(seqNo).setIfPrimaryTerm(primaryTerm).get(); + client().prepareIndex("test", "type", "1").setSource("f", 1).setVersionType(VersionType.EXTERNAL).setVersion(1).get(); + client().prepareDelete("test", "type", "1").setVersionType(VersionType.EXTERNAL).setVersion(2).get(); + // delete is still in cache this should fail + assertThrows(client().prepareIndex("test", "type", "1").setSource("f", 3).setVersionType(VersionType.EXTERNAL).setVersion(1), + VersionConflictEngineException.class); assertAcked(client().admin().indices().prepareUpdateSettings("test").setSettings(Settings.builder().put("index.gc_deletes", 0))); - response = client().prepareDelete("test", "type", "1").get(); - seqNo = response.getSeqNo(); + client().prepareDelete("test", "type", "1").setVersionType(VersionType.EXTERNAL).setVersion(4).get(); // Make sure the time has advanced for InternalEngine#resolveDocVersion() for (ThreadPool threadPool : internalCluster().getInstances(ThreadPool.class)) { @@ -466,9 +464,8 @@ public class UpdateSettingsIT extends ESIntegTestCase { assertBusy(() -> assertThat(threadPool.relativeTimeInMillis(), greaterThan(startTime))); } - // delete is should not be in cache - assertThrows(client().prepareIndex("test", "type", "1").setSource("f", 3).setIfSeqNo(seqNo).setIfPrimaryTerm(primaryTerm), - VersionConflictEngineException.class); + // delete should not be in cache + client().prepareIndex("test", "type", "1").setSource("f", 2).setVersionType(VersionType.EXTERNAL).setVersion(1); } public void testUpdateSettingsWithBlocks() { diff --git a/server/src/test/java/org/elasticsearch/versioning/SimpleVersioningIT.java b/server/src/test/java/org/elasticsearch/versioning/SimpleVersioningIT.java index f42857216f8..caafeaecd42 100644 --- a/server/src/test/java/org/elasticsearch/versioning/SimpleVersioningIT.java +++ b/server/src/test/java/org/elasticsearch/versioning/SimpleVersioningIT.java @@ -283,13 +283,8 @@ public class SimpleVersioningIT extends ESIntegTestCase { assertThrows(client().prepareDelete("test", "type", "1").setIfSeqNo(3).setIfPrimaryTerm(12), VersionConflictEngineException.class); assertThrows(client().prepareDelete("test", "type", "1").setIfSeqNo(1).setIfPrimaryTerm(2), VersionConflictEngineException.class); - - // This is intricate - the object was deleted but a delete transaction was with the right version. We add another one - // and thus the transaction is increased. - deleteResponse = client().prepareDelete("test", "type", "1").setIfSeqNo(2).setIfPrimaryTerm(1).get(); - assertEquals(DocWriteResponse.Result.NOT_FOUND, deleteResponse.getResult()); - assertThat(deleteResponse.getSeqNo(), equalTo(3L)); - assertThat(deleteResponse.getPrimaryTerm(), equalTo(1L)); + // the doc is deleted. Even when we hit the deleted seqNo, a conditional delete should fail. + assertThrows(client().prepareDelete("test", "type", "1").setIfSeqNo(2).setIfPrimaryTerm(1), VersionConflictEngineException.class); } public void testSimpleVersioningWithFlush() throws Exception {