admin: stop holding the shell admin lock across whole scheduler batches (#10290)

* admin: let waiting shell clients win the admin lock

The admin lock manager re-acquired the cluster shell lock immediately
after releasing it, while a waiting weed shell client only polls the
master once a second, so an operator could lose the race indefinitely.
Leave the lock free for slightly more than one poll interval before
re-acquiring, and once overlapping reference-counted holds have kept
the lock continuously held for over a minute, make new admin-side
acquires wait for a full release before piggybacking.

* admin/plugin: take the shell lock per detection and per job, not per batch

The default-lane scheduler acquired the cluster shell lock once and
held it across the whole pass: every due job type's detection plus all
of its dispatched jobs, each drained to completion. A manual weed
shell operation sharing that lock could stall behind the batch for up
to the extended execution window.

Hold the lock only while it protects something: around each detection
scan and around each dispatched job. Manual shell operations now wait
for at most one in-flight job. The admin UI detect+execute path stops
wrapping dispatch in an outer hold, since a nested acquire would
deadlock against the lock manager's fairness window; detection and
per-job dispatch take the lock themselves.

* admin/plugin: stop extending the dispatch window to the largest job estimate

When any proposal's estimated runtime exceeded the remaining
JobTypeMaxRuntime, the whole dispatch context was replaced with a
fresh one capped at eight hours, so a single balance backlog could
hold the default lane (and with it erasure_coding and vacuum
detection) for that long.

Keep the dispatch window at JobTypeMaxRuntime and instead detach each
started attempt onto its own estimated-runtime deadline. Large jobs
still get their full time once started; jobs not yet started when the
window closes are canceled and re-proposed by a later detection, so
sibling job types get a turn every window.

* worker/balance: re-check each planned move against the master before executing

A balance plan is computed at detection time, but the admin lock is
released between detection and execution, so a manual shell operation
can rearrange the volume in the gap. The task's own guards catch a
vanished source, but a target that gained a replica in the meantime
would be silently overwritten by VolumeCopy and the source delete
would then reduce the volume to a single copy.

Before executing each move, ask the master for the volume's current
locations (uncached) and skip the move if the volume has left the
source or the target already holds a replica. Skipped moves fail with
a stale-move error and the next detection replans them. Without master
addresses in the cluster context the check is skipped, preserving the
old behavior with older admins.

* admin: block re-acquire while the final lock release is in flight

Release dropped the manager mutex before calling ReleaseLock, so a
concurrent Acquire could see hold count zero and call RequestLock
while the locker still considered itself locked. That request no-ops,
leaving a hold with no live master lease. Track the in-flight release
and make Acquire wait for it.

Also normalize a nil release function from lock manager
implementations, and make the yield and fairness windows per-instance
fields so tests stop mutating globals.

weed/admin/dash/admin_lock_manager.go

@@ -14,16 +14,38 @@ const (
 	adminLockClientName = "admin-plugin"
 )
 
+const (
+	// A competing shell client polls the master once a second, while a local
+	// re-acquire would win immediately. Leave the lock free for slightly more
+	// than one poll interval so a waiting operator gets it first.
+	defaultAdminLockYieldWindow = 2 * time.Second
+	// After the lock has been held continuously for this long (overlapping
+	// reference-counted holds can keep it held indefinitely), new Acquire
+	// calls wait for a full release so shell clients are not starved.
+	defaultAdminLockFairnessWindow = time.Minute
+)
+
+// adminLocker is the subset of exclusive_locks.ExclusiveLocker the manager uses.
+type adminLocker interface {
+	RequestLock(clientName string)
+	ReleaseLock()
+	SetMessage(message string)
+}
+
 // AdminLockManager coordinates exclusive admin locks with reference counting.
 // It is safe for concurrent use.
 type AdminLockManager struct {
-	locker     *exclusive_locks.ExclusiveLocker
-	clientName string
+	locker         adminLocker
+	clientName     string
+	yieldWindow    time.Duration
+	fairnessWindow time.Duration
 
 	mu        sync.Mutex
 	cond      *sync.Cond
 	acquiring bool
+	releasing bool
 	holdCount int
+	heldSince time.Time
 
 	lastAcquiredAt time.Time
 	lastReleasedAt time.Time
@@ -40,13 +62,19 @@ func NewAdminLockManager(masterClient *wdclient.MasterClient, clientName string)
 		clientName = adminLockClientName
 	}
 	manager := &AdminLockManager{
-		locker:     exclusive_locks.NewExclusiveLocker(masterClient, adminLockName),
-		clientName: clientName,
+		locker:         exclusive_locks.NewExclusiveLocker(masterClient, adminLockName),
+		clientName:     clientName,
+		yieldWindow:    defaultAdminLockYieldWindow,
+		fairnessWindow: defaultAdminLockFairnessWindow,
 	}
 	manager.cond = sync.NewCond(&manager.mu)
 	return manager
 }
 
+// Acquire takes the shared cluster lock, reference counted so overlapping
+// admin-side holders piggyback on one lease. It must not be called while the
+// caller's own call stack already holds the lock: once the fairness window
+// makes new acquires wait for a full release, a nested Acquire deadlocks.
 func (m *AdminLockManager) Acquire(reason string) (func(), error) {
 	if m == nil || m.locker == nil {
 		return func() {}, nil
@@ -57,18 +85,28 @@ func (m *AdminLockManager) Acquire(reason string) (func(), error) {
 		m.locker.SetMessage(reason)
 		m.currentReason = reason
 	}
-	for m.acquiring {
+	for m.acquiring || m.releasing || (m.holdCount > 0 && time.Since(m.heldSince) > m.fairnessWindow) {
 		m.cond.Wait()
 	}
 	if m.holdCount == 0 {
 		m.acquiring = true
 		m.waitingSince = time.Now().UTC()
 		m.waitingReason = reason
+		yield := time.Duration(0)
+		if !m.lastReleasedAt.IsZero() {
+			if since := time.Since(m.lastReleasedAt); since < m.yieldWindow {
+				yield = m.yieldWindow - since
+			}
+		}
 		m.mu.Unlock()
+		if yield > 0 {
+			time.Sleep(yield)
+		}
 		m.locker.RequestLock(m.clientName)
 		m.mu.Lock()
 		m.acquiring = false
 		m.holdCount = 1
+		m.heldSince = time.Now()
 		m.lastAcquiredAt = time.Now().UTC()
 		m.waitingSince = time.Time{}
 		m.waitingReason = ""
@@ -96,14 +134,22 @@ func (m *AdminLockManager) Release() {
 	}
 	m.holdCount--
 	shouldRelease := m.holdCount == 0
+	if shouldRelease {
+		// Block new acquires until ReleaseLock completes: a RequestLock
+		// issued while the locker still considers itself locked would no-op
+		// and leave a hold with no live lease.
+		m.releasing = true
+	}
 	m.mu.Unlock()
 
 	if shouldRelease {
+		m.locker.ReleaseLock()
 		m.mu.Lock()
+		m.releasing = false
 		m.lastReleasedAt = time.Now().UTC()
 		m.currentReason = ""
+		m.cond.Broadcast()
 		m.mu.Unlock()
-		m.locker.ReleaseLock()
 	}
 }
 

weed/admin/dash/admin_lock_manager_test.go

@@ -0,0 +1,156 @@
+package dash
+
+import (
+	"sync"
+	"testing"
+	"time"
+)
+
+type fakeAdminLocker struct {
+	releaseDelay time.Duration
+
+	mu       sync.Mutex
+	requests []time.Time
+	releases []time.Time // recorded when ReleaseLock returns
+}
+
+func (f *fakeAdminLocker) RequestLock(clientName string) {
+	f.mu.Lock()
+	f.requests = append(f.requests, time.Now())
+	f.mu.Unlock()
+}
+
+func (f *fakeAdminLocker) ReleaseLock() {
+	if f.releaseDelay > 0 {
+		time.Sleep(f.releaseDelay)
+	}
+	f.mu.Lock()
+	f.releases = append(f.releases, time.Now())
+	f.mu.Unlock()
+}
+
+func (f *fakeAdminLocker) SetMessage(message string) {}
+
+func (f *fakeAdminLocker) counts() (int, int) {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	return len(f.requests), len(f.releases)
+}
+
+func newTestLockManager(locker adminLocker, yieldWindow, fairnessWindow time.Duration) *AdminLockManager {
+	m := &AdminLockManager{
+		locker:         locker,
+		clientName:     "test",
+		yieldWindow:    yieldWindow,
+		fairnessWindow: fairnessWindow,
+	}
+	m.cond = sync.NewCond(&m.mu)
+	return m
+}
+
+func TestAdminLockManagerRefCountsOverlappingHolds(t *testing.T) {
+	t.Parallel()
+
+	locker := &fakeAdminLocker{}
+	m := newTestLockManager(locker, defaultAdminLockYieldWindow, defaultAdminLockFairnessWindow)
+
+	release1, _ := m.Acquire("a")
+	release2, _ := m.Acquire("b")
+
+	if requests, releases := locker.counts(); requests != 1 || releases != 0 {
+		t.Fatalf("expected one lease request while overlapping, got requests=%d releases=%d", requests, releases)
+	}
+
+	release1()
+	if _, releases := locker.counts(); releases != 0 {
+		t.Fatalf("lock released while still held by another caller")
+	}
+	release2()
+	if _, releases := locker.counts(); releases != 1 {
+		t.Fatalf("expected one release after last holder, got %d", releases)
+	}
+}
+
+func TestAdminLockManagerYieldsBeforeReacquire(t *testing.T) {
+	t.Parallel()
+
+	locker := &fakeAdminLocker{}
+	m := newTestLockManager(locker, 100*time.Millisecond, defaultAdminLockFairnessWindow)
+
+	release, _ := m.Acquire("first")
+	release()
+	release, _ = m.Acquire("second")
+	release()
+
+	locker.mu.Lock()
+	gap := locker.requests[1].Sub(locker.releases[0])
+	locker.mu.Unlock()
+	if gap < 90*time.Millisecond {
+		t.Fatalf("re-acquire did not yield to competing clients: gap=%v", gap)
+	}
+}
+
+func TestAdminLockManagerFairnessWindowForcesFullRelease(t *testing.T) {
+	t.Parallel()
+
+	locker := &fakeAdminLocker{}
+	m := newTestLockManager(locker, 20*time.Millisecond, 50*time.Millisecond)
+
+	release1, _ := m.Acquire("long-hold")
+	m.mu.Lock()
+	m.heldSince = time.Now().Add(-time.Second)
+	m.mu.Unlock()
+
+	acquired := make(chan func(), 1)
+	go func() {
+		release2, _ := m.Acquire("late-joiner")
+		acquired <- release2
+	}()
+
+	select {
+	case <-acquired:
+		t.Fatalf("late acquire joined a hold older than the fairness window")
+	case <-time.After(50 * time.Millisecond):
+	}
+
+	release1()
+
+	select {
+	case release2 := <-acquired:
+		release2()
+	case <-time.After(time.Second):
+		t.Fatalf("late acquire did not proceed after full release")
+	}
+
+	if requests, releases := locker.counts(); requests != 2 || releases != 2 {
+		t.Fatalf("expected a full release/re-acquire cycle, got requests=%d releases=%d", requests, releases)
+	}
+}
+
+func TestAdminLockManagerAcquireWaitsForReleaseInFlight(t *testing.T) {
+	t.Parallel()
+
+	locker := &fakeAdminLocker{releaseDelay: 50 * time.Millisecond}
+	m := newTestLockManager(locker, time.Millisecond, defaultAdminLockFairnessWindow)
+
+	release1, _ := m.Acquire("first")
+	releaseDone := make(chan struct{})
+	go func() {
+		release1()
+		close(releaseDone)
+	}()
+	// Let the release enter the slow ReleaseLock before re-acquiring.
+	time.Sleep(10 * time.Millisecond)
+	release2, _ := m.Acquire("second")
+	<-releaseDone
+	release2()
+
+	locker.mu.Lock()
+	defer locker.mu.Unlock()
+	if len(locker.requests) != 2 || len(locker.releases) != 2 {
+		t.Fatalf("unexpected lease traffic: requests=%d releases=%d", len(locker.requests), len(locker.releases))
+	}
+	if locker.requests[1].Before(locker.releases[0]) {
+		t.Fatalf("RequestLock ran while ReleaseLock was still in flight")
+	}
+}

weed/admin/dash/admin_server.go

@@ -1459,8 +1459,8 @@ func (s *AdminServer) RunPluginDetectionWithReport(
 
 // DispatchPluginProposals dispatches a batch of proposals using the same
 // capacity-aware dispatch logic as the scheduler loop (executor reservation with
-// backoff, per-job retry on transient errors). The plugin lock must already be
-// held by the caller.
+// backoff, per-job retry on transient errors). The dispatch takes the cluster
+// admin lock around each job itself; callers must not hold it.
 func (s *AdminServer) DispatchPluginProposals(
 	ctx context.Context,
 	jobType string,

weed/admin/dash/plugin_api.go

@@ -595,20 +595,15 @@ func (s *AdminServer) TriggerPluginDetectionAPI(w http.ResponseWriter, r *http.R
 }
 
 // RunPluginJobTypeAPI runs full workflow for one job type: detect then dispatch detected jobs.
+// RunPluginDetection and the dispatch path take the cluster admin lock
+// themselves (around detection and around each job), so no lock is held
+// across the whole workflow here.
 func (s *AdminServer) RunPluginJobTypeAPI(w http.ResponseWriter, r *http.Request) {
 	jobType := strings.TrimSpace(mux.Vars(r)["jobType"])
 	if jobType == "" {
 		writeJSONError(w, http.StatusBadRequest, "jobType is required")
 		return
 	}
-	releaseLock, err := s.acquirePluginLock(fmt.Sprintf("plugin detect+execute %s", jobType))
-	if err != nil {
-		writeJSONError(w, http.StatusInternalServerError, err.Error())
-		return
-	}
-	if releaseLock != nil {
-		defer releaseLock()
-	}
 
 	var req struct {
 		ClusterContext json.RawMessage `json:"cluster_context"`