Repository navigation
[SUPERSEDED by revert #21484 / RE-LAND BASE] fix(android): close retention-probe audio-loss windows and make the escalated backoff reachable - #21475
Git-on-my-level wants to merge 5 commits into
Conversation
…scalated backoff reachable Review of #21404 left three verified P1s against the merged design (independent astra audit concurred; the fourth, stale-proof-consumption on new GATT, was refuted as stated but is hardened here anyway): - Subscribe before probing: fireDeviceReady now requests the audio CCCD write before the optional retention probe, and the probe read is posted through the same looper as subscribeCharacteristic's deferred enqueue, so the probe can never own the serial GATT queue ahead of capture (firmware drops dequeued frames while connected-but-unsubscribed, and a lost read callback has no queue timeout). - Require a valid RTC before certifying retention: retentionProbeSucceeds now demands the status payload's rtc-valid field; the firmware offline writer drops every request while its clock is unsynced, so mounted-but-timeless storage cannot license longer reconnect gaps. - Preserve a completed proof across disconnected retries and invalidate it on each new GATT connection: RetentionProbeLifecycle.interrupt() becomes cancel() (retires an in-flight probe without erasing completed proof), so delayForFailure can actually reach the advertised 6/12/30/60s schedule; the proof resets in onGattConnected before anything on the new link consumes it, closing the rebooted-into-SD-failure window (the refuted finding 3). Tests: RTC pins in BleReconnectBackoffTest, proof-lifecycle pins in RetentionProbeLifecycleTest, and a new source-reading contract test pinning subscribe-before-probe, the deferred probe enqueue, cancel-vs-reset semantics, and connect-time invalidation. Invariant: INV-CAP-1 (product/invariants/pendant-audio-retention.md).
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdfc905216
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // posts its enqueue behind this subscription's own post, so the CCCD | ||
| // write owns the serial queue first and a slow or lost probe read can | ||
| // never delay it. | ||
| ensureBackgroundAudioSubscription(addr, services) |
There was a problem hiding this comment.
On a first connection, or whenever nativeBleStreamConfig is absent or does not match this address, this call returns without posting a CCCD write, but probeRetentionRuntime is still posted before Flutter's onDeviceReady notification. Flutter cannot create its audio subscription until that notification, so the storage read still reaches the serial GATT queue first; because ordinary reads have no queue timeout, a missing callback can wedge the queue and leave the connected pendant unsubscribed indefinitely. Defer the probe until either the native subscription was actually queued or Flutter has had an opportunity to queue its audio subscription.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| val prefix = body.substringBefore(match.range.first) | ||
| prefix.substringAfterLast("mainHandler.post", "").isNotEmpty() |
There was a problem hiding this comment.
This assertion treats any preceding text containing mainHandler.post as proof that the read is inside a posted block, but the production method's explanatory comment already contains that exact text. Moving readCharacteristic back to a direct call while retaining the comment therefore leaves both this prefix check and the earlier body.contains("mainHandler.post") check green, so the new contract test does not catch the queue-order regression it was added to prevent. Strip comments or inspect the Kotlin syntax/block structure instead.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
…ssertion in the retention contract test
…e after clock sync Addresses the fresh review round on this PR (P1 x2, P2 x1): - fireDeviceReady now tracks whether the native audio subscription was queued. With no native target and Flutter alive, the retention probe is posted behind the onDeviceReady notification, so Dart's own subscription (queued from that callback) reaches the serial GATT queue before the probe read. With neither native target nor Flutter, no subscription can ever arrive, so the probe cannot delay one. - The probe failure shape 'mounted storage with RTC still unsynced' schedules ONE bounded re-probe 20s after the first (Dart performs performSyncTime after onDeviceReady). Budget is per-link, reset on each new GATT connection. - The third finding (invalidate at attempt start) is rejected: it contradicts the original review's own prescription and re-breaks reachability of the escalated schedule (the finding this PR fixes); the proof boundary stays 'new GATT connection', as prescribed. Tests: contract pins for the Flutter-path deferral and the re-probe budget; backoff pins for storageStatusHasDataWithoutClock.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4daad82767
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| val managed = managedDevices[address.uppercase()] ?: return@postDelayed | ||
| probeRetentionRuntime(address) |
There was a problem hiding this comment.
When link A schedules this callback and disconnects before 20 seconds, a replacement link B for the same address may be ready when it fires; the looked-up managed value is unused, so probeRetentionRuntime() simply captures B's current GATT instead of rejecting the replaced link as the comment claims. If B has already verified retention, starting this stale probe clears runtimeVerified, and a read failure, timeout, or disconnect during it loses B's valid proof and returns retries to flat cadence. Capture the originating GATT or a connection generation when scheduling and ignore the callback after replacement.
Useful? React with 👍 / 👎.
…in self-healing Round-3 review: a stale re-probe callback could fire on a successor GATT and clear its fresh proof (probeRetentionRuntime captures the current GATT), and a single fixed-delay re-probe could expire before Dart's connect-time GATT work finishes, wasting the per-link budget. Re-probes now carry the originating GATT and attempt number: a replaced link rejects the callback outright, and the chain re-arms only while the probe keeps seeing mounted-storage-without- clock, up to 3 cycles per link (budget reset on each new GATT connection). The RTC gate itself is unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5288ac400a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ange retry loops (#21404)" (#21484) ## Summary Reverts merge b7e38cf (#21404, "exponential BLE reconnect backoff for out-of-range retry loops") ahead of the 1.0.559 train, per the review-debt ruling of 2026-10-14. Why revert rather than fix forward: #21475 (the fix-forward for #21404's four unanswered P1s) resolved the three verifiable findings, but a focused second-model arbitration of the remaining design disagreement (invalidate retention proof per new GATT connection vs. per attempt) ruled the conservative design audio-loss-critical, not battery-efficiency-only: with a stale proof surviving failed attempts, a device that rebooted into broken storage and then returns to range mid-gap is reached later (6-60s schedule instead of 3s), extending the connected-but-unsubscribed window over audio the broken device is discarding — exactly the class INV-CAP-1's own history rejected ("a trade that silently deletes user recordings is not acceptable at any battery saving"). The conservative design eliminates the escalation feature entirely, so the honest disposition is revert; the fixes and the gate design (per-link proof, RTC-validity gate, subscribe-before-probe) are preserved in #21475 for a re-land only after the escalation gate gets a retention guarantee that covers the disconnected interval. ## Product invariants - INV-CAP-1 (pendant-audio-retention): the revert removes the battery policy whose gate could not prove retention across the disconnected interval. Reconnect pacing returns to the pre-#21404 steady cadence. ## Failure class Failure-Class: none ## Test plan - [x] `git revert -m 1 b7e38cf` — clean (it reverses the original merge wholesale, including its tests and changelog fragment) - [x] No remaining references to BleReconnectBackoff / RetentionProbeLifecycle in main or test sources - [ ] Full CI green on this exact head, then merge to clear the train <!-- This is an auto-generated description by cubic. --> <a href="https://cubic.dev/pr/BasedHardware/omi/pull/21484?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="View guided diff" src="https://www.cubic.dev/buttons/review-in-cubic-light.svg"></picture></a> <a href="https://www.cubic.dev/action/auto-fix/pr/BasedHardware/omi/21484?returnTo=https%3A%2F%2Fgithub.com%2FBasedHardware%2Fomi%2Fpull%2F21484&source=description" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/turn-on-auto-fix-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/turn-on-auto-fix-light.svg"><img alt="Turn on auto-fix" src="https://www.cubic.dev/buttons/turn-on-auto-fix-light.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img](https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"></picture></a](https://www.cubic.dev/buttons/review-in-cubic-light.svg"></picture></a){kind=link}
![https://www.cubic.dev/buttons/turn-on-auto-fix-dark.svg"><source](https://www.cubic.dev/buttons/turn-on-auto-fix-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/turn-on-auto-fix-light.svg"><img](https://www.cubic.dev/buttons/turn-on-auto-fix-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/turn-on-auto-fix-light.svg"></picture></a](https://www.cubic.dev/buttons/turn-on-auto-fix-light.svg"></picture></a){kind=link}
…ion) for the re-land record
Re-land of the #21404 battery backoff, gated on a redesigned escalation licenseBlocked on a design decision (David): the 10-14 arbitration (gpt-6-astra, adversarial, audio-loss-only standard) ruled that escalating reconnect gaps on a per-connection retention proof is audio-loss-critical — a device that reboots into broken storage while out of range is reached later (6–60s vs 3s), extending the deaf window over audio it is discarding (transport.c:1224–1251). INV-CAP-1's own history ("a trade that silently deletes user recordings is not acceptable at any battery saving") therefore forces one of:
What survives from #21475 regardless (engineering record, all tested)
Evidence
|
Git-on-my-level
changed the title
Closed wont-fix per owner ruling (Option A) — 2026-10-14Decision (David, 2026-10-14T12:23Z, ANX ask Why: the 2026-10-14 independent arbitration (gpt-6-astra via codex, exec Correction of the first version of this comment: the arbitration was performed by gpt-6-astra (codex), not grok-4.7-high; grok-4.7-high was the separate fixed-code re-audit ( What this PR remains: the preserved RE-LAND BASE at head
If firmware ever ships the precondition (advertised disconnected-interval retention flag + writer-health word incl. Battery work continues elsewhere (INV-CAP-1 unchanged, no backoff in 1.0.559). |
|
Hey @Git-on-my-level 👋 Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request. After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:
Before your next PR, please skim:
If this was declined for direction or taste, maintainers should cite an invariant ID or open a proposed one — ask if that citation is missing. Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out. Thank you for being part of the Omi community! |
1 participant
Summary
Closes the review debt of #21404: three of the four P1 threads left unanswered at merge were verified REAL against current origin/main by an independent line-level trace, and a second independent model audit (gpt-6-astra, unseeded) concurred on all four verdicts. The fourth P1 was REFUTED as filed, and this PR still hardens the exact boundary it pointed at. No audio-loss path ships in the 1.0.55x builds, and the escalated reconnect schedule this PR series advertised becomes actually reachable.
Subscribe before probing (P1, real).
fireDeviceReadyran the retention probe before the audio subscription.readCharacteristicenqueues synchronously into the serial GATT command queue whilesubscribeCharacteristicdefers through the main looper, so the probe read could own the queue first; the firmware drops dequeued frames while connected-but-unsubscribed, and a lost read callback (the queue has no timeout for reads) blocked the CCCD write indefinitely. The subscription now runs first and the probe read is posted behind it on the same looper, so it can never precede the CCCD write.Require a valid RTC before certifying retention (P1, real). The firmware offline writer drops every request while its clock is unsynced (
sd_card.c process_write_data_req), butretentionProbeSucceedsdiscarded the status payload's rtc-valid field, so a mounted-but-timeless device (rebooted while Flutter absent, no persisted epoch) was certified as retaining audio it was actually throwing away. RTC validity is now required; RTC alone still does not license escalation.Preserve a completed proof across disconnected retries; invalidate on each new GATT connection (P1, real).
RetentionProbeLifecycle.interrupt()erased completed verification at every disconnect and retry attempt, always BEFOREhandleRetryLogicread it, sodelayForFailurealways sawretentionVerified=falseand the advertised 6/12/30/60s schedule was unreachable (flat 3s forever). The real hazard ran the other way: the schedule was dead, and the naive fix (keep proof forever) would strand offline audio behind long gaps on devices whose storage later broke. Nowinterrupt()iscancel()(retires an in-flight probe, keeps completed proof), and the proof resets inonGattConnectedbefore anything on the new link can consume stale verification.Stale proof on a new GATT (P1, refuted as filed, hardened anyway). The disconnect path already retired the in-flight probe before retry selection, so the filed window (consume stale proof on a pre-ready disconnect) could not occur as described. The connect-time invalidation in (3) re-establishes the same guarantee structurally: any proof describes exactly the GATT link it was proven on.
Product invariants
Failure class
Failure-Class: none
Test plan
BleReconnectBackoffTest: RTC-less payloads with nonzero storage stay flat; escalation requires rtc=1; overflow cases keep rtc=1RetentionProbeLifecycleTest: completed proof survivescancel(); fresh connection resets it; late/replaced-GATT callbacks still rejectedRetentionCaptureOrderingContractTest(new): reads the real service source and pins fireDeviceReady ordering, the deferred probe enqueue, cancel-vs-reset, and connect-time invalidationandroid-unit-tests)