Optimizer: reduce interval to 15min#29137
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Access to the package-level
optimizerUpdatedtimestamp is now happening from multiple goroutines (e.g. inupdateMeters,optimizerUpdateAsync, and viaOptimize) without synchronization, which will cause a data race; consider guarding it with a mutex or using anatomic.Value/atomic.Int64to store the time. - The new
OptimizeAPI endpoint triggersoptimizerUpdateAsyncwithout checkingoptimizerEnabledor the tariff slot interval, which means it can bypass the gating logic inupdateMeters; if this is not intentional for debug only, align its preconditions with the existing optimizer trigger.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Access to the package-level `optimizerUpdated` timestamp is now happening from multiple goroutines (e.g. in `updateMeters`, `optimizerUpdateAsync`, and via `Optimize`) without synchronization, which will cause a data race; consider guarding it with a mutex or using an `atomic.Value`/`atomic.Int64` to store the time.
- The new `Optimize` API endpoint triggers `optimizerUpdateAsync` without checking `optimizerEnabled` or the tariff slot interval, which means it can bypass the gating logic in `updateMeters`; if this is not intentional for debug only, align its preconditions with the existing optimizer trigger.
## Individual Comments
### Comment 1
<location path="core/site_optimizer.go" line_range="33" />
<code_context>
- updated time.Time
- mu atomic.Uint32
+ optimizerUpdated time.Time
+ mu atomic.Uint32
)
</code_context>
<issue_to_address>
**issue (bug_risk):** Access to `optimizerUpdated` is not synchronized, which can cause a data race
`optimizerUpdated` is written in `optimizerUpdateAsync` (in the defer) and read from `optimizerUpdateAsync`, `updateMeters`, and via `Optimize` across multiple goroutines. Because `time.Time` is not safe for concurrent read/write, this introduces a data race. Please store the timestamp in an atomic type (e.g., `atomic.Int64` with `UnixNano`) or guard it with a mutex so all accesses are synchronized.
</issue_to_address>
### Comment 2
<location path="core/site_api.go" line_range="39-40" />
<code_context>
}
+// Optimize updates the optimizer
+func (site *Site) Optimize() error {
+ go site.optimizerUpdateAsync()
+ return nil
+}
</code_context>
<issue_to_address>
**🚨 issue (security):** Public `Optimize` bypasses existing authorization/feature checks applied in `updateMeters`
`updateMeters` enforces `sponsor.IsAuthorized()` and `optimizerEnabled()` before calling `optimizerUpdateAsync`, but `Optimize` calls it directly and is exposed via HTTP. This allows optimization to run even when the optimizer is disabled or the caller wouldn’t normally be authorized. If that behavior isn’t explicitly desired, please apply the same checks here (or centralize them inside `optimizerUpdateAsync`/a shared helper) so all entry points are consistently guarded.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
naltatis
reviewed
Apr 15, 2026
| defer mu.Unlock() | ||
|
|
||
| if !mu.CompareAndSwap(0, 1) { | ||
| if time.Since(optimizerUpdated) < 2*time.Minute { |
Member
Author
There was a problem hiding this comment.
That's https://github.com/evcc-io/evcc/pull/29137/changes#diff-7aa5baae0bd3243f85bfc6db64612f7450eb0170fa310ca2c93dc3968fc02e02R795- need this here for on demand update
This comment was marked as low quality.
This comment was marked as low quality.
This comment was marked as low quality.
This comment was marked as low quality.
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
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.
Add "Optimize now" button for manual updates. Becomes available again once ui has received new optimizer data.
optimize.now.webm
TODO