fix: scoped-lock guard dead code in buzz/irc/line adapters (tuple truthiness) - #74978
Open
spector-in-london wants to merge 1 commit into
Open
spector-in-london wants to merge 1 commit into
spector-in-london wants to merge 1 commit into
Conversation
…thiness) acquire_scoped_lock() returns tuple[bool, Optional[dict]]. Three platform adapters tested the return for truthiness instead of unpacking it: - buzz/adapter.py:475 — if not acquire_scoped_lock(buzz, lock_key): - irc/adapter.py:170 — if not acquire_scoped_lock(irc, lock_key): - line/adapter.py:790 — if not acquire_scoped_lock(line, tok_hash): A non-empty 2-tuple is always truthy, so 'not ret' was always False and the lock_conflict refuse-branch was unreachable dead code. The scoped lock silently never prevented anything on these three platforms. The correct shape already exists in gateway/platforms/base.py:3213 and plugins/platforms/feishu/adapter.py:1751 — unpack the tuple and test the bool. This fixes all three call sites to match. Also enriches the log lines with the holding PID (from the existing dict) so the operator learns which process holds the lock, matching base.py. Regression tests added for all three adapters asserting the guard fires on conflict. The existing buzz test mock (returning bare False) is fixed to return the real tuple shape — without this it would crash on unpack.
Collaborator
|
Thanks for tracking this down. The production premise is verified on current main: Problems
Suggested changes
The patch otherwise targets all current production instances found by the scoped-lock search. GitHub main |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
gateway/status.py:acquire_scoped_lock()returnstuple[bool, Optional[dict]].Three platform adapters test the return value for truthiness instead of unpacking it:
plugins/platforms/buzz/adapter.py:475—if not acquire_scoped_lock("buzz", lock_key):plugins/platforms/irc/adapter.py:170—if not acquire_scoped_lock("irc", lock_key):plugins/platforms/line/adapter.py:790—if not acquire_scoped_lock("line", tok_hash):A non-empty 2-tuple is always truthy, so
not retis alwaysFalseand thelock_conflictrefuse-branch is unreachable dead code. The scoped lock silently never prevents anything on these three platforms.Correct shape (already in tree)
The correct pattern already exists in the same codebase:
gateway/platforms/base.py:3213—acquired, existing = acquire_scoped_lock(...)plugins/platforms/feishu/adapter.py:1751— sameFix
One-line change per call site — unpack the tuple and test the bool:
Also enriches the log lines with the holding PID (from the
existingdict) so the operator learns which process holds the lock, matchingbase.py.Tests
test_connect_fails_when_identity_lock_heldbuzz test: its mock returned a bareFalse, which would crash on tuple unpacking after the fix. Now returns the real(False, {...})shape.TestIRCScopedLockRegression— two tests: conflict path fires, acquired path does not fail withlock_conflict.TestLineScopedLockRegression— conflict path fires, acquired path exercises the lock guard.All 75 tests in the three adapter test files pass.
Impact
For Buzz this is the control that prevents two Hermes profiles from driving one Nostr identity on one relay — Nostr events are immutable and there is no rotation story for a misattributed signed event. The same class of bug affects IRC (duplicate nick on the same server) and LINE (duplicate channel access token).