From c38c507aed89c1bc2ef86824673a1542e3236097 Mon Sep 17 00:00:00 2001 From: Tom Elliott Date: Wed, 28 Oct 2026 08:19:42 -0500 Subject: [PATCH 1/2] Settings: a failed read no longer warns, and the supervisor reconnects to log PDODB::get() answers false when a read fails. getSetting() and _loadAllSettings() cast that to an array and iterated it; (array) false is [false], so each failed read wrote "Trying to access array offset on false" at FOGBase.php:3747 and :3752. A failed bulk load also emptied the cache in refreshSettingsCache() and could persist an empty cache file. Both loaders now treat a non-array result as a failure: a stale entry keeps its value and is retried next call, and a failed bulk load replaces nothing. The daemon supervisor is where the failures came from. On RC18 it shares a socket with its child, so its reads fail while the child starts or stops. Since #1930 it closes that socket before each fork, but its next log line can still read a setting -- the log gate, once the cache entry is past its TTL. The #1930 commit said the supervisor makes no queries after the fork; that was wrong. Service_Log_message() now reopens the link first, the same call the child makes. Service_persist() still closes it before every fork, so no child inherits it. tests/settings-read-failure.test.php runs the real FOGBase on a DB stub that fails every read; on the unfixed tree it reports the reporter's exact warnings. A shadow-tree run of the real Service_Log_message() after disconnect(): unfixed, 3 warnings and the link left closed; fixed, none and the link open. Fixes #1994 Co-Authored-By: Claude Opus 5.5 (cherry picked from commit 8e0b3c2e3b309daec0a1365e76d3ffe3777a4f4e) --- packages/service/lib/service_lib.php | 7 ++ packages/web/src/Base/FOGBase.php | 29 ++++++- tests/settings-read-failure.test.php | 115 +++++++++++++++++++++++++++ 3 files changed, 149 insertions(+), 2 deletions(-) create mode 100644 tests/settings-read-failure.test.php diff --git a/packages/service/lib/service_lib.php b/packages/service/lib/service_lib.php index 4f0f6fb22b..b60bc82a8e 100644 --- a/packages/service/lib/service_lib.php +++ b/packages/service/lib/service_lib.php @@ -105,6 +105,13 @@ */ function Service_Log_message($logpath, $name, $msg) { + // The supervisor closes its connection before every fork (#1930), but + // formatting the time below can still read a setting -- the log gate + // behind it, once the cache entry is past its TTL. On a closed link that + // read failed and spilled warnings into this log (GH-1994). Reopen it; + // a no-op when connected, and Service_persist() closes it again before + // the next fork, so no child inherits it. + DatabaseManager::getLink(); $msg = sprintf( "[%s] %s %s\n", FOGCore::formatTime('now', 'm-d-y g:i:s a'), diff --git a/packages/web/src/Base/FOGBase.php b/packages/web/src/Base/FOGBase.php index 9efad6b6bd..700b50fa3f 100644 --- a/packages/web/src/Base/FOGBase.php +++ b/packages/web/src/Base/FOGBase.php @@ -3528,7 +3528,8 @@ private static function _useSettingsFileCache() * Load every global setting from the database in a single query, applying * the same normalization as getSetting(). * - * @return array Map of settingKey => normalized value. + * @return array|null Map of settingKey => normalized value, or null when + * the read failed. */ private static function _loadAllSettings() { @@ -3538,6 +3539,14 @@ private static function _loadAllSettings() "SELECT `settingKey`, `settingValue` FROM `globalSettings`" )->fetch(\PDO::FETCH_ASSOC, 'fetch_all')->get(); + // A failed read answers false, not an empty set. (array) false is + // [false], so iterating it walked one "row" that was false itself + // (GH-1994). null tells the callers not to replace what they hold + // with an empty map -- query() has already logged the cause. + if (!is_array($rows)) { + return null; + } + $data = []; foreach ((array) $rows as $row) { $data[$row['settingKey']] = trim( @@ -3675,6 +3684,11 @@ private static function _warmFromFileCache() // Missing, stale, or flushed: rebuild from the database and persist. ++self::$_settingsCacheQueries; $data = self::_loadAllSettings(); + if (null === $data) { + // Leave the file alone: siblings would read an empty map as + // "no settings" for a whole TTL. getSetting() reads per key. + return; + } foreach ($data as $k => $v) { self::$_settingsCache[$k] = ['value' => $v, 'ts' => $now]; } @@ -3743,7 +3757,14 @@ public static function getSetting($key) $rows = self::$DB->query($sql) ->fetch(\PDO::FETCH_ASSOC, 'fetch_all')->get(); - foreach ((array) $rows as $row) { + // A failed read answers false; (array) false is [false], one + // "row" that is false itself (GH-1994). Skip it: a stale entry + // keeps its value and is retried on the next call, and query() + // has already logged the cause. + if (!is_array($rows)) { + $rows = []; + } + foreach ($rows as $row) { self::$_settingsCache[$row['settingKey']] = [ 'value' => trim( str_replace( @@ -3850,6 +3871,10 @@ public static function refreshSettingsCache() { $now = time(); $data = self::_loadAllSettings(); + if (null === $data) { + // A failed read must not empty the cache it was meant to refresh. + return 0; + } self::$_settingsCache = []; foreach ($data as $k => $v) { diff --git a/tests/settings-read-failure.test.php b/tests/settings-read-failure.test.php new file mode 100644 index 0000000000..919181229f --- /dev/null +++ b/tests/settings-read-failure.test.php @@ -0,0 +1,115 @@ +fail ? false : $this->rows; + } +} + +$db = new FailingDB(); +$db->rows = [['settingKey' => 'FOG_GH1994', 'settingValue' => 'kept']]; +$prop = new ReflectionProperty('FOG\Base\FOGBase', 'DB'); +$prop->setAccessible(true); +$prop->setValue(null, $db); + +$warnings = []; +set_error_handler(function ($no, $str, $file, $line) use (&$warnings) { + $warnings[] = "$str at " . basename($file) . ":$line"; + return true; +}); + +$fails = []; +$base = 'FOG\Base\FOGBase'; + +// Prime the cache while the database answers. +if ('kept' !== $base::getSetting('FOG_GH1994')) { + $fails[] = 'getSetting() did not read the stub row; the test cannot see its subject'; +} + +// Every entry is now stale, and every read fails. +$base::$settingsCacheTTL = 0; +$db->fail = true; + +$missing = $base::getSetting('FOG_GH1994_ABSENT'); +if (null !== $missing) { + $fails[] = 'a failed read of an uncached key answered ' . var_export($missing, true); +} +if ('kept' !== $base::getSetting('FOG_GH1994')) { + $fails[] = 'a failed read dropped the stale value it was refreshing'; +} +$loaded = $base::refreshSettingsCache(); +if (0 !== $loaded) { + $fails[] = "refreshSettingsCache() reported $loaded settings from a failed read"; +} +if ('kept' !== $base::getSetting('FOG_GH1994')) { + $fails[] = 'a failed refreshSettingsCache() emptied the cache'; +} +foreach ($warnings as $w) { + $fails[] = "warning: $w"; +} + +restore_error_handler(); +array_map('unlink', glob($tmp . '/*') ?: []); +@unlink($tmp . '/.settings_cache_flush'); +@rmdir($tmp); + +if ($fails) { + foreach ($fails as $f) { + fwrite(STDERR, "FAIL: $f\n"); + } + exit(1); +} +echo "OK: a failed settings read warns nothing and keeps the cache\n"; +exit(0); From 3017e5d8066de6ce5a4725cd2fcec500b77d8205 Mon Sep 17 00:00:00 2001 From: Tom Elliott Date: Wed, 28 Oct 2026 08:24:32 -0500 Subject: [PATCH 2/2] Test: prefix the built-in ReflectionProperty with a backslash global-class-prefix requires it. Co-Authored-By: Claude Opus 5.5 (cherry picked from commit 94ed3588dd681205a3cb4c86ac2284f584ccf786) --- tests/settings-read-failure.test.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/settings-read-failure.test.php b/tests/settings-read-failure.test.php index 919181229f..94762619eb 100644 --- a/tests/settings-read-failure.test.php +++ b/tests/settings-read-failure.test.php @@ -60,7 +60,7 @@ public function get() $db = new FailingDB(); $db->rows = [['settingKey' => 'FOG_GH1994', 'settingValue' => 'kept']]; -$prop = new ReflectionProperty('FOG\Base\FOGBase', 'DB'); +$prop = new \ReflectionProperty('FOG\Base\FOGBase', 'DB'); $prop->setAccessible(true); $prop->setValue(null, $db);