Skip to content

Commit d041bac

Browse files
Lykhoydaclaude
andauthored
fix(428): harden Android raw screenshot capture (#467)
The Android raw path in device-screenshot-raw.ts was fallback-only and unchanged when #427 hardened iOS, carrying three pre-existing bugs: 1. Truncate-before-success: createWriteStream opened the caller's FINAL path (truncating it) before capture succeeded, and unlinkSync'd it on failure — destroying a file the tool never created. Now stage bytes in a unique sibling temp file and renameSync onto the final path only after BOTH the write stream drained AND adb exited 0. Same-dir temp keeps the rename an atomic same-filesystem move. 2. Multi-emulator first-pick: the resolver grabbed the first emulator line, so with several booted and no session binding a raw screenshot could hit the wrong device. Now exactly-one-or-refuse via resolveAndroidEmu (null on ambiguity), mirroring iOS resolveIosUdid; the first-pick parseAdbDevicesEmu is removed. Sessions still bind via tryRawScreenshot's preferredDeviceId. 3. adb child leak on write-stream error: on out.on('error') the adb child kept running blocked on stdout. Now unpipe + kill it before settling. Adds gh-428-android-raw-screenshot.test.ts (fake-adb spawn seam drives the real capturer: temp/rename, two-track settle, stream-error kill) and updates the two prose references to the removed parser. Full unit suite green (2924). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 3fe5e32 commit d041bac

8 files changed

Lines changed: 415 additions & 92 deletions
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
---
2+
'rn-dev-agent-cdp': patch
3+
'rn-dev-agent-plugin': patch
4+
---
5+
6+
Harden the Android **raw** screenshot capture path (`device_screenshot`, GH #428),
7+
mirroring the iOS hardening from #427:
8+
9+
- **Truncate-before-success**: raw capture now stages `adb exec-out screencap`
10+
bytes in a unique sibling temp file and `renameSync`s onto the caller's path
11+
only after both the write stream drains and adb exits 0. A failed or timed-out
12+
capture can no longer truncate-then-delete an existing file the tool never
13+
created.
14+
- **Multi-emulator first-pick**: with several emulators booted and no session
15+
binding, resolution now refuses (exactly-one-or-null via `resolveAndroidEmu`)
16+
instead of silently grabbing the first emulator — matching iOS
17+
exactly-one-or-refuse. Sessions still bind to their device id.
18+
- **adb child leak on stream error**: a write-stream error (ENOSPC/EACCES) now
19+
unpipes and kills the `adb` child before settling, instead of leaving it
20+
running blocked on stdout.

scripts/cdp-bridge/dist/tools/device-list.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,7 @@ export async function captureAndResizeScreenshot(args) {
277277
// capturing the other platform via agent-device's broken `--platform`
278278
// routing defeats the entire purpose of passing the arg. Re-evidence on
279279
// the user-reported regression: an OOM-unstable emulator leaves
280-
// `adb devices` returning the emulator as `offline`, parseAdbDevicesEmu
280+
// `adb devices` returning the emulator as `offline`, parseAdbDevicesEmuAll
281281
// skips it, the fallback fires, iOS screen is returned.
282282
const rawResultOk = (path, platform) => ({
283283
content: [

scripts/cdp-bridge/dist/tools/device-screenshot-raw.js

Lines changed: 92 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,8 @@
1919
* spawning real `xcrun`/`adb` subprocesses.
2020
*/
2121
import { execFile, spawn } from 'node:child_process';
22-
import { createWriteStream, unlinkSync } from 'node:fs';
22+
import { createWriteStream, renameSync, unlinkSync } from 'node:fs';
23+
import { basename, dirname, join } from 'node:path';
2324
import { promisify } from 'node:util';
2425
const execFileAsync = promisify(execFile);
2526
// GH #422: the single-pick parseSimctlBootedUDID was removed — first-booted
@@ -75,19 +76,44 @@ export async function resolveIosUdid(explicit, probe = defaultSimctlBootedJson)
7576
}
7677
}
7778
const EMU_LINE = /^(emulator-\d+)\s+device\b/;
78-
export function parseAdbDevicesEmu(stdout) {
79-
const lines = stdout.split('\n');
80-
for (const line of lines) {
79+
// GH #428: the single-pick parseAdbDevicesEmu was removed — first-booted
80+
// selection was a silent wrong-device capture with several emulators booted and
81+
// no session binding. All resolution goes through parseAdbDevicesEmuAll +
82+
// exactly-one, mirroring the iOS parseSimctlBootedAll hardening from GH #422/#427.
83+
export function parseAdbDevicesEmuAll(stdout) {
84+
const ids = [];
85+
for (const line of stdout.split('\n')) {
8186
const trimmed = line.trim();
82-
if (!trimmed)
83-
continue;
84-
if (trimmed.startsWith('List of devices'))
87+
if (!trimmed || trimmed.startsWith('List of devices'))
8588
continue;
8689
const match = trimmed.match(EMU_LINE);
8790
if (match)
88-
return match[1];
91+
ids.push(match[1]);
92+
}
93+
return ids;
94+
}
95+
async function defaultAdbDevicesStdout() {
96+
const { stdout } = await execFileAsync('adb', ['devices'], {
97+
timeout: 5000,
98+
maxBuffer: 1024 * 1024,
99+
});
100+
return stdout;
101+
}
102+
/**
103+
* GH #428: exactly-one-or-refuse Android emulator resolution, mirroring the iOS
104+
* resolveIosUdid contract. With several emulators booted and no session UDID,
105+
* first-pick could silently capture the wrong device — refuse (return null) so
106+
* the caller hard-fails with an actionable message instead. Callers with a
107+
* session pass its device id through tryRawScreenshot's preferredDeviceId.
108+
*/
109+
export async function resolveAndroidEmu(probe = defaultAdbDevicesStdout) {
110+
try {
111+
const all = parseAdbDevicesEmuAll(await probe());
112+
return all.length === 1 ? all[0] : null;
113+
}
114+
catch {
115+
return null;
89116
}
90-
return null;
91117
}
92118
const defaultIosResolver = async () => {
93119
try {
@@ -109,18 +135,7 @@ const defaultIosResolver = async () => {
109135
export async function resolveBootedIosUdid() {
110136
return defaultIosResolver();
111137
}
112-
const defaultAndroidResolver = async () => {
113-
try {
114-
const { stdout } = await execFileAsync('adb', ['devices'], {
115-
timeout: 5000,
116-
maxBuffer: 1024 * 1024,
117-
});
118-
return parseAdbDevicesEmu(stdout);
119-
}
120-
catch {
121-
return null;
122-
}
123-
};
138+
const defaultAndroidResolver = () => resolveAndroidEmu();
124139
// Honor the requested format via the path extension, matching how the
125140
// agent-device path infers format. Writing JPEG bytes into a `.png` file
126141
// (the prior hardcoded `--type=jpeg`) produced a mislabeled image because
@@ -147,6 +162,28 @@ export function resolveCaptureOutcome(streamFinished, procCode) {
147162
return 'pending';
148163
return procCode === 0 ? 'success' : 'failure';
149164
}
165+
/**
166+
* GH #428 finding 1: the caller's FINAL path must never be opened until capture
167+
* succeeds. Staging bytes in a unique sibling temp file (same directory → the
168+
* rename is an atomic same-filesystem move, never a cross-device EXDEV copy)
169+
* means a failed/timed-out `adb screencap` can't truncate-then-unlink a file
170+
* the tool didn't create. The name is dotfile-prefixed and `.rawtmp`-suffixed
171+
* so a crash leaves an obviously-transient artifact, not a plausible screenshot.
172+
*/
173+
export function rawTempPath(finalPath, uniq) {
174+
return join(dirname(finalPath), `.${basename(finalPath)}.${uniq}.rawtmp`);
175+
}
176+
// Per-process monotonic suffix — collision-free across rapid successive captures
177+
// without Date.now()/Math.random(), which keeps temp names deterministic in tests.
178+
let captureCounter = 0;
179+
function nextCaptureSuffix() {
180+
captureCounter += 1;
181+
return `${process.pid}.${captureCounter}`;
182+
}
183+
const defaultAndroidSpawn = (emuId) => spawn('adb', ['-s', emuId, 'exec-out', 'screencap', '-p'], {
184+
stdio: ['ignore', 'pipe', 'pipe'],
185+
});
186+
let androidSpawn = defaultAndroidSpawn;
150187
// Android needs the binary screen bytes piped to a file. execFile can't redirect
151188
// stdout, so spawn directly and pipe to a write stream — no shell, so the path
152189
// is safely passed as a literal filename, not interpolated into a command string.
@@ -156,30 +193,30 @@ export function resolveCaptureOutcome(streamFinished, procCode) {
156193
// alone is insufficient: 'finish' before non-zero close = truncated/partial
157194
// file reported as success (deepsec 2026-05-12 finding); 'close' before
158195
// 'finish' = success reported before bytes hit disk (earlier multi-LLM
159-
// review finding). On any failure path, the partial file is unlinked so
160-
// `resizeWithSips` never sees a corrupt artifact.
161-
const defaultAndroidCapturer = async (emuId, path) => new Promise((resolve) => {
196+
// review finding). Bytes are staged in a temp file (GH #428 finding 1) and
197+
// promoted onto `path` via renameSync only once the outcome is success; every
198+
// failure path unlinks the temp and leaves the caller's path untouched.
199+
export const defaultAndroidCapturer = async (emuId, path) => new Promise((resolve) => {
162200
let settled = false;
163201
let streamFinished = false;
164202
let procCode = null;
165-
const proc = spawn('adb', ['-s', emuId, 'exec-out', 'screencap', '-p'], {
166-
stdio: ['ignore', 'pipe', 'pipe'],
167-
});
168-
const out = createWriteStream(path);
169-
const cleanupPartial = () => {
203+
const proc = androidSpawn(emuId);
204+
const tmp = rawTempPath(path, nextCaptureSuffix());
205+
const out = createWriteStream(tmp);
206+
const cleanupTemp = () => {
170207
try {
171-
unlinkSync(path);
208+
unlinkSync(tmp);
172209
}
173210
catch {
174-
/* file may not exist yet — ignore */
211+
/* temp may not exist yet — ignore */
175212
}
176213
};
177214
const timer = setTimeout(() => {
178215
if (settled)
179216
return;
180217
proc.kill();
181218
out.destroy();
182-
cleanupPartial();
219+
cleanupTemp();
183220
settle(false);
184221
}, 15_000);
185222
const settle = (ok) => {
@@ -195,22 +232,38 @@ const defaultAndroidCapturer = async (emuId, path) => new Promise((resolve) => {
195232
return;
196233
if (outcome === 'failure') {
197234
out.destroy();
198-
cleanupPartial();
235+
cleanupTemp();
236+
settle(false);
237+
return;
238+
}
239+
// success: promote the staged temp onto the caller's path only now that
240+
// both the stream drained AND adb exited 0. A rename failure (e.g. cross
241+
// volume, vanished dir) degrades to capture-failed rather than a partial.
242+
try {
243+
renameSync(tmp, path);
244+
settle(true);
245+
}
246+
catch {
247+
cleanupTemp();
248+
settle(false);
199249
}
200-
settle(outcome === 'success');
201250
};
202251
proc.stdout.pipe(out);
203252
out.on('finish', () => {
204253
streamFinished = true;
205254
maybeSettle();
206255
});
207256
out.on('error', () => {
208-
cleanupPartial();
257+
// GH #428 finding 3: unpipe + kill the adb child before settling — else it
258+
// keeps running, blocked writing to a stdout no one is draining.
259+
proc.stdout.unpipe(out);
260+
proc.kill();
261+
cleanupTemp();
209262
settle(false);
210263
});
211264
proc.on('error', () => {
212265
out.destroy();
213-
cleanupPartial();
266+
cleanupTemp();
214267
settle(false);
215268
});
216269
proc.on('close', (code) => {
@@ -231,12 +284,15 @@ export function _setForTest(overrides) {
231284
iosCapturer = overrides.iosCapturer;
232285
if (overrides.androidCapturer)
233286
androidCapturer = overrides.androidCapturer;
287+
if (overrides.androidSpawn)
288+
androidSpawn = overrides.androidSpawn;
234289
}
235290
export function _resetForTest() {
236291
iosResolver = defaultIosResolver;
237292
androidResolver = defaultAndroidResolver;
238293
iosCapturer = defaultIosCapturer;
239294
androidCapturer = defaultAndroidCapturer;
295+
androidSpawn = defaultAndroidSpawn;
240296
}
241297
export async function tryRawScreenshot(platform, path, preferredDeviceId) {
242298
const resolver = platform === 'ios' ? iosResolver : androidResolver;

scripts/cdp-bridge/src/tools/device-list.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -367,7 +367,7 @@ export async function captureAndResizeScreenshot(args: ScreenshotArgs): Promise<
367367
// capturing the other platform via agent-device's broken `--platform`
368368
// routing defeats the entire purpose of passing the arg. Re-evidence on
369369
// the user-reported regression: an OOM-unstable emulator leaves
370-
// `adb devices` returning the emulator as `offline`, parseAdbDevicesEmu
370+
// `adb devices` returning the emulator as `offline`, parseAdbDevicesEmuAll
371371
// skips it, the fallback fires, iOS screen is returned.
372372
const rawResultOk = (path: string, platform: 'ios' | 'android'): ToolResult => ({
373373
content: [

0 commit comments

Comments
 (0)