Skip to content

Zoo: check for write permissions before updating a model #1388

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 21, 2025

Conversation

lnotspotl
Copy link
Member

Purpose

Without this check, the zoo manager tries to update a model, create a lock and in case of a protected path (such as /usr/lib) this leads to an insufficient permission error.

With this check in place, we do not try to update the model and simply return the already cached one or in the edge case of there being no cached model, we error out.

@moratom
Copy link
Collaborator

moratom commented Jul 18, 2025

bugbot run

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: ZooManager Cache Locking and Validation Issues

The commit introduces a compilation error as getModelFromZoo attempts to access the private member ZooManager::cacheFolderLock. Additionally, when the cache directory lacks write permissions, the ZooManager cannot acquire a lock. In this state, if a model is cached, it is immediately returned. This behavior ignores the useCached parameter (returning cached models even when explicitly disallowed) and bypasses validation against the latest online version, potentially serving stale models even with internet connectivity.

src/modelzoo/Zoo.cpp#L595-L605

// Check if model is cached
bool hasLock = (zooManager.cacheFolderLock != nullptr);
bool modelIsCached = zooManager.isModelCached();
// Return the model right away if lock is not held and model is cached
if(!hasLock && modelIsCached) {
fs::path modelPath = zooManager.loadModelFromCache();
logger::info("Model is cached but model lock could not be acquired (likely due to insufficient write permissions). Using cached model located at {}",
modelPath);
return modelPath;
}

Fix in CursorFix in Web


Comment bugbot run to trigger another review on this PR
Was this report helpful? Give feedback by reacting with 👍 or 👎

Copy link
Collaborator

@moratom moratom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@@ -171,7 +201,6 @@ class ZooManager {
*/
static bool connectionToZooAvailable();

private:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentionally removed?

@lnotspotl lnotspotl merged commit 36a4026 into develop Jul 21, 2025
57 of 66 checks passed
@lnotspotl lnotspotl deleted the lnotspotl/zoo_check_permissions branch July 21, 2025 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants