Add check for lfs pointer files by rasapala · Pull Request #4107 · openvinotoolkit/model_server

rasapala · 2026-04-01T11:57:00Z

🛠 Summary

CVS-CVS-184757

Libgit2 Lfs-filter.c patch changes:

Added a process-wide cancellation mechanism with an exported global flag and shutdown probe path, so host applications can request LFS aborts reliably: lfs_filter.c:59, lfs_filter.c:73.
Improved cancellation responsiveness by checking cancel state in progress callbacks, before/after curl operations, during resume attempts, and even during retry sleep intervals: lfs_filter.c:1063, lfs_filter.c:1349, lfs_filter.c:1424, lfs_filter.c:1452.
Switched logging to an error-focused logger and changed log file lifecycle so lfs_error.txt is created/truncated only when an actual error is emitted: lfs_filter.c:71, lfs_filter.c:127.
Extended cancellation coverage to both the batch metadata request and object download request (both now wired to progress callbacks and explicit abort guards): lfs_filter.c:1593, lfs_filter.c:1659.
Kept resume behavior but made it safer and more interruptible, including repeated cancel checks around resume retries and fallback logic: lfs_filter.c:1414, lfs_filter.c:1687.

Ovms Libgit2.cpp Changes:

End-to-end cancellation propagation was added from server shutdown signaling into clone and LFS download paths, so Ctrl+C can abort both clone and ongoing LFS transfers.
Resume logic was redesigned to reliably re-trigger LFS smudge after interruption by repairing index entries and re-checking out specific paths.
A shared shutdown state module was introduced to decouple pull module cancellation checks from server internals and satisfy Bazel dependency boundaries.
New status codes and tests were added for cancellation and resume-after-cancel behavior.
Third-party libgit2 integration was updated significantly (new upstream commit + larger LFS patch), including Windows linkage/packaging adjustments.

🧪 Checklist

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

Copilot

Pull request overview

Adds a safeguard to detect Git LFS pointer files left behind after Hugging Face model downloads (clone or resume) and surfaces a dedicated status code when this occurs.

Changes:

Introduces a new StatusCode for LFS download failures.
Adds status message mapping for the new status code.
Extends HfDownloader::downloadModel() to scan for LFS pointer files after clone/resume and fail fast if any are found.

Reviewed changes

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

File	Description
`src/status.hpp`	Adds a new status code for LFS download failure.
`src/status.cpp`	Adds a human-readable message for the new status code.
`src/pull_module/libgit2.cpp`	Detects remaining LFS pointer files after clone/resume and returns the new failure status.

Co-authored-by: Adrian Tobiszewski <adrian.tobiszewski@intel.com>

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

atobiszei · 2026-04-30T11:08:03Z

+/**
+ * Builds the side-marker path used to track interrupted LFS operations.
+ * The marker lives next to the repository directory as <repo>.lfswip.
+ * 
+ * @param repositoryPath Absolute path to the model repository directory.
+ * @return Filesystem path to the .lfswip marker associated with repositoryPath.
+ * @note Pure path computation; does not access filesystem or network.
+ */


Do we really need so much comments for those short functions? I would skip them, naming convention is good so they are self-explanatory I think, and it just bloats the code.

atobiszei · 2026-04-30T11:09:14Z

+ * @return -1 if cancellation requested (aborts transfer), 0 otherwise.
+ * @note Connects to internet during clone operations; works on the clone/fetch domain.
+ */
+int cloneTransferProgressCb(const git_indexer_progress* stats, void* payload) {


Cb->Callback?

Since we don't do a lot in those callbacks maybe name those (
checkIfCancelCloneTransferProgressCallback or sth like that?

atobiszei · 2026-04-30T11:11:08Z

+    (void)str;
+    (void)len;
+    (void)payload;
+    if (libgit2::isCloneCancellationRequestedFromServer()) {


This is used in several places. Use macro to shorten the code?
#define STOP_IF_OVMS_CANCELLED or sth like that?

atobiszei · 2026-04-30T11:11:52Z

+}
+
+/**
+ * Callback for sideband progress messages during git operations (clone, fetch).


Here comments again - we don't even use those methods for anythin usefull, so this comments only bloats the code.

atobiszei · 2026-04-30T11:36:30Z

+ * @note Works on specific git repository location; reads and deletes lfs_error.txt.
+ * @note Part of the git LFS domain: detects LFS download failures recorded by patched libgit2.
+ */
+bool hasLfsErrorFileAndLogContent(const std::string& repositoryRootPath) {


Suggested change

bool hasLfsErrorFileAndLogContent(const std::string& repositoryRootPath) {

bool ifHasLfsErrorFileLogContentAndRemove(const std::string& repositoryRootPath) {

or sth like that?
It not only checks but removes as well.

atobiszei · 2026-04-30T11:43:49Z

-            std::cout << " Resuming " << p.string() << "...\n";
-            std::string path = p.string();
-            resumeLfsDownloadForFile(repoGuard.get(), path.c_str());
+Status resumeExistingRepository(git_repository* repo,


Nice - clear logic

atobiszei · 2026-04-30T11:45:10Z

    }

-    auto status = IModelDownloader::checkIfOverwriteAndRemove();
+    auto normalizeIndexStatus = normalizeIndexToHead(repo);


normalizeIndex->restoreGitIndex?

atobiszei · 2026-04-30T12:29:27Z

+// ResumeAfterShutdownRequestAndRerun
+TEST_F(HfPull, ResumeShutdown) {


Suggested change

// ResumeAfterShutdownRequestAndRerun

TEST_F(HfPull, ResumeShutdown) {

TEST_F(HfPull,ResumeAfterShutdownRequestAndRerun) {

We cannot have long names in unit tests for lib git 2 clone because on windows we have a path max that can be exceeded.

atobiszei · 2026-04-30T12:30:08Z

+    EXPECT_EQ(preservedDigestBefore, preservedDigestAfter);
+}
+
+// PullAfterUserEditedTrackedFileDoesNotOverwriteIt


Put that in test name

Copilot

Pull request overview

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

Comments suppressed due to low confidence (1)

third_party/libgit2/lfs.patch:435

The new lfs_filter.c being introduced by this patch starts with a UTF-8 BOM and the license block lines are prefixed with +/ instead of the usual *. The BOM (the invisible character before /*) can break compilation on some toolchains or cause lint/style checks to fail. Please remove the BOM and normalize the comment to a standard C block comment format.

+/*
+/ Copyright 2025 Intel Corporation
+/
+/ Licensed under the Apache License, Version 2.0 (the "License");
+/ you may not use this file except in compliance with the License.

…oolkit/model_server into fix_pull_all_dl_failed

Add check for lfs pointer files

d7585ab

Copilot AI review requested due to automatic review settings April 1, 2026 11:57

Copilot started reviewing on behalf of rasapala April 1, 2026 11:57 View session

Copilot AI reviewed Apr 1, 2026

View reviewed changes

Comment thread src/status.hpp Outdated

Comment thread src/pull_module/libgit2.cpp Outdated

Comment thread src/pull_module/libgit2.cpp Outdated

Comment thread src/pull_module/libgit2.cpp Outdated

Fix style

09ae483

rasapala requested review from atobiszei and dtrawins April 1, 2026 12:54

Fix build

044ee86

atobiszei approved these changes Apr 2, 2026

View reviewed changes

rasapala added 21 commits April 10, 2026 12:11

Merge branch 'main' into fix_pull_all_dl_failed

49d8bc1

Add error handling and detailed fail report

3e4e07a

Full cancel on git_clone and lfs_download

ab90f1c

Add cancel

10fd422

Style

6c912b2

Add cancel on resume

38e5143

Dynamic symbol load on windows

9f4107f

Static on windows

f13bc0b

Add missing link

055a504

Remove from package

e679a15

Fix build

ad44862

Add missing libs

d45a61c

Change check order

f52acd5

Merge branch 'main' into fix_pull_all_dl_failed

aa9fd06

Fix cancel resume

9766f33

Merge branch 'main' into fix_pull_all_dl_failed

c12f521

Style

66ff312

Descriptions

ee93898

wiplfs and cleanup

d0402d3

Style

7811c0e

Merge branch 'main' into fix_pull_all_dl_failed

7adeeb9

atobiszei reviewed Apr 28, 2026

View reviewed changes

Comment thread src/pull_module/libgit2.cpp Outdated

atobiszei reviewed Apr 28, 2026

View reviewed changes

Comment thread src/pull_module/libgit2.cpp Outdated

atobiszei self-requested a review April 28, 2026 12:19

rasapala and others added 11 commits April 28, 2026 14:49

Terminate windows test

c2b0f53

Update src/server.cpp

e40dc7a

Co-authored-by: Adrian Tobiszewski <adrian.tobiszewski@intel.com>

Update src/pull_module/libgit2.cpp

5e1faf4

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Fix terminate - clean index and refactor download

7cb5671

Tests refactor

22dd06f

Code review

ea6a96d

Style

9e6f72c

Fix test

21bb89c

Merge branch 'main' into fix_pull_all_dl_failed

e2779d9

Switch back to dll

1781077

Fix dll

1344d1b

atobiszei reviewed Apr 30, 2026

View reviewed changes

atobiszei self-requested a review April 30, 2026 11:48

atobiszei reviewed Apr 30, 2026

View reviewed changes

atobiszei requested a review from Copilot April 30, 2026 12:59

Copilot started reviewing on behalf of atobiszei April 30, 2026 13:00 View session

Copilot AI reviewed Apr 30, 2026

View reviewed changes

Comment thread src/test/pull_hf_model_test.cpp

Comment thread src/test/pull_hf_model_test.cpp Outdated

Comment thread src/server.cpp

dkalinowski requested a review from atobiszei April 30, 2026 13:18

rasapala added 2 commits April 30, 2026 16:07

Code review

1237677

Merge branch 'fix_pull_all_dl_failed' of https://github.com/openvinot…

eac5fc8

…oolkit/model_server into fix_pull_all_dl_failed

atobiszei approved these changes Apr 30, 2026

View reviewed changes

rasapala added 5 commits April 30, 2026 16:26

Style

c376646

Add better logs

39fc47c

Marker tests

8acd4c4

Style

d97b79c

Fix test

919b871

	bool hasLfsErrorFileAndLogContent(const std::string& repositoryRootPath) {
	bool ifHasLfsErrorFileLogContentAndRemove(const std::string& repositoryRootPath) {

		// ResumeAfterShutdownRequestAndRerun
		TEST_F(HfPull, ResumeShutdown) {

	// ResumeAfterShutdownRequestAndRerun
	TEST_F(HfPull, ResumeShutdown) {
	TEST_F(HfPull,ResumeAfterShutdownRequestAndRerun) {

Conversation

rasapala commented Apr 1, 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

Reviewed changes

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!

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!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rasapala commented Apr 1, 2026 •

edited

Loading