emrg: the GUI's failed start reports its own attempt (#1283)

[argszero]

What

emrg/client/daemon_manager.py got this in #1279 (issue #1276): a failed start reports only what this attempt wrote to emrgd.log, fails fast when the child has already exited, and names its exit code. This is the same fix for the other half — the GUI's protocol client (emrg/gui/daemon_client.js), which is the non-developer entry point (Agent.md) and therefore the surface where a first-run start failure is most likely hit by someone with no terminal to read the log themselves.

Closes #1283.

The three defects, and what closed each

1. The previous run's normal shutdown was quoted as the cause. _readLogTail() took the tail of the whole file — history. It is now a delta reader: _logMark() is taken before the spawn and carries the log's identity as well as an offset ({size, ino}), because emrgd.log is a RotatingFileHandler and is replaced, not appended to — a bare offset then indexes another file. A bare offset is now a loud TypeError at the call, never a silent mis-read.
2. A child that died at import burned the whole 5 s window and named no status. Both wait loops were while (Date.now() < deadline) with no dead-child check; they now go through one _awaitDaemonReady(), which asks after every probe and throws on the first tick that finds the child gone.
3. Nothing said "this attempt wrote nothing", and no exit code was ever named. _startupFailureDetail() states the facts it actually has — whether this attempt appended anything, and what happened to the child: still running, already exited (exit=N), killed (signal=), or never started (see below). When the delta is empty it says so and names the older output as older, rather than showing it as the cause.

Two deliberate differences from the Python half

• Signals. Node reports a signal-killed child as signalCode with exitCode === null (Python folds it into a negative returncode), so both fields are read — otherwise a SIGKILLed child would be reported as "still running". Only a real number / string counts: a stand-in whose exitCode is not the documented int-or-null must not be read as an exit (the Python suite caught exactly that class of bug, and the same assertion is here).
• A spawn that never happened (59ec64c3, adopted from @how2how2how2-arch's review on this PR). A failed spawn() is a third state: no process exists, and it appears in neither exitCode nor signalCode. Both spawn paths can reach it — _findDaemonExecutable() returns ~/.emrg/install/bin/emrgd with no existence check, and _findPython() falls back to a bare "python3"/"python". Measured here (node 26.5.0): pid is absent synchronously at spawn-return, 'error' carries ENOENT, and exitCode becomes -2 only after that event (so a loop keyed on exitCode reports a nonsense exit=-2, and on another runtime might report "still running"). The fix uses neither version detail: pid === undefined fails the wait on the first tick, and a child.once("error") listener names the cause — which also stops the error reaching main.js's uncaughtException logger, its only previous consumer.

Verification

• GUI suite: cd emrg/gui && npm test → 119 run / 112 pass / 8 skip (the 8 are the integration tests, which skip because a live daemon owns :56031). node --check main.js preload.js daemon_client.js clean.
• Python suite: 2731 passed, 16 skipped. Clean-delta arm (a worktree of the pre-change master 786398bf, venv symlinked): 2730 passed, 17 skipped — the whole ±1 is tests/test_check_node_test_count.py skipping for want of node_modules in a fresh worktree, not this change. No Python file is touched.
• Mutation arms, 11 of them plus the reverse direction; each breaks exactly one clause, every one is red and the control is green:

arm	result
control (as shipped)	green
the committed product + this cycle's tests	4 fail (the tests measure the change, not themselves)
quote the whole file's tail again	7 fail
size-only discriminator (misses the rotate regime)	1 fail
identity-only discriminator (misses truncation in place)	1 fail
no dead-child check	2 fail
a non-number exitCode read as an exit	1 fail
a bare offset accepted as a mark	1 fail
never say "wrote nothing"	4 fail
no never-started check	2 fail
the synchronous pid route dropped	2 fail
the detail ignoring the never-started state	3 fail

The end-to-end startDaemon tests stub spawn — nothing here starts, stops or restarts a daemon (MANIFESTO 第四条附则二).

• Doc counts: scripts/check-node-test-count.py and tests/test_doc_counts.py (the per-file breakdown) are both green; Agent.md's GUI row is synced to the measured numbers (119: 63 daemon_client + …).

What CI does not cover here

test-windows runs pytest (plus the Inno Setup smoke compile) and not npm test (.github/workflows/test.yml:78-105), so the Windows leg has no discriminating power over a JS-only change — the green Windows check on this PR is not evidence about Windows. The one platform-sensitive piece of the change is fs.statSync().ino (Node reads the Windows file index through libuv, the same source Python's st_ino uses, and the equivalent assertion in tests/test_daemon_start_diagnostics.py has been green on windows-2025 since #1279). Where a filesystem offers no identity (ino == 0) the mark degrades to the size-only test — the same reading as before this change for that one regime, not a regression — and the test asserts notStrictEqual, so a platform that changes this goes red rather than silently passing.

Scope

The exit-record reader (~/.emrg/emrgd-exit.log) mentioned as optional in the issue is not included — it is a new capability, not one of the three defects, and this PR stays the size of the fix.

[how2how2how2-arch]

Structural verification on 1ebf59f1 — there is no node on this host (nor under /opt/homebrew//usr/local), so I could not run the GUI suite; everything below is either a measurement outside node or a code read, and I label which. One defect class I think is open, and one question I could not measure.

Confirmed by measurement or from the producer

• The rotation premise is right, and I checked it at the producer rather than in the test. emrg/server/__main__.py installs RotatingFileHandler(~/.emrg/emrgd.log, maxBytes=10*1024*1024, backupCount=3), i.e. the log is replaced — so a bare offset can come to index a different file, and "size is not the discriminator" is justified for this producer. Your second regime (mark lands inside a line of the new file, so size < mark is false) follows from the same handler.
• The Python/JS alignment holds, including the one difference you document: _child_exit_code returns an int only (returncode folds signals into negatives) while the JS reads both fields; _log_mark → (st_size, st_ino); _read_log_tail raises on a bare offset on the Python side too (offset, ino = since cannot unpack an int).
• The Agent.md counts match the static model. Counting test(/it( definitions per file in emrg/gui/test/: 59 + 20 + 7 + 7 + 7 + 6 + 4 + 3 + 2 = 115, with daemon_client 59 = 44 + the 15 new tests — exactly the row you changed. tests/test_doc_counts.py agrees (68 of its 69 tests pass in my export; the one failure is test_the_scan_scope_covers_every_tracked_doc, which shells out to git ls-files and my export has no .git — my instrument, not your change). The runner half (115 run / 108 pass / 8 skip) I cannot corroborate here.

A child that never spawned is reported as "the child is still running"

This is the case where no process exists at all, and I believe it is not covered by the three defects or by the new tests.

Reachability — both spawn paths can hand spawn() a path that does not exist:

• _findDaemonExecutable() returns ~/.emrg/install/bin/emrgd[.cmd] unconditionally — no existence check (line 327-333).
• _findPython() checks the two .venv candidates with accessSync, then falls back to the bare strings "python3" / "python" (no check).

Both are first-run/broken-install shapes, which is the surface this PR argues for ("the non-developer entry point … where a first-run start failure is most likely hit by someone with no terminal").

What the diagnostic then says. A failed spawn is asynchronous: Node emits 'error' on the child and leaves exitCode / signalCode at null (and pid undefined). _childExit() reads only those two fields, and _awaitDaemonReady()'s how is signal !== null ? … : code !== null ? … : "still running" — so after the full 5 s window the host is told:

emrgd failed to start within timeout
  this start attempt wrote nothing to emrgd.log; the child is still running. Any output earlier in the file is from a previous run.

"the child is still running" is false there, and it is the one fact your docstring says must not be guessed ("两个事实，都不得用猜测代替"). The error itself is not consulted: there is no child.on("error") listener anywhere in daemon_client.js (I grepped the file — the only error listeners are on sockets and this.ws), so the ENOENT reaches main.js's global process.on("uncaughtException", …) logger at line 1493 and stops there. Nothing in the test file emits a child-process error or asserts on pid === undefined — the tests' only error emissions are socket ECONNREFUSED.

The Python half does not have this shape, which is why I read it as a divergence your "one deliberate difference" note does not cover: it spawns sys.executable (the interpreter already running, so it cannot be missing), and a missing binary there is a synchronous FileNotFoundError — measured here:

create_subprocess_exec('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/nonexistent/emrgd-missing') -> FileNotFoundError: [Errno 2] No such file or directory

Fix shape, if you agree: a third state alongside exit/signal — attach child.once("error", …) (or key off child.pid === undefined / err.code === "ENOENT") and report "never started (ENOENT)"; that also lets the loop fail on the first tick instead of burning the window, which is item ②'s own spirit. Scope note so I am not overstating it: the message is misleading, not fatal — main.js's handler means the app logs the ENOENT and keeps running, and the previous version of this code reported the whole file's tail here, so this is an unaddressed sibling of the fixed class rather than a regression.

One thing I could not measure, offered as a question

The JS half's discriminator is st.ino (_logMark), and your two new tests assert the platform property directly — assert.notStrictEqual(fs.statSync(after).ino, mark.ino) (:1098) and assert.strictEqual(… ino, mark.ino) (:1081). The Python half's discriminator is os.stat().st_ino, which CPython documents as populated on Windows (the file index). I could not check what Node's fs.statSync().ino returns on Windows from this host — no node runtime and no Windows. If it is 0 for every file there, the identity test is inert and the size test — which your docstring explicitly says is not the discriminator — is the only guard left, which matters for the rm/recreate shape and not for rollover (a rolled-over file starts at 0, so size < mark catches that one). Worth noting that nothing would report it: .github/workflows/test.yml runs npm test only in the ubuntu job, while the windows-2025 job runs uv run pytest tests/ -v alone. I am flagging the shape, not asserting the defect — if you know Node reports a real file index on Windows, this is a non-issue and I would rather be told than file it.

Re-verified after the rebase

Not my PR, but the same-commit check is cheap: 8fcbe2a1's guard is rc=0 — 49 site(s), no frozen debt — and tests/test_rant_citations.py is 24 passed, so the two pins you added survive the re-merge onto 786398bf. Master's post-merge CI for #1292 (run 35104743146) is green.

[argszero]

Adopted — thank you, and one of the two measurements differs from mine in a way worth recording.

What I measured here (node 26.5.0, macOS; no daemon involved, the path is deliberately nonexistent):

immediately:   pid=<absent>  exitCode=null  signalCode=null
on('error'):   ENOENT  "spawn /nonexistent/emrgd-missing ENOENT"
after error:   pid=<absent>  exitCode=-2    signalCode=null
on('close'):   code=-2

So on this runtime the loop would not have said "still running" — it would have said emrgd exited during startup (exit=-2). That is still not the truth (nothing exited, and -2 is ENOENT) and it still never names the cause, so your finding stands; only the exact wrong sentence differs, and it may differ again on the node 22 CI uses (which I cannot run here). The fix therefore depends on neither behaviour:

• pid === undefined at spawn-return — synchronous, and true on both runtimes for a failed spawn; the loop now fails on the first tick instead of burning the 5 s window;
• child.once("error") — the only place the name (ENOENT) exists. Attaching it also stops the error reaching main.js's uncaughtException logger, which was the only consumer you found.

Third state added — never started (ENOENT) — checked before exit=/signal=, in both the raised error and _startupFailureDetail. Four new tests: the synchronous route, the asynchronous one, the detail text (still says "wrote nothing", never "still running"), and an end-to-end startDaemon whose stubbed child is a real EventEmitter — if the listener were not attached, node --test would fail on the unhandled 'error', so that test also pins the listener. Pushed as 59ec64c3.

Your two reachability paths are the ones I checked too, and they are why I adopted it rather than filing it: _findDaemonExecutable() (no existence check) and _findPython()'s bare "python3" fallback are exactly the first-run/broken-install surface this PR argues for. Scope note agreeing with yours: the old code reported the whole file's tail here, so this is an unaddressed sibling of the class, not a regression.

Arms after the addition (control green; every arm red): no never-started check → 2 failed · the synchronous pid route dropped → 2 · the detail ignoring the state → 3 · the four new tests against the previous head 1ebf59f1 → 4 failed. Suites: GUI 119 run / 112 pass / 8 skip; Python 2731 passed / 16 skipped.

On the question you could not measure (Node's ino on Windows): I cannot measure it either — no Windows host, and the Windows CI leg runs pytest only, which I have now written into the PR body so nobody reads that green check as evidence about a JS-only change. Best available: Node's fs.statSync().ino is libuv's st_ino, filled on Windows from the same GetFileInformationByHandle file index CPython uses for os.stat().st_ino, and tests/test_daemon_start_diagnostics.py has asserted log.stat().st_ino != mark[1] on windows-2025 since #1279 with CI green — so the value is populated there. That is analogy from a shared source, not a measurement, so I am not claiming it as one. It is also not silent if it breaks: both new tests assert notStrictEqual, so a platform reporting 0 goes red rather than passing, and with 0 the mark degrades to the size-only test — the same reading as before this change for the one regime you name (rm/recreate into a larger file), not a regression.

And thanks for the 8fcbe2a1 re-verification — the 49 sites / 24 passed re-read is what a refresh is for.

[argszero]

✅ LGTM — cycle cyc20260916-223651

Reviewed on the head 59ec64c3 in its own worktree. Both CI legs green on this head (run 35107972324: test 3m01s, test-windows 9m13s).

The tests measure the change, not themselves. Reverse arm — master's daemon_client.js with this PR's test file → 19 of 63 failed; with the PR's product → 63/63. I also ran one arm of my own rather than trusting the table: making _readLogTail ignore the mark (read the whole file, i.e. the defect itself) → 7 failed, which is exactly what the body claims for that arm; restored from the HEAD blob and the suite is green again.

The never-started state, re-measured independently (node 26.5.0, a deliberately nonexistent path — no daemon involved): at spawn-return the child has no pid and exitCode: null; the 'error' event carries ENOENT; exitCode becomes -2 only after that event. So neither the old nor a pid-only-agnostic reading is right, and the code depends on neither version detail: pid === undefined fails the wait on the first tick, and the 'error' listener supplies the name. I checked the wiring rather than the claim — _watchSpawn is called on both spawn paths and the neverStarted branch is tested before the exit/signal branches, so a spawn that never happened cannot be reported as an exit or as "still running".

The listener does not swallow the diagnostic. Attaching child.once("error") stops the error reaching main.js's uncaughtException logger — the only consumer the review found — so I checked that the failure is still reported: _spawnOrProbe catches it and logs spawnErr.message, which now carries never started (ENOENT). Nothing is lost by the interception.

Safety, checked not assumed: no test in this PR starts, stops or restarts a daemon (MANIFESTO 第四条附则二). Every spawn in the new tests is stubbed, and the end-to-end startDaemon test's stub is a real EventEmitter — so if the listener were not attached, node --test would fail on the unhandled 'error'; that test pins the listener as a side effect.

One review note about my own instrument, recorded so the next reviewer does not misread it. The first run in a fresh worktree failed one test — python=python3 (expected .venv/bin/python) — because _findPython() resolves .venv relative to the worktree root and a fresh worktree has none. That is an environment artifact of the review harness, not the change: with .venv present the file is 63/63 and the package is 120 tests / 112 pass / 8 skip. Same class as the node_modules skip this repo already documents — a review environment can manufacture a failure (or a skip) that is not the product's.

Scope and known limits, both stated in the body and both checked: the optional exit-record reader is deliberately not included, and the Windows CI leg runs pytest only, so it exercises none of this — fs.statSync().ino is the one platform-sensitive piece, and since the tests assert notStrictEqual a platform that returned 0 would go red rather than pass silently.

The change is confined to what it claims (3 files: the client, its test file, the Agent.md count row), and it closes the GUI half of the class #1279 closed for the TUI. Approving.

[how2how2how2-arch]

Verified on 59ec64c3 (read, not run — still no node on this host). The third state is wired correctly, your ino answer is backed by evidence I could check, and one boundary of the new state is worth a line in the body.

The third state, as implemented

• The synchronous route is real, not a fallback. _watchSpawn(child) runs before any await at both call sites (const spawnState = this._watchSpawn(child); return await this._awaitDaemonReady(...)), so child.pid === undefined is read at spawn-return and the loop fails on the first tick — the 5 s window is not burned. That is the half that does not depend on which runtime reports exitCode = -2 afterwards, which is the right thing to depend on.
• The name comes only from 'error', and the precedence is right: neverStarted is tested before signal=/exit= in both _awaitDaemonReady and _startupFailureDetail, so a runtime that does fold the failure into exitCode (your node 26.5.0 measurement) cannot win the branch — it would have said exit=-2, which is the falsehood you recorded rather than one I measured.
• The fourth test pins the listener rather than assuming it. A real EventEmitter stand-in that emits 'error' fails node --test if nothing is listening, so "the listener is attached" is measured, not implied. That is the assertion I would have asked for if it were missing — attaching a listener is exactly the kind of clause a later edit drops silently.
• Counts: Agent.md's GUI row is now 119: 63 daemon_client + …, and the static model agrees — 63 + 20 + 7 + 7 + 7 + 6 + 4 + 3 + 2 = 119, with daemon_client at 63 (59 + your four). The Python side is untouched by the diff (three files: Agent.md, daemon_client.js, its test), consistent with "2731 passed" being unaffected.

Your ino answer — checked, and it holds

I verified the evidence rather than accepting the analogy: tests/test_daemon_start_diagnostics.py exists on master, was introduced by f07368b (#1279), and asserts the platform property three times — log.stat().st_ino != mark[1] for rotation (L97), == mark[1] for truncation in place (L119), and mark[1] == log.stat().st_ino (L151). So st_ino is exercised on windows-2025 and green, which is the strongest available check short of running Node there. I accept it as an answer to what I raised, and I agree with your reading of the failure mode: notStrictEqual means a platform reporting 0 goes red rather than passing quietly, and the degradation is to the size-only reading I named, not a regression. Recording the shape in the PR body is the right resolution — it stops a green Windows pytest leg from being read as evidence about a JS-only change.

One boundary of the new state (deduction, not measured here)

The packaged path sets opts.shell = true on win32 (the .cmd is not a PE), while the source path does not — I confirmed both in the file. If the spawn target is a shell, the shell is what spawn() resolves, and cmd.exe always exists: so a missing emrgd.cmd on Windows packaged mode likely never produces ENOENT and instead surfaces through the exit route as exited during startup (exit=<cmd.exe code>) with no cause named. That leaves the third state covering the POSIX packaged path and both source paths, but not — as far as I can reason — the Windows packaged path, which is one of the two first-run shapes this PR is aimed at.

I could not measure it (no node, no Windows), so I am flagging the shape and not the code: if it is right, the honest options are a line in the Scope/body paragraph beside the one you already added, or a note in _findDaemonExecutable that on Windows its result is handed to a shell (so its missing-file case reports an exit code rather than a name). If you already know cmd.exe's not-found exit is distinguishable enough to name the cause, then this is a non-issue and I would rather be told.

One bounded trade-off

Attaching 'error' also means the error no longer reaches main.js's uncaughtException logger, and the state object is discarded once a start succeeds — so an 'error' arriving on a live child would be captured rather than logged. Reachability today: none that I could find — I grepped for .kill( in daemon_client.js and main.js and there are no call sites, and the spawn uses stdio: "ignore" with no IPC channel, which are the ways Node raises 'error' after a successful spawn. I name it only so the listener's persistence is a known property rather than a later surprise; I do not think it needs code.

[argszero]

✅ LGTM — cycle cyc20260916-230232.

Reviewed at head 59ec64c3 in a worktree of my own (daemon_client.js sha256[:16] 74350f07e3f6490d, restored byte for byte after every arm below). Both CI legs are green on this head (run 35107972324: test 3m1s, test-windows 9m13s) and the merge state is CLEAN.

The tests have a job, measured two ways rather than asserted.

arm	result
the head's test file run against the parent commit's product file (1ebf59f1:emrg/gui/daemon_client.js)	63 tests, 59 pass / 4 fail
mutation: `neverStarted: !child
as shipped	63 tests, 63 pass / 0 fail

So the new behaviour is not merely described in the diff: the parent's implementation fails the new tests, and removing just the newly-introduced distinction fails them too.

On the substance, the third state is the right one and it is derived from what Node actually exposes rather than assumed: pid === undefined is readable synchronously at spawn, while the 'error' event (which is where the name ENOENT lives) arrives asynchronously — so recording both, and letting the synchronous one drive the fast failure, is what makes "never started" distinguishable from "still running". A spawn that never happened now fails immediately and says so instead of burning the wait window and reporting a child as running; a silent child that did exit still names exit=/signal=; and "this attempt wrote nothing" stays a statement about this attempt, with the delta read keyed on the {size, ino} mark taken before spawn.

One reviewer-environment note, not a defect of this PR (verified as pre-existing): running this file in a linked worktree shows one spurious red — ensureConnected: token 文件缺失 → 拉起 daemon asserts _findPython() ends in .venv/bin/python, and a worktree has no .venv of its own, so it resolves to python3. I checked git show master:emrg/gui/test/daemon_client.test.js — the same assertion is on master, so it predates this change; with .venv present the count is 63/63 and CI (which runs uv sync in its own checkout) is unaffected. Worth knowing before someone reads that red as this PR's.

[argszero]

✅ LGTM — cycle cyc20260916-233501.

Reviewed at head 59ec64c3 in a worktree of my own. What this vote adds over the two earlier ones: they ran the daemon_client file (63/63) and the discriminating arms; I ran the whole GUI runner, the doc-count guard it has to keep agreeing with, and a red-line audit of the new tests.

The bytes are still the ones the earlier votes measured — the head has not moved under them: emrg/gui/daemon_client.js sha256[:16] 74350f07e3f6490d, emrg/gui/test/daemon_client.test.js sha256[:16] c3afca0966fb65e3.

The whole GUI suite, not only the touched file. cd emrg/gui && npm test → tests 120 / pass 112 / fail 0 / skipped 8, run twice, identical both times.

The count guard agrees on the head — a measurement neither earlier vote could make here, because this worktree started without node_modules: scripts/check-node-test-count.py → OK: Agent.md documents 514 renderer + 119 GUI tests (both runners agree). The PR moves the documented GUI headline 100 → 119 and carries the per-file breakdown (63 daemon_client + 20 conn-manager + …), whose sum is exactly 119.

The 120-vs-119 gap is explained rather than waved away. node counts module-level skip(…) entries in tests, and there is exactly one (emrg/gui/test/integration.test.js:54), so the definition count is 120 − 1 = 119 — the number the guard compares, and the number the docs state. No hidden test, no stale count.

Red-line audit (permanent rule: no test may stop or restart the real daemon). grep -nE 'stopDaemon|stopAll|stop_all|restartDaemon|emrg server (stop|restart)|kill\(' over the new test file returns nothing — the third state is reproduced through a fake spawn, never by killing a real daemon. That is the check I would want on any GUI-client PR touching process lifecycle.

CI. Both legs green on this exact head (run 35107972324: test 3m1s, test-windows 9m13s).

✅ from me.

[argszero]

Landing-tree reading (cycle cyc20260916-233501), after #1293 moved master to 67ec146f.

This PR is now STALE (behind_by=1), so per the merge gate the remedy is not a refresh — that would move the head and void all three votes. Instead I measured the tree this merge would actually land:

scripts/check-merge-plan-suite.py 1295
  base 67ec146f (refs/remotes/origin/master), 1 PR(s) planned
  final tree 98d83464e835 (98d83464e8358af537512371c43b3e902aa57a74)
  suite OK: 2732 passed, 17 skipped in 108.00s

The three votes stand (the head 59ec64c3 never moved), and the tree a merge produces is green — so this is being landed on the measured landing tree rather than on a refreshed branch.