From 25afcfa1bc9b6f1690f217dab02ca5096d9da4a8 Mon Sep 17 00:00:00 2001 From: Gerard Louis Recinto Date: Sat, 19 Sep 2026 16:47:48 -0700 Subject: [PATCH] add real GetResults coverage for swarm store, drop the inert found check staticcheck SA4006 flagged found in GetResults as a dead store, the if !found block right after it was comments only, no actual handling. zero tests existed for this file at all, so before touching anything wrote 3 tests against the real storage engine, not a mock: empty store, a jobID that sorts past every key already in the tree (the case most likely to expose a stale-cursor bug if found actually mattered), and a real multi-result positive case. all three passed against the code exactly as it was, confirming the prefix-check loop right after Find is a correct, sufficient guard on its own regardless of the found flag. removed the dead if-block, replaced the author's own uncertain comment ('for now, let's assume...') with what's now verified fact. --- ai/swarm/store.go | 15 +++--- ai/swarm/store_test.go | 104 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 8 deletions(-) create mode 100644 ai/swarm/store_test.go diff --git a/ai/swarm/store.go b/ai/swarm/store.go index 1e5b51a39..833016c1e 100644 --- a/ai/swarm/store.go +++ b/ai/swarm/store.go @@ -94,16 +94,15 @@ func (s *Store) GetResults(ctx context.Context, jobID string) ([]JobResult, erro var results []JobResult - found, err := s.results.Find(ctx, startKey, true) - if err != nil { + // The found flag isn't checked directly: whether or not Find landed + // exactly on startKey, the loop below verifies every candidate item + // against the prefix itself and breaks the moment it doesn't match, + // which is a correct and sufficient guard on its own (verified in + // store_test.go against the real storage engine, including the case + // of a jobID that sorts past every key already in the tree). + if _, err := s.results.Find(ctx, startKey, true); err != nil { return nil, err } - if !found { - // Check if we are at a key that starts with the prefix (Find behavior varies) - // If not found, we might need to check Next if Find landed before. - // For now, let's assume standard SOP behavior: Find(true) positions at >= key. - // We just need to check the current item. - } // Iterate for { diff --git a/ai/swarm/store_test.go b/ai/swarm/store_test.go new file mode 100644 index 000000000..f66675b5b --- /dev/null +++ b/ai/swarm/store_test.go @@ -0,0 +1,104 @@ +package swarm + +import ( + "context" + "os" + "testing" + + "github.com/sharedcode/joltrin" + "github.com/sharedcode/joltrin/infs" +) + +func newTestStore(t *testing.T) (*Store, context.Context) { + t.Helper() + tmpDir, err := os.MkdirTemp("", "swarm-store-test-*") + if err != nil { + t.Fatalf("MkdirTemp: %v", err) + } + t.Cleanup(func() { os.RemoveAll(tmpDir) }) + + ctx := context.Background() + trans, err := infs.NewTransaction(ctx, sop.TransactionOptions{ + Mode: sop.ForWriting, + StoresFolders: []string{tmpDir}, + CacheType: sop.InMemory, + }) + if err != nil { + t.Fatalf("NewTransaction: %v", err) + } + if err := trans.Begin(ctx); err != nil { + t.Fatalf("Begin: %v", err) + } + + store, err := NewStore(ctx, trans) + if err != nil { + t.Fatalf("NewStore: %v", err) + } + return store, ctx +} + +// GetResults positions the results cursor with Find(startKey, true) but +// never checks the returned found flag - staticcheck flags it as a dead +// store (SA4006). The loop right after relies purely on a prefix check +// to decide whether the current item belongs to the requested job, so +// the found value being unused isn't itself a bug as long as that +// prefix check is a safe net on its own. These three cases verify it +// actually is, on the real storage engine, not a mock. + +func TestGetResults_EmptyStoreReturnsNoResults(t *testing.T) { + store, ctx := newTestStore(t) + + results, err := store.GetResults(ctx, "does-not-exist") + if err != nil { + t.Fatalf("GetResults on empty store: %v", err) + } + if len(results) != 0 { + t.Errorf("expected 0 results on an empty store, got %d: %+v", len(results), results) + } +} + +func TestGetResults_QueryPastEndOfTreeReturnsNoResults(t *testing.T) { + store, ctx := newTestStore(t) + + if err := store.SubmitResult(ctx, JobResult{JobID: "job-aaa", NodeID: "n1"}); err != nil { + t.Fatalf("SubmitResult: %v", err) + } + + // "job-zzz" sorts after every key in the tree, so Find(startKey, true) + // cannot land on or after it - this is the case most likely to expose + // a stale-cursor bug if the found flag actually mattered. + results, err := store.GetResults(ctx, "job-zzz") + if err != nil { + t.Fatalf("GetResults past end of tree: %v", err) + } + if len(results) != 0 { + t.Errorf("expected 0 results querying a jobID past the end of the tree, got %d: %+v", len(results), results) + } +} + +func TestGetResults_ReturnsOnlyMatchingJobResults(t *testing.T) { + store, ctx := newTestStore(t) + + if err := store.SubmitResult(ctx, JobResult{JobID: "job-a", NodeID: "n1", Output: "a1"}); err != nil { + t.Fatalf("SubmitResult: %v", err) + } + if err := store.SubmitResult(ctx, JobResult{JobID: "job-a", NodeID: "n2", Output: "a2"}); err != nil { + t.Fatalf("SubmitResult: %v", err) + } + if err := store.SubmitResult(ctx, JobResult{JobID: "job-b", NodeID: "n1", Output: "b1"}); err != nil { + t.Fatalf("SubmitResult: %v", err) + } + + results, err := store.GetResults(ctx, "job-a") + if err != nil { + t.Fatalf("GetResults: %v", err) + } + if len(results) != 2 { + t.Fatalf("expected 2 results for job-a, got %d: %+v", len(results), results) + } + for _, r := range results { + if r.JobID != "job-a" { + t.Errorf("GetResults(\"job-a\") returned a result for a different job: %+v", r) + } + } +}