[krassx]

Summary

• Android createReport keeps one outstanding report. A second call is E_REPORT_CREATE_BUSY.
• fulfil writes only when the reservation stamp captured at tryReserve is still current, so a listener that outlives invalidate cannot fill the next runtime's slot.
• invalidate still clears the registry.

Test plan

• CreatedReportsTest 8/8, including aStaleStampDoesNotConsumeANewerReservation
• JVM unit tests and assembleDebug
• Not a device pass. Task 8.3a is the device suite, and it is blocked because the report dialog's send control has no stable resource id
• Merge after Task 8.2a. This branch does not contain the iOS bridge (Task 8.2c).

Made with Cursor

[cursor]

Stale comment

Deep code review — Task 8.2b (Android createReport registry)

Reviewed 0542501 against feat/phase-8-2a (cd79431), with the JS createReport / CreatedReport contract, ReportHandlerBridge / WrapperEventBus reload pattern, and plan P5 (one outstanding created report per JS runtime, released on upload() and on module invalidate).

Android createReport is no longer a stub: it reserves a process-wide slot, asks Bugsee.createReport, and mints cr-<n> through CreatedReports. The stamp added in the follow-up commit correctly stops a listener that outlived clear() from filling or freeing a later reservation. That part is sound. The remaining hole is the other half of the same reload overlap this module already documents.

Finding

Severity: P1 High
Location: BugseeModule.java:140 (CreatedReports.shared().clear()), with no matching attach in the constructor
Problem: CreatedReports is a process-wide singleton, but invalidate() clears it unconditionally. Every other process-wide sink in this class is identity-checked for the documented ordering where a reload constructs and attaches the new module before the old one is invalidated. Stamps only protect fulfil; they do not protect an already-reserved or already-fulfilled slot from clear().
Impact: On RN context recreation the next runtime can lose a report it just created (E_REPORT_HANDLE_DEAD on read/update/upload), or its first createReport() can spuriously reject E_REPORT_CREATE_BUSY because the dying runtime still occupies the only slot. Plan P5 (“released by a JS reload”) is supposed to free the old runtime’s slot, not the live one.
Scenario:

1. Reload starts. New BugseeModule is constructed and new JS runs while the old module is still alive (invalidate() javadoc already states this order).
2. New JS calls createReport(), tryReserve succeeds, the SDK listener fulfils cr-n.
3. Old invalidate() runs CreatedReports.shared().clear() and drops that handle.
4. New JS read() / update() / upload() all miss and reject E_REPORT_HANDLE_DEAD. The filled report is gone.

Symmetric window the other way: old JS still holds a reservation or a fulfilled report when new JS starts. New createReport() hits tryReserve() == 0 and rejects E_REPORT_CREATE_BUSY. The dying runtime then clear()s, leaving the slot empty. Typical app code does not retry BUSY, so a launch-time createReport() fails for that runtime.

Identity-checking only detach is not enough: if the new module has already taken ownership, an old clear() must no-op, which means leftover state from the previous owner would otherwise sit in the slot forever (new JS does not know cr-(n-1) and cannot take it). The new module has to claim the registry on construct and drop that leftover then, before new JS runs.
Fix: Mirror WrapperEventBus / ReportHandlerBridge / DataRequestBridge:

• CreatedReports.attach(owner) in the constructor: set the owner, drop any leftover reservation/report (new JS cannot use those handles), leave stamps invalid so an in-flight listener cannot fulfil into the new runtime.
• CreatedReports.detach(stale) from invalidate(): clear() only if stale is still the owner.
• Unit-test both sides: attach(B) then detach(A) must not drop B’s reservation; attach(B) must drop A’s leftover so B can tryReserve.

---

1. Overall risk: Medium

One P1 on the reload overlap this module already treats as real. Not a crash, not a security issue, and the in-process one-slot + stamp path looks correct for a single live runtime.

2. Merge recommendation: Wait

Fix the owner-scoped attach/detach before merging. The stamp work should stay.

3. Most important issue to fix

Unconditional CreatedReports.clear() on invalidate(), with no constructor attach. That is the same class of bug the rest of this file already refuses to ship.

4. Positive observations

• The stamp/tryReserve split is the right answer for a listener that outlives clear(), and aStaleStampDoesNotConsumeANewerReservation pins both a stale report and a stale null fulfil.
• Concurrent reserve admission, fresh cr-<n> handles, and take-vs-get are covered.
• Created-report ops correctly reuse ReportOps and copy files (move = false). Taking the handle before Bugsee.upload matches the JS CreatedReport contract that the handle is dead whether or not upload succeeds.
• E_REPORT_CREATE_BUSY matches ReportErrorCode.CreateBusy and the JS translation path from 8.2a.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep code review — Task 8.2b (re-review of 4d61e8a)

Re-reviewed against feat/phase-8-2a (cd79431), the JS createReport / CreatedReport contract from 8.2a, plan P5, Android SDK 7.3.0 Bugsee.createReport / createAndPrepareBugReport / openAsync, and the iOS sibling (#21).

Production Android is unchanged since 0542501 (the stamp on fulfil). The three later commits are tests only: JS mutation pins for severity/argument text, the iOS settle-once race no longer has a third deadline caller, and created-report.test.ts covers omitted mime types, empty path/name, and native rejection mapping.

Prior finding

P1 — unconditional CreatedReports.clear() on invalidate(): not addressed (owner declined attach/detach).

The 0.87 serial-teardown argument is accepted for that version: this constructor never reserves, so old clear() cannot drop a reservation the new module has not opened yet, and the stamp still ignores a listener that outlives invalidate. The residual is the floor this package still declares (react-native >= 0.81). This method’s own javadoc still describes new-module-construct-before-old-invalidate for WrapperEventBus / ReportHandlerBridge. On that order, new JS createReport() can reserve (or sit behind the dying runtime’s slot) before old clear():

• new runtime already holds cr-n → old clear() drops it → read / update / upload reject E_REPORT_HANDLE_DEAD;
• dying runtime still occupies the one slot → new createReport() rejects E_REPORT_CREATE_BUSY once, then clear() frees an empty slot.

Same residual as iOS #21. Stamps protect fulfil only; they do not protect an already-reserved or already-fulfilled slot from clear().

No new P0–P3

Walked CreatedReports (reserve / fulfil / take / clear, stamp skip of 0, concurrent admission), BugseeModule.createReport and the five createdReport* methods, ReportOps reuse, and 7.3.0:

• createReport always invokes the listener (null when not launched); openAsync runs on the storage thread, so the native-modules thread is not blocked on onBeforeReportCreated.
• Overlapping SDK creates are independent ReportingRequest instances (no iOS-style file-scope globals). Holding the slot until a stale listener returns would make reload BUSY; ignoring a stale stamp is the right Android answer.
• take before Bugsee.upload(report, …) matches the JS contract that the handle is dead whether or not upload succeeds. The SDK looks the request up from the Report (WeakRef, then IRR on disk) and keeps a strong map from open() until discard, so a held Report remains uploadable.
• addFile is copy (move = false). Attachment decline vs dead handle reuses the handler’s settle rule against this registry.

JVM CreatedReportsTest was not re-run here (standalone Gradle lacked the Android plugin in this environment). CI android was still pending at review time; rn compat 0.81–0.87 had passed.

---

1. Overall risk: Low

One declined residual on 0.81 reload overlap. The in-process one-slot + stamp path is correct for a single live runtime.

2. Merge recommendation: Safe into feat/phase-8-2a

Do not treat owner-checked clear as done for the 0.81 floor. iOS registry is Task 8.2c.

3. Most important issue

The residual P1 above, if 0.81 (or any reload where new JS runs before old invalidate) matters. No new production defect on this head.

4. Positive observations

• Stamp/tryReserve still correctly ignores a listener after clear(), including a stale null fulfil, and aStaleStampDoesNotConsumeANewerReservation pins both.
• Created-report ops correctly reuse ReportOps and copy files. E_REPORT_CREATE_BUSY matches ReportErrorCode.CreateBusy.
• The extra JS tests close the mutation holes that were surviving (severity 5, exact TypeError/RangeError text, omitted mime type, native rejection → BugseeReportError).

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep code review — Task 8.2b (re-review of edc94cc)

Delta since 4d61e8a is one test commit: testTwoConcurrentCallsSettleOnce now uses a separate enter/release semaphore pair per race window. A shared go let the faster caller leave window 0, enter window 1, and consume the other thread’s release, so that caller never reached window 1 (CI timed out). Per-window pairs plus a per-window 5s wait are the right fix. Production Android (CreatedReports, BugseeModule.createReport*) is unchanged since 0542501.

Prior finding

P1 — unconditional CreatedReports.clear() on invalidate(): not addressed (owner declined attach/detach). Residual remains the 0.81 floor / any reload where new JS createReport runs before old invalidate (HANDLE_DEAD or a one-shot E_REPORT_CREATE_BUSY). Stamps still protect fulfil only. Same decision as iOS #21.

No new P0–P3

The settle-once barrier change does not alter BGSRNSettleOnce production code. _Thread_local phase is per GCD worker and is correct for a single run of this test (each fire() visits the window twice).

---

1. Overall risk: Low

2. Merge recommendation: Safe into feat/phase-8-2a

Do not treat owner-checked clear as done for the 0.81 floor. iOS registry is Task 8.2c.

3. Most important issue

The residual P1 above. No new production defect on this head.

4. Positive observations

The per-window semaphore split matches how RunRaceWindow is actually invoked (before the load, then after it) and removes a real CI timeout mode.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep code review — Task 8.2b (re-review of edc94cc)

Re-reviewed against feat/phase-8-2a (cd79431), the JS createReport / CreatedReport contract from 8.2a, plan P5, Android SDK 7.3.0 Bugsee.createReport / createAndPrepareBugReport / openAsync, and the iOS sibling (#21).

Production Android is unchanged since 0542501 (the stamp on fulfil). The only commit after the last review (4d61e8a) is edc94cc: test-only. Each settle-once race window now has its own enter/release pair so a faster GCD caller cannot leave window 0, enter window 1, and consume the other caller’s signal (the timeout that showed up as “caller 3”).

Earlier follow-ups on this PR remain tests: JS mutation pins for severity/argument text, dropping the settle-once deadline as a third waiter, and created-report.test.ts coverage for omitted mime types, empty path/name, and native rejection mapping.

Prior finding

P1 — unconditional CreatedReports.clear() on invalidate(): not addressed (owner declined attach/detach).

The 0.87 serial-teardown argument is accepted for that version: this constructor never reserves, so old clear() cannot drop a reservation the new module has not opened yet, and the stamp still ignores a listener that outlives invalidate. The residual is the floor this package still declares (react-native >= 0.81). This method’s own javadoc still describes new-module-construct-before-old-invalidate for WrapperEventBus / ReportHandlerBridge. On that order, new JS createReport() can reserve (or sit behind the dying runtime’s slot) before old clear():

• new runtime already holds cr-n → old clear() drops it → read / update / upload reject E_REPORT_HANDLE_DEAD;
• dying runtime still occupies the one slot → new createReport() rejects E_REPORT_CREATE_BUSY once, then clear() frees an empty slot.

Same residual as iOS #21. Stamps protect fulfil only; they do not protect an already-reserved or already-fulfilled slot from clear().

No new P0–P3

Walked CreatedReports (reserve / fulfil / take / clear, stamp skip of 0, concurrent admission), BugseeModule.createReport and the five createdReport* methods, ReportOps reuse, and 7.3.0 bytecode (BugseeInternal.createReport, createAndPrepareBugReport, sendBugReport):

• A null listener returns without creating. A live listener always gets a callback: no app token → posted null on main; otherwise openAsync.
• Overlapping SDK creates are independent ReportingRequests (no iOS-style file-scope globals). Holding the slot until a stale listener returns would make reload BUSY; ignoring a stale stamp is the right Android answer.
• take before Bugsee.upload(report, …) matches the JS contract that the handle is dead whether or not upload succeeds. processAndUploadReport looks the request up from the held Report before that call returns.
• addFile is copy (move = false). Attachment decline vs dead handle reuses the handler’s settle rule against this registry.

CreatedReportsTest was not re-run here. CI on 0542501 had android and ios unit (BugseeRNSupport) SUCCESS; lint, typecheck, unit, mutation FAILED (the hole the later JS tests close). CI on edc94cc was still in progress at review time.

---

1. Overall risk: Low

One declined residual on 0.81 reload overlap. The in-process one-slot + stamp path is correct for a single live runtime.

2. Merge recommendation: Safe into feat/phase-8-2a

Do not treat owner-checked clear as done for the 0.81 floor. iOS registry is Task 8.2c.

3. Most important issue

The residual P1 above, if 0.81 (or any reload where new JS runs before old invalidate) matters. No new production defect on this head.

4. Positive observations

• Stamp/tryReserve still correctly ignores a listener after clear(), including a stale null fulfil, and aStaleStampDoesNotConsumeANewerReservation pins both.
• Created-report ops correctly reuse ReportOps and copy files. E_REPORT_CREATE_BUSY matches ReportErrorCode.CreateBusy.
• edc94cc is the right fix for the two-phase barrier: a shared go cannot keep both callers in the same window. Per-window enter/release matches how BGSRNSettleOnce actually parks (before the load, then after it).

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep code review — Task 8.2b (re-review of 02ddf11)

Reviewed the three-dot range 1fd9ded...02ddf11 against the live tree, not the pre-merge CreatedReports design in isolation. The unique production work on this branch is still the one-slot registry (CreatedReports) and the five createdReport* ops. 02ddf11 merged main (#19 stubs, split ExceptionExecutors, iOS comment/dialog nil) into that work.

P0 — duplicate created-report methods after the merge

BugseeModule now contains both the 8.2a stubs (resolve null / E_REPORT_HANDLE_DEAD) and the 8.2b implementations. Six pairs share a signature. That is a Java compile error, not a runtime race: this head cannot build.

The merge message said to keep this branch’s registry. The stub block at L771–L821 should have been deleted; it was not.

I did not wait for the in-flight Android CI job; duplicate methods fail javac regardless.

Prior finding

P1 — unconditional CreatedReports.clear() on invalidate(): still not addressed (owner declined attach/detach). Residual is the 0.81 floor / any reload where new JS createReport runs before old invalidate (stolen cr-n → HANDLE_DEAD, or a one-shot E_REPORT_CREATE_BUSY). Stamps still protect fulfil only. Same decision as iOS #21. Do not treat it as newly introduced by this merge.

No other new P1–P3 in CreatedReports / the real createReport* path. The stamp skip of 0, concurrent admission, take before Bugsee.upload, addFile copy, and attachment settle-against-registry logic are unchanged since 0542501.

---

1. Overall risk: Critical

The merge does not compile. Registry correctness is moot until the stubs are removed.

2. Merge recommendation: Do not merge

Delete the 8.2a stub block, confirm CreatedReportsTest and assembleDebug, then re-review. iOS registry remains Task 8.2c.

3. Most important issues to fix

1. Remove the six stub methods at BugseeModule.java L771–L821 so only the registry implementations remain.
2. Residual P1 (clear() vs 0.81 reload overlap) — already declined; not a merge blocker once the class compiles.

4. Positive observations

• ExceptionExecutors split from main is wired correctly (HANDLED / UNHANDLED); that part of the merge is fine.
• The one-slot + stamp design is still the right Android answer for a late onCreated after invalidate.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep code review — Task 8.2b (re-review of 02ddf11)

The branch was retargeted to main and merged 1fd9ded (#19). Merge message: keep this branch's created-report registry; keep main's split exception executors, beta3 recovery comment, and omitted dialog fields as nil.

ExceptionExecutors (handled vs unhandled queues) and iOS showReportDialog passing omitted summary/description as nil are correctly taken from main. The registry itself (CreatedReports, deliverCreatedReport, stamp on fulfil) is still present.

The merge did not drop the 8.2a stubs. BugseeModule.java now defines createReport and all five createdReport* methods twice.

Findings

Severity: P0 Critical
Location: BugseeModule.java:771–821 (stubs) and :830–973 (registry implementations)
Problem: After merging main, the 8.2a stubs (promise.resolve(null) / rejectHandleDead) and this PR's real methods share the same signatures. javac rejects a class with two createReport(Promise) methods (and the same for the five createdReport* ops).
Impact: The Android library does not compile. This slice cannot ship, and CI android cannot go green on this head.
Scenario: Merge of #19 onto this branch kept both sides of the conflict. Android compile: method createReport(Promise) is already defined in class BugseeModule (repeated for each duplicate).
Fix: Delete the six stub methods (the block commented Task 8.2b. No registry yet). Keep the registry implementations. Also drop the unused import java.util.concurrent.Executors left after EXCEPTION_EXECUTOR moved to ExceptionExecutors.

P1 residual (not addressed): unconditional CreatedReports.clear() on invalidate() (owner declined attach/detach). Same 0.81 reload overlap as before.

---

1. Overall risk: Critical

The merge does not compile. Nothing else on this head matters until the stubs are removed.

2. Merge recommendation: Wait

Delete the stubs, then this is again the 8.2b registry on top of main.

3. Most important issue to fix

Duplicate createReport / createdReport* methods introduced by the main merge.

4. Positive observations

• Split ExceptionExecutors from #17 is intact (unhandled is not queued behind handled).
• iOS dialog omitted fields stay nil (#18).
• The registry, stamps, and take-before-upload path were not dropped; they are just unreachable until the class compiles.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep code review — Task 8.2b (re-review of 5eb1935)

5eb1935 deletes the six 8.2a stub methods the main merge left beside the registry. BugseeModule now has one createReport and one of each createdReport* op, all going through CreatedReports. That was the compile break; it is gone.

Prior findings

P0 — duplicate stubs + registry methods: fully addressed. Stub block (former L771–L821) is deleted. Confirmed a single definition of each method on this head.

P1 — unconditional CreatedReports.clear() on invalidate(): not addressed (owner declined attach/detach). Residual remains the 0.81 floor / any reload where new JS createReport runs before old invalidate (HANDLE_DEAD or a one-shot E_REPORT_CREATE_BUSY). Stamps still protect fulfil only. Same decision as iOS #21.

No new P0–P3

Registry, stamp skip of 0, take before Bugsee.upload, file copy (move = false), and attachment settle-against-registry are unchanged since 0542501. Split ExceptionExecutors from main is still wired. Leftover unused import java.util.concurrent.Executors is not a compile error; not raised as a finding.

---

1. Overall risk: Low

The merge compiles again. One declined residual on 0.81 reload overlap.

2. Merge recommendation: Safe into main

Do not treat owner-checked clear as done for the 0.81 floor. iOS registry is Task 8.2c.

3. Most important issue

The residual P1 above. No remaining production defect that blocks this slice.

4. Positive observations

The stub deletion matches the merge intent (“keep this branch’s created-report registry”) and is the minimum fix that makes javac succeed.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Deep code review — Task 8.2b (re-review of 5eb1935)

Reviewed 1fd9ded...5eb1935 against the live tree, the JS createReport / CreatedReport contract from 8.2a, plan P5, ReportHandlerBridge / WrapperEventBus reload identity-checks, and the iOS sibling (#21).

5eb1935 deletes the six 8.2a stub methods the main merge left beside the registry. BugseeModule now has a single createReport and a single copy of each createdReport* op, all going through CreatedReports. That was the compile break; it is gone.

Prior findings

P0 — duplicate stubs + registry methods: fully addressed. The stub block (former L771–L821, commented Task 8.2b. No registry yet) is deleted. Confirmed one definition each of createReport (L778), createdReportRead (L830), createdReportUpdate (L844), createdReportAddDataAttachment (L865), createdReportAddFileAttachment (L894), and createdReportUpload (L921).

P1 — unconditional CreatedReports.clear() on invalidate() (L125): not addressed (owner declined attach/detach). Residual remains the package floor (react-native >= 0.81) / any reload where new JS createReport runs before old invalidate:

• new runtime already holds cr-n → old clear() drops it → read / update / upload reject E_REPORT_HANDLE_DEAD;
• or the dying runtime still occupies the one slot → new createReport() rejects E_REPORT_CREATE_BUSY once, then clear() frees an empty slot.

Stamps still protect fulfil only. The 0.87 serial-teardown argument is accepted for that version (constructor never reserves). Same decision as iOS #21.

No new P0–P3

Walked CreatedReports (reserve / fulfil / take / clear, stamp skip of 0, concurrent admission), BugseeModule.createReport + the five ops, ReportOps reuse, and the JS wrapper:

• tryReserve then Bugsee.createReport; a throw ends that stamp; the listener carries the stamp so a late onCreated cannot fill or free a later reservation.
• take before Bugsee.upload matches the JS contract that the handle is dead whether or not upload succeeds. Android requests are independent (no iOS-style file-scope globals), so freeing the slot at upload() start is the right Android answer.
• addFile is copy (move = false). Attachment decline vs dead handle reuses the handler settle rule against this registry.
• E_REPORT_CREATE_BUSY matches ReportErrorCode.CreateBusy and the 8.2a translation path.
• Split ExceptionExecutors from main is still wired (HANDLED / UNHANDLED).
• Leftover unused import java.util.concurrent.Executors is not a compile error; not raised as a finding.

Did not run CreatedReportsTest here: standalone Gradle failed with SDK location not found (ANDROID_HOME unset). CI android on this SHA was still queued at review time.

---

1. Overall risk: Low

The merge compiles again. One declined residual on 0.81 reload overlap. The in-process one-slot + stamp path is correct for a single live runtime.

2. Merge recommendation: Safe into main

Do not treat owner-checked clear as done for the 0.81 floor. iOS registry remains Task 8.2c.

3. Most important issue

The residual P1 above. No remaining production defect that blocks this slice.

4. Positive observations

• Stub deletion matches the merge intent (“keep this branch’s created-report registry”) and is the minimum fix that makes javac succeed.
• Stamp/tryReserve still correctly ignores a listener after clear(), including a stale null fulfil, and aStaleStampDoesNotConsumeANewerReservation pins both.
• Created-report ops correctly reuse ReportOps and copy files.

  

_{Sent by Cursor Automation: Bugsee code review}