Skip to content

fix(reliability): stop Windows mid-run deaths (keep-awake + real --detach) - #2063

Merged
Wirasm merged 5 commits into
coleam00:devfrom
buun-dev:feat/keep-awake-during-runs
Jul 13, 2026
Merged

fix(reliability): stop Windows mid-run deaths (keep-awake + real --detach)#2063
Wirasm merged 5 commits into
coleam00:devfrom
buun-dev:feat/keep-awake-during-runs

Conversation

@buun-dev

@buun-dev buun-dev commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Describe this PR in 2-5 bullets:

  • Problem: On unattended Windows hosts, workflow runs die or stall mid-DAG. Two distinct failure modes: (1) long in-flight runs get frozen by Modern Standby when the screen turns off — on thaw, every new bash child the executor spawns exits 66 (EX_NOINPUT) with no stdout/stderr, collapsing the whole CI/ledger tail and leaving failed/zombie running rows; (2) archon workflow run --detach doesn't actually detach — the "background" child dies ~1s in at worktree_creating, forcing a manual finish. Historically ~44% of runs never completed (209 completed / 82 cancelled / 81 failed at one capture).
  • Why it matters: This is the single biggest blocker to unattended automation. You can't leave a dispatch loop running overnight if a mid-run death silently parks work until a human notices and hand-recovers it (flip the zombie row to paused, archon workflow approve, or manual-finish from the worktree).
  • What changed: Two complementary hardening measures + their root-cause dossiers. (1) keep-awake — hold a per-thread SetThreadExecutionState(ES_CONTINUOUS | ES_SYSTEM_REQUIRED) request for exactly the executing window so the system stays awake while ≥1 run is active (refcounted across concurrent runs; best-effort no-op off Windows / on FFI failure). (2) --detach fix — replace Bun.spawn(...) + unref() with Node's spawn(..., { detached: true, windowsHide: true }) so the detached child becomes a new process-group leader and survives its launcher's teardown.
  • What did NOT change (scope boundary): buildDetachedRunCmd (the pure, unit-tested argv builder) is untouched — only the spawn mechanism changes. No schema, DB, config, or env changes. keep-awake never blocks a run: acquire/release wrap the run's try/finally, failures are swallowed, and the OS clears the execution state on process exit.

Note on the two docs' differing root causes (read before reviewing): these are two independent investigations of the same "mid-run death" class, done two days apart, and they land on different primary causes for the exit-66 signature. docs/2026-07-05-archon-mid-run-death-problem-record.md (+ its evening addendum) traces the exit-66 bash-tail collapse to Windows Modern Standby. docs/2026-07-07-archon-detach-fix.md traces the ~1s detached-run death to Bun.spawn never detaching, and explicitly rules sleep out for that specific failure. They are not in conflict about the fix: they describe complementary failure modes — keep-awake protects long in-flight runs from standby freeze; the detach fix protects backgrounded runs from launcher teardown. Both are defensive; neither depends on the other. We've kept both dossiers verbatim so reviewers can see the full evidence trail rather than a sanitized single narrative.

UX Journey

Before

Operator                 Archon (Windows, unattended)
────────                 ────────────────────────────
dispatch --detach ─────▶ Bun.spawn + unref()  (NOT a new process group)
walk away                launching shell tears down
                         └─▶ child KILLED ~1s in at worktree_creating
                             (log stops dead mid-line, no error)
                         → run never starts → manual finish required

  ── or, for a long in-flight run ──

dispatch (foreground)──▶ executor enters a long AI / bash node
screen turns off ──────▶ host drops into Modern Standby, executor frozen
                         on thaw: every new bash child → exit 66, no output
                         → CI/ledger tail collapses → failed / zombie 'running'
sees stuck run  ◀─────── human flips row to paused, approve/resume/manual-finish
(hours later)

After

Operator                 Archon (Windows, unattended)
────────                 ────────────────────────────
dispatch --detach ─────▶ *node spawn(detached:true, windowsHide:true)*
walk away                launcher tears down → *child survives (own group)*
                         └─▶ worktree_created → bootstrap → ... → commit → PR

  ── and for any active run ──

run starts ───────────▶ *acquire keep-awake (ES_CONTINUOUS|ES_SYSTEM_REQUIRED)*
screen turns off ──────▶ *system stays awake — no standby freeze*
run completes ────────▶ *release keep-awake (last run out drops the request)*
sees finished PR ◀───── no hand-recovery needed

Architecture Diagram

Before

@archon/cli
  workflow.ts
    spawnDetachedWorkflowRun ──uses──▶ Bun.spawn + child.unref()   [no detach]

@archon/workflows
  executor.ts
    executeWorkflow ── runs DAG ──▶ dag-executor ──▶ bash/AI nodes  [no sleep guard]

After

@archon/cli
  workflow.ts
    spawnDetachedWorkflowRun ──uses──▶ [~] node:child_process spawn(detached:true)

@archon/workflows
  executor.ts
    executeWorkflow
      ├── [+] acquireKeepAwake() ═══▶ [+] utils/keep-awake.ts ──ffi──▶ kernel32
      │                                                        SetThreadExecutionState
      ├── runs DAG ──▶ dag-executor ──▶ bash/AI nodes
      └── finally: [+] releaseKeepAwake()   (refcounted; last-out clears)

Connection inventory:

From To Status Notes
cli/workflow.ts:spawnDetachedWorkflowRun Bun.spawn removed replaced by Node spawn
cli/workflow.ts:spawnDetachedWorkflowRun node:child_process spawn new detached: true, windowsHide: true
cli/workflow.ts:spawnDetachedWorkflowRun buildDetachedRunCmd unchanged argv builder untouched
workflows/executor.ts:executeWorkflow utils/keep-awake.ts new acquire above run try, release first in finally
workflows/utils/keep-awake.ts kernel32!SetThreadExecutionState (bun:ffi) new Windows-only; best-effort no-op elsewhere

Label Snapshot

  • Risk: risk: medium — changes the process-spawn lifecycle for every --detach run on all platforms and wraps every executeWorkflow with acquire/release; mitigated by mirroring an in-repo proven pattern and best-effort no-op semantics.
  • Size: size: M (~270 lines code across 3 files; ~450 lines of root-cause docs)
  • Scope: workflows, cli, docs
  • Module: workflows:executor, workflows:keep-awake, cli:workflow

Change Metadata

  • Change type: bug (primary: stop mid-run deaths) — includes one new capability (keep-awake) + docs
  • Primary scope: multi

Linked Issue

  • Closes # (none — no existing issue tracks this failure class; happy to file one if maintainers prefer a tracking issue)
  • Related # (n/a)
  • Depends on # (n/a)
  • Supersedes # (n/a)

Validation Evidence (required)

Targeted checks for the touched packages (@archon/cli, @archon/workflows):

bun x tsc --noEmit -p packages/cli
# → clean, exit 0

bun test packages/cli/src/commands/workflow.test.ts \
         packages/workflows/src/utils/keep-awake.test.ts
# → 152 pass, 0 fail, 294 expect() calls

bun x eslint packages/cli/src/commands/workflow.ts
# → clean, exit 0 (also enforced by the pre-commit lint-staged hook)

bun run format:check
# → All matched files use Prettier code style!

Full-suite bun run validate:

bun run validate
# All 7 pre-test gates PASS (it is a sequential && chain, so reaching `test`
# proves each passed): check:bundled · check:bundled-skill · check:bundled-schema
# · check:pi-vendor-map · type-check · lint (--max-warnings 0) · format:check
#
# test stage: the only 2 failures are pre-existing, ENVIRONMENT-SPECIFIC and in
# code this PR does not touch — both are symlink-setup tests that this Windows
# host cannot run without Developer Mode:
#   - @archon/providers  binary-resolver.test.ts "returns 'missing' for a broken
#                        symlink"        → EPERM: operation not permitted, symlink
#   - @archon/workflows  "resolves a symlinked home command"  → same EPERM cause
# (the --parallel runner then SIGINTs the remaining packages.)

Touched-package suites, run in isolation, are fully green:

bun --filter '@archon/workflows' test   # 651 pass, 1 fail (only the EPERM symlink test above)
bun --filter '@archon/cli' test         # 18 pass, 0 fail
  • Evidence provided (test/log/trace/screenshot): command output above; the --detach root-cause is evidenced by a real detach log frozen mid-line at worktree_creating (see docs/2026-07-07-archon-detach-fix.md); the standby root-cause is evidenced by 7/7 exit-66 runs correlated to Kernel-Power 506/507 events (see the problem-record addendum).
  • If any command is intentionally skipped, explain why: none skipped. The two bun run validate test failures are unrelated Windows symlink-privilege (EPERM) failures in untouched packages, not regressions from this PR — they reproduce independent of these commits and require OS Developer Mode to create symlinks. CI (Linux) runs these symlink tests normally.

Security Impact (required)

  • New permissions/capabilities? Yes — keep-awake calls kernel32!SetThreadExecutionState via bun:ffi (Windows) to inhibit sleep. This is an OS power-state hint only; it grants no new access and is a best-effort no-op on failure or off-Windows. The --detach change swaps Bun.spawn for node:child_process spawn running the same argv — no new capability.
  • New external network calls? No.
  • Secrets/tokens handling changed? No.
  • File system access scope changed? No.
  • If any Yes, describe risk and mitigation: SetThreadExecutionState only signals "keep the system awake while a run is active"; it cannot read/write data or escalate privilege. Wrapped in try/catch; OS auto-clears the state on process exit.

Compatibility / Migration

  • Backward compatible? Yes.
  • Config/env changes? No.
  • Database migration needed? No.
  • If yes, exact upgrade steps: n/a.

Human Verification (required)

What was personally validated beyond CI:

  • Verified scenarios: type-check (cli), lint (workflow.ts), format check, and unit tests (152 pass) all green in this session. Root causes are code-/log-evidenced in both dossiers.
  • Edge cases checked (documented): keep-awake refcounting for concurrent runs and best-effort no-op off Windows / on FFI failure are covered by keep-awake.test.ts; buildDetachedRunCmd argv builder unchanged and still passing.
  • What was not verified in this session: the live --detach survival smoketest (a real detached run continuing past worktree_creating — this is OS process-detachment behavior, not unit-testable; procedure is documented in 2026-07-07-archon-detach-fix.md Task 2) and a real Modern-Standby cycle with keep-awake held. These require an unattended Windows host and are best confirmed by a maintainer on target hardware.

Side Effects / Blast Radius (required)

  • Affected subsystems/workflows: all detached runs (--detach spawn path, every platform); every executeWorkflow invocation (keep-awake acquire/release wrapper — a no-op on non-Windows).
  • Potential unintended effects: a regression in the detached spawn would surface as detached runs failing to start; a keep-awake bug is contained by best-effort try/catch (falls back to prior behavior — the OS may sleep, but the run is never blocked).
  • Guardrails/monitoring for early detection: detached-run logs should now progress past worktree_creating; keep-awake acquire/release is refcounted with release as the first statement of the finally, so no path can leak an unpaired acquire; the OS clears execution state on process exit as a final backstop.

Rollback Plan (required)

  • Fast rollback command/path: git revert 2800552b ae233ea7 (detach fix + keep-awake). To revert only the detach fix: git checkout <base> -- packages/cli/src/commands/workflow.ts. The edits run from source, so a revert is effective on the next archon invocation with no rebuild.
  • Feature flags or config toggles (if any): none; keep-awake self-disables off Windows / on FFI failure.
  • Observable failure symptoms: detached runs dying ~1s in at worktree_creating (detach regression); host sleeping mid-run / exit-66 bash-tail collapse (keep-awake not holding).

Risks and Mitigations

  • Risk: the detached-spawn change alters process lifecycle for --detach on all platforms.
    • Mitigation: mirrors setup.ts's already-proven spawn(..., { detached: true }) pattern; buildDetachedRunCmd (argv) is unchanged and unit-tested; a start /b Job-Object fallback is documented in the detach doc (Task 3) if detached alone proves insufficient on some shells.
  • Risk: keep-awake FFI behaves unexpectedly on non-standard Windows environments.
    • Mitigation: best-effort — wrapped in try/catch, no-op on any failure, and refcount-released in finally; never blocks or fails a run.
  • Risk: the two dossiers' differing root-cause conclusions could confuse reviewers.
    • Mitigation: called out explicitly at the top of this PR — they address complementary failure modes and are both defensive; kept verbatim for evidence transparency.

Summary by CodeRabbit

  • New Features

    • Windows workflow runs now automatically inhibit system sleep during active execution (best-effort native support), while still allowing the display to turn off.
  • Bug Fixes

    • Detached workflow startup now fails fast when the child can’t be started, and avoids reporting success prematurely.
    • Detached run output more reliably reflects log-capture availability, warning when logs can’t be opened instead of implying they are captured.
  • Documentation

    • Added Windows deployment guidance for “Sleep During Workflow Runs (Native Windows Only)”.
  • Tests

    • Expanded coverage for keep-awake behavior and detached process start/error paths.

buun-dev and others added 3 commits July 5, 2026 19:32
Unattended Windows hosts drop into Modern Standby when the screen turns
off. A standby-frozen executor thaws into bash spawns that exit 66 with
no output, collapsing the DAG tail; other runs become zombie 'running'
rows. Hold a per-thread SetThreadExecutionState(ES_CONTINUOUS |
ES_SYSTEM_REQUIRED) request for exactly the executing window so the
system stays awake while at least one run is active.

Refcounted so concurrent runs in one server process share a single
request until the last finishes. Best-effort: no-op off Windows or on
FFI failure, since a keep-awake failure must never block a run and the
OS clears the state on process exit. Acquire sits directly above the
run's try so early-return paths can't leak an unpaired acquire; release
is the first statement of that finally so it pairs on every exit path.
Reference dossier cataloging the Windows Modern Standby failures
(exit-66 bash-tail collapse, zombie 'running' runs) that motivate the
keep-awake change. Captures the evidence, incidences, and
confirmed-vs-hypothesized causes in one place.
spawnDetachedWorkflowRun used Bun.spawn(...) + child.unref(). unref()
stops the parent waiting, but does NOT put the child in its own process
group, so when the launching shell/console tore down (right after the
parent returned) Windows killed the "detached" child with it — the run
died ~1s in at worktree_creating and forced a manual finish. Evidence:
detach logs stop dead mid-line at worktree_creating with no error.

Switch to node:child_process spawn(cmd[0], cmd.slice(1), { detached:
true, windowsHide: true }). detached makes the child a new process-group
leader so it survives the parent's exit; mirrors setup.ts's proven
spawn(..., { detached: true }) pattern already in this codebase. Only the
spawn mechanism changes — buildDetachedRunCmd (the unit-tested argv
builder) and the log-fd handling are untouched.

Docs: 2026-07-07-archon-detach-fix.md records the root cause, the live
verification path (a --detach run that survives past worktree_creating),
and the start /b Job-Object fallback if detached alone proves insufficient.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 28fb2c64-3504-4db0-910a-50fe67c5c2e5

📥 Commits

Reviewing files that changed from the base of the PR and between a614ebb and 8163167.

📒 Files selected for processing (4)
  • packages/cli/src/commands/workflow.test.ts
  • packages/cli/src/commands/workflow.ts
  • packages/workflows/src/executor.test.ts
  • packages/workflows/src/executor.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/cli/src/commands/workflow.test.ts
  • packages/workflows/src/executor.test.ts

📝 Walkthrough

Walkthrough

Changes

Workflow keep-awake lifecycle

Layer / File(s) Summary
Keep-awake controller and native binding
packages/workflows/src/utils/keep-awake.ts, packages/workflows/src/utils/keep-awake.test.ts
Adds a refcounted Windows SetThreadExecutionState controller with best-effort native loading, failure handling, singleton wiring, and transition tests.
Executor lifecycle wiring
packages/workflows/src/executor.ts, packages/workflows/src/executor.test.ts, packages/docs-web/src/content/docs/deployment/windows.md
Acquires keep-awake after early validation, releases it first in finally, tests success and failure paths, and documents native Windows behavior.

Detached workflow spawning

Layer / File(s) Summary
Detached child-process launch and validation
packages/cli/src/commands/workflow.ts, packages/cli/src/commands/workflow.test.ts
Uses Node’s detached spawn, preserves environment and log handling, validates the child PID, adjusts output messaging, and tests startup failure behavior.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

Keep-awake lifecycle

sequenceDiagram
  participant workflowRun as executeWorkflow
  participant keepAwake
  participant kernel32
  workflowRun->>keepAwake: acquire()
  keepAwake->>kernel32: SetThreadExecutionState(acquire flags)
  workflowRun->>workflowRun: execute workflow
  workflowRun->>keepAwake: release() in finally
  keepAwake->>kernel32: SetThreadExecutionState(clear flags)
Loading

Detached workflow launch

sequenceDiagram
  participant command as workflowRunCommand
  participant child as Node child process
  participant log as conversation log
  command->>child: spawn command with cwd, env, and detached options
  child->>log: redirect stdout and stderr
  child-->>command: pid or spawn error
  command-->>command: success acknowledgment or startup failure
Loading

Possibly related issues

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main reliability fix: Windows keep-awake plus a true detached spawn for workflow runs.
Description check ✅ Passed The description matches the template well, covering summary, UX, architecture, validation, risks, rollback, and other required sections.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/2026-07-07-archon-detach-fix.md`:
- Around line 118-131: The verification step is parsing CLI output with brittle
text matching, which can break if formatting changes. Update the Task 2 commands
to use a JSON-aware parser for the run ID and status checks instead of grep/cut,
and keep the logic anchored around the workflow runs and workflow get commands
so the checks remain deterministic even if output formatting shifts.

In `@packages/cli/src/commands/workflow.ts`:
- Around line 195-208: The detached child created in workflow.ts via spawn() can
still emit an asynchronous error before or after unref(), which may crash the
parent CLI if unhandled. Add an error listener on the ChildProcess immediately
after creation and before calling child.unref(), and log the spawn failure with
enough context for ENOENT or invalid cwd cases instead of allowing an uncaught
exception.

In `@packages/workflows/src/utils/keep-awake.ts`:
- Around line 67-101: Catch FFI exceptions in createKeepAwake so errors from
native(ACQUIRE_FLAGS) and native(RELEASE_FLAGS) cannot escape into executor.ts
and abort a workflow run. Update the acquire() and release() methods to wrap the
native calls in try/catch, keep the refcount behavior intact, and log the
failure through getLog() with clear symbols like createKeepAwake, acquire, and
release.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7184d01c-5e21-4bf2-a724-8ddb3705fb1e

📥 Commits

Reviewing files that changed from the base of the PR and between 4dcc998 and 2800552.

📒 Files selected for processing (6)
  • docs/2026-07-05-archon-mid-run-death-problem-record.md
  • docs/2026-07-07-archon-detach-fix.md
  • packages/cli/src/commands/workflow.ts
  • packages/workflows/src/executor.ts
  • packages/workflows/src/utils/keep-awake.test.ts
  • packages/workflows/src/utils/keep-awake.ts

Comment thread docs/2026-07-07-archon-detach-fix.md Outdated
Comment on lines +118 to +131
- [ ] **Step 2: Wait ~90s, then confirm the child SURVIVED past the death point.** Tail the newest detach log:

```bash
newest=$(ls -t ~/.archon/logs/detached-run-*.log | head -1); echo "$newest"; tail -20 "$newest"
```

**PASS** = the log continues well past `worktree_creating` (you'll see `worktree_created`, `bootstrap`, migrations, etc.). **FAIL** = it still stops dead at `worktree_creating` → go to Task 3.

- [ ] **Step 3: Corroborate via the run row.**

```bash
rid=$(archon workflow runs --json --limit 1 | grep -oE '"id":"[a-f0-9]+"' | head -1 | cut -d'"' -f4)
archon workflow get "$rid" --json | grep -E '"status"|"last_activity_at"'
```

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Make the verification step parse JSON structurally.

The current ls | grep | cut workflow is brittle and can break if the CLI output formatting changes. Please use a JSON-aware parser for the run ID and status checks so Task 2 stays deterministic.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/2026-07-07-archon-detach-fix.md` around lines 118 - 131, The
verification step is parsing CLI output with brittle text matching, which can
break if formatting changes. Update the Task 2 commands to use a JSON-aware
parser for the run ID and status checks instead of grep/cut, and keep the logic
anchored around the workflow runs and workflow get commands so the checks remain
deterministic even if output formatting shifts.

Comment thread packages/cli/src/commands/workflow.ts
Comment thread packages/workflows/src/utils/keep-awake.ts

@Wirasm Wirasm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


pr: 2063
title: "fix(reliability): stop Windows mid-run deaths (keep-awake + real --detach)"
author: "buun-dev"
reviewed: 2026-07-13
recommendation: request-changes

PR Review: #2063 - fix(reliability): stop Windows mid-run deaths (keep-awake + real --detach)

Author: @buun-dev
Branch: feat/keep-awake-during-runs -> dev
Files Changed: 6 (+722/-3)


Summary

Two complementary Windows-reliability fixes: (1) a refcounted keep-awake controller (SetThreadExecutionState via bun:ffi) held for exactly the executing window of executeWorkflow, and (2) replacing Bun.spawn + unref() with Node's spawn(..., { detached: true, windowsHide: true }) so --detach children survive launcher teardown. The code changes are small, well-commented, well-tested, and correctly scoped. Two of CodeRabbit's three findings from the 2026-07-07 review are real and remain unaddressed on the current head (2800552b), and the two ~450-line "dossier" docs are private-environment artifacts that shouldn't land in docs/ verbatim.


Implementation Context

Artifact Path
Implementation Report Not found (external contribution, not via prp-implement)
Original Plan The PR itself ships its plan as docs/2026-07-07-archon-detach-fix.md
Documented Deviations PR body explicitly flags the two dossiers' differing root causes as intentional

The PR body is unusually thorough (root-cause evidence, rollback plan, explicit "not verified" section). The author openly states the live --detach survival smoketest and a real Modern-Standby cycle were not verified — both require an unattended Windows host.


Changes Overview

File Changes Assessment
packages/workflows/src/utils/keep-awake.ts +137 WARN — unguarded native calls contradict stated contract
packages/workflows/src/utils/keep-awake.test.ts +110 PASS — good DI-based coverage, runs in the src/utils/ batch
packages/workflows/src/executor.ts +14/-1 PASS — correct placement (post-validation acquire, release first in finally)
packages/cli/src/commands/workflow.ts +8/-2 WARN — missing 'error' listener on the detached child
docs/2026-07-05-…problem-record.md +244 FAIL — private-environment dossier, dangling references
docs/2026-07-07-archon-detach-fix.md +207 FAIL — task prompt for the author's own agent, private paths

Issues Found

Critical

No critical issues found.

High Priority

  • packages/cli/src/commands/workflow.ts:201-208 — No 'error' listener on the spawned child (unaddressed CodeRabbit finding).
    • Why: Node's spawn() reports ENOENT / EACCES / invalid-cwd asynchronously via an 'error' event. Unhandled, that becomes an uncaught exception that crashes the CLI — after it has already printed Started '<workflow>' in the background. (or ok: true JSON at workflow.ts:672-695). The replaced Bun.spawn threw synchronously, so spawn failures previously surfaced through the CLI's normal error path before any success output. This is a real error-handling regression, and it hits --json consumers hardest (success line emitted, then a stack trace + non-zero exit).
    • Fix: Attach child.on('error', err => getLog().error({ err, cmd: cmd[0] }, 'cli.detached_run_spawn_failed')) immediately after spawn(), before unref(). Printing success-before-confirmation is inherent to async detach, but the crash and the silent log gap are not.

Medium Priority

  • packages/workflows/src/utils/keep-awake.ts:79-97acquire()/release() call the FFI function unguarded (unaddressed CodeRabbit finding).

    • Why: The module header promises "a keep-awake failure must NEVER block or fail a workflow run," and the PR body claims failures are "wrapped in try/catch" — but only dlopen (load time) and the 0 return value are handled. A native call that throws in acquire() escapes executeWorkflow at executor.ts:724 (before the try), leaving the already-created run row stuck running — the exact zombie class this PR fights. A throw in release() at executor.ts:993 (first statement of the finally) skips the zombie backstop and masks the in-flight return value. A bun:ffi u32 call throwing is unlikely in practice, but the stated invariant should be enforced, not assumed.
    • Fix: Wrap both native(...) calls in try/catch, log via getLog().warn(...), keep refcount behavior intact. ~6 lines; the existing tests still pass and a "native throws" test becomes trivial to add.
  • docs/2026-07-05-…problem-record.md + docs/2026-07-07-archon-detach-fix.md — Private-environment dossiers committed as project docs.

    • Why: Both files reference paths that don't exist in this repo (.agents/plans/…, docs/retros/worked-failures.md, .claude/scripts/heartbeat.py, docs/superpowers/specs/…, MarphobBrain/Memory/…) and hardcode the author's machine layout (D:\Project\Archon-template\…). The detach doc is explicitly a task prompt for the author's own agent ("For the human executor… You said you'll run this yourself"). Additionally, this PR creates the root docs/ directory — which is Archon's default $DOCS_DIR, injected into workflow prompts — so these dossiers become ambient "project documentation" for every workflow run that references $DOCS_DIR.
    • Fix: The root-cause evidence is genuinely valuable — preserve it, but not verbatim in docs/. Preferred: move both dossiers into a GitHub issue (the PR body already offers to file one) and, if desired, keep a short distilled root-cause note in the repo. At minimum, strip the private paths/references and the executor-task framing.

Suggestions

  • packages/workflows/src/utils/keep-awake.ts:52-56 — The "no module-load side effects" claim on the lazy logger is slightly undercut by loadNative()'s catch path calling getLog() at import time (Windows FFI-failure case only). Cosmetic; fix the comment or accept it.
  • packages/workflows/src/executor.test.ts — Consider one assertion that keepAwake.activeCount() returns to 0 after executeWorkflow completes (success and failure paths), since the singleton is importable. Cheap insurance against a future early-return between acquire and try.
  • Verification debt (author-acknowledged): the live Windows --detach survival smoketest (detach doc, Task 2) and a real Modern-Standby cycle are still pending. I verified locally that Bun's node:child_process shim honors detached: true on POSIX (child becomes its own process-group leader), so the option is plumbed through — but the Windows DETACHED_PROCESS behavior should be confirmed on target hardware before this is trusted for unattended use. The start /b Job-Object fallback is already documented if it isn't enough.

Validation Results

Run on macOS (Darwin) in a clean worktree at 2800552b:

Check Status Details
Type Check PASS all 10 packages, exit 0
Lint PASS eslint clean (--max-warnings 0)
Format PASS prettier clean
Tests (@archon/workflows) PASS all batches green, incl. 7 keep-awake tests
Tests (@archon/cli) PASS 43 pass / 0 fail
Generated files PASS check:bundled, check:bundled-schema, check:pi-vendor-map all up to date

The two symlink EPERM failures the author reported are Windows-host-specific (Developer Mode) and do not reproduce here — everything is green on macOS, confirming they're environmental, not regressions.


Pattern Compliance

  • Follows existing code structure (utils placement, DI factory + singleton, logger conventions)
  • Type safety maintained (no any, explicit return types, SDK/FFI types via bun:ffi)
  • Naming conventions followed (keepawake.acquire_failed, _completed/_failed pairing)
  • Tests added for new code (keep-awake; picked up by the existing src/utils/ test batch — no mock.module, no isolation concerns)
  • Documentation — the two dossiers need relocation/trimming (see Medium)
  • Fail-fast exception documented per CLAUDE.md ("intentional, documented fallback") — but see the Medium finding: the code doesn't yet fully enforce it

What's Good

  • Exemplary root-cause discipline: exit-66 correlated to Kernel-Power events; the detach failure evidenced by a log frozen mid-line; the two investigations honestly kept separate instead of merged into a false single narrative — and the PR body flags that tension up front.
  • keep-awake design is clean: dependency-injected native fn + platform (testable without Windows), refcounting for concurrent runs, ES_SYSTEM_REQUIRED without ES_DISPLAY_REQUIRED, correct >>> 0 uint32 coercion with an explanation, OS-clears-on-exit as the crash backstop, and the single-thread constraint documented.
  • Executor wiring is exactly right: acquire placed after all early-return validation paths so it can't leak, release as the first statement of the finally with the reasoning written down.
  • Surgical scope: the unit-tested buildDetachedRunCmd argv builder untouched; no schema/config/env changes; honest "what was NOT verified" section and a concrete rollback plan.

Recommendation

REQUEST CHANGES

Three items before merge, all small:

  1. Add the 'error' listener on the detached child (High — real error-handling regression, ~4 lines).
  2. Wrap the FFI calls in acquire()/release() in try/catch to actually enforce the stated never-fail contract (Medium — ~6 lines; also makes the PR body's claim true).
  3. Move the two dossiers out of docs/ (into a tracking issue, ideally with a distilled root-cause note left behind) and strip the private-machine paths/references.

Items 1 and 2 were already raised by CodeRabbit on this same head commit and are still unaddressed. The core approach is sound and the code quality is high — once these land, this is a valuable reliability fix. A maintainer-side Windows smoketest of --detach survival before release would close the remaining verification gap.


Reviewed by Claude
Report: .claude/PRPs/reviews/pr-2063-review.md

@Wirasm

Wirasm commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

PR Review Summary

Multi-agent review (7 specialist agents: code-reviewer, silent-failure-hunter, pr-test-analyzer, comment-analyzer, type-design-analyzer, docs-impact, code-simplifier) of head 2800552b, run in a clean worktree on macOS. Follow-up to the earlier single-reviewer CHANGES_REQUESTED review — findings below include everything the agents surfaced beyond it.

Critical Issues (2 found)

Agent Issue Location
silent-failure-hunter + code-reviewer Unguarded FFI calls × placement: a throw from acquire() (called before the try) bypasses failWorkflowRun, telemetry, workflow_failed event, and user notification — leaving a zombie running row, the exact class this PR fights; a throw from release() (first statement of finally) replaces the function's return value (even a successful {success: true}) and skips the #1561 zombie backstop two lines below it packages/workflows/src/utils/keep-awake.ts:79,94packages/workflows/src/executor.ts:724,993
pr-test-analyzer Missing 'error' listener on the detached child — empirically reproduced: spawn() returns without throwing on a bad executable path, the CLI prints ok: true / "Started … in the background", then the parent crashes with an uncaught ENOENT (exit 1). Machine callers of --json proceed as though a run is in flight when none exists packages/cli/src/commands/workflow.ts:201-208

Important Issues (7 found)

Agent Issue Location
silent-failure-hunter release() discards the native call's return value entirely (asymmetric with acquire(), which checks === 0): a failed clear leaves ES_SYSTEM_REQUIRED asserted for the life of the server process — the host can never sleep again — while logging release_completed as success packages/workflows/src/utils/keep-awake.ts:94
pr-test-analyzer The existing CLI spawn tests capture the spawn options but only assert .cwd/.cmd — nothing asserts detached: true / windowsHide: true, so the single behavioral change this PR exists to make has zero regression protection (one-line fix, the spy is already in place) packages/cli/src/commands/workflow.test.ts:2259-2273
silent-failure-hunter Log-open failure compounds into a zero-trace failure: stdio degrades to all-ignore, human mode prints "Started … in the background" with no warning, JSON asserts ok: true — no log, no DB row, no exit code, no signal anywhere packages/cli/src/commands/workflow.ts:182-193,672-696
code-reviewer "Mirrors setup.ts's proven pattern" copies only the options, not the technique: setup.ts's trySpawn() wraps the call in try/catch and checks child.pid to detect a silently-failed spawn (setup.ts:1738-1753) — the solution to the missing-error-detection problem already exists one file away packages/cli/src/commands/workflow.ts:196-208
docs-impact The dossiers are personal operational notes for a different deployment: gh auth switch --user buun-dev, tasks against an unrelated repo (marphob-page) and a personal vault (MarphobBrain/Memory/…), hard-coded D:\Project\Archon-template. Keep the evidence in the PR/issue; don't merge as durable docs/ content docs/2026-07-07-archon-detach-fix.md:19,109-113,188,195
docs-impact Missing user-facing doc: keep-awake is a real observable behavior change on native Windows (machine won't sleep while a run is active, no config toggle) — 2-3 lines belong in the existing Windows-quirks page packages/docs-web/src/content/docs/deployment/windows.md (~line 72)
comment-analyzer Comments promise what the code doesn't enforce: module header "must NEVER block or fail a workflow run" (unguarded calls can); "no module-load side effects" (loadNative's catch invokes the logger at import time); "proven pattern … so it survives the parent's exit" (the PR's own companion doc hedges with the Job-Object caveat and an unshipped start /b fallback) packages/workflows/src/utils/keep-awake.ts:12-15,47; packages/cli/src/commands/workflow.ts:196-200

Suggestions (8 found)

Agent Suggestion Location
pr-test-analyzer + code-reviewer Executor integration test: spyOn(keepAwake, 'acquire'/'release') (safe on non-Windows CI — native stays undefined) asserting exactly-once pairing on success, thrown-error, and early-return paths; plus a throwing-native-fn unit test (fails against current code, proving the gap) packages/workflows/src/executor.test.ts, keep-awake.test.ts
silent-failure-hunter acquire_failed warn can't carry a workflowRunId (module is context-free by design) — return a boolean from acquire() so the executor can log workflow.keepawake_unprotected with run context; release_unbalanced warn has an empty {} context (add a stack so it's debuggable) keep-awake.ts:80-89
pr-test-analyzer Existing spawn tests spy on Bun.spawn — they only still pass because Bun's node:child_process shim happens to delegate to it (verified empirically; undocumented internal). Prefer mock.module('node:child_process', …) matching the actual import (safe: this file already runs in its own isolated bun test invocation) packages/cli/src/commands/workflow.test.ts:2246,2293
comment-analyzer "Bun runs all JS on one thread" overgeneralizes (worker_threads exist) — restate as "this codebase never runs workflow execution in a Worker" keep-awake.ts:133-135
code-simplifier The best-effort rationale is written out in full three times (module doc, loadNative doc, executor call site) — state once, reference elsewhere; each call site keeps only its unique fact (placement rationale / bun:ffi-resolves-everywhere) keep-awake.ts:103-112,129-136; executor.ts:715-723
code-simplifier Optional: makeKeepAwake(platform?, returnValue?) test helper — the same 2-line construction repeats in 6 of 7 tests, matching the sibling files' makeX() convention keep-awake.test.ts
type-design-analyzer activeCount() is doc-commented "for tests and diagnostics" but has zero production/diagnostic callers — tighten to "for tests" (do NOT split the interface; not worth the ceremony) keep-awake.ts:44
docs-impact Discretionary CLAUDE.md candidate: "children that must outlive the parent use node:child_process spawn(..., {detached: true}), not Bun.spawn().unref()" — same class of steering rule as the existing execFileAsync-not-exec bullet; judgment call with only two call sites today CLAUDE.md

Strengths

  • The refcounted controller design is right-sized: type-design rates it 7.5/10 overall (encapsulation 9/10) and explicitly advises against a withKeepAwake/Disposable wrapper as overengineering for a single call site; the injectable platform param is a good test seam, not a smell.
  • The executor placement comments were verified against actual control flow and are accurate: every early return sits above the acquire, and release is literally the first statement of the finally.
  • keep-awake.test.ts is genuinely strong behavioral coverage of the refcounting core (nested acquire, 2→1 vs 1→0, re-acquire, unbalanced release, native-returns-0, both disabled paths) — DI'd fake, no mock.module, no pollution risk.
  • The lazy-logger idiom matches the sibling convention (condition-evaluator.ts, dag-executor.ts, etc.) — module-level createLogger would be the outlier here.
  • The >>> 0 uint32 coercion comment and the ES_CONTINUOUS-alone-is-the-release comment were both verified correct and are exactly the kind of non-obvious "why" worth writing down.

Documentation Issues

  • docs/ (new top-level dir) — don't establish it for personal dossiers; this repo's own .archon/config.yaml overrides docs.path to packages/docs-web/src/content/docs, so $DOCS_DIR doesn't even resolve here (a correction to the earlier review's claim that it does), which makes the folder pure drift next to the real docs site.
  • packages/docs-web/src/content/docs/deployment/windows.md — add the keep-awake behavior note (see Important).
  • packages/docs-web/src/content/docs/reference/cli.md:207 — checked: --detach description remains accurate; no stale docs from the spawn-mechanism change.

Verdict

NEEDS FIXES

Recommended Actions

  1. Wrap the native(...) calls in acquire()/release() in try/catch AND check release()'s return value — this closes both Critical Model stucked at response stream text #1 and Important Model stucked at response stream text #1 in ~10 lines and makes the module's own "never fail a run" contract true.
  2. Add the 'error' listener (or the trySpawn-style child.pid check from setup.ts) to spawnDetachedWorkflowRun, and surface the no-log-file case instead of printing unconditional success.
  3. Assert detached/windowsHide in the existing spawn tests (one line) and add the executor pairing + throwing-native tests.
  4. Move the two dossiers out of docs/ (tracking issue preferred); add the 2-3-line Windows keep-awake note to the docs site.
  5. Align the three overclaiming comments with what the code actually guarantees (or make the code guarantee it — action 1 does most of this).

Multi-agent review dispatched via prp-review --agents; all agents advisory, no files modified.

Review follow-ups on coleam00#2063 (CodeRabbit + multi-agent review):

- keep-awake: wrap both native SetThreadExecutionState calls in try/catch
  so a throwing FFI call can never escape into executeWorkflow (acquire
  sits before the run's try — a throw left a zombie 'running' row; release
  is first in its finally — a throw replaced the run's return value and
  skipped the zombie backstop)
- keep-awake: check release()'s return value — a failed clear pins
  ES_SYSTEM_REQUIRED until process exit; log keepawake.release_failed
  instead of unconditionally logging success
- keep-awake: include a stack in the release_unbalanced warn so an
  unbalanced call site is findable
- cli --detach: add an 'error' listener on the spawned child (Node spawn
  reports ENOENT/bad-cwd asynchronously — unhandled it crashed the CLI
  after the success ack) and fail fast on undefined child.pid (same check
  as setup.ts trySpawn) so a spawn failure can't print a false "Started"
- cli --detach: warn in human mode when no log file could be opened
  (previously the degraded no-trail state was invisible)
- comments: align overclaims with reality (the "never fail a run" contract
  is now enforced, not asserted; lazy-logger side-effect caveat; the
  single-thread claim scoped to this codebase, not Bun; "proven pattern"
  softened with the Job-Object caveat); best-effort rationale stated once
  and referenced instead of restated three times
- tests: assert detached/windowsHide on the spawn options (the actual fix
  was previously unasserted), add the spawn-failure path, a throwing
  native-fn keep-awake test, and executor acquire/release pairing tests
  (success, DAG throw, blocked early return)
- docs: remove the two investigation dossiers from docs/ (they referenced
  the author's private environment; evidence preserved in coleam00#2080 and in
  this PR's file history at 2800552) and document the keep-awake
  behavior on the Windows deployment page

Deferred with rationale:
- acquire() boolean return for executor-side run-id logging: ambiguous
  semantics on the disabled/no-op paths; keep-awake already logs failures
- mock.module('node:child_process') test refactor: the Bun.spawn spy
  exercises the real shim path and now asserts the detach options
- makeKeepAwake() test helper and a CLAUDE.md spawn-guidance bullet:
  below rule-of-three; maintainer's call

Related coleam00#2080
@Wirasm

Wirasm commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Maintainer follow-up: I've pushed a614ebb5 addressing the review findings (both CodeRabbit's and the multi-agent review above) directly on this branch — thanks @buun-dev for the thorough groundwork, the core approach needed no changes.

Addressed:

  • Keep-awake FFI calls are now try/catch-guarded (acquire() and release()), so the module's "must NEVER block or fail a workflow run" contract is enforced rather than asserted. A throw from acquire() previously escaped executeWorkflow before its try (zombie running row); a throw from release() — first statement of the finally — replaced the run's return value and skipped the zombie backstop.
  • release() now checks the native return value — a failed clear pins ES_SYSTEM_REQUIRED until process exit (host can never sleep), and previously logged release_completed regardless. Now logs keepawake.release_failed. The release_unbalanced warn also carries a stack now.
  • Detached spawn failures are surfaced: added the 'error' listener (Node's spawn reports ENOENT/bad-cwd asynchronously — unhandled, it crashed the CLI after printing the success ack) plus a fail-fast child.pid === undefined check (same technique as setup.ts's trySpawn), so a spawn failure errors out instead of acking ok: true for a run that never started. Human mode also warns when no log file could be opened.
  • Tests: the spawn options detached/windowsHide are now asserted (the actual fix previously had no regression protection), plus a spawn-failure test, a throwing-native-fn keep-awake test, and executor acquire/release pairing tests (success, DAG-throw, and blocked-early-return paths).
  • Comments aligned with behavior: the lazy-logger side-effect caveat, the single-thread claim scoped to this codebase rather than Bun, "proven pattern" softened with the Job-Object caveat, and the best-effort rationale stated once and referenced instead of restated three times.
  • Dossiers relocated: the two docs/ investigation records referenced a private environment (D:\Project\Archon-template, MarphobBrain/…, gh auth switch), so they're removed from the tree — the distilled root causes and the remaining on-hardware verification checklist now live in Windows reliability: verify the Modern Standby + --detach fixes on real hardware (code merged in #2063) #2080, and the verbatim originals remain in this PR's file history at 2800552b. The user-facing keep-awake behavior is documented on the Windows deployment docs page instead.

Deferred (with rationale):

  • acquire() returning a boolean so the executor can log an unprotected run with its workflowRunId — ambiguous semantics on the disabled/no-op paths (what does "success" mean off-Windows?); keep-awake already logs its own failures.
  • Refactoring the spawn tests from the Bun.spawn spy to mock.module('node:child_process') — the spy exercises the real shim path today and now asserts the options that matter; standalone cleanup if the shim ever changes.
  • makeKeepAwake() test helper and a CLAUDE.md detached-spawn guidance bullet — below the rule-of-three bar; maintainer's call on the latter.

Validation (macOS): type-check all packages, eslint (--max-warnings 0), prettier, @archon/workflows and @archon/cli suites all green (147 CLI tests incl. the new ones; 8 keep-awake tests; executor suite + 3 new pairing tests).

Still open before merge: the live Windows smoketests (detach survival past worktree_creating, and a real Modern-Standby cycle with keep-awake held) — tracked as the checklist in #2080. @buun-dev, since you have the target hardware, running those two checks against this branch would close the loop.

Conflicts: executor.test.ts import hunks (spyOn/keepAwake vs the
folder-projects join/resolveProjectPaths imports — union of both).

Semantic fix: the folder-project detach test added by coleam00#2055 spawns with a
fake lacking `pid`, which now trips the fail-fast undefined-pid check —
gave its fake a pid like the other detach tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants