From d0a98f6491f6ce9b13689fa9868a0c7229809a06 Mon Sep 17 00:00:00 2001 From: Oliver Date: Tue, 15 Sep 2026 21:24:40 +1000 Subject: [PATCH] Add more robust plugin installation check (#12858) --- src/backend/InvenTree/plugin/installer.py | 62 +++++++++ src/backend/InvenTree/plugin/registry.py | 42 +++--- src/backend/InvenTree/plugin/test_plugin.py | 145 ++++++++++++++++---- 3 files changed, 203 insertions(+), 46 deletions(-) diff --git a/src/backend/InvenTree/plugin/installer.py b/src/backend/InvenTree/plugin/installer.py index 6f0dcfd6ce..dc226ef63b 100644 --- a/src/backend/InvenTree/plugin/installer.py +++ b/src/backend/InvenTree/plugin/installer.py @@ -3,6 +3,7 @@ import re import subprocess import sys +from pathlib import Path from typing import Optional from django.conf import settings @@ -128,6 +129,67 @@ def plugins_file_hash(): return None +def plugin_env_marker_path() -> Path: + """Return the path to the plugin-install marker file for the *current* python environment. + + This lives inside the running interpreter's environment (``sys.prefix``) + rather than in the database, so it disappears along with the rest of the + environment whenever a fresh virtual environment is created - e.g. a + container replaced without a persistent venv volume. A database-only hash + cannot tell such a fresh environment apart from one that already has the + packages installed (inventree/InvenTree#12848). + """ + return Path(sys.prefix) / '.inventree_plugins_hash' + + +# Process-local fallback for get_env_plugin_hash(), used when the marker file +# itself cannot be written (e.g. a read-only sys.prefix). +_env_plugin_hash_cache: Optional[str] = None + + +def get_env_plugin_hash() -> Optional[str]: + """Return the plugin file hash last installed into the *current* python environment. + + Returns None if no install has been recorded here (e.g. a fresh environment). + """ + if _env_plugin_hash_cache is not None: + return _env_plugin_hash_cache + + path = plugin_env_marker_path() + + if not path.exists(): + return None + + try: + return path.read_text().strip() + except Exception: + log_error('get_env_plugin_hash', scope='plugins') + return None + + +def set_env_plugin_hash(file_hash: str) -> None: + """Record that the current python environment has installed the given plugin file hash.""" + global _env_plugin_hash_cache + + _env_plugin_hash_cache = file_hash + + try: + plugin_env_marker_path().write_text(file_hash) + except Exception: + # Not logged via log_error/the database error log: on a deployment + # where sys.prefix is not writable (by design, e.g. a read-only + # root filesystem) this would otherwise happen on every single + # process start forever, and it is an environment property rather + # than an application bug. + logger.warning( + "Could not persist plugin install marker to '%s' - this " + 'environment will be re-verified on every process restart ' + 'instead of only when %s changes', + plugin_env_marker_path(), + settings.PLUGIN_FILE, + ) + + def install_plugins_file(): """Install plugins from the plugins file.""" logger.info('Installing plugins from plugins file') diff --git a/src/backend/InvenTree/plugin/registry.py b/src/backend/InvenTree/plugin/registry.py index 653b0f7d94..a9337c9ac1 100644 --- a/src/backend/InvenTree/plugin/registry.py +++ b/src/backend/InvenTree/plugin/registry.py @@ -682,32 +682,31 @@ class PluginsRegistry: def install_plugin_file(self): """Make sure all plugins are installed in the current environment. - The hash is only persisted *after* a successful install, never before - - the lease alone is what stops two processes from installing at once (a - process that cannot acquire it simply skips, since another one is - already handling it). Writing the hash as soon as the lease is acquired - would be a narrower window with a worse failure mode: a process killed - outright (OOM, a container stopped mid-install) between that write and - actually finishing would leave the hash pointing at content that was - never installed, and - unlike a normal exception - a hard kill does not - run `finally`, so nothing would ever revert it. The next check would - then see the hash already matches and skip forever, until the plugins - file changes again. Persisting only on success means a kill at any - point simply leaves the previous hash in place, so the next check - retries normally. + - The hash is only persisted *after* a successful install + - Store the hash into the database + - Also store the hash as a local marker file in the current environment + """ - from plugin.installer import install_plugins_file, plugins_file_hash + from plugin.installer import ( + get_env_plugin_hash, + install_plugins_file, + plugins_file_hash, + set_env_plugin_hash, + ) file_hash = plugins_file_hash() if file_hash is None: return - current_hash = get_global_setting( - '_PLUGIN_FILE_HASH', '', create=False, cache=False - ) + def already_satisfied() -> bool: + """True only if the database *and* this environment agree it's installed.""" + current_hash = get_global_setting( + '_PLUGIN_FILE_HASH', '', create=False, cache=False + ) + return current_hash == file_hash and get_env_plugin_hash() == file_hash - if current_hash == file_hash: + if already_satisfied(): return if not try_acquire_lease('_PLUGIN_FILE_HASH'): @@ -716,15 +715,12 @@ class PluginsRegistry: try: # Re-check under the lease: another process may have already # installed this exact change while we were waiting to acquire it - current_hash = get_global_setting( - '_PLUGIN_FILE_HASH', '', create=False, cache=False - ) - - if current_hash == file_hash: + if already_satisfied(): return if install_plugins_file() is not False: set_global_setting('_PLUGIN_FILE_HASH', file_hash) + set_env_plugin_hash(file_hash) finally: release_lease('_PLUGIN_FILE_HASH') diff --git a/src/backend/InvenTree/plugin/test_plugin.py b/src/backend/InvenTree/plugin/test_plugin.py index d4a54b01f4..36c5de42df 100644 --- a/src/backend/InvenTree/plugin/test_plugin.py +++ b/src/backend/InvenTree/plugin/test_plugin.py @@ -753,14 +753,22 @@ class RegistryTests(TestQueryMixin, PluginRegistryMixin, TestCase): ) return True - with mock.patch( - 'plugin.installer.plugins_file_hash', return_value='test-hash-1' - ): - with mock.patch( + with ( + tempfile.TemporaryDirectory() as tmpdir, + mock.patch('plugin.installer._env_plugin_hash_cache', None), + mock.patch( + 'plugin.installer.plugin_env_marker_path', + return_value=Path(tmpdir) / '.inventree_plugins_hash', + ), + mock.patch( + 'plugin.installer.plugins_file_hash', return_value='test-hash-1' + ), + mock.patch( 'plugin.installer.install_plugins_file', side_effect=fake_install_plugins_file, - ): - registry.install_plugin_file() + ), + ): + registry.install_plugin_file() # While the install was in progress, the hash must not yet reflect the # new (not-yet-installed) value @@ -778,13 +786,19 @@ class RegistryTests(TestQueryMixin, PluginRegistryMixin, TestCase): set_global_setting('_PLUGIN_FILE_HASH', '') - with mock.patch( - 'plugin.installer.plugins_file_hash', return_value='test-hash-2' + with ( + tempfile.TemporaryDirectory() as tmpdir, + mock.patch('plugin.installer._env_plugin_hash_cache', None), + mock.patch( + 'plugin.installer.plugin_env_marker_path', + return_value=Path(tmpdir) / '.inventree_plugins_hash', + ), + mock.patch( + 'plugin.installer.plugins_file_hash', return_value='test-hash-2' + ), + mock.patch('plugin.installer.install_plugins_file', return_value=False), ): - with mock.patch( - 'plugin.installer.install_plugins_file', return_value=False - ): - registry.install_plugin_file() + registry.install_plugin_file() self.assertEqual(get_global_setting('_PLUGIN_FILE_HASH', '', create=False), '') @@ -792,19 +806,104 @@ class RegistryTests(TestQueryMixin, PluginRegistryMixin, TestCase): """Test that install_plugin_file() is a no-op once the hash already matches.""" from plugin.registry import registry - with mock.patch( - 'plugin.installer.plugins_file_hash', return_value='test-hash-3' - ): - with mock.patch( + with ( + tempfile.TemporaryDirectory() as tmpdir, + mock.patch('plugin.installer._env_plugin_hash_cache', None), + mock.patch( + 'plugin.installer.plugin_env_marker_path', + return_value=Path(tmpdir) / '.inventree_plugins_hash', + ), + mock.patch( + 'plugin.installer.plugins_file_hash', return_value='test-hash-3' + ), + mock.patch( 'plugin.installer.install_plugins_file', return_value=True - ) as mock_install: - registry.install_plugin_file() - mock_install.assert_called_once() + ) as mock_install, + ): + registry.install_plugin_file() + mock_install.assert_called_once() - # A second call with the same (already-installed) hash must not - # install again - registry.install_plugin_file() - mock_install.assert_called_once() + # A second call with the same (already-installed) hash must not + # install again + registry.install_plugin_file() + mock_install.assert_called_once() + + def test_install_plugin_file_reinstalls_in_fresh_environment(self): + """Test that a matching database hash alone does not skip installation. + + Regression test for inventree/InvenTree#12848: a fresh python + environment (e.g. a container replaced without a persistent venv + volume) must reinstall even though the database still remembers a + previous environment's successful install of the exact same file. + """ + from common.settings import set_global_setting + from plugin.registry import registry + + # Simulate a database that already believes this hash is installed - + # as if a *previous* (now-replaced) environment installed it + set_global_setting('_PLUGIN_FILE_HASH', 'test-hash-4') + + with ( + tempfile.TemporaryDirectory() as tmpdir, + mock.patch('plugin.installer._env_plugin_hash_cache', None), + mock.patch( + 'plugin.installer.plugin_env_marker_path', + # A fresh environment has no marker file at all + return_value=Path(tmpdir) / '.inventree_plugins_hash', + ), + mock.patch( + 'plugin.installer.plugins_file_hash', return_value='test-hash-4' + ), + mock.patch( + 'plugin.installer.install_plugins_file', return_value=True + ) as mock_install, + ): + registry.install_plugin_file() + mock_install.assert_called_once() + + # Now that this environment has recorded the install, a second + # call is correctly skipped + registry.install_plugin_file() + mock_install.assert_called_once() + + def test_install_plugin_file_falls_back_to_process_cache_when_marker_unwritable( + self, + ): + """Test that a non-writable marker location still settles within one process. + + If sys.prefix is not writable (e.g. a read-only root filesystem), + set_env_plugin_hash() cannot persist the marker file to disk. Without + an in-process fallback, get_env_plugin_hash() would then return None + forever, and install_plugin_file() would re-attempt `pip install` on + every single call within the same process - not just once per + process start. + """ + from common.settings import set_global_setting + from plugin.registry import registry + + set_global_setting('_PLUGIN_FILE_HASH', '') + + marker = mock.MagicMock() + marker.exists.return_value = False + marker.write_text.side_effect = OSError('Read-only file system') + + with ( + mock.patch('plugin.installer._env_plugin_hash_cache', None), + mock.patch('plugin.installer.plugin_env_marker_path', return_value=marker), + mock.patch( + 'plugin.installer.plugins_file_hash', return_value='test-hash-5' + ), + mock.patch( + 'plugin.installer.install_plugins_file', return_value=True + ) as mock_install, + ): + registry.install_plugin_file() + mock_install.assert_called_once() + + # The marker file could not be written, but the in-memory cache + # still settles this process - a second call must not reinstall + registry.install_plugin_file() + mock_install.assert_called_once() def test_builtin_mandatory_plugins(self): """Test that mandatory builtin plugins are always loaded."""