Skip to content

Call Launcher afterDisconnect() only once when disconnecting an agent #26402

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

Open
Anvarjon7 wants to merge 1 commit into
base: master
Choose a base branch
from
+35 −2
Open

Call Launcher afterDisconnect() only once when disconnecting an agent #26402

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 35 additions & 2 deletions core/src/main/java/hudson/slaves/SlaveComputer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,18 @@ public class SlaveComputer extends Computer {
*/
private ComputerLauncher launcher;

/**
* Lock used to synchronize the execution of {@link ComputerLauncher#afterDisconnect(SlaveComputer, TaskListener)}.
* This ensures the disconnect logic is only executed once per connection cycle, and that
* concurrent attempts to disconnect block until the first execution completes.
* Blocking is critical to prevent file-lock race conditions during teardown on Windows.
*/
private final Object disconnectLock = new Object();
/**
* Guard flag protected by {@link #disconnectLock} to track if the launcher's afterDisconnect
* has already been called for the current channel lifecycle.
*/
private boolean afterDisconnectCalled = false;
/**
* Perpetually writable log file.
*/
Expand Down Expand Up @@ -651,7 +663,14 @@ public void onClosed(Channel c, IOException cause) {
}
closeChannel();
try {
launcher.afterDisconnect(SlaveComputer.this, taskListener);
// Synchronize to prevent double execution (e.g., JENKINS-35272) while
// forcing concurrent callers to wait for teardown to complete cleanly.
synchronized (disconnectLock) {
if (!afterDisconnectCalled) {
afterDisconnectCalled = true;
launcher.afterDisconnect(SlaveComputer.this, taskListener);
}
}
} catch (Throwable t) {
LogRecord lr = new LogRecord(Level.SEVERE,
"Launcher {0}'s afterDisconnect method propagated an exception when {1}'s connection was closed: {2}");
Expand Down Expand Up @@ -742,6 +761,12 @@ public void onClosed(Channel c, IOException cause) {
isUnix = _isUnix;
numRetryAttempt = 0;
this.channel = channel;

// Reset the disconnect guard for the new connection lifecycle
synchronized (disconnectLock) {
this.afterDisconnectCalled = false;
}
Comment on lines +765 to +768
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

afterDisconnectCalled is reset fairly late in setChannel() (after the channel listener is already registered and multiple remote calls have run). If the new channel closes quickly (e.g., setup failure) before this reset executes, onClosed() can run while afterDisconnectCalled is still true from the previous lifecycle and will skip launcher.afterDisconnect, potentially leaving teardown undone for that failed connection attempt.

Consider resetting the guard under disconnectLock earlier (right after the initial this.channel != null check, before channel.addListener(...) / remote calls). This also avoids holding channelLock while waiting for disconnectLock, which can unnecessarily block other channel state transitions.

Copilot uses AI. Check for mistakes.

this.absoluteRemoteFs = remoteFS;
defaultCharset = Charset.forName(defaultCharsetName);

Expand Down Expand Up @@ -823,7 +848,15 @@ public Future<?> disconnect(OfflineCause cause) {
// (which could be typical) won't block UI thread.
launcher.beforeDisconnect(SlaveComputer.this, taskListener);
closeChannel();
launcher.afterDisconnect(SlaveComputer.this, taskListener);

// Synchronize to prevent double execution (e.g., JENKINS-35272) while
// forcing concurrent callers to wait for teardown to complete cleanly.
synchronized (disconnectLock) {
if (!afterDisconnectCalled) {
afterDisconnectCalled = true;
launcher.afterDisconnect(SlaveComputer.this, taskListener);
}
}
});
}

Expand Down
Loading