mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
fix(review): clamp env-path trim below max; revert unwired scrollback raise (PR #138)
- UNBOUNDED-MEMORY: DEFAULT_TERMINAL_BUFFER_TRIM_BYTES from CODEMAN_TRIM_TERMINAL_TO had no relation to DEFAULT_TERMINAL_BUFFER_MAX_BYTES — setting only CODEMAN_MAX_TERMINAL_BUFFER=2097152 left the 24MB trim default in force, making BufferAccumulator.trim() (slice(-trimSize)) a no-op: unbounded growth past the cap plus a full string re-join on every append (O(n²)). Trim default is now clamped to 75% of the resolved max (the 24MB/32MB default ratio, preserved as hysteresis); regression test re-evaluates the module under the env via vi.resetModules. - OVERCLAIM: reverted DEFAULT_TERMINAL_SCROLLBACK_LINES 100k -> 50k — it has zero consumers; browser xterm scrollback is the separate hardcoded DEFAULT_SCROLLBACK (50k) in constants.js and deliberately stays 50k (mobile-memory hazard). The tmux history-limit raise (50k -> 100k) and PTY 32MB/24MB raise remain (those are wired). Module docstring now claims only what is wired; fixed the stale tmux-manager.ts comment saying the tmux limit "matches the xterm-side default in constants.js". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -1,19 +1,31 @@
|
||||
/**
|
||||
* Defaults, bounds, and resolution for terminal history retention.
|
||||
*
|
||||
* Defaults are sized to retain a full default scrollback for replay:
|
||||
* - browser/tmux scrollback: 50,000 -> 100,000 lines
|
||||
* - server PTY buffer cap: 2MB -> 32MB (room for 100k normal-width lines + ANSI)
|
||||
* Raised defaults (the ones actually wired):
|
||||
* - tmux history-limit: 50,000 -> 100,000 lines (applied at session spawn)
|
||||
* - server PTY buffer cap: 2MB max / 1.5MB trim -> 32MB / 24MB (via buffer-limits.ts)
|
||||
* Browser xterm scrollback is a separate hardcoded DEFAULT_SCROLLBACK (50,000) in
|
||||
* src/web/public/constants.js and deliberately stays at 50k — 100k xterm lines per tab
|
||||
* is a mobile-memory hazard — so DEFAULT_TERMINAL_SCROLLBACK_LINES stays 50,000 to match.
|
||||
* The terminalScrollbackLines/terminalBufferMaxBytes/terminalBufferTrimBytes settings keys
|
||||
* remain schema-validated but inert (a follow-up wires them); only tmuxHistoryLimit is live.
|
||||
* All values remain env- and settings-overridable and bounds-clamped via
|
||||
* resolveTerminalHistoryConfig().
|
||||
*/
|
||||
|
||||
export const DEFAULT_TERMINAL_SCROLLBACK_LINES = 100_000;
|
||||
export const DEFAULT_TERMINAL_SCROLLBACK_LINES = 50_000;
|
||||
export const DEFAULT_TMUX_HISTORY_LIMIT = 100_000;
|
||||
export const DEFAULT_TERMINAL_BUFFER_MAX_BYTES =
|
||||
parseInt(process.env.CODEMAN_MAX_TERMINAL_BUFFER || '', 10) || 32 * 1024 * 1024;
|
||||
export const DEFAULT_TERMINAL_BUFFER_TRIM_BYTES =
|
||||
parseInt(process.env.CODEMAN_TRIM_TERMINAL_TO || '', 10) || 24 * 1024 * 1024;
|
||||
// Trim must stay below the max: BufferAccumulator.trim() keeps the last trimSize chars, so a
|
||||
// trim >= max never shrinks the buffer — every append then re-joins the whole string (O(n²))
|
||||
// and memory overshoots the operator's cap (e.g. CODEMAN_MAX_TERMINAL_BUFFER=2097152 with no
|
||||
// trim env would leave the 24MB trim default in force). Clamp to 75% of the resolved max,
|
||||
// preserving the 24MB/32MB default ratio as trim hysteresis.
|
||||
export const DEFAULT_TERMINAL_BUFFER_TRIM_BYTES = Math.min(
|
||||
parseInt(process.env.CODEMAN_TRIM_TERMINAL_TO || '', 10) || 24 * 1024 * 1024,
|
||||
Math.floor(DEFAULT_TERMINAL_BUFFER_MAX_BYTES * 0.75)
|
||||
);
|
||||
|
||||
export const MIN_TERMINAL_SCROLLBACK_LINES = 1_000;
|
||||
export const MAX_TERMINAL_SCROLLBACK_LINES = 1_000_000;
|
||||
|
||||
+2
-1
@@ -1202,7 +1202,8 @@ export class TmuxManager extends EventEmitter implements TerminalMultiplexer {
|
||||
/* Already set globally as fallback */
|
||||
}),
|
||||
// Raise tmux scrollback from its 2000-line default so re-attach preserves
|
||||
// more context. Matches the xterm-side default in constants.js.
|
||||
// more context. Intentionally exceeds the xterm-side DEFAULT_SCROLLBACK (50k
|
||||
// in constants.js), which stays lower to protect browser/mobile memory.
|
||||
execAsync(`${this.tmux()} set-option -t "${muxName}" history-limit ${historyLimit}`, {
|
||||
timeout: EXEC_TIMEOUT_MS,
|
||||
})
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import { describe, it, expect, vi } from 'vitest';
|
||||
import {
|
||||
resolveTerminalHistoryConfig,
|
||||
DEFAULT_TERMINAL_SCROLLBACK_LINES,
|
||||
@@ -22,13 +22,36 @@ describe('resolveTerminalHistoryConfig', () => {
|
||||
});
|
||||
});
|
||||
|
||||
it('defaults raise tmux scrollback to 100k and terminal buffer cap to 32MB', () => {
|
||||
it('defaults raise tmux history to 100k and buffer caps to 32MB/24MB; browser scrollback stays 50k', () => {
|
||||
expect(DEFAULT_TMUX_HISTORY_LIMIT).toBe(100_000);
|
||||
expect(DEFAULT_TERMINAL_SCROLLBACK_LINES).toBe(100_000);
|
||||
// Matches the hardcoded browser-side DEFAULT_SCROLLBACK in src/web/public/constants.js —
|
||||
// deliberately NOT raised (100k xterm lines per tab is a mobile-memory hazard).
|
||||
expect(DEFAULT_TERMINAL_SCROLLBACK_LINES).toBe(50_000);
|
||||
expect(DEFAULT_TERMINAL_BUFFER_MAX_BYTES).toBe(32 * 1024 * 1024);
|
||||
expect(DEFAULT_TERMINAL_BUFFER_TRIM_BYTES).toBe(24 * 1024 * 1024);
|
||||
});
|
||||
|
||||
it('clamps the env-derived trim default below an env-lowered max (CODEMAN_MAX_TERMINAL_BUFFER only)', async () => {
|
||||
const originalMax = process.env.CODEMAN_MAX_TERMINAL_BUFFER;
|
||||
const originalTrim = process.env.CODEMAN_TRIM_TERMINAL_TO;
|
||||
vi.resetModules();
|
||||
process.env.CODEMAN_MAX_TERMINAL_BUFFER = String(2 * 1024 * 1024);
|
||||
delete process.env.CODEMAN_TRIM_TERMINAL_TO;
|
||||
try {
|
||||
const mod = await import('../src/config/terminal-history.js');
|
||||
expect(mod.DEFAULT_TERMINAL_BUFFER_MAX_BYTES).toBe(2 * 1024 * 1024);
|
||||
// trim >= max would make BufferAccumulator.trim() a no-op (unbounded growth + O(n²) appends).
|
||||
expect(mod.DEFAULT_TERMINAL_BUFFER_TRIM_BYTES).toBeLessThan(mod.DEFAULT_TERMINAL_BUFFER_MAX_BYTES);
|
||||
expect(mod.DEFAULT_TERMINAL_BUFFER_TRIM_BYTES).toBe(Math.floor(2 * 1024 * 1024 * 0.75));
|
||||
} finally {
|
||||
if (originalMax === undefined) delete process.env.CODEMAN_MAX_TERMINAL_BUFFER;
|
||||
else process.env.CODEMAN_MAX_TERMINAL_BUFFER = originalMax;
|
||||
if (originalTrim === undefined) delete process.env.CODEMAN_TRIM_TERMINAL_TO;
|
||||
else process.env.CODEMAN_TRIM_TERMINAL_TO = originalTrim;
|
||||
vi.resetModules();
|
||||
}
|
||||
});
|
||||
|
||||
it('passes valid in-range values through unchanged', () => {
|
||||
const cfg = resolveTerminalHistoryConfig({
|
||||
terminalScrollbackLines: 50_000,
|
||||
|
||||
Reference in New Issue
Block a user