appandflow / appandflow/stim

listIosDevices can leave its temp directory behind, and the test that guards it asserts on the shared tmpdir

Abierto
#714 2 comentarios 0 reacciones 0 asignados Ver en GitHub
Lenguaje dominante
TypeScript
Estrellas
41
Forks
1
Merge medio
1 h 25 min
PR fusionados (30 d)
397

Descripción

Two problems, found because the second one failed the unit suite on a developer machine while CI stayed green.

## 1. The temp directory leaks from real runs

`listIosDevices` writes `devicectl` output into a `stim-devicectl-*` directory under `tmpdir()` and removes it afterwards. After an iOS `caches` e2e run, one survived with its payload intact:

```
drwx------ 3 janicduplessis staff 96 Sep 11 08:17 /var/folders/.../T/stim-devicectl-YRNy7x
-rw-r--r-- 1 janicduplessis staff 487 Sep 11 08:17 devices.json
```

So the removal is skipped on some path — most likely when the process is interrupted or the call throws between writing and cleaning up. Each leak is small, but they accumulate silently in `tmpdir()` and are invisible to `stim gc`, which does not survey them.

## 2. The guard asserts on global state, so anything else on the machine can fail it

`packages/stim-cli/src/__tests__/engine-ios-device.test.ts:281`:

```ts
expect(readdirSync(tmpdir()).some((e) => e.startsWith('stim-devicectl-') && existsSync(join(tmpdir(), e)))).toBe(
false,
);
```

and at `:692` the same idea as a deep-equality against `[]`. These read the **shared** system temp directory, not a directory the test owns. Any leaked directory from any earlier or concurrent Stim process fails them, regardless of whether the code under test behaved correctly.

Observed as:

```
FAIL engine-ios-device.test.ts > listIosDevices runs devicectl into a temp file, parses it, and removes the directory
AssertionError: expected true to be false

FAIL engine-ios-device.test.ts > listIosDevices against a real devicectl > the argv is accepted and every entry it returns is well formed
AssertionError: expected [ 'stim-devicectl-YRNy7x' ] to deeply equal []
```

Removing the single leaked directory made the file pass with nothing else changed — 131/131 test files green afterwards.

This is why CI never sees it: a fresh runner has an empty `tmpdir()`. It is reproducible for anyone who has run Stim on their machine, and it presents as a failure in code the developer did not touch, which is the expensive kind of false signal.

## Suggested fix

For 2, have the test scope its assertion to directories it created — record the paths the stubbed executor was handed and assert those are gone — rather than scanning a directory it does not own.

For 1, remove the directory in a `finally` so an interrupted or throwing call cannot leave it, and consider having `gc` report `stim-devicectl-*` residue so existing leaks are reclaimable.

Both predate 1.1.0.

Guía de contribución

No hay ninguna guía de contribución indexada para este repositorio

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.