Commit Graph

3 Commits

Author SHA1 Message Date
Jamkris
d904edc615 test(lib): make concurrent-write test actually concurrent + use regex matcher for assert.throws
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.
2026-05-19 04:57:10 -04:00
Jamkris
5acb01a276 test(lib): concurrent writeBridgeAtomic + tmp-cleanup regression
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.
2026-05-19 04:57:10 -04:00
Affaan Mustafa
940135ea47 feat: add ECC statusline observability hooks
Salvages the useful statusline/context monitor work from stale PR #1504 while preserving the current continuous-learning hook runner wiring.

Adds the metrics bridge, context monitor, statusline script, shared cost/session bridge utilities, and tests. Fixes the reviewed false loop-detection hash collision for non-file tools, avoids default-session cost inflation, sanitizes statusline task lookup, and records hook payload session IDs in cost-tracker.
2026-05-11 23:44:06 -04:00