From 97715565e4319bc3b4c1de9cdc91db5b544b3d28 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 9 Aug 2026 19:34:17 +0800 Subject: [PATCH 1/7] fix(test): stop background-shell tests sharing a fixed /tmp sidecar path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `makeEntry` defaulted to `outputPath: '/tmp/s1.output'`, so every entry in this file — across tests, across workers, across CI jobs on the same host — mirrored its status sidecar to the single path `/tmp/s1.status`. `/tmp` carries the sticky bit. Once that file belongs to another uid, the atomic rename in `atomicWriteFileSync` fails EPERM, and `renameWithRetrySync` burns its full 50+100+200ms backoff before the registry swallows the error. Every register/complete then costs ~350ms and the sidecar never lands. That is what the loop tests were paying: the retention-cap cases do 68 register/complete calls, and CI measured 23.8s each. The durations across the whole file were exact multiples of 351ms — 352 / 703 / 1405 / 2113 / 3520 — with no variance, which is the backoff sum, not disk latency. Give each entry its own temp directory instead. The shared path is gone, the rename succeeds, and the file drops from 128.9s to 3.2s locally with `/tmp/s1.status` made immutable to reproduce the CI condition. #8797 raised these four cases to a 120s timeout to survive the cost. With the cost removed the band-aid goes too, so a future regression fails loudly instead of silently taking two minutes. --- .../services/backgroundShellRegistry.test.ts | 161 ++++++++---------- 1 file changed, 75 insertions(+), 86 deletions(-) diff --git a/packages/core/src/services/backgroundShellRegistry.test.ts b/packages/core/src/services/backgroundShellRegistry.test.ts index a462c0fa918..1c0fa45052d 100644 --- a/packages/core/src/services/backgroundShellRegistry.test.ts +++ b/packages/core/src/services/backgroundShellRegistry.test.ts @@ -54,15 +54,26 @@ function makeTempDir(): string { function makeEntry( overrides: Partial = {}, ): ShellTaskRegistration { + const shellId = overrides.shellId ?? 's1'; return { - shellId: 's1', + shellId, command: 'sleep 60', cwd: '/tmp', status: 'running', startTime: 1000, - outputPath: '/tmp/s1.output', abortController: new AbortController(), ...overrides, + // Every register/complete/fail/cancel mirrors the entry into a + // `.status` sidecar, so the default outputPath decides where + // that write lands. A fixed `/tmp/s1.output` pointed every entry in this + // file — across tests, across workers, across CI jobs — at the single + // path `/tmp/s1.status`. `/tmp` is sticky, so once that file belongs to + // another uid the atomic rename fails EPERM, and `renameWithRetrySync` + // burns its full 50+100+200ms backoff before the registry swallows the + // error. Give each entry its own directory instead: no shared state, and + // the sidecar write actually succeeds. + outputPath: + overrides.outputPath ?? join(makeTempDir(), `shell-${shellId}.output`), }; } @@ -669,95 +680,73 @@ describe('BackgroundShellRegistry', () => { }); }); - // Every register/complete in these loop tests also writes the status - // sidecar through atomicWriteFileSync, and a loaded CI runner has been - // measured spending ~700ms per sidecar write — ~50s for the ~70 writes - // of the longest loop, past the 15s default. The explicit timeout buys - // the I/O the time it costs; the assertions are unchanged. - const SIDECAR_IO_TIMEOUT = 120_000; describe('terminal-entry retention cap', () => { - it( - 'retains only a bounded number of terminal entries (oldest by endTime evicted)', - () => { - const reg = new BackgroundShellRegistry(); - // Register and complete one more entry than the cap allows. Use - // strictly increasing endTimes so eviction order is deterministic. - for (let i = 0; i < MAX_RETAINED_TERMINAL_SHELLS + 2; i++) { - reg.register(makeEntry({ shellId: `s-${i}`, startTime: i * 10 })); - reg.complete(`s-${i}`, 0, i * 10 + 5); - } - expect(reg.getAll()).toHaveLength(MAX_RETAINED_TERMINAL_SHELLS); - // The two oldest (`s-0`, `s-1`) get pruned; the newest survives. - expect(reg.get('s-0')).toBeUndefined(); - expect(reg.get('s-1')).toBeUndefined(); - expect(reg.get(`s-${MAX_RETAINED_TERMINAL_SHELLS + 1}`)).toBeDefined(); - }, - SIDECAR_IO_TIMEOUT, - ); - - it( - 'never evicts running entries even when the cap is exceeded', - () => { - const reg = new BackgroundShellRegistry(); - // Register one extra terminal entry beyond the cap, then a single - // running entry. The running entry must be retained regardless of - // its launch order — pruning a still-running shell would lose the - // user's only handle on a live process. - reg.register(makeEntry({ shellId: 'live', startTime: 1 })); - for (let i = 0; i < MAX_RETAINED_TERMINAL_SHELLS + 1; i++) { - reg.register( - makeEntry({ shellId: `done-${i}`, startTime: 100 + i * 10 }), - ); - reg.complete(`done-${i}`, 0, 100 + i * 10 + 5); - } - // Cap-of-32 terminals + 1 running survivor = 33 entries kept. - expect(reg.getAll()).toHaveLength(MAX_RETAINED_TERMINAL_SHELLS + 1); - expect(reg.get('live')?.status).toBe('running'); - // The oldest terminal entry (lowest endTime) is the one evicted. - expect(reg.get('done-0')).toBeUndefined(); - }, - SIDECAR_IO_TIMEOUT, - ); + it('retains only a bounded number of terminal entries (oldest by endTime evicted)', () => { + const reg = new BackgroundShellRegistry(); + // Register and complete one more entry than the cap allows. Use + // strictly increasing endTimes so eviction order is deterministic. + for (let i = 0; i < MAX_RETAINED_TERMINAL_SHELLS + 2; i++) { + reg.register(makeEntry({ shellId: `s-${i}`, startTime: i * 10 })); + reg.complete(`s-${i}`, 0, i * 10 + 5); + } + expect(reg.getAll()).toHaveLength(MAX_RETAINED_TERMINAL_SHELLS); + // The two oldest (`s-0`, `s-1`) get pruned; the newest survives. + expect(reg.get('s-0')).toBeUndefined(); + expect(reg.get('s-1')).toBeUndefined(); + expect(reg.get(`s-${MAX_RETAINED_TERMINAL_SHELLS + 1}`)).toBeDefined(); + }); - it( - 'prunes after fail() too, not just complete()', - () => { - const reg = new BackgroundShellRegistry(); - for (let i = 0; i < MAX_RETAINED_TERMINAL_SHELLS; i++) { - reg.register(makeEntry({ shellId: `done-${i}`, startTime: i * 10 })); - reg.complete(`done-${i}`, 0, i * 10 + 5); - } - const overflowStart = MAX_RETAINED_TERMINAL_SHELLS * 10 + 100; + it('never evicts running entries even when the cap is exceeded', () => { + const reg = new BackgroundShellRegistry(); + // Register one extra terminal entry beyond the cap, then a single + // running entry. The running entry must be retained regardless of + // its launch order — pruning a still-running shell would lose the + // user's only handle on a live process. + reg.register(makeEntry({ shellId: 'live', startTime: 1 })); + for (let i = 0; i < MAX_RETAINED_TERMINAL_SHELLS + 1; i++) { reg.register( - makeEntry({ shellId: 'overflow', startTime: overflowStart }), + makeEntry({ shellId: `done-${i}`, startTime: 100 + i * 10 }), ); - reg.fail('overflow', 'boom', overflowStart + 5); - expect(reg.getAll()).toHaveLength(MAX_RETAINED_TERMINAL_SHELLS); - expect(reg.get('done-0')).toBeUndefined(); - expect(reg.get('overflow')?.status).toBe('failed'); - }, - SIDECAR_IO_TIMEOUT, - ); + reg.complete(`done-${i}`, 0, 100 + i * 10 + 5); + } + // Cap-of-32 terminals + 1 running survivor = 33 entries kept. + expect(reg.getAll()).toHaveLength(MAX_RETAINED_TERMINAL_SHELLS + 1); + expect(reg.get('live')?.status).toBe('running'); + // The oldest terminal entry (lowest endTime) is the one evicted. + expect(reg.get('done-0')).toBeUndefined(); + }); - it( - 'prunes after cancel() too, not just complete()', - () => { - const reg = new BackgroundShellRegistry(); - for (let i = 0; i < MAX_RETAINED_TERMINAL_SHELLS; i++) { - reg.register(makeEntry({ shellId: `done-${i}`, startTime: i * 10 })); - reg.complete(`done-${i}`, 0, i * 10 + 5); - } - const overflowStart = MAX_RETAINED_TERMINAL_SHELLS * 10 + 100; - reg.register( - makeEntry({ shellId: 'overflow', startTime: overflowStart }), - ); - reg.cancel('overflow', overflowStart + 5); - expect(reg.getAll()).toHaveLength(MAX_RETAINED_TERMINAL_SHELLS); - expect(reg.get('done-0')).toBeUndefined(); - expect(reg.get('overflow')?.status).toBe('cancelled'); - }, - SIDECAR_IO_TIMEOUT, - ); + it('prunes after fail() too, not just complete()', () => { + const reg = new BackgroundShellRegistry(); + for (let i = 0; i < MAX_RETAINED_TERMINAL_SHELLS; i++) { + reg.register(makeEntry({ shellId: `done-${i}`, startTime: i * 10 })); + reg.complete(`done-${i}`, 0, i * 10 + 5); + } + const overflowStart = MAX_RETAINED_TERMINAL_SHELLS * 10 + 100; + reg.register( + makeEntry({ shellId: 'overflow', startTime: overflowStart }), + ); + reg.fail('overflow', 'boom', overflowStart + 5); + expect(reg.getAll()).toHaveLength(MAX_RETAINED_TERMINAL_SHELLS); + expect(reg.get('done-0')).toBeUndefined(); + expect(reg.get('overflow')?.status).toBe('failed'); + }); + + it('prunes after cancel() too, not just complete()', () => { + const reg = new BackgroundShellRegistry(); + for (let i = 0; i < MAX_RETAINED_TERMINAL_SHELLS; i++) { + reg.register(makeEntry({ shellId: `done-${i}`, startTime: i * 10 })); + reg.complete(`done-${i}`, 0, i * 10 + 5); + } + const overflowStart = MAX_RETAINED_TERMINAL_SHELLS * 10 + 100; + reg.register( + makeEntry({ shellId: 'overflow', startTime: overflowStart }), + ); + reg.cancel('overflow', overflowStart + 5); + expect(reg.getAll()).toHaveLength(MAX_RETAINED_TERMINAL_SHELLS); + expect(reg.get('done-0')).toBeUndefined(); + expect(reg.get('overflow')?.status).toBe('cancelled'); + }); }); describe('cancel', () => { From cc588b2f6f0f596dd7008a8ed398cb86e6634262 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 9 Aug 2026 14:13:47 +0000 Subject: [PATCH 2/7] fix(test): unify sidecar test helpers and pin per-entry outputPath uniqueness Co-authored-by: Qwen-Coder --- .../services/backgroundShellRegistry.test.ts | 26 +++++++------------ 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/packages/core/src/services/backgroundShellRegistry.test.ts b/packages/core/src/services/backgroundShellRegistry.test.ts index 1c0fa45052d..03f0e5bffff 100644 --- a/packages/core/src/services/backgroundShellRegistry.test.ts +++ b/packages/core/src/services/backgroundShellRegistry.test.ts @@ -78,6 +78,10 @@ function makeEntry( } describe('BackgroundShellRegistry', () => { + it('gives each entry a unique default outputPath', () => { + expect(makeEntry().outputPath).not.toBe(makeEntry().outputPath); + }); + describe('register / get / getAll', () => { it('captures the Todo work-chain owner at registration', () => { const reg = new BackgroundShellRegistry(); @@ -334,7 +338,7 @@ describe('BackgroundShellRegistry', () => { shellId: 'a&b', command: 'echo "