SDK-7138: add browser.uploadAttachment / uploadMedia to the WebdriverIO service - #193
harshit-browserstack wants to merge 11 commits into
Conversation
…a (SDK-7138) WebdriverIO had no way to attach a file to a test, hook or build in Test Reporting. Every sibling SDK ships one (BrowserStack.uploadAttachment in Java, driver.upload_attachment in Python, page.uploadAttachment in Node), and the binary's webdriverio language module already handles TEST_ATTACHMENT LogCreated entries end to end -- only the service-side entry point was missing, so driver.uploadMedia(...) threw "is not a function" and killed the customer's hook. UploadAttachmentModule registers the command the same way CustomTagsModule registers setCustomTags: on AutomationFrameworkState.CREATE / HookState.POST, instantiated from loadModules() when the testhub pipeline is up. It resolves the level (Test / Hook / Build) plus the uuid it hangs off, and emits one TEST_ATTACHMENT LogCreated entry. The file is not copied -- the binary streams it from filePath while draining its upload queue, which can outlive this process. grpcClient.logCreatedEvent was dropping fileName / fileSize / filePath on the floor even though the proto and generated types already carry them; without that the binary has nothing to stream. Also hardens CLI bootstrap against a degenerate bin-session response, observed on parallel workers alongside this bug: an empty config made JSON.parse throw in setConfig, and updateURLSForGRR then dereferenced the undefined config and threw out of loadModules. That aborted the entire bootstrap, so no module loaded -- custom tags, observability and the rest silently went away and the build recorded no test results. Both sites now degrade to defaults instead. Verified against the SDK-7138 reproduction (wdio_mocha upload-media/custom-tags spec, @wdio/browserstack-service built from main): uploadMedia and uploadAttachment both register, before-all/after-all hooks and the first test run clean where they previously died in "before all".
…138) uploadAttachment runs inside the customer's test body and awaited the binary's LogCreated ack with no bound, so a wedged binary would stall the calling test until the framework's own timeout fired. Race the ack against a 10s budget: the event is already on the wire when the timer wins, so nothing is dropped. Also re-arm the logCreatedEvent mock per test — afterEach's restoreAllMocks drops the implementation, so every test after the first was getting a non-promise back from the ack.
…er's stack (SDK-7138) uploadAttachment is called from the customer's test body and the next statement is usually a browser command that the accessibility module wraps with a pre-command scan. Awaiting the binary round-trip on that stack stalled the following executeAsync scan under load: chrome sessions issued the scan and then no further WebDriver request, until the framework timeout fired and the hub reaped the session (reproduced 4/4 in BStackAutomation at logLevel warn; absent 2/2 with the uploadMedia calls removed). The ack carries nothing the caller can act on — the binary streams the file from filePath while draining its own upload queue — so the event is written and its ack observed off-stack, still bounded so a wedged binary cannot leak a pending timer.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited), Workspace UI (inherited) Review profile: ASSERTIVE Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Duplicate changesets — one is truncated and would ship a broken release noteThis PR carries two changeset files describing the same two changes:
Both are Suggest deleting Worth noting the second bullet is doing real work and deserves to survive intact — "an empty or partial response aborted the whole bootstrap, which silently disabled every BrowserStack feature for that run — including custom tags and Test Reporting" is a meaningful robustness fix that a reader shouldn't have to infer from a duplicate. 🤖 Generated with Claude Code |
|
0 critical · 0 warnings · 0 suggestions — 2 changed files reviewed fresh this round (two passes each), 11 unchanged files carried from the prior clean review (e4dc5cd). The whole-PR cross-cutting pass found no cross-file defect. The pending state below is structural (the ledger's per-region coverage does not carry across a run boundary for unchanged files, so those files' regions read as not-(re-)judged-this-run) — not a code defect; see the full review for detail.
Process integrityThe review gate could NOT certify this run — the verdict is held at
Change map (generated deterministically from the diff)graph LR
subgraph nwdio_service["wdio-service"]
npackages_browserstack_service_tests_cli_modules_uploadAttachmentModule_test_ts["⚠ uploadAttachmentModule.test.ts<br/>~331 lines"]
npackages_browserstack_service_src_cli_modules_uploadAttachmentModule_ts["⚠ uploadAttachmentModule.ts<br/>~288 lines"]
npackages_browserstack_service_src_cli_apiUtils_ts["⚠ apiUtils.ts<br/>~62 lines"]
npackages_browserstack_service_tests_cli_apiUtils_test_ts["⚠ apiUtils.test.ts<br/>~62 lines"]
npackages_browserstack_service_src_cli_index_ts["⚠ index.ts<br/>~13 lines"]
npackages_browserstack_service_src_types_ts["types.ts<br/>~7 lines"]
n_changeset_pr_193_md["pr-193.md<br/>~6 lines"]
npackages_browserstack_service_src_constants_ts["constants.ts<br/>~5 lines"]
npackages_browserstack_service_src__types_bstack_service_types_d_ts["bstack-service-types.d.ts<br/>~4 lines"]
npackages_browserstack_service_src_index_ts["index.ts<br/>~4 lines"]
npackages_browserstack_service_src_service_ts["service.ts<br/>~3 lines"]
npackages_browserstack_service_src_launcher_ts["launcher.ts<br/>~2 lines"]
npackages_browserstack_service_src_cli_frameworks_constants_testFrameworkConstants_ts["testFrameworkConstants.ts<br/>~1 lines"]
end
↻ 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 |
- Drop the hand-written duplicate changeset; keep pr-193.md (the repo's generated file) with the complete bullet text. - updateURLSForGRR: debug-log when the bin-session config has no apis and which endpoints keep their defaults, instead of falling back silently. - uploadAttachmentModule tests: cover a path that exists but is not a regular file, and assert the ack-timeout warning actually fires. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
RUN_TESTS |
| private resolveTarget(instance: TestFrameworkInstance, options?: AttachmentOptions): { level: AttachmentLevel, uuid: string, testFrameworkState: string } | null { | ||
| const testFrameworkState = instance.getCurrentTestState().toString().split('.')[1] ?? '' | ||
| const inHook = CLIUtils.matchHookRegex(testFrameworkState) | ||
| const hook = inHook ? WdioMochaTestFramework.lastActiveHook(instance, WdioMochaTestFramework.KEY_HOOK_LAST_STARTED) : null |
There was a problem hiding this comment.
resolveTarget uses WdioMochaTestFramework.lastActiveHook for both runners. The test_hook_last_started key matches WdioCucumberTestFramework, but has hook-level attribution been verified end-to-end for Cucumber (e.g. an attachment from a Before hook)?
There was a problem hiding this comment.
Code-wise it's the same path: WdioCucumberTestFramework.KEY_HOOK_LAST_STARTED and WdioMochaTestFramework.KEY_HOOK_LAST_STARTED are both 'test_hook_last_started', and lastActiveHook is a plain state lookup on the tracked instance, not Mocha-specific logic. But it has not been verified end-to-end for Cucumber (e.g. an attachment from a Before hook). The E2E runs were wdio-mocha only. I'll call that out in the description rather than claim it.
| const observed = Promise.race([ | ||
| ack.then(() => 'ok', (error) => `failed: ${error}`), | ||
| new Promise<string>((resolve) => { | ||
| timer = setTimeout(() => resolve('unacked'), UPLOAD_ATTACHMENT_ACK_TIMEOUT_MS) |
There was a problem hiding this comment.
Minor: this timer isn't unref()'d, so if the binary is wedged (the case this timeout exists for), a call made at the end of the run keeps the worker alive for up to 10s. timer.unref() would avoid that.
There was a problem hiding this comment.
Done in e4dc5cd: the ack-timeout timer is now unref()'d, so a wedged binary can't hold the worker open for the 10s budget.
| @@ -0,0 +1,6 @@ | |||
| --- | |||
| "@wdio/browserstack-service": patch | |||
There was a problem hiding this comment.
This adds a new public API (browser.uploadAttachment / browser.uploadMedia), so per the PR template it should be a minor bump, not patch ("New features mislabeled as patch is the common mistake — when unsure, choose minor"). The release-notes type should also be New Feature rather than Bug Fix.
Fix: tick minor + New Feature in the Release section; the changeset regenerates from it.
|
|
||
| // Attachments ride a TEST_ATTACHMENT LogCreated event keyed on the test / | ||
| // hook uuid, so they are gated on the same pipeline. | ||
| this.modules[UploadAttachmentModule.MODULE_NAME] = new UploadAttachmentModule() |
There was a problem hiding this comment.
uploadAttachment / uploadMedia are only registered inside if (startBinResponse.testhub), and never on the classic (non-CLI) path (launcher.ts classicBuildStartAttempted). So whenever Test Reporting is off, build start fails, or the CLI isn't running, browser.uploadMedia(...) still throws TypeError: driver.uploadMedia is not a function — the exact symptom SDK-7138 reports — while the .d.ts declares the method unconditionally, so TS users get no warning.
The sibling SDKs never throw here: Python's upload_attachment and Java's BrowserStack.uploadAttachment are always callable and just no-op/log when reporting is inactive.
Fix: always attach a no-op stub (debug log) to the browser, and let UploadAttachmentModule overwrite it when testhub is up.
There was a problem hiding this comment.
Agreed, that was the SDK-7138 symptom surviving on every non-testhub path. Fixed in e4dc5cd: service.before() now calls UploadAttachmentModule.installNoopFallback(browser), which installs a no-op uploadAttachment/uploadMedia (debug log "Test Reporting is not active") whenever the browser doesn't already have one. That covers the classic path, no testhub, and CLI down. onBeforeExecute still replaces it with the real implementation when the binary is up, and an already-registered implementation is left alone. Unit-tested all three cases.
| return | ||
| } | ||
|
|
||
| this.sendAttachmentEvent(instance, resolvedPath, stats.size, target) |
There was a problem hiding this comment.
The file is not snapshotted at call time. The binary opens filePath only when it drains its upload queue (webdriverio/index.js fs.createReadStream(event.filePath)), and this send is now fire-and-forget. A common pattern — afterEach writes ./logs/browser.log, calls uploadMedia, and the next test overwrites or deletes it — attaches the wrong contents or silently loses the attachment (ENOENT). fileSize sent here can also disagree with what's actually streamed.
Python copies into ~/.browserstack/UploadedAttachments-<platformIndex>/<Level>/ (with a de-dup suffix) and cleans up post-run (sdk_cli/utils/file_uploader.py); Java does the same (Junit5Framework.java).
Fix: copy to a per-worker attachments dir before dispatching, send the copy's path, and clean the dir up at session end.
There was a problem hiding this comment.
Agreed. Fixed in e4dc5cd, mirroring Python's FileUploader. The file is copied into <CLIUtils.getWritableDir()>/UploadedAttachments-<platformIndex>/<level>/ (counter suffix on name collisions: media.txt, media1.txt, …) before dispatch, and the event carries the copy's path, so overwriting or deleting the source afterwards can't change or lose the attachment. The launcher removes the UploadedAttachments-<n> folders in onComplete, after the CLI/build stop (i.e. after the binary has drained its queue). fileSize comes from the same stat as the copy. Tests cover overwrite-after-call, same-name collisions, and that cleanup touches only those folders.
| * information the caller can act on: the binary streams the file from `filePath` while | ||
| * draining its own upload queue. | ||
| */ | ||
| private sendAttachmentEvent( |
There was a problem hiding this comment.
This rebuilds the same LogCreated payload that TestHubModule.sendLogCreatedEvent already builds (testHubModule.ts ~L361–400): platformIndex from WDIO_WORKER_ID, the executionContext triple, framework name/version/state, and hook-or-test uuid resolution.
Suggestion: extend sendLogCreatedEvent to pass through fileName / fileSize / filePath and return the promise; this path can then call it without awaiting and keep only the ack-timeout observation. One place to fix when the payload shape changes.
There was a problem hiding this comment.
Fair point on the duplication. I've kept it separate for now because sendLogCreatedEvent awaits and returns nothing, and changing its contract touches every log path in TestHubModule, which is broader than this PR. Happy to do it as a follow-up that extends sendLogCreatedEvent with fileName/fileSize/filePath and returns the promise.
| const trackedContext = instance.getContext() | ||
| const platformIndex = process.env.WDIO_WORKER_ID ? parseInt(process.env.WDIO_WORKER_ID.split('-')[0]) : 0 | ||
|
|
||
| const ack = GrpcClient.getInstance().logCreatedEvent({ |
There was a problem hiding this comment.
The E2E proof shows the TypeError is gone and tests/sessions are reported, but nothing shows an attachment actually landing in Test Reporting.
Question: could you add the O11Y attachment evidence (API listing or screenshot) for each level — TestLevel, HookLevel, and BuildLevel via { buildAttachment: true }?
Related (binary side, not this diff): the wdio uploadAttachmentEvents authenticates with session.testhub?.jwt directly, while the node/python modules use centralTokenService.fetch() which refreshes it — on a long build, wdio attachments could 401 after token expiry. Worth checking with a long-running suite.
There was a problem hiding this comment.
Fair ask. The BStackAutomation case test_upload_media_artifacts (browserstack/BStackAutomation#83493) asserts the attachments on the O11y test run, but its read-back API (api/v1/testRuns/v2/<id>/consolidatedLogs) returns 401 with local credentials, so that evidence has to come from the Jenkins run. I'll add it per level (Test/Hook/Build) once it's in. Noted on the binary-side session.testhub?.jwt vs centralTokenService.fetch() refresh; that's worth a long-running check and is outside this diff.
07souravkunda
left a comment
07souravkunda
left a comment
There was a problem hiding this comment.
Adds the missing command cleanly (mirrors CustomTagsModule), and the binary's webdriverio module handles Test/Hook/Build attachment levels as sent. Main asks are inline: the version bump (should be minor), registering a no-op fallback so uploadMedia never throws when Test Reporting is inactive, and snapshotting the file before the fire-and-forget send.
Before this goes into the nightly regression, please link a ticket for the Chrome + accessibility stall noted in the description (~40% of runs); "binary side" is still unproven. The PR description is also partly stale (it says the branch wasn't modified, lists a grpcClient fix that's already on main, and mentions sdk-7138-upload-attachment.md, which has been removed).
…f ack timer Review follow-ups: - Register a no-op uploadAttachment/uploadMedia on every browser in service.before(); UploadAttachmentModule replaces it when the binary is up. Without it the call still threw "uploadMedia is not a function" whenever Test Reporting was inactive (classic path, no testhub, CLI down) - the SDK-7138 symptom. Matches Python/Java, which no-op instead of throwing. - Snapshot the file into <writable>/UploadedAttachments-<platformIndex>/<level>/ (counter suffix on collisions) and send the copy's path, so a caller that overwrites or deletes the file right after the fire-and-forget call no longer attaches the wrong content or nothing. The launcher removes those folders in onComplete after the CLI/build stop, as the Python SDK does post-run. - unref() the ack-timeout timer so a wedged binary cannot hold the worker open. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
RUN_TESTS |
| const ext = path.extname(sourcePath) | ||
| const base = path.basename(sourcePath, ext) | ||
| let targetPath = path.join(targetDir, `${base}${ext}`) | ||
| for (let counter = 1; fs.existsSync(targetPath); counter++) { |
There was a problem hiding this comment.
The snapshot fixes the overwrite-after-call case, but this collision check introduces a race between parallel workers.
The folder is keyed only on platformIndex, so every worker running the same capability (WDIO_WORKER_ID 0-0, 0-1, …) shares UploadedAttachments-0/<level>/. existsSync followed by copyFileSync isn't atomic: two workers attaching a same-named file (e.g. browser.log, screenshot.png) at the same moment can both see browser.log as free, and the second copy overwrites the first. The first test's attachment then uploads the second test's content, which is the problem the snapshot is meant to prevent.
Also, Python and Java use the same ~/.browserstack/UploadedAttachments-<n>/ layout. Java scans HookLevel/ there, and Python's post-run cleanup deletes every UploadedAttachments-\d+. A concurrent Python or Java run on the same CI host could therefore pick up or delete these copies (and cleanupUploadedAttachments here can delete theirs).
Fix (either one):
- Key the folder on something unique per worker and SDK, e.g.
UploadedAttachments-wdio-${process.pid}/<level>/, and have cleanup match only that prefix. This removes both collisions. - Or copy with
fs.copyFileSync(src, dst, fs.constants.COPYFILE_EXCL)in the loop and retry the next counter onEEXIST. That makes the pick atomic, but doesn't fix the cross-SDK overlap.
There was a problem hiding this comment.
Fixed in a5fb1d8, using both of your options:
- Per-worker, per-run folder: snapshots now go to
UploadedAttachments-wdio-<bin session id>/<worker pid>/<level>/. Workers on the same capability never share a folder. The name is wdio-specific and per run (viaBROWSERSTACK_CLI_BIN_SESSION_ID, which every worker already inherits), so Python/Java'sUploadedAttachments-<n>/and other runs on the host are never touched. - Atomic name pick: the copy uses
COPYFILE_EXCLand moves to the next name onEEXIST. - Scoped cleanup:
cleanupUploadedAttachmentsremoves only this run's folder.
Tests cover a same-named file in another worker's folder, a writer that claims the name between the pick and the copy (this test fails without COPYFILE_EXCL), and cleanup leaving other runs' folders and the UploadedAttachments-<n> folders alone. In E2E, two workers wrote the same file names into …/66023/… and …/66025/…, with 0 file errors and nothing left behind after the run.
Snapshots were keyed only on platformIndex, so parallel workers on the same capability shared UploadedAttachments-<n>/<level>/. The name pick (existsSync, then copyFileSync) wasn't atomic, so two workers attaching a same-named file could overwrite each other. The Python and Java SDKs also use UploadedAttachments-<n>/ in the same directory, and each SDK's cleanup could delete the others' copies. Snapshots now go to UploadedAttachments-wdio-<bin session id>/<worker pid>/<level>/. The copy uses COPYFILE_EXCL and moves to the next name on EEXIST, and the launcher removes only this run's folder. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🟢 SDK PR Review gate is green — the SDK PR Review Agent has run on the current head commit (verdict: This gate confirms a review ran on the latest commit. The verdict itself is advisory — read the findings and use your judgement; it does not block merge. A native GitHub reviewer approval is still separately required by branch protection before this PR can merge. |
|
RUN_TESTS |
|
[SDK Wdio Test] TRA build state: passed | Stability 100% — verdict: success. Passed: 51, Failed: 0, Aggregate: 51. TRA: https://observability.browserstack.com/builds/setjmlybqmeg3pwiumd69yu4bcwmyyo9yz5aqanb |
07souravkunda
left a comment
07souravkunda
left a comment
There was a problem hiding this comment.
Approving at a5fb1d8: the no-op fallback, the per-worker/per-run attachment snapshot with COPYFILE_EXCL, and the scoped cleanup address the review, and the wdio CI run is green (51/51). Before merge, please switch the Release section to minor / New Feature (this adds a public API). Follow-ups: link tickets for the Chrome + accessibility stall and the sendLogCreatedEvent consolidation, and add the per-level attachment evidence from Jenkins.
4 participants
What is this about?
Adds
browser.uploadAttachment(filePath)(aliasedbrowser.uploadMedia) to theWebdriverIO service. The command did not exist at all — not in published
8.48.0, noton
main— sodriver.uploadMedia(...)threwTypeError: driver.uploadMedia is not a functionand killed the caller's hook. Every sibling SDK already ships one(
BrowserStack.uploadAttachmentin Java,driver.upload_attachmentin Python,page.uploadAttachmentin Node), and the binary'swebdriveriolanguage module alreadyhandles
TEST_ATTACHMENTLogCreatedentries end to end — only the service-side entrypoint was missing.
The original commits are by Shivam Kumar (branch pushed 2026-08-17). Review fixes sit on top of them: attachment snapshots, a never-throwing fallback, the ack-timer
unref, and per-worker snapshot folders.UploadAttachmentModuleregisters the command exactly asCustomTagsModuleregisterssetCustomTags— onAutomationFrameworkState.CREATE/HookState.POST, instantiatedfrom
loadModules()when the testhub pipeline is up. It resolves the level(Test / Hook / Build) plus the uuid it hangs off and emits one
TEST_ATTACHMENTLogCreatedentry.grpcClient.logCreatedEventwas droppingfileName/fileSize/filePath,even though the proto already carried them. That's fixed too.
The binary streams the file from
filePathonly when it drains its upload queue, so thefile is snapshotted first. Without that, a caller that overwrites or deletes it right after the call attaches the wrong
content or nothing. Snapshots go to
<writable dir>/UploadedAttachments-wdio-<bin session id>/<worker pid>/<level>/:UploadedAttachments-<n>/) and other runs on the same host never read or delete these copies.COPYFILE_EXCLand moves to the next name onEEXIST.onComplete.uploadAttachment/uploadMediaare no-op stubs, so they never throw.
Two robustness changes ride along:
updateURLSForGRRno longer throws on a degeneratebin-session config (which previously aborted the whole CLI bootstrap and silently disabled
every product for that run), and
setConfigkeeps its defaults on an empty config insteadof leaving
this.configstale.Related Jira task/s
SDK-7138 (regression of SDK-3420, Closed)
Release (mandatory for every PR — required for the
ready-for-reviewlabel)Version bump: (required — tick exactly one)
Release notes type: (optional)
Release notes (customer-facing): (optional but encouraged)
browser.uploadAttachment(filePath)(also available asbrowser.uploadMedia) so WebdriverIO tests can attach files to a test, hook, or build in Test Reporting — the same capability the Java, Python and Node SDKs already offer. Pass{ buildAttachment: true }to attach to the build instead of the current test.Release notes (internal): (required — engineer-facing; what actually changed / why)
UploadAttachmentModule(src/cli/modules/uploadAttachmentModule.ts) registersbrowser.uploadAttachment/browser.uploadMediaonAutomationFrameworkState.CREATE/HookState.POST, resolves Test/Hook/Build level plus the uuid, and emits a singleTEST_ATTACHMENTLogCreatedentry. Rejects missing files, non-files and >100 MB (parity with the other SDKs) without throwing at the caller.grpcClient.logCreatedEventnow forwardsfileName/fileSize/filePath, which the proto and generated types already carried but the mapper dropped.UPLOAD_ATTACHMENT_ACK_TIMEOUT_MS): awaiting the binary round-trip inline stalled the following a11y pre-commandexecuteAsyncscan on Chrome. See the caveat below — this is reduced but not fully eliminated.APIUtils.updateURLSForGRRis now fully optional-tolerant per field;setConfigshort-circuits on an emptyresponse.configinstead of throwing insideJSON.parse.UploadedAttachments-wdio-<bin session id>/<worker pid>/<level>/. The copy usesCOPYFILE_EXCLplus a retry onEEXIST, and the launcher cleans up only this run's folder inonComplete.uploadAttachment/uploadMediaare always installed, as no-op stubs when Test Reporting is inactive. The ack timer isunref()'d.tests/cli/modules/uploadAttachmentModule.test.ts(21) andtests/cli/apiUtils.test.ts(4).Checklist
PR Validations
Run Tests: Comment RUN_TESTS to trigger sanity tests.
Verification
Merge state. Branch is 67 commits behind
mainbutgit merge-treereports a cleanmerge. I built and tested the merge result locally:
mainmain+ this branchIdentical failing sets (
launcher,service,crash-reporter,util,cliUtils,cliUtils.staleBinary,funnelInstrumentation,requestUtils) — allpre-existing on
main. This branch adds 2 test files / 15 tests, all passing, andintroduces zero new failures.
npm run buildandeslint --ext .ts src testsbothclean on the merge result (node 18.20.8).
End-to-end, via BStackAutomation
test_wdio_mocha_wrapper_upload_media_tags.pywith theservice npm-linked from this branch (link provenance confirmed:
node_modules/@wdio/browserstack-service -> .../wdio-browserstack-service/packages/browserstack-service):8.48.0TypeError: driver.uploadMedia is not a functionx2 platforms[]['4194886789','4194887675','4194889268','4194891201']{"passed":0,"failed":0,...}{"passed":4,"failed":0,...}['unmarked','unmarked']['passed','passed']O11Y build
t39ilc2tdvh20gtjtgj9yoqi3shzzzfjlit1tjht. Companion test-side PR:browserstack/BStackAutomation#83493.
Snapshot folders, head
a5fb1d8(handles the parallel-worker race and cross-SDK overlap review comment):uploadAttachmentModule.test.tsCOPYFILE_EXCL); with no bin session nothing is sent; cleanup removes only this run's folder, and leaves another run's folder and the Python/JavaUploadedAttachments-<n>alone.tsc --noEmitand eslint are clean. GitHub CI (node 18.20 / 20 / 22) passes.test_wdio_mocha_wrapper_upload_media_tags.pyUploadedAttachments-wdio-7f9aa99e…/66023/…and…/66025/…. The folder name matches the run's bin session id. 70TEST_ATTACHMENTevents, 0 file-read errors in the binary log, and noUploadedAttachments*left in the writable dir after the run. The O11y rollup is 4 passed / 0 failed, and session run status and test-run data pass. The attachment and custom-tag read-back checks return 401 on this machine (its O11y service key isn't authorized forapi/v1/testRuns/*), so Jenkins covers those.Known caveat — intermittent Chrome stall (not fixed by this PR)
With
uploadMediaactive and accessibility auto-scanning on, the Chrome workerintermittently stalls: every command issued after
uploadMediahangs until the 60s mochatimeout, then the hub reaps the session (
Session not started or terminated). Measuredacross 5 end-to-end runs on this branch:
uploadMediacalls removed)So ~40% of runs, uncorrelated with attachment size, and absent when the
uploadMediacalls are removed. Edge (no a11y) is never affected. This is the same failure mode
37f672dtargeted — the off-stack dispatch reduced it but did not eliminate it. Theservice-side send is genuinely fire-and-forget (
recordAttachmentnever awaits the gRPCround-trip), so the remaining stall most likely sits on the binary side, where the
attachment upload and the a11y scan path meet. I did not root-cause it further and I do
not think it should block this PR — the command is strictly better than the
TypeErrorit replaces — but it needs its own ticket before
uploadMediais recommended alongsideaccessibility auto-scanning.
🤖 Generated with Claude Code