From adb36d8a5b2c0c33285fb0167b26454ec015ffe9 Mon Sep 17 00:00:00 2001 From: Divarion_D Date: Tue, 22 Sep 2026 16:21:51 +0300 Subject: [PATCH] fix(modules): unblock disabling, contain lifecycle failures, keep one dir per module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Disabling a module whose own dependency was already off was impossible: the guard counted a dependent's nominal Enabled state, while ModuleLoader prunes unsatisfiable modules transitively. A dependent sitting on an already-broken chain is not running, so it must not block. The guard now mirrors the loader's prune and only counts dependents that would genuinely stop working. Alongside that, three lifecycle holes found while reviewing ModuleManager: - updateModule() had no failure handling and no incremental watermark. A migration failing half-way left the module Enabled at the old version, so a retry replayed already-applied deltas — a non-idempotent one then failed forever. File deltas and programmatic migrations are now merged into one ascending timeline, the recorded version advances per COMPLETED version, and a failure marks the module Failed. ModuleMigrator gains pending() and a public applyFile(); up() is reimplemented on top of them, unchanged. - uninstallModule() had no try/catch, unlike installModule(). A failing uninstall() hook left the module installed and Enabled with half-deleted data. It now marks the module Failed. The duplicated dependents guard moves into assertNoInstalledDependents(), shared with deleteModule(). - Platform (store) installs assumed a bare modules/{slug} directory while every other path uses {name}_{hash5}. The backup step then found nothing and the pulled copy landed beside the existing install, leaving two manifests for one module. The platform flow now reuses placeModuleFiles(), which drops rival copies and moves the source into place instead of copying it; listModules() de-duplicates by name so a stray copy cannot show the module twice. archivesPath now sits beside modulesPath rather than resolving under MAIN_HOME, so an injected modules path is honoured (identical path in production). Verified: 852 tests, make gates, CRAP gate. --- src/Core/Module/ModuleManager.php | 369 +++++++++++++++------ src/Core/Module/ModuleMigrator.php | 45 ++- tests/Unit/ModuleManagerMigrationsTest.php | 187 +++++++++++ 3 files changed, 489 insertions(+), 112 deletions(-) diff --git a/src/Core/Module/ModuleManager.php b/src/Core/Module/ModuleManager.php index b8095cd5..dea8c185 100644 --- a/src/Core/Module/ModuleManager.php +++ b/src/Core/Module/ModuleManager.php @@ -53,7 +53,11 @@ class ModuleManager { ) { $this->modulesPath = $modulesPath ?: (defined('MAIN_HOME') ? MAIN_HOME . 'Modules' : dirname(__DIR__, 2) . '/Modules'); $this->overridesPath = $overridesPath ?: (defined('CONFIG_PATH') ? CONFIG_PATH . 'modules.php' : dirname(__DIR__, 2) . '/config/modules.php'); - $this->archivesPath = defined('MAIN_HOME') ? MAIN_HOME . 'modules_archives' : dirname(__DIR__, 2) . '/modules_archives'; + // Sits beside the modules directory (same idiom as the .module_backups + // area) so it follows an injected modulesPath instead of always resolving + // under MAIN_HOME. Identical path in production, where modulesPath is + // MAIN_HOME . 'Modules'. + $this->archivesPath = dirname($this->modulesPath) . '/modules_archives'; $this->container = $container; } @@ -184,24 +188,48 @@ class ModuleManager { $name = $this->sanitizeModuleName($this->manifestNameFromDir($moduleDir)); // Guarantee a hash_id (generating + persisting one when the upload lacks it) // so the module is always placed in a `{name}_{hash5}` directory, never bare. - $hash = $this->ensureHashId($moduleDir); + $targetDir = $this->modulesPath . '/' . $this->moduleDirName($name, $this->ensureHashId($moduleDir)); - // Remove any current install of the same module (may live under a different - // {name}_{hash5} or a legacy {name} directory). - $existing = $this->modulePathFor($name); - if (is_dir($existing)) { - $this->deleteDirectory($existing); + // Remove every other copy of this module (a legacy bare `{name}` directory, + // or an install under a different hash). The source is skipped on purpose: a + // platform pull extracts straight INTO the modules directory, and deleting + // it here would destroy the very files being placed. + $this->dropRivalModuleDirs($name, $moduleDir); + + if (realpath($targetDir) === realpath($moduleDir)) { + return $name; } - - $targetDir = $this->modulesPath . '/' . $this->moduleDirName($name, $hash); - if (is_dir($targetDir)) { - $this->deleteDirectory($targetDir); + if (!@rename($moduleDir, $targetDir)) { + $this->copyDirectory($moduleDir, $targetDir); } - $this->copyDirectory($moduleDir, $targetDir); - return $name; } + /** Remove every on-disk copy of $name except $keep — one install per name. */ + private function dropRivalModuleDirs(string $name, string $keep): void { + $keepReal = realpath($keep); + foreach ($this->moduleDirsFor($name) as $dir) { + if (realpath($dir) !== $keepReal) { + $this->deleteDirectory($dir); + } + } + } + + /** + * Every directory under modulesPath whose manifest declares $name. + * + * @return string[] + */ + private function moduleDirsFor(string $name): array { + $out = []; + foreach (glob($this->modulesPath . '/*', GLOB_ONLYDIR) ?: [] as $dir) { + if (is_file($dir . '/module.json') && $this->manifestNameFromDir($dir) === $name) { + $out[] = $dir; + } + } + return $out; + } + /** * Retire legacy hash-less module directories: rename every bare `{name}` * directory to the canonical `{name}_{hash5}`, generating a hash_id when the @@ -299,33 +327,71 @@ class ModuleManager { } /** - * Names of currently-loadable (enabled) installed modules that declare $name - * as a required dependency. + * Names of installed dependents that would actually stop working if $name + * were disabled. * - * Used to guard disabling: a dependent that is itself disabled won't be loaded - * either, so disabling $name under it is harmless and must not be blocked. - * Only enabled dependents would be broken (ModuleLoader skips them on the next - * boot), so only those count. + * Used to guard disabling. Only a dependent that currently loads can break, + * and being nominally Enabled is not enough: ModuleLoader drops any module + * whose required dependency is unavailable and cascades that transitively + * (see ModuleLoader::pruneUnsatisfiableModules), so a dependent sitting on an + * already-broken chain is not running either. Counting it would make a module + * whose own dependency is disabled impossible to disable, for no runtime gain. * * @return string[] */ - private function enabledDependentsOf(string $name): array { + private function breakableDependentsOf(string $name): array { $out = []; - foreach ($this->listModules() as $module) { - if (($module['installed_version'] ?? '') === '') { + foreach ($this->loadableModules() as $module) { + if ($module['installed_version'] === '') { continue; // not installed — its requirements don't apply } - $state = $module['state'] ?? null; - if (!($state instanceof ModuleState) || !$state->isLoadable()) { - continue; // already disabled/failed — disabling its dep won't break it - } - if (in_array($name, $module['dependencies'] ?? [], true)) { + if (in_array($name, $module['dependencies'], true)) { $out[] = $module['name']; } } return $out; } + /** + * Modules that ModuleLoader would actually boot right now, keyed by name. + * + * @return array listModules() entries that are enabled and + * whose required dependency chain is satisfied. + */ + private function loadableModules(): array { + $enabled = []; + foreach ($this->listModules() as $module) { + if ($module['state']->isLoadable()) { + $enabled[$module['name']] = $module; + } + } + return $this->pruneUnsatisfiable($enabled); + } + + /** + * Drop every module with a required dependency outside the set, repeating + * until it settles — the same transitive cascade ModuleLoader applies. + * + * @param array $modules Candidate modules keyed by name. + * @return array + */ + private function pruneUnsatisfiable(array $modules): array { + do { + $removed = false; + foreach ($modules as $name => $module) { + foreach ($module['dependencies'] as $dependency) { + if (!isset($modules[$dependency])) { + unset($modules[$name]); + $removed = true; + break; + } + } + } + } while ($removed); + + return $modules; + } + /** * Install any on-disk module that has never been installed. * @@ -617,7 +683,7 @@ class ModuleManager { $overrides = $this->readOverrides(); $items = []; - $jsonFiles = glob($this->modulesPath . '/*/module.json') ?: []; + $jsonFiles = $this->dedupeManifestFiles(glob($this->modulesPath . '/*/module.json') ?: []); // Pre-resolve each module's state by name so the dependency diagnostics // below can see the full set while building items. @@ -686,6 +752,28 @@ class ModuleManager { return $items; } + /** + * Keep one manifest per module name, preferring the canonical directory. + * + * A module should occupy exactly one directory, but a legacy bare `{name}` + * copy can linger beside the canonical `{name}_{hash5}` one; listing both + * would show the module twice in the admin table. The hashed directory wins. + * + * @param string[] $jsonFiles + * @return string[] + */ + private function dedupeManifestFiles(array $jsonFiles): array { + $byName = []; + foreach ($jsonFiles as $jsonFile) { + $dir = dirname($jsonFile); + $name = $this->manifestNameFromDir($dir); + if (!isset($byName[$name]) || basename($dir) !== $name) { + $byName[$name] = $jsonFile; + } + } + return array_values($byName); + } + /** * Install a module by name. * @@ -738,32 +826,51 @@ class ModuleManager { */ public function uninstallModule(string $name): void { $name = $this->sanitizeModuleName($name); - - // Refuse to remove a module that still-installed dependents rely on - // (e.g. plex depends on watch — watch cannot be removed under it). - $dependents = $this->installedDependentsOf($name); - if ($dependents !== []) { - throw new \RuntimeException( - "Cannot uninstall '{$name}': still required by " . implode(', ', $dependents) - . '. Uninstall ' . (count($dependents) === 1 ? 'it' : 'them') . ' first.' - ); - } + $this->assertNoInstalledDependents($name, 'uninstall'); $module = $this->loadModuleInstance($name); // The module's own uninstall() hook runs first (it clears the data/rows // it created), then the module's schema is torn down via its single - // teardown file (database_drop.sql). - $module->uninstall(); - $db = $this->getDb() ?? DatabaseFactory::get(); - if ($db !== null) { - ModuleMigrator::uninstall($this->modulePathFor($name), $db); + // teardown file (database_drop.sql). A failure between the two would + // otherwise leave the module recorded as installed and Enabled with + // half-deleted data, so mark it Failed and let the admin see it. + try { + $module->uninstall(); + $db = $this->getDb() ?? DatabaseFactory::get(); + if ($db !== null) { + ModuleMigrator::uninstall($this->modulePathFor($name), $db); + } + } catch (\Throwable $e) { + $this->setState($name, ModuleState::Failed); + throw $e; } $this->clearInstalledVersion($name); $this->setState($name, ModuleState::Disabled); } + /** + * Refuse an action that would strip a module out from under installed + * dependents (e.g. plex depends on watch — watch cannot be removed under it). + * + * Unlike the disable guard this counts every INSTALLED dependent regardless of + * state: a disabled dependent still owns rows that reference this module's + * tables, and uninstalling drops them. + * + * @param string $verb Lowercase action name used in the message ('uninstall', 'delete'). + */ + private function assertNoInstalledDependents(string $name, string $verb): void { + $dependents = $this->installedDependentsOf($name); + if ($dependents === []) { + return; + } + throw new \RuntimeException( + "Cannot {$verb} '{$name}': still required by " . implode(', ', $dependents) + . '. ' . ucfirst($verb) . ' ' . (count($dependents) === 1 ? 'it' : 'them') . ' first.' + ); + } + /** * Fully delete a module: uninstall it, then remove its directory from disk and * its override entry from config/modules.php. @@ -786,13 +893,7 @@ class ModuleManager { $name = $this->sanitizeModuleName($name); // Same guard as uninstall: refuse while an installed dependent needs it. - $dependents = $this->installedDependentsOf($name); - if ($dependents !== []) { - throw new \RuntimeException( - "Cannot delete '{$name}': still required by " . implode(', ', $dependents) - . '. Delete ' . (count($dependents) === 1 ? 'it' : 'them') . ' first.' - ); - } + $this->assertNoInstalledDependents($name, 'delete'); // Read the manifest (for LB propagation) BEFORE the files are removed. $manifest = []; @@ -910,26 +1011,107 @@ class ModuleManager { } $module = $this->loadModuleInstance($name); - $toVersion = $this->manifestVersion($name) ?? $module->getVersion(); + $toVersion = (string) ($this->manifestVersion($name) ?? $module->getVersion()); if (version_compare($fromVersion, $toVersion, '>=')) { return; } - // File-based schema migrations: every up file in (fromVersion, toVersion]. - $db = $this->getDb() ?? DatabaseFactory::get(); - if ($db !== null) { - ModuleMigrator::up($this->modulePathFor($name), $db, $fromVersion, (string) $toVersion); - } - - // Programmatic migrations (callables) — coexist with the file-based ones. - if ($module instanceof MigratableInterface) { - $this->runPendingMigrations($module->getMigrations(), $fromVersion, $toVersion); - } - + $this->applyUpdateSteps($name, $this->pendingUpdateSteps($name, $module, $fromVersion, $toVersion)); $this->recordInstalledVersion($name, $toVersion); } + /** + * Run each version's update steps, advancing the recorded version only once a + * version is fully applied. + * + * The watermark moves per COMPLETED version rather than once at the end, so a + * failure half-way leaves the module Failed at the last version that actually + * landed and a retry resumes from there. Without that, a retry replays deltas + * that already ran — and a non-idempotent one (`ALTER TABLE ADD COLUMN`) then + * fails forever, wedging the module's updates. + * + * @param array> $steps version => steps, ascending. + */ + private function applyUpdateSteps(string $name, array $steps): void { + try { + foreach ($steps as $version => $versionSteps) { + foreach ($versionSteps as $step) { + $step(); + } + $this->recordInstalledVersion($name, $version); + } + } catch (\Throwable $e) { + $this->setState($name, ModuleState::Failed); + throw $e; + } + } + + /** + * Everything still to run to get from $from to $to, grouped by version. + * + * Merges the two migration systems — file deltas and programmatic callables — + * into one ascending timeline so a version's schema change and its code hook + * are applied together and watermarked together. + * + * @return array> version => steps, ascending. + */ + private function pendingUpdateSteps(string $name, ModuleInterface $module, string $from, string $to): array { + $steps = $this->pendingSchemaSteps($this->modulePathFor($name), $from, $to); + foreach ($this->pendingCallableSteps($module, $from, $to) as $version => $step) { + $steps[$version][] = $step; + } + uksort($steps, 'version_compare'); + return $steps; + } + + /** + * File-based schema deltas in (`$from`, `$to`], one step per version. + * + * @return array> + */ + private function pendingSchemaSteps(string $modulePath, string $from, string $to): array { + $db = $this->getDb() ?? DatabaseFactory::get(); + if ($db === null) { + return []; + } + + $steps = []; + foreach (ModuleMigrator::pending($modulePath, $from, $to) as $version => $file) { + $steps[$version] = [static fn() => ModuleMigrator::applyFile($db, $file)]; + } + return $steps; + } + + /** + * Programmatic migrations declared by the module in (`$from`, `$to`]. + * + * @return array + */ + private function pendingCallableSteps(ModuleInterface $module, string $from, string $to): array { + if (!$module instanceof MigratableInterface) { + return []; + } + + $steps = []; + foreach ($module->getMigrations() as $version => $callable) { + if (version_compare($version, $from, '>') && version_compare($version, $to, '<=')) { + $steps[$version] = fn() => $this->runMigration($callable); + } + } + return $steps; + } + + /** Run one programmatic migration, in a transaction when the handler supports it. */ + private function runMigration(callable $callable): void { + $db = $this->getDb(); + if ($db !== null && method_exists($db, 'transactional')) { + $db->transactional(fn() => $callable($this->container)); + return; + } + $callable($this->container); + } + /** * Update a module by fetching new files from its declared source, then running * migrations. This is what the panel's "Update" button triggers (P4). @@ -1126,14 +1308,14 @@ class ModuleManager { public function setState(string $name, ModuleState $state): void { $name = $this->sanitizeModuleName($name); - // Refuse to disable a module that still-enabled dependents rely on + // Refuse to disable a module that a still-working dependent relies on // (e.g. plex requires watch — watch cannot be disabled under it, or // ModuleLoader would skip plex on the next boot). Mirrors the guard in // uninstallModule(). Scoped strictly to a deliberate Disabled transition: // the internal lifecycle states (Installing, Failed) are also non-loadable // but are set by installModule() itself and must never be blocked. if ($state === ModuleState::Disabled) { - $dependents = $this->enabledDependentsOf($name); + $dependents = $this->breakableDependentsOf($name); if ($dependents !== []) { throw new \RuntimeException( "Cannot disable '{$name}': still required by " . implode(', ', $dependents) @@ -1393,8 +1575,12 @@ class ModuleManager { * @throws \RuntimeException If the C extension is missing, download fails, or install fails. */ public function downloadFromPlatform(string $slug, string $version = '', ?string $apiKey = null): void { - $slug = $this->sanitizeModuleName($slug); - $targetDir = $this->modulesPath . '/' . $slug; + $slug = $this->sanitizeModuleName($slug); + // Resolve the module's ACTUAL directory (canonical `{name}_{hash5}`, or a + // legacy bare one) rather than assuming the bare form: otherwise the backup + // below finds nothing and the pulled copy lands beside the existing install + // instead of replacing it. + $targetDir = $this->modulePathFor($slug); // Snapshot current state so a failed (re)install rolls back cleanly. We // MOVE the existing module aside (outside modulesPath so the loader never @@ -1404,8 +1590,12 @@ class ModuleManager { $backupDir = $this->backupModuleDir($slug, $targetDir); try { - $result = $this->pullFilesFromPlatform($slug, $version, $apiKey); - $modulePath = $result['path']; + $result = $this->pullFilesFromPlatform($slug, $version, $apiKey); + // The extension extracts wherever it likes; fold that into the canonical + // `{name}_{hash5}` directory and track it as the rollback target below. + $this->placeModuleFiles($result['path']); + $modulePath = $this->modulePathFor($slug); + $targetDir = $modulePath; $resolvedVersion = (string) ($result['version'] ?: $version); // Acquire the per-machine ionCube license BEFORE installModule(): if the @@ -1558,7 +1748,7 @@ class ModuleManager { return false; } - $dir = $this->modulesPath . '/' . $slug; + $dir = $this->modulePathFor($slug); if (!is_dir($dir)) { error_log("ModuleManager: module dir missing for '{$slug}': {$dir}"); return false; @@ -1661,17 +1851,19 @@ class ModuleManager { */ public function deployFromPlatformFilesOnly(string $slug, string $version, ?string $apiKey = null): void { $result = $this->pullFilesFromPlatform($slug, $version, $apiKey); + $this->placeModuleFiles($result['path']); + $modulePath = $this->modulePathFor($slug); EventDispatcher::dispatch(new PackageInstalledEvent( slug: $result['module'], version: $result['version'], - path: $result['path'], + path: $modulePath, installedAt: time(), )); $this->recordInstalledVersion($slug, (string) ($result['version'] ?: $version)); $this->setState($slug, ModuleState::Enabled); - $this->hotReloadSafe($slug, $result['path']); + $this->hotReloadSafe($slug, $modulePath); } /** @@ -1710,7 +1902,7 @@ class ModuleManager { 'ok' => true, 'module' => $result['module'] ?? $slug, 'version' => $result['version'] ?? $version, - 'path' => $result['path'] ?? ($this->modulesPath . '/' . $slug), + 'path' => $result['path'] ?? $this->modulePathFor($slug), // Previous approved version reported by the platform (for the // Rollback button). May be absent/null when there is no prior version. 'previous_version' => $result['previous_version'] ?? null, @@ -1767,35 +1959,6 @@ class ModuleManager { $loader->bootAll($container, $router instanceof Router ? $router : null); } - /** - * Run migrations whose target version falls in (fromVersion, toVersion]. - * - * @param array $migrations - * @param string $fromVersion Currently installed version (exclusive lower bound). - * @param string $toVersion New version (inclusive upper bound). - */ - private function runPendingMigrations(array $migrations, string $fromVersion, string $toVersion): void { - $pending = []; - foreach ($migrations as $version => $callable) { - if (version_compare($version, $fromVersion, '>') - && version_compare($version, $toVersion, '<=') - ) { - $pending[$version] = $callable; - } - } - - uksort($pending, 'version_compare'); - - $db = $this->getDb(); - foreach ($pending as $callable) { - if ($db !== null && method_exists($db, 'transactional')) { - $db->transactional(fn() => $callable($this->container)); - } else { - $callable($this->container); - } - } - } - /** * Persist the installed version for a module in config/modules.php. * @@ -2021,6 +2184,10 @@ class ModuleManager { /** Extract .tar/.tar.gz via PharData (no PHP extension needed) or the `tar` CLI. */ private function extractTarArchive(string $archivePath, string $destination): void { if (class_exists('PharData')) { + // No per-entry check here (unlike the zip branch): PharData resolves + // members itself and refuses to write outside $destination — verified + // against a hand-built tar carrying a `../` member, which it parses but + // neither exposes nor extracts. try { (new \PharData($archivePath))->extractTo($destination, null, true); return; diff --git a/src/Core/Module/ModuleMigrator.php b/src/Core/Module/ModuleMigrator.php index 02dc3821..e8ca2dc6 100644 --- a/src/Core/Module/ModuleMigrator.php +++ b/src/Core/Module/ModuleMigrator.php @@ -52,7 +52,7 @@ class ModuleMigrator { public static function install(string $modulePath, object $db, string $to): array { $master = $modulePath . '/database.sql'; if (is_file($master)) { - self::runFile($db, $master); + self::applyFile($db, $master); return ['database.sql']; } // No master schema — replay forward deltas up to the target version. @@ -72,7 +72,7 @@ class ModuleMigrator { public static function uninstall(string $modulePath, object $db): array { $drop = $modulePath . '/database_drop.sql'; if (is_file($drop)) { - self::runFile($db, $drop); + self::applyFile($db, $drop); return ['database_drop.sql']; } return []; @@ -92,19 +92,42 @@ class ModuleMigrator { */ public static function up(string $modulePath, object $db, ?string $from, string $to): array { $applied = []; - foreach (self::discover($modulePath) as [$version, $file]) { - if ($from !== null && version_compare($version, $from, '<=')) { - continue; - } - if (version_compare($version, $to, '>')) { - continue; - } - self::runFile($db, $file); + foreach (self::pending($modulePath, $from, $to) as $version => $file) { + self::applyFile($db, $file); $applied[] = $version; } return $applied; } + /** + * Forward deltas still to apply for versions in the (`$from`, `$to`] range. + * + * Exposed separately from up() so a caller can apply them one version at a + * time and persist its own watermark between steps. + * + * @param string $modulePath Absolute path of the module directory. + * @param string|null $from Already-applied version, or null to list all ≤ $to. + * @param string $to Target version (inclusive). + * @return array version => absolute delta path, ascending. + */ + public static function pending(string $modulePath, ?string $from, string $to): array { + $out = []; + foreach (self::discover($modulePath) as [$version, $file]) { + if (self::isPending($version, $from, $to)) { + $out[$version] = $file; + } + } + return $out; + } + + /** Whether $version falls in the (`$from`, `$to`] range. */ + private static function isPending(string $version, ?string $from, string $to): bool { + if ($from !== null && version_compare($version, $from, '<=')) { + return false; + } + return version_compare($version, $to, '<='); + } + /** * Whether the module ships any schema files at all (master, teardown, or deltas). */ @@ -144,7 +167,7 @@ class ModuleMigrator { /** * Execute every statement in a SQL file. Throws on the first failure. */ - private static function runFile(object $db, string $file): void { + public static function applyFile(object $db, string $file): void { $sql = trim((string) file_get_contents($file)); if ($sql === '') { return; diff --git a/tests/Unit/ModuleManagerMigrationsTest.php b/tests/Unit/ModuleManagerMigrationsTest.php index 231f00d8..42854972 100644 --- a/tests/Unit/ModuleManagerMigrationsTest.php +++ b/tests/Unit/ModuleManagerMigrationsTest.php @@ -1,6 +1,7 @@ statements[] = $sql; + return $this->failOn === '' || strpos($sql, $this->failOn) === false; + } +} + final class ModuleManagerMigrationsTest extends TestCase { private string $modulesPath; private string $overridesPath; private string $workDir; + private FakeModuleDb $db; protected function setUp(): void { $this->workDir = sys_get_temp_dir() . '/xc_vm_mgr_' . bin2hex(random_bytes(6)); $this->modulesPath = $this->workDir . '/modules'; $this->overridesPath = $this->workDir . '/modules.php'; mkdir($this->modulesPath, 0775, true); + $this->db = new FakeModuleDb(); + ServiceContainer::getInstance()->set('db', $this->db); MigrationCallTracker::reset(); } protected function tearDown(): void { + ServiceContainer::getInstance()->remove('db'); $this->deleteDir($this->workDir); MigrationCallTracker::reset(); } @@ -331,6 +348,149 @@ final class ModuleManagerMigrationsTest extends TestCase { $this->assertSame([], $byName['ok-consumer']['dependency_warnings']); } + // ── update/uninstall failure containment ────────────────────────────── + + public function testUpdateKeepsWatermarkAtLastCompletedVersionOnFailure(): void { + // Deltas 1.1.0 and 1.2.0 pending; the second one fails. The recorded + // version must stop at 1.1.0 so a retry resumes instead of replaying it. + $this->createModule('resume-mod', '1.2.0'); + $dir = $this->modulesPath . '/resume-mod/migrations'; + mkdir($dir, 0775, true); + file_put_contents($dir . '/1.1.0.sql', 'SELECT 1;'); + file_put_contents($dir . '/1.2.0.sql', 'BOOM;'); + $this->writeOverrides(['resume-mod' => ['installed_version' => '1.0.0']]); + + $this->db->failOn = 'BOOM'; + + try { + $this->manager()->updateModule('resume-mod'); + $this->fail('expected the failing delta to throw'); + } catch (RuntimeException $e) { + // expected + } + + $overrides = $this->readOverrides(); + $this->assertSame('1.1.0', $overrides['resume-mod']['installed_version'] ?? null); + $this->assertSame('failed', $overrides['resume-mod']['state'] ?? null); + } + + public function testUpdateRecordsTargetVersionWhenEveryDeltaApplies(): void { + $this->createModule('resume-ok', '1.2.0'); + $dir = $this->modulesPath . '/resume-ok/migrations'; + mkdir($dir, 0775, true); + file_put_contents($dir . '/1.1.0.sql', 'SELECT 1;'); + file_put_contents($dir . '/1.2.0.sql', 'SELECT 2;'); + $this->writeOverrides(['resume-ok' => ['installed_version' => '1.0.0']]); + + $this->manager()->updateModule('resume-ok'); + + $overrides = $this->readOverrides(); + $this->assertSame('1.2.0', $overrides['resume-ok']['installed_version'] ?? null); + $this->assertArrayNotHasKey('state', $overrides['resume-ok']); + } + + public function testUninstallMarksModuleFailedWhenTeardownThrows(): void { + $this->createModule('teardown-mod', '1.0.0'); + file_put_contents($this->modulesPath . '/teardown-mod/database_drop.sql', 'BOOM;'); + $this->writeOverrides(['teardown-mod' => ['installed_version' => '1.0.0']]); + + $this->db->failOn = 'BOOM'; + + try { + $this->manager()->uninstallModule('teardown-mod'); + $this->fail('expected the failing teardown to throw'); + } catch (RuntimeException $e) { + // expected + } + + $overrides = $this->readOverrides(); + $this->assertSame('failed', $overrides['teardown-mod']['state'] ?? null); + // Still recorded as installed — its tables were not dropped. + $this->assertSame('1.0.0', $overrides['teardown-mod']['installed_version'] ?? null); + } + + // ── one directory per module ────────────────────────────────────────── + + public function testUploadPlacesModuleInHashedDirAndDropsLegacyBareCopy(): void { + // A stale bare `{name}` copy must not survive beside the canonical one — + // two dirs for one module is what made the platform flow lose track of it. + $legacy = $this->modulesPath . '/upl-mod'; + mkdir($legacy, 0775, true); + file_put_contents($legacy . '/module.json', json_encode(['name' => 'upl-mod', 'version' => '0.9.0'])); + + try { + $this->manager()->uploadAndInstall($this->makeModuleTar('upl-mod', '1.0.0', str_repeat('ab12', 8))); + } catch (Error $e) { + // uploadAndInstall ends by distributing to load balancers through the + // xcvm_core extension, which is absent here. Extraction, placement and + // install have all already run by then — that is what this asserts. + $this->assertStringContainsString('XC_VM', $e->getMessage()); + } + + $this->assertDirectoryExists($this->modulesPath . '/upl-mod_ab12a'); + $this->assertDirectoryDoesNotExist($legacy); + $this->assertSame('1.0.0', $this->readOverrides()['upl-mod']['installed_version'] ?? null); + } + + public function testListModulesShowsOneRowWhenBareAndHashedCopiesCoexist(): void { + // The platform flow used to extract into a bare `{name}` dir beside the + // canonical `{name}_{hash5}` one; the table then listed the module twice. + $this->createModuleWithDeps('dupe-mod', '1.0.0', []); + $hashed = $this->modulesPath . '/dupe-mod_ab123'; + mkdir($hashed, 0775, true); + copy($this->modulesPath . '/dupe-mod/module.json', $hashed . '/module.json'); + + $names = array_column($this->manager()->listModules(), 'name'); + + $this->assertSame(['dupe-mod'], $names); + } + + // ── setState() disable guard ────────────────────────────────────────── + + public function testDisableBlockedByWorkingDependent(): void { + $this->createModuleWithDeps('guard-base', '1.0.0', []); + $this->createModuleWithDeps('guard-consumer', '1.0.0', ['guard-base']); + $this->writeOverrides([ + 'guard-base' => ['installed_version' => '1.0.0'], + 'guard-consumer' => ['installed_version' => '1.0.0'], + ]); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('guard-consumer'); + $this->manager()->setState('guard-base', ModuleState::Disabled); + } + + public function testDisableAllowedWhenDependentIsAlreadyBrokenByDisabledDependency(): void { + // Chain: top -> middle -> bottom. bottom is disabled, so the loader already + // prunes middle AND top. Disabling middle breaks nothing and must succeed — + // the guard used to count top as an enabled dependent and refuse. + $this->createModuleWithDeps('chain-bottom', '1.0.0', []); + $this->createModuleWithDeps('chain-middle', '1.0.0', ['chain-bottom']); + $this->createModuleWithDeps('chain-top', '1.0.0', ['chain-middle']); + $this->writeOverrides([ + 'chain-bottom' => ['state' => 'disabled', 'installed_version' => '1.0.0'], + 'chain-middle' => ['installed_version' => '1.0.0'], + 'chain-top' => ['installed_version' => '1.0.0'], + ]); + + $this->manager()->setState('chain-middle', ModuleState::Disabled); + + $this->assertSame('disabled', $this->readOverrides()['chain-middle']['state'] ?? null); + } + + public function testDisableAllowedWhenDependentIsItselfDisabled(): void { + $this->createModuleWithDeps('solo-base', '1.0.0', []); + $this->createModuleWithDeps('solo-consumer', '1.0.0', ['solo-base']); + $this->writeOverrides([ + 'solo-base' => ['installed_version' => '1.0.0'], + 'solo-consumer' => ['state' => 'disabled', 'installed_version' => '1.0.0'], + ]); + + $this->manager()->setState('solo-base', ModuleState::Disabled); + + $this->assertSame('disabled', $this->readOverrides()['solo-base']['state'] ?? null); + } + private function manager(): ModuleManager { return new ModuleManager($this->modulesPath, $this->overridesPath, ServiceContainer::getInstance()); } @@ -344,6 +504,33 @@ final class ModuleManagerMigrationsTest extends TestCase { return $byName; } + /** Build a .tar holding one module under a `{name}/` prefix; returns its path. */ + private function makeModuleTar(string $name, string $version, string $hashId): string { + $pascal = $this->pascal($name); + $manifest = $this->manifest($name, $version); + $manifest['hash_id'] = $hashId; + + $tarPath = $this->workDir . '/' . $name . '.tar'; + @unlink($tarPath); + $tar = new PharData($tarPath); + $tar->addFromString( + $name . '/module.json', + json_encode($manifest, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES) + ); + $tar->addFromString($name . '/' . $pascal . 'Module.php', + "modulesPath . '/' . $name;