From e0dddff176a01d410904b6750b3395de4f7e54db Mon Sep 17 00:00:00 2001 From: Lukasz Kasprzak Date: Mon, 14 Sep 2026 21:39:00 +0200 Subject: plan 9: keyword cache keys on extension, max-read and Unicode tables, trims removed keywords --- docs/design.md | 28 ++++++++++++-------- internal/engine/cache_test.go | 57 ++++++++++++++++++++++++++++++++++++++++ internal/engine/facts.go | 2 +- internal/engine/match.go | 29 ++++++++++++++------ internal/kwcache/fuzz_test.go | 4 +-- internal/kwcache/kwcache.go | 53 ++++++++++++++++++++++++++++++------- internal/kwcache/kwcache_test.go | 49 +++++++++++++++++++++++++++++++--- internal/norm/norm.go | 8 ++++++ man/krino.conf.5 | 14 +++++++--- 9 files changed, 205 insertions(+), 39 deletions(-) diff --git a/docs/design.md b/docs/design.md index caa2a0f..bea0e2f 100644 --- a/docs/design.md +++ b/docs/design.md @@ -336,9 +336,11 @@ Extraction is nearly all of a run's time, so krino remembers what it found. For each file it extracted, `.cache` records which of the directory's content keywords the text contains, against the full list of keywords it was checked against. It stores no text and no file names; a -file is known by device, inode, size and modification time, which krino's -own moves and renames keep. The keywords themselves are stored, as they -appear in the config. +file is known by device, inode, size, modification time and extension +(the extension picks the extractor), which a move within one filesystem +keeps. The keywords themselves are stored, normalised the way the test +compares them (case, folding, whitespace), and only those the directory +still uses. - When a file is extracted, every content keyword of its directory is answered at once, so one extraction serves every rule and exclude. @@ -347,16 +349,20 @@ appear in the config. replaced: a changed file, or a new keyword, costs one extraction. - Files above `max-read` are refused before the cache is consulted. Failures (unreadable, tool missing, timeout) are never cached. -- The cache is discarded whole when the extractor fingerprint changes: a - tool installed, removed or replaced, or krino's extraction code changing - (`extract.Version`), or its text normalisation changing (`norm.Version`). +- The cache is discarded whole when its fingerprint changes: a tool + installed, removed or replaced; krino's extraction code (`extract.Version`) + or normalisation (`norm.Version`) changing; the Go release or the Unicode + tables changing; or the directory's `max-read` changing. - Planning a run (`-n` included) reads the cache and writes it back holding only files still in the directory, under the directory's lock, via a - temporary file renamed into place; the directory is 0700, the file 0600. - `explain` reads it and never writes. A directory with no content tests - has no cache. -- Known gap: a file edited in place with its size and modification time - deliberately preserved keeps its old answers. Deleting + temporary file renamed into place; the directory is made 0700, the file + 0600. `explain` reads it and never writes. A directory with no content + tests has no cache, and an old one is removed. +- Known gaps: a file edited in place with its size and modification time + deliberately preserved keeps its old answers, and so does a new file that + reuses a deleted file's inode with the same size, extension and preserved + modification time. A `.doc` that `antiword` failed on (a timeout, say) and + `catdoc` then read is cached with `catdoc`'s answers. Deleting `~/.cache/krino` resets everything. ## 7. Actions diff --git a/internal/engine/cache_test.go b/internal/engine/cache_test.go index eee2860..8c52d48 100644 --- a/internal/engine/cache_test.go +++ b/internal/engine/cache_test.go @@ -6,9 +6,14 @@ import ( "context" "os" "path/filepath" + "runtime" "strings" "testing" "time" + "unicode" + + "krino/internal/config" + "krino/internal/extract" ) // countingPDFTree makes ~/dl holding the given .pdf files, and a fake @@ -222,3 +227,55 @@ func TestExplainUsesCacheWithoutWriting(t *testing.T) { t.Errorf("explain extracted although the run cached a.pdf") } } + +// TestCacheRereadsARenamedExtension: the extension picks the extractor, so +// a file renamed from .html to .txt - same inode, size and mtime - is read +// again, not answered from what the markup reader found (review M6). +func TestCacheRereadsARenamedExtension(t *testing.T) { + home, dl := excludeTree(t, map[string]string{"page.html": "

ACME

 Ltd"}) + main := writeConfig(t, home, `(include "dl")`, map[string]string{"dl": "(path \"~/dl\")\n(rule \"acme\" (when (content \"acme ltd\")) (move \"Acme\"))\n"}) + if r := cachedMatch(t, home, main); matchedNames(r) != "page.html" { + t.Fatalf("as html: matched %q", matchedNames(r)) + } + if err := os.Rename(filepath.Join(dl, "page.html"), filepath.Join(dl, "page.txt")); err != nil { + t.Fatal(err) + } + if r := cachedMatch(t, home, main); matchedNames(r) != "" { + t.Errorf("as txt, answered from the html read: matched %q", matchedNames(r)) + } +} + +// TestCacheFingerprintCoversMaxReadAndTables: answers depend on max-read +// (it caps what is read), on the Go release and on the Unicode tables, so +// all are in the fingerprint (review cache F2, F4). +func TestCacheFingerprintCoversMaxReadAndTables(t *testing.T) { + e := &Engine{Extract: extract.New()} + small := e.cacheFingerprint(&Dir{Settings: config.Resolved{MaxRead: 1 << 10}}) + large := e.cacheFingerprint(&Dir{Settings: config.Resolved{MaxRead: 1 << 20}}) + if small == large { + t.Error("max-read does not change the fingerprint") + } + for _, want := range []string{runtime.Version(), unicode.Version} { + if !strings.Contains(small, want) { + t.Errorf("fingerprint %q lacks %q", small, want) + } + } +} + +// TestCacheRemovedWhenNoContentTestsRemain: once a directory has no content +// test, its cache - holding keywords it no longer uses - is removed (review +// cache F6). +func TestCacheRemovedWhenNoContentTestsRemain(t *testing.T) { + home, _, _ := countingPDFTree(t, map[string]string{"a.pdf": "Invoice ACME"}) + main := writeConfig(t, home, `(include "dl")`, map[string]string{"dl": acmeRules}) + cachedMatch(t, home, main) + file := filepath.Join(home, ".cache", "krino", "dl.cache") + if _, err := os.Stat(file); err != nil { + t.Fatal(err) + } + writeConfig(t, home, `(include "dl")`, map[string]string{"dl": "(path \"~/dl\")\n(rule \"all\" (move \"Out\"))\n"}) + cachedMatch(t, home, main) + if _, err := os.Stat(file); !os.IsNotExist(err) { + t.Errorf("the cache of a directory without content tests is still there: %v", err) + } +} diff --git a/internal/engine/facts.go b/internal/engine/facts.go index 9035245..487604b 100644 --- a/internal/engine/facts.go +++ b/internal/engine/facts.go @@ -212,7 +212,7 @@ func (f *facts) cacheID() (kwcache.ID, bool) { // fileCacheID is file's kwcache.ID. func fileCacheID(file scan.File) kwcache.ID { - return kwcache.ID{Dev: file.Dev, Ino: file.Ino, Size: file.Size, MTime: file.ModTime.UnixNano()} + return kwcache.ID{Dev: file.Dev, Ino: file.Ino, Size: file.Size, MTime: file.ModTime.UnixNano(), Ext: strings.ToLower(filepath.Ext(file.Name))} } // Duplicate resolves dirs against the directory's root, builds (or reuses) diff --git a/internal/engine/match.go b/internal/engine/match.go index b922829..08d75af 100644 --- a/internal/engine/match.go +++ b/internal/engine/match.go @@ -114,7 +114,11 @@ func (e *Engine) Match(ctx context.Context, d *Dir) (*Result, error) { ids = append(ids, fileCacheID(f)) } } - if err := run.cache.Save(e.cacheFile(d), ids); err != nil { + keys := make([]string, len(d.ContentKeywords)) + for i, k := range d.ContentKeywords { + keys[i] = k.Key() + } + if err := run.cache.Save(e.cacheFile(d), ids, keys); err != nil { warnings = append(warnings, "cache: "+err.Error()) } } @@ -297,11 +301,12 @@ func (e *Engine) Explain(ctx context.Context, path string) (*Explanation, error) return &Explanation{Dir: d, File: sf, Skip: skip, Excludes: excludes, Excluded: excluded, Rules: rules}, nil } -// cacheFingerprint identifies what a cached keyword answer depends on -// besides the file: the extractor (its version and tools) and the -// normalisation version. A cache written under any other is discarded. -func (e *Engine) cacheFingerprint() string { - return fmt.Sprintf("%s norm%d", e.Extract.Fingerprint(), norm.Version) +// cacheFingerprint identifies what a cached keyword answer of d depends on +// besides the file: the extractor (its version and tools), normalisation +// and its Unicode tables, the Go release, and d's max-read, which caps what +// is read. A cache written under any other is discarded. +func (e *Engine) cacheFingerprint(d *Dir) string { + return fmt.Sprintf("%s %s %s max-read=%d", e.Extract.Fingerprint(), norm.Fingerprint(), runtime.Version(), d.Settings.MaxRead) } // cacheFile is d's keyword cache file. @@ -313,10 +318,18 @@ func (e *Engine) cacheFile(d *Dir) string { // engine has a CacheDir and the directory has content tests at all. A cache // that cannot be read is replaced by an empty one, reported as a warning. func (e *Engine) openCache(run *matchRun) []string { - if e.CacheDir == "" || len(run.d.ContentKeywords) == 0 { + if e.CacheDir == "" { + return nil + } + if len(run.d.ContentKeywords) == 0 { + // No content test left: a cache from an earlier configuration only + // holds keywords this directory no longer uses (review cache F6). + if err := os.Remove(e.cacheFile(run.d)); err != nil && !os.IsNotExist(err) { + return []string{"cache: " + err.Error()} + } return nil } - c, err := kwcache.Load(e.cacheFile(run.d), e.cacheFingerprint()) + c, err := kwcache.Load(e.cacheFile(run.d), e.cacheFingerprint(run.d)) run.cache = c if err != nil { return []string{"cache: " + err.Error() + " (starting a new one)"} diff --git a/internal/kwcache/fuzz_test.go b/internal/kwcache/fuzz_test.go index 0b0eced..8ac6c95 100644 --- a/internal/kwcache/fuzz_test.go +++ b/internal/kwcache/fuzz_test.go @@ -15,7 +15,7 @@ func FuzzLoad(f *testing.F) { seed := New("fp") seed.Store(a, map[string]bool{"k:acme": true, "k:x": false}) path := filepath.Join(f.TempDir(), "seed.cache") - if err := seed.Save(path, []ID{a}); err != nil { + if err := seed.Save(path, []ID{a}, allKeys); err != nil { f.Fatal(err) } good, err := os.ReadFile(path) @@ -43,7 +43,7 @@ func FuzzLoad(f *testing.F) { if err != nil && ok { t.Fatalf("a cache that failed to load (%v) answered", err) } - if err := c.Save(filepath.Join(t.TempDir(), "out.cache"), []ID{a, b}); err != nil { + if err := c.Save(filepath.Join(t.TempDir(), "out.cache"), []ID{a, b}, allKeys); err != nil { t.Fatal(err) } }) diff --git a/internal/kwcache/kwcache.go b/internal/kwcache/kwcache.go index b61cfb1..c9af7b8 100644 --- a/internal/kwcache/kwcache.go +++ b/internal/kwcache/kwcache.go @@ -19,15 +19,18 @@ import ( ) // version is the on-disk format; a file of any other version loads empty. -const version = 1 +// 2: the extension is part of a file's ID. +const version = 2 // ID is how a file is recognised: the same inode with the same size and -// modification time is taken to hold the same content. A move or rename -// within one filesystem keeps it. +// modification time is taken to hold the same content, and the same +// extension is read by the same extractor. A move within one filesystem +// keeps it; a rename that changes the extension does not (review M6). type ID struct { Dev, Ino uint64 Size int64 - MTime int64 // Unix nanoseconds + MTime int64 // Unix nanoseconds + Ext string // lower case, with its dot; "" for none } // entry is what is known about one file: every keyword it was checked @@ -58,6 +61,7 @@ type diskFile struct { Ino uint64 `json:"ino"` Size int64 `json:"size"` MTime int64 `json:"mtime"` + Ext string `json:"ext"` Set int `json:"keywords"` // index into diskCache.Keywords Hits []int `json:"hits"` // indices into that keyword list } @@ -106,7 +110,7 @@ func Load(path, fingerprint string) (*Cache, error) { } hits[set[h]] = true } - c.old[ID{Dev: f.Dev, Ino: f.Ino, Size: f.Size, MTime: f.MTime}] = entry{checked: set, hits: hits} + c.old[ID{Dev: f.Dev, Ino: f.Ino, Size: f.Size, MTime: f.MTime, Ext: f.Ext}] = entry{checked: set, hits: hits} } c.existed = true return c, nil @@ -154,14 +158,22 @@ func (c *Cache) Store(id ID, answers map[string]bool) { // Save writes the entries of the files in present, from this run or loaded, // to path, and drops every other: the cache only ever describes files -// still in the directory. The directory is created 0700 and the file -// written 0600 under a temporary name, then renamed into place. A cache -// with nothing to write and no file on disk writes nothing. -func (c *Cache) Save(path string, present []ID) error { +// still in the directory. Each entry keeps only the keywords in keywords - +// the directory's current ones - so a keyword removed from the +// configuration leaves the cache too, and an entry left with none is +// dropped. The directory is made private (0700, tightened if it already +// existed) and the file written 0600 under a temporary name, then renamed +// into place. A cache with nothing to write and no file on disk writes +// nothing. +func (c *Cache) Save(path string, present []ID, keywords []string) error { c.mu.Lock() defer c.mu.Unlock() d := diskCache{Version: version, Fingerprint: c.fingerprint, Keywords: [][]string{}, Files: []diskFile{}} + current := make(map[string]bool, len(keywords)) + for _, k := range keywords { + current[k] = true + } sets := map[string]int{} seen := map[ID]bool{} for _, id := range present { @@ -176,6 +188,10 @@ func (c *Cache) Save(path string, present []ID) error { if !ok { continue } + e = trimmed(e, current) + if len(e.checked) == 0 { + continue + } key := strings.Join(e.checked, "\x00") set, ok := sets[key] if !ok { @@ -183,7 +199,7 @@ func (c *Cache) Save(path string, present []ID) error { sets[key] = set d.Keywords = append(d.Keywords, e.checked) } - f := diskFile{Dev: id.Dev, Ino: id.Ino, Size: id.Size, MTime: id.MTime, Set: set, Hits: []int{}} + f := diskFile{Dev: id.Dev, Ino: id.Ino, Size: id.Size, MTime: id.MTime, Ext: id.Ext, Set: set, Hits: []int{}} for i, k := range e.checked { if e.hits[k] { f.Hits = append(f.Hits, i) @@ -209,6 +225,9 @@ func (c *Cache) Save(path string, present []ID) error { if err := os.MkdirAll(dir, 0o700); err != nil { return err } + if err := os.Chmod(dir, 0o700); err != nil { + return err + } tmp, err := os.CreateTemp(dir, ".kwcache-*") if err != nil { return err @@ -229,3 +248,17 @@ func (c *Cache) Save(path string, present []ID) error { c.existed = true return nil } + +// trimmed returns e keeping only the keywords in current. +func trimmed(e entry, current map[string]bool) entry { + out := entry{hits: map[string]bool{}} + for _, k := range e.checked { + if current[k] { + out.checked = append(out.checked, k) + if e.hits[k] { + out.hits[k] = true + } + } + } + return out +} diff --git a/internal/kwcache/kwcache_test.go b/internal/kwcache/kwcache_test.go index 8395b2d..077ecd3 100644 --- a/internal/kwcache/kwcache_test.go +++ b/internal/kwcache/kwcache_test.go @@ -6,9 +6,13 @@ import ( "os" "path/filepath" "reflect" + "strings" "testing" ) +// allKeys is every keyword the tests store, for Save calls that keep them all. +var allKeys = []string{"k:acme", "k:faktura", "k:nip", "k:new", "k:x"} + var ( a = ID{Dev: 1, Ino: 10, Size: 100, MTime: 1000} b = ID{Dev: 1, Ino: 11, Size: 200, MTime: 2000} @@ -17,7 +21,7 @@ var ( func saved(t *testing.T, c *Cache, present ...ID) string { t.Helper() path := filepath.Join(t.TempDir(), "sub", "dl.cache") - if err := c.Save(path, present); err != nil { + if err := c.Save(path, present, allKeys); err != nil { t.Fatal(err) } return path @@ -94,7 +98,7 @@ func TestSaveKeepsOnlyPresentFiles(t *testing.T) { if err != nil { t.Fatal(err) } - if err := next.Save(path, []ID{b}); err != nil { + if err := next.Save(path, []ID{b}, allKeys); err != nil { t.Fatal(err) } last, err := Load(path, "fp") @@ -171,10 +175,49 @@ func TestLoadMissingAndCorrupt(t *testing.T) { // has no file yet, creates none. func TestSaveWithNothingWritesNothing(t *testing.T) { path := filepath.Join(t.TempDir(), "sub", "dl.cache") - if err := New("fp").Save(path, []ID{a}); err != nil { + if err := New("fp").Save(path, []ID{a}, allKeys); err != nil { t.Fatal(err) } if _, err := os.Stat(filepath.Dir(path)); !os.IsNotExist(err) { t.Errorf("Save created %s for an empty cache", filepath.Dir(path)) } } + +// TestSaveTrimsToCurrentKeywords: a keyword no longer in the configuration +// is not kept in the cache file (review cache F6). +func TestSaveTrimsToCurrentKeywords(t *testing.T) { + c := New("fp") + c.Store(a, map[string]bool{"k:keep": true, "k:gone client": true}) + path := filepath.Join(t.TempDir(), "dl.cache") + if err := c.Save(path, []ID{a}, []string{"k:keep"}); err != nil { + t.Fatal(err) + } + b, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(b), "gone client") { + t.Errorf("a removed keyword is still stored:\n%s", b) + } + got, _ := Load(path, "fp") + if hits, ok := got.Lookup(a, []string{"k:keep"}); !ok || !hits[0] { + t.Errorf("the kept keyword's answer was lost: %v %v", hits, ok) + } +} + +// TestSaveTightensAnExistingDirectory: the cache directory is private even +// when it already existed with wider permissions (review cache F7). +func TestSaveTightensAnExistingDirectory(t *testing.T) { + dir := filepath.Join(t.TempDir(), "krino") + if err := os.Mkdir(dir, 0o755); err != nil { + t.Fatal(err) + } + c := New("fp") + c.Store(a, map[string]bool{"k:acme": true}) + if err := c.Save(filepath.Join(dir, "dl.cache"), []ID{a}, []string{"k:acme"}); err != nil { + t.Fatal(err) + } + if fi, err := os.Stat(dir); err != nil || fi.Mode().Perm() != 0o700 { + t.Errorf("directory mode %v, %v; want 0700", fi.Mode().Perm(), err) + } +} diff --git a/internal/norm/norm.go b/internal/norm/norm.go index 77bb604..701f765 100644 --- a/internal/norm/norm.go +++ b/internal/norm/norm.go @@ -5,6 +5,7 @@ package norm import ( + "fmt" "strings" "unicode" @@ -16,6 +17,13 @@ import ( // another version are discarded (spec ยง6.1). const Version = 1 +// Fingerprint names everything Text and Name's output depends on: Version, +// and the Unicode tables of the standard library and of x/text's +// normalisation, which a Go or x/text upgrade can change (review cache F4). +func Fingerprint() string { + return fmt.Sprintf("norm%d unicode%s nfd%s", Version, unicode.Version, unorm.Version) +} + // special holds the letters that do not decompose under Unicode NFD, so // Fold maps them explicitly. var special = map[rune]string{ diff --git a/man/krino.conf.5 b/man/krino.conf.5 index cd16ada..b00967a 100644 --- a/man/krino.conf.5 +++ b/man/krino.conf.5 @@ -529,9 +529,10 @@ records in which of the directory's content keywords the text contains, and answers later content tests from it while the file is unchanged. No text and no file names are stored; a file is known by device, inode, -size and modification time, which a move or rename within one filesystem +size, modification time and extension, which a move within one filesystem keeps. -The keywords are stored as written in the configuration. +The keywords are stored normalised, the way the test compares them, and +only those the directory still uses. .Pp A file is extracted again when it changes, or when a test asks about a keyword its entry was never checked against. @@ -539,10 +540,15 @@ Failures are never cached, and a file above .Ic max-read is refused before the cache is read. The whole cache is discarded when an extraction tool is installed, removed -or replaced, or when krino's extraction or normalisation changes. +or replaced, when krino's extraction or normalisation, the Go release or the +Unicode tables change, or when the directory's +.Ic max-read +changes. Each run keeps entries only for files still in the directory. A file edited in place with its size and modification time preserved keeps -its old answers; deleting the cache resets everything. +its old answers, as does a new file reusing a deleted file's inode with the +same size, extension and preserved modification time; deleting the cache +resets everything. .Sh ACTIONS .Ss Chains A file's chain is the actions of every matching rule, in rule order, and -- cgit v1.3