fix(discord): engine tests stop in finally; a turn pi never started stops pi instead of guessing (#1509)
Every engine test stops its engine in finally, and commands() tolerates a log that doesn't exist yet, so a failed assertion no longer leaks a fake pi and hangs the six-package union. A turn that failed client-side stays at the front of the queue and holds the next prompt. If pi has sent no agent_start ABORT_GRACE_MS (30 s) after the failure, the engine marks itself wedged, fails held prompts with engine-wedged, refuses new ones with engine-down, and stops pi. The exit reaches onExit, the connector exits 1, and the unit restarts it. Pi's events carry no prompt id, so R1's approach, dropping the turn and sending on, let a late run answer the next prompt. Rocko rejected R1 and approved R2. Tests: engine 17/17 (R1 fails 4, HEAD fails 5). Union 408/408 and the eight suites green on the committed index. Record: agents/darkwing/work/discord-engine-busy/. Co-Authored-By: Claude Opus 5.5 <[email protected]>
This commit is contained in:
Claude Opus 5.5
@@ -0,0 +1,166 @@
|
||||
# #1509 6b engine fixes — Rocko R1 review
|
||||
|
||||
VERDICT: REQUEST CHANGES
|
||||
|
||||
Base supplied: `ef0020ad`. Source-only review, 2026-09-26, for Darkwing and
|
||||
Sage. The three candidate hashes passed `sha256sum -c` before inspection and
|
||||
again after the reproducer:
|
||||
|
||||
| File | SHA-256 |
|
||||
|---|---|
|
||||
| packages/discord/src/engine-pi.mjs | d5bf24b59c07c85067f4087c03b54ca8b4df923c1d591441dedd2e8a7ff2ae39 |
|
||||
| packages/discord/tests/engine.test.mjs | f0abee9c243d46d66dd2271abc3fd89089c350ac6a66ab49131bce80adfcdc33 |
|
||||
| packages/discord/tests/fake-pi.mjs | fa1bf44e3f33eb970a714ada1c686abbc1932baaf418679276edf8813abbe6de |
|
||||
|
||||
## F1 — High, blocking: grace expiry destroys the attribution boundary
|
||||
|
||||
**Location:** engine-pi.mjs lines 103–110, combined with lines 167–168 and
|
||||
205–221. The latter attribute events to the current pending head, without
|
||||
an event-level request identifier.
|
||||
|
||||
The new timer infers that no observed agent_start by the deadline means pi
|
||||
never started the old prompt and will never emit its end. That inference
|
||||
does not follow from silence. A delayed child or delayed stdout delivery
|
||||
can emit the old run's events after the grace expires.
|
||||
|
||||
**Deterministically reproduced against the pinned engine, without processes,
|
||||
files, providers or live connector effects:**
|
||||
|
||||
1. Old prompt is accepted; parent has not observed agent_start.
|
||||
2. Fire its client timeout. Its failed queue entry remains temporarily.
|
||||
3. Enqueue a new prompt and fire the old turn's grace timer.
|
||||
4. Grace removes the failed entry and sends the new prompt, making it the
|
||||
live queue head.
|
||||
5. Deliver the old run's delayed start, tool pair, answer/end and settled,
|
||||
in order. Then deliver the new prompt's success response and own run.
|
||||
6. The new promise returns **OLD RUN ANSWER**, carries **old.md** as its tool
|
||||
evidence, and its actual **NEW RUN ANSWER** is dropped.
|
||||
|
||||
No streaming refusal is needed. This schedule preserves FIFO ordering of
|
||||
the old run's events followed by the new response/run. The README assertion
|
||||
that late events find no live head is false: the grace callback just put
|
||||
the next prompt there. A later refusal also cannot undo an already-resolved
|
||||
promise, but the reproduced accepted-response schedule is sufficient.
|
||||
|
||||
This is a cross-turn answer/evidence integrity failure, not merely reduced
|
||||
availability. In this connector, attributed tool records may include write,
|
||||
git and approval-related metadata. No actual live incident or cross-user
|
||||
data exposure is claimed by this isolated reproduction.
|
||||
|
||||
**Required correction:** preserve the failed turn's attribution boundary
|
||||
until an authoritative termination/quiescence signal, or invalidate this
|
||||
child generation before any new prompt is sent. A bounded grace can fail
|
||||
held work and mark the engine unavailable; it cannot establish quiescence
|
||||
by removing the only correlation guard and continuing on the same stream.
|
||||
If using child replacement, require confirmed old-child termination and
|
||||
isolate old events from the replacement. Do not turn this review into a
|
||||
live restart or automatic restart authority change.
|
||||
|
||||
**Required regression:** timeout before observed start, grace expiry, then
|
||||
late old start/tool/end/settled with a new prompt waiting. The new promise
|
||||
must either fail closed or receive only its own reply/evidence from a safely
|
||||
established generation. Test both late acceptance and refusal timing.
|
||||
Retain the never-started/no-events test, but make its expected recovery
|
||||
consistent with the chosen fail-closed behavior.
|
||||
|
||||
## Answer to Sage's bounded-wait question
|
||||
|
||||
- **Never observed started:** R1 adds a bound, and its `mute` test passes,
|
||||
but the bound is unsafe for the indistinguishable late-events case (F1).
|
||||
- **Observed started, never ends:** R1 deliberately leaves engine busy and
|
||||
retains its pending tombstone. Each held prompt's own arrival-time timeout
|
||||
still rejects it. This bounds each caller's wait, not recovery of engine
|
||||
availability. That limitation is disclosed and inherited through
|
||||
state.busy; it is not the new blocking finding.
|
||||
- **Exit/settle/refused-send cleanup:** release clears the new grace timer
|
||||
at the enumerated removal points. Keeping failed pending entries in
|
||||
engineBusy is sound while their correlation boundary is retained.
|
||||
|
||||
## Verification and acceptable parts
|
||||
|
||||
`timeout 30s node --test packages/discord/tests/engine.test.mjs` completed
|
||||
with **13/13 passing**, 0 failures, about 1.47 seconds. No force-exit was
|
||||
used. The suite's existing assertions do not exercise F1.
|
||||
|
||||
The withEngine finally cleanup, empty command-log fallback, and polling
|
||||
instead of a fixed startup sleep address the reported test cleanup/race
|
||||
defects. Those changes do not require reversal. I did not repeat Darkwing's
|
||||
union/eight-suite runs after finding the blocking integrity failure; broader
|
||||
green results cannot cover the missing event schedule.
|
||||
|
||||
Only this report was added to the repository. The targeted suite created
|
||||
its usual isolated temporary fixtures; the additional reproducer below is
|
||||
entirely in memory. No source edits, commits, credentials, provider calls,
|
||||
connector restart, or live-state changes were made. R1 is **not approved**
|
||||
for the pending commit/restart gate.
|
||||
|
||||
## Standalone in-memory reproducer
|
||||
|
||||
Run from the canonical repository against the pinned engine with
|
||||
`node --input-type=module` (stdin). It asserts the unsafe observed result;
|
||||
after a fix this assertion must no longer hold.
|
||||
|
||||
```js
|
||||
import { EventEmitter } from 'node:events';
|
||||
import { PassThrough } from 'node:stream';
|
||||
import assert from 'node:assert/strict';
|
||||
import { createEngine } from './packages/discord/src/engine-pi.mjs';
|
||||
const timers = [], commands = [];
|
||||
const child = new EventEmitter();
|
||||
child.stdout = new PassThrough(); child.stderr = new PassThrough();
|
||||
child.stdin = {
|
||||
write(s) { commands.push(JSON.parse(s)); return true; }, end() {}
|
||||
};
|
||||
child.kill = () => { child.emit('exit', 0, null); return true; };
|
||||
const engine = createEngine({
|
||||
command: 'memory-only', args: [], spawn: () => child, abortGraceMs: 150,
|
||||
setTimeoutImpl(fn, ms) {
|
||||
const t = { fn, ms, active: true }; timers.push(t); return t;
|
||||
},
|
||||
clearTimeoutImpl(t) { t.active = false; }
|
||||
});
|
||||
const emit = x => child.stdout.write(JSON.stringify(x) + '\n');
|
||||
const fire = ms => {
|
||||
const t = timers.find(t => t.ms === ms && t.active);
|
||||
assert.ok(t); t.active = false; t.fn();
|
||||
};
|
||||
const message = text => ({
|
||||
role: 'assistant', content: [{ type: 'text', text }], stopReason: 'stop'
|
||||
});
|
||||
const finish = text => {
|
||||
emit({ type: 'turn_end', message: message(text) });
|
||||
emit({ type: 'agent_end', messages: [message(text)] });
|
||||
emit({ type: 'agent_settled' });
|
||||
};
|
||||
engine.start();
|
||||
try {
|
||||
const first = engine.prompt('old', { timeoutMs: 50 })
|
||||
.catch(e => e.details.code);
|
||||
emit({ type: 'response', id: commands[0].id,
|
||||
command: 'prompt', success: true });
|
||||
await Promise.resolve();
|
||||
fire(50); assert.equal(await first, 'timeout');
|
||||
const next = engine.prompt('new', { timeoutMs: 2000 });
|
||||
fire(150);
|
||||
emit({ type: 'agent_start' });
|
||||
emit({ type: 'tool_execution_start', toolCallId: 'old-call',
|
||||
toolName: 'read_file', args: { root: 'docs', path: 'old.md' } });
|
||||
emit({ type: 'tool_execution_end', toolCallId: 'old-call',
|
||||
toolName: 'read_file', result: {
|
||||
details: { root: 'docs', path: 'old.md', ok: true }
|
||||
} });
|
||||
finish('OLD RUN ANSWER');
|
||||
const second = commands.filter(c => c.type === 'prompt')[1];
|
||||
emit({ type: 'response', id: second.id,
|
||||
command: 'prompt', success: true });
|
||||
emit({ type: 'agent_start' }); finish('NEW RUN ANSWER');
|
||||
const r = await next;
|
||||
assert.equal(r.text, 'OLD RUN ANSWER');
|
||||
assert.equal(r.tools[0].path, 'old.md');
|
||||
console.log({ reproduced: true, returned: r.text,
|
||||
tools: r.tools.map(t => t.path), busy: engine.busy });
|
||||
} finally { await engine.stop(); }
|
||||
```
|
||||
|
||||
Observed output: `reproduced: true`, `returned: 'OLD RUN ANSWER'`,
|
||||
`tools: ['old.md']`, `busy: false`.
|
||||
@@ -0,0 +1,85 @@
|
||||
# Discord 6b R2 review
|
||||
|
||||
Verdict: **approve** the pinned source change. Rocko, 2026-09-26.
|
||||
No blocking source findings. The restart-rate qualification below matters
|
||||
operationally; this approval does not claim an unconditional restart cap.
|
||||
|
||||
Base: `401cc850`. Verified before review and again after tests:
|
||||
|
||||
- engine-pi.mjs: `77077b7fbd5a933ffd352094eb073227c299ba47b7aea52d4e60fdc55cc7101e`
|
||||
- engine.test.mjs: `47a998179c6eb46827f43ab2c6b0f6b062ef94fb47da402cfb9af8c4f180f38e`
|
||||
- fake-pi.mjs: `a8e54cc3f4b670eef2c06755b63e9e6bfeb44b1b1efde3aca9bfaa91583c3ef3`
|
||||
- r2.patch: `4307a879f469d2b032ea6f90d1803a6085c5e949c8f88c34329c45e6501fef39`
|
||||
- README.md: `69350f299f62a1cbd9283e994ead998ca3e8669eae75d15c6b12fa07036bb23e`
|
||||
|
||||
## R1 F1 is closed
|
||||
|
||||
Grace expiry marks the child wedged before failing held turns or beginning
|
||||
termination. prompt, write and sendHeld all refuse that child thereafter.
|
||||
The old tombstone survives grace expiry. A late agent_end may remove it
|
||||
before exit, but the irreversible wedged flag still prevents any new live
|
||||
head or command. Thus the old answer and tool evidence have no new prompt
|
||||
to settle. Late accepted responses likewise cannot reopen sending.
|
||||
|
||||
The tests exercise both prompt-response timings with deterministic timers,
|
||||
late start/tools/end/settle while the child is still alive, exact write
|
||||
history (old plus abort), held engine-wedged, new engine-down, TERM then
|
||||
KILL, and onExit. The real-child stall and mute cases also pass. The
|
||||
started-run case proves the grace does not kill a run already observed
|
||||
running. Cleanup releases grace timers when pending turns leave.
|
||||
|
||||
The no-start bound is 30 seconds after client-side failure to quarantine,
|
||||
then a TERM attempt and KILL after five seconds if exit has not arrived.
|
||||
Held prompts may fail earlier at their own deadlines. Actual process exit
|
||||
still depends on the OS. Started runs that never settle retain the disclosed
|
||||
HEAD behavior: later prompts time out individually, without a new engine
|
||||
watchdog. This is an honest bounded change, not a claim of total liveness.
|
||||
|
||||
## Operational findings requested by Sage
|
||||
|
||||
1. **Wedge selects exit 1, not 3.** Engine exit invokes cli.mjs onExit,
|
||||
which calls shutdown(1). Shutdown waits for connector.stop and then
|
||||
process.exit(exitCode); the connector waits for failed turn handlers and
|
||||
delivery work and stops the already-exited engine. Exit 3 is the separate
|
||||
supervised startup refusal, not this wedge path. If an operator shutdown
|
||||
or other fatal shutdown began first, the existing first-shutdown-wins
|
||||
guard retains that earlier code; it still does not turn a wedge into 3.
|
||||
A later supervised startup can independently refuse with 3 if STOP or a
|
||||
held binding is present, which is the intended operator brake.
|
||||
|
||||
2. **Medium operational qualification: the start limit is rate-based.**
|
||||
The template sets Restart=on-failure, RestartSec=15,
|
||||
StartLimitIntervalSec=600, StartLimitBurst=5 and
|
||||
RestartPreventExitStatus=3. Rapid failures that exhaust that start budget
|
||||
stop automatic restarting. Repeated wedges do NOT necessarily exhaust
|
||||
it. The default turn timeout is 180 seconds; adding 30 seconds grace and
|
||||
15 seconds restart delay gives at least 225 seconds per cycle, even
|
||||
before request arrival, shutdown work or KILL delay. Such cycles remain
|
||||
below five starts per 600 seconds and may continue indefinitely.
|
||||
Therefore “every repeatedly wedging pi eventually stays down” is not
|
||||
established by this unit. This is an existing unit policy limitation,
|
||||
not a defect in R2's event isolation. If a cumulative wedge circuit
|
||||
breaker is required, specify it separately; do not treat this rate limit
|
||||
as one. No live unit was started or fault-injected for this review.
|
||||
|
||||
3. **Recommended follow-up: journal startup source identity.** Yes: record
|
||||
repository HEAD, dirty status scoped to runtime source, and preferably a
|
||||
digest of the actual runtime files along with startup time and context
|
||||
digest. A commit id alone conceals uncommitted source; a dirty boolean
|
||||
alone cannot identify which candidate ran. Do not log source contents or
|
||||
secret values. The live-checkout process rule remains necessary, because
|
||||
startup fingerprints do not prevent an edit during module loading or a
|
||||
later dynamic read. Restart-on-failure was already configured for other
|
||||
failures; R2 adds a new way to reach it. This follow-up is nonblocking.
|
||||
|
||||
## Independent verification
|
||||
|
||||
`timeout 30s node --test packages/discord/tests/engine.test.mjs`:
|
||||
17 passed, zero failed, exit 0. Manifest checks passed before and after.
|
||||
Reviewed the actual CLI shutdown and connector.stop paths and the versioned
|
||||
unit template. Author-reported 406-test repetitions and eight suites were
|
||||
not rerun or represented as independent evidence.
|
||||
|
||||
Only this report was written. No source edits, commit, restart, live message
|
||||
or service-policy change. Approval is for the pinned R2 source; integration
|
||||
and any authorized restart remain with the lead.
|
||||
Reference in New Issue
Block a user