fix(host): undo a failed fork, say why, and offer a folder when a copy has nowhere to run - #52
Conversation
A fork wrote its copy, registered it, and only then started the runtime. When the runtime refused to start, the copy stayed: an orphan session plus the lock its short-lived child took on the way down. The command reported outcome_unknown with "operation failed", which told an operator nothing, even though the child's stderr had already named the cause. Roll the copy back on that failure, durable artifact first so a delete that itself fails leaves the session visible and retryable rather than silently orphaned. Await the child's exit before removing anything, because stop() only signals and a surviving child can still rewrite its lock. Report the failure as session_start_failed. The child's stderr can carry tokens, credentialed URLs, and paths, so none of it is reproduced to the renderer or to a log; known startup failures map to fixed operator text and anything else stays generic. Carry the child's diagnostic on the stdout-EOF path, which previously reported a bare EOF. Both that path and the exit callback now wait on a bounded stderr drain, since the readers race and the diagnostic would otherwise be lost at random. Covered by three tests that use the real child factory. The existing fork tests stub the factory, the authority, and the lock check, so none of them exercised any of this.
A historic transcript records the project directory the session ran in, and for older sessions that directory is frequently gone. The copy is a new session that has to start somewhere real, so a fork of one of those failed on a bare ENOENT with nothing an operator could act on. session.fork now accepts an optional cwd. The authority writes it into the copy's own header, so the choice survives rediscovery instead of living only in this process's memory. Nothing is substituted silently: a fork with no choice and a vanished source directory is refused with session_cwd_missing, which names the condition and invites a retry, and a chosen directory that does not exist is refused with session_cwd_invalid. Both argument gates validate the value, not just the key, so the wire decoder and the post-decode check cannot drift apart. DesktopRuntimeError carries the host's own code now, so a caller can branch on session_cwd_missing and offer a directory chooser rather than matching on message text.
Continuing a historic session in a copy failed outright when the project directory it recorded had since been deleted, which is common across an old session archive. The host now names that condition, so the app can ask instead of dead-ending. A refused fork carrying session_cwd_missing opens a directory chooser and retries with what the operator picked. The choice is matched on the host's own error code, not on message text. The renderer may ask for the chooser but never names a directory: the main process runs the dialog, parented to the requesting window, and returns only the path it selected. A payload carrying a path is rejected, as is an untrusted sender.
The chooser had only rejection coverage, so the gates could stay green even if the picker never returned a usable path. Inject the dialog through a narrow seam and assert what it actually returns: the operator's path, and nothing when dismissed, parented to the requesting window either way. The retry decision moves out of the component so it can be tested directly: retry once with the chosen directory, only for session_cwd_missing, never after a dismissal, and never for an unrelated failure. Two corrections that came out of writing those tests. A locally chosen path says nothing about a remote host's filesystem, and an identically named path there would run the copy somewhere unintended, so the chooser is offered only for local targets and a remote refusal stands. And a dismissed chooser no longer raises a banner: the operator just answered that question. Absolute-path validation now uses the platform path API instead of a leading slash, matching the bridge.
The cleanup path escalated SIGTERM to SIGKILL but ignored whether the second signal worked. A child that survives it still owns the session's lock and can rewrite the file, so deleting that session would recreate the very race the escalation exists to close. Treat a child that outlives SIGKILL as a cleanup failure: keep the copy and its record, and say so. Losing track of a live writer is worse than leaving a visible session an operator can retry. Covered by a child that traps SIGTERM, which forces the escalation and then confirms the copy is only removed once the process is actually gone.
The SIGTERM test only checked the resulting file state, so it passed with or without the quiescence wait it was meant to defend. Record the child's pid and assert the process is already gone when the response lands. Removing the wait now fails this test at that assertion while the copy check still passes, which is the ordering the fix is about.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77f8747ec2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| call("session.fork", { | ||
| session: sessionReference(source), | ||
| ...(cwd === undefined ? {} : { cwd }), | ||
| }) as never, |
There was a problem hiding this comment.
In packaged bridge-mode environments, I checked that compat/omp-app-matrix.json and the cluster runtime remain pinned to t4code-17.0.5-appserver-15; that bridge validates session.fork parameters as exactly session. Consequently, after the host returns session_cwd_missing and the desktop obtains a directory, this retry always fails when this newly added cwd field reaches the shipped bridge. Update the runtime pin and bridge contract in the same cutover, or withhold the fallback until the authority advertises support for this parameter.
AGENTS.md reference: AGENTS.md:L23-L24
Useful? React with 👍 / 👎.
| this.#createdPending.delete(sessionId); | ||
| this.#records.delete(sessionId); | ||
| this.#projections.delete(sessionId); | ||
| await this.#sessionOwnership?.delete(sessionId); |
There was a problem hiding this comment.
When a spawned fork child exits during startup, RpcChildSupervisor invokes markSupervisorCrashed, which persists an attention outcome before ensureSupervisor rejects. This rollback removes the copied file and the session maps but never deletes that outcome, unlike the normal session deletion and eviction paths. Every failed fork therefore leaves durable metadata for a nonexistent session, and enough failures can consume the store's 1,000-entry cap and evict outcomes belonging to real sessions; delete the outcome as part of this cleanup.
Useful? React with 👍 / 👎.
1 participant
Draft: the directory selector needs a new pinned OMP runtime. See Blocked on below.
Why
Chasing why "Continue in a copy" failed, the fork itself turned out to be fine — my sandbox simply had no model configured, so the copy's RPC child exited before reporting ready. But getting there surfaced four real defects, all of which this addresses.
What changed
A failed fork left an orphan.
handleForkwrote the copy, registered it, then started the runtime. A runtime that could not start left the copy behind, plus the lock its short-lived child took on the way down. The copy is now rolled back, durable artifact first: if that delete itself fails the session stays visible and retryable rather than silently orphaned. The child's exit is awaited before anything is removed, becausestop()only signals and a surviving child can still rewrite its lock.The error said nothing.
outcome_unknown/ "operation failed" becamesession_start_failedwith a real cause. The child's stderr can carry tokens, credentialed URLs, and paths from any platform, so none of it is reproduced — not to the renderer, not to a log. Known startup failures map to fixed operator text; anything else stays generic.The diagnostic was also being thrown away: a child that dies during startup closes stdout without sending ready, and that path reported a bare EOF. It now carries the child's stderr, and both that path and the exit callback wait on a bounded drain, since the two readers race and the reason was otherwise lost at random.
A copy had nowhere to run. A transcript records the project directory its session ran in, and for older sessions that directory is frequently gone — common across a large session archive.
session.forknow takes an optionalcwd. The authority writes it into the copy's own header so the choice survives rediscovery; an override applied only in memory would revert on the next rediscovery. Nothing is substituted silently: no choice plus a vanished directory givessession_cwd_missing, and a bad choice givessession_cwd_invalid. Both argument gates validate the value, not just the key, so the wire decoder and the post-decode check cannot drift.The desktop now offers a folder. A refused fork carrying
session_cwd_missingopens a directory chooser and retries with what the operator picked, matched on the host's error code rather than message text. The renderer may ask for the chooser but never names a directory: the main process runs the dialog, parented to the requesting window, and returns only the path it selected.Why the existing tests missed all of this
The fork tests stub
childFactory,sessionAuthority.fork, andlockCheck, so none of the real spawn, bridge, or lock behaviour was ever exercised. Six new host tests use the real child factory and real spawns; one desktop test covers the chooser channel.Testing
pnpm checkclean.{"code":"ENOENT","message":"operation failed"}with an orphan copy left behind; after,session_start_failedwith an actionable message and the copy cleaned up.Blocked on
wolfiesch/oh-my-pi#13 adds the bridge side. The currently pinned
appserver-15validatessession.forkparams as exactly["session"], so it rejectscwd— the chooser cannot work until that lands, a runtime is released, and the pin is updated here.Everything else in this PR works against the current pin: without a chosen directory the host never sends
cwd, so the failure-handling and error-reporting changes are unaffected.Not verified
The rendered click-through of the chooser, for the reason above.