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)). 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;