Second review found that removeTree (851daeec2) opened the directory it
was handed with opendir, which follows a symbolic link. The stale sweep
hands it every entry of $TMPDIR whose name carries the executor's prefix
and a past stamp, so a link planted under such a name had the directory
it pointed to emptied - where the recursive rm it replaced removed the
link alone. A script that replaced its own exchange directory with a
link got the same. removeTree now lstats the top and removes anything
that is not a directory as itself; below the top a directory entry
already says whether it is a link.
Review found that 851daeec2 weakened "has finished its sweep by the time
the step resolves": its spy delayed every fs.promises.rm call, and since
the step now removes its own exchange directory through fs.promises.rm
too, that removal outlasted the sweep and the case passed with the
executor's `await pendingSweep` deleted. The spy now delays only the
abandoned directory's removal, so the case fails again without the wait.
With the IPC fix in, ten bash cases still failed on the Windows runner.
None is a product failure; each asserts something Windows does not do, or
measures it in a way that cannot work there.
- A named pipe as the document: Git Bash's `mkfifo` makes a Cygwin FIFO
that Node sees as no file at all. The directory row still runs there.
- Three CRLF cases: Git for Windows' bash drops a carriage return from the
line it reads, so a CRLF script runs there as its LF twin - all three
ran clean on the runner. POSIX only.
- The stray-sibling case wrote a literal carriage return inside `$'...'`,
which that same rule turned into an empty word, so the redirection
emptied the real document. It now writes U+F00D on Windows, the name
msys2 gives such a file, as the case beside it already does.
- Three signal-verdict cases: the step dies by a signal, and Windows has
none to send. POSIX only, as the file already says of signal verdicts.
- The deadline-watchdog case found its descendant through `sleep & echo
$!`, and under Git Bash that pid is an MSYS number `process.kill` cannot
ask Windows about, so it read as dead from the start. A Node descendant
writes its own pid now, as the lifeline case does - which makes this the
case that runs the watchdog's `taskkill` arm on Windows.
- The sweep's 12 ms stall bound sits under Windows' ~15.6 ms timer tick,
so a 5 ms heartbeat cannot resolve it there. Skipped on Windows.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X2EFhLrnjAkfgPkN1fva1k
`removeExchange`'s `fs.rmSync(dir, { recursive: true, force: true })` and the
sweep's `fs.promises.rm(..., { recursive: true, force: true })` differ only in
being sync and async, so a cause that refused the removal refuses the sweep too.
A script that runs `chmod 500` on its own exchange directory leaves the note
"a later bash step sweeps it" and an `output.json` that is still in $TMPDIR
after the next step's sweep. The note now separates the two causes: one that
clears on its own is swept, and a mode the script put on the directory needs it
removed by hand.
Two lifecycle guarantees stated near here were also asserted by nothing -
deleting the throttle or the `await pendingSweep` left the whole suite green.
Both now have a case: the root is read at most once across four steps inside the
interval, and an abandoned directory is gone the moment `execute` resolves, with
the sweep's own `rm` slowed so that a step which did not wait for it could not
pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015Z6tHq3XazgZbrS4YT73xs
`sweepStaleExchanges` ran a `readdirSync` on the tool server's main thread -
it is called synchronously from `createExchange`, which `runOne` calls on the
bash path - and in production its root is `os.tmpdir()`, a directory shared
with every other process on the host and bounded by nothing here.
The 60 s throttle bounds how often the read happens, not what it costs. On
this machine a root holding 60 000 entries cost 33 ms of blocked loop per
sweep: no MCP request, device socket or timer ran during it, and the step's
own wall time went from 53 ms to 89 ms. The stall grows over a machine's life,
since the directory it reads is one the tool server never prunes.
Awaiting a `readdir` is not the fix - the read is scheduled off-thread either
way, and it is building the array of every name that blocks. The sweep now
walks a directory handle in batches of 64, which hands back a little work per
turn, and it runs beside the step rather than in front of it; `runOne` waits
for it at the end, so a step never outlives its own sweep.
Measured with the same rig, one bash step against a 60 000-entry root:
before step 89 ms, worst blocked loop 33 ms
after step 71 ms, worst blocked loop 0 ms
(an empty root is 53 ms / 1 ms either way)
Pinned by a case that plants 40 000 entries and watches a 5 ms heartbeat:
21-22 ms of block before, 0 after.
Removing the `Math.ceil` left all 2165 tests in `packages/tool-server/test/
flows/` green, so a future simplification there was silent in CI.
The fractional value really reaches that line: `flow-script-step-parse.test.ts`
pins `timeout: 1500.5` as a legal step, and `clampTimeout` returns a wanted
value at or under `maxTimeoutMs` un-rounded. What the guard prevents is what
the comment above it already describes - the directory is named
`argent-flow-script-<n>.5-XXXXXX`, `exchangeOwnedUntil`'s `/^(\d+)-/` cannot
match it, the sweep passes over it forever, and an exchange whose owner could
not remove it keeps an `output.json` that may hold values derived from a
secret under the shared `os.tmpdir()`.
The new case runs a step with `timeoutMs: 1500.5` and has the script report
its own exchange directory - otherwise removed before anything outside the
executor could look at it - then reads the stamp back the way the sweep does.
Removing the `Math.ceil` now fails it.
A `script` step's work is as often a shell one-liner as a Node program:
`curl` an internal API, `psql -f` a seed file, `adb shell` a settings
change. This adds `.sh` as a second language on the existing step - one
new extension, no new key, no new tool, no new step kind and no new
report field.
The contract is one line each: input is the environment, the output
document is the file named by `$ARGENT_OUTPUT`, the verdict is the exit
status, and a failure reason is the file named by `$ARGENT_REASON`.
Bash runs inside the runner rather than instead of it. The executor forks
the same `flow-script-runner.mjs`, as the entry module rather than as an
`--import` preload, and the runner spawns bash as its own child. That
keeps one process group per step, so the parent's group stop and both
watchdogs' group kills reach a bash descendant unchanged - which matters
because a bash polling loop is exactly the orphan the watchdogs exist for,
and a shell has no thread to watch a descriptor from.
Hubert Gancarczyk