Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -431,3 +431,133 @@ describe("terminalRuntimeRegistry eviction cleanup", () => {
}
});
});

describe("terminalRuntimeRegistry.getRegisteredTerminalIds", () => {
const keysByTerminalId = (
terminalRuntimeRegistry as unknown as {
entryKeysByTerminalId: Map<string, Set<string>>;
}
).entryKeysByTerminalId;

afterEach(() => {
keysByTerminalId.delete("registered-a");
keysByTerminalId.delete("registered-b");
});

test("reports every registered terminal, parked ones included", () => {
keysByTerminalId.set("registered-a", new Set(["registered-a\u0000one"]));
keysByTerminalId.set(
"registered-b",
new Set(["registered-b\u0000one", "registered-b\u0000two"]),
);

const ids = terminalRuntimeRegistry.getRegisteredTerminalIds();

expect(ids.has("registered-a")).toBe(true);
expect(ids.has("registered-b")).toBe(true);
expect(ids.has("never-registered")).toBe(false);
});

test("returns a copy, so reclaim cannot mutate registry state", () => {
keysByTerminalId.set("registered-a", new Set(["registered-a\u0000one"]));

const ids = terminalRuntimeRegistry.getRegisteredTerminalIds();
ids.delete("registered-a");

expect(keysByTerminalId.has("registered-a")).toBe(true);
});

test("still reports a released terminal whose snapshot was preserved", () => {
// `release` persists the buffer and then drops the registry entry, leaving
// the snapshot on disk for the next mount to restore. Reclaim must not read
// that missing entry as "orphaned" and delete the scrollback of a pane the
// user still has open.
const terminalId = "released-but-restorable";
const entryKey = `${terminalId}\u0000${terminalId}`;
const registryInternals = terminalRuntimeRegistry as unknown as {
entries: Map<string, unknown>;
entryKeysByTerminalId: Map<string, Set<string>>;
disposeEntry: (entry: unknown, options: unknown) => void;
};
const entry = {
terminalId,
instanceId: terminalId,
runtime: {
terminalId,
serializeAddon: { serialize: () => "scrollback" },
lastCols: 120,
lastRows: 32,
},
transport: {},
linkManager: null,
pendingLinkHandlers: null,
disposeBufferChangeListener: null,
lastUsedAt: 1,
};
const realDisposeEntry = registryInternals.disposeEntry;
registryInternals.entries.set(entryKey, entry);
registryInternals.entryKeysByTerminalId.set(
terminalId,
new Set([entryKey]),
);
// Stub only the runtime teardown (it needs real xterm/DOM), keeping the
// bookkeeping under test: deleteEntry plus the preserved-snapshot record.
registryInternals.disposeEntry = (disposed, options) => {
(
terminalRuntimeRegistry as unknown as {
recordDisposedEntrySnapshot: (e: unknown, o: unknown) => void;
deleteEntry: (e: unknown) => void;
}
).recordDisposedEntrySnapshot(disposed, options);
(
terminalRuntimeRegistry as unknown as {
deleteEntry: (e: unknown) => void;
}
).deleteEntry(disposed);
};

try {
terminalRuntimeRegistry.release(terminalId);

expect(fakeStorage.values.has(`terminal-buffer:${terminalId}`)).toBe(
true,
);
expect(registryInternals.entryKeysByTerminalId.has(terminalId)).toBe(
false,
);
expect(
terminalRuntimeRegistry.getRegisteredTerminalIds().has(terminalId),
).toBe(true);
} finally {
registryInternals.disposeEntry = realDisposeEntry;
registryInternals.entries.delete(entryKey);
registryInternals.entryKeysByTerminalId.delete(terminalId);
terminalRuntimeRegistry.dispose(terminalId);
}
});

test("stops reporting a terminal once dispose clears its snapshot", () => {
// Seed the preserved record rather than storage: getRegisteredTerminalIds
// reads the registry maps, so a storage-only fixture would report absent
// before dispose too and assert nothing.
const terminalId = "disposed-terminal";
const preserved = (
terminalRuntimeRegistry as unknown as {
preservedSnapshotTerminalIds: Set<string>;
}
).preservedSnapshotTerminalIds;
fakeStorage.values.set(`terminal-buffer:${terminalId}`, "scrollback");
preserved.add(terminalId);

expect(
terminalRuntimeRegistry.getRegisteredTerminalIds().has(terminalId),
).toBe(true);

terminalRuntimeRegistry.dispose(terminalId);

expect(
terminalRuntimeRegistry.getRegisteredTerminalIds().has(terminalId),
).toBe(false);
expect(fakeStorage.values.has(`terminal-buffer:${terminalId}`)).toBe(false);
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
});
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,14 @@ class TerminalRuntimeRegistryImpl {
private useSeq = 0;
private pendingEviction: ReturnType<typeof setTimeout> | null = null;
private persistFailureWarnedTerminalIds = new Set<string>();
/**
* Terminals whose persisted snapshot outlived their registry entry. Release
* and eviction write the buffer and then drop the entry, so the entry map
* alone would report a still-restorable terminal as unknown — and anything
* reclaiming storage would delete the scrollback of a pane the user still has
* open. Cleared by `dispose`, which is what actually removes the snapshot.
*/
private preservedSnapshotTerminalIds = new Set<string>();
/**
* Cap on parked (hidden) xterm runtimes. Each live runtime holds its full
* scrollback and a WebGL context (~55–70 MB RSS measured), so parked
Expand Down Expand Up @@ -125,6 +133,32 @@ class TerminalRuntimeRegistryImpl {
return firstKey ? (this.entries.get(firstKey) ?? null) : null;
}

/**
* Terminals whose persisted snapshot must survive: those with a live entry,
* plus those released or evicted while preserving their buffer for restore.
*/
getRegisteredTerminalIds(): Set<string> {
return new Set([
...this.entryKeysByTerminalId.keys(),
...this.preservedSnapshotTerminalIds,
]);
}

/**
* Mirrors what `disposeRuntime` does with the snapshot: `preserve` keeps it on
* disk for a later mount, anything else has already cleared it.
*/
private recordDisposedEntrySnapshot(
entry: RegistryEntry,
options: { persistedState?: "clear" | "preserve" } = {},
) {
if (options.persistedState === "preserve") {
this.preservedSnapshotTerminalIds.add(entry.terminalId);
return;
}
this.preservedSnapshotTerminalIds.delete(entry.terminalId);
}

private getEntries(terminalId: string): RegistryEntry[] {
const keys = this.entryKeysByTerminalId.get(terminalId);
if (!keys) return [];
Expand Down Expand Up @@ -394,6 +428,7 @@ class TerminalRuntimeRegistryImpl {
if (entry.runtime) {
disposeRuntime(entry.runtime, options);
}
this.recordDisposedEntrySnapshot(entry, options);
this.deleteEntry(entry);
}

Expand Down Expand Up @@ -444,6 +479,7 @@ class TerminalRuntimeRegistryImpl {
}
// Eviction deletes the live registry entry but deliberately leaves its
// snapshot behind. Closing that pane must still clear the orphaned keys.
this.preservedSnapshotTerminalIds.delete(terminalId);
clearPersistedRuntimeState(terminalId);
}

Expand Down
24 changes: 15 additions & 9 deletions apps/desktop/src/renderer/lib/terminal/terminal-runtime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ import type { ProgressAddon } from "@xterm/addon-progress";
import type { SearchAddon } from "@xterm/addon-search";
import { SerializeAddon } from "@xterm/addon-serialize";
import { Terminal as XTerm } from "@xterm/xterm";
import { DEFAULT_TERMINAL_SCROLLBACK } from "shared/constants";
import {
DEFAULT_TERMINAL_SCROLLBACK,
TERMINAL_BUFFER_KEY_PREFIX,
TERMINAL_DIMS_KEY_PREFIX,
} from "shared/constants";
import {
applyTerminalFontFamilyCssVariable,
type TerminalAppearance,
Expand All @@ -24,8 +28,6 @@ import { getTerminalParkingContainer } from "./terminal-parking";
import { installInputModeReclaimer } from "./terminalInputModeReclaimer";

const SERIALIZE_SCROLLBACK = 1000;
const STORAGE_KEY_PREFIX = "terminal-buffer:";
const DIMS_KEY_PREFIX = "terminal-dims:";
const DEFAULT_COLS = 120;
const DEFAULT_ROWS = 32;
const RESIZE_DEBOUNCE_MS = 75;
Expand Down Expand Up @@ -95,7 +97,7 @@ function persistBuffer(
): boolean {
try {
const data = serializeAddon.serialize({ scrollback: SERIALIZE_SCROLLBACK });
localStorage.setItem(`${STORAGE_KEY_PREFIX}${terminalId}`, data);
localStorage.setItem(`${TERMINAL_BUFFER_KEY_PREFIX}${terminalId}`, data);
return true;
} catch {
return false;
Expand All @@ -104,7 +106,9 @@ function persistBuffer(

function restoreBuffer(terminalId: string, terminal: XTerm) {
try {
const data = localStorage.getItem(`${STORAGE_KEY_PREFIX}${terminalId}`);
const data = localStorage.getItem(
`${TERMINAL_BUFFER_KEY_PREFIX}${terminalId}`,
);
if (data) terminal.write(data);
} catch {}
}
Expand All @@ -126,7 +130,7 @@ export function tryPersistRuntimeState(runtime: TerminalRuntime): boolean {

function clearPersistedBuffer(terminalId: string) {
try {
localStorage.removeItem(`${STORAGE_KEY_PREFIX}${terminalId}`);
localStorage.removeItem(`${TERMINAL_BUFFER_KEY_PREFIX}${terminalId}`);
} catch {}
}

Expand All @@ -137,7 +141,7 @@ function persistDimensions(
): boolean {
try {
localStorage.setItem(
`${DIMS_KEY_PREFIX}${terminalId}`,
`${TERMINAL_DIMS_KEY_PREFIX}${terminalId}`,
JSON.stringify({ cols, rows }),
);
return true;
Expand All @@ -150,7 +154,9 @@ function loadSavedDimensions(
terminalId: string,
): { cols: number; rows: number } | null {
try {
const raw = localStorage.getItem(`${DIMS_KEY_PREFIX}${terminalId}`);
const raw = localStorage.getItem(
`${TERMINAL_DIMS_KEY_PREFIX}${terminalId}`,
);
if (!raw) return null;
const parsed = JSON.parse(raw);
if (typeof parsed.cols === "number" && typeof parsed.rows === "number") {
Expand All @@ -164,7 +170,7 @@ function loadSavedDimensions(

function clearPersistedDimensions(terminalId: string) {
try {
localStorage.removeItem(`${DIMS_KEY_PREFIX}${terminalId}`);
localStorage.removeItem(`${TERMINAL_DIMS_KEY_PREFIX}${terminalId}`);
} catch {}
}

Expand Down
Loading