fix: add top-level recover guards to callback hook chain#199
Open
bruceding wants to merge 2 commits into
Open
Conversation
Two production-impacting goroutine paths in the callback hook chain had no
recover, so any downstream panic (nil deref, type assertion, broken user-
registered CallBackProcessFunc) would crash the whole recommendation server:
1. Hook dispatch in service/user_recommend.go:173 and
service/user_recall_service.go:43:
go hf(context, user, items)
2. Worker pool in web/callback_controller_handler.go:
go func() { for c := range ch { c.doCallbackLog() } }()
Changes:
- service/hook/recommend.go: add SafeRun helper that wraps a
RecommendCleanHookFunc with deferred recover + stack-trace log,
preserving the existing dispatch semantics.
- service/user_recommend.go, service/user_recall_service.go: dispatch
hooks via hook.SafeRun.
- web/callback_controller_handler.go: route worker invocations through
runCallbackSafely, which provides the same guard for doCallbackLog.
Tests:
- service/hook/recommend_test.go: cover panic recovery, nil context,
param pass-through, normal return, and goroutine isolation.
- web/callback_controller_handler_test.go: add a deterministic
injected-panic test using RegisterCallBackProcessFunc, plus a nil
controller defensive check.
doc/code_reviews/2026-06-01-callback-hook-recover.md records the review
that motivated the test refactor.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Production worker goroutines read callBackProcessFuncMap on every callback request (web/callback_controller.go:159) while user code may still register new scenes at startup, so the map needs synchronization to be race-free. Without it, `go test -race` would fail once any test (or production caller) exercised both sides concurrently. Changes: - Add sync.RWMutex protecting callBackProcessFuncMap; expose lookupCallBackProcessFunc (RLock) and unregisterCallBackProcessFunc (Lock, test-only) helpers. Keep RegisterCallBackProcessFunc as the public entry, now locked. - Switch the dispatch site in callback_controller.go to use the helper. - Update TestRunCallbackSafely_RecoversFromInjectedPanic cleanup to call unregisterCallBackProcessFunc so tests no longer poke the global directly. - Add TestCallBackProcessFuncMap_ConcurrentAccess: 32 goroutines x 200 register/lookup/unregister ops to surface any future regression under `go test -race`. Also drop doc/code_reviews/2026-06-01-callback-hook-recover.md (review C5); self-review notes belong on the PR description, not on master.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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
Two production-impacting goroutine paths in the callback hook chain had no top-level
recover, so any downstream panic — nil deref, type assertion, broken user-registeredCallBackProcessFunc, etc. — would crash the entire recommendation server.service/user_recommend.go:173andservice/user_recall_service.go:43dispatch hooks via rawgo hf(...).web/callback_controller_handler.goworker pool runscontroller.doCallbackLog()without a guard. A panic both terminates the process and permanently shrinks the worker pool.This PR localizes failure to the hook/worker level:
hook.SafeRun(hf, ctx, params...)helper wraps aRecommendCleanHookFuncwithdefer recover+ stack-trace log. Both hook-dispatch sites now use it.runCallbackSafely(c)in the web worker pool provides the same guard fordoCallbackLog.requestId=...\tmodule=...\terror=...\tstack=..., seeservice/recall.go:135).Test plan
go test ./service/hook/...— 5 new unit tests covering panic recovery, nil context, params pass-through, normal return, goroutine isolationgo test ./web/...— added a deterministic injected-panic test viaRegisterCallBackProcessFunc, plus a defensive nil-controller testgo vet ./service/... ./web/...— cleango build ./...— cleanservice/recall.TestMultibizRecallconfirmed to fail onorigin/masterbefore this change (depends on external BE data source) — unrelated to this PRCode review
A pre-merge code review was performed. The review concluded "可以合并但建议改进" with two
Importantitems, both of which were addressed in the same commit:var _ = context.NewRecommendContextplaceholder import in the test file.TestRunCallbackSafely_RecoversFromPanicto inject a panic viaRegisterCallBackProcessFuncso the panic path is deterministic, instead of relying on a chance nil-deref insidedoCallbackLog.Two
Suggestion-level items are recorded as follow-ups indoc/code_reviews/2026-06-01-callback-hook-recover.md:feature_log.FeatureLog,LogSampleResult) that deserve the same protection in a separate PR.RecommendCleanHookFuncstill usesparams ...interface{}; could be unified toanyproject-wide later.🤖 Generated with Claude Code