refactor(sdk): cleanup tests, rename model.go, fix history compactor race#43
Merged
Conversation
The file's content is the Message type (not generic "model"); aligning the filename with its primary symbol makes navigation more intuitive. Pure rename, no API change. Made-with: Cursor
TestAllCategories, TestRegisterCategory and TestAllCategoryStrings exclusively exercise recall.Category APIs but were living in sdk/history/history_test.go, which forced an otherwise unnecessary history -> recall test-only dependency. Move them to a new sdk/recall/categories_test.go (package recall_test, matching the other black-box tests in the package) and drop the history import. Also drops a dead code block in the original TestAllCategories that built an "expected" map and only assigned its values to _ without asserting on them. Made-with: Cursor
The convQueue.recovered flag was read/written from two paths without a
shared mutex:
- lazyRecover, called from the per-conversation worker goroutine,
did an unlocked read+write on q.recovered.
- kickoffStartupRecovery's pre-marking pass wrote
q.recovered = true while holding compactor.mu.
CI's race detector caught this on TestCoordinator_ArchiveRunsThroughQueue
(sdk/history/compactor.go:420 vs :562). Switch the field to atomic.Bool
and use CompareAndSwap inside lazyRecover so the "already recovered"
short-circuit and the "mark recovered" step are a single atomic step.
The startup pre-marking path uses Store(true), which interleaves
correctly with the worker's CAS regardless of which one observes the
queue first.
Made-with: Cursor
lIang70
changed the title
CI's race detector (Go 1.26) caught a wg.Add / wg.Wait ordering
violation on TestEventReloader_CloseStopsRun. The test launches Run on
one goroutine and immediately calls Close on another:
go func() { _ = r.Run(ctx); close(done) }()
if err := r.Close(); err != nil { ... }
The previous implementation called wg.Add(1) inside Run, so when the
scheduler let Close win the race, Close's wg.Wait observed counter=0
and returned without ever waiting — silently violating Close's
contract that "after Close returns, no further Rebuild / timer flush
can fire" because the late-starting Run could still enqueue and run
flushes after the notifier was closed. sync.WaitGroup also explicitly
forbids wg.Add to race with wg.Wait when the counter starts at zero.
Move wg.Add(1) into NewEventReloader so it is strictly happens-before
any Close-side wg.Wait. The Add is conditional on target/notifier
being non-nil so the documented "nil → no-op Run/Close" path keeps
the counter at zero and Close stays a no-op without deadlocking. Run
is documented as at-most-once, matching how it has always been used
in this package.
Made-with: Cursor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three small changes inside
sdk/:sdk/model: renamemodel.go/model_test.gotomessage.go/message_test.go. The file's content is theMessagetype, so the new filename matches its primary symbol. Pure rename, no API change.sdk/recall&sdk/history: moveTestAllCategories,TestRegisterCategoryandTestAllCategoryStringsout ofsdk/history/history_test.gointo a newsdk/recall/categories_test.go(packagerecall_test). These tests only exerciserecall.CategoryAPIs and were the sole reasonhistoryhad a (test-only) import ofrecall. After the move,historyno longer importsrecallat all, removing a virtual cross-package dependency. Also drops a dead code block in the original test that built anexpectedmap and discarded its values without asserting.sdk/history: fix a real data race the CI race detector caught onTestCoordinator_ArchiveRunsThroughQueue.convQueue.recoveredwas touched from two unrelated goroutines without a shared mutex (lazyRecoverran on the per-conversation worker withoutcompactor.mu;kickoffStartupRecoverywrote the same field while holdingcompactor.mu). Switched the field toatomic.Booland usedCompareAndSwapinsidelazyRecoverso "already recovered?" and "mark recovered" become a single atomic step.Auto-tag
The previous version of this PR carried
[skip-tag:sdk]because it was a pure test relocation + filename rename. The newly added race fix is a production-code change insdk/history/compactor.go, so the marker has been removed andsdkshould receive a normal patch-version bump on merge.Test plan
go test ./sdk/history/... ./sdk/recall/...passes locallygo test -race ./sdk/...passes locally (full sdk module, including the previously-flakyTestCoordinator_ArchiveRunsThroughQueue)go test -race -run TestCoordinator_ArchiveRunsThroughQueue -count=200 ./sdk/history/passesrg 'flowcraft/sdk/recall' sdk/historyreturns no matches (history no longer imports recall)