fix(treatment-service): remove nested RLock deadlock and add optional LocalStorage call metrics - #93
Open
anantadwi13 wants to merge 3 commits into
Open
fix(treatment-service): remove nested RLock deadlock and add optional LocalStorage call metrics#93anantadwi13 wants to merge 3 commits into
anantadwi13 wants to merge 3 commits into
Conversation
…ngs lookup findSubscribedProjectSettingsById held s.RLock() and, while still holding it, called findProjectSettingsById which took s.RLock() again on the same goroutine. Go's sync.RWMutex blocks new readers once a writer is queued (to avoid writer starvation), so if a writer (e.g. PollerService.Refresh -> Init, or a pubsub update handler) queued between the outer and nested RLock, the nested call blocked forever waiting for the writer, and the writer blocked forever waiting for the outer RLock to release -- a circular-wait deadlock. Extract the unlocked lookup into findProjectSettingsByIdLocked so callers that already hold the lock no longer re-lock. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rage Adds a call counter and call-duration histogram around LocalStorage's public methods, gated behind a new Monitoring.LocalStorageMetricsEnabled config flag. A stuck method (e.g. a lock deadlock) shows up as its call counter climbing while its duration histogram's count stays frozen. Routes through the existing MetricService (LogRequestCount / LogLatencyHistogram) rather than a separate metrics backend, so this respects the existing Monitoring.Kind sink and reuses the same Prometheus registration path as every other treatment-service metric. Since services already imports models (for *LocalStorage), models can't import services back, so LocalStorage depends only on a small LocalStorageMetricsRecorder interface (satisfied by MetricService as-is); appcontext wires the two together after both are constructed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The simulated writer goroutine's Lock()/Unlock() pair is deliberately empty -- acquiring and releasing the lock is the observable event under test, not protection of shared state -- but staticcheck flags it as an empty critical section, failing CI lint. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
What this PR does / why we need it:
Fixes a deadlock in
LocalStorage.findSubscribedProjectSettingsById: it tooks.RLock()and then, while still holding it, called
findProjectSettingsById, which tooks.RLock()again on the same goroutine. Go's
sync.RWMutexblocks new readers once a writer is queued(to prevent writer starvation), so if a writer (
PollerService.Refresh->Init(), or apubsub
InsertExperiment/UpdateProjectSettingshandler) queued between the outer andnested
RLock, the nested call would block forever waiting for the writer, and the writerwould block forever waiting for the outer
RLockto release — a genuine circular-waitdeadlock, suspected to be the root cause of a reported
GetTreatmentForRequesthang.Fix: split the lock-acquiring
findProjectSettingsByIdfrom a new lock-freefindProjectSettingsByIdLocked, and have the already-locked caller invoke the latterdirectly instead of re-acquiring the lock.
Adds feature-flagged instrumentation on
LocalStorage's public methods (a call counterand call-duration histogram, labeled by method name), so a stuck method — e.g. a lock
deadlock like the one above — shows up as its call count stalling and duration climbing,
instead of silently hanging with no observability.
Monitoring.LocalStorageMetricsEnabled(defaultfalse).SetMetricsRecorderseam (LocalStorageMetricsRecorderinterface) set fromappcontext.NewAppContext, avoiding an import cycle betweenmodelsandservices.local_storage_calls_total,local_storage_call_duration_ms.plugins/turing/runner/experiment_runner.go.Tests:
storage_deadlock_test.godeterministically reproduces the nested-RLock deadlock inisolation and stress-tests the real call path to confirm it no longer hangs;
storage_metrics_test.go/metric_service_test.gocover the metrics recorder contract onboth read and write paths.