diff --git a/CHANGELOG.md b/CHANGELOG.md index c63154f..850e6a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,13 @@ APIs change between minors without a deprecation period. ### Fixed +- **`ctl weights` offered models in use for deletion.** A model of several + files (a set, its address ending in `/`) is fetched into a directory named + from the address without the slash; the sweep named it from the address + with it, so every set in the catalog looked like nobody's. On the PoC that + was 330 GB of six catalog models, the qwen3.8-flash-next Q8_0 among them; + `--collect`, and `ctl model remove --purge` of a set, would have deleted + them. Both now compute the name the fetch does. - **A sign-in took an organization's manager role away.** A group from single sign-on, SCIM or Active Directory that puts someone in an organization wrote the membership again at every sign-in or sync, as a plain member: a manager diff --git a/internal/agent/weights/gc.go b/internal/agent/weights/gc.go index 9c64b5e..42c46c7 100644 --- a/internal/agent/weights/gc.go +++ b/internal/agent/weights/gc.go @@ -33,6 +33,17 @@ func CachePathFor(dir, source string) string { return filepath.Join(dir, hex.EncodeToString(sum[:8])+"-"+name) } +// cacheSource is the source a cache name is computed from: a set's address +// without its trailing slash, as its directory was named when it was fetched. +// Computed from the slash-ended address, every set in the catalog looked like +// nobody's, and a sweep offered 330 GB of models still in use for deletion. +func cacheSource(source string) string { + if IsSet(source) { + return strings.TrimSuffix(source, "/") + } + return source +} + // Owned reports whether a path is one this package created. // // The dividing line of the whole feature. A file without the cache name was @@ -65,7 +76,7 @@ func Orphans(have []string, wanted []string) []string { // By base name rather than full path: the control plane does not know // each node's cache directory, and the name is what identifies the // file wherever the node keeps it. - keep[filepath.Base(CachePathFor("", src))] = struct{}{} + keep[filepath.Base(CachePathFor("", cacheSource(src)))] = struct{}{} } var out []string diff --git a/internal/agent/weights/set.go b/internal/agent/weights/set.go index 3e6090c..18edfae 100644 --- a/internal/agent/weights/set.go +++ b/internal/agent/weights/set.go @@ -51,7 +51,7 @@ type manifest struct { // as a broken model rather than failing to load, so a partial set is left // unfinished under its temporary name and the next attempt starts again. func (f *Fetcher) ensureSet(ctx context.Context, source string, allowPickle bool) (string, error) { - dir, err := f.cachePath(strings.TrimSuffix(source, "/")) + dir, err := f.cachePath(cacheSource(source)) if err != nil { return "", err } diff --git a/internal/agent/weights/set_test.go b/internal/agent/weights/set_test.go index 1a95ff0..e53b156 100644 --- a/internal/agent/weights/set_test.go +++ b/internal/agent/weights/set_test.go @@ -74,6 +74,23 @@ func TestASetIsFetchedIntoOneDirectory(t *testing.T) { } } +func TestASetInTheCatalogIsNotAnOrphan(t *testing.T) { + // The catalog names a set by its address with the trailing slash; the + // fetch names the directory without it. A sweep that computed the name + // from the catalog's spelling offered every set in use for deletion. + f, srv, _ := mirrorOf(t, map[string]string{"model.safetensors": "aaaa", "config.json": "{}"}) + dir, err := f.Ensure(t.Context(), srv.URL+"/m/v/", false) + if err != nil { + t.Fatal(err) + } + if got := Orphans([]string{dir}, []string{srv.URL + "/m/v/"}); len(got) != 0 { + t.Errorf("a set the catalog still names was called an orphan: %v", got) + } + if got := Orphans([]string{dir}, nil); len(got) != 1 { + t.Errorf("a set nothing names was not found: %v", got) + } +} + func TestAFinishedSetIsNotFetchedAgain(t *testing.T) { f, srv, hits := mirrorOf(t, map[string]string{"a.gguf": "aaaa", "b.gguf": "bb"})