From 96edce3a3c194629fcff761e778caf180294dfd8 Mon Sep 17 00:00:00 2001 From: Chuck Date: Wed, 29 Apr 2026 19:15:52 -0400 Subject: [PATCH] =?UTF-8?q?fix(update-banner):=20address=20review=20findin?= =?UTF-8?q?gs=20=E2=80=94=20lock,=20returncode=20checks,=20update=5Favaila?= =?UTF-8?q?ble=20logic,=20a11y,=20button=20state?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add _update_check_lock (threading.Lock) around all reads/writes to _update_check_cache in check_for_update() and git_pull, preventing races on concurrent requests - Validate returncode for git fetch, rev-parse HEAD, and rev-parse origin/main; raise RuntimeError on failure so errors are caught and returned as error payloads instead of silently producing stale/empty SHAs - Set update_available = commits_behind > 0 (was unconditionally True when local_sha != remote_sha); prevents false positive when local is ahead of remote - Add type="button" and aria-label="Dismiss update" to the icon-only dismiss button - Restore btn.innerHTML and btn.disabled in both success and error paths of applyUpdate(); only hide the banner and clear sessionStorage when data.status === 'success' Co-Authored-By: Claude Sonnet 4.6 --- web_interface/blueprints/api_v3.py | 43 +++++++++++++++++++--------- web_interface/templates/v3/base.html | 15 ++++++---- 2 files changed, 39 insertions(+), 19 deletions(-) diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index 3204eaa2..93484d0a 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -8,6 +8,7 @@ import time import hashlib import uuid import logging +import threading from datetime import datetime from pathlib import Path from typing import Optional, Tuple, Dict, Any, Type @@ -1343,29 +1344,40 @@ def get_system_version(): _update_check_cache: Dict = {} _UPDATE_CHECK_TTL = 300 # 5 minutes +_update_check_lock = threading.Lock() @api_v3.route('/system/check-update', methods=['GET']) def check_for_update(): """Check if a newer version is available on the remote.""" - import time as _time - now = _time.time() - if _update_check_cache.get('ts', 0) + _UPDATE_CHECK_TTL > now: - return jsonify(_update_check_cache['data']) + now = time.time() + with _update_check_lock: + if _update_check_cache.get('ts', 0) + _UPDATE_CHECK_TTL > now: + return jsonify(_update_check_cache['data']) project_dir = str(PROJECT_ROOT) try: - subprocess.run( + fetch_result = subprocess.run( ['git', 'fetch', 'origin', 'main'], capture_output=True, text=True, timeout=15, cwd=project_dir ) - local_sha = subprocess.run( + if fetch_result.returncode != 0: + raise RuntimeError(f"git fetch failed: {fetch_result.stderr.strip()}") + + local_result = subprocess.run( ['git', 'rev-parse', 'HEAD'], capture_output=True, text=True, timeout=5, cwd=project_dir - ).stdout.strip() - remote_sha = subprocess.run( + ) + if local_result.returncode != 0: + raise RuntimeError(f"git rev-parse HEAD failed: {local_result.stderr.strip()}") + local_sha = local_result.stdout.strip() + + remote_result = subprocess.run( ['git', 'rev-parse', 'origin/main'], capture_output=True, text=True, timeout=5, cwd=project_dir - ).stdout.strip() + ) + if remote_result.returncode != 0: + raise RuntimeError(f"git rev-parse origin/main failed: {remote_result.stderr.strip()}") + remote_sha = remote_result.stdout.strip() if local_sha == remote_sha: data = {'status': 'success', 'update_available': False, @@ -1376,20 +1388,22 @@ def check_for_update(): capture_output=True, text=True, timeout=5, cwd=project_dir ) lines = [l for l in log_result.stdout.strip().split('\n') if l] + commits_behind = len(lines) data = { 'status': 'success', - 'update_available': True, + 'update_available': commits_behind > 0, 'local_sha': local_sha[:8], 'remote_sha': remote_sha[:8], - 'commits_behind': len(lines), + 'commits_behind': commits_behind, 'latest_message': lines[0].split(' ', 1)[1] if lines else '', } except Exception as e: logger.warning("[System] check-update failed: %s", e) data = {'status': 'error', 'update_available': False, 'message': str(e)} - _update_check_cache['ts'] = now - _update_check_cache['data'] = data + with _update_check_lock: + _update_check_cache['ts'] = now + _update_check_cache['data'] = data return jsonify(data) @api_v3.route('/system/action', methods=['POST']) @@ -1502,7 +1516,8 @@ def execute_system_action(): ) # Invalidate update-check cache so the banner hides immediately - _update_check_cache.clear() + with _update_check_lock: + _update_check_cache.clear() # Return custom response for git_pull if result.returncode == 0: diff --git a/web_interface/templates/v3/base.html b/web_interface/templates/v3/base.html index 03a2f3a4..245bdb40 100644 --- a/web_interface/templates/v3/base.html +++ b/web_interface/templates/v3/base.html @@ -948,9 +948,9 @@ update-banner-action transition-colors duration-150"> Update Now - @@ -4947,6 +4947,7 @@ window.applyUpdate = function() { var btn = document.getElementById('update-banner-btn'); + var originalHTML = ' Update Now'; btn.innerHTML = ' Updating...'; btn.disabled = true; fetch('/api/v3/system/action', { @@ -4956,14 +4957,18 @@ }) .then(function(r) { return r.json(); }) .then(function(data) { - document.getElementById('update-banner').style.display = 'none'; + btn.innerHTML = originalHTML; + btn.disabled = false; + if (data.status === 'success') { + document.getElementById('update-banner').style.display = 'none'; + try { sessionStorage.removeItem('update-dismissed'); } catch(e) {} + } if (typeof showNotification === 'function') { showNotification(data.message || 'Update complete', data.status || 'success'); } - try { sessionStorage.removeItem('update-dismissed'); } catch(e) {} }) .catch(function() { - btn.innerHTML = ' Update Now'; + btn.innerHTML = originalHTML; btn.disabled = false; if (typeof showNotification === 'function') { showNotification('Update failed — check your connection', 'error');