diff --git a/test/test_update_check_reports_failure.py b/test/test_update_check_reports_failure.py new file mode 100644 index 00000000..f638bb9d --- /dev/null +++ b/test/test_update_check_reports_failure.py @@ -0,0 +1,85 @@ +"""A check that could not run must not be reported as "up to date". + +check-update returned update_available=False whenever git failed. The banner +is the only route to the update button, so a checkout git refuses to touch +looked exactly like a current one -- permanently, and with nothing for the +user to act on. The usual cause is an install performed as root, after which +every git command fails with "detected dubious ownership". +""" +import subprocess +import sys +from pathlib import Path +from unittest.mock import patch + +import pytest +from flask import Flask + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from web_interface.blueprints import api_v3 as mod # noqa: E402 +from web_interface.blueprints.api_v3 import api_v3 # noqa: E402 + +DUBIOUS = ("fatal: detected dubious ownership in repository at " + "'/home/pi/LEDMatrix'\nTo add an exception for this directory, call:\n" + "\tgit config --global --add safe.directory /home/pi/LEDMatrix\n") + + +@pytest.fixture +def client(): + app = Flask(__name__) + app.config['TESTING'] = True + app.register_blueprint(api_v3, url_prefix='/api/v3') + mod._update_check_cache['result'] = None + mod._update_check_cache['ts'] = 0 + return app.test_client() + + +def _fetch_fails(stderr: bytes): + def fake_run(args, **kwargs): + if args[:2] == ['git', 'fetch']: + return subprocess.CompletedProcess(args, 1, stdout=b'', stderr=stderr) + return subprocess.CompletedProcess(args, 0, stdout='', stderr='') + return fake_run + + +class TestFailedCheckIsNotSilence: + def test_dubious_ownership_is_reported_not_swallowed(self, client): + with patch.object(mod.subprocess, 'run', _fetch_fails(DUBIOUS.encode())): + data = client.get('/api/v3/system/check-update').get_json() + assert data['check_failed'] is True, ( + "a git failure was reported as a successful 'no update' check") + assert data['update_available'] is False + + def test_the_message_tells_the_user_what_to_do(self, client): + with patch.object(mod.subprocess, 'run', _fetch_fails(DUBIOUS.encode())): + data = client.get('/api/v3/system/check-update').get_json() + assert 'chown' in data['error'], ( + "dubious ownership is unactionable without the fix command") + assert 'root' in data['error'] + + def test_an_ordinary_git_failure_still_surfaces(self, client): + with patch.object(mod.subprocess, 'run', + _fetch_fails(b'fatal: some other git problem\n')): + data = client.get('/api/v3/system/check-update').get_json() + assert data['check_failed'] is True + assert 'some other git problem' in data['error'] + + def test_offline_reads_as_offline(self, client): + with patch.object(mod.subprocess, 'run', + _fetch_fails(b'fatal: could not resolve host: github.com\n')): + data = client.get('/api/v3/system/check-update').get_json() + assert 'Could not reach GitHub' in data['error'] + + +class TestSuccessPathUnchanged: + def test_up_to_date_carries_no_failure_flag(self, client): + def fake_run(args, **kwargs): + if args[:2] == ['git', 'fetch']: + return subprocess.CompletedProcess(args, 0, stdout=b'', stderr=b'') + if args[:2] == ['git', 'rev-parse']: + return subprocess.CompletedProcess(args, 0, stdout='abc123\n', stderr='') + return subprocess.CompletedProcess(args, 0, stdout='0\n', stderr='') + with patch.object(mod.subprocess, 'run', fake_run): + data = client.get('/api/v3/system/check-update').get_json() + assert data['update_available'] is False + assert not data.get('check_failed'), "a healthy check must not look like a failure" diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index 984637e8..35b35380 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -1821,6 +1821,33 @@ def get_system_version(): _update_check_cache: Dict[str, Any] = {'result': None, 'ts': 0.0} _UPDATE_CHECK_TTL = 300 # 5 minutes — avoids a git fetch on every page load +def _update_check_failed(detail: str) -> Dict[str, Any]: + """A check that could not run is not the same as being up to date. + + Reporting update_available=False on a git failure hides the banner, and + the banner is the only route to the update button -- so a checkout git + refuses to touch looks exactly like a current one, permanently. The most + common cause is an install performed as root: git then reports "dubious + ownership" and every command fails, including the fetch here. + """ + return {'update_available': False, 'remote_sha': 'unknown', + 'commits_behind': 0, 'check_failed': True, 'error': detail} + + +def _describe_git_failure(stderr: str) -> str: + """Turn git's stderr into something the user can act on.""" + text = (stderr or '').strip() + if 'dubious ownership' in text or 'detected dubious ownership' in text: + return ("This checkout is owned by a different user than the one " + "running the web interface, so git refuses to use it. It is " + "usually the result of installing as root. Fix the ownership " + "and the update will work: sudo chown -R $USER:$USER " + + str(PROJECT_ROOT)) + if 'could not resolve host' in text.lower() or 'network is unreachable' in text.lower(): + return "Could not reach GitHub to check for updates." + return "Could not check for updates: " + (text.splitlines()[0] if text else "git failed") + + @api_v3.route('/system/check-update', methods=['GET']) def check_for_update(): """Check whether a newer LEDMatrix commit is available on origin/main.""" @@ -1836,12 +1863,13 @@ def check_for_update(): capture_output=True, timeout=10, cwd=cwd, ) if fetch_result.returncode != 0: + stderr = fetch_result.stderr.decode(errors='replace').strip() logger.warning("check-update: git fetch failed (rc=%d): %s", - fetch_result.returncode, - fetch_result.stderr.decode(errors='replace').strip()) - _update_check_cache['result'] = _safe + fetch_result.returncode, stderr) + failed = _update_check_failed(_describe_git_failure(stderr)) + _update_check_cache['result'] = failed _update_check_cache['ts'] = now - return jsonify(_safe) + return jsonify(failed) local = subprocess.run( ['git', 'rev-parse', 'HEAD'], capture_output=True, text=True, timeout=5, cwd=cwd, @@ -1869,7 +1897,8 @@ def check_for_update(): return jsonify(result) except Exception as e: logger.warning("check-update failed: %s", e) - return jsonify(_safe) + return jsonify(_update_check_failed( + "Could not check for updates; see logs for details.")) @api_v3.route('/system/action', methods=['POST']) def execute_system_action(): diff --git a/web_interface/templates/v3/base.html b/web_interface/templates/v3/base.html index fb4cfa6a..1b5b6ec8 100644 --- a/web_interface/templates/v3/base.html +++ b/web_interface/templates/v3/base.html @@ -1107,15 +1107,29 @@ fetch('/api/v3/system/check-update') .then(function(r) { return r.json(); }) .then(function(data) { + var banner = document.getElementById('update-banner'); + var btn = document.getElementById('update-banner-btn'); + if (data.check_failed) { + // A check that could not run is not the same as being up + // to date. Hiding the banner here made a checkout git + // refuses to touch look permanently current, with no + // route to the update button and nothing to act on. + document.getElementById('update-banner-text').textContent = + data.error || 'Could not check for updates.'; + if (btn) btn.style.display = 'none'; + banner.style.display = ''; + return; + } + if (btn) btn.style.display = ''; if (data.update_available && getDismissedSha() !== data.remote_sha) { var n = data.commits_behind || 0; var msg = 'A new LEDMatrix update is available'; if (n > 0) msg += ' (' + n + ' commit' + (n > 1 ? 's' : '') + ')'; document.getElementById('update-banner-text').textContent = msg; - document.getElementById('update-banner').style.display = ''; + banner.style.display = ''; try { sessionStorage.setItem('update-sha', data.remote_sha); } catch(e) {} } else { - document.getElementById('update-banner').style.display = 'none'; + banner.style.display = 'none'; } }) .catch(function() {});