From de80fd84c289727dc69256a7abf3457307675f01 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sun, 25 Oct 2026 19:06:58 +1100 Subject: [PATCH 1/3] Keep stale virtual member copy when the remote lookup fails --- internal/virtual/engine.go | 4 ++-- internal/virtual/engine_stale_test.go | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/internal/virtual/engine.go b/internal/virtual/engine.go index 15a973a..a48d388 100644 --- a/internal/virtual/engine.go +++ b/internal/virtual/engine.go @@ -548,8 +548,8 @@ func (e *Engine) fetchAlpineMember(ctx context.Context, name, dir string) (*Alpi // the background, one refresh per member at a time; the next merge picks it up. func staleMember[T any](ctx context.Context, e *Engine, key, name string, fetch func(context.Context) (*T, error)) (*T, error) { sk := key + "\x00" + name - remote, err := e.getRemote(ctx, name) - if err != nil || !remote.ServeStale { + // A failed lookup keeps the copy: a transient DB error must not force a blocking fetch. + if remote, err := e.getRemote(ctx, name); err == nil && !remote.ServeStale { e.mu.Lock() delete(e.stale, sk) e.mu.Unlock() diff --git a/internal/virtual/engine_stale_test.go b/internal/virtual/engine_stale_test.go index c7d0e16..cadb3f1 100644 --- a/internal/virtual/engine_stale_test.go +++ b/internal/virtual/engine_stale_test.go @@ -328,3 +328,18 @@ func TestMergePrunesExpiredCopiesOfOtherMembers(t *testing.T) { t.Fatal("any merge should prune expired copies of removed members") } } + +func TestServeStaleLookupErrorKeepsCopy(t *testing.T) { + e, _ := staleEngine(true, func() *RPMMember { return rpmPkgMember("a", "1") }) + v1 := fetchRepomd(t, e) + e.getRemote = func(context.Context, string) (*models.Remote, error) { return nil, errors.New("db down") } + if got := fetchRepomd(t, e); got != v1 { + t.Fatal("a failed remote lookup should keep serving the previous copy") + } + waitRefresh(e, rpmStaleKey) + e.mu.Lock() + defer e.mu.Unlock() + if e.stale[rpmStaleKey] == nil || e.stale[rpmStaleKey].m == nil { + t.Fatal("a failed remote lookup must not drop the copy") + } +} -- 2.47.3 From f1fd1a442cfbe32c9cd2ac99ef2b18eecc8c15e7 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sun, 25 Oct 2026 19:16:16 +1100 Subject: [PATCH 2/3] Create virtual member stale copies only after a successful serve_stale lookup --- internal/virtual/engine.go | 9 +++++++-- internal/virtual/engine_stale_test.go | 29 +++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/internal/virtual/engine.go b/internal/virtual/engine.go index a48d388..bf477e3 100644 --- a/internal/virtual/engine.go +++ b/internal/virtual/engine.go @@ -548,8 +548,8 @@ func (e *Engine) fetchAlpineMember(ctx context.Context, name, dir string) (*Alpi // the background, one refresh per member at a time; the next merge picks it up. func staleMember[T any](ctx context.Context, e *Engine, key, name string, fetch func(context.Context) (*T, error)) (*T, error) { sk := key + "\x00" + name - // A failed lookup keeps the copy: a transient DB error must not force a blocking fetch. - if remote, err := e.getRemote(ctx, name); err == nil && !remote.ServeStale { + remote, lookupErr := e.getRemote(ctx, name) + if lookupErr == nil && !remote.ServeStale { e.mu.Lock() delete(e.stale, sk) e.mu.Unlock() @@ -560,6 +560,11 @@ func staleMember[T any](ctx context.Context, e *Engine, key, name string, fetch e.stale = map[string]*staleEntry{} } prev := e.stale[sk] + // A failed lookup serves an existing copy but never creates one. + if lookupErr != nil && (prev == nil || prev.m == nil) { + e.mu.Unlock() + return fetch(ctx) + } // A cold fetch holds a placeholder so eviction or toggle-off during it is seen. token := prev if prev == nil || prev.m == nil { diff --git a/internal/virtual/engine_stale_test.go b/internal/virtual/engine_stale_test.go index cadb3f1..0545848 100644 --- a/internal/virtual/engine_stale_test.go +++ b/internal/virtual/engine_stale_test.go @@ -343,3 +343,32 @@ func TestServeStaleLookupErrorKeepsCopy(t *testing.T) { t.Fatal("a failed remote lookup must not drop the copy") } } + +func TestServeStaleLookupErrorColdFetchStoresNothing(t *testing.T) { + e, calls := staleEngine(true, func() *RPMMember { return rpmPkgMember("a", "1") }) + e.getRemote = func(context.Context, string) (*models.Remote, error) { return nil, errors.New("db down") } + fetchRepomd(t, e) + if calls.Load() != 1 { + t.Fatalf("cold member with a failed lookup should block on one fetch, got %d", calls.Load()) + } + e.mu.Lock() + defer e.mu.Unlock() + if _, ok := e.stale[rpmStaleKey]; ok { + t.Fatal("a failed lookup must not create a copy") + } +} + +func TestNonServeStaleLookupErrorStoresNothing(t *testing.T) { + e, calls := staleEngine(false, func() *RPMMember { return rpmPkgMember("a", "1") }) + fetchRepomd(t, e) + e.getRemote = func(context.Context, string) (*models.Remote, error) { return nil, errors.New("db down") } + fetchRepomd(t, e) + if calls.Load() != 2 { + t.Fatalf("each merge should fetch the member, got %d", calls.Load()) + } + e.mu.Lock() + defer e.mu.Unlock() + if _, ok := e.stale[rpmStaleKey]; ok { + t.Fatal("a failed lookup must not create a copy for a non-serve_stale member") + } +} -- 2.47.3 From b54eb5fe2fe8bfd2750095b0d33cc6b9bd3c35b3 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sun, 25 Oct 2026 19:23:15 +1100 Subject: [PATCH 3/3] Prove lookup-error stale serving and cold-fetch isolation in tests --- internal/virtual/engine_stale_test.go | 91 +++++++++++++++++++++++++-- 1 file changed, 86 insertions(+), 5 deletions(-) diff --git a/internal/virtual/engine_stale_test.go b/internal/virtual/engine_stale_test.go index 0545848..389bd5a 100644 --- a/internal/virtual/engine_stale_test.go +++ b/internal/virtual/engine_stale_test.go @@ -329,18 +329,99 @@ func TestMergePrunesExpiredCopiesOfOtherMembers(t *testing.T) { } } +func primary(m any) string { return string(m.(*RPMMember).Data["primary"]) } + func TestServeStaleLookupErrorKeepsCopy(t *testing.T) { - e, _ := staleEngine(true, func() *RPMMember { return rpmPkgMember("a", "1") }) + release := make(chan struct{}) + started := make(chan struct{}, 1) + var slow atomic.Bool + e, calls := staleEngine(true, func() *RPMMember { + if slow.Load() { + select { + case started <- struct{}{}: + default: + } + <-release + return rpmPkgMember("a", "2") + } + return rpmPkgMember("a", "1") + }) v1 := fetchRepomd(t, e) + slow.Store(true) e.getRemote = func(context.Context, string) (*models.Remote, error) { return nil, errors.New("db down") } - if got := fetchRepomd(t, e); got != v1 { - t.Fatal("a failed remote lookup should keep serving the previous copy") + + done := make(chan string) + go func() { done <- fetchRepomd(t, e) }() + select { + case got := <-done: + if got != v1 { + t.Fatal("a failed remote lookup should serve the previous copy") + } + case <-time.After(time.Second): + t.Fatal("merge blocked on the member fetch instead of serving the copy") } + select { + case <-started: + case <-time.After(time.Second): + t.Fatal("no background refresh started") + } + if n := calls.Load(); n != 2 { + t.Fatalf("member fetched %d times, want 2 (cold + one background refresh)", n) + } + close(release) waitRefresh(e, rpmStaleKey) e.mu.Lock() defer e.mu.Unlock() - if e.stale[rpmStaleKey] == nil || e.stale[rpmStaleKey].m == nil { - t.Fatal("a failed remote lookup must not drop the copy") + if se := e.stale[rpmStaleKey]; se == nil || se.m == nil || primary(se.m) != primary(rpmPkgMember("a", "2")) { + t.Fatal("the background refresh should replace the copy") + } +} + +func TestServeStaleLookupErrorDuringColdFetch(t *testing.T) { + e := fakeEngine(nil) + e.getRemote = staleRemote + ctx := context.Background() + entered, release := make(chan struct{}), make(chan struct{}) + first := make(chan *RPMMember) + go func() { + m, err := staleMember(ctx, e, "rpm\x00v", "a", func(context.Context) (*RPMMember, error) { + close(entered) + <-release + return rpmPkgMember("a", "1"), nil + }) + if err != nil { + t.Error(err) + } + first <- m + }() + <-entered + e.mu.Lock() + placeholder := e.stale[rpmStaleKey] + e.mu.Unlock() + if placeholder == nil || placeholder.m != nil { + t.Fatal("a cold fetch should hold an empty placeholder") + } + + e.getRemote = func(context.Context, string) (*models.Remote, error) { return nil, errors.New("db down") } + m, err := staleMember(ctx, e, "rpm\x00v", "a", func(context.Context) (*RPMMember, error) { return rpmPkgMember("a", "2"), nil }) + if err != nil || primary(m) != primary(rpmPkgMember("a", "2")) { + t.Fatalf("a failed lookup during a cold fetch should fetch directly, got %v", err) + } + e.mu.Lock() + if e.stale[rpmStaleKey] != placeholder || placeholder.m != nil { + e.mu.Unlock() + t.Fatal("a failed lookup must not touch the in-flight placeholder") + } + e.mu.Unlock() + + close(release) + if got := <-first; got == nil || primary(got) != primary(rpmPkgMember("a", "1")) { + t.Fatal("the cold fetch should return its own result") + } + e.mu.Lock() + defer e.mu.Unlock() + if se := e.stale[rpmStaleKey]; se == nil || se.m == nil || primary(se.m) != primary(rpmPkgMember("a", "1")) { + t.Fatal("the cold fetch should store its own result") } } -- 2.47.3