From 971b992e785a69b6b12a740f9906741e022a6554 Mon Sep 17 00:00:00 2001 From: Liu Yuan Date: Sat, 4 Apr 2026 11:07:29 +0800 Subject: [PATCH] fix(seahorse): add mutex for assembler lazy initialization Issue #5 from PR review: The check-then-create pattern for e.assembler was a data race when multiple goroutines called Assemble() concurrently: if e.assembler == nil { e.assembler = &Assembler{...} } Changes: - Add assemblerMu sync.Mutex to Engine struct - Add initAssemblerOnce() using double-checked locking (same pattern as initCompactionOnce) - Add TestAssemblerLazyInitRace to verify thread-safety --- .gitignore | 2 ++ pkg/seahorse/short_engine.go | 16 ++++++++--- pkg/seahorse/short_engine_test.go | 44 +++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/.gitignore b/.gitignore index b869ecc33..135867842 100644 --- a/.gitignore +++ b/.gitignore @@ -67,3 +67,5 @@ web/backend/dist/* .claude/ docker/data + +.omc/ diff --git a/pkg/seahorse/short_engine.go b/pkg/seahorse/short_engine.go index 11cbe01d7..4cd4d3887 100644 --- a/pkg/seahorse/short_engine.go +++ b/pkg/seahorse/short_engine.go @@ -58,6 +58,7 @@ type Engine struct { compaction *CompactionEngine compactionMu sync.Mutex assembler *Assembler + assemblerMu sync.Mutex retrieval *RetrievalEngine config Config complete CompleteFn @@ -307,9 +308,7 @@ func (e *Engine) Assemble(ctx context.Context, sessionKey string, input Assemble return nil, fmt.Errorf("get conversation: %w", err) } - if e.assembler == nil { - e.assembler = &Assembler{store: e.store, config: e.config} - } + e.initAssemblerOnce() return e.assembler.Assemble(ctx, conv.ConversationID, input) } @@ -362,6 +361,17 @@ func (e *Engine) initCompactionOnce() { } } +// initAssemblerOnce lazily initializes the assembler. +func (e *Engine) initAssemblerOnce() { + if e.assembler == nil { + e.assemblerMu.Lock() + defer e.assemblerMu.Unlock() + if e.assembler == nil { + e.assembler = &Assembler{store: e.store, config: e.config} + } + } +} + // IngestMessages is an alias for Ingest. func (e *Engine) IngestMessages(ctx context.Context, sessionKey string, messages []Message) (*IngestResult, error) { return e.Ingest(ctx, sessionKey, messages) diff --git a/pkg/seahorse/short_engine_test.go b/pkg/seahorse/short_engine_test.go index 401b6ca39..d2f108efd 100644 --- a/pkg/seahorse/short_engine_test.go +++ b/pkg/seahorse/short_engine_test.go @@ -1329,3 +1329,47 @@ func TestBootstrapAnchorWithDuplicateContent_Simple(t *testing.T) { t.Errorf("last message content = %q, want 'D'", lastMsg.Content) } } + +// --- Assembler lazy init race detection --- + +func TestAssemblerLazyInitRace(t *testing.T) { + // This test verifies that Assemble() lazy initialization of e.assembler + // is thread-safe. The original code has a data race: + // if e.assembler == nil { + // e.assembler = &Assembler{...} + // } + + // Run multiple iterations to increase chance of catching race + for i := 0; i < 30; i++ { + // Create fresh engine with nil assembler + e := newTestEngine(t) + + ctx := context.Background() + sessionKey := fmt.Sprintf("race-test-%d", i) + + // Add message first (avoid SQLite concurrency issues) + _, err := e.Ingest(ctx, sessionKey, []Message{ + {Role: "user", Content: "hello", TokenCount: 5}, + }) + if err != nil { + t.Fatalf("Ingest: %v", err) + } + + // Use a barrier to ensure all goroutines start at the same time + start := make(chan struct{}) + var wg sync.WaitGroup + + for j := 0; j < 20; j++ { + wg.Add(1) + go func() { + defer wg.Done() + <-start // Wait for all goroutines to be ready + e.Assemble(ctx, sessionKey, AssembleInput{Budget: 1000}) + }() + } + + // Start all goroutines simultaneously + close(start) + wg.Wait() + } +}