Skip to content

fix(observability): inject TRA support-file listener synchronously (SDK-7121) - #1160

Open
osho-20 wants to merge 2 commits into
masterfrom
fix/sdk-7121-tra-support-file-race
Open

fix(observability): inject TRA support-file listener synchronously (SDK-7121)#1160
osho-20 wants to merge 2 commits into
masterfrom
fix/sdk-7121-tra-support-file-race

Conversation

@osho-20

@osho-20 osho-20 commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

The new Automate dashboard (TRA / Test Observability) received no test events for Cypress runs on a customer's DEV pipeline, while the old Automate dashboard worked and the same suite/CLI/packages worked on another environment. Ref: SDK-7121.

Root cause

setEventListeners (bin/testObservability/helper/helper.js) injects the observability plugin into the user's Cypress support file:

require('browserstack-cypress-cli/bin/testObservability/cypress');

It did this inside an async glob(pattern, {}, cb) callback and returned immediately without awaiting it. In bin/commands/runs.js, the caller proceeds synchronously to md5 hashing (checkUploadedMd5) and zip archiving (archiver.archive) right after the call. So the injection raced the archive:

  • Race won (fast FS / UAT): instrumented suite is zipped → TRA works.
  • Race lost (slower CI / customer DEV): the archive reads the un-instrumented support file → remote runner has no o11y plugin → zero TRA events.

Md5 caching makes a lost race sticky: once a non-instrumented zip is cached under the original-content md5, every subsequent run logs "Skipping zip upload since BrowserStack already has your test suite..." and reuses the bad zip. The old Automate dashboard is unaffected because it does not depend on the injected plugin — exactly the reported asymmetry.

Reproduction

Calling setEventListeners and reading the support file synchronously afterwards (as md5/archive do):

before fix:  injected SYNCHRONOUSLY? false   (archive can ship un-instrumented suite)
after  fix:  injected SYNCHRONOUSLY? true

Fix

  • setEventListeners → use glob.sync so the support-file writes complete before the function returns.
  • Applied the same fix to the identical race in setAccessibilityEventListeners' glob-pattern branch (bin/accessibility-automation/helper.js).

Testing

  • New regression test test/unit/bin/testObservability/setEventListeners.js — asserts the observability require is present synchronously after the call. Fails on unfixed code, passes with the fix.
  • Existing accessibility-automation/cypress unit tests: green (6/6).
  • commands/runs.js unit file shows the same pre-existing standalone failures with and without this change (unrelated setUsageReportingFlag assertions).

Release

  • patch — bug fix
  • Release note: Fix Cypress Test Observability (new Automate dashboard) receiving no test events on some CI pipelines due to a race between support-file instrumentation and test-suite archiving/caching.

🤖 Generated with Claude Code

…DK-7121)

setEventListeners deferred the support-file write to an async glob callback
while runs.js proceeded synchronously to md5 hashing and zip archiving. The
injection raced the archive: a lost race shipped an un-instrumented suite, and
md5 caching ("Skipping zip upload...") made the bad zip sticky, so the new
Automate dashboard (TRA) received zero test events while the old dashboard
(independent of the injected plugin) kept working. This is why the symptom was
environment-specific — a slower CI pipeline loses the race.

Switch setEventListeners (and the identical race in the accessibility
setAccessibilityEventListeners glob branch) to glob.sync so the writes complete
before the caller reads the files. Adds a regression test asserting the
observability require is present synchronously after the call returns.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@osho-20

osho-20 commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator Author

⚠️ Needs human review

File Status Reason
bin/accessibility-automation/helper.js ✅ All Clear Covered by all lenses, no issues found
bin/testObservability/helper/helper.js ✅ All Clear Covered by all lenses, no issues found
test/unit/bin/testObservability/setEventListeners.js 🔴 Author to Fix 1 1 ungrounded test-isolation finding — verify independently

↻ 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

Address SDK PR Review coverage-gap finding on the regression test. The fix
touched two files (setEventListeners and the accessibility
setAccessibilityEventListeners glob branch) but the test only covered the
observability path. Add:
- accessibility setAccessibilityEventListeners synchronous-injection test
  (exercises the glob.sync pattern branch),
- observability idempotency test (a second call does not double-inject).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@pri-gadhiya pri-gadhiya left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: Approve. Root cause is correctly diagnosed, the fix is minimal and correct, and it ships with a genuine regression guard. I verified the load-bearing claims against the source rather than the description.

Confirmed

  • The race is real. runs.js (216/227/230) calls the void-returning listeners, then flows straight into checkUploadedMd5archiver.archive (263, 276) with no await in between. The old async glob(...cb) fired after the archive had already read the file.
  • glob.sync is the right APIglob is pinned ^7.2.0; glob.sync(pattern, opts) is a valid v7 signature.
  • Only runs.js consumes these functions — nothing depended on the async behavior.
  • a11y fix is correctly scopedsetAccessibilityEventListeners' non-magic branch was already synchronous; only the magic-pattern branch was async, and that's exactly what's fixed. No residual race.
  • No other async-glob-then-archive races remain — the only other callback-glob usages are the archive stream itself and the md5 walk.
  • Error-handling parity holdsglob.sync throws instead of if(err) return, but the outer try/catch catches and debug-logs it: equivalent outcome.
  • Test wiring is sound — both helpers resolve getSupportFiles from the same bin/helpers/helper module, so the single sinon stub covers both paths; asserted require strings match the injectors exactly.

Worth adding (positive)

  • Self-healing cache. checkSpecsMd5 hashes the whole cypress folder, which includes the support file. Post-fix the file is always instrumented → different md5 → the stale un-instrumented zip is no longer a cache hit → fresh upload. Affected customers recover on their first run with the fixed CLI, no manual cache bust. Worth noting in the release note.

Non-blocking nits

  • Idempotency is asserted only for the o11y path; the a11y injector has the same !includes(...) guard, so adding a symmetric assertion would be nice-to-have.
  • test/unit/bin/testObservability/setEventListeners.js also houses the a11y test — fine as an SDK-7121 bundle, but the path implies o11y-only.
  • Please confirm CI is green before merge (I couldn't run the suite locally). The patch bump is correct.

if(err) {
logger.debug('EXCEPTION IN BUILD START EVENT : Unable to parse cypress support files');
return;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are removing this, how do we know if exception caused issue in build start?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants