Fix race condition on files in libgit2 by rasapala · Pull Request #4229 · openvinotoolkit/model_server

rasapala · 2026-05-21T09:06:43Z

🛠 Summary

Fixes a race condition in the LFS shutdown path and hardens the interrupt-and-resume test coverage for the HF model pull module.

Production fix:

Adds clearStaleErrorFile() in libgit2.cpp to remove stale lfs_error.txt artifacts left by a previous interrupted pull. Without this, a false error file caused subsequent resume attempts to misidentify the repository state and fail.
Adds a shutdown check inside the LFS write callback so that an in-progress LFS download honours a cancellation request promptly rather than completing the current chunk before noticing the shutdown.

Test improvements:

Refactors HfPull and HfPullCache fixtures to centralize repeated model path setup into shared members, eliminating per-test duplication.
Removes the deterministic fallback helper that masked the real SIGKILL / SIGINT code paths; both ResumeTerminate and ResumeCtrlC now require observing genuine in-progress LFS activity before signalling.
Replaces fragile fixed-sleep interruption gating with a polling loop that checks for main plus LFS artifacts or an in-flight model file, making the tests reliable across both fast and slow network conditions.

🧪 Checklist

Unit tests added.
The documentation updated.
Change follows security best practices.
``

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates the vendored libgit2 LFS patch to mitigate rename-related races and improve diagnostics when finalizing downloaded LFS objects.

Changes:

Removes the pre-unlink of the destination file before renaming the temp file into place.
Adds helpers to detect whether parent directories exist for improved rename(...)=ENOENT diagnostics.
Captures and logs errno more robustly during rename failures (and logs Win32 error code).

Comments suppressed due to low confidence (1)

third_party/libgit2/lfs.patch:1

GetLastError() is logged after calls to p_access() / p_stat() (via lfs_parent_dir_exists). Those calls can overwrite the thread’s last-error value on Windows, so the logged GetLastError is likely unrelated to the p_rename failure. Capture DWORD winerr = GetLastError(); immediately after the failed p_rename (before any other syscalls), and log the captured value later.

diff --git a/.gitignore b/.gitignore

- ResumeShutdown, ResumeTerminate, ResumeCtrlC: Only wait for .lfs_part files (not pointer files) - Pointer files exist BEFORE download starts (part of git repo), causing shutdown to arrive AFTER completion - Updated ResumeTerminate child process to detect ANY .lfs_part file, not just the large model's This ensures shutdown/cancel signals arrive DURING download, not after completion, allowing proper LFS finalization with .lfs_part artifacts preserved for resume.

atobiszei · 2026-05-27T10:18:56Z

    ASSERT_FALSE(std::filesystem::exists(gitDir));

+#ifdef _WIN32
+    // On Windows, gtest stdout capture can conflict with SPDLOG stdout sink.


We could use log file with ovms to check, unless sth we expect does not appear in spdlong but only in stdout?

Logger configuration is process-global and initialized earlier in the test
binary, so a per-test --log_path does not reliably capture this warning.

atobiszei · 2026-05-27T10:28:23Z

 #endif

-    EXPECT_TRUE(observedPartialDownload);
+    SPDLOG_INFO("ResumeCtrlC test state: observedPartialDownload={}, interruptionSent={}",


atobiszei · 2026-05-27T10:33:43Z

 #ifdef _WIN32
    SKIP_AND_EXIT_IF_NOT_RUNNING_UNSTABLE();
 #endif


Left skipped?

atobiszei · 2026-05-27T10:38:54Z

What are those env variables? Why are they needed?
Also couldn't we have static utility funcitons that would do proper sending signals in tests? Now test logic is containing both setup how the initial state is prepared, then details how to properly send signals depending on OS.

Its not easily readable at first glance what is actual test logic, what is preparation, what is mechanism to simulate ctrl c etc.

Also I see magic constants:
ASSERT_EQ(std::filesystem::file_size(modelPath), 52417240);

have const size_t MAIN_MODEL_SIZE
TOKENIZER_MODEL_SIZE

mzegla · 2026-05-27T11:05:33Z

+        std::error_code ec;
+        const bool hasMainRef = std::filesystem::exists(mainRefPath, ec);
+        ec.clear();
+        const bool modelExists = std::filesystem::exists(modelPath, ec);


no ec check after this call?

mzegla · 2026-05-27T11:06:49Z

+        // Wait until pull is in progress and repository is already resumable.
+        std::error_code ec;
+        const bool hasMainRef = std::filesystem::exists(mainRefPath, ec);
+        ec.clear();


error code cleared without checking? why is it even cleared? won't the last call before check from line 841 override it anyway before check?

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

        auto lfsCandidates = ovms::libgit2::findLfsLikeFiles(downloadPath, true);
-        auto hasOpenvinoModelPointer = std::find_if(lfsCandidates.begin(), lfsCandidates.end(),
-                                           [](const std::filesystem::path& p) { return p.filename() == "openvino_model.bin"; }) != lfsCandidates.end();
-        const std::string partPath = ovms::FileSystem::appendSlash(basePath) + "openvino_model.binlfs_part";
-        const bool hasPartFile = std::filesystem::exists(partPath);
-        if (hasOpenvinoModelPointer || hasPartFile) {
+        const bool hasLfsArtifacts = !lfsCandidates.empty();
+        const bool modelInFlight = modelExists && (modelSize > 0) && (modelSize < 52417240);
+        if (hasMainRef && (hasLfsArtifacts || modelInFlight)) {


        auto lfsCandidates = ovms::libgit2::findLfsLikeFiles(downloadPath, true);
-        const bool hasOpenvinoModelPointer = std::find_if(lfsCandidates.begin(), lfsCandidates.end(),
-                                                 [](const std::filesystem::path& p) { return p.filename() == "openvino_model.bin"; }) != lfsCandidates.end();
-        const std::string partPath = ovms::FileSystem::appendSlash(basePath) + "openvino_model.binlfs_part";
-        const bool hasPartFile = std::filesystem::exists(partPath);
-        if (hasOpenvinoModelPointer || hasPartFile) {
+        const bool hasLfsArtifacts = !lfsCandidates.empty();
+        const bool modelInFlight = modelExists && (modelSize > 0) && (modelSize < 52417240);
+        if (hasMainRef && (hasLfsArtifacts || modelInFlight)) {


mzegla · 2026-05-27T11:07:33Z

-        const bool hasPartFile = std::filesystem::exists(partPath);
-        if (hasOpenvinoModelPointer || hasPartFile) {
+        const bool hasLfsArtifacts = !lfsCandidates.empty();
+        const bool modelInFlight = modelExists && (modelSize > 0) && (modelSize < 52417240);


hardcoded limits - why this upper bound has been chosen?
please move to const

mzegla · 2026-05-27T11:07:57Z

    ASSERT_TRUE(TerminateProcess(pi.hProcess, 1));
-    ASSERT_EQ(WaitForSingleObject(pi.hProcess, 10000), WAIT_OBJECT_0);
+    interruptionSent = true;
+    ASSERT_EQ(WaitForSingleObject(pi.hProcess, 120000), WAIT_OBJECT_0);


move value to const variable so we know the context

mzegla · 2026-05-27T11:09:19Z

+        std::error_code ec;
+        const bool hasMainRef = std::filesystem::exists(mainRefPath, ec);
+        ec.clear();
+        const bool modelExists = std::filesystem::exists(modelPath, ec);
+        std::uintmax_t modelSize = 0;
+        if (modelExists) {
+            ec.clear();
+            modelSize = std::filesystem::file_size(modelPath, ec);
+            if (ec) {
+                modelSize = 0;
+            }
+        }


Duplicate of part of logic above (same comments apply here as well).
Could we have it in a function?

mzegla · 2026-05-27T11:09:33Z

-        const bool hasPartFile = std::filesystem::exists(partPath);
-        if (hasOpenvinoModelPointer || hasPartFile) {
+        const bool hasLfsArtifacts = !lfsCandidates.empty();
+        const bool modelInFlight = modelExists && (modelSize > 0) && (modelSize < 52417240);


duplicate use of hardcoded values

Fix race condition on files

b7fd0b9

Copilot AI review requested due to automatic review settings May 21, 2026 09:06

Copilot AI reviewed May 21, 2026

View reviewed changes

Comment thread third_party/libgit2/lfs.patch

Comment thread third_party/libgit2/lfs.patch Outdated

Comment thread third_party/libgit2/lfs.patch

Clean error log

a4d6afa

ngrozae approved these changes May 21, 2026

View reviewed changes

mzegla reviewed May 22, 2026

View reviewed changes

Comment thread src/pull_module/libgit2.cpp Outdated

Comment thread third_party/libgit2/lfs.patch

Comment thread third_party/libgit2/lfs.patch Outdated

Comment thread third_party/libgit2/lfs.patch Outdated

Comment thread third_party/libgit2/lfs.patch

rasapala added 10 commits May 22, 2026 11:54

Fix sporadic win test

6bc58d2

Fix too soon cancel and terminate

1bb5520

Write callback shutdown check

01bee5c

File check

beeecc6

Make tests deterministic

e385c8f

Style

2a3418e

Merge branch 'main' into fix_race_condition

3da0c60

Merge branch 'main' into fix_race_condition

a22953b

Fix tests and code review

5633654

atobiszei reviewed May 27, 2026

View reviewed changes

mzegla requested a review from Copilot May 27, 2026 11:04

Copilot started reviewing on behalf of mzegla May 27, 2026 11:04 View session

mzegla reviewed May 27, 2026

View reviewed changes

Copilot AI reviewed May 27, 2026

View reviewed changes

mzegla reviewed May 27, 2026

View reviewed changes

rasapala added 3 commits May 27, 2026 14:23

Merge branch 'main' into fix_race_condition

fd46e71

Code review

6206119

Unit test refactor

3c69e56

Conversation

rasapala commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🛠 Summary

🧪 Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rasapala commented May 21, 2026 •

edited

Loading