PR #1983 round 1 introduced unique-suffix tmp paths so two concurrent
writers no longer share a single `.tmp` file. That fix is correct
under POSIX semantics — `rename(2)` is atomic between source and
destination, so each writer renames onto the same target without
conflict.
Windows `MoveFileExW` is not the same. It fails with
EPERM / EACCES / EBUSY when the target is currently being renamed
by *another* process — a short race window that fires reliably under
this hook's PostToolUse + statusline concurrency. Round 1's CI run
made this visible:
Test (windows-latest, Node 18.x, npm) — FAILURE
Error: EPERM: operation not permitted, rename
'C:\…\ecc-metrics-test-bridge-race-….json.9504.4aef575a.tmp' ->
'C:\…\ecc-metrics-test-bridge-race-….json'
at writeBridgeAtomic (scripts/lib/session-bridge.js:79:8)
All nine Windows matrix cells (Node 18 / 20 / 22 × npm / pnpm / yarn)
hit the same path. POSIX matrices (Linux + macOS) passed unchanged.
Fix: extract a `renameWithRetry(tmp, target)` helper that retries
`fs.renameSync` up to 5 times on EPERM / EACCES / EBUSY with
exponential backoff (20 ms → 320 ms total). Other error codes
(ENOENT, ENOSPC, EROFS, …) re-throw on the first attempt — they are
not transient. POSIX runs hit the first try and exit immediately.
The backoff uses `Atomics.wait` on a throwaway `SharedArrayBuffer`
so the retry path does not busy-spin the CPU; verified on Node ≥ 17
that this works on the main thread. There is a `try/catch` fallback
to a brief busy-wait for older runtimes where `Atomics.wait` is
restricted to workers.
`writeBridgeAtomic` calls the helper instead of `fs.renameSync` and
keeps its existing best-effort tmp cleanup on terminal failure.
`renameWithRetry` is added to `module.exports` so the companion
`writeWarnState` in `scripts/hooks/ecc-context-monitor.js` can
adopt the same retry policy without duplicating the helper. That
adoption lands in the next commit.
Local: `node tests/lib/session-bridge.test.js` 14/14, `yarn test`
green, `yarn lint` clean. The round-1 test (two concurrent child
writers, 200 iterations each) now passes on macOS without retrying
at all (POSIX path) and is expected to pass on Windows via the new
retry loop.
Two round-1 review findings in `tests/lib/session-bridge.test.js`,
both about test correctness rather than the underlying fix:
1. **greptile P1 + coderabbitai Major + cubic P2 (all three): concurrent-write test ran sequentially.**
The test spawned two child processes with two consecutive
`spawnSync` calls. Because `spawnSync` blocks until the child
exits, the second writer started *after* the first finished —
the two writers never overlapped, so the rename race the fix
targets was never actually exercised. The test would have passed
with the old broken `${target}.tmp` suffix.
Fix: introduce a one-off "race runner" helper that runs inside
its own subprocess and uses async `spawn` to start both writers
simultaneously. The runner waits for both to exit (the event
loop is local to the runner subprocess, so this stays compatible
with the synchronous test harness used elsewhere in this file)
and reports both exit codes plus stderrs on stdout. The test
then calls the runner via `spawnSync` and parses the result.
Both writer children now overlap for the duration of their 200
`writeBridgeAtomic` calls each, which is enough wall time to
reliably trigger the rename race against the pre-fix code.
Verified: with the fixed `${target}.${pid}.${nonce}.tmp` suffix,
the test passes; with the old fixed `${target}.tmp` suffix
reintroduced, it fails as expected (one writer hits ENOENT on
roughly half its rename calls).
2. **greptile P2 + cubic P3: `assert.throws` used a string as the second argument.**
Node deprecated passing a string as the second argument to
`assert.throws` years ago: the string is silently treated as
the assertion failure message (what to print when the function
does *not* throw) rather than as an error matcher. The check
passed for any thrown error, not just the rename failure.
Fix: pass a regex matcher as the second arg and keep the
explanatory text as the third. The regex matches `EISDIR`,
`EPERM`, `ENOTDIR`, or `ENOENT` because `renameSync` of a
regular tmp file onto an existing directory raises different
codes on Linux / macOS / BSD — making the matcher portable
across CI runners.
Test count unchanged at 14; `npm test` green; `npm run lint` clean.
The two helper files (`tests/__tmp_bridge_writer.js`,
`tests/__tmp_bridge_race_runner.js`) are written and unlinked
inside the test's try/finally so they never persist beyond the
test run.
Two regression tests pin down the previous two commits' atomic-rename
fixes:
1. **concurrent writes don't throw ENOENT or corrupt the file** —
spawns two child Node processes (`tests/__tmp_bridge_writer.js`
created in-test, cleaned up in finally) that each call
`writeBridgeAtomic(sid, …)` 200 times against the same session
ID with independent payloads. Asserts both subprocesses exit 0
(the previous implementation produced ENOENT on roughly 50% of
rename calls, all swallowed by the in-test catch) and the final
bridge file is parseable JSON belonging to one of the two writers
(last-writer-wins is fine; the contract is *no corruption* and
*no rename ENOENT*, not data preservation).
2. **tmp file cleanup on rename failure** — pre-creates a directory
at the target bridge path so `renameSync(tmp, target)` fails,
calls `writeBridgeAtomic`, asserts the call throws AND that no
tmp file with the writer's `pid.<nonce>.tmp` prefix is left
behind in `os.tmpdir()`. The previous code had no cleanup; the
fix's `try/catch + unlinkSync` keeps tmpdir from accumulating
orphan files across repeated rename failures.
The first test deliberately writes independent payloads from each
subprocess so this regression doesn't try to claim a property the
fix doesn't actually deliver (read-modify-write race in the caller
is a separate issue and out of scope per PR body).
Test count: 12 → 14 in `tests/lib/session-bridge.test.js`;
`npm test` green; `npm run lint` clean.
Mirror the previous commit's `writeBridgeAtomic` fix on the
companion `writeWarnState` in `ecc-context-monitor.js`. Same shape:
fixed `${target}.tmp` → `${target}.${process.pid}.${randomNonce}.tmp`,
plus best-effort cleanup of the tmp file on `renameSync` failure
(throws original error after cleanup).
`writeWarnState` debounces the context-monitor's threshold alarms
(`COST_NOTICE_USD`, `COST_WARNING_USD`, `COST_CRITICAL_USD`, plus the
context-remaining and loop-detection ones). Without unique suffixes,
two PostToolUse subprocesses racing on the warn-state file produce
either a corrupted JSON debounce-state on disk or an ENOENT throw
that the hook catches and swallows — either way the next warn-state
read returns the default `{callsSinceWarn: 0, lastSeverity: null}`
and the threshold alarms re-fire or stop firing erratically. Users
see warning messages flicker or vanish; debounce no longer works.
Three call sites in this repo now share the same atomic-write
contract:
- `writeBridgeAtomic` (scripts/lib/session-bridge.js) — primary
- `writeCostWarningIfChanged` (scripts/hooks/ecc-metrics-bridge.js) — cost cache
- `writeWarnState` (this file) — debounce state
`yarn lint` clean. Regression test covering both `writeBridgeAtomic`
and `writeWarnState` under concurrent load lands in the next commit.
`writeBridgeAtomic` wrote to a fixed `${target}.tmp` path before
calling `renameSync`. When two processes write to the same session
bridge concurrently (e.g. PostToolUse `ecc-metrics-bridge` + the
background `ecc-statusline`, both calling `writeBridgeAtomic(sessionId, ...)`),
the canonical atomic-rename race fires:
1. Process A: writeFileSync(target.tmp, JSON_A) — tmp file exists.
2. Process B: writeFileSync(target.tmp, JSON_B) — tmp file overwritten.
3. Process A: renameSync(target.tmp, target) — succeeds; target = JSON_B
(A's payload silently corrupted en-route).
4. Process B: renameSync(target.tmp, target) — throws ENOENT (the
rename consumed the file).
Every caller in the repo wraps `writeBridgeAtomic` in `try {} catch {}`,
so the ENOENT exception is swallowed and the user-visible symptom is
just "the bridge file occasionally contains the wrong process's
payload" with no diagnostic.
Reproduced before this commit:
$ # two concurrent writers, each calling writeBridgeAtomic 500 times
$ # against the same session ID
[A] errors=244 # 244 ENOENT exceptions swallowed
[B] errors=248 # ditto
After this commit the same workload reports 0 errors in both
subprocesses: tmp paths no longer collide.
Fix: change `${target}.tmp` to
`${target}.${process.pid}.${crypto.randomBytes(4).toString('hex')}.tmp`,
matching the pattern already used by `writeCostWarningIfChanged` in
`scripts/hooks/ecc-metrics-bridge.js` (commit 9b1d8918). The pid +
4-byte nonce gives each writer process a distinct tmp path, so step 2
above no longer overwrites step 1's payload and step 4 no longer
races step 3.
Also added: on `renameSync` failure, attempt `fs.unlinkSync(tmp)` so
a writer that fails (disk full, permission, parent dir gone) does
not leak its tmp file. The cleanup is best-effort and the original
error is still re-thrown.
**Scope clarification.** This commit closes the atomic-rename
primitive's race only. The *read-modify-write* race in callers —
two writers each read the same bridge state, increment, and write
back, the second clobbering the first — is a separate concern that
needs locking or per-writer logs, and is intentionally out of scope
for this PR. The cost-tracker / metrics-bridge callers tolerate
last-writer-wins on their cumulative aggregates today and this
commit does not change that contract.
The companion `writeWarnState` in `ecc-context-monitor.js` has the
same fixed-suffix pattern and the same race; that fix lands in the
next commit so each can be reviewed against its own diff.
Adds the Blender motion state inspection skill with maintainer refinements for tools metadata, usage guidance, meter-scale threshold assumptions, and Blender interpreter notes.
The OpenAI-compatible API can return HTTP 200 with an empty choices list
or choices[0].message = None (content-filtered responses on Gemini,
overwhelmed Ollama instances). Without a guard, both sites raise an
unhandled IndexError or AttributeError crashing the provider.
Added guard in OpenAIProvider.generate() and AstraFlowProvider.generate().
Jamkris