Phase 10: notify and APM spans

[krassx]

Summary

• notify queues a persisted notification. startTransaction, startSpan, and getActiveSpan hold native spans until finish releases them, including children the SDK has already finished.
• The four span setters return a boolean so they run on the JS thread ahead of finish. A setAttribute then finish in the same turn reaches the SDK, and the device test requires that attribute on the child span.
• iPhone XS (KRSFT) was already running BareExample, so that handset was not run.

Test plan

• Unit tests, including the spec-return lock and aFinishedSpanIsReleased
• Android WOD_LX1 AMRJCP4718402860 Debug: notify-f70ead5fa74c / txn-f70ead5fa74c, child attributes held attr-f70ead5fa74c
• iOS Simulator 6FA9B3E8-26C7-4232-AA2C-537D9DF32957 Debug: notify-c88c83270b1d / txn-c88c83270b1d, same attribute
• iPhone XS not run (BareExample already running)

Made with Cursor

[cursor]

Stale comment

Deep code review: Phase 10 notify and APM spans

The PR's intent is clear: notify is a fire-and-forget relay, and spans are objects with a lifetime that the bridge must not leak. The setter follow-up (boolean returns so codegen stays on the JS thread) is the right fix for the setAttribute then finish in one turn bug, and the device cases for the attribute landing in the report are the evidence that path needed.

I reviewed the JS facade, both native registries, invalidate, the TurboModule spec, and the surrounding JSON/thread patterns this repo already uses. I did not run the unit or device suites in this environment.

Findings

1. P2 Native finish only drops a handle when isFinished() is already true; JS then throws E_SPAN_HANDLE_DEAD on the first finish() if that handle is missing from the returned list. That combination can leak the native span and make finish() unrecoverable.
2. P2 setAttribute sends JSON.stringify(value) instead of the well-formed JSON transport this package already uses. A lone surrogate is rejected by iOS NSJSONSerialization, the setter's boolean is ignored, and JS still caches the attribute.
3. P2 The new span registry is mutated on the span call path with no lock, while invalidate() clears it from another queue (iOS) / another thread (Android). This module already documents that overlap for CreatedReports.
4. P2 Android startTransaction / startSpan / spanStartChild NPE if the SDK returns null. iOS adoptSpan: already returns an empty handle for nil; getActiveSpan is the only Android path that does.

Overall risk: Medium

Merge recommendation

Request changes. The APM surface is new and the leak/teardown issues sit on the lifetime contract this PR is supposed to pin. None of these look like an immediate crash on the happy path the device tests covered (APM on, finish after setters, no reload), but 1 and 3 are real failure modes around APM-off / no-op spans and RN instance teardown.

Most important to fix

1. Always drop the handle finish was called with, then also drop any other retained span that is now finished. JS finish() must not throw on the first call.
2. Put span-attribute values through the same well-formed JSON path as notify fields / startTransaction attributes, and do not update the JS cache when native returns false.
3. Serialize SpanHandles / BGSRNSpanHandles against invalidate() the way CreatedReports already does (iOS: main-queue releaseAll, or a lock; Android: synchronize the registry). Do not lazily recreate the iOS registry after _spanHandles = nil.

What looks solid

• Boolean setter returns, with a spec-source lock and a device check that the child attribute actually reached the capture.
• Handle identity reuse so getActiveSpan() returns the same JS object.
• notify validation (empty title, field types, extra args, urgent) matches the rest of the facade.
• Native unit tests pin "a finished span leaves the registry" for the Fake that does set isFinished.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Re-review: 1547bf2 addresses the prior P2s

Checked 1547bf2 against the four findings from the last pass. The author’s thread replies match the code.

Finding	Verdict
Native finish only dropped isFinished() entries	Fixed. The called handle is removed first (releaseFinished(called) / dropCalled:), then other finished spans are swept. aSpanThatStaysUnfinishedIsStillReleased / testASpanThatStaysUnfinishedIsStillReleased pin a no-op that leaves the flag down.
JS first finish() threw E_SPAN_HANDLE_DEAD	Fixed. First call marks dead and returns; a later call is what throws (the first finish returns even when native omits the handle).
setAttribute used raw JSON.stringify and cached on failure	Fixed. Values go through encodeBridgeJson; the JS cache updates only when native returns true (a lone surrogate is well-formed, and a refused attribute is not cached).
Registry vs invalidate()	Fixed. Android methods are synchronized; iOS uses os_unfair_lock, _spansRetired, and closedRegistry so the getter cannot allocate a live map after teardown (testAClosedRegistryAdoptsNothing).
Android NPE on a null SDK span	Fixed. startTransaction / startSpan / spanStartChild return noSpan(); retain(null, …) is "" (aNullSpanIsNotRetained).

No new high-signal defects in this delta. I did not run the unit or device suites in this environment.

Overall risk: Low

Merge recommendation

Approve. The lifetime, JSON, teardown, and null-span holes from the last review are closed. Remaining work is ordinary (device confirmation of the no-op/finish path if you want it, iPhone XS still unrun per the PR body).

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Deep code review: Phase 10 notify and APM spans (re-review)

Re-reviewed 1547bf2 (prior review was 88b23c7). The follow-up commit matches the five P2s: native finish always drops the handle it was called with (even when isFinished stays false), JS finish() returns on that first call, setAttribute uses encodeBridgeJson and updates the cache only when native returns true, both registries serialize against invalidate (iOS closedRegistry after retire; Android synchronized), and a null SDK span is noSpan() rather than an NPE. I checked those paths against the JS facade, both native registries, invalidate, and the unit tests (Sticky / BGSRNStickySpan, closed registry, null retain, omitted-handle finish). I did not run the unit or device suites in this environment. GitHub CI on this head: Android, iOS unit, and iOS simulator e2e already green; lint, typecheck, unit, mutation was still pending at review time.

Findings

No remaining P0–P3. The five prior P2s are fully addressed.

Overall risk: Low

Merge recommendation

Safe once CI is green on 1547bf2.

Most important to fix

Nothing blocking. Residual, not a finding: iPhone XS (KRSFT) was still running BareExample, so that handset was not run; Android/iOS simulator device cases for the review fix are recorded in the plan.

What looks solid

• Boolean setter returns (spec-source lock) plus the device check that a child setAttribute actually landed in the capture.
• Handle identity reuse so getActiveSpan() is the same JS object.
• notify validation (empty title, field types, extra args, urgent) matches the rest of the facade; omitted severity still crosses as 0 (Android null / iOS unset).
• Native tests now cover the no-op that leaves isFinished false, and JS covers the first finish when native returns [].

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Stale comment

Re-review: 670f63c (mutation coverage)

Checked 670f63c against 1547bf2. The delta is the Stryker break from the previous CI run (92.83% < 95%): tighter tests in span.test.ts / notify.test.ts, plus equivalent simplifications in span.ts so redundant branches are no longer mutants.

Production behavior is unchanged from the last pass:

• assertText still rejects empty/whitespace via trim().
• Number.isFinite without typeof is the same finite-number gate.
• parseAttributes still yields an empty map for bad JSON, null, and arrays (Object.entries on null hits the existing catch).
• First finish() still marks dead even when native returns []; a later call is what throws.
• Native registries, encodeBridgeJson, and noSpan() from 1547bf2 are untouched.

The five prior P2s remain fully addressed. No new P0–P3. I did not run Jest or Stryker in this environment (no workspace install); GitHub CI lint, typecheck, unit, mutation was still in progress on this head at review time (run 37079165154).

Findings

No remaining P0–P3.

Overall risk: Low

Merge recommendation

Approve. Safe once CI is green on 670f63c, in particular the mutation job that failed on 1547bf2.

Most important to fix

Nothing blocking. Residual, not a finding: wait for lint, typecheck, unit, mutation on this head; iPhone XS was still unrun in the PR body.

What looks solid

• Tests now pin the exact error strings, omitted vs empty notify fields, dead-handle reuse of a handle id, getActiveSpan identity, and the no-span / missing-child paths Stryker had been surviving.
• The span.ts edits delete only checks that a later condition already covered.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

Deep code review: Phase 10 notify and APM spans (re-review)

Re-reviewed 670f63c (prior review was 1547bf2). This head is the mutation-coverage follow-up after CI lint, typecheck, unit, mutation failed on 1547bf2 at 92.83 (span.ts 65.23, notify.ts 74.29; threshold 95). The production contract is the same one that closed the five P2s: native finish always drops the called handle (even when isFinished stays false), JS finish() returns on that first call, setAttribute uses encodeBridgeJson and caches only when native returns true, both registries serialize against invalidate (iOS closedRegistry after retire; Android synchronized), and a null SDK span is noSpan() rather than an NPE.

I re-read the JS facade, both native registries, invalidate, the TurboModule spec, the JSON transport, and the unit/e2e coverage. The 670f63c edits in span.ts (drop typeof number next to Number.isFinite, let parseAttributes treat JSON null via Object.entries throwing, stop defaulting attributesJson to '{}', always markDead on the first finish) are equivalent to the previously reviewed behavior, not a new lifetime hole. I did not run the unit or device suites in this environment (node_modules is not installed here). GitHub CI on this head: ios unit, ios (spm), and RN compat already green; android, iOS cocoapods, iOS simulator e2e, and mutate were still pending at review time.

Findings

No remaining P0–P3. The five prior P2s are still fully addressed. The new tests pin the paths mutate actually killed (finish when native returns [] then reuses the handle, refused attributes, missing child, dead-handle later calls, wire-field drops).

Overall risk: Low

Merge recommendation

Safe once CI is green on 670f63c, especially mutate (that is the job this commit exists to clear).

Most important to fix

Nothing blocking. Residual, not a finding: iPhone XS (KRSFT) was still running BareExample, so that handset was not run.

What looks solid

• Boolean setter returns (spec-source lock) plus the device check that a child setAttribute actually landed in the capture.
• Handle identity reuse so getActiveSpan() is the same JS object; a later native start with a released handle is a new JS object.
• notify validation (empty title, field types, extra args, urgent) matches the rest of the facade; omitted severity still crosses as 0 (Android null / iOS unset).
• Native tests still cover the no-op that leaves isFinished false, and JS now covers the first finish when native returns [] plus the mutation-killed branches around wire parsing and dead handles.

  

_{Sent by Cursor Automation: Bugsee code review}