From d1a614ce6629f33810c832c690b087586bd6f4c5 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 11 Oct 2026 08:37:45 +0200 Subject: [PATCH] Decide lock requests from the current lock state (#14978) The locked binding once read true after an unlock, though the request, the session lock, and its secure state were all false. A lock before suspend then took the session for locked and did nothing, and the machine slept unlocked. Lock requests, the status reply, and stranded lock recovery now read those three values directly. Co-authored-by: Claude Opus 5.5 --- shell/plugins/lock/Service.qml | 21 ++++++++++++++------- test/shell.d/lock-stranded-recovery-test.sh | 19 ++++++++++++++++--- 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/shell/plugins/lock/Service.qml b/shell/plugins/lock/Service.qml index e6bc4ea5..c775ba02 100644 --- a/shell/plugins/lock/Service.qml +++ b/shell/plugins/lock/Service.qml @@ -57,6 +57,13 @@ Item { property bool strandedLockResolved: false readonly property bool locked: lockRequested || sessionLock.locked || sessionLock.secure + + // Decisions read the lock state afresh. A property binding that missed a change + // notification once reported locked after an unlock, and a lock request before + // suspend then took that for done and never locked the session. + function currentlyLocked() { + return lockRequested || sessionLock.locked || sessionLock.secure + } readonly property bool authenticating: authenticatingPassword || fingerprintAuthenticating readonly property var batteryService: shell && shell.services ? shell.firstPartyServiceFor("omarchy.battery") : null readonly property bool powerSaverActive: batteryService ? batteryService.powerSaverOnBattery : false @@ -109,7 +116,7 @@ Item { if (strandedLockResolved || strandedLockCheckProc.running) return // A lock this shell took is nobody's orphan. - if (locked || lockRequested) { + if (currentlyLocked()) { strandedLockResolved = true return } @@ -118,7 +125,7 @@ Item { } function recoverStrandedLock() { - if (!strandedLock || locked || !passwordPamConfigured) return + if (!strandedLock || currentlyLocked() || !passwordPamConfigured) return strandedLock = false logEvent("lock-stranded: recovering") @@ -218,7 +225,7 @@ Item { } function finishUnlock() { - if (!root.locked && !lockRequested) return + if (!root.currentlyLocked()) return lockRequested = false pendingSessionLock = false @@ -688,7 +695,7 @@ Item { root.strandedLockResolved = true // A lock taken while this was in flight is this shell's own. - root.strandedLock = exitCode === 0 && !root.locked && !root.lockRequested + root.strandedLock = exitCode === 0 && !root.currentlyLocked() root.recoverStrandedLock() } } @@ -833,17 +840,17 @@ Item { function lock(): string { if (!root.passwordPamConfigured) return "missing-pam" - if (!root.locked && !root.beginLock()) return "failed" + if (!root.currentlyLocked() && !root.beginLock()) return "failed" return "ok" } function isLocked(): string { - return root.locked ? "true" : "false" + return root.currentlyLocked() ? "true" : "false" } function status(): string { return JSON.stringify({ - locked: root.locked, + locked: root.currentlyLocked(), requested: root.lockRequested, pending: root.pendingSessionLock, sessionLocked: sessionLock.locked, diff --git a/test/shell.d/lock-stranded-recovery-test.sh b/test/shell.d/lock-stranded-recovery-test.sh index dec67038..d0e5cb81 100755 --- a/test/shell.d/lock-stranded-recovery-test.sh +++ b/test/shell.d/lock-stranded-recovery-test.sh @@ -32,10 +32,23 @@ assert( // omarchy-restart-shell re-locks a fresh shell, possibly mid-question. assert( - /root\.strandedLock = exitCode === 0 && !root\.locked && !root\.lockRequested/.test(serviceQml), + /root\.strandedLock = exitCode === 0 && !root\.currentlyLocked\(\)/.test(serviceQml), 'a lock this shell took while the check was in flight is not stranded' ) +// A locked binding that missed a change once read locked after an unlock, and +// the lock before suspend took that for done. Decisions read the state afresh. +assert( + /function currentlyLocked\(\) \{\s*return lockRequested \|\| sessionLock\.locked \|\| sessionLock\.secure\s*\}/.test(serviceQml), + 'the lock state is read from the session lock itself, not a cached binding' +) +assert( + /function lock\(\): string \{[\s\S]*?if \(!root\.currentlyLocked\(\) && !root\.beginLock\(\)\) return "failed"/.test(serviceQml) + && /function isLocked\(\): string \{\s*return root\.currentlyLocked\(\)/.test(serviceQml) + && /locked: root\.currentlyLocked\(\),/.test(serviceQml), + 'lock, isLocked, and status decide from the current lock state' +) + assert( /id: strandedLockRetryTimer[\s\S]*running: !root\.strandedLockResolved && remaining > 0/.test(serviceQml), 'the check retries while the compositor cannot answer, and stops once it has' @@ -60,12 +73,12 @@ assert( // A lock this shell owns ends the search. assert( - /function checkStrandedLock\(\) \{\s*if \(strandedLockResolved \|\| strandedLockCheckProc\.running\) return[\s\S]*if \(locked \|\| lockRequested\) \{\s*strandedLockResolved = true/.test(serviceQml), + /function checkStrandedLock\(\) \{\s*if \(strandedLockResolved \|\| strandedLockCheckProc\.running\) return[\s\S]*if \(currentlyLocked\(\)\) \{\s*strandedLockResolved = true/.test(serviceQml), 'a lock this shell took is not treated as stranded' ) assert( - /function recoverStrandedLock\(\) \{\s*if \(!strandedLock \|\| locked \|\| !passwordPamConfigured\) return/.test(serviceQml), + /function recoverStrandedLock\(\) \{\s*if \(!strandedLock \|\| currentlyLocked\(\) \|\| !passwordPamConfigured\) return/.test(serviceQml), 'recovery is skipped unless a stranded lock is waiting and PAM can authenticate it' )