More agressive updates fore SAIC

[kscholty]

I partly reverted #30603

Going back to the 1-to-1 connection between background requests and stored values.
A stored value will only be returned once. Any "retry" error from the servers is now again sent upstream to make sure, the request is repeated so that background values are retrieved in a timely manner.

In order to make sure that invalid values always trigger a new request to the SAICV API, the cache is now reset in cases where an invalid return value is observed.

The SAIC servers sometimes return a valid answer but some of the values in there (SOC and range) are invalid. This will now lead to a reset of the cache, while in the old implementation the value was kept in the cache and returend until the cache timed out (15 minutes if not overwritten in the config). The new implementation forces a new query of the SAIC servers.

Locking has been replaced by atomic operations which is more efficient for the single-value checks used in the implementation.

[sourcery-ai]

Hey - I've found 2 issues

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="vehicle/saic/api.go" line_range="28-30" />
<code_context>
 	RegionAU = "https://gateway-mg-au.soimt.com/api.app/v1/"
 )

+type ConcurrentRequest struct {
+	Status atomic.Int32
+	Result requests.ChargeStatus
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Concurrent access to Result is racy despite using atomic for Status

Status is atomic, but Result is a plain field written in doRepeatedRequest/repeatRequest and read in Status from multiple goroutines. Since Status and Result are not updated/read as a single atomic operation, this introduces a data race and readers can observe inconsistent Result. Please either protect both fields with a mutex, store Result in an atomic.Value, or use an atomically swapped pointer to an immutable Result snapshot so readers see a race-free, consistent view.
</issue_to_address>

### Comment 2
<location path="vehicle/saic/api.go" line_range="81-82" />
<code_context>
-		v.store(res.Data)
+	_, err = doRequest(v, req, &res)
+	if err == nil {
+		v.request = ConcurrentRequest{
+			Result: res.Data,
+		}
+		v.request.Status.Store(StatValid)
</code_context>
<issue_to_address>
**issue (bug_risk):** Overwriting v.request copies atomic.Int32, which is unsafe after first use

Assigning a new ConcurrentRequest to v.request copies the embedded atomic.Int32, which sync/atomic explicitly forbids after first use because the copy has independent, unsynchronized state. Instead of reassigning v.request, mutate the existing struct in place, e.g.:

    v.request.Result = res.Data
    v.request.Status.Store(StatValid)

This updates the cache without copying the atomic field.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

issue (bug_risk): Concurrent access to Result is racy despite using atomic for Status

Status is atomic, but Result is a plain field written in doRepeatedRequest/repeatRequest and read in Status from multiple goroutines. Since Status and Result are not updated/read as a single atomic operation, this introduces a data race and readers can observe inconsistent Result. Please either protect both fields with a mutex, store Result in an atomic.Value, or use an atomically swapped pointer to an immutable Result snapshot so readers see a race-free, consistent view.

[andig]

With the locking change its hard toi see what this PR really does in the diff. Since performance is not a concern here, can we keep the mutex pattern?