From 638552e5f8444ac724666684b8a162445ddb69ae Mon Sep 17 00:00:00 2001 From: VAIO73 <50487331+Vaso73@users.noreply.github.com> Date: Wed, 14 Oct 2026 16:40:14 +0000 Subject: [PATCH] fix: preserve verified LXC state before a failed start --- AppImage/scripts/flask_server.py | 25 +++++- AppImage/scripts/lxc_update_observation.py | 77 ++++++++++++++++++- AppImage/scripts/managed_installs.py | 6 +- scripts/lxc/apply_updates.sh | 10 +++ .../test_stopped_registry_retention.py | 36 +++++++-- .../test_stopped_update_observation.py | 47 +++++++++-- tests/lxc_updates/test_update_contract.py | 46 +++++++++++ 7 files changed, 228 insertions(+), 19 deletions(-) diff --git a/AppImage/scripts/flask_server.py b/AppImage/scripts/flask_server.py index 6f19195c..14d20310 100644 --- a/AppImage/scripts/flask_server.py +++ b/AppImage/scripts/flask_server.py @@ -2076,7 +2076,14 @@ def _refresh_started_guest(vmid: int, vm_type: str) -> None: lxc_apps.invalidate_suggestion_probes(vmid) - managed_installs.refresh_lxc(vmid) + try: + managed_installs.refresh_lxc(vmid) + except Exception as exc: + # Package inventory is one stage of a started-guest rebuild. + # A slow/failed `pct list` must not skip app, cache, and UI + # refreshes that can still complete for this guest. + print(f'[ProxMenux] lifecycle refresh lxc {vmid}: ' + f'package inventory: {exc}', flush=True) sidecar = lxc_apps.check_all(vmid, force=True) sidecar = sidecar or {'vmid': vmid, 'apps': []} @@ -14551,10 +14558,22 @@ def _finalize_lxc_update( import managed_installs import lxc_update_observation observation = lxc_update_observation.read(vmid, safe_run_id) - verified = managed_installs.store_lxc_update_observation(vmid, observation) + started = lxc_update_observation.read_started(vmid, safe_run_id) + marker_write_failed = str(reason or '').endswith('code 8') + # A failed start, foreign-node rejection, or pre-start cancel has + # no trusted start marker and must not erase the last observed + # package state. A rejected marker is unknown and is handled as a + # touched run; runner exit 8 also means a start occurred but its + # marker could not be recorded. + if observation is None and started is False and not marker_write_failed: + verified = True + untouched = True + else: + verified = managed_installs.store_lxc_update_observation(vmid, observation) + untouched = False print(f'[ProxMenux] stopped LXC verification: vmid={vmid} ' f'run_id={safe_run_id} handoff={observation is not None} ' - f'verified={verified}', flush=True) + f'started={started} untouched={untouched} verified={verified}', flush=True) if not verified: verification_errors.append('post-update OS verification unavailable') apps_persisted = True diff --git a/AppImage/scripts/lxc_update_observation.py b/AppImage/scripts/lxc_update_observation.py index ded9ffd3..643a9173 100644 --- a/AppImage/scripts/lxc_update_observation.py +++ b/AppImage/scripts/lxc_update_observation.py @@ -29,6 +29,11 @@ def _path(vmid: int, run_id: str) -> Path: return OBSERVATION_DIR / f"{int(vmid)}-{run_id}.json" +def _started_path(vmid: int, run_id: str) -> Path: + _path(vmid, run_id) + return OBSERVATION_DIR / f"{int(vmid)}-{run_id}.started" + + def _private_directory(*, create=True) -> None: if create: OBSERVATION_DIR.mkdir(mode=0o700, parents=True, exist_ok=True) @@ -38,6 +43,65 @@ def _private_directory(*, create=True) -> None: raise OSError("unsafe LXC observation directory") +def mark_started(vmid: int, run_id: str) -> None: + """Record that this run started a stopped CT before it can change state.""" + path = _started_path(vmid, run_id) + _private_directory() + payload = { + "vmid": int(vmid), + "run_id": run_id, + "started_at": datetime.datetime.now(datetime.timezone.utc).isoformat(), + } + fd, temporary = tempfile.mkstemp(prefix=".started-", dir=OBSERVATION_DIR) + try: + os.fchmod(fd, 0o600) + with os.fdopen(fd, "w", encoding="utf-8") as handle: + json.dump(payload, handle) + os.replace(temporary, path) + print(f"[ProxMenux] LXC update started: vmid={int(vmid)} run_id={run_id}", flush=True) + finally: + if os.path.exists(temporary): + os.unlink(temporary) + + +def read_started(vmid: int, run_id: str) -> bool | None: + """Return True for a trusted start marker, False when absent, else None.""" + path = _started_path(vmid, run_id) + + def reject(reason: str) -> None: + print(f"[ProxMenux] LXC update start marker rejected: vmid={int(vmid)} " + f"run_id={run_id} reason={reason}", flush=True) + return None + + try: + _private_directory(create=False) + except FileNotFoundError: + return False + except OSError as exc: + return reject(type(exc).__name__) + try: + fd = os.open(path, os.O_RDONLY | os.O_NOFOLLOW | os.O_NONBLOCK) + except FileNotFoundError: + return False + except OSError as exc: + return reject(type(exc).__name__) + try: + with os.fdopen(fd, "r", encoding="utf-8") as handle: + info = os.fstat(handle.fileno()) + if (not stat.S_ISREG(info.st_mode) or info.st_uid != os.geteuid() + or info.st_mode & 0o077 or info.st_size > MAX_BYTES): + return reject('unsafe-file') + marker = json.load(handle) + started = datetime.datetime.fromisoformat(marker["started_at"]) + age = (datetime.datetime.now(datetime.timezone.utc) - started).total_seconds() + if (not isinstance(marker, dict) or marker.get("vmid") != int(vmid) + or marker.get("run_id") != run_id or not 0 <= age <= MAX_AGE): + return reject('identity-age-or-shape') + return True + except (OSError, ValueError, TypeError, KeyError, OverflowError) as exc: + return reject(type(exc).__name__) + + def capture(vmid: int, run_id: str, *, refresh_indexes: bool = False, include_apps: bool = False) -> dict: path = _path(vmid, run_id) @@ -46,7 +110,7 @@ def capture(vmid: int, run_id: str, *, refresh_indexes: bool = False, now = datetime.datetime.now(datetime.timezone.utc) for old in OBSERVATION_DIR.iterdir(): info = old.lstat() - if (re.fullmatch(r"[1-9][0-9]*-[A-Za-z0-9._:-]{1,128}\.json", old.name) + if (re.fullmatch(r"[1-9][0-9]*-[A-Za-z0-9._:-]{1,128}(?:\.json|\.started)", old.name) and stat.S_ISREG(info.st_mode) and info.st_uid == os.geteuid() and now.timestamp() - info.st_mtime > MAX_AGE): old.unlink() @@ -156,7 +220,11 @@ def read(vmid: int, run_id: str) -> dict | None: def discard(vmid: int, run_id: str) -> None: try: _private_directory(create=False) - _path(vmid, run_id).unlink() + for path in (_path(vmid, run_id), _started_path(vmid, run_id)): + try: + path.unlink() + except FileNotFoundError: + pass except FileNotFoundError: pass @@ -165,10 +233,15 @@ def main(argv=None) -> int: parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--vmid", required=True, type=int) parser.add_argument("--run-id", required=True) + parser.add_argument('--mark-started', action='store_true', + help='Record that this run started a previously stopped CT') parser.add_argument('--include-apps', action='store_true', help='Capture registered installed versions after an update') args = parser.parse_args(argv) try: + if args.mark_started: + mark_started(args.vmid, args.run_id) + return 0 capture(args.vmid, args.run_id, include_apps=args.include_apps) return 0 except Exception as exc: diff --git a/AppImage/scripts/managed_installs.py b/AppImage/scripts/managed_installs.py index 430c652a..e85f3fb7 100644 --- a/AppImage/scripts/managed_installs.py +++ b/AppImage/scripts/managed_installs.py @@ -540,6 +540,10 @@ def _list_pve_lxcs(*, strict: bool = False) -> list[dict]: raise RuntimeError('LXC inventory could not be read') return [] lines = r.stdout.splitlines() + # `pct list` returns successful empty stdout when the node has no CTs. + # That is a complete inventory, so strict callers may safely retire rows. + if not any(line.strip() for line in lines): + return [] if strict and (not lines or not lines[0].strip().startswith('VMID') or 'Status' not in lines[0]): raise RuntimeError('LXC inventory is incomplete') @@ -1892,7 +1896,7 @@ def store_lxc_update_observation(vmid: int, observation: Optional[dict]) -> bool reg = _read_registry() items = reg.setdefault("items", []) target = next((it for it in items if it.get("id") == f"lxc:{vmid}" - and it.get("type") == "lxc" and not it.get("removed_at")), None) + and it.get("type") == "lxc"), None) if target is None: if observation is None: return False diff --git a/scripts/lxc/apply_updates.sh b/scripts/lxc/apply_updates.sh index 39b46b6a..bf2728f6 100755 --- a/scripts/lxc/apply_updates.sh +++ b/scripts/lxc/apply_updates.sh @@ -44,6 +44,7 @@ # nor explicitly-enabled verified helper) available in the CT # 6 post-update restart failed # 7 another ProxMenux update is already running for this CT +# 8 started CT could not record its verification handoff identity # # The frontend surfaces exit code + duration in a follow-up POST to # /api/lxc-updates//applied so the notification event fires with @@ -183,6 +184,15 @@ if [[ "$STATE" != "running" ]]; then exit 2 fi STARTED_BY_PROXMENUX=1 + # The finalizer must distinguish a failed/pre-start run from a run that + # actually started this previously stopped CT but later lacks a result. + if [[ -n "${RUN_ID:-}" ]]; then + if ! python3 /usr/local/share/proxmenux/monitor-app/usr/bin/lxc_update_observation.py \ + --vmid "$VMID" --run-id "$RUN_ID" --mark-started; then + echo "ERROR: started CT verification identity could not be recorded." >&2 + exit 8 + fi + fi # give the CT a moment for services to come up sleep 3 fi diff --git a/tests/lxc_updates/test_stopped_registry_retention.py b/tests/lxc_updates/test_stopped_registry_retention.py index ecc41ed7..9679ade9 100644 --- a/tests/lxc_updates/test_stopped_registry_retention.py +++ b/tests/lxc_updates/test_stopped_registry_retention.py @@ -128,6 +128,24 @@ class StoppedRegistryTests(unittest.TestCase): self.assertEqual(row['_os_family'], 'alpine') self.assert_no_guest_probe() + def test_targeted_observation_reactivates_removed_row_without_a_duplicate(self): + registry = managed._read_registry() + registry['items'][0]['removed_at'] = self.now + managed._write_registry(registry) + observation = { + 'entry': {**self.row, '_os_family': 'debian'}, + 'result': {'available': True, 'last_check': self.now, '_count': 2}, + } + + self.assertTrue(managed.store_lxc_update_observation(210, observation)) + + registry = managed.get_registry() + rows = [row for row in registry['items'] if row['id'] == 'lxc:210'] + self.assertEqual(len(rows), 1) + self.assertNotIn('removed_at', rows[0]) + self.assertEqual(rows[0]['update_check']['_count'], 2) + self.assert_no_guest_probe() + def test_retained_result_still_expires_at_existing_48_hour_boundary(self): managed.detect_and_register() current = managed.get_active_items()[0]['update_check'] @@ -145,11 +163,11 @@ class StoppedRegistryTests(unittest.TestCase): class InventoryContractTests(unittest.TestCase): def test_default_failure_contract_and_strict_detector_contract(self): - for result in [subprocess.CompletedProcess([], 1, '', ''), subprocess.CompletedProcess([], 0, '', '')]: - with patch.object(managed.subprocess, 'run', return_value=result): - self.assertEqual(managed._list_pve_lxcs(), []) - with self.assertRaises(RuntimeError): - managed._list_pve_lxcs(strict=True) + result = subprocess.CompletedProcess([], 1, '', '') + with patch.object(managed.subprocess, 'run', return_value=result): + self.assertEqual(managed._list_pve_lxcs(), []) + with self.assertRaises(RuntimeError): + managed._list_pve_lxcs(strict=True) def test_timeout_and_malformed_rows_are_not_confirmed_removal(self): for failure in [subprocess.TimeoutExpired('pct', 5), FileNotFoundError(), OSError()]: @@ -162,8 +180,12 @@ class InventoryContractTests(unittest.TestCase): managed._list_pve_lxcs(strict=True) def test_confirmed_empty_inventory_is_not_a_failed_read(self): - with patch.object(managed.subprocess, 'run', return_value=subprocess.CompletedProcess([], 0, 'VMID Status Lock Name\n', '')): - self.assertEqual(managed._list_pve_lxcs(strict=True), []) + for stdout in ['', '\n', 'VMID Status Lock Name\n']: + with self.subTest(stdout=repr(stdout)), patch.object( + managed.subprocess, 'run', + return_value=subprocess.CompletedProcess([], 0, stdout, ''), + ): + self.assertEqual(managed._list_pve_lxcs(strict=True), []) if __name__ == '__main__': diff --git a/tests/lxc_updates/test_stopped_update_observation.py b/tests/lxc_updates/test_stopped_update_observation.py index 645b7ef7..2f348324 100644 --- a/tests/lxc_updates/test_stopped_update_observation.py +++ b/tests/lxc_updates/test_stopped_update_observation.py @@ -59,9 +59,13 @@ root = pathlib.Path(os.environ['FIXTURE_DIR']) args = sys.argv[1:] with (root/'commands').open('a') as log: log.write(json.dumps(args)+'\\n') state = root/'state' -if args[0] == 'list': print('VMID Status Name\\n100 '+state.read_text()+' fixture') +if args[0] == 'list': + if os.environ.get('FOREIGN') == '1': print('VMID Status Name\\n200 stopped foreign') + else: print('VMID Status Name\\n100 '+state.read_text()+' fixture') elif args[0] == 'status': print('status: '+state.read_text()) -elif args[0] == 'start': state.write_text('running') +elif args[0] == 'start': + if os.environ.get('START_FAIL') == '1': sys.exit(1) + state.write_text('running') elif args[0] == 'shutdown': if os.environ.get('SHUTDOWN_FAIL') == '1': sys.exit(1) state.write_text('stopped') @@ -102,8 +106,10 @@ m._PCT_BIN = str(root/'bin/pct') h.OBSERVATION_DIR = root/'observations' apps._APPS_DIR = str(root/'apps') apps._PCT_BIN = str(root/'bin/pct') -with (root/'commands').open('a') as log: log.write('CAPTURE\\n') -if os.environ.get('CAPTURE_FAIL') == '1': raise SystemExit(1) +marker = '--mark-started' in os.sys.argv +with (root/'commands').open('a') as log: log.write('MARK_STARTED\\n' if marker else 'CAPTURE\\n') +if os.environ.get('MARKER_FAIL') == '1' and marker: raise SystemExit(1) +if os.environ.get('CAPTURE_FAIL') == '1' and not marker: raise SystemExit(1) m._detect_lxc_containers = lambda **_: ([{'id':'lxc:100','type':'lxc','_vmid':'100', '_is_oci':os.environ.get('OCI') == '1'}] if (root/'state').read_text() == 'running' else []) raise SystemExit(h.main()) @@ -169,11 +175,12 @@ raise SystemExit(h.main()) self.assertIsNone(row['error']) self.assertEqual(row['os_family'], 'debian') log = self.log.read_text() + self.assertLess(log.index('MARK_STARTED'), log.index('CAPTURE')) self.assertLess(log.index('CAPTURE'), log.index('shutdown')) self.assertLess(log.index('apt list --upgradable'), log.index('shutdown')) self.assertNotIn('apt-get update', log) self.assertEqual(log.count('dist-upgrade'), 1) - self.assertFalse(list(self.observations.glob('*.json'))) + self.assertFalse(list(self.observations.iterdir())) self.assertEqual(managed.get_registry()['items'][1]['update_check']['_count'], 7) self.assertEqual(self.fixture.commands, [], 'GET and stopped finalization run no guest probes') @@ -198,6 +205,33 @@ raise SystemExit(h.main()) def test_missing_capture_does_not_present_old_count_or_fresh_zero(self): self.assert_unverified('CAPTURE_FAIL') + def test_pre_start_failure_or_foreign_target_preserves_last_verified_result(self): + for condition in ('START_FAIL', 'FOREIGN'): + with self.subTest(condition=condition): + registry = managed.get_registry() + registry['items'][0]['update_check'] = { + 'available': True, 'last_check': '2026-10-14T00:00:00Z', + '_count': 20, 'error': None, + } + managed._write_registry(registry) + result = self.run_manual(**{condition: '1'}) + self.assertNotEqual(result.returncode, 0) + self.assertEqual(self.state.read_text(), 'stopped') + stored = managed.get_registry()['items'][0]['update_check'] + self.assertEqual(stored['_count'], 20) + self.assertIsNone(stored['error']) + log = self.log.read_text() + self.assertNotIn('MARK_STARTED', log) + self.assertNotIn('CAPTURE', log) + + def test_started_run_with_unwritable_marker_is_reported_as_unverified(self): + result = self.run_manual(MARKER_FAIL='1', CAPTURE_FAIL='1') + self.assertEqual(result.returncode, 8) + self.assertEqual(self.state.read_text(), 'stopped') + stored = managed.get_registry()['items'][0]['update_check'] + self.assertTrue(stored['error']) + self.assertNotIn('_count', stored) + def test_web_runner_without_identity_reports_unverified_instead_of_silent_skip(self): env = {**self.env, 'EXECUTION_MODE': 'web'} env.pop('RUN_ID') @@ -210,6 +244,7 @@ raise SystemExit(h.main()) def test_rejected_handoff_is_retained_with_safe_diagnostic_and_no_false_zero(self): self.observations.mkdir(mode=0o700) + handoff.mark_started(100, 'manual-fixture') path = self.observations / '100-manual-fixture.json' path.write_text(json.dumps({'private_payload': 'must-not-be-logged'})) path.chmod(0o600) @@ -219,7 +254,7 @@ raise SystemExit(h.main()) status='success', source='manual', target='os') self.assertTrue(path.exists()) self.assertIn('reason=KeyError', output.getvalue()) - self.assertIn('handoff=False verified=False', output.getvalue()) + self.assertIn('handoff=False started=True untouched=False verified=False', output.getvalue()) self.assertNotIn('must-not-be-logged', output.getvalue()) self.assertTrue(self.fixture.row()['update_check']['error']) self.assertNotIn('_count', managed.get_registry()['items'][0]['update_check']) diff --git a/tests/lxc_updates/test_update_contract.py b/tests/lxc_updates/test_update_contract.py index c5e35e60..5466a7e2 100644 --- a/tests/lxc_updates/test_update_contract.py +++ b/tests/lxc_updates/test_update_contract.py @@ -1,6 +1,9 @@ +import ast import sys import subprocess import tempfile +import threading +import time import unittest from types import SimpleNamespace from pathlib import Path @@ -23,6 +26,7 @@ class UpdateStrategyValidationTests(unittest.TestCase): self.assertTrue(ok, config) self.assertEqual(config["update_strategy"], "custom_override") + def test_legacy_helper_then_custom_is_normalised_to_override(self): ok, config = lxc_apps.validate_config({ "name": "Jellyfin", @@ -42,6 +46,48 @@ class UpdateStrategyValidationTests(unittest.TestCase): self.assertEqual(config["update_strategy"], "custom_override") +class LifecycleInventoryResilienceTests(unittest.TestCase): + def test_failed_targeted_inventory_refresh_does_not_abort_started_lxc_rebuild(self): + source = ast.parse((REPO_ROOT / 'AppImage/scripts/flask_server.py').read_text()) + function = next(node for node in source.body if isinstance(node, ast.FunctionDef) + and node.name == '_refresh_started_guest') + warmed = [] + published = MagicMock() + lxc = SimpleNamespace( + invalidate_suggestion_probes=MagicMock(), + check_all=MagicMock(return_value={'vmid': 100, 'apps': []}), + _pct_exec=MagicMock(return_value=(1, '', 'not installed')), + ensure_oci_registration=MagicMock(), + get_suggestions=MagicMock(return_value={'suggestions': []}), + ) + managed = SimpleNamespace( + refresh_lxc=MagicMock(side_effect=RuntimeError('pct list timed out')), + ) + scope = { + 'time': time, + '_wait_for_started_guest': lambda *_: True, + '_vm_cache_invalidate': MagicMock(), + '_vm_disk_cache': {}, + '_warm_started_guest_handler': lambda vmid, route, _: warmed.append((vmid, route)), + 'get_vm_config': object(), 'api_vm_backups': object(), + 'api_vm_apps_schedule': object(), 'api_lxc_mount_points': object(), + '_publish_guest_modal_cache_revision': published, + '_vm_cache_put': MagicMock(), '_vm_app_suggestions_cache': {}, + '_guest_lifecycle_lock': threading.RLock(), + '_guest_refresh_inflight': {100}, '_guest_modal_cache_revision': {}, + '_guest_modal_cache_epoch': 0, + } + with patch.dict(sys.modules, {'lxc_apps': lxc, 'managed_installs': managed}): + exec(compile(ast.Module(body=[function], type_ignores=[]), 'lifecycle-refresh', 'exec'), scope) + scope['_refresh_started_guest'](100, 'lxc') + + managed.refresh_lxc.assert_called_once_with(100) + lxc.check_all.assert_called_once_with(100, force=True) + self.assertEqual(len(warmed), 4) + self.assertGreaterEqual(published.call_count, 1) + self.assertNotIn(100, scope['_guest_refresh_inflight']) + + class HelperEvidenceTests(unittest.TestCase): def test_legacy_literal_wrapper_slug_is_supported(self): wrapper = (