Skip to content

fix(restore): a terminal's last frame stays through a slow attach, the quit and a daemon restart (dogfood port) - #31851

Merged
lawrencecchen merged 5 commits into
feat-cmux-nextfrom
fix/dogfood-port-carried-frame
Nov 2, 2026
Merged

lawrencecchen merged 5 commits into
feat-cmux-nextfrom
fix/dogfood-port-carried-frame

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Ports 0bb9b66 (= d70bdd0), d22c177 (= faeb78e), b09da9e (= 13c71aa), 3d2b2ee (= e0d7c39) and bfc0f69 (= 091de45) from cp/dogfood-v2-2-12-opt (audit handoff/dogfood-only-fixes.md, row 6). Feat has its own simpler frame hold (no renderer-reported presentation, no MainDeadline), so this is a port onto feat's code, not a cherry-pick.

What the person sees on feat without it:

  1. After a relaunch with many workspaces, a terminal shows its last frame for 3 s, then its empty surface with a cursor box under "Restoring…" until the slow attach answers. TerminalFrameHold.begin(lastFrame:) ended the hold after contentLimit (3 s). Now the last frame stays until the session's first content is parsed and drawn (Chrome keeps a tab's last frame until new content paints). A failed link still says so in the banner over it.
  2. A quit during a slow attach saves the empty surface under the hold as the terminal's frame, so the next launch shows an empty pane. The hold now keeps its frame typed (HeldFrame: a presented IOSurface or a decoded picture) and TerminalSession.shownFrameInBackground returns it while the hold is on screen, else the surface snapshot. The quit's save and the hover card use it.
  3. After a daemon restart (new generation), TabContentCache makes a new view of each tab's terminal and the old view's frame goes with it: an empty pane for 0.6-4.8 s at 600 workspaces (dogfood bench-restore). The cache now hands the old view's shown frame to the new view (carriedFrame(for:) / showsTerminal(_:carrying:)), only when the view shows the same daemon terminal (DaemonService.terminalIdentity(of:): its term_ id, or the tab's content id while the restarted daemon still adopts it, else its surface handle; for a remote tab, its terminal and resource). A carried surface frame is reclaimed and given away (TerminalSurfaceView.giveAway), so the closed old surface never marks it volatile under the new view.

The view keys (terminalViewValidity, the identity) are pure functions of the tab and the daemon, so they move from CmuxNextApp to CmuxNextDaemonService (DaemonService+TerminalViews.swift); CmuxNextApp lines -3. No main-thread blocking: the held surface is rendered in a detached task, as TerminalLastFrame.snapshot does.

Testing

Head 5747137. Red ref 3a160fb (branch fix/dogfood-port-carried-frame-red: feat 9ce095b + the head's tests + the read-only TerminalFrameHold.hasDeadline accessor, no fix).
Focused command (fleet): package-test-lane.sh suite Packages/macOS/CmuxNext TerminalLaunchFrameTests,TerminalRestartCarriesFrameTests via proof-step.py.

  • Compile-check v2 (CmuxNext, app, cmux-tui): head ac04556975aeea4f2bdc47be passed; red ref 776a2adea637b95720c7ab85 passed.
  • RED at 3a160fb, step bdca42330893519346906deb (aws-m4pro-4): 4 tests fail as expected: aLastFrameStaysUntilTheTerminalsContentDrawsHoweverLongTheAttachTakes (a 3 s timer is armed), aQuitDuringASlowAttachSavesTheHeldFrameNotTheEmptySurface (no frame saved), aDaemonRestartKeepsTheTerminalsLastFrameOnScreen and aTerminalStillAdoptingKeepsItsLastFrame (the new view holds nothing). anotherTerminalInTheTabDoesNotShowTheOldTerminalsFrame is the guard against a false carry: it passes on both (it fails only if the identity check is removed).
  • GREEN at 5747137, step 4f9ae8788a76225324a6c4df (aws-m4pro-8): both suites 5 of 5 runs, 12 tests each run, 0 failures.
  • Earlier head 9acc0c7: red step 2693f97b4fa7f34ea0acc04b (red ref 8acaf62), green 5x step 3221780623886b77a28df776; superseded by the review repair.

Observed / Not run

Observed: the fleet red and green runs above (Swift Testing output read per test).
Not run: a live daemon restart in a tagged build (bench-restore restart); no test carries an IOSurface frame (all carry tests use a decoded launch frame) or a remote-terminal frame; the hover card path is covered by reading only. Follow-up (review, non-blocking): a held surface on a hidden session stays non-volatile while held.

Changelog

Fixed: After a daemon restart, a slow attach or a quit during one, a terminal keeps showing its last content instead of an empty pane

Proof

No capture: the change keeps an existing frame on screen longer during a restart or slow attach; a still image cannot show the difference, and the tests above check the held frame directly.

… and a daemon restart (red)

Red tests for the dogfood-only restore fixes (cp/dogfood-v2-2-12-opt 0bb9b66, d22c177,
b09da9e, 3d2b2ee, bfc0f69):
- TerminalLaunchFrameTests.aLastFrameStaysUntilTheTerminalsContentDrawsHoweverLongTheAttachTakes:
  the hold gives up after 3 s and the pane shows its empty surface.
- TerminalLaunchFrameTests.aQuitDuringASlowAttachSavesTheHeldFrameNotTheEmptySurface: the quit saves
  the surface under the hold, not the frame the pane shows.
- TerminalRestartCarriesFrameTests: after a daemon restart (a new generation) the tab's new view
  shows an empty pane; a tab that shows another terminal must not show the old frame.
…e quit and a daemon restart

Ports 0bb9b66 (= d70bdd0), d22c177 (= faeb78e), b09da9e (= 13c71aa),
3d2b2ee (= e0d7c39) and bfc0f69 (= 091de45) from cp/dogfood-v2-2-12-opt, onto
feat's own frame hold (feat has no renderer-reported presentation or MainDeadline; the volatile
frame stays in TerminalSurfaceView).

- TerminalFrameHold.begin(lastFrame:) ended a held last frame after 3 s when no content had come.
  A relaunch whose daemon restores hundreds of terminals answers an attach seconds late, so the
  pane showed its empty surface (a cursor box) under "Restoring..." until the attach came back.
  The last frame now stays until the session's first content is parsed and drawn (Chrome keeps a
  tab's last frame until the new content paints); a failed link says so in the banner over it.
- The quit saved each terminal's surface: a quit during a slow attach saved the empty surface
  under the hold, and the next launch showed that as the terminal's last frame. The hold keeps its
  frame typed (HeldFrame: a presented IOSurface or a decoded picture), and
  TerminalSession.shownFrameInBackground returns it while the hold is on screen, else the surface's
  snapshot. The quit's save and the hover card use it.
- A daemon restart gives the terminal a new generation, so TabContentCache makes a new view of
  the tab's terminal, and the old view's held frame went with it: an empty pane for 0.6-4.8 s at
  600 workspaces. The cache now hands the old view's shown frame (carriedFrame: the held frame,
  else the surface's presented frame once it drew content) to the new view (holdCarriedFrame),
  only when the view shows the same daemon terminal (DaemonService.terminalIdentity(of:): its term_
  id or the tab's content id while the daemon still adopts it, else its surface handle; for a
  remote tab, its terminal and resource). A carried surface frame is reclaimed and given away
  (TerminalSurfaceView.giveAway), so the closed old surface never marks it volatile under the new
  view.
- The view keys (terminalViewValidity, the terminal identity) are pure functions of the tab and
  the daemon, so they move from CmuxNextApp to CmuxNextDaemonService (DaemonService+TerminalViews).

Tests (red at 8acaf62): TerminalLaunchFrameTests
aLastFrameStaysUntilTheTerminalsContentDrawsHoweverLongTheAttachTakes,
aQuitDuringASlowAttachSavesTheHeldFrameNotTheEmptySurface; TerminalRestartCarriesFrameTests.
@lawrencecchen lawrencecchen added the risk:normal Risk tier (docs/ci/risk-tiers.md) label Nov 2, 2026
… identity at the confirm

A Cmd-D split's view is made for the provisional tab (a surface handle only) and keeps its session
when the daemon's tab replaces it; its identity now follows to the daemon terminal, so a later
daemon restart carries its frame too.
@cursor

cursor Bot commented Nov 2, 2026

Copy link
Copy Markdown

Bugbot needs on-demand usage enabled

Bugbot uses usage-based billing for this team and requires on-demand usage to be enabled.

A team admin can enable on-demand usage in the Cursor dashboard.

@blacksmith-sh

blacksmith-sh Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

Found 2 test failures on Blacksmith runners:

Failures

Test View Logs
test_every_compile_job_names_its_warm_sources (main.ThisTree.test_every_compile_job
_names_its_warm_sources)/
test_every_compile_job_names_its_warm_sources (main.ThisTree.test_every_compile_job
_names_its_warm_sources)
View Logs
test_the_repository_highlights_validate (main.DigestTests.test_the_repository_highl
ights_validate)/
test_the_repository_highlights_validate (main.DigestTests.test_the_repository_highl
ights_validate)
View Logs

Fix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (independent review subagent): FIX at 9acc0c7

Read-only review of the diff 9ce095b..9acc0c7 and the dogfood commits it ports (0bb9b66, d22c177, b09da9e, 3d2b2ee, bfc0f69). Nothing was compiled or run locally.

The core port is correct: the identity check (machine prefix, then the term_ id, else the content id, else the surface), the confirm hand-off, giveAway + reclaim before the old view closes, and the off-main render in shownFrameInBackground. No callers of contentLimit, HeldFrameView.shown or terminalValidity remain. Two items keep this PR from landing: a CI red that this PR causes, and a guard test that never runs its assertion.

Must fix

  1. This PR causes a CI red. Packages/macOS/CmuxNext/Tests/CmuxNextAppTests/TerminalLaunchFrameTests.swift:110: try await Task.sleep(for: .milliseconds(3_500)) followed by an assertion. The quality-determinism guard (cmux-next CI guard group, run 41393291125 on this head) reports sleep-then-assert at this line: "1 guard steps red on this change, 0 red on the base too". The test also adds 3.5 s to the suite.
    Fix: assert the cause directly, with no wait. For example, expose TerminalFrameHold.waitsOnTimer (= deadline.isScheduled; DemandTimer.isScheduled exists) and #expect(!pane.session.frameHold.waitsOnTimer) right after frames.show(...). This is red at base (the contentLimit timer is armed there) and green at head.

  2. The false-carry guard test never reaches its assertion. TerminalRestartCarriesFrameTests.swift:75: guard moved.id == tab.id else { return }. The snapshots have no tabResourceID and no terminalID, so TabModel.identity gives surface:1, then surface:2. The ids always differ, and the test returns before its #expect. The PR body says it "passes on both", and this is why. The rule "a tab that now shows another terminal does not carry the frame" has no real test.
    Fix: give both snapshots the same tabResourceID. Use terminalResourceID/contentResourceID term_A in g1 and term_B on a new surface. Replace the guard with try #require(moved.id == tab.id). Then check that the test fails when carriedFrame(for:) ignores the identity.

Should fix

  1. The hover card does not show the held frame (intent 2 is only half done). TabContentCache+Previews.swift:17: guard entry.session.showsOutput else { return nil } runs before shownFrameInBackground. A held session has a fresh, empty surface: viewportText() returns "", and the new model's lastCommand is nil. So showsOutput is false for every launch, evicted or carried hold, and the new call is unreachable for those sessions. Dogfood has the same guard. The PR body says this path was "covered only by reading", so nothing caught it.
    Fix: guard entry.session.showsOutput || entry.session.isHoldingFrame (expose frameHold.isHolding as public). Also, the .image case returns the full-size launch picture and ignores maxPixelSize, so previews.insert caches a full-resolution image. Downscale it, or confirm that the preview cache accepts that cost.

Low / follow-up

  1. TerminalSession.close() (TerminalSession.swift:409) does not call frameHold.end(). Before, the 3 s timer released a last-frame hold. Now, a closed session keeps its held IOSurface or CGImage as long as anything retains the session object. Suggest frameHold.end() in close().
  2. Purgeability (cx-8w027.19.1): a held .surface frame (carried, or an evicted TerminalLastFrame) on a suspended (hidden) session now stays non-volatile with no time limit. Example: a hidden view whose re-attach waits for an event, per the DaemonTerminalIO docs. Before, this was at most 3 s. The warm set limits the count (24 x about 20 MB at worst). A follow-up could make the held surface volatile while the session is suspended, then reclaim it (or drop it if it was purged) on reveal, as holdPresentedFrame does.
  3. Coverage: every carry test uses a .image launch frame. No test covers the IOSurface path this PR adds (giveAway, reclaim, the closed old surface not marking the carried frame volatile), or the remote-terminal carry (TabContentCache+RemoteTerminals.swift). One test should carry a .surface frame from a paused old view and check it stays non-volatile after the old view closes.
  4. Nit: TerminalFrameHold.swift:185 refers to TerminalSession/carriedFrame(). The method is now carriedFrame(for:).
  5. The PR body says "Fleet jobs: pending" and "Observed: pending". No red/green run of the focused tests is recorded yet.

Checked, no defect

  • IOSurface lifecycle: carriedFrame gives the frame away before the old view closes. The old view's holdPresentedFrame(volatile: true) then skips it, and reclaim() refuses a purged frame. The old session closes in the same turn (stale.close() / discardTerminal), so no paused old surface can mark the frame volatile under the new hold.
  • Holds without a limit: a hold ends on the first replay, snapshot or output plus the 500 ms draw limit. A missing lane ends it at once. No display link runs before content, so a hold that waits has no per-frame cost. A failed attach or a dead tab keeps the old content under the status banner, which is the intended behavior.
  • Identity: term: / surface: / remote# prefixes and machineID keep different terminals and machines apart. content_resource_id is the terminal's public id for terminal tabs, so the "still adopting" carry matches the same terminal.
  • Provisional (Cmd-D): a gated entry is never replaced (early return), and the confirm sets the real identity.
  • Main thread: only IOSurfaceSetPurgeable runs on main (already used there). The GPU render is in Task.detached. No blocking calls are added.
  • Swift 6: TerminalCarriedFrame is @MainActor, and only SendableSurface crosses into the detached task.
  • Other reds on this head (tooling-tests, release-notary highlights attachment-drag-follows-pointer.md) are base findings, not from this PR.

… a closed session drops it, the tests need no sleep

- The hover card returned no picture for a held session (its fresh surface shows no output yet);
  it now pictures the held frame, scaled to the card's size off the main thread.
- TerminalSession.close() ends the frame hold: with no time limit, it went only with the object.
- TerminalLaunchFrameTests checks that no timer ends the last frame (TerminalFrameHold.hasDeadline)
  instead of sleeping 3.5 s; anotherTerminalInTheTabDoesNotShowTheOldTerminalsFrame keeps the tab id
  (tabResourceID) so its assertion runs.
@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

cmux status (one comment, edited in place; each line is updated by the job that owns it)

Review 5747137: HQ feed card · gallery before/after runs · checks

App of this head (app.zip, kept 1 day)

gh workflow run reload-build.yml -R manaflow-ai/cmux --ref feat-cmux-next -f tag=pr31851 -f ref=fix/dogfood-port-carried-frame -f platform=macos -f backend_mode=local

Shipped in:

The feat-cmux-next push run https://github.com/manaflow-ai/cmux/actions/runs/41399258629 at 00ae302 failed: cmux-next checks (god files, concurrency, crash safety, l10n).
Those jobs last passed at 54efdc7. This pull request is one of 2 merged since: #31764, #31851.
If the failure is in your change, fix forward on feat-cmux-next. Pull requests run only the tiers their paths reach (docs/ci/cmux-next-tiers.md); label a batch PR full-ci to run them all before merging.

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (independent review subagent, delta): SHIP at 5747137

Read-only review of the repair delta 9acc0c7..5747137 and of the whole PR 9ce095b..5747137. The new red ref 3a160fb was also checked: it is base plus the head's tests plus a read-only hasDeadline accessor. Nothing was compiled or run locally.

Earlier FIX findings: all fixed

  1. sleep-then-assert (CI red): fixed. TerminalLaunchFrameTests.swift now checks !pane.session.frameHold.hasDeadline right after frames.show(...), with no sleep. hasDeadline is deadline.isScheduled. At head, only waitForDraw() schedules deadline, and only with the 500 ms limit. The swap hold and contentQueued() call waitForDraw(), so the property shows only the post-content limit. At the red ref, base begin(lastFrame:) schedules contentLimit. TerminalLaunchFrames.show gets there through takeOver and TerminalLaunchFrame.show(on:), so the test is red at the red ref and green at head. The cmux-next CI guard group passes on this head.

  2. "Another terminal" guard test: fixed. Both snapshots now have the same tabResourceID, so TabModel.identity gives the same tab id. try #require(moved.id == tab.id) replaces the early return. The terminal ids differ (term_aaaa… vs term_bbbb…), so terminalIdentity(of:) differs and carriedFrame(for:) returns nil. If the identity check were removed, the old session's held .image frame would be carried, holdCarriedFrame would begin a hold on the empty new surface, and #expect(!second.session.frameHold.isHolding) would fail. The test now proves the rule.

  3. Hover card: fixed. previewImage now passes showsOutput || showsHeldFrame, so a held session reaches shownFrameInBackground. The .image case now scales down in a Task.detached(priority: .utility) through the new TerminalSnapshotRenderer.render(_: CGImage, maxPixelSize:). Only the size comparison runs on main. The quit save passes .greatestFiniteMagnitude and still gets the image unchanged.

  4. close() ends the hold: fixed, and the carry is safe. The order is correct in both places:

    • TabContentCache.terminal(for:daemon:) calls carriedFrame(for:) (on the line with let identity = …) before terminals.removeValue / stale.close().
    • remoteTerminal calls it before discardTerminal(key) (which calls stale.close()).

    TerminalCarriedFrame holds its own strong reference to the IOSurface or CGImage. end() only clears the old HeldFrameView contents and the hold's held, and it does not change purgeability, so the new hold keeps its frame. Eviction reads lastFrames.evict(entry.session, …) before entry.close(). The quit save reads live cache.terminals, not closed sessions. FrameClient.deactivate() and DemandTimer.cancel() are idempotent. On a session that never held, end() in close() only creates the lazy FrameClient, which costs almost nothing.

Nit 7 (the doc link carriedFrame(for:)) is also fixed.

Preview cache with showsOutput || showsHeldFrame

Safe. For a live terminal entry, every hover renders again. previews is only the fallback when a render returns nil, and it is overwritten on the next successful render. The cached image is now at most maxPixelSize, not the full launch picture.

New defects

None found in code.

Earlier "checked, no defect" items: still hold

The delta does not touch these: IOSurface give-away and reclaim before the old view closes, hold end conditions, identity prefixes, the provisional confirm, main-thread work and Swift 6 sendability. CGImage already crosses detached tasks as their result type.

Before ready-to-land (PR body, not code)

  • The body still names red ref 8acaf62 and green 4ee1b43. It also says the guard test "passes on both", the hover-card path is "covered only by reading", and "Fleet jobs: pending" / "Observed: pending". Update it: give red ref 3a160fb and green 5747137, and record the focused fleet run (both TerminalLaunchFrameTests tests and the 3 TerminalRestartCarriesFrameTests) red at the red ref and green at head. Also record the anotherTerminal… mutation result if you ran it. cmux-next swift test is still pending on this head.
  • Non-green checks on this head are base findings, not from this PR (it changes only Swift files under Packages/macOS/CmuxNext): tooling-tests (test_every_compile_job_names_its_warm_sources), release-notary (highlights validation), and the nightly-guards / Guard status wrapper over them.

Low / follow-up (not blockers, unchanged)

  • A held .surface frame on a hidden (suspended) session stays non-volatile with no time limit (earlier item 5).
  • No test carries an IOSurface frame or a remote-terminal frame (earlier item 6).
  • Nit: shownFrameInBackground checks the .image size, and render(_: CGImage, …) checks it again. This costs nothing.

@lawrencecchen lawrencecchen added the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Nov 2, 2026
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Non-green checks at 5747137, classified: tooling-tests, nightly-guards / workflow-guard-tests / release-notary and its Guard status wrapper are base findings (this PR changes only Swift files under Packages/macOS/CmuxNext; the delta reviewer confirmed). ready-to-land added after the delta SHIP at this head.

@lawrencecchen
lawrencecchen merged commit 00ae302 into feat-cmux-next Nov 2, 2026
192 of 200 checks passed
@lawrencecchen
lawrencecchen deleted the fix/dogfood-port-carried-frame branch November 2, 2026 06:20

This branch was previously deployed

1 inactive deployment
ci-routing-agent — 5747137d Deployed Nov 2, 2026 by lawrencecchen via namespace autofix #11995
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds risk:normal Risk tier (docs/ci/risk-tiers.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant