Uh oh!
There was an error while loading. Please reload this page.
fix(desktop): preserve Runtime Host exit diagnostics - #3344
Conversation
jackwener
left a comment
There was a problem hiding this comment.
Automated Codex review on exact head 40ecd476303d230f601119b1fb2a33f6a134e446 found no actionable code issue.
The bug still exists on current main: detached Runtime Host candidates use ignored stdio, so after transport EOF Desktop cannot report the selected child’s exit code, signal, or stderr. This revision keeps the evidence bounded (4 KiB stderr tail and four Desktop exit records), associates it only when the final registration PID matches a process spawned by this election, redacts before logging/persistence, and keeps detached lifecycle handles unreferenced. The startup-failure path still derives from the same exit evidence, and the process map is election-local rather than long-lived.
The required test check is green. The optional Windows package failure is a renderer CDP smoke timeout; a contemporaneous documentation-only PR failed with the same Packaged Maka renderer did not expose CDP signature, while this run’s earlier Windows package/installer smokes passed. I therefore do not attribute that failure to this diff, though a rerun would provide cleaner evidence.
Required conclusions:
- Optimal for the actual problem: yes.
- Production code to delete: none identified.
- Tests to delete/replace: none identified.
- Deeper refactor: no; launcher exit evidence and Desktop presentation remain at the correct boundary.
- Ready to merge: no; independent human review is still missing.
- Residual risk/gaps: rerun the non-required Windows package smoke; exact-head human judgment remains the merge gate.
This changes user-visible diagnostics and detached-process observability, so independent human review is required under CONTRIBUTING.md. No security, licensing, governance, or public-contract effect was identified.
HuYellow
commented
Aug 21, 2026
Thanks for the review. I’ll keep the current head unchanged since the required |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — this closes a real gap. stdio: 'ignore' meant that the one moment you most need the Host's own words, after its transport hits EOF, was exactly the moment they were guaranteed to be gone. Associating the evidence with the PID the final registration actually selected, rather than with whichever candidate happened to lose, is the detail that makes the report trustworthy, and the dedicated bounded buffer means repeated transport errors cannot push the exits out of the ordinary log.
No [P0]/[P1]. Two [P2]s and two [P3]s below. Both [P2]s are questions rather than defects I can demonstrate — I could not settle either from the diff alone, and in both cases the thing I want is a sentence in the code saying which behaviour was intended.
The one I would most like an answer on is the detached case. stdio for a detached Host changes from 'ignore' to 'pipe', which means the Host's stderr is now connected to a parent that is designed to exit first. 'ignore' pointed at /dev/null and was safe for the entire life of the Host no matter what Desktop did. A pipe is not: once Desktop is gone, the read end is closed, and what happens to the Host's next stderr write depends on details I would rather see pinned down than inferred.
Review assisted by AI (Claude Opus 5). Findings were verified against the files at this head; the reviewer is accountable for them.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
40ecd47 to
583d62fCompareHuYellow
commented
Aug 22, 2026
Addressed all four review threads on exact head
The branch is rebased onto current @Astro-Han, I have re-requested review for this exact head. Could a maintainer also approve the two workflows currently awaiting approval so the required |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — you answered all four points from the previous round, and this comment exists mostly to confirm that on the record. Re-reviewed at exact head 583d62f7408805e0ac820249810ff58b92ef7454 against base 4acfa26934ce4b2b385b76f8a11048bb83fab861. None of the six existing reviews are bound to this head, so everything below was re-checked against the current code rather than carried forward.
Each of our four items, verified here:
- [P2] detached stderr pipe lifetime — fixed.
launcher.tsnow spawns with['ignore','ignore','pipe']behind theMAKA_RUNTIME_HOST_STDERR_PIPEopt-in, and the EPIPE guard inprocess-diagnostics.tsswallows onlyEPIPEand rethrows everything else. That distinction matters and it is drawn correctly — a blanket catch here would have hidden the next real write failure. The detached-survival regression covers it. - [P2]
redactSecretstwo-pass semantics — resolved by explanation, which was the right resolution. The comment now states what each pass is for: full-text pattern matching, then per-token structured redaction to catch compact JSON the first pass steps over. The ambiguity was in the reader's head, not the code, so documenting it was the proportionate fix. - [P3] orphaned truncation marker on empty stderr — fixed. Empty stderr returns status-only before the marker is attached, with a whitespace-only regression alongside it.
- [P3] off-by-one at exactly 4096 — fixed.
chunk.length > MAXrather than>=, with a regression pinning the exact boundary.
Also checked, and sound:observeCandidateExit listens on 'close' rather than 'exit', so the stderr tail is complete before the record is built — using 'exit' would have truncated exactly the evidence this PR exists to preserve. Diagnostics associate with the finally-registered PID, so a connect-or-spawn race attributes output to the process that actually won. The log buffer is bounded at 16KiB across four entries.
[P3, ordinary path] The survival regression has a read race in the fixture, not in the product.
stderr-after-launcher-exit writes its marker with writeFile(marker, 'alive'), which truncates before it writes, while waitForFileText polls the file every 20ms. A poll landing between truncate and write reads an empty string and the assertion fails on '' !== 'alive'. We reproduced this once in five isolated runs, so it will go red on CI at some rate.
The product behaviour is correct — the marker being written at all is what demonstrates the EPIPE guard works and the process survived. Writing to a temp file and renaming it into place would make the read atomic; retrying on empty would also do it.
On CI: this head has zero check runs. The description reports a passing suite, and we do not doubt it, but nothing has executed against 583d62f7 itself, so there is no independent verification to point at. A push to trigger CI is the remaining gap — it is also what would let this be approved rather than commented on.
Two reviewers went over this independently, and both reached the same conclusion on all four items. Our own run of the host-kernel suite was 48/51, with the three failures being Electron-dependent tests that cannot start on a headless machine (missing libatk-1.0.so.0) — an environment limit, not a result about this change.
This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are ours to correct — please push back where we got it wrong.
Astro-Han
left a comment
There was a problem hiding this comment.
One follow-up now that the full check set has run on this head.
test is green, but Release Windows check is failing — Verify automatic update end to end times out with Maka.exe did not appear among installed processes within 120000ms.
I looked at whether that check is just flaky, and it doesn't appear to be: of the twelve most recent runs, it succeeded on eight branches around the same window and failed on two, one of which is this branch. So it isn't a broken runner.
I have not proven this PR causes it, and I'd rather say so than assert it. But the inline note below is where I'd start looking — the failing step is a detached-spawn scenario, and this is the PR that changes how that spawn's stdio is wired.
Everything from my previous review still stands: all four earlier points are resolved, and the only other open item is the P3 test race in the fixture.
This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are mine to correct — please push back where I got it wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Astro-Han
commented
Aug 23, 2026
Hi — this PR conflicts with current I tested a rebase onto current
These are real source conflicts, so they need your judgement rather than a mechanical rebase — please rebase onto current Thanks for the contribution — happy to help if any conflict is unclear. AI-assisted maintenance note, not a review. It does not count as the required human review under |
Generated-by: Codex
583d62f to
2f7c10dCompareHuYellow
commented
Aug 23, 2026
@Astro-Han Rebased onto current I manually resolved the five conflicts, preserving the new election diagnostics and keeping current-run Runtime Host exits out of previous-main-process reports. I also addressed the two follow-ups:
Local verification on Windows is green:
The exact detached-process regression where the launcher exits and the Host later writes stderr passes on this head. GitHub currently shows the new head without source conflicts; |
HuYellow
commented
Aug 23, 2026
@Astro-Han All requested follow-ups are now complete on exact head |
jackwener
left a comment
There was a problem hiding this comment.
Representative review under @WAWQAQ's standing apache/maka review authorization, bound to exact head 2f7c10dbab4bab45856a5e5e57eaaad843843bfb.
GO: no P0–P2 findings. I verified the bounded stderr-tail collection, final-registration PID association, redaction/truncation path, detached-process behavior, and the dedicated copied-diagnostics path. The affected Runtime Host tests passed 5/5, Desktop diagnostics/candidate tests passed 41/41, and removing the selected-process handoff made its targeted regression fail before restoration. The exact-head test and Windows package check-runs are both completed/success; the PR is open, non-draft, and mergeable, with no current live hold.
One non-blocking P3 is recorded inline. This agent-executed approval does not claim to satisfy any separate repository human-review requirement. No merge performed.
| maxBytes: MAIN_PROCESS_DIAGNOSTIC_LOG_MAX_BYTES, | ||
| }); | ||
| export const runtimeHostProcessLogBuffer = new DiagnosticLogBuffer({ | ||
| maxBytes: 16 * 1024, |
There was a problem hiding this comment.
[P3] The promised four-exit history only retains three maximum-size records. This buffer combines maxBytes: 16 * 1024 with maxEntries: 4, but each formatted 4096-byte stderr-tail diagnostic is larger than 4 KiB once the timestamp, level, exit status, truncation marker, and JSON string encoding are included. Reproduction on this production buffer: append four such records; snapshot() returns 3 entries (4206 encoded bytes each), retaining PIDs 2/3/4 and evicting PID 1. The current diagnostic test injects only one short record, so it cannot catch the contract gap. Please either size the total cap for four worst-case formatted entries or impose a per-entry bound that makes the stated four-entry retention true, and add a four-maximum-entry regression.
Uh oh!
There was an error while loading. Please reload this page.
Generated-by: Codex
Summary
The detached local Runtime Host was launched with ignored stdio, so after its
transport reached EOF Desktop had no process exit code, signal, or Host stderr
to include in the diagnostic report.
This change captures a bounded 4 KiB stderr tail and exit status while
preserving detached-process behavior, associates that evidence only with the
spawned PID selected by the final Host registration, and records a redacted
diagnostic in both the main-process log and a dedicated bounded process-exit
buffer. Copied Desktop diagnostics retain the last four local Host exits even
when repeated transport errors fill the ordinary log buffer and the Host is no
longer available.
Fixes#3333
Verification
npx biome check <13 changed files>npm run buildnpm run typecheckgit diff --checkdetached stderr after launcher exit, bounded stderr, and the exact 4096-byte
boundary
evidence in copied diagnostics
The full Desktop suite was also run on Windows: 955 tests passed, 17 unrelated
existing environment-dependent tests failed, and 7 were skipped. The failures
were Windows symlink
EPERM, SQLite cleanupEBUSY, and Rive CLI fixturefailures. All tests added or changed by this PR pass.
AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex assisted with root-cause analysis,
implementation, regression tests, local verification, and PR drafting. The
contributor will review the final diff and accepts responsibility for the contribution.
Checklist
Lint, formatting, typechecks, builds, and targeted affected tests pass. The
full Desktop suite has the unrelated Windows failures listed under Verification.
Does this PR entail a change in behavior?