Opus stage-360 review caught that the docstring at api/streaming.py:40-43
said 'around the entire agent run' which is no longer accurate after the
narrow-lock refactor. The lock is now held only briefly for the env-mutation
critical section; the agent runs outside the lock and the finally block
re-acquires to atomically restore env vars.
Docstring now points to both narrow-lock implementations as references:
- _run_agent_streaming at line ~2719 (the original pattern)
- profile_env_for_background_worker at api/profiles.py:715 (added stage-360)
#2299 added test_env_lock_importable_from_streaming asserting reentrance,
which contradicts the architectural invariant enforced by QA
test_env_lock_is_non_reentrant. The QA test wins because the non-reentrant
property is what makes _ENV_LOCK catch deadlock bugs early.
Updated the new test to assert NON-reentrance to match the actual lock
type (threading.Lock) and the QA invariant.
#2299 introduced profile_env_for_background_worker() in api/profiles.py and
changed _ENV_LOCK from threading.Lock() to threading.RLock(). Both changes
were incorrect:
1. RLock masked rather than fixed the underlying deadlock. The QA
test_env_lock_is_non_reentrant test exists precisely to enforce
non-reentrance — RLock would let a single thread hold _ENV_LOCK across
nested critical sections, which hides bugs while still allowing
different-thread races.
2. The original context manager held _ENV_LOCK for the ENTIRE 'yield'
duration, meaning the lock was held for the full background worker's
runtime (title generation, compression, update summary — possibly
many seconds). That blocked ALL other sessions on _ENV_LOCK, which
the QA test_third_message_completes runtime test caught as a timeout
on the third sequential message.
Fix: mirror the narrow-lock pattern from _run_agent_streaming:
- Acquire _ENV_LOCK only for env mutation (set runtime_env + patch
skill modules)
- Release immediately, yield to worker (no lock held)
- Reacquire in finally to restore env + skill modules
Restored _ENV_LOCK back to threading.Lock(). All 20 QA tests now pass,
including test_third_message_completes (was timing out, now 35s).
- Remove duplicate mobile-close-btn from HTML
- Remove dead .mobile-close-btn CSS rules; unhide .close-preview at all viewports
- Change btnClearPreview tooltip from 'Hide workspace panel' to 'Close'
- Update tests across test_sprint41.py, test_sprint44.py, test_issue781.py,
and test_mobile_layout.py to match new single-button model
The boot IIFE unconditionally overwrote localStorage with whatever
settings.json had on the server. If the appearance autosave POST
ever failed (network glitch, transient error) the next page load
would revert the user's chosen theme/skin to the server's stale
defaults.
Fix: reconcile localStorage against the server on boot. When
localStorage carries a non-default skin or system theme (the user
explicitly chose something), localStorage wins and the fix pushes
those values back to the server. When localStorage is at defaults
(new browser / first visit), the server still wins.
Tested scenarios:
- User chose non-default skin, autosave failed → preserved + reconciled
- New browser, server has non-default skin → server value applied
- Normal use (autosave works) → unchanged behavior
Michael Lam