🐛 bug: fix data race on lazy appListKeys generation in Render#4440
Conversation
Test_Ctx_RenderWithLocals runs two parallel subtests that share one app and call Render concurrently. renderExtensions lazily generated app.mountFields.appListKeys behind a non-atomic `len(...) == 0` guard, so both goroutines could enter generateAppListKeys at once: one appending to the slice while the other read its length and later iterated it during Render, tripping the race detector. Guard the generation with a dedicated sync.Once (appListKeysOnce) shared by both callers (the lazy Render path and mountStartupProcess). The keys are now generated exactly once with proper happens-before ordering; concurrent renders block until the first generation completes. Sharing one Once also removes a latent double-append that was possible when the lazy path ran before mountStartupProcess. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughA Changessync.Once guard for appListKeys
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4440 +/- ##
==========================================
- Coverage 91.59% 91.59% -0.01%
==========================================
Files 134 134
Lines 13518 13517 -1
==========================================
- Hits 12382 12381 -1
Misses 722 722
Partials 414 414
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
Test_Ctx_RenderWithLocalsintermittently fails the race detector in CI (see failed run). The test runs two parallel subtests (EmptyBind,NilBind) that share a single*Appand callRenderconcurrently.renderExtensions(ctx.go) lazily generatedapp.mountFields.appListKeysbehind a non-atomic guard:The
lencheck and theappendinsidegenerateAppListKeys(mount.go) are not atomic, so two concurrentRendercalls both observelen == 0and both write to the shared slice while the other reads it (thelenread, and theslices.Backwarditeration over the slice duringRender). The race detector reports:mount.go:130ingenerateAppListKeys(theappend)ctx.go:834/ duringRenderinres.goFix
Introduce a dedicated
sync.Once(appListKeysOnce) inmountFieldsand route both callers ofgenerateAppListKeysthrough it:renderExtensions(ctx.go)mountStartupProcess(mount.go), kept inside the existingsubAppsProcessed.Doblock, afterappendSubAppListsThe keys are now generated exactly once with proper happens-before ordering; concurrent renders block until generation finishes and then read a slice that is never mutated again. Sharing a single
Oncebetween both callers also removes a latent double-append that the old code allowed if the lazy path ever ran beforemountStartupProcess.This preserves existing behavior:
appListKeyswas already generated only once in practice (the oldlen == 0guard never re-fired, the slice is never reset, andappListdoes not grow after startup). Route rebuilds viaRebuildTree()are unaffected, since they only rebuild the route prefix trees and never touchappListKeys.Verification
go test -race -run 'Test_Ctx_RenderWithLocals$' -count=200 .passes clean. The original code trips the detector on the first iteration.-race.go vet ./...is clean.🤖 Generated with Claude Code