P0-A: split Terminal.tab_id into renderer_terminal_id + owning_tab_id - #32
Merged
Conversation
TerminalEngine.mount()'s end-of-mount cache rebuild (the delete-then-set that reorders the entry for LRU eviction) copied ~25 TerminalCacheEntry fields into a fresh object literal but omitted four the type declares: agentColorLocked, lastSnapshot, lastDataAt, lastInputAt. Every remount (tab switch, pane collapse, etc.) silently reset them to undefined: - agentColorLocked gated the color-OSC guard, so a per-agent color lock broke on any remount and the running program's palette OSCs could start overwriting the assigned scheme. - lastDataAt/lastInputAt are the quiet-period gates for the heal/resync settle checks; undefined short-circuits every "&&" guard as already settled, risking a term.reset() mid-keystroke right after a remount. - lastSnapshot losing its value forces one guaranteed extra full repaint on the next mirror-mode resync. Fix: spread the existing cache entry first, before the explicit field list, so any field TerminalCacheEntry declares survives a remount by default instead of needing to be named here. The explicit keys still come after the spread and win where the rebuild intentionally overwrites/resets a field (terminal, fitAddon, disposables, kbState, win32State, etc.) — spreading first cannot clobber them. Added a cache.test.ts case that sets all four fields, remounts on the same cacheKey, and asserts they survive; confirmed it fails against the prior literal and passes with the spread.
Fixes agentColorLocked/lastSnapshot/lastDataAt/lastInputAt being silently reset by mount()'s end-of-mount cache rebuild. Verified red (test fails without the spread) and green (483 tests, 30 suites).
… owning_tab_id Terminal.tab_id was overloaded: eight readers wanted the renderer LEAF id (tb-* root / tm-* split) and one (emit_external_activity) wanted the owning TAB, which state.tabs never contains for a split leaf. Renamed the field to renderer_terminal_id (semantics unchanged, #[serde(rename = "tab_id")] keeps the wire key identical in both directions) and added a genuinely new owning_tab_id field, defaulting to None so pre-P0-A payloads still deserialise. Pure mechanical rename at all 13 call sites; no behaviour change yet (pty_manager.rs keeps its pc-* fallback for now, removed in a later task).
list_terminals, create_terminal and get_terminal each hand-built the same identity JSON block and had already drifted (get_terminal omitted promptHook and used a different mode string). Factored them through one terminal_identity_json helper so they can't drift again, and it adds the new owningTabId field to all three. tabId keeps its existing meaning (the renderer leaf) as a deprecated alias of terminalId, so no existing client observes a change other than the additive field.
emit_external_activity only sent tabId, which for a split pane is the tm-* leaf. flagTabActivity resolves its argument against state.tabs, which holds only root tab ids, so a leaf silently matched nothing and the tab's activity indicator never lit. Extracted the payload into external_activity_payload so the routing contract is unit-testable, and it now also carries owningTabId/processId/rendererTerminalId. The legacy terminalId/tabId keys are unchanged for existing consumers.
Adds resolve_api_spawn_identity, the pure decision function behind the fix for the terminal_history primary-key collision: two API-created splits in one tab used to store the same tab_id, reaping one PTY and letting closing one pane delete the other's scrollback. Per spec D7, a fresh tm- leaf is minted whenever the resolved owner already has a live terminal registered against it -- not only when the caller supplied a pane_id. pane_id alone is not a reliable "this is a split" signal: it's optional in the MCP tool, and App.tsx Mode 2 splits an already-populated tab without one. Also closes ground-truth correction C3: a caller-supplied tm- id in the tab field used to be silently discarded and replaced with an unrelated fresh tb-, landing the pane in the wrong tab with no diagnostic. Now fails closed with a message naming the right field (owningTabId). Not yet wired into create_terminal -- that's the next task, kept separate so this one stays a pure, independently-testable decision.
spawn_terminal took one overloaded tab_id and, when it was None, fell back to the pc-* process id -- which persist_terminal_history then upserted like any other key, filing a history row under an id that cannot survive a restart (ground-truth correction C1: the field was never actually None at runtime because of this fallback). Split it into renderer_terminal_id (no fallback -- None means no renderer pane owns this PTY) and a new owning_tab_id parameter. This deliberately does not compile in isolation: the four call sites (api_server.rs create_terminal and fleet_local_run, commands.rs the in-process branch and host_fallback) are wired up by the next two tasks, which need the new owning_tab_id parameter to exist first.
Wires resolve_api_spawn_identity (previously added as a standalone, unused decision function) into create_terminal: both renderer identities are now resolved BEFORE the spawn call, so the Terminal registers with them up front instead of the single ambiguous tab_id the handler used to accept verbatim. The api:createTerminalTab payload gains processId/rendererTerminalId/ owningTabId alongside the existing terminalId/tabId keys, which keep their pre-existing meanings (backend process id and owning tab, respectively) so no existing renderer consumer breaks. Proven end to end at the storage layer: two API splits targeting the same tab now occupy two terminal_history rows, and closing one no longer deletes the other's scrollback (they used to share one row keyed by the tab id). This does not compile in isolation -- it depends on the owning_tab_id parameter Task 7 (commands.rs) still needs to thread through the sidecar path, and fleet_local_run (api_server.rs) still calls the old spawn_terminal signature. The tree recompiles once that lands too.
On the sidecar path the app terminalId IS the DashMap key, the reattach key, and the vt100 screen key -- ground-truth correction C2 confirmed four further maps (host_terminals, host_reattach_pending, host_stream_offsets, host_close_pending) key off that same value. P0-A must not disturb it, so the change here is purely additive: an owning_tab_id parameter threaded through create_terminal -> create_host_terminal -> register_host_terminal/ host_fallback, with the leaf/owner pair computed by one small helper (host_identity) shared by both terminal-registration call sites. A caller that sends no owner (a renderer that predates P0-A) degrades to the pre-P0-A behaviour: the pane owns itself. Task 11 (renderer) is what makes the owner always present going forward.
…fter fleet_local_run spawned with tab_id: None, then patched entry.renderer_terminal_id in after the fact -- the exact patch-after-spawn race review 062 F-01 already closed for create_terminal: a fast-exiting shell's exit-path persist can run in that window and file the final scrollback under the ephemeral pc- id. Mints the tb- identity before the spawn and passes it as both renderer_terminal_id and owning_tab_id, matching create_terminal's pattern. fleet_terminals also gains terminalId/owningTabId so GET /api/fleet/ terminals has identity parity with the other terminal responses; the MCP list_terminals tool proxies this body verbatim. This is the last of the four spawn_terminal call sites Task 6 left broken; the crate compiles clean again and the full suite is green (286 tests).
Ground-truth correction C1: before P0-A, Terminal.tab_id was never actually None at runtime (every write site wrapped it in Some(...)), so the "skip terminals with no renderer id" guard in persist_terminal_history was dead code that happened to look correct. Now that renderer_terminal_id can genuinely be None (a headless API/fleet spawn), and as defence in depth against ever reintroducing the old pc-* fallback, extracted the "is this a valid history key" decision into a pure history_key function: a pc-* process id -- regenerated on every spawn, so a row keyed by one is orphaned the moment the app restarts -- is never a valid key, same as None.
Design 011 §6 (review 086 Q6.3): assigning the Rust owning_tab_id field without this is a no-op for every UI-created terminal -- the renderer never sent an owner, so Task 7's backend parameter always saw None. TerminalService.createTerminal gains an optional 7th owningTabId parameter, threaded through the Tauri bridge and ElectronAPI type down to the create_terminal invoke (Tauri maps the camelCase JS key onto the Rust command's snake_case owning_tab_id parameter). TerminalPane resolves the owner via findTabIdByTerminalId (falling back to the pane's own terminalId for a root/solo pane) at both call sites: the first-spawn mount effect and handleRestart.
Ground-truth correction C4: App.tsx's handleExternalActivity did `detail.tabId ?? null` and only fell back to findTabIdByTerminalId when that was falsy. For a split pane the backend always sent a TRUTHY tm- leaf as tabId, so the fallback never ran -- and even if it had, detail.terminalId is the PROCESS id (a pc-* value), which matches no pane-tree leaf on the in-process path either. flagTabActivity resolves its argument against state.tabs, which holds only root tab ids, so a split pane's activity indicator was silently dropped in every case. Extracted resolveActivityTabId as a pure module (the house pattern -- runningActivity.ts/RunningActivityTracker.ts, notificationLogic.ts/ NotificationService.ts) so the resolution order is testable without React/Redux: prefer the new explicit owningTabId, then walk the pane tree for a renderer leaf (rendererTerminalId, then the deprecated tabId alias), then fall back to the legacy terminalId path for events from a backend build that predates P0-A. Every candidate is checked against the live tab set so a closed tab can never resurrect an indicator.
reconcileExistingTerminals grouped live PTYs by term.tabId to decide which duplicates to reap. Before the Task 4/5 backend fix, two API splits in one tab shared that value, so reconcile treated a live pane as a duplicate of its sibling and closed one of them outright. Extracted the grouping step as groupLiveTerminalsByLeaf, a pure module testable without fetch/localStorage/Redux, and switched the correlation key to term.terminalId -- the renderer LEAF -- explicitly not tabId, which api_server.rs's terminal_identity_json documents as a deprecated alias of the same field and could diverge from it in the future. The sort-newest-first step moves into the helper itself. Note: with the Task 2 backend response shape as actually landed, tabId and terminalId are wire-identical in every /api/terminals response (both copy renderer_terminal_id), so this change does not alter today's grouping output -- the reaping bug itself was already closed by Task 4/5 minting a distinct tm- leaf per split. This is defense-in-depth: it stops a future change to the deprecated tabId alias from silently reintroducing the collision here.
…ss id
handleAPICreateTerminalTab's Modes 1 and 2 both did
registerExistingTerminal(terminalId, terminalId) with a pc-* value on
both sides, then wrote that same process id into the pane tree as its
terminalId -- binding the pane-tree leaf to a process id instead of a
renderer identity. StateManager.sanitizeLayoutData later rewrites any
non-tb-/tm- leaf to a fresh tm-* on the next restore, orphaning the
binding this event set up and losing the pane's restored cwd.
resolveApiCreateIds (apiCreatedTab.ts) disambiguates the event payload:
Task 5's emit carries explicit processId/rendererTerminalId/owningTabId
alongside the legacy terminalId/tabId pair (terminalId on THIS event has
always been the process id, unlike a REST response where it's the
leaf), falling back to the legacy keys for an event from a backend that
predates P0-A. Modes 1 and 2 now register leaf -> process and seed the
pane tree with the leaf, matching the identity Task 4/5's backend
already minted for a split. Mode 0 is unaffected -- for a root create
leafId === owningTabId === targetTabId, so its existing
registerExistingTerminal(targetTabId, terminalId) stays correct.
Renamed two call sites' local `process ${processId}` log/registration
argument to the outer-scope `terminalId` (which already carries the
resolved process id) rather than the freshly destructured `processId`,
since both Mode 1 and Mode 2 separately re-declare a block-scoped
`processId` later in the same block to hold the post-split terminal's
own process id -- using the new binding there would have shadowed it.
Design 011 §6 (review 086 Q4). sanitizeLayoutData rewrites a non-tb-/tm-
split leaf to a fresh tm-* id, but terminalCwds is keyed separately and
was never remapped -- the saved directory stayed filed under the OLD
id and the restored pane silently lost its cwd, starting in the
profile default instead. Task 14 makes this reachable more often, since
an API split used to persist a pc-* leaf.
remapCwds (stateManagerCwd.ts) re-keys the saved directories using the
id-rewrite mapping sanitizeNode now records; an entry that already
exists under the new id is treated as fresher and wins.
sanitizeNode runs TWICE over the same logical tree -- once in the
tabPanes loop (whose output restoreTabPanesInPlace actually restores)
and once standalone over paneTree (whose output restoreState never
dispatches). For a legacy leaf needing regeneration, each pass calls
generateId('tm') independently and gets a different id; without a
guard the second (unused) id would overwrite the first (real) one in
terminalIdMap, and remapCwds would then re-key the cwd onto an id no
restored pane carries. The guard keeps the FIRST mapping, since the
tabPanes pass runs first. Added a dedicated end-to-end test against
sanitizeLayoutData itself (StateManager.sanitizeLayoutData.test.ts) --
the pure remapCwds unit tests can't see this double-pass collision.
Contributor
Author
External review: two independent reviewers, split verdictagy (Gemini 3.7 Flash High) — approved, no blocking bugs. It independently verified the serde codex (gpt-5.6-sol) — do not merge yet, two blocking findings agy missed. Both verified against the source before posting this. 🔴 CRITICAL — the leaf-uniqueness check is a TOCTOU race
Two concurrent That is precisely the invariant this PR exists to establish ( Needs atomic reservation across identity choice and registration, with rollback on spawn failure — a read followed by a later insert is not sufficient. 🟠 HIGH — deferred
|
…-leaf race External review 099 T2-F1 (CRITICAL). The leaf-uniqueness rule was decided by a read-only scan of `state.terminals` that completes BEFORE `spawn_terminal`, which registers the new Terminal LAST -- after PTY creation, writer, and screen parser (pty_manager.rs:862-871, an order that is load-bearing for the close/delete existence gate and is deliberately left alone here). Axum serves requests in parallel, so two POST /api/terminals with the same caller-supplied, currently unoccupied `tb-` owner and no `paneId` both scanned "unoccupied", both took `leaf == owner`, and both registered a live Terminal on the SAME renderer leaf -- one `terminal_history` PRIMARY KEY for two panes, the exact invariant P0-A exists to establish (design 011 §3 / success criterion 7). Fix: reserve the OWNER, not the leaf, for the decision -> registration window. * `RootLeafClaims` (state.rs): a DashMap of owners with an in-flight root-leaf claim. `try_claim` takes the reservation with ONE atomic insert -- returning whether it was newly inserted -- never contains-then-insert. * Order is claim FIRST, scan `terminals` SECOND. Scanning first only narrows the hole: A scans empty, A registers, A releases, B claims (now free) and still acts on its stale scan. Claiming first closes it, because a claim is released only after the winner's Terminal is visible. * `resolve_api_spawn_identity` now returns the guard alongside the identity: reserved + unoccupied -> tab root, leaf == owner; already claimed or already registered -> split, fresh `tm-` leaf. A `paneId` create is unconditionally a split and reserves nothing. * Release is RAII (`Drop`), so no early return can leak it, and the handler drops it explicitly right after `spawn_terminal` returns -- covering the failure path and making the guard's lifetime visible. A leaked claim would be degraded but safe (later creates mint `tm-`); releasing early is the unsafe direction. Tests (all fail against the pre-fix code, verified): * `a_create_inside_another_creates_spawn_window_cannot_take_the_same_root_leaf` drives the real interleaving -- B resolves while A holds its claim and has not yet registered -- and asserts distinct leaves, then that the post-release create still splits. * `only_one_of_many_racing_creates_gets_the_root_leaf` runs 8 real threads on a barrier, exercising the atomicity of the claim itself. * `a_failed_spawn_releases_the_owner_reservation` asserts the reservation is released when the spawn fails and the retry is a root again. cargo test: 292 passed, 0 failed.
… 099 T2-F2) `owning_tab_id` was written once at spawn and never again, so moving a pane into another tab left the backend naming the tab the pane had left. That is not cosmetic: the stale owner is echoed by get_terminal_detail/get_my_terminal, and the MCP tool descriptions tell an agent to pass that owningTabId back when it creates a sibling pane — so the agent's next pane is created in the wrong tab. External activity also lit the wrong (still-open) tab. Before P0-A a split pane's indicator was merely DROPPED; actively routing new work somewhere wrong is a regression, not a pre-existing condition, so the plan's "safe to defer / no worse than today" judgement was wrong. Backend: `state::retarget_owning_tab` + the `set_terminal_owning_tab` command. Keyed by the renderer LEAF (what the pane tree holds, unique per live pane, and the same identity on both spawn paths), matched and written under one shard guard. Guarded like the create path — fail closed on a `tm-` "tab", reject blank ids — and a leaf with no live PTY is a miss, not an error. Renderer: the update is driven off `panes.treesByTabId` itself rather than off the individual dispatch sites. The plan's proposed hook (TerminalService. bindProcess) cannot work — a moved pane already has a mapping and takes TerminalPane's reuse path without ever binding. Diffing the tree covers every reparent path by construction: same-window drag (movePaneToTab), cross-window drop (insertPaneIntoTab), detached-window boot and whole-tab reattach (addTabTree), reload/restore reattach, and any future programmatic move. resolveActivityTabId now treats the emitted owner as a HINT: the pane tree is consulted FIRST and wins whenever it has an answer, so a moved pane lights the tab it is in. The hint is kept as a fallback because it is the only answer available before the renderer has inserted an API-created pane into the tree, and on the headless/sidecar paths. Tests: retarget_owning_tab (inline, 7 cases) covers the owner update, leaf-not- map-key matching, misses and the rejected inputs; paneOwnership / paneOwnershipSync cover the diff rules and prove a real movePaneToTab dispatch reaches the bridge; externalActivity covers a moved pane's activity landing on the NEW tab for both a tm- and a tb- leaf.
…rminal sig Fixes review 099 non-blocking findings T2-F3 and T2-F4: - resolveApiCreateIds' legacy-backend fallback used to set leafId = owningTabId. Every caller that reads leafId (App.tsx Mode 1/Mode 2) mints a sibling pane in a tab that may already have an occupied root pane at leaf === owningTabId, so that fallback handed the new pane the root's own leaf -- overwriting the root's TerminalService mapping and duplicating its pane-tree identity for a legacy split event. Fall back to the unique processId instead, matching the pre-P0-A behaviour (a process id briefly doubling as a leaf until StateManager.sanitizeLayoutData remaps it to a fresh tm-* on next restore) -- a known, already-handled degradation rather than a fresh collision. Corrected the "keeps working" claim in the function's doc comment and updated/added unit tests. - Widened the local ElectronAPI.createTerminal interface in tauri-bridge.ts to seven params (added owningTabId), matching both the implementation (which already forwards it) and the global electron.d.ts declaration.
Contributor
Author
Both blocking findings fixed — verified🔴 T2-F1 (CRITICAL) — TOCTOU race closed with an atomic owner reservation
Three properties that make it actually correct rather than narrower:
🟠 T2-F2 (HIGH) — backend owning tab now tracks pane moves
🟡 T2-F3 / T2-F4
Verification (re-run independently, not just reported)
The +10 Rust and +17 renderer tests are the new regression coverage, including the concurrent-claim interleaving that the previous sequential pure-function tests structurally could not reach. Ready for another look. |
…ew 101 F1) The reservation added in 6941b4c lived only in api_server::create_terminal, so it serialised the REST path against itself and left the renderer's own create outside it entirely. A restart-in-place of a dead tab root spawns with renderer_terminal_id == owning_tab_id == tb-a; a REST create for the same tab landing before spawn_terminal's final terminals.insert scanned the tab as empty and took tb-a as its leaf too, registering one live leaf twice. Takes the same claim here, which closes the renderer-first ordering: the REST path's try_claim then returns None and correctly mints a tm- split. We claim but never refuse on contention — this call is a user action on a pane that already owns its leaf and must not fail — so the reverse ordering stays open by design. That one is a product question (which creator wins a contested root leaf), not a lock, and the warning makes it observable instead of silent. Options written up in fabric docs/progress/010. Decision extracted as a pure fn so it is testable without a tauri::State.
… 101 F2) A pane dragged between tabs while its own create is still in flight loses the move for the rest of the session. The tree subscription fires, but the backend can only retarget a terminal it has already registered and spawn_terminal registers LAST, so set_terminal_owning_tab matches nothing — and it reports that as Ok(()), which it must, since the renderer fires off its own tree lifecycle and a pane's PTY may legitimately not exist. The subscription has meanwhile advanced lastOwners, so no later tree change re-sends it. The pane then sits visibly in the new tab while get_terminal_detail keeps naming the old one, and an agent told to pass owningTabId back creates its next pane in the wrong tab. Re-asserts the tree's current owner at the one moment the race resolves: the create has returned, so the leaf IS registered. Costs zero IPC in the common case, where the owner the spawn carried still matches the tree. Wired at TerminalService.createTerminal, the single choke point every renderer create passes through. Tested on both sides — the rule in paneOwnershipSync, and the wiring in TerminalService, because 'nobody called them' is exactly how this regressed the first time.
Both e2e jobs run on [self-hosted] — the Windows box where bun is already installed and usually already running. oven-sh/setup-bun downloads a fresh copy and fails to overwrite it with 'EBUSY: resource busy or locked', which shows up as a random red X on an otherwise green PR. It is what failed this PR's e2e run; nothing in the suite itself broke. rust-tests.yml has carried the fix for a while and e2e-tests.yml was simply missed. Ported verbatim rather than invented: install on GitHub-hosted, verify on Windows, so a future move off self-hosted still gets bun.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Terminal.tab_idwas overloaded. Eight of nine readers wanted the renderer leaf id (tb-*root pane,tm-*split); the ninth wanted the owning tab. One writer stored an owning-tab id. For a tab's root pane leaf == owner, which is why the conflation stayed invisible — until an API-created split existed.Two bugs live in shipped code today
1. Two MCP/API-created splits in one tab collide.
api_server.rsaccepted a caller-suppliedtabIdverbatim, so both terminals stored the sametab_id. That field is theterminal_historyPRIMARY KEY (history_store.rs:93-98), so:StateManager.reconcileExistingTerminalsgrouped by it, saw duplicates, and closed the older PTY;2. Split panes never lit up their tab.
emit_external_activitysent a leaf id whereflagTabActivityresolves againststate.tabs, which holds only root tab ids — so the indicator silently no-oped for every split pane, not just API-created ones.Three more found on the way
spawn_terminalfell back to thepc-*process id when no renderer id was supplied, writing history rows keyed by an id that cannot survive a restart.fleet_local_runpatchedtab_idafter spawn — reintroducing the exact race review 062 F-01 already fixed forcreate_terminal.api_server.rsrecognised onlytb-, silently discarding any caller-suppliedtm-id and minting an unrelatedtb-in its place.What changed
Terminal.idrenderer_terminal_id(wastab_id)Option, and nowNonefor a headless spawn rather than falling back topc-*owning_tab_id(new)Wire contract is additive.
tabIdkeeps its current meaning and value as a deprecated alias ofterminalId;owningTabIdis new.#[serde(rename = "tab_id")], deliberately notalias—aliasaccepts the old key inbound but emits the Rust field name, silently changing the output contract. A test asserts the emitted key.The minting rule is the real invariant: a renderer leaf id is unique per live terminal.
pane_idwas only ever a proxy for it, so minting triggers onpane_id.is_some() || owner_has_live_terminal(owner). Review found that keying onpane_idalone still reproduced the collision, because the MCP tool documentspaneIdas optional andApp.tsxhas a whole branch for "tabId given, no paneId — split a pane in that tab".Verification
cargo test(src-tauri)bun test)tsc --noEmitRust tests are inline
#[cfg(test)]only — theintegration-testsfeature breaks the Windows test binary. New regression tests are named after the bugs, e.g.two_api_splits_get_distinct_leaves_and_one_owner,second_create_into_a_populated_tab_gets_a_distinct_leaf,a_pane_leaf_id_in_the_tab_field_is_rejected_not_silently_replaced,a_process_id_is_never_a_history_key.Not in scope
tabIdJSON alias.TERMFLOW_TERMINAL_IDresolving to a different id namespace on the in-process vs sidecar spawn paths — real, documented, separate.Note for reviewers
This branch currently also contains the 2 commits from #31, which is not yet on
develop. Merge #31 first and this diff shrinks to P0-A alone.