Skip to content

Commit 9bfd44f

Browse files
robster7674Rob
andauthored
fix(battery): address Greptile race + backoff + burst-gating findings (#57)
* fix(battery): address greptile race + backoff-consistency + burst-gating Follow-up to #56. Three Greptile findings: 1. P1 — mScanning = true was assigned outside the synchronized block in MeterScanner.scanRunnable, opening a window where a concurrent stopScan(false) (e.g. processScanResult on line 126) sees mScanning == false, skips scanStopper(), and returns — leaving the BLE scan active for the full 6.5 min scantimeout. Move mScanning=true into the same synchronized block that clears scanpending and resets currentwait so the state transition is atomic. 2. P2 — currentwait is written under synchronized(MeterScanner.this) but read in stopScan() without the lock (JMM data race). Declare currentwait and scanpending volatile so the read in stopScan is guaranteed to see the latest value without expanding the lock scope. 3. P2 — onScanFailed (MeterScanner line 202) hard-coded scanStarter(scaninterval) so the new exponential backoff in currentwait was bypassed on BLE-stack-reported failures (SCAN_FAILED_APPLICATION_REGISTRATION_FAILED / SCAN_FAILED_INTERNAL_ERROR). Use currentwait so both failure paths stay consistent and a stream of scan failures backs off up to scanstartmaxwait (5 min) instead of always retrying in 60 s. 4. P2 (testlab.yml, discussion_r3489714891) — the inter-start burst warning was nested inside 'if len(ble_starts) > len(ble_stops) + 1:'. A balanced but rapid cycle satisfied starts == stops and got the 'balanced' line, hiding what could be a high-frequency startScan burst. Hoist the delta computation out of that branch and lower the _short threshold from >= 3 to >= 2 so the warning fires for balanced rapid cycles too. * fix(battery): make mScanning volatile (greptile follow-up on #57) Greptile second-pass on #57 flagged that the volatile fix applied to currentwait and scanpending was incomplete: mScanning has the same shape — written inside synchronized(MeterScanner.this) in scanRunnable, read outside any lock in stopScan() when called from the timeout runnable. The JMM doesn't guarantee the timeout thread sees mScanning = true, so stopScan(true) could skip scanStopper() and reschedule a redundant scan. Make mScanning volatile so the read in stopScan sees the latest value without expanding the lock scope. --------- Co-authored-by: Rob <rob@performanceinsights.ai>
1 parent 7ac101d commit 9bfd44f

2 files changed

Lines changed: 26 additions & 23 deletions

File tree

.github/workflows/testlab.yml

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -616,24 +616,27 @@ jobs:
616616
lines.append(f"- Registered: **{len(ble_starts)}** stopped: **{len(ble_stops)}**")
617617
if len(ble_starts) > len(ble_stops) + 1:
618618
lines.append(f"- ⚠️ {len(ble_starts)-len(ble_stops)} unmatched start(s)")
619-
# Surface the worst scan-restart bursts so they're actionable.
620-
from datetime import datetime
621-
_ts_re = re.compile(r'^(\d{2}-\d{2} \d{2}:\d{2}:\d{2}\.\d+)')
622-
def _ts(s):
623-
m = _ts_re.match(s)
624-
if not m: return None
625-
try: return datetime.strptime(m.group(1), '%m-%d %H:%M:%S.%f')
626-
except ValueError: return None
627-
_starts = [(t, l) for l in ble_starts if (t := _ts(l)) is not None]
628-
if len(_starts) >= 3:
629-
_starts.sort()
630-
_gaps = [(_starts[i+1][0]-_starts[i][0]).total_seconds() for i in range(len(_starts)-1)]
631-
_short = [g for g in _gaps if g < 60]
632-
if len(_short) >= 3:
633-
lines.append(f"- ⚠️ {len(_short)}/{len(_gaps)} scan restarts fired <60s apart — likely redundant startScan calls")
634-
lines.append("<details><summary>Start timestamps</summary>\n\n```")
635-
lines.extend(f"{t.strftime('%H:%M:%S.%f')[:-3]}" for t, _ in _starts[:30])
636-
lines.append("```\n</details>")
619+
620+
# Surface the worst scan-restart bursts so they're actionable. Computed
621+
# independently of the matched/unmatched check so a balanced but rapid
622+
# cycle (e.g. many short start/stop pairs) is not silently hidden.
623+
from datetime import datetime
624+
_ts_re = re.compile(r'^(\d{2}-\d{2} \d{2}:\d{2}:\d{2}\.\d+)')
625+
def _ts(s):
626+
m = _ts_re.match(s)
627+
if not m: return None
628+
try: return datetime.strptime(m.group(1), '%m-%d %H:%M:%S.%f')
629+
except ValueError: return None
630+
_starts = [(t, l) for l in ble_starts if (t := _ts(l)) is not None]
631+
if len(_starts) >= 3:
632+
_starts.sort()
633+
_gaps = [(_starts[i+1][0]-_starts[i][0]).total_seconds() for i in range(len(_starts)-1)]
634+
_short = [g for g in _gaps if g < 60]
635+
if len(_short) >= 2:
636+
lines.append(f"- ⚠️ {len(_short)}/{len(_gaps)} scan restarts fired <60s apart — likely redundant startScan calls")
637+
lines.append("<details><summary>Start timestamps</summary>\n\n```")
638+
lines.extend(f"{t.strftime('%H:%M:%S.%f')[:-3]}" for t, _ in _starts[:30])
639+
lines.append("```\n</details>")
637640
638641
# ── App AlarmManager calls ─────────────────────────────────────────
639642
if power_only:

Common/src/mobile/java/tk/glucodata/MeterScanner.java

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ class MeterScanner {
5252
List<BluetoothDevice> devices=new ArrayList<BluetoothDevice>();
5353
List<String> deviceNames=new ArrayList<String>();
5454
private static final String LOG_ID="MeterScanner";
55-
private boolean mScanning=false;
55+
private volatile boolean mScanning=false;
5656
private BluetoothLeScanner mBluetoothLeScanner=null;
5757
boolean knowName=true;
5858
MeterScanner() {
@@ -199,7 +199,7 @@ public void onScanFailed(int errorCode) {
199199
if(errorCode != SCAN_FAILED_ALREADY_STARTED) {
200200
stopScan(false);
201201
if(errorCode != SCAN_FAILED_FEATURE_UNSUPPORTED) {
202-
scanStarter(scaninterval) ;
202+
scanStarter(currentwait) ;
203203
}
204204
}
205205
}
@@ -262,8 +262,8 @@ public void scanStopper() {
262262

263263
private static final int scaninterval=60000;
264264
private static final int scanstartmaxwait=300000;
265-
private int currentwait=scaninterval;
266-
private boolean scanpending=false;
265+
private volatile int currentwait=scaninterval;
266+
private volatile boolean scanpending=false;
267267
public void stopScan(boolean retry) {
268268
if(doLog) {Log.d(LOG_ID,"Stop scanning "+(retry?"retry":"don't retry"));};
269269
if(scanFuture!=null) {
@@ -303,8 +303,8 @@ public void run() {
303303
scanpending=true;
304304
}
305305
if(scanStarter()) {
306-
mScanning = true;
307306
synchronized(MeterScanner.this) {
307+
mScanning=true;
308308
scanpending=false;
309309
currentwait=scaninterval;
310310
}

0 commit comments

Comments
 (0)