Repository navigation
Serialize paired incident BLE log transfers - #4767
Conversation
PhilippeFerreiraDeSousa
added
the
bug:app
GitHub Actions
Active
GitHub Actions
Active
PR SummaryMedium Risk Overview
Adds broad JVM tests for concurrent BLE/HTTP reports, capture timeout/interruption, and partial firmware retention. Reviewed by Cursor Bugbot for commit 3d3157c. Bugbot is set up for automated code reviews on this repo. Configure here. |
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. |
📋 PR Review Helper📱 Mobile App Build✅ APK published (commit 🕶️ Updates Mentra Live glasses to this PR's ASG client via OTA (pinned to ota-pr-4767-3d3157c7fb5877f0c4a8e288d3f691614c43e87d.json, published by the ASG Client Build job). 🕶️ ASG Client Build✅ Ready to test! (commit 🔄 OTA manifest: ota-pr-4767-3d3157c7fb5877f0c4a8e288d3f691614c43e87d.json - the Mobile App PR build updates Mentra Live glasses to this ASG build. 🔀 Test Locallygh pr checkout 4767 |
![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: b2121ab453
ℹ️ 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".
| void relayLogsViaBle(String incidentId) { | ||
| // Both reports share one BES capture session and one BLE file-transfer slot. | ||
| // Hold this handler's monitor for the complete B/L transaction, not just its idle check. | ||
| synchronized void relayLogsViaBle(String incidentId) { |
There was a problem hiding this comment.
The monitor guards only relayLogsViaBle; the successful callback in uploadLogsOverHttp still calls requestBesLogs directly. If report A enters BLE fallback while report B's logcat HTTP request succeeds, B replaces the single K900CommandHandler.mBesLogSession while A is awaiting its latch, so A can time out and send an empty firmware payload while subsequent BES packets are attributed to B. Serialize every incident BES capture through the same lock rather than only the BLE relay path.
Useful? React with 👍 / 👎.
![mentra-release-coordinator[bot]](https://avatars.githubusercontent.com/u/123270946?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed by local Codex (gpt-6.1-sol, medium).
Checked: exact head b2121ab; full origin/dev...HEAD diff; root/ASG AGENTS.md and CLAUDE.md, Mentra Live spec; relay handler/tests, handler registry, K900 BES session/callbacks, trace poller and BLE transfer admission; relevant notes/agents/docs and recent dev history. The instance monitor correctly serializes two BLE relays, and completion callbacks do not require that monitor. Wire format, existing timeout/interruption handling and independent Java-log continuation are preserved.
Validation: git diff --check passed. ./scripts/check-android-compile.sh asg stopped because StreamPackLite is absent. From asg_client, ./gradlew :app:testDebugUnitTest --tests '*UploadIncidentLogsCommandHandlerTest' --no-daemon failed dependency resolution for the absent StreamPackLite core/extension-rtmp/extension-srt variants; no tests ran. Inspected all 14 tests, including the two new concurrency cases. No files edited, dependencies installed, or device actions performed. The author's passing checks are reported evidence, not locally reproduced results; paired device attachment qualification remains outstanding.
Existing comments:
- Cursor summary #6159940233: agree with its description of BLE-only serialization and the two tests; it does not establish shared ownership across HTTP callbacks.
- Codex activity summary #6159940493 and review PRR_kwDOKGbaFc8AAAABSJNQuw: status/introductory text, no separate technical claim.
- github-actions helper #6159940621: build/artifact status, not concurrency or device proof.
- Codex inline #4266341416: agree with the mixed HTTP/BLE race, independently confirmed below.
- No cubic comments or other review comments were present.
Defect:
- [P1] UploadIncidentLogsCommandHandler.java:237 (also :203-208): successful HTTP upload still invokes requestBesLogs outside the monitor. A BLE fallback for report A can be collecting BES logs when report B's HTTP success replaces K900CommandHandler.mBesLogSession (:196-200). Subsequent packets go to B while A's original collector times out and returns an empty/partial firmware payload. HTTP capture can also start during A's BLE file phase. The shared single BES resource therefore remains unowned across supported incident paths. Use one completion-aware incident capture coordinator for HTTP success and BLE relay, retaining ownership through capture and, for BLE, both file transfers; merely synchronizing the asynchronous requestBesLogs invocation is insufficient. Add mixed HTTP-success/BLE-fallback and concurrent HTTP-capture regressions.
Is this change needed?
Yes. Per-request threads, one replaceable BES session and one rejecting BLE transfer slot make overlapping paired incidents a real source-level failure mode, consistent with the PR's retained incident evidence. Existing transport admission only refuses collisions; it does not serialize the capture-plus-files transaction. Recent relay fixes and the active incident receipt/cleanup contract complement this work; the older BLE cleanup note offers no existing transaction coordinator. A small shared coordinator is justified without new dependencies or a general transport redesign.
Verdict: Request changes. The BLE-only implementation is simple and useful, but the mixed-path ownership hole leaves the same diagnostic-loss class on a normal HTTP fallback path.
|
✅ PR build ready to test for Download Android APK · Glasses OTA manifest Install on iPhone · Download iPhone IPA · Install on Mac · Download Mac app · Installation instructions |
GitHub Actions
Active
There was a problem hiding this comment.
Reviewed by local Codex (gpt-6.1-sol, medium).
Checked: exact head 6942d56; PR description and full origin/dev...HEAD diff; root/ASG AGENTS.md and CLAUDE.md, Mentra Live spec; UploadIncidentLogsCommandHandler, BesLogManager, K900CommandHandler, relay/dispatch tests, handler registration, trace poller and BLE admission; relevant notes/agents/docs and recent dev commits. The shared monitor now covers HTTP capture and the BLE B/L transaction; callbacks do not acquire it. Wire payloads, independent Java-log continuation and existing command dispatch remain compatible.
Validation: git diff --check origin/dev...HEAD passed. ./scripts/check-android-compile.sh asg stopped because StreamPackLite is missing. ./asg_client/gradlew -p asg_client :app:testDebugUnitTest --tests '*UploadIncidentLogsCommandHandlerTest' --tests '*K900CommandHandlerDispatchTest' --no-daemon failed resolving the missing StreamPackLite core/extension-rtmp/extension-srt variants; no tests ran. Read all 18 relay tests and 7 dispatch tests. The author's passing checks are reported evidence; paired concurrent-incident device qualification and the original packet-8 timeout remain unproven. No tracked files edited, dependencies installed, or device actions performed.
Existing comments:
- Cursor summary #6159940233: agree with the shared-monitor and exact HTTP-session description; clean expiry/interruption settlement applies to awaitCollection, which the BLE capture path does not use.
- Codex activity #6159940493 and its b2121ab review: status/introduction, no separate correctness claim.
- github-actions helper #6159940621: artifact status (current mobile APK published, ASG building when checked), not device/concurrency proof.
- github-actions build notification #6160063237: explicitly covers older b2121ab artifacts, not this head.
- Codex inline #4266341416: valid on the old head, addressed by the synchronized completion-aware HTTP path at :233-245.
- Previous mentra-release-coordinator request-changes review: agree with its original mixed-path diagnosis and necessity assessment; that blocker is now addressed.
- No cubic comments or additional findings were present.
Defect:
- [P2] asg_client/app/src/main/java/com/mentra/asg_client/service/core/handlers/UploadIncidentLogsCommandHandler.java:273-285, :316-318: BLE capture still discards the exact session and waits only on the JSON callback latch. Interruption releases the monitor without finishing that session; a subsequent HTTP/BLE report can replace it while BES is still streaming. Expiry likewise starts firmware/Java transfers without settling capture if the main-looper watchdog has been delayed beyond the worker's 25-second wait. The new HTTP path explicitly handles both cases, but BLE does not. Retain the exact session through a returning relay overload and use one capture-settlement path on expiry/interruption before files or lock release, preserving partial JSON delivery and the interrupt flag. Add real-session BLE-capture interruption and delayed-watchdog expiry regressions; current tests cover HTTP interruption or file-transfer interruption, not this gap.
Is this change needed?
Yes. Per-report workers race on one replaceable BES collector and one rejecting BLE file slot; the failure is real in source and consistent with the described incident evidence. Transport admission rejects collisions rather than serializing whole incidents. Recent relay fixes and the active incident receipt/cleanup contract complement this work, and the older BLE cleanup proposal supplies no existing transaction coordinator. A small shared monitor plus exact-session settlement is justified without new dependencies or a transport redesign.
Verdict: Request changes. The earlier HTTP/BLE blocker is fixed, but every exit from the newly serialized BLE capture must settle its exact session before releasing ownership.
GitHub Actions
Active
There was a problem hiding this comment.
Reviewed by local Codex (gpt-6.1-sol, medium).
Checked: exact head 3d3157c; description and full origin/dev...HEAD diff; root/ASG AGENTS.md and CLAUDE.md, Mentra Live spec; UploadIncidentLogsCommandHandler, BesLogManager, K900CommandHandler, all 20 relay tests and 7 dispatch tests, handler registration, trace poller and BLE transfer admission; relevant notes/agents/docs and recent dev history. HTTP/BLE incident captures share one monitor; BLE retains it through both files. Frozen payload publication and synchronized packet/finalization handling avoid callback dependence, retain partial logs, and preserve interruption. Existing overloads and wire format remain compatible.
Validation: git diff --check origin/dev...HEAD passed. ./scripts/check-android-compile.sh asg stopped because StreamPackLite is missing. ./asg_client/gradlew -p asg_client :app:testDebugUnitTest --tests '*UploadIncidentLogsCommandHandlerTest' --tests '*K900CommandHandlerDispatchTest' --no-daemon failed resolving the missing StreamPackLite core/extension-rtmp/extension-srt variants; no tests ran. The author's passing compile/27 tests are reported evidence, not locally reproduced. Current-head builds/CI were still pending when checked. No source edits, dependency installs or device actions.
Existing comments:
- Cursor summary #6159940233: agree; the shared monitor and exact-session settlement match this head, and callback execution cannot block the BLE payload read.
- Codex activity #6159940493 and review PRR_kwDOKGbaFc8AAAABSJNQuw: historical status/introduction for b2121ab, no separate technical claim.
- github-actions helper #6159940621: accurately distinguishes current 3d3157c builds in progress from previous 6942d56 artifacts; not device proof.
- github-actions notification #6160063237: explicitly covers 6942d56 artifacts, not this head.
- Codex inline #4266341416: agree with the original HTTP/BLE race; addressed by the synchronized HTTP helper and its completion wait (UploadIncidentLogsCommandHandler.java:231-250).
- Previous coordinator review PRR_kwDOKGbaFc8AAAABSJNxBg: original mixed-path blocker was valid and is now addressed.
- Previous coordinator review PRR_kwDOKGbaFc8AAAABSJSlmg: original BLE expiry/interruption blocker was valid and is now addressed. BLE retains the returned session and uses awaitBesCapture before files (:272-285); BesLogManager.java:167-176 finalizes that exact session on both exits. New real-session expiry/interruption and blocked-callback tests cover these cases.
- No cubic comments or additional review findings were present.
Defects: no remaining blocking defect found in this change. Independent debug trace/other transport users retain their existing admission behavior; this monitor coordinates incident requests, not every BES operation. Paired concurrent-incident artifact qualification remains outstanding, and the original packet-8 timeout is not established as fixed.
Is this change needed?
Yes. Per-report workers can overlap one replaceable BES collector and one BLE slot that rejects collisions, matching the failure mechanism described in the retained incident evidence. Existing transport admission does not serialize capture plus both files. Recent Java-log/cancellation fixes and the active incident receipt-before-cleanup contract complement this change; the older BLE cleanup proposal provides no transaction coordinator. A shared monitor plus exact-session settlement is a small, coherent fix with no new dependencies.
Verdict: Approve. Both previous ownership blockers are addressed with one settlement path and useful regressions. This is source approval, with local build/test and device-evidence limitations stated above.
This branch was successfully deployed
1 participant
Two automatic incidents five seconds apart can race through the glasses’ single BES log session and BLE file-transfer slot. In rep_01M4VZKDJ9MKFP93XGD8FN22SK, the second BES capture overlapped the first report’s glasses file transfer; native logs show “File transfer already in progress” and refusal of the second firmware file. Serialize both direct-HTTP incident capture and BLE capture plus firmware/Java file sequences on this handler’s monitor. The HTTP path waits for its exact returned BES session to finish, separately from upload. Expiry or interruption finalizes that capture through the existing partial-log completion path before releasing ownership. Other command handlers and the phone report API are unchanged.
Validation: repo Android compile check and all 27 focused JVM tests pass with Java 17 (20 incident relay tests plus 7 K900 dispatch tests). Coverage includes concurrent distinct-report files, HTTP/BLE overlap in both directions, continuation after the first firmware start is refused, exactly one partial delivery on capture expiry, and interruption preservation. The two concurrent BLE regressions fail when the transaction monitor is removed. Existing timeout, connection loss and interruption tests remain passing. The original first report’s packet-8 timeout remains unresolved: concurrent BES traffic is observed, but this PR does not claim a proven transport fix or complete device attachment qualification.
The incident’s existing diagnostic bundle contains the original ASG logcat: second mh_logs at 01:14:43.088 UTC, first Java file start at 43.412, second capture completion at 46.733 and rejected firmware transfer at 46.761. Report delivery failure stayed a rejected incident result and all subsequent teardown steps passed.
Device coverage: inspected Harness main d7487aea and ota-roundtrip-mac/feedback-submit. OTA roundtrip exercises updates, but does not deliberately submit two concurrent paired incidents; feature feedback intentionally has no logs. No duplicate OTA request is added while the separately owned PR4765 verification runs. Paired concurrent-incident attachment qualification remains required; source tests do not establish firmware deployment.
Both HTTP and BLE capture paths use one exact-session settlement helper. BLE reads the frozen payload directly after settlement, so callback scheduling cannot deadlock the shared monitor. Interrupted captures settle before releasing ownership; expired captures preserve partial firmware logs even when the native watchdog is delayed. The focused tests cover both exits and a deliberately blocked completion callback.