From 4b5de74e9782bdc170383450dd8d829bd3dd6560 Mon Sep 17 00:00:00 2001 From: Mike Hardy Date: Wed, 8 Jul 2026 18:07:40 -0500 Subject: [PATCH 1/2] fix(functions): thread-safe Android streaming listener lifecycle Synchronize access to the streaming listeners SparseArray and defer removal until explicit cancel or invalidate so final stream events are not dropped under concurrent unsubscribe. Add e2e coverage for native streaming errors and early stream cancel. --- .../functions/NativeRNFBTurboFunctions.java | 66 ++++++++++++------- packages/functions/e2e/functions.e2e.js | 52 +++++++++++++++ 2 files changed, 96 insertions(+), 22 deletions(-) diff --git a/packages/functions/android/src/main/java/io/invertase/firebase/functions/NativeRNFBTurboFunctions.java b/packages/functions/android/src/main/java/io/invertase/firebase/functions/NativeRNFBTurboFunctions.java index 6b0e7da981..5bb38b71be 100644 --- a/packages/functions/android/src/main/java/io/invertase/firebase/functions/NativeRNFBTurboFunctions.java +++ b/packages/functions/android/src/main/java/io/invertase/firebase/functions/NativeRNFBTurboFunctions.java @@ -53,6 +53,7 @@ public class NativeRNFBTurboFunctions extends NativeRNFBTurboFunctionsSpec { private static final String STREAMING_EVENT = "functions_streaming_event"; private static final SparseArray functionsStreamingListeners = new SparseArray<>(); + private static final Object functionsStreamingListenersLock = new Object(); private final TaskExecutorService executorService; public NativeRNFBTurboFunctions(ReactApplicationContext reactContext) { @@ -268,7 +269,9 @@ private void httpsCallableStreamSetup( @Override public void onSubscribe(Subscription s) { - functionsStreamingListeners.put(listenerId, s); + synchronized (functionsStreamingListenersLock) { + functionsStreamingListeners.put(listenerId, s); + } s.request(Long.MAX_VALUE); } @@ -287,7 +290,8 @@ public void onNext(StreamResponse streamResponse) { if (isFinalResult) { emitStreamEvent(appName, listenerId, responseData, true, false, null); - removeFunctionsStreamingListener(listenerId); + // Map-only cleanup: do not cancel here (avoids cancel race with emit). + unregisterFunctionsStreamingListener(listenerId); } else { emitStreamEvent(appName, listenerId, responseData, false, false, null); } @@ -297,33 +301,53 @@ public void onNext(StreamResponse streamResponse) { public void onError(Throwable t) { WritableMap errorMap = createErrorMap(t); emitStreamEvent(appName, listenerId, null, true, true, errorMap); - removeFunctionsStreamingListener(listenerId); + unregisterFunctionsStreamingListener(listenerId); } @Override public void onComplete() { - Object listener = functionsStreamingListeners.get(listenerId); - if (listener != null) { + boolean stillRegistered; + synchronized (functionsStreamingListenersLock) { + stillRegistered = functionsStreamingListeners.get(listenerId) != null; + if (stillRegistered) { + functionsStreamingListeners.remove(listenerId); + } + } + if (stillRegistered) { emitStreamEvent(appName, listenerId, null, true, false, null); - removeFunctionsStreamingListener(listenerId); } } }); } catch (Exception e) { WritableMap errorMap = createErrorMap(e); emitStreamEvent(appName, listenerId, null, true, true, errorMap); - removeFunctionsStreamingListener(listenerId); + unregisterFunctionsStreamingListener(listenerId); } }); } + /** + * Removes the listener from the map only. Does not cancel the subscription — used after terminal + * stream events so emit is not raced by cancel. Explicit {@link + * #removeFunctionsStreamingListener} still cancels and removes for the JS {@code + * removeFunctionsStreaming} API. + */ + private void unregisterFunctionsStreamingListener(int listenerId) { + synchronized (functionsStreamingListenersLock) { + functionsStreamingListeners.remove(listenerId); + } + } + + /** Cancels the subscription and removes from the map (explicit JS teardown). */ private void removeFunctionsStreamingListener(int listenerId) { - Object listener = functionsStreamingListeners.get(listenerId); - if (listener != null) { - if (listener instanceof Subscription) { - ((Subscription) listener).cancel(); + synchronized (functionsStreamingListenersLock) { + Object listener = functionsStreamingListeners.get(listenerId); + if (listener != null) { + if (listener instanceof Subscription) { + ((Subscription) listener).cancel(); + } + functionsStreamingListeners.remove(listenerId); } - functionsStreamingListeners.remove(listenerId); } } @@ -348,10 +372,6 @@ private void emitStreamEvent( new FirebaseFunctionsStreamHandler(STREAMING_EVENT, body, appName, listenerId); ReactNativeFirebaseEventEmitter.getSharedInstance().sendEvent(handler); - - if (done) { - removeFunctionsStreamingListener(listenerId); - } } private void handleFunctionsException(Exception exception, Promise promise) { @@ -435,14 +455,16 @@ public void invalidate() { super.invalidate(); // Cancel all active streaming listeners before shutdown - for (int i = 0; i < functionsStreamingListeners.size(); i++) { - int listenerId = functionsStreamingListeners.keyAt(i); - Object listener = functionsStreamingListeners.get(listenerId); - if (listener instanceof Subscription) { - ((Subscription) listener).cancel(); + synchronized (functionsStreamingListenersLock) { + for (int i = 0; i < functionsStreamingListeners.size(); i++) { + int listenerId = functionsStreamingListeners.keyAt(i); + Object listener = functionsStreamingListeners.get(listenerId); + if (listener instanceof Subscription) { + ((Subscription) listener).cancel(); + } } + functionsStreamingListeners.clear(); } - functionsStreamingListeners.clear(); executorService.shutdown(); } diff --git a/packages/functions/e2e/functions.e2e.js b/packages/functions/e2e/functions.e2e.js index 630fd014c6..093f9f2bc9 100644 --- a/packages/functions/e2e/functions.e2e.js +++ b/packages/functions/e2e/functions.e2e.js @@ -754,6 +754,58 @@ describe('functions() modular', function () { finalData.should.be.an.Object(); }); + it('should handle native streaming errors', async function () { + const { getApp } = modular; + const { getFunctions, httpsCallable } = functionsModular; + const functionRunner = httpsCallable( + getFunctions(getApp()), + 'testStreamWithError', + e2eCallableTimeoutOptions(), + ); + + const { stream, data } = await functionRunner.stream({ shouldError: true, errorAfter: 2 }); + let streamFailed = false; + try { + for await (const _chunk of stream) { + // drain until native onError propagates + } + } catch (e) { + streamFailed = true; + e.should.be.an.Error(); + } + streamFailed.should.equal(true); + try { + await data; + return Promise.reject(new Error('Expected data promise to reject')); + } catch (e) { + e.should.be.an.Error(); + } + }); + + it('should cancel streaming cleanly when iteration stops early', async function () { + const { getApp } = modular; + const { getFunctions, httpsCallable } = functionsModular; + const functionRunner = httpsCallable( + getFunctions(getApp()), + 'testStreamingCallable', + e2eCallableTimeoutOptions(), + ); + const { stream, data } = await functionRunner.stream({ count: 5, delay: 300 }); + const chunks = []; + for await (const chunk of stream) { + chunks.push(chunk); + if (chunks.length >= 2) { + break; + } + } + chunks.should.have.length(2); + // Generator `finally` removes the native listener; allow in-flight onComplete to finish. + await Promise.race([ + data.catch(() => undefined), + new Promise(resolve => setTimeout(resolve, 1500)), + ]); + }); + it('should work with multiple streams in parallel', async function () { const { getApp } = modular; const { getFunctions, httpsCallable } = functionsModular; From b04fb15d19369a4177d779cb4b7bad859cf9c41f Mon Sep 17 00:00:00 2001 From: Mike Hardy Date: Wed, 8 Jul 2026 18:49:55 -0500 Subject: [PATCH 2/2] docs(testing): tighten up coverage evidence requirements Add coverage_evidence_gate, mandatory Jacoco/lcov evidence package, NYC anti-pattern, and subagent return fields so native/lib bridge changes cannot close review without per-file coverage proof. --- okf-bundle/documentation-policy.md | 2 +- .../testing/change-authoring-workflow.md | 15 ++++---- okf-bundle/testing/coverage-design.md | 34 ++++++++++++++++++- okf-bundle/testing/iteration-vocabulary.md | 4 ++- okf-bundle/testing/validation-checklist.md | 4 +-- 5 files changed, 48 insertions(+), 11 deletions(-) diff --git a/okf-bundle/documentation-policy.md b/okf-bundle/documentation-policy.md index 88b65ca904..919204d553 100644 --- a/okf-bundle/documentation-policy.md +++ b/okf-bundle/documentation-policy.md @@ -44,7 +44,7 @@ Confirm: | Check | Requirement | |-------|-------------| -| **Canonical location** | Each topic has one owning doc; others link to it ([agent command policy](testing/agent-command-policy.md) for **all** agent shell commands; [change authoring](testing/change-authoring-workflow.md) for workflow/gates/frozen tree; [change authoring § validation evidence (blocking)](testing/change-authoring-workflow.md#validation-evidence-blocking); [running e2e](testing/running-e2e.md) for e2e `yarn tests:*` detail — [agent rule](testing/running-e2e.md#agent-rule-read-first); [platform coverage gate](testing/running-e2e.md#platform-coverage-gate-blocking); [iteration vocabulary](testing/iteration-vocabulary.md) for term ids only; [change authoring § quality standards](testing/change-authoring-workflow.md#quality-standards) for the review-findings resolution rule and the intractable-limitation bar; [compare-types README § justification bar](../.github/scripts/compare-types/README.md#justification-bar) for firebase-js-sdk type/API drift justification; [coverage design § expectations](testing/coverage-design.md#coverage-expectations-policy); [coverage design § evidence package](testing/coverage-design.md#coverage-evidence-package) for coverage completion evidence; [documentation site maintenance](documentation-site-maintenance.md) for `docs.json`, TypeDoc, and legacy redirect audits; this file for doc/commit policy, etc.) | +| **Canonical location** | Each topic has one owning doc; others link to it ([agent command policy](testing/agent-command-policy.md) for **all** agent shell commands; [change authoring](testing/change-authoring-workflow.md) for workflow/gates/frozen tree; [change authoring § validation evidence (blocking)](testing/change-authoring-workflow.md#validation-evidence-blocking); [running e2e](testing/running-e2e.md) for e2e `yarn tests:*` detail — [agent rule](testing/running-e2e.md#agent-rule-read-first); [platform coverage gate](testing/running-e2e.md#platform-coverage-gate-blocking); [iteration vocabulary](testing/iteration-vocabulary.md) for term ids and gate field names (`coverage_evidence_gate`); [change authoring § quality standards](testing/change-authoring-workflow.md#quality-standards) for the review-findings resolution rule and the intractable-limitation bar; [compare-types README § justification bar](../.github/scripts/compare-types/README.md#justification-bar) for firebase-js-sdk type/API drift justification; [coverage design § expectations](testing/coverage-design.md#coverage-expectations-policy); [coverage design § evidence package](testing/coverage-design.md#coverage-evidence-package) for coverage completion evidence and anti-patterns; [documentation site maintenance](documentation-site-maintenance.md) for `docs.json`, TypeDoc, and legacy redirect audits; this file for doc/commit policy, etc.) | | **DRY** | No duplicated procedures, policy paragraphs, or ephemeral snapshots outside work queues | | **Link hygiene** | Cross-links resolve; indexes list canonical entry points | | **Durability** | No ephemeral fields leaked into general reference docs | diff --git a/okf-bundle/testing/change-authoring-workflow.md b/okf-bundle/testing/change-authoring-workflow.md index 9dfc07a677..af5f6018c4 100644 --- a/okf-bundle/testing/change-authoring-workflow.md +++ b/okf-bundle/testing/change-authoring-workflow.md @@ -98,8 +98,9 @@ E2e scope, pre-flight, and harness gate: [running e2e § agent rule](running-e2e | Gate | Closes when | |------|-------------| | `implementation` | `implementation` work type complete — code plus **unit-focused**-tier checks green on **every required platform** when native bridge or embed path changed ([platform coverage gate](running-e2e.md#platform-coverage-gate-blocking)); [static analysis](validation-checklist.md#lint-and-formatting) green on the diff | -| `review` | `independent-review` complete — **area-focused**-tier checks green on frozen tree; applicable [validation checklist](validation-checklist.md) rows green (including static analysis); **every review finding resolved** ([§ quality standards](#quality-standards)) | -| `commit` | Durable commit exists for the item **after** prior gates closed with [recorded evidence](#validation-evidence-blocking) | +| `coverage_evidence` | [Coverage evidence package](coverage-design.md#coverage-evidence-package) attached with verdict line — **required** when frozen diff touches `packages/*/lib/**` or native bridge (`packages/*/{android,ios}/**`); otherwise `n/a` | +| `review` | `independent-review` complete — **area-focused**-tier checks green on frozen tree; applicable [validation checklist](validation-checklist.md) rows green (including static analysis); **`coverage_evidence` gate closed** when required; **every review finding resolved** ([§ quality standards](#quality-standards)) | +| `commit` | Durable commit exists for the item **after** `implementation`, `coverage_evidence` (when required), and `review` gates closed with [recorded evidence](#validation-evidence-blocking) | **Trust rule:** Code on disk or in git with `review` still **open** is unverified until `independent-review` closes the gate. @@ -114,7 +115,8 @@ Gates close **only** when **recorded evidence** shows the required validation ti | Gate | Minimum evidence (record in work-queue notes or review handoff) | |------|------------------------------------------------------------------| | **`implementation`** | Prepare/tsc/jest **exit codes**; when native or macOS runtime touched: **e2e pass count per required platform** + log path (e.g. `/tmp/rnfb-e2e-*.log`); **`yarn lint` exit code 0**; when `docs/**` changed: **`yarn lint:markdown`** + **`yarn lint:spellcheck` exit code 0** | -| **`review`** | Frozen-tree re-run of area-focused checklist; **`yarn lint` exit code 0**; when `docs/**` in frozen diff: **`yarn lint:markdown`** + **`yarn lint:spellcheck` exit code 0**; **coverage evidence package** when native or `packages/*/lib/**` bridge code touched ([coverage design § evidence package](coverage-design.md#coverage-evidence-package)); compare:types row for touched registered package | +| **`coverage_evidence`** | When lib/native bridge touched: full [coverage evidence package](coverage-design.md#coverage-evidence-package) at `.agents/reports//coverage-evidence.md` (or work-queue notes) including **verdict line**; post-process native artifacts after fresh e2e ([coverage design § reading per-file](coverage-design.md#reading-per-file-coverage-locally)) | +| **`review`** | Frozen-tree re-run of area-focused checklist; **`yarn lint` exit code 0**; when `docs/**` in frozen diff: **`yarn lint:markdown`** + **`yarn lint:spellcheck` exit code 0**; **`coverage_evidence` gate closed** when required; compare:types row for touched registered package | | **`commit`** | Prior gates closed **with evidence**; no `.only` / harness overrides staged | | **Publication** (`git push`, force-push, PR refresh) | **`review` gate closed on the exact commits being published**; evidence still valid (no product edits since last area-focused run) | @@ -124,7 +126,8 @@ Gates close **only** when **recorded evidence** shows the required validation ti ### Forbidden shortcuts -- **`git commit`** while the current work type's validation tier is incomplete or evidence is missing. +- **`git commit`** while the current work type's validation tier is incomplete or evidence is missing — including **`coverage_evidence` gate open** when lib/native bridge touched. +- **Jet NYC `text-summary` after narrowed e2e** as coverage proof for native or `packages/*/lib/**` changes — it measures remapped TS only; use Jacoco/lcov on **changed files** ([coverage design § anti-patterns](coverage-design.md#anti-patterns-not-coverage-evidence)). - **`git push` / force-push / PR update** claiming remediation or review-green **without** fresh area-focused evidence after the last product edit on the published commits. - **History rewrite** (rebase, amend stack) **without** re-running validation for the rewritten scope — prior green results are **invalid**. - **Self-accepted** parity or coverage gaps — only [acceptable exceptions](#acceptable-exceptions) with user confirmation or intractability evidence in durable OKF (e.g. parity registry row). @@ -235,8 +238,8 @@ On a **frozen tree**: 1. Revert all `.only`. 2. Keep area narrowing; run **area-focused**-tier e2e for loaded package spec(s) on [**every required platform**](running-e2e.md#platform-coverage-gate-blocking) (serial; pre-flight each run). 3. Run applicable [validation checklist](validation-checklist.md) rows — **blocking:** [static analysis § lint and formatting](validation-checklist.md#lint-and-formatting) (`yarn lint:js` on the frozen tree; markdown/spellcheck when docs touched); `yarn reference:api` when public surface changed. For packages registered in `compare:types`, `yarn compare:types` is a **blocking review gate**: the touched package must have zero undocumented or stale differences before `review_gate` closes. If the global command fails on unrelated registered packages, record/fix that drift in the work queue; do not treat an unrelated failure as permission to skip the touched package's type-parity check. -4. **Coverage evidence package** — **blocking** when `packages/*/lib/**` or native bridge sources in the frozen diff: produce and attach per [coverage design § evidence package](coverage-design.md#coverage-evidence-package); investigate every non-100% reachable line before closing `review`. -5. Outcome closes **review gate** or returns to **`implementation`**. +4. **Coverage evidence package** — **blocking** when `packages/*/lib/**` or native bridge sources in the frozen diff: after fresh area-focused e2e, run native post-process ([coverage design § reading per-file](coverage-design.md#reading-per-file-coverage-locally)); produce package per [coverage design § evidence package](coverage-design.md#coverage-evidence-package); close **`coverage_evidence` gate** with verdict line; investigate every non-100% reachable line. **Missing or stale package = serious finding** — same bar as missing e2e counts. +5. Outcome closes **`review` gate** (only after **`coverage_evidence` gate** when required) or returns to **`implementation`**. Keep **`implementation`** and **`independent-review`** in separate passes ([§ frozen tree](#frozen-tree)). diff --git a/okf-bundle/testing/coverage-design.md b/okf-bundle/testing/coverage-design.md index edc3d5b355..beac76516d 100644 --- a/okf-bundle/testing/coverage-design.md +++ b/okf-bundle/testing/coverage-design.md @@ -61,7 +61,39 @@ Produce after fresh e2e on every required platform, then post-process native art **Verdict line:** `100% on reachable touched lines` **or** `NOT 100%` with numbered gaps and disposition (fixed / intractable with evidence / user-accepted deferral). -Reviewers treat missing or stale coverage evidence as a **blocking** finding — same bar as missing e2e counts ([change authoring § validation evidence](change-authoring-workflow.md#validation-evidence-blocking)). +Record in `.agents/reports//coverage-evidence.md` (preferred) or work-queue notes. Orchestrators **must not** close `coverage_evidence_gate` or `review_gate` without this file when lib/native bridge is touched. + +Reviewers treat missing, stale, or NYC-only summaries as a **serious** finding ([change authoring § independent-review](change-authoring-workflow.md#independent-review)). + +### Subagent return fields (native / lib bridge touched) + +Blocking YAML (or equivalent table) before `coverage_evidence_gate` closes: + +```yaml +coverage_verdict: "100% on reachable touched lines" | "NOT 100%" +coverage_artifacts: + jacoco_xml: path # Android; use lcov path for iOS native + timestamp: ISO-8601 +touched_regions: + - file: packages/.../Foo.java + lines: "L10-L45" + line_pct: 100 +branch_map: + - branch: "onComplete when still registered" + test: "Functions.e2e.js — streaming cancel race" +gaps: [] # or numbered: disposition fixed | intractable+evidence | user-accepted deferral +``` + + + +### Anti-patterns (not coverage evidence) + +| Looks like coverage | Why it is not | +|---------------------|---------------| +| Jet NYC `text-summary` after narrowed `:test-cover` | Remapped TS aggregate for loaded modules — does not report Jacoco/lcov on native bridge lines | +| Whole-package or whole-harness statement % | Signal is **per changed file/region** in the frozen diff | +| Stale Jacoco XML / lcov without matching e2e run | Post-process deletes raw artifacts; re-run e2e first ([§ stale coverage](#stale-coverage-data)) | +| E2e pass counts alone | Proves behaviour, not line/branch coverage on touched native code | ## Platform parity (pipeline and bridge code) diff --git a/okf-bundle/testing/iteration-vocabulary.md b/okf-bundle/testing/iteration-vocabulary.md index 48d42d931c..f4faa6a0f9 100644 --- a/okf-bundle/testing/iteration-vocabulary.md +++ b/okf-bundle/testing/iteration-vocabulary.md @@ -51,12 +51,13 @@ Work queues use these **field names** (values: `open` | `closed`): | Field | Tracks | |-------|--------| | `implementation_gate` | `implementation` work type complete | +| `coverage_evidence_gate` | [Coverage evidence package](coverage-design.md#coverage-evidence-package) complete with verdict line when lib/native bridge touched; otherwise `n/a` (treat as closed) | | `review_gate` | `independent-review` work type complete | | `commit_gate` | Durable commit exists for the item **after** prior gates closed with [validation evidence](change-authoring-workflow.md#validation-evidence-blocking) | What closes each gate, trust rules, and loop transitions: [change authoring § gates](change-authoring-workflow.md#gates). -`commit_gate` closes when a durable commit exists whose subject matches the row's `commit_subject`, **after** `implementation_gate` and `review_gate` closed with [validation evidence](change-authoring-workflow.md#validation-evidence-blocking). +`commit_gate` closes when a durable commit exists whose subject matches the row's `commit_subject`, **after** `implementation_gate`, `coverage_evidence_gate` (when required), and `review_gate` closed with [validation evidence](change-authoring-workflow.md#validation-evidence-blocking). Items may also be marked **`blocked`** when a dependency gate is open elsewhere. @@ -70,6 +71,7 @@ Ephemeral work queues may record: | `validation_tier` | `unit-focused` \| `area-focused` \| `full` | | `platform` | Optional scope (e.g. `ios`) | | `implementation_gate` | `open` \| `closed` | +| `coverage_evidence_gate` | `open` \| `closed` \| `n/a` | | `review_gate` | `open` \| `closed` | | `commit_gate` | `open` \| `closed` | | `commit_subject` | Planned or landed **first line** of the item's focused commit (Conventional Commits subject). Set **before** `git commit`; must match the commit that closes `commit_gate`. Do not record SHAs. | diff --git a/okf-bundle/testing/validation-checklist.md b/okf-bundle/testing/validation-checklist.md index 553e6af2f7..8d1e49dc47 100644 --- a/okf-bundle/testing/validation-checklist.md +++ b/okf-bundle/testing/validation-checklist.md @@ -143,7 +143,7 @@ Before closing **`implementation_gate`**, **`review_gate`**, **`commit_gate`**, | lint (CI) | yarn lint | 0 | — | | lint:markdown (CI docs) | yarn lint:markdown | 0 | when docs/** in diff | | lint:spellcheck (CI docs) | yarn lint:spellcheck | 0 | when docs/** in diff | -| coverage | post-process + region table | — | see coverage-design § evidence package | +| coverage | post-process + region table | — | [coverage evidence package](coverage-design.md#coverage-evidence-package); closes `coverage_evidence_gate` when lib/native touched | ``` **History rewrite invalidates** prior rows — re-run and replace the table after amend/rebase. @@ -160,7 +160,7 @@ Before closing **`implementation_gate`**, **`review_gate`**, **`commit_gate`**, - [ ] `yarn lint` (CI Lint job); `yarn lint:markdown` + `yarn lint:spellcheck` when `docs/**` changed - [ ] E2e green on **every required platform** for the changed module ([platform coverage gate](running-e2e.md#platform-coverage-gate-blocking); [harness narrowing gate](running-e2e.md#harness-narrowing-gate-blocking); no `.only`; committed `RNFBDebug` remains `false`) - [ ] [Validation evidence package](validation-checklist.md#validation-evidence-package) recorded (exit codes, e2e counts, log paths) -- [ ] [Coverage evidence package](coverage-design.md#coverage-evidence-package) when lib/native bridge touched — gaps investigated to fix, delete, or acceptable-exception bar +- [ ] [Coverage evidence package](coverage-design.md#coverage-evidence-package) when lib/native bridge touched — `coverage_evidence_gate` closed with verdict line; gaps investigated to fix, delete, or acceptable-exception bar - [ ] OKF bundle reviewed/updated per § above Package workflows may add items (e.g. pipeline before/after snapshots — [pipeline workflow](../packages/firestore/pipeline-implementation-workflow.md)).