Skip to content

Ludicrous Mode: Lockable resources queue contention reduction and script caching #966

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
github-actions merged 5 commits into from
Mar 31, 2026
Merged

Ludicrous Mode: Lockable resources queue contention reduction and script caching #966

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f698eb7
Ludicrous Mode: Lockable resources queue contention reduction and scr…
samrocketman Mar 25, 2026
38ceef2
Fix async save state loss on restart
samrocketman Mar 25, 2026
aecec79
spotless apply
samrocketman Mar 25, 2026
931fc9d
Merge branch 'master' into ludicrous-mode
mPokornyETM Mar 31, 2026
65c0c3d
Merge branch 'master' into ludicrous-mode
mPokornyETM Mar 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/main/java/org/jenkins/plugins/lockableresources/FreeDeadJobs.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ private FreeDeadJobs() {}
public static void freePostMortemResources() {

LockableResourcesManager lrm = LockableResourcesManager.get();
boolean freedAny = false;
synchronized (lrm.syncResources) {
List<LockableResource> orphan = new ArrayList<>();
LOG.log(Level.FINE, "lockable-resources-plugin free post mortem task run");
Expand All @@ -47,7 +48,11 @@ public static void freePostMortemResources() {
+ " due post mortem job: "
+ resource.getBuildName());
resource.recycle();
freedAny = true;
}
}
if (freedAny) {
LockableResourcesManager.scheduleQueueMaintenance();
}
Comment on lines 34 to +56
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

freePostMortemResources() holds lrm.syncResources while calling resource.recycle(). Since recycle() calls back into LockableResourcesManager.recycle(..), which calls unlockResources(..) / unreserve(..) that now trigger scheduleQueueMaintenance(), this can end up attempting to acquire the Jenkins Queue lock while still holding syncResources (lock-order inversion / potential deadlock). Consider collecting the orphan resources under syncResources, releasing the lock, then recycling them and scheduling queue maintenance once afterwards.

Copilot uses AI. Check for mistakes.
}
}
93 changes: 92 additions & 1 deletion src/main/java/org/jenkins/plugins/lockableresources/LockableResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,13 @@
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ExecutionException;
import java.util.logging.Level;
import java.util.logging.Logger;
import jenkins.model.Jenkins;
import jenkins.util.SystemProperties;
import org.jenkins.plugins.lockableresources.util.Constants;
import org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SecureGroovyScript;
import org.jenkinsci.plugins.workflow.steps.StepContext;
import org.kohsuke.accmod.Restricted;
Expand Down Expand Up @@ -101,6 +104,63 @@ public class LockableResource extends AbstractDescribableImpl<LockableResource>

private static final long serialVersionUID = 1L;

private static final long SCRIPT_CACHE_TTL_MS =
SystemProperties.getLong(Constants.SYSTEM_PROPERTY_SCRIPT_CACHE_TTL_MS, 30_000L);
private static final long LABEL_CACHE_TTL_MS =
SystemProperties.getLong(Constants.SYSTEM_PROPERTY_LABEL_CACHE_TTL_MS, 30_000L);

/** Per-resource cache: Groovy script text -> (result, timestamp). */
private transient volatile ConcurrentHashMap<String, CachedResult> scriptCache;
/** Per-resource cache: label expression -> (result, timestamp). */
private transient volatile ConcurrentHashMap<String, CachedResult> labelCache;

Comment on lines +112 to +116
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scriptCache/labelCache are unbounded ConcurrentHashMaps and TTL is only checked on reads; expired entries are never evicted. In long-lived controllers with many distinct label expressions or scripts, these maps can grow without bound (per resource). Consider using a bounded cache with time-based eviction (e.g., Caffeine/Guava) or removing entries when they are detected as expired.

Copilot uses AI. Check for mistakes.
private static final class CachedResult {
final boolean value;
final long timestamp;

CachedResult(boolean value) {
this.value = value;
this.timestamp = System.currentTimeMillis();
}

boolean isExpired(long ttlMs) {
return (System.currentTimeMillis() - timestamp) > ttlMs;
}
}

private ConcurrentHashMap<String, CachedResult> getScriptCache() {
ConcurrentHashMap<String, CachedResult> c = scriptCache;
if (c == null) {
synchronized (this) {
c = scriptCache;
if (c == null) {
scriptCache = c = new ConcurrentHashMap<>();
}
}
}
return c;
}

private ConcurrentHashMap<String, CachedResult> getLabelCache() {
ConcurrentHashMap<String, CachedResult> c = labelCache;
if (c == null) {
synchronized (this) {
c = labelCache;
if (c == null) {
labelCache = c = new ConcurrentHashMap<>();
}
}
}
return c;
}

void invalidateCaches() {
ConcurrentHashMap<String, CachedResult> sc = scriptCache;
if (sc != null) sc.clear();
ConcurrentHashMap<String, CachedResult> lc = labelCache;
if (lc != null) lc.clear();
}

private transient boolean isNode = false;

/**
Expand Down Expand Up @@ -239,6 +299,7 @@ public void setLabels(@Nullable String labels) {
}
this.labelsAsList.add(label);
}
invalidateCaches();
}

/**
Expand Down Expand Up @@ -288,12 +349,25 @@ public boolean isValidLabel(@Nullable String candidate) {
return true;
}

if (LABEL_CACHE_TTL_MS > 0) {
ConcurrentHashMap<String, CachedResult> cache = getLabelCache();
CachedResult cached = cache.get(candidate);
if (cached != null && !cached.isExpired(LABEL_CACHE_TTL_MS)) {
return cached.value;
}
boolean result = evaluateLabelExpression(candidate);
cache.put(candidate, new CachedResult(result));
return result;
}
return evaluateLabelExpression(candidate);
}

private boolean evaluateLabelExpression(@NonNull String candidate) {
final Label labelExpression = Label.parseExpression(candidate);
Set<LabelAtom> atomLabels = new HashSet<>();
for (String label : this.getLabelsAsList()) {
atomLabels.add(new LabelAtom(label));
}

return labelExpression.matches(atomLabels);
}

Expand Down Expand Up @@ -330,6 +404,22 @@ public void setProperties(@Nullable List<LockableResourceProperty> properties) {
@Restricted(NoExternalUse.class)
public boolean scriptMatches(@NonNull SecureGroovyScript script, @CheckForNull Map<String, Object> params)
throws ExecutionException {
if (SCRIPT_CACHE_TTL_MS > 0) {
String cacheKey = script.getScript();
ConcurrentHashMap<String, CachedResult> cache = getScriptCache();
CachedResult cached = cache.get(cacheKey);
if (cached != null && !cached.isExpired(SCRIPT_CACHE_TTL_MS)) {
return cached.value;
}
boolean result = evaluateScript(script, params);
cache.put(cacheKey, new CachedResult(result));
return result;
Comment on lines 405 to +416
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scriptMatches caches results by script.getScript() only, but the evaluation also depends on the provided params (build parameters/matrix combinations) and on bound resource fields (description/labels/note). This can return incorrect matches for different queue items within the TTL window. Include the relevant params (e.g., a stable hash of the map entries) and/or the resource fields in the cache key, or only cache when params is null/empty, and ensure caches are invalidated when bound fields change.

Copilot uses AI. Check for mistakes.
}
return evaluateScript(script, params);
Comment on lines 405 to +418
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no tests covering the new script/label TTL caching behavior. In particular, caching script results without considering params can change matching semantics across builds; adding unit/integration tests that queue multiple items with different parameters and a resourceMatchScript would help prevent regressions.

Copilot uses AI. Check for mistakes.
}

private boolean evaluateScript(@NonNull SecureGroovyScript script, @CheckForNull Map<String, Object> params)
throws ExecutionException {
Binding binding = new Binding(params);
binding.setVariable("resourceName", name);
binding.setVariable("resourceDescription", description);
Expand Down Expand Up @@ -577,6 +667,7 @@ public void reset() {
this.unReserve();
this.unqueue();
this.setBuild(null);
invalidateCaches();
}

/**
Expand Down
122 changes: 100 additions & 22 deletions src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -79,6 +82,14 @@ public class LockableResourcesManager extends GlobalConfiguration {
private static final int enabledCausesCount =
SystemProperties.getInteger(Constants.SYSTEM_PROPERTY_PRINT_QUEUE_INFO, 2);

private static final boolean asyncSaveEnabled =
SystemProperties.getBoolean(Constants.SYSTEM_PROPERTY_ASYNC_SAVE, true);
private static final long saveCoalesceMs =
SystemProperties.getLong(Constants.SYSTEM_PROPERTY_SAVE_COALESCE_MS, 1000L);

private transient volatile AtomicBoolean savePending;
private transient volatile ScheduledExecutorService saveExecutor;

@DataBoundSetter
public void setAllowEmptyOrNullValues(boolean allowEmptyOrNullValues) {
this.allowEmptyOrNullValues = allowEmptyOrNullValues;
Expand Down Expand Up @@ -494,6 +505,29 @@ public List<LockableResource> tryQueue(
Map<String, Object> params,
Logger log)
throws ExecutionException {

final SecureGroovyScript systemGroovyScript;
try {
systemGroovyScript = requiredResources.getResourceMatchScript();
} catch (Descriptor.FormException x) {
throw new ExecutionException(x);
}
boolean candidatesByScript = (systemGroovyScript != null);

// Resolve candidates outside syncResources when possible — label matching
// and Groovy script evaluation are heavyweight and should not extend the
// critical section.
List<LockableResource> candidates = null;
if (candidatesByScript || (requiredResources.label != null && !requiredResources.label.isEmpty())) {
candidates = cachedCandidates.getIfPresent(queueItemId);
if (candidates == null) {
candidates = (systemGroovyScript == null)
? getResourcesWithLabel(requiredResources.label)
: getResourcesMatchingScript(systemGroovyScript, params);
Comment on lines +519 to +528
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says candidates are resolved “outside syncResources”, but both getResourcesWithLabel(..) and getResourcesMatchingScript(..) acquire synchronized(syncResources) internally, so label/script evaluation is still performed under the plugin lock. Either update the comment to reflect the actual behavior (lock is released and reacquired) or refactor to evaluate against a snapshot of resources so the heavyweight work can truly happen without holding syncResources.

Copilot uses AI. Check for mistakes.
cachedCandidates.put(queueItemId, candidates);
}
}

List<LockableResource> selected = new ArrayList<>();
synchronized (syncResources) {
if (!checkCurrentResourcesStatus(selected, queueItemProject, queueItemId, log)) {
Expand All @@ -505,26 +539,10 @@ public List<LockableResource> tryQueue(
return null;
}

final SecureGroovyScript systemGroovyScript;
try {
systemGroovyScript = requiredResources.getResourceMatchScript();
} catch (Descriptor.FormException x) {
throw new ExecutionException(x);
}
boolean candidatesByScript = (systemGroovyScript != null);
List<LockableResource> candidates = requiredResources.required; // default candidates

if (candidatesByScript || (requiredResources.label != null && !requiredResources.label.isEmpty())) {

candidates = cachedCandidates.getIfPresent(queueItemId);
if (candidates != null) {
candidates.retainAll(this.resources);
} else {
candidates = (systemGroovyScript == null)
? getResourcesWithLabel(requiredResources.label)
: getResourcesMatchingScript(systemGroovyScript, params);
cachedCandidates.put(queueItemId, candidates);
}
if (candidates != null) {
candidates.retainAll(this.resources);
} else {
candidates = requiredResources.required;
}

for (LockableResource rs : candidates) {
Expand Down Expand Up @@ -723,6 +741,7 @@ public void unlockResources(List<LockableResource> resourcesToUnLock, Run<?, ?>

save();
}
scheduleQueueMaintenance();
}
Comment on lines 733 to 747
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unlockResources(..) now calls scheduleQueueMaintenance() unconditionally. However, this method is invoked from call sites that already hold synchronized(syncResources) (e.g., steal(..) and recycle(..)), so the queue maintenance trigger can still run while the plugin lock is held—contradicting scheduleQueueMaintenance()'s own Javadoc and risking lock-order inversion with the Jenkins Queue lock. Consider removing the scheduling side effect from unlockResources(..) (or splitting into an internal helper) and only calling scheduleQueueMaintenance() from outermost methods after all syncResources monitors have been released.

Copilot uses AI. Check for mistakes.

private boolean proceedNextContext() {
Expand Down Expand Up @@ -1007,6 +1026,7 @@ public void unreserve(List<LockableResource> resources) {

save();
}
scheduleQueueMaintenance();
}

// ---------------------------------------------------------------------------
Expand All @@ -1025,6 +1045,7 @@ public void reset(List<LockableResource> resources) {
}
save();
}
scheduleQueueMaintenance();
}

// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -1392,6 +1413,52 @@ public static LockableResourcesManager get() {
}

// ---------------------------------------------------------------------------
/**
* Trigger an immediate Queue re-evaluation so items waiting for lockable
* resources are dispatched as soon as resources become available, instead of
* waiting for the next 5-second timer tick.
* <p>
* Must be called <b>outside</b> {@code synchronized(syncResources)} to avoid
* holding the plugin lock while Jenkins acquires the Queue lock.
*/
public static void scheduleQueueMaintenance() {
Jenkins j = Jenkins.getInstanceOrNull();
if (j != null) {
j.getQueue().scheduleMaintenance();
}
}

// ---------------------------------------------------------------------------
private AtomicBoolean getSavePending() {
AtomicBoolean sp = savePending;
if (sp == null) {
synchronized (this) {
sp = savePending;
if (sp == null) {
savePending = sp = new AtomicBoolean(false);
}
}
}
return sp;
}

private ScheduledExecutorService getSaveExecutor() {
ScheduledExecutorService se = saveExecutor;
if (se == null) {
synchronized (this) {
se = saveExecutor;
if (se == null) {
saveExecutor = se = Executors.newSingleThreadScheduledExecutor(r -> {
Thread t = new Thread(r, "lockable-resources-async-save");
t.setDaemon(true);
return t;
});
}
Comment on lines +1447 to +1458
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The async-save implementation creates a dedicated ScheduledExecutorService but there is no lifecycle management (shutdown) when Jenkins reloads configuration / the plugin is reloaded, which can leave daemon threads around. Prefer using Jenkins' shared timer (jenkins.util.Timer) or add explicit cleanup (e.g., via an @Initializer/@Terminator hook) so executors do not leak across reloads.

Copilot uses AI. Check for mistakes.
}
}
return se;
}

@Override
public void save() {
if (enableSave == -1) {
Expand All @@ -1401,9 +1468,20 @@ public void save() {

if (enableSave == 0) return; // saving is disabled

synchronized (syncResources) {
if (BulkChange.contains(this)) return;
if (BulkChange.contains(this)) return;

if (asyncSaveEnabled && saveCoalesceMs > 0) {
if (getSavePending().compareAndSet(false, true)) {
getSaveExecutor().schedule(this::doSave, saveCoalesceMs, TimeUnit.MILLISECONDS);
}
} else {
doSave();
}
Comment on lines +1475 to +1481
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With async-save enabled, save() may return before the configuration has been persisted. If Jenkins is shutting down or crashes within the coalescing window, the latest lock state changes may never be written. Consider flushing pending saves on shutdown (or when disabling async mode), or ensuring critical state changes force an immediate doSave() when the controller is terminating.

Copilot uses AI. Check for mistakes.
}
Comment on lines 1464 to +1482
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The async-save/coalescing behavior is new and not covered by tests (e.g., verifying that multiple rapid state changes result in exactly one persisted write after the coalesce delay, and that disabling async mode still saves immediately). Adding tests around save() would help ensure future changes don’t reintroduce Queue-lock contention or cause persistence regressions.

Copilot uses AI. Check for mistakes.

private void doSave() {
getSavePending().set(false);
synchronized (syncResources) {
try {
getConfigFile().write(this);
} catch (IOException e) {
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/org/jenkins/plugins/lockableresources/queue/LockRunListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ public void onCompleted(Run<?, ?> build, @NonNull TaskListener listener) {
}
LOGGER.info(build.getFullDisplayName());
LockableResourcesManager.get().unlockBuild(build);
LockableResourcesManager.scheduleQueueMaintenance();
}

@Override
Expand All @@ -113,5 +114,6 @@ public void onDeleted(Run<?, ?> build) {
}
LOGGER.info(build.getFullDisplayName());
LockableResourcesManager.get().unlockBuild(build);
LockableResourcesManager.scheduleQueueMaintenance();
}
}
12 changes: 12 additions & 0 deletions src/main/java/org/jenkins/plugins/lockableresources/util/Constants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,16 @@ public class Constants {
"org.jenkins.plugins.lockableresources.PRINT_BLOCKED_RESOURCE";
public static final String SYSTEM_PROPERTY_PRINT_QUEUE_INFO =
"org.jenkins.plugins.lockableresources.PRINT_QUEUE_INFO";
/// Enable asynchronous save to reduce syncResources lock hold time.
public static final String SYSTEM_PROPERTY_ASYNC_SAVE =
"org.jenkins.plugins.lockableresources.ASYNC_SAVE";
/// Coalesce window (ms) for async saves — rapid state changes within this window are batched.
public static final String SYSTEM_PROPERTY_SAVE_COALESCE_MS =
"org.jenkins.plugins.lockableresources.SAVE_COALESCE_MS";
/// TTL (ms) for Groovy script evaluation result cache per resource.
public static final String SYSTEM_PROPERTY_SCRIPT_CACHE_TTL_MS =
"org.jenkins.plugins.lockableresources.SCRIPT_CACHE_TTL_MS";
/// TTL (ms) for label expression evaluation result cache per resource.
public static final String SYSTEM_PROPERTY_LABEL_CACHE_TTL_MS =
"org.jenkins.plugins.lockableresources.LABEL_CACHE_TTL_MS";
}
Loading
⚡