mirror of
https://github.com/stablyai/orca.git
synced 2026-10-22 00:03:47 +00:00
fix(terminal): make the output queue own the strings it holds
Round 2 review found a third producer feeding the queue parent-pinning strings: agent-status-osc.ts builds its cleaned output by concatenating slices of the combined buffer, and pty-transport hands that straight to the scheduler for local, SSH and remote-runtime output. That retained 17,283,160 bytes for 524,288 queued chars. Patching producers one at a time was the wrong shape - the contract was enforced only by a comment, and a fourth producer would reintroduce it. Copy once at the enqueue boundary instead, which makes retainedChars true by construction, and revert the two producer-side flattens as redundant. The copy runs at 7.6-38 GB/s against a ~30 MB/s sustained drain. Regression test feeds the scheduler a raw slice of a 2 MB parent: 16,800,424 bytes retained without the boundary, under 4 MB with it.
This commit is contained in:
@@ -334,7 +334,6 @@ import {
|
||||
} from './renderer-owned-agent-status-registry'
|
||||
import type { DirectSshPaneRetryAttempt } from '@/store/slices/direct-ssh-terminal-recovery'
|
||||
import { directSshAuthoritiesEqual } from '@/store/slices/direct-ssh-terminal-authority-ledger'
|
||||
import { flattenRetainedSlice } from '@/lib/flatten-retained-slice'
|
||||
|
||||
const pendingSpawnByPaneKey = new Map<string, Promise<string | null>>()
|
||||
const SSH_SESSION_EXPIRED_ERROR = 'SSH_SESSION_EXPIRED'
|
||||
@@ -6777,9 +6776,7 @@ export function connectPanePty(
|
||||
if (rawLength !== chunk.data.length) {
|
||||
return null
|
||||
}
|
||||
// Why flatten: this tail outlives the chunk it was cut from once queued, and a raw slice
|
||||
// would pin the whole original chunk in the output queue (STA-3567).
|
||||
return flattenRetainedSlice(chunk.data.slice(offset))
|
||||
return chunk.data.slice(offset)
|
||||
}
|
||||
|
||||
type RestoredSnapshotReconciliation =
|
||||
@@ -6829,9 +6826,7 @@ export function connectPanePty(
|
||||
// Why: renderer-only OSC stripping makes raw seq offsets unmappable onto cleaned text; refetch instead of risking duplicate output.
|
||||
return { action: 'force-fresh-restore' }
|
||||
}
|
||||
// Why flatten: same as above - the trimmed tail is handed to the output queue, and a raw
|
||||
// slice would keep the entire pre-trim payload alive behind it (STA-3567).
|
||||
const sliced = flattenRetainedSlice(data.slice(restoredSnapshotBaselineSeq - startSeq))
|
||||
const sliced = data.slice(restoredSnapshotBaselineSeq - startSeq)
|
||||
return {
|
||||
action: 'write',
|
||||
data: sliced,
|
||||
|
||||
@@ -1693,6 +1693,34 @@ describe('pane terminal output scheduler', () => {
|
||||
return process.memoryUsage().heapUsed
|
||||
}
|
||||
|
||||
it('does not pin the parent when a producer enqueues a slice', async () => {
|
||||
vi.useFakeTimers()
|
||||
const { writeTerminalOutput } = await loadScheduler()
|
||||
|
||||
// Why a slice: agent-status-osc.ts and the restore-overlap trims hand the scheduler
|
||||
// strings cut from a much larger buffer. The queue must own its copy rather than
|
||||
// trusting every producer to flatten first (STA-3567 review round 2).
|
||||
const KEEP_CHARS = 64 * 1024
|
||||
const sinks = Array.from({ length: TERMINALS }, () => ({
|
||||
write: (_data: string, callback?: () => void) => callback?.()
|
||||
}))
|
||||
|
||||
const baseline = collect()
|
||||
|
||||
for (const [index, sink] of sinks.entries()) {
|
||||
const parent = String.fromCharCode(65 + index) + 'q'.repeat(CHUNK_CHARS - 1)
|
||||
writeTerminalOutput(sink, parent.slice(parent.length - KEEP_CHARS), {
|
||||
foreground: false,
|
||||
latencySensitive: false
|
||||
})
|
||||
}
|
||||
|
||||
const retainedBytes = collect() - baseline
|
||||
|
||||
// 8 x 64 KB of real payload must not keep 8 x 2 MB of parents alive.
|
||||
expect(retainedBytes).toBeLessThan(4 * 1024 * 1024)
|
||||
})
|
||||
|
||||
it('drops the parent chunk once only a small tail is still queued', async () => {
|
||||
vi.useFakeTimers()
|
||||
const { writeTerminalOutput } = await loadScheduler()
|
||||
|
||||
@@ -698,12 +698,14 @@ function enqueueChunk(
|
||||
ackCredit?: () => void
|
||||
}
|
||||
): void {
|
||||
// Why copy at the boundary: producers hand us slices and concatenations that pin far more than
|
||||
// they measure - restore-overlap trims and OSC status stripping both do - and the queue holds
|
||||
// this string long after the parent is otherwise dead. Owning our copy is what makes
|
||||
// retainedChars true, and it costs ~0.1ms per 512KB against a ~30MB/s sustained drain.
|
||||
const owned = flattenRetainedSlice(data)
|
||||
entry.chunks.push({
|
||||
data,
|
||||
// Why data.length is the truth here: callers must hand the queue a string that owns its own
|
||||
// storage. A caller passing a slice of a much larger buffer would understate what the queue
|
||||
// pins, so producers flatten overlap-trimmed payloads before they reach the scheduler.
|
||||
retainedChars: data.length,
|
||||
data: owned,
|
||||
retainedChars: owned.length,
|
||||
foreground: options?.foreground === true,
|
||||
forceForegroundRefresh: options?.forceForegroundRefresh === true,
|
||||
followupForegroundRefresh: options?.followupForegroundRefresh === true,
|
||||
|
||||
Reference in New Issue
Block a user