From 97696c0546a18940faed2ff7b1e3c6d349a896d1 Mon Sep 17 00:00:00 2001 From: Rudy Ges Date: Thu, 16 Jul 2026 11:56:49 +0200 Subject: [PATCH] [server] don't leave modules in unknown cache state when a check raises check_module sets m_cache_state to MSUnknown while checking a module and only resolves that state once the whole subgraph has been checked. If anything raises in between (e.g. a parse error while a CheckFileContentModification policy makes the check parse the file), modules stay MSUnknown with the m_checked of that compilation, which breaks the invariant asserted on the next check: File "src/compiler/server/serverCache.ml", line 258, characters 12-19 Called from ServerCache.check_module.check.check_dependencies.(fun) ... Modules cached by reference (import.hx modules, -D disable-hxb-cache) keep that state across requests, so the server keeps failing until it's restarted. Register modules in unknown_state_modules before checking them and revert their state if the check raises. --- src/compiler/server/serverCache.ml | 38 ++++++++++++++++++--------- tests/server/src/cases/ServerTests.hx | 25 ++++++++++++++++++ tests/server/src/utils/Vfs.hx | 10 +++++++ 3 files changed, 61 insertions(+), 12 deletions(-) diff --git a/src/compiler/server/serverCache.ml b/src/compiler/server/serverCache.ml index 116ea9a55b4..418e64eb966 100644 --- a/src/compiler/server/serverCache.ml +++ b/src/compiler/server/serverCache.ml @@ -257,8 +257,10 @@ let check_module sctx com m_path m_extra p = (* This should not happen because any MSUnknown module is supposed to have the current m_checked. *) die "" __LOC__ | MSGood -> - (* Otherwise, run the checks *) + (* Otherwise, run the checks. Register the module before checking it so that its MSUnknown + state is reverted even if the check raises. *) m_extra.m_cache_state <- MSUnknown; + unknown_state_modules := m_extra :: !unknown_state_modules; check () in (* Update the module now. It will use this dirty status for the remainder of this compilation. *) @@ -268,21 +270,16 @@ let check_module sctx com m_path m_extra p = m_extra.m_cache_state <- MSBad reason; | None -> (* We cannot update if we're clean because at this point it might just be an assumption. - Instead We add the module to a list which is updated at the end of handling this subgraph. *) - unknown_state_modules := m_extra :: !unknown_state_modules; + The module stays in unknown_state_modules, which is updated at the end of handling this subgraph. *) + () end; dirty end in - let state = check m_path m_extra in - begin match state with - | None -> - (* If the entire subgraph is clean, we can set all modules to good state *) - List.iter (fun m_extra -> m_extra.m_cache_state <- MSGood) !unknown_state_modules; - | Some _ -> - (* Otherwise, unknown state module may or may not be dirty. We didn't check everything eagerly, so we have - to make sure that the module is checked again if it appears in a different check. This is achieved by - setting m_checked to a lower value and assuming Good state again. *) + (* Unknown state modules may or may not be dirty. We didn't check everything eagerly, so we have to make sure + that they are checked again if they appear in a different check. This is achieved by setting m_checked to a + lower value and assuming Good state again. *) + let revert_unknown_state_modules () = List.iter (fun m_extra -> match m_extra.m_cache_state with | MSUnknown -> m_extra.m_checked <- start_mark - 1; @@ -290,6 +287,23 @@ let check_module sctx com m_path m_extra p = | MSGood | MSBad _ -> () ) !unknown_state_modules + in + let state = try + check m_path m_extra + with e -> + (* Leaving modules in MSUnknown state would violate the m_checked invariant of any later check. *) + revert_unknown_state_modules(); + raise e + in + begin match state with + | None -> + (* If the entire subgraph is clean, we can set all modules to good state *) + List.iter (fun m_extra -> match m_extra.m_cache_state with + | MSUnknown -> m_extra.m_cache_state <- MSGood + | MSGood | MSBad _ -> () + ) !unknown_state_modules; + | Some _ -> + revert_unknown_state_modules() end; state diff --git a/tests/server/src/cases/ServerTests.hx b/tests/server/src/cases/ServerTests.hx index 96437dd227c..18ea3e01eeb 100644 --- a/tests/server/src/cases/ServerTests.hx +++ b/tests/server/src/cases/ServerTests.hx @@ -768,4 +768,29 @@ class ServerTests extends TestCase { } // Hover may also succeed in some configurations — that's fine too. } + + function testErrorWhileCheckingModules() { + vfs.putContent("Util.hx", "class Util {\n\tpublic static function foo() return 1;\n}"); + // import.hx modules are kept in the cache by reference, so a stale cache state survives the request + vfs.putContent("import.hx", "import Util;"); + vfs.putContent("Main.hx", "class Main {\n\tstatic function main() {\n\t\ttrace(Util.foo());\n\t}\n}"); + var args = [ + "-main", "Main", "--macro", + "server.setModuleCheckPolicy(['Util'], [CheckFileContentModification], false)", "--no-output", "-js", "no.js" + ]; + runHaxe(args); + assertSuccess(); + // The check policy makes the module check parse Util.hx, which now fails + vfs.putContent("Util.hx", "class Util {\n\tpublic static function foo() return 1;\n\t@#$%\n}"); + vfs.touch("Util.hx"); + runHaxe(args); + assertErrorMessage("Invalid character '#'"); + // The error must not leave any module in an unknown cache state + runHaxe(args); + assertErrorMessage("Invalid character '#'"); + vfs.putContent("Util.hx", "class Util {\n\tpublic static function foo() return 2;\n}"); + vfs.touch("Util.hx"); + runHaxe(args); + assertSuccess(); + } } diff --git a/tests/server/src/utils/Vfs.hx b/tests/server/src/utils/Vfs.hx index b73678aa4cc..8bbf526c6c0 100644 --- a/tests/server/src/utils/Vfs.hx +++ b/tests/server/src/utils/Vfs.hx @@ -31,6 +31,16 @@ class Vfs { Fs.writeFileSync(path.toString(), content); } + /** + Bumps the modification time of `path` into the future. The server compares file times with + second granularity, so a rewrite within the same second is invisible to it otherwise. + **/ + public function touch(path:String) { + var path = getPhysicalPath(path).toString(); + var time = Date.fromTime(Date.now().getTime() + 2000); + Fs.utimesSync(path, time, time); + } + public function getContent(path:String):String { var path = getPhysicalPath(path); FileSystem.createDirectory(path.dir);