appandflow / appandflow/stim

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

オープン
#714 コメント 2 件 リアクション 0 件 担当者 0 名 GitHub で見る
主要言語
TypeScript
スター
41
フォーク
1
平均マージ
1時間 25分
マージ済み PR(30日)
397

説明

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.

コントリビューションガイド

このリポジトリのコントリビューションガイドは索引されていません

評価

この issue はまだ評価されていません。

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。