fix(o11y): batch the event flush so command-heavy specs stop hitting spec_timeout [SDK-7399]

[shivam5643]

What is this about?

Customers on 1.35.x / 1.36.x see a large share of their tests reported as skipped; the same suite on 1.32.8 is clean (SDK-7399: ~113 of ~525 skipped on 1.36.18 vs 4 on 1.32.8).

The tests are not skipped by mocha and nothing throws — the spec is killed at spec_timeout and every test that has not run yet is reported as skipped.

build-info on a reproducing build:

test_status:   { failed: 0, success: 6, queued: 0, ignored: 27, pending: 0 }
spec_timeout:  900000 ms          duration: 993 s
sessions:      status=error, 980 / 984 / 986 s

failed: 0 is the tell. A thrown error always marks a test failing; nothing failed here. The sessions ran to ~985s against a 900s spec_timeout and were killed.

Why they run that long. The queue flush issues one cy.task per queued event, and each cy.task round-trip costs roughly 0.8s on a remote terminal. Measured there, 6 tests per spec, N events per afterEach:

events per afterEach	cy.task calls	session duration
1	6	113s
10	60	114s
100	600	581s
1000	6000	killed at spec_timeout

A command-heavy test queues hundreds of events (command:start + command:end + platform_details per command, plus log:changed), so its afterEach alone runs for minutes. Load-dependent by construction: heavy tests exhaust the spec budget, light tests do not — which is exactly the reported 45-passed / 113-skipped shape rather than all-or-nothing.

Why 1.32.8 is unaffected. It dispatches via cy.now('task', ...), which throws immediately on Cypress 14 with no IPC round-trip. 1.32.8 is fast because its dispatch does nothing — it also delivers no browser-side telemetry there. v1.33.0 moved dispatch into the internal hooks and onto cy.task, which genuinely delivers but costs ~0.8s per event.

Related Jira task/s

• SDK-7399

Dependent PRs / release order

• Dependent PRs: None — CLI-only, no paired change in railsApp / realMobile / binary.
• No cross-repo deploy order applies.

Automation cases to add

• Cypress case asserting a command-heavy spec completes well inside spec_timeout with expected_skipped_number: 0. Existing cypress_cli_and_dashboard.feature rows already assert skipped counts and cover the success path.

Code changes to check

• Spread operator is not used.
• Syntax supported by older Node — no ?. / ??.

The change

plugin/index.js — new test_observability_batch task takes an array and fans out to the same IPC events. The four individual tasks stay registered for backward compatibility.

cypress/index.js — the flush builds one batch per drain instead of one cy.task per event, split at 512KB so each call stays under the ~1MB per-payload ceiling (also measured on the remote: 768KB passes, 1MB fails). sanitizeForTask caps payloads at 128KB with 8KB string truncation. shouldSkipCommand filters test_observability_batch, without which each batch dispatch would itself be captured as a command event and refill the queue.

Batching was verified before the code was written: the same 600 events sent as one cy.task call completed in 109s versus 581s as 600 calls — i.e. back to the ~110s baseline for that spec shape.

Verification

Fix, 3-spec repro (suite modelled on the customer's shape: cy.session per test, per-test retry overrides, fixture chains in before()). Same suite and settings both arms; only the CLI dependency differs. Patched arm confirmed to ship a fresh dependency bundle, not a cached one.

CLI	spec verdicts	tests run	duration	build
unpatched 1.36.9	all 3 passed_with_skipped	6 success / 27 ignored	993s	automate
this change	all 3 passed	33 success / 0 ignored	213s	automate · o11y

Telemetry still delivers — checked explicitly, because a passing spec cannot distinguish delivered from dropped. test_observability_batch accepts a mixed batch of all four event types, a 600-event batch, and a batch containing an unknown task name, all without failing (3/3).

Release

Version bump:

• minor (backwards-compatible feature)
• patch (bug fix or other small change)

Release notes type:

• Bug Fix

Release notes (customer-facing):

• Fixed tests being incorrectly reported as skipped when Test Observability is enabled. Observability data is now sent in batches, so specs no longer run past their spec timeout on command-heavy tests.

Release notes (internal):

• bin/testObservability/cypress/index.js + bin/testObservability/plugin/index.js: the beforeEach/afterEach event-queue flush now sends one batched cy.task per drain (new test_observability_batch task) instead of one cy.task per event. Each round-trip costs ~0.8s on a remote terminal, so command-heavy tests were spending minutes in the hook and the spec was killed at spec_timeout, reporting unrun tests as skipped — the SDK-7399 symptom. Introduced in v1.33.0 when dispatch moved into these hooks and onto cy.task.
• Batches are split at 512KB and per-event payloads capped at 128KB (8KB string truncation) to stay under the ~1MB per-cy.task ceiling. Skipped events are logged rather than dropped silently.

Checklist

• Ready to review
• Has it been approved by a member of your team?
• Verified end-to-end on BrowserStack infra (build links above)
• Telemetry delivery verified separately from the pass/fail result
• Required automation cases noted in description

Reviewer notes

Environment gotchas found while verifying, worth knowing for any future o11y change:

• A CLI branch must resolve on both the local machine and the remote Windows terminal. A git-branch dependency works (repo is public); a local file: path or a package_config_options.scripts.postinstall pointing at a local path does not — run_settings ships verbatim to the remote (capabilityHelper.js:131), so the remote re-runs it and fails with NPM_INSTALL_FAILED.
• BrowserStack caches the dependency bundle by the generated package.json md5. Re-testing a changed patch under an unchanged dependency string silently reuses the old bundle ("Skipping the upload of node_modules…"). Add a bare cacheBust key to force a fresh upload.
• browserstack-cypress build-info <buildId> is the reliable way to see test_status / spec_timeout / session durations. The o11y ext API returned nulls for these builds and the session-logs URL returns a bot-challenge page.

Earlier commits on this branch explored two approaches that were verified and rejected: a reporter-side duplicate-event guard (no effect on a remote A/B), and switching dispatch to cy.now (stopped the skipping only by never delivering anything, since cy.now('task') throws on Cypress 14 in every context). Both are superseded by this change; history retained deliberately.

🤖 Generated with Claude Code

[shivam5643]

PR Review: browserstack-cypress-cli PR #1177

Summary

Intent: Add a real try/catch + .catch() error boundary around the test-observability event flush in bin/testObservability/cypress/index.js, and switch the dispatch call from cy.task to cy.now('task', ...), so that an instrumentation failure inside the beforeEach/afterEach mocha hooks can no longer throw and make mocha skip every remaining test in the spec (SDK-7399).
Risk: Medium
1 critical · 0 warnings · 0 suggestions | Files reviewed: 1

═══════════════════════════════════════════════════════════════

Findings

Populated mechanically from unified-findings.json — the union of every unit reviewer's output, never hand-merged.

Two channels. Blocking = Critical + Warning — the must-fix set the Verdict gates on. Non-blocking = Suggestions — polish; never gates. An empty Warning/Suggestion section here means examined and clean, not an incomplete review.

🔴 Critical (Blocking)

#	Finding	File · Symbol	Confidence
1	[Graceful Degradation] All three swallow points in flushEventsQueue drop errors with no diagnostic signal at all	bin/testObservability/cypress/index.js · flushEventsQueue	🟢

1. [Graceful Degradation] All three swallow points in flushEventsQueue drop errors with no diagnostic signal at all — bin/testObservability/cypress/index.js · flushEventsQueue

Problem:
flushEventsQueue() has three places that intentionally swallow an error to protect the customer's run: the async .catch(() => {}) on the cy.now promise, the per-event catch (e), and the outer function-level catch (e) that resets the whole queue. All three are correctly scoped — each isolates one bad event / one bad flush from the rest, which is exactly what this PR needs to satisfy the graceful-degradation contract — but none of them leaves any trace that an event was dropped. Not even a console.warn.

This PR exists specifically because a prior silent-failure mode (a thrown instrumentation error making mocha skip every remaining test) went undiagnosed for a full release cycle before a customer noticed. The fix as written trades that loud failure for a new, completely silent one: an observability event can now fail to reach the dashboard with zero signal anywhere that it happened. The next time this recurs — a different cy.now('task', ...) failure mode, a malformed payload, whatever — nobody will know until someone notices missing data in TestHub/O11y, which is a much harder thing to notice than a customer's suite breaking.

const result = cy.now('task', event.task, payload, event.options);
if (result && typeof result.catch === 'function') result.catch(() => {});
} catch (e) {
  /* one bad event must not stop the remaining events, and must not fail the hook */
}

Suggested Fix:
Add a bare console.warn/console.error inside each of the three catches, e.g. console.warn('[browserstack] failed to flush test-observability event', e). Do not route the failure through another Cypress command (cy.task/cy.now) — that risks reintroducing the exact enqueue-time failure this PR is fixing. A plain console.* call keeps the guarantee that instrumentation can never fail the hook, while leaving a log line an engineer can grep for the next time events go missing.

Confidence: 🟢 — grounded against the default.md graceful-degradation rule (an error boundary must still surface the failure somewhere — debug log, telemetry, or status — its exemption is only for a catch that does log at debug) and verified objectively against the diff: none of the three catch bodies contains any logging call, only code comments.

───────────────────────────────────────────────────────────────

🟠 Warnings (Blocking)

None found — the region-by-region walk of both hook call sites (beforeEach, afterEach) surfaced no other defect.

💡 Suggestions (Non-blocking)

None.

═══════════════════════════════════════════════════════════════

External Services

No external-contract changes detected. (No finding in this PR carries contract_change: true; the change is entirely internal to the CLI's Cypress-hook flush path and does not alter any request/response shape, endpoint, or wire protocol.)

═══════════════════════════════════════════════════════════════

Per-File Confidence (for reviewers)

File	Status	Reason
bin/testObservability/cypress/index.js	🔴 Author to Fix 1	1 grounded (kb-high) finding — swallowed errors with no diagnostic signal

═══════════════════════════════════════════════════════════════

What's Good

• Both beforeEach and afterEach flush sites are updated identically to call the same flushEventsQueue() helper — the fix isn't applied asymmetrically to only one of the two hooks.
• The per-event try/catch inside flushEventsQueue isolates one bad event so the rest of the queued batch still flushes, and the outer try/catch stops a catastrophic failure from ever reaching the mocha hook — the core "instrumentation must never fail the hook" contract is satisfied on every path.
• The switch from cy.task to cy.now('task', ...) is deliberate and well-documented in the PR body: cy.task enqueues a command that drains later (still inside the hook's failure window), while cy.now executes immediately and returns a promise that can actually be caught here — this is the crux of the fix, not a cosmetic change.

═══════════════════════════════════════════════════════════════

Verdict

🔴 Fix 1 blocking issue — flushEventsQueue's three swallow points drop instrumentation errors with no diagnostic signal at all (Critical, grounded).

Coverage: 3 of 3 regions judged, 0 unjudged. No coverage gap.

═══════════════════════════════════════════════════════════════

— SDK PR Review Agent

[shivam5643]

Fixed in 82cf3f9 — valid finding, the guard was silent.

All three suppression points now log via warnFlushFailure(stage, err), naming the stage and failing task. Used console.warn as suggested (not cy.task — that would reintroduce the contained failure); matches the existing convention in accessibility-automation/cypress/index.js.

Guarantee re-verified after the change: 11-test spec with a failing dispatch in afterEach → 11 passing, 0 skipped.

Also trimmed the code comments in 937f2d9 (33 → 12 lines), no logic change.

[shivam5643]

✅ Good to go

File	Status	Reason
bin/testObservability/cypress/index.js	✅ All Clear	Covered — all 3 regions judged clean; 1 non-blocking Suggestion noted separately, does not affect this status

Change map (generated deterministically from the diff)

graph LR
  subgraph ncypress_cli["cypress-cli"]
    nbin_testObservability_cypress_index_js["index.js<br/>~57 lines"]
  end

Loading

↻ This verdict comment is the review anchor — it's updated in place on each run (the gate posts its status separately).

— SDK PR Review Agent

[shivam5643]

⚠️ Please hold this PR — do not merge yet. Further verification found a problem with my own change.

What I found. The fix swaps cy.task(...) → cy.now('task', ...). On a real remote Windows 11 / Chrome 136 machine, cy.now('task', ...) throws for every task, in every context (test body, mocha hook, and Cypress.on listener), while cy.task(...) delivers correctly:

dispatch	remote result
cy.task(...)	✅ delivers
cy.now('task', ...) (this PR)	❌ throws — Cannot read properties of undefined (reading 'get') from runPrivilegedCommand

So this PR stops the skipping by never dispatching anything. Browser-side observability events (test_observability_command / _log / _step / _platform_details) would silently stop reaching the dashboard. My earlier green builds could not tell "contained a genuine error" from "sent nothing" — both produce a passing run.

Side note that reframes the baseline: 1.32.8 dispatches only via cy.now, so on Cypress 14 it delivers no browser-side telemetry either — its throws land in listeners where they are harmless. So this PR is effectively 1.32.8 parity: no skipping, no browser-side telemetry. That is a product trade-off, not just a technical one, since customers on 1.36.x get that telemetry today.

Two other approaches tested and rejected:

• Error boundary only, keep cy.task — does not fix it (1 failing + 10 skipped). cy.task enqueues; the failure lands during queue drain, outside the try/catch.
• Keep cy.task + scoped cy.on('fail') filter — dangerous. Looked ideal (11 passing, 0 skipped), but a safety check with the customer's own failing assertion in afterEach reported 3 passing / 0 failing — our failing cy.task aborts the hook's command queue, so their real assertion never ran and the failure was hidden. Strictly worse than the original bug.

Still confirmed and unaffected by all this: the root cause (a throw inside the internal beforeEach/afterEach flush makes mocha skip every remaining test in the spec) and the regression window (v1.33.0 moved dispatch into those hooks and dropped the per-call .catch() guards).

Suggested direction. The reporter already runs an HTTP server on 127.0.0.1:REPORTER_API_PORT_NO with CORS enabled (reporter/index.js:267,295-307, used today by accessibility-automation/plugin/index.js). Posting queued events browser→node over that channel bypasses Cypress commands entirely, so a dispatch failure could never fail a mocha hook and delivery is preserved. That is a design change rather than a hotfix and needs a maintainer's call.

Happy to take direction on whether to pursue that here or open a fresh PR.

[shivam5643]

Hold lifted. The earlier cy.now approach is superseded — the diagnosis in this PR has been rewritten and the fix reverified.

Correction to my earlier comments: the bug is not a thrown error. build-info on a reproducing build reports failed: 0, success: 6, ignored: 27 with sessions at ~985s against a 900s spec_timeout. Nothing throws — the spec is killed at spec_timeout and every test not yet run is reported as skipped.

Cause: the flush issued one cy.task per queued event, and each round-trip costs ~0.8s on a remote terminal (measured: 600 calls → 581s; 6000 calls → killed). A command-heavy test queues hundreds of events, so its afterEach ran for minutes.

Fix: batch the flush into a single cy.task per drain. Verified before writing the code — the same 600 events as one call took 109s vs 581s as 600 calls, i.e. baseline.

Result on the 3-spec repro: passed_with_skipped / 6 of 33 tests run / 993s → passed / 33 of 33 / 213s.

Telemetry delivery was checked separately this time, since a passing spec cannot distinguish delivered from dropped: test_observability_batch accepts a mixed batch of all four event types, a 600-event batch, and a batch containing an unknown task name, all without failing.