Merge branch 'master' into fix/933-concurrent-modification-exception

Author: mPokornyETM

src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java

@@ -1178,6 +1178,11 @@ public void reset(List<LockableResource> resources) {
                 uncacheIfFreeing(r, true, true);
                 r.reset();
             }
+
+            while (proceedNextContext()) {
+                // process as many contexts as possible
+            }
+
             save();
         }
         scheduleQueueMaintenance();

src/test/java/org/jenkins/plugins/lockableresources/LockStepReserveInsideLockHonouredTest.java

@@ -4,6 +4,7 @@
 import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
 import org.jenkinsci.plugins.workflow.job.WorkflowJob;
 import org.jenkinsci.plugins.workflow.job.WorkflowRun;
+import org.jenkinsci.plugins.workflow.test.steps.SemaphoreStep;
 import org.junit.jupiter.api.Test;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.junit.jupiter.WithJenkins;
@@ -39,7 +40,6 @@ void reserveInsideLockHonoured(JenkinsRule j) throws Exception {
                         + "    def res = "
                         + lmget
                         + ".reserve([lr], 'test2a')\n"
-                        // + "    semaphore 'wait-inside'\n"
                         + "    echo \"Locked resource cause 1-2a: ${lr.getLockCause()}\"\n"
                         + "    echo \"Locked resource reservedBy 1-2a: ${lr.getReservedBy()}\"\n"
                         + "    if (!res) {\n"
@@ -49,34 +49,27 @@ void reserveInsideLockHonoured(JenkinsRule j) throws Exception {
                         + "        echo \"Locked resource reservedBy 1-2b: ${lr.getReservedBy()}\"\n"
                         + "    }\n"
                         + "    echo \"Unlocking parallel closure 1\"\n"
+                        + "    semaphore 'p1-inside'\n" // Java controls when p1 exits lock
                         + "  }\n"
                         + "  echo \"Locked resource cause 1-3 (after unlock): ${lr.getLockCause()}\"\n"
                         + "  echo \"Locked resource reservedBy 1-3: ${lr.getReservedBy()}\"\n"
-                        + "  echo \"Ended locked parallel closure 1 with resource reserved, sleeping...\"\n"
-                        + "  sleep (5)\n"
+                        + "  echo \"Ended locked parallel closure 1 with resource reserved\"\n"
                         + "  echo \"Locked resource cause 1-4: ${lr.getLockCause()}\"\n"
                         + "  echo \"Locked resource reservedBy 1-4: ${lr.getReservedBy()}\"\n"
-                        + "  echo \"Resetting Locked resource via LRM and sleeping ...\"\n"
+                        + "  semaphore 'p1-before-reset'\n" // Java asserts p2 hasn't started before reset
+                        + "  echo \"Resetting Locked resource via LRM\"\n"
                         + "  "
                         + lmget
                         + ".reset([lr])\n"
-                        + "  sleep (5)\n"
-                        + "  echo \"Un-reserving Locked resource via LRM and sleeping...\"\n"
+                        + "  echo \"Un-reserving Locked resource via LRM\"\n"
                         + "  "
                         + lmget
                         + ".unreserve([lr])\n"
-                        + "  sleep (5)\n"
-                        // Note: the unlock attempt here might steal this resource
-                        // from another parallel stage, so we don't do it:
-                        // + "  echo \"Un-locking Locked resource via LRM and sleeping...\"\n"
-                        // + "  " + lmget + ".unlock([lr], null)\n"
-                        // + "  sleep (5)\n"
+                        + "  semaphore 'p1-after-reset'\n" // Java ensures p2 started before p1 logs 1-5
                         + "  echo \"Locked resource cause 1-5: ${lr.getLockCause()}\"\n"
                         + "  echo \"Locked resource reservedBy 1-5: ${lr.getReservedBy()}\"\n"
-                        + "  sleep (5)\n"
                         + "  if (lr.getLockCause() == null) {\n"
                         + "    echo \"LRM seems stuck; trying to reserve/unreserve this resource by LRM methods\"\n"
-                        // + "    lock(label: 'label1') { echo \"Secondary lock trick\" }\n"
                         + "    if ("
                         + lmget
                         + ".reserve([lr], 'unstucker')) {\n"
@@ -86,76 +79,71 @@ void reserveInsideLockHonoured(JenkinsRule j) throws Exception {
                         + ".unreserve([lr])\n"
                         + "    } else { echo \"Could not reserve by LRM methods as 'unstucker'\" }\n"
                         + "  }\n"
-                        + "  sleep (5)\n"
                         + "  echo \"Locked resource cause 1-6: ${lr.getLockCause()}\"\n"
                         + "  echo \"Locked resource reservedBy 1-6: ${lr.getReservedBy()}\"\n"
                         + "},\n"
                         + "p2: {\n"
-                        // + "  semaphore 'wait-outside'\n"
                         + "  org.jenkins.plugins.lockableresources.LockableResource lr = null\n"
                         + "  echo \"Locked resource cause 2-1: not locked yet\"\n"
                         + "  lock(label: 'label1', variable: 'someVar2') {\n"
                         + "    echo \"VAR2 IS $env.someVar2\"\n"
                         + "    lr = "
                         + lmget
                         + ".fromName(env.someVar2)\n"
-                        + "    sleep (1)\n"
                         + "    echo \"Locked resource cause 2-2: ${lr.getLockCause()}\"\n"
                         + "    echo \"Locked resource reservedBy 2-2: ${lr.getReservedBy()}\"\n"
                         + "    echo \"Setting (directly) and dropping (via LRM) a reservation on locked resource\"\n"
                         + "    lr.reserve('test2-1')\n"
-                        + "    sleep (3)\n"
                         + "    "
                         + lmget
                         + ".unreserve([lr])\n"
-                        + "    echo \"Just sleeping...\"\n"
-                        + "    sleep (20)\n"
+                        + "    semaphore 'p2-holding'\n" // Java controls when p2 sets test2-2 and exits
                         + "    echo \"Setting (directly) a reservation on locked resource\"\n"
                         + "    lr.reserve('test2-2')\n"
                         + "    echo \"Unlocking parallel closure 2\"\n"
                         + "  }\n"
                         + "  echo \"Locked resource cause 2-3: ${lr.getLockCause()}\"\n"
                         + "  echo \"Locked resource reservedBy 2-3: ${lr.getReservedBy()}\"\n"
-                        + "  sleep (5)\n"
                         + "  echo \"Recycling (via LRM) the reserved not-locked resource\"\n"
+                        + "  semaphore 'p2-before-recycle'\n" // Java asserts p3 hasn't got the lock before recycle
                         + "  "
                         + lmget
                         + ".recycle([lr])\n"
-                        + "  sleep (5)\n"
+                        + "  semaphore 'p2-after-recycle'\n" // Java ensures p3 started before p2 logs 2-4
                         + "  echo \"Locked resource cause 2-4: ${lr.getLockCause()}\"\n"
                         + "  echo \"Locked resource reservedBy 2-4: ${lr.getReservedBy()}\"\n"
                         + "},\n"
                         // Test that reserve/unreserve in p2 did not "allow" p3 to kidnap the lock:
                         + "p3: {\n"
                         + "  org.jenkins.plugins.lockableresources.LockableResource lr = null\n"
                         + "  echo \"Locked resource cause 3-1: not locked yet\"\n"
-                        + "  sleep 1\n"
+                        + "  semaphore 'p3-wait'\n" // Java controls when p3 enters the queue
                         + "  lock(label: 'label1', variable: 'someVar3') {\n"
                         + "    echo \"VAR3 IS $env.someVar3\"\n"
                         + "    lr = "
                         + lmget
                         + ".fromName(env.someVar3)\n"
                         + "    echo \"Locked resource cause 3-2: ${lr.getLockCause()}\"\n"
                         + "    echo \"Locked resource reservedBy 3-2: ${lr.getReservedBy()}\"\n"
-                        + "    echo \"Just sleeping...\"\n"
-                        + "    sleep (10)\n"
                         + "    echo \"Unlocking parallel closure 3\"\n"
+                        + "    semaphore 'p3-in-lock'\n" // Java controls when p3 exits lock
                         + "  }\n"
                         + "  echo \"Locked resource cause 3-3: ${lr.getLockCause()}\"\n"
                         + "  echo \"Locked resource reservedBy 3-3: ${lr.getReservedBy()}\"\n"
                         + "},\n"
                         // Add some pressure to try for race conditions:
-                        + "p4: { sleep 2; lock(label: 'label1') { sleep 1 } },\n"
-                        + "p5: { sleep 2; lock(label: 'label1') { sleep 3 } },\n"
-                        + "p6: { sleep 2; lock(label: 'label1') { sleep 2 } },\n"
-                        + "p7: { sleep 2; lock(label: 'label1') { sleep 1 } },\n"
-                        + "p8: { sleep 2; lock(label: 'label1') { sleep 2 } },\n"
-                        + "p9: { sleep 2; lock(label: 'label1') { sleep 1 } }\n"
+                        + "p4: { semaphore 'p4'; lock(label: 'label1') { } },\n"
+                        + "p5: { semaphore 'p5'; lock(label: 'label1') { } },\n"
+                        + "p6: { semaphore 'p6'; lock(label: 'label1') { } },\n"
+                        + "p7: { semaphore 'p7'; lock(label: 'label1') { } },\n"
+                        + "p8: { semaphore 'p8'; lock(label: 'label1') { } },\n"
+                        + "p9: { semaphore 'p9'; lock(label: 'label1') { } }\n"
                         + "\necho \"Survived the test\"\n"
                         + "}", // timeout wrapper
                 false));
         WorkflowRun b1 = p.scheduleBuild2(0).waitForStart();
 
+        // --- Phase 1: p1 holds lock exclusively (p2,p3 blocked) ---
         j.waitForMessage("Locked resource cause 1-1", b1);
         j.assertLogNotContains("Locked resource cause 2-2", b1);
         j.assertLogNotContains("Locked resource cause 2-3", b1);
@@ -164,99 +152,86 @@ void reserveInsideLockHonoured(JenkinsRule j) throws Exception {
         j.assertLogNotContains("Locked resource cause 2-2", b1);
         j.assertLogNotContains("Locked resource cause 2-3", b1);
 
+        // Let p1 exit lock closure
+        SemaphoreStep.success("p1-inside/1", null);
+
+        // --- Phase 2: p1 released lock but resource is still reserved ---
         j.waitForMessage("Locked resource cause 1-3", b1);
         j.assertLogNotContains("Locked resource cause 2-2", b1);
         j.assertLogNotContains("Locked resource cause 2-3", b1);
 
-        // Bug #1 happens here (without further patch):
-        // although resource is seen as reserved, it is
-        // grabbed anyway by the other parallel thread
-        // which is already waiting. Notably, log is like:
-        //  62.908 [setReservedByInsideLockHonoured #1] Lock acquired on [Label: label1]
-        //  62.909 [setReservedByInsideLockHonoured #1] Lock released on resource [Label: label1]
-        // and the consistent ordering of acquired first,
-        // released later is unsettling.
+        // Bug #1: although resource is reserved, p2 must NOT grab it.
         j.waitForMessage("Locked resource cause 1-4", b1);
-        // Note: stage in test has a sleep(1) to reduce chances that
-        // this line is noticed in log although it is there AFTER 1-4:
         j.assertLogNotContains("Locked resource cause 2-2", b1);
         j.assertLogNotContains("Locked resource cause 2-3", b1);
         LOGGER.info("GOOD: Did not encounter Bug #1 " + "(parallel p2 gets the lock on a still-reserved resource)!");
 
+        // --- Phase 3: p1 calls reset() → p2 gets lock via proceedNextContext ---
+        // Ensure p2 is in the waiting queue before p1 calls reset()
+        j.waitForMessage("[Label: label1] is not free, waiting for execution ...", b1);
+
+        // p1 calls reset+unreserve, then blocks at semaphore p1-after-reset.
+        // p2 gets the lock (dispatched by proceedNextContext), enters closure,
+        // does reserve/unreserve cycle, then blocks at semaphore p2-holding.
+        SemaphoreStep.success("p1-before-reset/1", null);
+        SemaphoreStep.waitForStart("p2-holding/1", b1);
+
+        // Release p3 and pressure stages into the lock queue while p2 holds the lock
+        SemaphoreStep.success("p3-wait/1", null);
+        for (String s : new String[] {"p4", "p5", "p6", "p7", "p8", "p9"}) {
+            SemaphoreStep.success(s + "/1", null);
+        }
+
+        // Now let p1 continue to log 1-5 (p2 already holds the lock)
+        SemaphoreStep.success("p1-after-reset/1", null);
+
         j.waitForMessage("Locked resource cause 1-5", b1);
-        // This line might not strictly be required,
-        // but we are processing a parallel pipeline
-        // and many seconds were spent sleeping, so:
         j.assertLogContains("Locked resource cause 2-1", b1);
-        // Here the other parallel stage may have already started
-        // (we try to recycle the resource between 1-4 and 1-5):
-        // j.assertLogNotContains("Locked resource cause 2-2", b1);
-        // j.assertLogNotContains("Locked resource cause 2-3", b1);
+        j.assertLogContains("Locked resource cause 2-2", b1);
+        LOGGER.info("GOOD: Parallel 2 started after Parallel 1 reset the resource!");
 
-        // Bug #2 happens here: even after the resource is known
-        // to be un-reserved, resources already looping waiting
-        // for it (after the fix for Bug #1) are not "notified".
-        // Adding and removing the resource helps unblock this.
-        boolean sawBug2a = false;
-        try {
-            j.waitForMessage("Locked resource cause 1-6", b1);
-            j.assertLogContains("Locked resource cause 2-2", b1);
-        } catch (java.lang.AssertionError t1) {
-            sawBug2a = true;
-            LOGGER.info("Bug #2a (Parallel 2 did not start after Parallel 1 finished "
-                    + "and resource later released) currently tolerated");
-            // LOGGER.info(t1.toString());
-            // throw t1;
-        }
-        if (!sawBug2a) {
-            LOGGER.info("GOOD: Did not encounter Bug #2a "
-                    + "(Parallel 2 did not start after Parallel 1 finished "
-                    + "and resource later released)!");
-        }
+        // p2 holds the lock, reservation cycle (test2-1 → unreserve) already done
+        j.assertLogContains("Locked resource reservedBy 1-5: null", b1);
 
-        // If the bug is resolved, then by the time we get to 1-5
-        // the resource should be taken by the other parallel stage
-        // and so not locked by not-"null"; reservation should be away though
-        // Note: Due to parallel execution timing, p2 may have already called
-        // reserve('test2-1') before p1 logs point 1-5, so we tolerate both states
-        boolean sawBug2b = false;
-        try {
-            j.assertLogContains("Locked resource reservedBy 1-5: null", b1);
-        } catch (java.lang.AssertionError t) {
-            // p2 set a reservation before p1 logged 1-5 - this is expected timing variation
-            j.assertLogContains("Locked resource reservedBy 1-5: test2-1", b1);
-            LOGGER.info("Timing variation: p2 reserved resource before p1 logged 1-5 state");
-        }
+        // The resource is locked by p2 at point 1-5, so "cause 1-5: null" and the
+        // un-stucking workaround should never appear.
         for (String line : new String[] {
             "Locked resource cause 1-5: null", "LRM seems stuck; trying to reserve/unreserve", "Secondary lock trick"
         }) {
-            try {
-                j.assertLogNotContains(line, b1);
-            } catch (java.lang.AssertionError t2) {
-                sawBug2b = true;
-                LOGGER.info("Bug #2b (LRM required un-stucking) currently tolerated: " + line);
-                // LOGGER.info(t2.toString());
-                // throw t2;
-            }
-        }
-        if (!sawBug2b) {
-            LOGGER.info("GOOD: Did not encounter Bug #2b " + "(LRM required un-stucking)!");
+            j.assertLogNotContains(line, b1);
         }
+        LOGGER.info("GOOD: Resource was properly locked at point 1-5 (no un-stucking needed)!");
+
+        j.waitForMessage("Locked resource cause 1-6", b1);
+        j.assertLogContains("Locked resource cause 2-2", b1);
 
         j.waitForMessage("Locked resource cause 2-2", b1);
         j.assertLogContains("Locked resource cause 1-5", b1);
-        LOGGER.info("GOOD: lock#2 was taken after we un-reserved lock#1");
+        LOGGER.info("GOOD: lock#2 was taken after p1 reset/released the resource");
 
-        j.waitForMessage("Unlocking parallel closure 2", b1);
+        // --- Phase 4: p2 exits lock (still reserved as test2-2) → p3 must NOT get in ---
+        SemaphoreStep.success("p2-holding/1", null);
+
+        // Wait for p2 to reach the semaphore before recycle — lock is released but reserved
+        SemaphoreStep.waitForStart("p2-before-recycle/1", b1);
+        j.assertLogContains("Unlocking parallel closure 2", b1);
         j.assertLogNotContains("Locked resource cause 3-2", b1);
         LOGGER.info("GOOD: lock#3 was NOT taken just after we un-locked closure 2 (keeping lock#2 reserved)");
 
-        // After 2-3 we lrm.recycle() the lock so it should
-        // go to the next bidder
+        // --- Phase 5: p2 recycles → p3 gets lock ---
+        // p2 calls recycle(), p3 gets the lock, then p2 blocks at p2-after-recycle.
+        // Wait for p3 to actually log 3-2 before letting p2 log 2-4.
+        SemaphoreStep.success("p2-before-recycle/1", null);
+        j.waitForMessage("Locked resource cause 3-2", b1);
+        SemaphoreStep.success("p2-after-recycle/1", null);
+
         j.waitForMessage("Locked resource cause 2-4", b1);
         j.assertLogContains("Locked resource cause 3-2", b1);
         LOGGER.info("GOOD: lock#3 was taken just after we recycled lock#2");
 
+        // Let p3 finish
+        SemaphoreStep.success("p3-in-lock/1", null);
+
         j.assertLogContains(", waiting for execution ...", b1);
 
         j.assertBuildStatusSuccess(j.waitForCompletion(b1));