Ownership tokens for every BLE handle - #1
Open
lockevod wants to merge 17 commits into
Open
Conversation
df5d112 committed everything that USES ErgBias — the import in BridgeService, the ergBiasW argument to PowerCorrection, the correctCommanded path — but the class itself and its tests were still untracked, so that commit does not compile. No behaviour change on top of df5d112; this is the missing half of it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R4wR6MG97CqNiC9shgQSvf
A four-reviewer audit (three Opus passes on separate axes, one adversarial Codex pass) found 19 issues; ten are fixed here. Two further Codex rounds then reviewed the fixes themselves and broke twelve and three of them respectively, which is the same lesson as 000b6f3 — the fixes need the adversary more than the original code does. Proxy: - The Zycle level anchor survived a trainer dropout, so the first frame back was differenced against a level from before it and the whole gap reached the app as one rider button press. Re-anchor instead, without moving what the app sees. This is the "sometimes the proxy misbehaves" symptom. - Four paths left BLE advertising dead for the rest of the ride: an exhausted retry budget, a start watchdog that cleared its latch without retrying or reporting, a synchronous throw, and openGattServer returning null. All four now enter one backoff loop with no attempt cap; a held profile is replayed when the server finally opens. - nudgeResistance wrote 0x04 straight past the mirror, so ErgBias never saw the op that ends ERG and kept measuring against a dead command — saturating the bias and PERSISTING it, which started the next ride 30 W out. - The servo-step budget was armed by writes that never reached the trainer, and survived a dropout to eat the rider's next press. - elapsedRealtime for every timeout and freshness decision; wall clock only for log timestamps. CorrectedFeed already documented why. Energy (all in the idle state, which is the default one): - The wake lock followed the master switch, not the trainer link, blocking suspend for every hour the app sat armed with no trainer. It now follows the link, lingering past the reconnect delay so we never suspend with no scan up. - The cold scan ran at BALANCED (25% radio duty) indefinitely. A search before any connection is not the mid-ride reacquisition the comment defends. - The ERG bias was written to prefs on every whole-watt flip. Once a minute. Performance: - The Monitor re-rendered at the trainer's packet rate on the same looper the notify fan-out posts to; the 1 Hz poller already covered it. - Every reconnect blacked out data for 0.6-2 s because the whole read burst was queued ahead of the subscribes. The data characteristics go first; the cold-cache invariant gates on the queue draining, not on the first subscribe. From the review rounds on the fixes: a generation guard so a replaced source's callbacks cannot drive the live one (validated on main, or the check is check-then-act), a lock making the server/profile handover atomic, stopped guards so a late callback cannot re-advertise a closed server, and try/finally around the burst so nothing can hold the GATT queue shut. Diagnostics, because none of the above is observable otherwise: a session header with the build and the correction values, the level decision (raw, shown, and whether it was scored servo or rider), wake lock transitions, scan start with its mode, and a state line a minute so a quiet log is informative rather than ambiguous. Log cap raised to 48 MB: at 16 MB rotation was throwing away the start of the ride, which is the part you turned logging on to see. Not verified on hardware. No test covers any file touched here.
… they broke Replaces the volatile/generation guards with identity tokens, so validating a callback and mutating on its behalf can no longer be split by a source, session or attempt switch landing in between. ZycleClient: a timed-out GATT op retires the handle rather than advancing the queue behind an operation Android still holds. Unsticking in place cannot cancel that op, so its late callback completes the wrong one, cancels the wrong watchdog and pumps a third while the second is still on the controller; deferring the retirement to an Nth consecutive timeout fails for the same reason, since that callback also resets the counter. connect() now owns its attempt with a token that stop() clears, so a connectGatt() returning after stop() closes its own handle instead of installing an orphan that keeps the trainer for the whole ride. MirrorServer: one AdvertiseCallback per attempt, plus a process-wide registry of callbacks retired while their start was still unresolved. The callback IS the controller registration and a stop issued mid-start is dropped by the stack, so those are swept with backoff until their own delivery proves them gone. One whose result is already known is stopped once and released — adopting it would strand it in the registry for the life of the process. BridgeService: one token for the receive source, one for the emit instance. The emit token closes a control write that entered the old mirror and reached the replacement trainer. The write dispatches outside the monitor: writeCharacteristic is a Binder call that stopEmit() would otherwise wait on from the main thread. ErgBias persistence: the learner runs on every power sample and the deadline is evaluated independently, so a bias converging inside the interval is no longer held back until shutdown. Four review rounds, two of them external and adversarial. Three fixes were reverted after review falsified the premise each rested on, and the reasoning is kept as comments where they sat so they are not retried. Two limits stand: the advertising sweep cannot guarantee retirement, because stopAdvertising() never confirms removal and a start can stay in flight indefinitely; and none of this is device-validated. The riskiest sequence to try on the Karoo is a start accepted without a callback, its watchdog, a successful retry, then the first callback arriving very late or never, followed by stop/restart and Bluetooth off/on.
There was a problem hiding this comment.
🔵 Needs a closer look
It changes multiple high-risk BLE lifecycle/concurrency paths (GATT, advertising, wake locks) that require device-level validation to confirm correctness under real Android interleavings.
Pull request overview
This PR hardens BLE runtime coordination by replacing volatile/generation guards with identity tokens (ownership) so late/stale callbacks cannot mutate state belonging to a newer session/attempt, and by improving operational robustness (timeouts, retries, logging, and clock correctness) around trainer receive/emit paths.
Changes:
- Add shared ownership/coordinator primitives (identity ownership, GATT session retirement on timeout, advertising attempt coordination, ERG bias persistence throttling) and JVM contract tests for them.
- Update BLE client/server coordination to retire stale handles/advertising attempts safely and to reduce risk from late callbacks and lost stack callbacks.
- Switch staleness/heartbeat timing to
elapsedRealtime(), adjust wake-lock behavior to follow the trainer link, and rate-limit/persist ERG bias updates.
File summaries
| File | Description |
|---|---|
| app/src/test/java/com/enderthor/trainerbridgeble/RuntimeHardeningTest.kt | Adds JVM-level contract tests for identity ownership and coordinator behavior. |
| app/src/test/java/com/enderthor/trainerbridgeble/correction/ErgBiasTest.kt | Adds unit tests covering ERG bias learning, clamping, and regressions. |
| app/src/main/java/com/enderthor/trainerbridgeble/RuntimeHardening.kt | Introduces identity-ownership and coordinator primitives plus ERG bias persistence throttling. |
| app/src/main/java/com/enderthor/trainerbridgeble/MonitorActivity.kt | Uses elapsedRealtime() for UI staleness calculations to tolerate clock changes. |
| app/src/main/java/com/enderthor/trainerbridgeble/FileLog.kt | Increases log retention to keep longer ride sequences available for diagnosis. |
| app/src/main/java/com/enderthor/trainerbridgeble/correction/ErgBias.kt | Implements ERG overshoot learning logic independent of Android clocking. |
| app/src/main/java/com/enderthor/trainerbridgeble/BridgeService.kt | Adds receive/emit ownership tokens, moves staleness timing to elapsedRealtime(), improves wake-lock/snapshot/logging, and rate-limits ERG bias persistence. |
| app/src/main/java/com/enderthor/trainerbridgeble/ble/ZycleClient.kt | Reworks connection/operation lifecycle with ownership tokens and GATT-session retirement on timeouts; improves scan mode policy and heartbeat timing. |
| app/src/main/java/com/enderthor/trainerbridgeble/ble/MirrorServer.kt | Adds per-attempt advertising callbacks with orphan sweeping/backoff and retries GATT server open with backoff; hardens against late callbacks after stop. |
| app/src/main/java/com/enderthor/trainerbridgeble/ant/RawAntLink.kt | Switches heartbeat timing to elapsedRealtime() to avoid wall-clock jump issues. |
| .gitignore | Ignores local notes directory. |
Review details
- Files reviewed: 10/11 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Config(this).let { c -> | ||
| // via PackageManager rather than BuildConfig: AGP 8 does not generate that class unless | ||
| // buildFeatures.buildConfig is turned on, and one log line does not justify a build change. | ||
| val ver = runCatching { packageManager.getPackageInfo(packageName, 0).versionName }.getOrNull() |
Owner
Author
There was a problem hiding this comment.
Arreglado en d364b94: ?: "?". La cabecera de sesión es diagnóstico de tirada, y vnull era justo el caso en que más falta hace saber la versión.
Comment on lines
+77
to
+80
| assertNull(emitOwner.clear().let { null }) | ||
|
|
||
| emitOwner.clear() // stopEmit() | ||
| emitOwner.replace(Any()) // startEmit() with a replacement source |
Owner
Author
There was a problem hiding this comment.
Correcto, y era peor que un no-op: el comentario afirmaba que comprobaba que clear() no espera al dispatch. No comprobaba nada. Arreglado en d364b94 con la propiedad real — assertEquals(oldToken, emitOwner.clear()) — y fuera el clear() redundante. Que el lambda de producción despache fuera del monitor es una forma de llamada que ningún test JVM de este suite alcanza; eso ya está declarado como hueco 3 en la KDoc de la clase, que es su sitio honesto.
…ader
The emit-ownership test asserted `emitOwner.clear().let { null }` is null, which
is true by construction and can never fail — with a comment claiming it checked
that clear() does not wait on the dispatch. It checked nothing. Replaced with the
real property: clear() hands back exactly the token it retired. The claim about
the production call shape already lives in the class KDoc's gap list, which is
the honest place for something no JVM test here can reach.
versionName is nullable and getPackageInfo can throw, so an unresolved version
was logging as "vnull" in the session header.
… what each fix broke The gate that admits FTMS procedures had no liveness property. An unanswered or opcode-mismatched Control Point response left `pending` set forever, after which every request — Request Control included — was refused for the rest of the ride. Power kept streaming, so nothing looked broken; ERG just stopped obeying. FtmsControlCoordinator now owns the three things every decision needs together: the per-connection identities, the admitted procedure, and its command bytes. They were in three places under two locks, which is where the bugs lived: - Identity lookup and admission were separate steps, so a disconnect landing between them could promote a client that had already gone — and lock out its own reconnect with CONTROL_NOT_PERMITTED for the session. - The command payload sat in a side slot, so one procedure's response could commit another's target, and the local button carried no payload at all — meaning ErgBias never saw opcode 0x04 and never retired an armed ERG target when the rider took manual control. - A rebuild of the local GATT server cleared six pieces of state but not the one gating ERG. Closing a server raises no disconnect callbacks, so ownership survived onto a generation that could never come back. Transport is no longer treated as acceptance. An ATT write the trainer ACKs can still be refused by the procedure, so UI, servo filtering and ERG bias learning now commit only on an FTMS Response Code of SUCCESS, with the bytes that procedure actually carried. Learning from targets the machine rejected is how a bias gets persisted against a trainer that was never holding it. Each procedure has a 30 s deadline, armed after the trainer takes the write — not at admission, which also measured our own serialised op queue. Expiry quarantines and recycles the link, because an opcode-only response arriving late can no longer be matched to a request. Arming and cancelling are both identity checked: the local path crosses two main-loop hops, so a stale arm would otherwise strip a live procedure of its only timer. Also here: addService publishes its claim before the binder call rather than after, where an early callback was discarded as stale and cost an 8 s watchdog plus a full rebuild; a rebuild stops advertising instead of broadcasting over a closed server; a refused or asynchronously failed terminal indication releases the owner so the next requester can arbitrate; bootstrap honours setCharacteristicNotification and rejects an FTMS Feature too short to carry Target Setting Features. The log can now prove all of it: admitted/rejected/response/timeout with opcode, result and procedure id; every relayed write with its sequence and terminal outcome, so an ordering violation is visible rather than inferred; and wakelock state in the minute snapshot. Diagnostic logging is still off by default and is only read at foreground start. 78 unit tests. Three adversarial review rounds got us here, and each of the first two shipped fixes that introduced a fresh variant of the same lockout — so the tests that matter are mutation checked against the exact line they name.
Held from master-on to master-off, the partial lock blocked CPU suspend for as long as the switch was left on with no trainer in the room. onTimeout would never save us — that is the Android 14 foreground-service timeout and the Karoo is API 32 — so the only release was the user remembering. So it follows the trainer link again, as it did before, but with 45 minutes of slack: a mechanical stop, a phone call or a bathroom break must not cost the lock mid-session, and a trainer drop mid-ride still recovers with it held. Release is gated on a scan actually being registered with the controller. That is the invariant the whole-session hold dropped: postDelayed does not wake a suspended CPU, so releasing with no scan up leaves the trainer's advertising with nothing to arrive at. Once a scan is up the controller wakes us on a match, and the lock is retaken on STATE_CONNECTED — before discoverServices, so the whole cold bootstrap runs awake. A trainer seen but not connectable — the status=133 flap — now refreshes the timer too. It was counting as an absent trainer and dropping the lock exactly when the retry chain most needed the CPU running on schedule. 45 and not 30 because Android 12 silently downgrades a long-running scan to opportunistic at 30 minutes, without onScanFailed, so our own scanning flag keeps saying true. Colliding with that boundary would make a screen-off test unable to tell the two apart. The downgrade itself is untouched and still there: nothing restarts a scan that is already running. The lock's log lines now say why they fired and report what actually happened, instead of claiming a release before attempting it and calling the master inactive while it was on.
Bestcycling asks for control before it subscribes to the Control Point — in fact it never subscribes at all, it drives blind and never reads a response. The undeliverable-result handling treated that as "this client will never hear us, drop its claim", so control was granted and revoked in the same millisecond and every command it sent afterwards came back Control Not Permitted. ERG was dead from the first second of a session. A client that has not subscribed is not gone. It is connected and can still drive the trainer; it just cannot hear the answer. Releasing its claim turned a lost indication into no control at all. So the subscription cases only log now, and the claim stands. Releasing is kept for a client that is genuinely gone — stale generation, device absent, server closed — or an indication the stack actually refused. Found on the trainer bridge with a real FTMS client after six review passes had gone over this code. Every one of them asked whether ownership could get stuck; none asked whether it could be dropped when it shouldn't be.
The Karoo shuts itself down ten minutes after the screen sleeps when no ride is recording. Not Android's doze — Hammerhead's own state manager arms it the moment the screen goes off, and it happens at any battery level; it took the device down at 99% with the log cut mid-line. A foreground service does not stop it, nor a partial wake lock, nor the battery whitelist: all three were held at the moment it powered off. The delay lives in a system property that is not writable without root. Recording a ride prevents it, which is no answer for someone who does not want to record. The SDK turns out to expose TurnScreenOn, and logcat confirms it lands in the same state manager, on the same channel that armed the shutdown. So: dispatch it every five minutes while a session is live, and the countdown never completes. Five and not eight because a single missed tick would otherwise eat the whole margin against ten; a dispatch the host does not take is retried in twenty seconds rather than at the next slot. It stops on the same leash as the wake lock. The idle guard releases that lock after forty-five minutes with no trainer so an abandoned session can sleep, and keeping the whole device awake past that point would defeat it with a more expensive resource — a screen instead of a CPU. It also skips a recorded ride, where the shutdown cannot happen anyway and the only effect would be relighting a display the rider let sleep, and skips whenever the screen is already on, where there is no countdown to disarm. Verified on battery with the cable out: the screen slept at 54 seconds, so the shutdown was due at 10:54. It ran past thirteen minutes — same pid, no restart, no shutdown line, eight pokes all accepted, and every state line exactly sixty seconds apart. The screen was seen waking on its own twice. Alongside it: an optional flag to hold the monitor screen on, which only applies while that screen is open and now says so; a shutdown receiver, so the next time this happens the log explains its own gap instead of stopping mid-sentence; and the mirror now says when a trainer value had nobody to go to, which until now was indistinguishable from never having arrived.
…tdown The skip trusts isInteractive to mean "no countdown is armed". It is only a proxy, and AOSP counts a dreaming screensaver state as interactive — so a firmware that armed the countdown in a state reading interactive would skip every poke forever. That is the one error the five-against-ten-minute margin cannot absorb, because it persists instead of passing. One skip is now the limit. Being wrong in this direction is free: a poke while the screen really is on does nothing. Also: the comment above applyKeepAwake still promised an immediate poke it no longer makes, which is the same defect this feature's own log lines were just fixed for, one layer up. The README now says what the Karoo does, where a rider meets it: it powers itself off ten minutes after the screen sleeps with no ride recording, and that looks exactly like the bridge crashing. Both new switches are documented with the limit that matters — the wake costs battery, and holding the screen on stops applying the moment you leave that screen.
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.
Sustituye los guards volátiles / de generación por tokens de identidad, de modo que validar un callback y mutar en su nombre ya no puedan separarse por un cambio de fuente, sesión o intento que caiga en medio.
Qué cambia
ZycleClient— una operación GATT expirada retira el handle en lugar de avanzar la cola por detrás de una operación que Android todavía tiene. Desatascar en sitio no puede cancelarla, así que su callback tardío completa la operación equivocada, cancela el watchdog equivocado y bombea una tercera con la segunda aún en el controlador.connect()posee ahora su intento mediante un token questop()limpia: unconnectGatt()que retorna después destop()cierra su propio handle en vez de instalar un huérfano que se queda el rodillo el resto de la salida.MirrorServer— unAdvertiseCallbackpor intento, más un registro de proceso con los callbacks retirados mientras su start seguía sin resolver. El callback es el registro del controlador y un stop emitido con el start en vuelo lo descarta el stack, así que esos se barren con backoff hasta que su propia entrega demuestre que desaparecieron. Uno cuyo resultado ya se conoce se para una vez y se suelta.BridgeService— un token para la fuente de recepción y otro para la instancia de emisión. El de emisión cierra que una escritura de control entrada por el mirror viejo alcance el rodillo nuevo. La escritura se despacha fuera del monitor:writeCharacteristices un binder call que si nostopEmit()esperaría desde el hilo principal.Persistencia de ERG bias — el learner corre en cada muestra y el plazo se evalúa por separado, así que un bias que converge dentro del intervalo ya no se retiene hasta el apagado.
Proceso
Cuatro rondas de review, dos externas y adversariales. Tres arreglos se revirtieron después de que la review falsara la premisa en que se apoyaba cada uno, y el razonamiento queda como comentario en el sitio donde estaban para que no se reintenten. Un defecto encontrado en la re-review dirigida —un anuncio ya resuelto adoptado como huérfano permanente— está corregido.
52 tests, 0 fallos.
assembleDebugOK. JDK 17.Límites conocidos
stopAdvertising()nunca confirma la eliminación y un start puede quedarse en vuelo indefinidamente. El barrido insiste, no promete. Documentado en el propio registro.Validación pendiente
La secuencia de más riesgo, en el rodillo real: start A aceptado sin callback → watchdog → retry con B exitoso → callback de A muy tardío o perdido → stop/restart → Bluetooth OFF/ON.
Tres cosas a comprobar:
bleAdvOkcoincide con la presencia real en el aire.Incluye también
b023781y638c780, ya presentes en la rama.