Increase counter on prepare as well

2 files changed, 16 insertions, 10 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CountingCuratorTransaction.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CountingCuratorTransaction.java
index 629ada46852..de803281a87 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CountingCuratorTransaction.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CountingCuratorTransaction.java
@@ -12,9 +12,10 @@ import com.yahoo.vespa.curator.transaction.TransactionChanges;
  * CuratorTransaction wrapper which increments a counter, to signal invalidation of node repository caches.
  *
  * This class ensures a CuratorTransaction against the cached data (the node repository data) is
- * accompanied by an increment of the data generation counter. This increment must occur <em>after</em>
+ * accompanied by an increment of the data generation counter. An increment must occur <em>after</em>
  * the write has completed, successfully or not. It is therefore placed in a {@code finally} block,
  * wrapping the super class' {@link #commit()}.
+ * Likewise, {@link #prepare()} is also wrapped with an increment, in case it fails due to an inconsistent cache.
  * The cache is invalid whenever the generation counter is higher than what the cache contents were read with.
  * The usual locking for modifications of shared data is then enough to ensure the cache provides a
  * consistent view of the shared data, with one exception: when incrementing the counter fails. This is
@@ -33,8 +34,13 @@ class CountingCuratorTransaction extends CuratorTransaction {
 
     @Override
     public void prepare() {
-        counter.get();
-        super.prepare();
+        try {
+            counter.get();
+            super.prepare();
+        }
+        finally {
+            counter.next();
+        }
     }
 
     @Override
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseTest.java
index 835ac4d0913..149510bdc97 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseTest.java
@@ -39,7 +39,7 @@ public class CuratorDatabaseTest {
         commitCreate("/2", database);
         commitCreate("/1/1", database);
         commitCreate("/2/1", database);
-        assertEquals(4L, (long)curator.counter("/changeCounter").get().get().postValue());
+        assertEquals(8L, (long)curator.counter("/changeCounter").get().get().postValue());
 
         List<String> children1Call1 = database.getChildren(Path.fromString("/1"));
         List<String> children1Call2 = database.getChildren(Path.fromString("/1"));
@@ -63,7 +63,7 @@ public class CuratorDatabaseTest {
         assertArrayEquals(new byte[0], database.getData(Path.fromString("/1")).get());
         commitReadingWrite("/1", "hello".getBytes(), database);
         // Data cached during commit of write transaction. Should be invalid now, and re-read.
-        assertEquals(2L, (long)curator.counter("/changeCounter").get().get().postValue());
+        assertEquals(4L, (long)curator.counter("/changeCounter").get().get().postValue());
         assertArrayEquals("hello".getBytes(), database.getData(Path.fromString("/1")).get());
 
         assertEquals(0, database.getChildren(Path.fromString("/1")).size());
@@ -82,7 +82,7 @@ public class CuratorDatabaseTest {
         commitCreate("/2", database);
         commitCreate("/1/1", database);
         commitCreate("/2/1", database);
-        assertEquals(4L, (long)curator.counter("/changeCounter").get().get().postValue());
+        assertEquals(8L, (long)curator.counter("/changeCounter").get().get().postValue());
 
         List<String> children1Call0 = database.getChildren(Path.fromString("/1")); // prime the db; this call returns a different instance
         List<String> children1Call1 = database.getChildren(Path.fromString("/1"));
@@ -104,16 +104,16 @@ public class CuratorDatabaseTest {
         catch (Exception expected) {
             // expected because the parent does not exist
         }
-        // Counter not increased, since prepare failed.
-        assertEquals(0L, (long)curator.counter("/changeCounter").get().get().postValue());
+        // Counter increased once, since prepare failed.
+        assertEquals(1L, (long)curator.counter("/changeCounter").get().get().postValue());
 
         try {
             commitFailing(database); // fail during commit
             fail("Expected exception");
         }
         catch (Exception expected) { }
-        // Counter increased, since commit failed.
-        assertEquals(1L, (long)curator.counter("/changeCounter").get().get().postValue());
+        // Counter increased, even though commit failed.
+        assertEquals(3L, (long)curator.counter("/changeCounter").get().get().postValue());
     }
 
     private void commitCreate(String path, CuratorDatabase database) {