morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[GH-ISSUE #5094] [Bug] Agent-evaluation helpers reject timeouts before owned child cleanup #3131

New issue

Closed

opened 2026-10-31 00:36:59 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-31 00:36:59 +00:00

Owner

Originally created by @coygeek on GitHub (Oct 18, 2026).
Original GitHub issue: https://github.com/morluto/rea/issues/5094

Before submitting

I searched the open and closed issues for a related report.
I removed credentials, tokens, private paths, and sensitive target data.

Area

MCP or agent integration

Observed behavior

Controlled production-helper boundary reproduction: runCodex, runProcess and readCommandVersion reject on timeout while their directly owned synthetic child remains active. The caller receives the timeout without confirmed cleanup or an explicit retained cleanup owner.

For runCodex, rejection also skips its stdout/readline completion await. A fixture that exits 200 ms after SIGTERM has an open readline and stdout at rejection; both close automatically after exit. Fixtures for the other helpers acknowledge SIGTERM and stay alive until the probe stops them through their retained handles. This establishes one premature-completion behavior across three helpers, without establishing a permanent readline leak.

Reproduction

From the source checkout at the revision below, run this dependency-free POSIX probe. It evaluates the unchanged final three function declarations; real spawn and readline are wrapped only to retain their exact handles for observation and cleanup. The executable is an authored Node fixture, with a watchdog. runCodex uses the supported minimum 10000 ms; runProcess uses a shortened 5000 ms; the version helper keeps its actual 10000 ms.

WORK="$(mktemp -d)"
env -i PATH="$PATH" HOME="$WORK/home" WORK="$WORK" node --input-type=module <<'JS'
import assert from 'node:assert/strict';
import {spawn as realSpawn} from 'node:child_process';
import {createInterface as realCreateInterface} from 'node:readline';
import {readFile, writeFile, mkdir, rm} from 'node:fs/promises';
import {once} from 'node:events';
import {join} from 'node:path';
const work = process.env.WORK;
await mkdir(join(work, 'home'), {recursive:true});
const fixture = join(work, 'fixture.mjs'), ack = join(work, 'term-ack');
await writeFile(fixture, `#!${process.execPath}
import {writeFileSync} from 'node:fs';
process.on('SIGTERM', () => {
  writeFileSync(${JSON.stringify(ack)}, 'term');
  if (process.argv.includes('--no-daemon')) setTimeout(() => process.exit(0), 200);
});
setInterval(() => {}, 1000);
setTimeout(() => process.exit(2), 15000);
`, {mode:0o700});
const source = await readFile('scripts/verify-agent-experience.mjs', 'utf8');
const start = source.indexOf('async function runCodex(');
assert(start > 0);
let owned, closed, lines;
const spawn = (...args) => {
  owned = realSpawn(...args);
  closed = once(owned, 'close');
  return owned;
};
const createInterface = options => (lines = realCreateInterface(options));
const env = {PATH:process.env.PATH, HOME:join(work,'home')};
const helpers = new Function('spawn','createInterface','codex','evaluationRoot',
  'evaluationEnvironment','optionalModel','timeoutMs','repositoryRoot',
  source.slice(start)+'; return {runCodex,runProcess,readCommandVersion};')(
    spawn,createInterface,fixture,work,env,undefined,10000,process.cwd());
try {
  for (const name of ['runCodex','runProcess','readCommandVersion']) {
    await rm(ack, {force:true});
    lines = undefined;
    try {
      await assert.rejects(() => name === 'runCodex' ? helpers.runCodex('fixture',[]) :
        name === 'runProcess' ? helpers.runProcess(fixture,[],work,5000,env) :
        helpers.readCommandVersion(fixture), /timed out/);
      assert.equal(owned.exitCode, null);
      assert.equal(owned.signalCode, null);
      if (name === 'runCodex') {
        assert.equal(lines.closed === true, false);
        assert.equal(owned.stdout.destroyed, false);
        console.log('runCodex: rejected before child exit/readline/stdout closure');
        await closed;
        assert.equal(owned.exitCode, 0);
        assert.equal(lines.closed, true);
        assert.equal(owned.stdout.destroyed, true);
        console.log('runCodex: child exited; readline/stdout closed automatically');
      } else {
        const deadline = performance.now()+2000;
        while (await readFile(ack,'utf8').catch(() => '') !== 'term') {
          assert(performance.now() < deadline);
          await new Promise(r => setTimeout(r,10));
        }
        assert.equal(owned.exitCode, null);
        assert.equal(owned.signalCode, null);
        process.kill(owned.pid, 0);
        console.log(name+': rejected; SIGTERM acknowledged; child still live');
      }
    } finally {
      if (owned && owned.exitCode === null && owned.signalCode === null) owned.kill('SIGKILL');
      if (closed) await closed;
      lines?.close();
    }
  }
  await helpers.runProcess('/bin/echo',['ok'],work,2000,env);
  assert.equal(await helpers.readCommandVersion('/bin/echo'), '--version');
  console.log('cooperative controls passed; all fixture child closes awaited');
} finally {
  await rm(fixture, {force:true}); await rm(ack, {force:true});
  await rm(join(work,'home'), {recursive:true,force:true});
}
JS
rmdir "$WORK"

This exact combined probe passed once. Separate retained probes for the same delayed-exit and signal-acknowledgement cases also passed during review. No real Codex, provider, API, build or user configuration was used.

Expected behavior

Preserve the primary timeout while joining bounded owned child/output cleanup, or retain and report unresolved resources with their exact cleanup owner and recovery action. Do not infer exit from sending SIGTERM, or output closure from exit alone.

The lifecycle guide applies to one-shot commands and separates process exit from output closure. Its cleanup rules require confirmed release or retained ownership while preserving the primary diagnostic; repository instructions adopt those invariants.

Impact and scope

The manual agent-evaluation lane calls these helpers for version discovery, setup/build and scenarios. On timeout, its failure path can reach disposable authentication/run-directory removal while the child remains active. Caller reachability and file-removal ordering are SOURCE-ONLY; the full lane and removal overlap were not executed.

The probes demonstrate unresolved direct-child ownership after rejection, including open stdout/readline for the delayed-exit case. The delayed fixture recovers automatically; the other two need the probe's external owner to stop them. Actual Codex/dotnet incidence, descendants, persistent resource leakage, real-world frequency and a general operator workaround are NOT_ENOUGH_INFO. No data loss or ordinary REA analysis failure was observed.

REA version

Source main 3537e58c1ab29309f06eabf50bc52adba9817a0c, package version 6.7.0. The runner is source-identical to rea-agents-6.7.0 (27dc42d10249f0410904a75c0997842eb03b931c); stable runtime coverage is NOT_ENOUGH_INFO because only current-main source was exercised.

Runtime and platform

Node.js v26.12.0; macOS 27.0.1; Darwin arm64. Repository-pinned Node 24.18.0, Windows and Linux were not tested.

Target and artifact context

Disposable authored Node children only: delayed SIGTERM exit for the scenario helper; SIGTERM acknowledgement without exit for setup/build/version helpers. No reverse-engineering target or real provider.

Interface

Other

Earliest failure stage

Cleanup, cancellation, or lifecycle

Relevant output or evidence

Observed from the combined probe:

runCodex: rejected before child exit/readline/stdout closure
runCodex: child exited; readline/stdout closed automatically
runProcess: rejected; SIGTERM acknowledged; child still live
readCommandVersion: rejected; SIGTERM acknowledged; child still live
cooperative controls passed; all fixture child closes awaited

SOURCE-ONLY supported path and failure boundary:

Documented public lane and package command.
Version/setup calls, managed build call, and scenario call await these helpers without a later child-cleanup guard.
Scenario timeout, setup/build timeout and version timeout all signal then immediately reject.
Outer finalization removes files without joining the children.

Typed outcome or recovery expected

For all three helpers, timeout completion must carry confirmed bounded cleanup or explicit unresolved ownership and recovery. For runCodex, that includes stdout/readline completion or an accurate incomplete-output state. Preserve successful command/version behavior. Closing readline alone or fixing only one helper does not meet this closure check.

Analysis or suspected cause

Source-derived: each timer calls child.kill("SIGTERM") and immediately rejects the promise awaiting exit. The registered exit listener may run later but cannot resettle that promise. runCodex additionally skips its following await linesClosed. No failure-path cleanup join or retained resource owner bridges these errors to the caller. The readline observation is an extra resource symptom of the same premature timeout settlement.

Additional context

Open and closed reports were searched for runner/helper names, readline and agent timeout terms. Related #1932 concerns evaluation clients; #4845 concerns a separate JADX readiness implementation; #3342 concerns Apktool descendants keeping a probe pending. Their closure checks do not cover this agent-evaluation runner's three helpers. This report makes no claim about real-provider signal responsiveness.

Originally created by @coygeek on GitHub (Oct 18, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/5094 ## Before submitting - [x] I searched the open and closed issues for a related report. - [x] I removed credentials, tokens, private paths, and sensitive target data. ## Area MCP or agent integration ## Observed behavior Controlled production-helper boundary reproduction: `runCodex`, `runProcess` and `readCommandVersion` reject on timeout while their directly owned synthetic child remains active. The caller receives the timeout without confirmed cleanup or an explicit retained cleanup owner. For `runCodex`, rejection also skips its stdout/readline completion await. A fixture that exits 200 ms after SIGTERM has an open readline and stdout at rejection; both close automatically after exit. Fixtures for the other helpers acknowledge SIGTERM and stay alive until the probe stops them through their retained handles. This establishes one premature-completion behavior across three helpers, without establishing a permanent readline leak. ## Reproduction From the source checkout at the revision below, run this dependency-free POSIX probe. It evaluates the unchanged final three function declarations; real `spawn` and readline are wrapped only to retain their exact handles for observation and cleanup. The executable is an authored Node fixture, with a watchdog. `runCodex` uses the supported minimum 10000 ms; `runProcess` uses a shortened 5000 ms; the version helper keeps its actual 10000 ms. ```sh WORK="$(mktemp -d)" env -i PATH="$PATH" HOME="$WORK/home" WORK="$WORK" node --input-type=module <<'JS' import assert from 'node:assert/strict'; import {spawn as realSpawn} from 'node:child_process'; import {createInterface as realCreateInterface} from 'node:readline'; import {readFile, writeFile, mkdir, rm} from 'node:fs/promises'; import {once} from 'node:events'; import {join} from 'node:path'; const work = process.env.WORK; await mkdir(join(work, 'home'), {recursive:true}); const fixture = join(work, 'fixture.mjs'), ack = join(work, 'term-ack'); await writeFile(fixture, `#!${process.execPath} import {writeFileSync} from 'node:fs'; process.on('SIGTERM', () => { writeFileSync(${JSON.stringify(ack)}, 'term'); if (process.argv.includes('--no-daemon')) setTimeout(() => process.exit(0), 200); }); setInterval(() => {}, 1000); setTimeout(() => process.exit(2), 15000); `, {mode:0o700}); const source = await readFile('scripts/verify-agent-experience.mjs', 'utf8'); const start = source.indexOf('async function runCodex('); assert(start > 0); let owned, closed, lines; const spawn = (...args) => { owned = realSpawn(...args); closed = once(owned, 'close'); return owned; }; const createInterface = options => (lines = realCreateInterface(options)); const env = {PATH:process.env.PATH, HOME:join(work,'home')}; const helpers = new Function('spawn','createInterface','codex','evaluationRoot', 'evaluationEnvironment','optionalModel','timeoutMs','repositoryRoot', source.slice(start)+'; return {runCodex,runProcess,readCommandVersion};')( spawn,createInterface,fixture,work,env,undefined,10000,process.cwd()); try { for (const name of ['runCodex','runProcess','readCommandVersion']) { await rm(ack, {force:true}); lines = undefined; try { await assert.rejects(() => name === 'runCodex' ? helpers.runCodex('fixture',[]) : name === 'runProcess' ? helpers.runProcess(fixture,[],work,5000,env) : helpers.readCommandVersion(fixture), /timed out/); assert.equal(owned.exitCode, null); assert.equal(owned.signalCode, null); if (name === 'runCodex') { assert.equal(lines.closed === true, false); assert.equal(owned.stdout.destroyed, false); console.log('runCodex: rejected before child exit/readline/stdout closure'); await closed; assert.equal(owned.exitCode, 0); assert.equal(lines.closed, true); assert.equal(owned.stdout.destroyed, true); console.log('runCodex: child exited; readline/stdout closed automatically'); } else { const deadline = performance.now()+2000; while (await readFile(ack,'utf8').catch(() => '') !== 'term') { assert(performance.now() < deadline); await new Promise(r => setTimeout(r,10)); } assert.equal(owned.exitCode, null); assert.equal(owned.signalCode, null); process.kill(owned.pid, 0); console.log(name+': rejected; SIGTERM acknowledged; child still live'); } } finally { if (owned && owned.exitCode === null && owned.signalCode === null) owned.kill('SIGKILL'); if (closed) await closed; lines?.close(); } } await helpers.runProcess('/bin/echo',['ok'],work,2000,env); assert.equal(await helpers.readCommandVersion('/bin/echo'), '--version'); console.log('cooperative controls passed; all fixture child closes awaited'); } finally { await rm(fixture, {force:true}); await rm(ack, {force:true}); await rm(join(work,'home'), {recursive:true,force:true}); } JS rmdir "$WORK" ``` This exact combined probe passed once. Separate retained probes for the same delayed-exit and signal-acknowledgement cases also passed during review. No real Codex, provider, API, build or user configuration was used. ## Expected behavior Preserve the primary timeout while joining bounded owned child/output cleanup, or retain and report unresolved resources with their exact cleanup owner and recovery action. Do not infer exit from sending SIGTERM, or output closure from exit alone. The [lifecycle guide](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/docs/tool-design.md#L74-L88) applies to one-shot commands and separates process exit from output closure. Its [cleanup rules](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/docs/tool-design.md#L123-L140) require confirmed release or retained ownership while preserving the primary diagnostic; [repository instructions](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/AGENTS.md#L19-L23) adopt those invariants. ## Impact and scope The manual agent-evaluation lane calls these helpers for version discovery, setup/build and scenarios. On timeout, its failure path can reach disposable authentication/run-directory removal while the child remains active. Caller reachability and file-removal ordering are SOURCE-ONLY; the full lane and removal overlap were not executed. The probes demonstrate unresolved direct-child ownership after rejection, including open stdout/readline for the delayed-exit case. The delayed fixture recovers automatically; the other two need the probe's external owner to stop them. Actual Codex/dotnet incidence, descendants, persistent resource leakage, real-world frequency and a general operator workaround are NOT_ENOUGH_INFO. No data loss or ordinary REA analysis failure was observed. ## REA version Source main `3537e58c1ab29309f06eabf50bc52adba9817a0c`, package version 6.7.0. The runner is source-identical to `rea-agents-6.7.0` (`27dc42d10249f0410904a75c0997842eb03b931c`); stable runtime coverage is NOT_ENOUGH_INFO because only current-main source was exercised. ## Runtime and platform Node.js v26.12.0; macOS 27.0.1; Darwin arm64. Repository-pinned Node 24.18.0, Windows and Linux were not tested. ## Target and artifact context Disposable authored Node children only: delayed SIGTERM exit for the scenario helper; SIGTERM acknowledgement without exit for setup/build/version helpers. No reverse-engineering target or real provider. ## Interface Other ## Earliest failure stage Cleanup, cancellation, or lifecycle ## Relevant output or evidence Observed from the combined probe: ```text runCodex: rejected before child exit/readline/stdout closure runCodex: child exited; readline/stdout closed automatically runProcess: rejected; SIGTERM acknowledged; child still live readCommandVersion: rejected; SIGTERM acknowledged; child still live cooperative controls passed; all fixture child closes awaited ``` SOURCE-ONLY supported path and failure boundary: - [Documented public lane](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/docs/client-verification.md#L446-L465) and [package command](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/package.json#L229). - [Version/setup calls](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/scripts/verify-agent-experience.mjs#L71-L109), [managed build call](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/scripts/verify-agent-experience.mjs#L422-L449), and [scenario call](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/scripts/verify-agent-experience.mjs#L267-L277) await these helpers without a later child-cleanup guard. - [Scenario timeout](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/scripts/verify-agent-experience.mjs#L502-L540), [setup/build timeout](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/scripts/verify-agent-experience.mjs#L543-L575) and [version timeout](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/scripts/verify-agent-experience.mjs#L577-L612) all signal then immediately reject. - [Outer finalization](https://github.com/morluto/rea/blob/3537e58c1ab29309f06eabf50bc52adba9817a0c/scripts/verify-agent-experience.mjs#L394-L399) removes files without joining the children. ## Typed outcome or recovery expected For all three helpers, timeout completion must carry confirmed bounded cleanup or explicit unresolved ownership and recovery. For `runCodex`, that includes stdout/readline completion or an accurate incomplete-output state. Preserve successful command/version behavior. Closing readline alone or fixing only one helper does not meet this closure check. ## Analysis or suspected cause Source-derived: each timer calls `child.kill("SIGTERM")` and immediately rejects the promise awaiting exit. The registered exit listener may run later but cannot resettle that promise. `runCodex` additionally skips its following `await linesClosed`. No failure-path cleanup join or retained resource owner bridges these errors to the caller. The readline observation is an extra resource symptom of the same premature timeout settlement. ## Additional context Open and closed reports were searched for runner/helper names, readline and agent timeout terms. Related [#1932](https://github.com/morluto/rea/issues/1932) concerns evaluation clients; [#4845](https://github.com/morluto/rea/issues/4845) concerns a separate JADX readiness implementation; [#3342](https://github.com/morluto/rea/issues/3342) concerns Apktool descendants keeping a probe pending. Their closure checks do not cover this agent-evaluation runner's three helpers. This report makes no claim about real-provider signal responsiveness.

bsakaguchi closed this issue

2026-10-31 00:36:59 +00:00

bsakaguchi commented

2026-10-31 00:37:00 +00:00

Author

Owner

@XinwangTao commented on GitHub (Oct 19, 2026):

Thanks for the detailed repro. The timeout path is exactly where a helper must guarantee no owned child survives, yet today the caller gets the timeout while the synthetic child is still running — that makes the error ambiguous (was the timeout caused by the child, or did the child keep running past it?).

I agree runCodex should also complete/destroy its stdout and readline before rejecting; the "fixture exits 200 ms after SIGTERM so handles auto-close" behavior relies on the process exit by luck rather than explicit cleanup. A cleaner order would be: kill the child -> await its exit -> destroy the readline and stdout handles -> then reject with the timeout. That yields one unambiguous completion behavior across runCodex, runProcess and readCommandVersion.

@XinwangTao commented on GitHub (Oct 19, 2026): Thanks for the detailed repro. The timeout path is exactly where a helper must guarantee no owned child survives, yet today the caller gets the timeout while the synthetic child is still running — that makes the error ambiguous (was the timeout caused by the child, or did the child keep running past it?). I agree runCodex should also complete/destroy its stdout and readline before rejecting; the "fixture exits 200 ms after SIGTERM so handles auto-close" behavior relies on the process exit by luck rather than explicit cleanup. A cleaner order would be: kill the child -> await its exit -> destroy the readline and stdout handles -> then reject with the timeout. That yields one unambiguous completion behavior across runCodex, runProcess and readCommandVersion.

bsakaguchi referenced this issue

2026-10-31 01:09:04 +00:00

[PR #3428] [MERGED] test(perf): measure real sigrok annotation selection workflows #5739

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#3131

No description provided.

Rows
Columns