cache: fix some bugs after refactor

8 files changed, 97 insertions, 45 deletions

diff --git a/cache/bug_subcache.go b/cache/bug_subcache.go
index 14b56cdc..920fe1dc 100644
--- a/cache/bug_subcache.go
+++ b/cache/bug_subcache.go
@@ -44,7 +44,7 @@ func NewRepoCacheBug(repo repository.ClockedRepo,
 	sc := NewSubCache[*bug.Bug, *BugExcerpt, *BugCache](
 		repo, resolvers, getUserIdentity,
 		makeCached, NewBugExcerpt, makeIndexData, actions,
-		"bug", "bugs",
+		bug.Typename, bug.Namespace,
 		formatVersion, defaultMaxLoadedBugs,
 	)
 
@@ -124,7 +124,7 @@ func (c *RepoCacheBug) Query(q *query.Query) ([]entity.Id, error) {
 	if q.Search != nil {
 		foundBySearch = map[entity.Id]*BugExcerpt{}
 
-		index, err := c.repo.GetIndex("bug")
+		index, err := c.repo.GetIndex("bugs")
 		if err != nil {
 			return nil, err
 		}
diff --git a/cache/cached.go b/cache/cached.go
index ce1d6637..9f9e170d 100644
--- a/cache/cached.go
+++ b/cache/cached.go
@@ -9,6 +9,8 @@ import (
 	"github.com/MichaelMure/git-bug/util/lamport"
 )
 
+var _ CacheEntity = &CachedEntityBase[dag.Snapshot, dag.Operation]{}
+
 // CachedEntityBase provide the base function of an entity managed by the cache.
 type CachedEntityBase[SnapT dag.Snapshot, OpT dag.Operation] struct {
 	repo            repository.ClockedRepo
@@ -92,6 +94,10 @@ func (e *CachedEntityBase[SnapT, OpT]) NeedCommit() bool {
 	return e.entity.NeedCommit()
 }
 
+func (e *CachedEntityBase[SnapT, OpT]) Lock() {
+	e.mu.Lock()
+}
+
 func (e *CachedEntityBase[SnapT, OpT]) CreateLamportTime() lamport.Time {
 	return e.entity.CreateLamportTime()
 }
diff --git a/cache/identity_cache.go b/cache/identity_cache.go
index 00f5ae95..466b6150 100644
--- a/cache/identity_cache.go
+++ b/cache/identity_cache.go
@@ -1,18 +1,22 @@
 package cache
 
 import (
+	"sync"
+
 	"github.com/MichaelMure/git-bug/entities/identity"
 	"github.com/MichaelMure/git-bug/entity"
 	"github.com/MichaelMure/git-bug/repository"
 )
 
 var _ identity.Interface = &IdentityCache{}
+var _ CacheEntity = &IdentityCache{}
 
 // IdentityCache is a wrapper around an Identity for caching.
 type IdentityCache struct {
 	repo          repository.ClockedRepo
 	entityUpdated func(id entity.Id) error
 
+	mu sync.Mutex
 	*identity.Identity
 }
 
@@ -29,7 +33,9 @@ func (i *IdentityCache) notifyUpdated() error {
 }
 
 func (i *IdentityCache) Mutate(repo repository.RepoClock, f func(*identity.Mutator)) error {
+	i.mu.Lock()
 	err := i.Identity.Mutate(repo, f)
+	i.mu.Unlock()
 	if err != nil {
 		return err
 	}
@@ -37,7 +43,9 @@ func (i *IdentityCache) Mutate(repo repository.RepoClock, f func(*identity.Mutat
 }
 
 func (i *IdentityCache) Commit() error {
+	i.mu.Lock()
 	err := i.Identity.Commit(i.repo)
+	i.mu.Unlock()
 	if err != nil {
 		return err
 	}
@@ -45,9 +53,15 @@ func (i *IdentityCache) Commit() error {
 }
 
 func (i *IdentityCache) CommitAsNeeded() error {
+	i.mu.Lock()
 	err := i.Identity.CommitAsNeeded(i.repo)
+	i.mu.Unlock()
 	if err != nil {
 		return err
 	}
 	return i.notifyUpdated()
 }
+
+func (i *IdentityCache) Lock() {
+	i.mu.Lock()
+}
diff --git a/cache/identity_subcache.go b/cache/identity_subcache.go
index a623d0e1..f862ca8b 100644
--- a/cache/identity_subcache.go
+++ b/cache/identity_subcache.go
@@ -48,7 +48,7 @@ func NewRepoCacheIdentity(repo repository.ClockedRepo,
 	sc := NewSubCache[*identity.Identity, *IdentityExcerpt, *IdentityCache](
 		repo, resolvers, getUserIdentity,
 		makeCached, NewIdentityExcerpt, makeIndex, actions,
-		"identity", "identities",
+		identity.Typename, identity.Namespace,
 		formatVersion, defaultMaxLoadedBugs,
 	)
 
diff --git a/cache/multi_repo_cache.go b/cache/multi_repo_cache.go
index 12d26e26..007737ad 100644
--- a/cache/multi_repo_cache.go
+++ b/cache/multi_repo_cache.go
@@ -21,13 +21,13 @@ func NewMultiRepoCache() *MultiRepoCache {
 }
 
 // RegisterRepository register a named repository. Use this for multi-repo setup
-func (c *MultiRepoCache) RegisterRepository(ref string, repo repository.ClockedRepo) (*RepoCache, chan BuildEvent, error) {
-	r, events, err := NewRepoCache(repo)
+func (c *MultiRepoCache) RegisterRepository(name string, repo repository.ClockedRepo) (*RepoCache, chan BuildEvent, error) {
+	r, events, err := NewNamedRepoCache(repo, name)
 	if err != nil {
 		return nil, nil, err
 	}
 
-	c.repos[ref] = r
+	c.repos[name] = r
 	return r, events, nil
 }
 
@@ -55,9 +55,9 @@ func (c *MultiRepoCache) DefaultRepo() (*RepoCache, error) {
 	panic("unreachable")
 }
 
-// ResolveRepo retrieve a repository with a reference
-func (c *MultiRepoCache) ResolveRepo(ref string) (*RepoCache, error) {
-	r, ok := c.repos[ref]
+// ResolveRepo retrieve a repository by name
+func (c *MultiRepoCache) ResolveRepo(name string) (*RepoCache, error) {
+	r, ok := c.repos[name]
 	if !ok {
 		return nil, fmt.Errorf("unknown repo")
 	}
diff --git a/cache/repo_cache.go b/cache/repo_cache.go
index 2cac711b..7852ec7d 100644
--- a/cache/repo_cache.go
+++ b/cache/repo_cache.go
@@ -27,6 +27,7 @@ var _ repository.RepoCommon = &RepoCache{}
 var _ repository.RepoConfig = &RepoCache{}
 var _ repository.RepoKeyring = &RepoCache{}
 
+// cacheMgmt is the expected interface for a sub-cache.
 type cacheMgmt interface {
 	Typename() string
 	Load() error
@@ -58,7 +59,7 @@ type RepoCache struct {
 	// the name of the repository, as defined in the MultiRepoCache
 	name string
 
-	// resolvers for all known entities
+	// resolvers for all known entities and excerpts
 	resolvers entity.Resolvers
 
 	bugs       *RepoCacheBug
@@ -71,10 +72,16 @@ type RepoCache struct {
 	userIdentityId entity.Id
 }
 
+// NewRepoCache create or open an unnamed (aka default) cache on top of a raw repository.
+// If the returned BuildEvent channel is not nil, the caller is expected to read all events before the cache is considered
+// ready to use.
 func NewRepoCache(r repository.ClockedRepo) (*RepoCache, chan BuildEvent, error) {
 	return NewNamedRepoCache(r, "")
 }
 
+// NewNamedRepoCache create or open a named cache on top of a raw repository.
+// If the returned BuildEvent channel is not nil, the caller is expected to read all events before the cache is considered
+// ready to use.
 func NewNamedRepoCache(r repository.ClockedRepo, name string) (*RepoCache, chan BuildEvent, error) {
 	c := &RepoCache{
 		repo: r,
@@ -96,16 +103,12 @@ func NewNamedRepoCache(r repository.ClockedRepo, name string) (*RepoCache, chan
 
 	err := c.lock()
 	if err != nil {
-		closed := make(chan BuildEvent)
-		close(closed)
-		return &RepoCache{}, closed, err
+		return &RepoCache{}, nil, err
 	}
 
 	err = c.load()
 	if err == nil {
-		closed := make(chan BuildEvent)
-		close(closed)
-		return c, closed, nil
+		return c, nil, nil
 	}
 
 	// Cache is either missing, broken or outdated. Rebuilding.
@@ -114,6 +117,23 @@ func NewNamedRepoCache(r repository.ClockedRepo, name string) (*RepoCache, chan
 	return c, events, nil
 }
 
+func NewRepoCacheNoEvents(r repository.ClockedRepo) (*RepoCache, error) {
+	cache, events, err := NewRepoCache(r)
+	if err != nil {
+		return nil, err
+	}
+	if events != nil {
+		for event := range events {
+			if event.Err != nil {
+				for range events {
+				}
+				return nil, err
+			}
+		}
+	}
+	return cache, nil
+}
+
 // Bugs gives access to the Bug entities
 func (c *RepoCache) Bugs() *RepoCacheBug {
 	return c.bugs
diff --git a/cache/repo_cache_test.go b/cache/repo_cache_test.go
index 939cb154..796b5db9 100644
--- a/cache/repo_cache_test.go
+++ b/cache/repo_cache_test.go
@@ -8,23 +8,17 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/MichaelMure/git-bug/entities/bug"
+	"github.com/MichaelMure/git-bug/entities/identity"
 	"github.com/MichaelMure/git-bug/entity"
 	"github.com/MichaelMure/git-bug/query"
 	"github.com/MichaelMure/git-bug/repository"
 )
 
-func noBuildEventErrors(t *testing.T, c chan BuildEvent) {
-	t.Helper()
-	for event := range c {
-		require.NoError(t, event.Err)
-	}
-}
-
 func TestCache(t *testing.T) {
 	repo := repository.CreateGoGitTestRepo(t, false)
 
-	cache, events, err := NewRepoCache(repo)
-	noBuildEventErrors(t, events)
+	cache, err := NewRepoCacheNoEvents(repo)
 	require.NoError(t, err)
 
 	// Create, set and get user identity
@@ -43,17 +37,27 @@ func TestCache(t *testing.T) {
 	// Two identical identities yield a different id
 	require.NotEqual(t, iden1.Id(), iden2.Id())
 
+	indexCount := func(name string) uint64 {
+		idx, err := repo.GetIndex(name)
+		require.NoError(t, err)
+		count, err := idx.DocCount()
+		require.NoError(t, err)
+		return count
+	}
+
 	// There is now two identities in the cache
 	require.Len(t, cache.Identities().AllIds(), 2)
 	require.Len(t, cache.identities.excerpts, 2)
 	require.Len(t, cache.identities.cached, 2)
+	require.Equal(t, uint64(2), indexCount(identity.Namespace))
+	require.Equal(t, uint64(0), indexCount(bug.Namespace))
 
 	// Create a bug
 	bug1, _, err := cache.Bugs().New("title", "message")
 	require.NoError(t, err)
 
 	// It's possible to create two identical bugs
-	bug2, _, err := cache.Bugs().New("title", "message")
+	bug2, _, err := cache.Bugs().New("title", "marker")
 	require.NoError(t, err)
 
 	// two identical bugs yield a different id
@@ -63,6 +67,8 @@ func TestCache(t *testing.T) {
 	require.Len(t, cache.Bugs().AllIds(), 2)
 	require.Len(t, cache.bugs.excerpts, 2)
 	require.Len(t, cache.bugs.cached, 2)
+	require.Equal(t, uint64(2), indexCount(identity.Namespace))
+	require.Equal(t, uint64(2), indexCount(bug.Namespace))
 
 	// Resolving
 	_, err = cache.Identities().Resolve(iden1.Id())
@@ -86,6 +92,12 @@ func TestCache(t *testing.T) {
 	require.NoError(t, err)
 	require.Len(t, res, 2)
 
+	q, err = query.Parse("status:open marker") // full-text search
+	require.NoError(t, err)
+	res, err = cache.Bugs().Query(q)
+	require.NoError(t, err)
+	require.Len(t, res, 1)
+
 	// Close
 	require.NoError(t, cache.Close())
 	require.Empty(t, cache.bugs.cached)
@@ -95,13 +107,14 @@ func TestCache(t *testing.T) {
 
 	// Reload, only excerpt are loaded, but as we need to load the identities used in the bugs
 	// to check the signatures, we also load the identity used above
-	cache, events, err = NewRepoCache(repo)
-	noBuildEventErrors(t, events)
+	cache, err = NewRepoCacheNoEvents(repo)
 	require.NoError(t, err)
 	require.Len(t, cache.bugs.cached, 0)
 	require.Len(t, cache.bugs.excerpts, 2)
 	require.Len(t, cache.identities.cached, 0)
 	require.Len(t, cache.identities.excerpts, 2)
+	require.Equal(t, uint64(2), indexCount(identity.Namespace))
+	require.Equal(t, uint64(2), indexCount(bug.Namespace))
 
 	// Resolving load from the disk
 	_, err = cache.Identities().Resolve(iden1.Id())
@@ -122,12 +135,10 @@ func TestCache(t *testing.T) {
 func TestCachePushPull(t *testing.T) {
 	repoA, repoB, _ := repository.SetupGoGitReposAndRemote(t)
 
-	cacheA, events, err := NewRepoCache(repoA)
-	noBuildEventErrors(t, events)
+	cacheA, err := NewRepoCacheNoEvents(repoA)
 	require.NoError(t, err)
 
-	cacheB, events, err := NewRepoCache(repoB)
-	noBuildEventErrors(t, events)
+	cacheB, err := NewRepoCacheNoEvents(repoB)
 	require.NoError(t, err)
 
 	// Create, set and get user identity
@@ -190,8 +201,7 @@ func TestRemove(t *testing.T) {
 	err = repo.AddRemote("remoteB", remoteB.GetLocalRemote())
 	require.NoError(t, err)
 
-	repoCache, events, err := NewRepoCache(repo)
-	noBuildEventErrors(t, events)
+	repoCache, err := NewRepoCacheNoEvents(repo)
 	require.NoError(t, err)
 
 	rene, err := repoCache.Identities().New("René Descartes", "rene@descartes.fr")
@@ -230,8 +240,7 @@ func TestRemove(t *testing.T) {
 
 func TestCacheEviction(t *testing.T) {
 	repo := repository.CreateGoGitTestRepo(t, false)
-	repoCache, events, err := NewRepoCache(repo)
-	noBuildEventErrors(t, events)
+	repoCache, err := NewRepoCacheNoEvents(repo)
 	require.NoError(t, err)
 	repoCache.setCacheSize(2)
 
@@ -299,8 +308,7 @@ func TestLongDescription(t *testing.T) {
 
 	repo := repository.CreateGoGitTestRepo(t, false)
 
-	backend, events, err := NewRepoCache(repo)
-	noBuildEventErrors(t, events)
+	backend, err := NewRepoCacheNoEvents(repo)
 	require.NoError(t, err)
 
 	i, err := backend.Identities().New("René Descartes", "rene@descartes.fr")
diff --git a/cache/subcache.go b/cache/subcache.go
index af75938a..7757ce82 100644
--- a/cache/subcache.go
+++ b/cache/subcache.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"encoding/gob"
 	"fmt"
+	"path/filepath"
 	"sync"
 
 	"github.com/pkg/errors"
@@ -21,6 +22,7 @@ type Excerpt interface {
 type CacheEntity interface {
 	Id() entity.Id
 	NeedCommit() bool
+	Lock()
 }
 
 type getUserIdentityFunc func() (*IdentityCache, error)
@@ -94,7 +96,7 @@ func (sc *SubCache[EntityT, ExcerptT, CacheT]) Load() error {
 	sc.mu.Lock()
 	defer sc.mu.Unlock()
 
-	f, err := sc.repo.LocalStorage().Open(sc.namespace + "-file")
+	f, err := sc.repo.LocalStorage().Open(filepath.Join("cache", sc.namespace))
 	if err != nil {
 		return err
 	}
@@ -123,7 +125,7 @@ func (sc *SubCache[EntityT, ExcerptT, CacheT]) Load() error {
 
 	sc.excerpts = aux.Excerpts
 
-	index, err := sc.repo.GetIndex(sc.typename)
+	index, err := sc.repo.GetIndex(sc.namespace)
 	if err != nil {
 		return err
 	}
@@ -162,7 +164,7 @@ func (sc *SubCache[EntityT, ExcerptT, CacheT]) write() error {
 		return err
 	}
 
-	f, err := sc.repo.LocalStorage().Create(sc.namespace + "-file")
+	f, err := sc.repo.LocalStorage().Create(filepath.Join("cache", sc.namespace))
 	if err != nil {
 		return err
 	}
@@ -180,7 +182,7 @@ func (sc *SubCache[EntityT, ExcerptT, CacheT]) Build() error {
 
 	allEntities := sc.actions.ReadAllWithResolver(sc.repo, sc.resolvers())
 
-	index, err := sc.repo.GetIndex(sc.typename)
+	index, err := sc.repo.GetIndex(sc.namespace)
 	if err != nil {
 		return err
 	}
@@ -462,7 +464,7 @@ func (sc *SubCache[EntityT, ExcerptT, CacheT]) entityUpdated(id entity.Id) error
 	sc.excerpts[id] = sc.makeExcerpt(e)
 	sc.mu.Unlock()
 
-	index, err := sc.repo.GetIndex(sc.typename)
+	index, err := sc.repo.GetIndex(sc.namespace)
 	if err != nil {
 		return err
 	}
@@ -489,8 +491,10 @@ func (sc *SubCache[EntityT, ExcerptT, CacheT]) evictIfNeeded() {
 			continue
 		}
 
-		// TODO
-		// b.Lock()
+		// as a form of assurance that evicted entities don't get manipulated, we lock them here.
+		// if something try to do it anyway, it will lock the program and make it obvious.
+		b.Lock()
+
 		sc.lru.Remove(id)
 		delete(sc.cached, id)