From 190a3832eb2150be158b2ee5a9b0e7618f3754fd Mon Sep 17 00:00:00 2001 From: AnzoBenjamin Date: Sun, 6 Sep 2026 19:17:37 +0300 Subject: [PATCH 1/8] fix(check-job, background-jobs): keep tail on log-quota truncation and force-emit carry on wait_for match MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bug fixes for background job log handling: (1) Log-quota truncation now keeps the TAIL (newest output) instead of the head. The newest output is the most diagnostically useful for a job terminated due to log quota — errors, build failures, and recent status messages live there. Adds truncateLogToTail() which opens the file safely (O_NOFOLLOW) and rewrites the tail in place to avoid leaving a sparse/holey file behind (truncating a live append-only fd below its write offset is undefined per POSIX). (2) wait_for match consistency: when the needle is found in the pending partial line (lineCarry) that has not yet been emitted as a registry output event, force-emit the carry via flushJobLineCarry() so the returned events are consistent with matched: true. Without this, a needle in an unterminated partial line could be reported as matched while being absent from the returned events/outputText. --- sdk/src/__tests__/check-job.test.ts | 17 +++++----- sdk/src/tools/background-jobs.ts | 51 ++++++++++++++++++++++++----- sdk/src/tools/check-job.ts | 35 +++++++++++++------- 3 files changed, 75 insertions(+), 28 deletions(-) diff --git a/sdk/src/__tests__/check-job.test.ts b/sdk/src/__tests__/check-job.test.ts index 58e5c2daa8..7d96ccbfd9 100644 --- a/sdk/src/__tests__/check-job.test.ts +++ b/sdk/src/__tests__/check-job.test.ts @@ -249,11 +249,11 @@ describe('checkJob', () => { expect(job.readOffset).toBe(offsetAfterLiveDrain) }) - test('wait_for matches only via peekJobLineCarry for an unterminated partial line', async () => { - // Documented live-drainer match-vs-events lag: a needle drained into - // lineCarry (no trailing newline yet) is matchable via peekJobLineCarry - // even though no complete per-line registry `output` event has been - // emitted for it yet. matched can be true while events/outputText lag. + test('wait_for matches via peekJobLineCarry for an unterminated partial line and force-emits the carry', async () => { + // A needle drained into lineCarry (no trailing newline yet) is matchable + // via peekJobLineCarry. When the match is found in the carry, checkJob + // force-emits the carry as a registry output event so the returned events + // are consistent with matched: true — the needle appears in outputText. const job = makeJob() const partial = 'Ready > Listening on :3000' fs.appendFileSync(job.logFile, partial) @@ -281,9 +281,10 @@ describe('checkJob', () => { expect(result.matched).toBe(true) expect(result.state).toBe('running') expect(result.timedOut).toBeUndefined() - // Carry still holds the unterminated needle; events need not include it. - expect(peekJobLineCarry(job)).toContain('Listening on') - expect(outputText(result)).not.toContain('Listening on') + // Carry was force-emitted as a registry output event, so the needle is + // now present in the returned events/outputText and the carry is cleared. + expect(peekJobLineCarry(job)).toBe('') + expect(outputText(result)).toContain('Listening on') }) test('follow mode returns matched=true once the pattern is present', async () => { diff --git a/sdk/src/tools/background-jobs.ts b/sdk/src/tools/background-jobs.ts index 757026f1ce..13e8536da0 100644 --- a/sdk/src/tools/background-jobs.ts +++ b/sdk/src/tools/background-jobs.ts @@ -288,6 +288,40 @@ export function safeOpenJobLogForRead( } } +/** + * Truncate a background-job log file to at most `maxBytes` by dropping the + * HEAD (oldest bytes) and keeping the TAIL (newest bytes). The newest output + * is the most diagnostically useful for a job terminated due to log quota — + * errors, build failures, and recent status messages live there. No-op when + * the file is already within quota. Opens the file safely (O_NOFOLLOW) and + * rewrites the tail in place to avoid leaving a sparse/holey file behind + * (truncating a live append-only fd below its write offset is undefined per + * POSIX). + */ +function truncateLogToTail(logFile: string, maxBytes: number): void { + let fd: number | undefined + try { + fd = fs.openSync(logFile, fs.constants.O_RDWR | O_NOFOLLOW_FLAG) + const size = fs.fstatSync(fd).size + if (size <= maxBytes) return + const keepStart = size - maxBytes + const buf = Buffer.alloc(maxBytes) + const bytesRead = fs.readSync(fd, buf, 0, maxBytes, keepStart) + fs.ftruncateSync(fd, 0) + fs.writeSync(fd, buf.subarray(0, bytesRead), 0, bytesRead, 0) + } catch { + // best-effort truncation; the exit path owns final cleanup + } finally { + if (fd !== undefined) { + try { + fs.closeSync(fd) + } catch { + // already closed + } + } + } +} + function safeReadJobMetadataFile(metadataFile: string): string | undefined { let fd: number | undefined try { @@ -697,11 +731,12 @@ export function startBackgroundJob(params: { // Keep trimming while the process is unwinding so a chatty child cannot // regrow a sparse/oversized log between SIGTERM and exit. // - // Deliberate tradeoff: truncation keeps the HEAD (oldest bytes) and drops - // the newest output. The job is being terminated for exceeding its log - // quota, and the startup/context head is the most diagnostically useful - // part; losing the tail is a known, accepted cost. - fs.truncateSync(logFile, MAX_BACKGROUND_LOG_BYTES) + // Deliberate tradeoff: truncation keeps the TAIL (newest bytes) and drops + // the oldest output. The job is being terminated for exceeding its log + // quota, and the newest output is the most diagnostically useful — errors, + // build failures, and recent status messages live in the tail; losing the + // head is a known, accepted cost. + truncateLogToTail(logFile, MAX_BACKGROUND_LOG_BYTES) } catch { // The process exit/error handlers own final cleanup. } @@ -732,11 +767,11 @@ export function startBackgroundJob(params: { ? 'completed' : 'error' if (quotaExceeded) { - // See the log-quota monitor above: truncation keeps the HEAD (oldest - // bytes) and drops the newest output. Known, accepted tradeoff for a + // See the log-quota monitor above: truncation keeps the TAIL (newest + // bytes) and drops the oldest output. Known, accepted tradeoff for a // job terminated for exceeding its log quota. try { - fs.truncateSync(logFile, MAX_BACKGROUND_LOG_BYTES) + truncateLogToTail(logFile, MAX_BACKGROUND_LOG_BYTES) } catch { // best-effort truncation; the exit path owns final cleanup } diff --git a/sdk/src/tools/check-job.ts b/sdk/src/tools/check-job.ts index 1ae8e55767..afabe4f705 100644 --- a/sdk/src/tools/check-job.ts +++ b/sdk/src/tools/check-job.ts @@ -1,4 +1,5 @@ import { + flushJobLineCarry, getBackgroundJob, killBackgroundJob, peekJobLineCarry, @@ -244,18 +245,28 @@ export async function checkJob(params: { // yet emitted as a per-line registry `output` event — is still matchable; // the carry is NOT folded into `collected` (that must stay bounded and // event-derived, or it would double-count once the line is later emitted). - // For a live job (hasLiveDrainer) this `wait_for` window includes the - // pending lineCarry, so checkJob can return matched:true while the - // corresponding needle has NOT yet appeared in the returned events/ - // outputText for that same response — the event is emitted on the next - // drain (≤250ms). This transient inconsistency is intentional and bounded - // by MAX_LINE_BYTES, not a bug. - if ( - waitFor && - !matched && - (collected + chunk + peekJobLineCarry(job)).includes(waitFor) - ) { - matched = true + // When the needle is found in the pending partial line (carry), force-emit + // the carry as a registry output event so the returned events are consistent + // with matched: true. Without this, a needle in an unterminated partial line + // could be reported as matched while being absent from the returned events/ + // outputText. The re-snapshot from entryCursor picks up the flushed event, + // so it appears in the returned events. The cursor variable in the follow + // loop is not updated by the flush, but this is fine because (a) the + // re-snapshot uses entryCursor not cursor, and (b) matched is now true so + // the loop exits. + if (waitFor && !matched) { + const matchWindow = collected + chunk + peekJobLineCarry(job) + if (matchWindow.includes(waitFor)) { + matched = true + // If the needle is in the pending partial line (carry) that has not + // yet been emitted as a registry output event, force-emit it now so + // the returned events are consistent with matched: true. Without this, + // a needle in an unterminated partial line could be reported as matched + // while being absent from the returned events/outputText. + if (peekJobLineCarry(job).includes(waitFor)) { + flushJobLineCarry(job) + } + } } // Bound the match window so a chatty long-running job can't grow // `collected` without limit (OOM) across many poll iterations. From 491a281b473a4aa6b60e49ac28a0c647b1bbff76 Mon Sep 17 00:00:00 2001 From: AnzoBenjamin Date: Sun, 6 Sep 2026 20:00:30 +0300 Subject: [PATCH 2/8] refactor(check-job): extract settlement helper and fix carry force-emit boundary Hoist the resolveSettlementTouchedPaths closure into a module-level helper with a typed job parameter, removing the duplicate definition inside checkJob. Correct the wait_for carry force-emit guard from 'needle fully inside carry' to 'needle depends on carry' so a needle spanning the chunk/carry boundary is still flushed and matched:true stays consistent with the returned events. Add two tests covering the boundary-span flush and the no-flush-when-fully-in-chunk cases. --- sdk/src/__tests__/check-job.test.ts | 52 +++++++++++++++++++ sdk/src/tools/check-job.ts | 79 +++++++++++++++-------------- 2 files changed, 92 insertions(+), 39 deletions(-) diff --git a/sdk/src/__tests__/check-job.test.ts b/sdk/src/__tests__/check-job.test.ts index 7d96ccbfd9..59da28274a 100644 --- a/sdk/src/__tests__/check-job.test.ts +++ b/sdk/src/__tests__/check-job.test.ts @@ -287,6 +287,58 @@ describe('checkJob', () => { expect(outputText(result)).toContain('Listening on') }) + test('wait_for force-emits the carry when the needle spans the chunk/carry boundary', async () => { + // A needle that spans the boundary between chunk (already emitted to the + // registry) and carry (not yet emitted) must still be fully present in the + // returned events. The carry must be force-emitted so the returned events + // are consistent with matched: true. + const job = makeJob() + // Write a complete line followed by a partial line (no newline at end). + // After draining, the complete line is in the registry (chunk) and the + // partial line is in the carry. + fs.appendFileSync(job.logFile, 'foo bar\nbaz') + expect(readNewJobOutput(job)).toBe('foo bar\nbaz') + expect(peekJobLineCarry(job)).toBe('baz') + + const result = value( + await checkJob({ + jobId: job.jobId, + wait_for: 'bar\nbaz', + owner: TRUSTED_OWNER, + }), + ) + expect(result.matched).toBe(true) + expect(result.state).toBe('running') + // The carry was force-emitted, so the needle is fully present in the + // returned events. + expect(peekJobLineCarry(job)).toBe('') + expect(outputText(result)).toContain('bar\nbaz') + }) + + test('wait_for does not force-emit the carry when the needle is fully in the chunk', async () => { + // When the needle is fully present in chunk (already emitted to the + // registry), the carry must NOT be force-emitted. This keeps the original + // path unchanged for the common case and verifies the new boundary check + // only flushes the carry when the needle depends on it. + const job = makeJob() + fs.appendFileSync(job.logFile, 'prefix\nListening on :3000\nsuffix') + expect(readNewJobOutput(job)).toBe('prefix\nListening on :3000\nsuffix') + expect(peekJobLineCarry(job)).toBe('suffix') + + const result = value( + await checkJob({ + jobId: job.jobId, + wait_for: 'Listening on :3000', + owner: TRUSTED_OWNER, + }), + ) + expect(result.matched).toBe(true) + expect(result.state).toBe('running') + // Carry must be untouched because the needle was fully in the chunk. + expect(peekJobLineCarry(job)).toBe('suffix') + expect(outputText(result)).toContain('Listening on :3000') + }) + test('follow mode returns matched=true once the pattern is present', async () => { const job = makeJob() fs.appendFileSync(job.logFile, 'starting...\nListening on :3000\n') diff --git a/sdk/src/tools/check-job.ts b/sdk/src/tools/check-job.ts index afabe4f705..c70dd3b874 100644 --- a/sdk/src/tools/check-job.ts +++ b/sdk/src/tools/check-job.ts @@ -114,6 +114,36 @@ export function boundEventsToOutputTail( return { events: result, truncated } } +/** + * Resolve the one-shot settlement dirty delta the first time a settled + * observation is returned. Stores `[]` when snapshot/git is missing so + * re-polls stay idempotent and never re-attribute post-settle dirt. + * Returns paths to emit only on the resolving observation (omit on + * subsequent polls and while still running). + */ +async function resolveSettlementTouchedPaths(job: { + settlementTouchedPaths?: string[] + dirtyBeforePaths?: string[] + projectRoot?: string +}): Promise { + if (job.settlementTouchedPaths !== undefined) { + // Already resolved on a prior settled check_job — do not re-emit. + return undefined + } + if (job.dirtyBeforePaths !== undefined && job.projectRoot !== undefined) { + const dirtyAfter = await listDirtyPaths(job.projectRoot) + const touched = + dirtyAfter !== null + ? dirtyDelta(new Set(job.dirtyBeforePaths), dirtyAfter) + : [] + job.settlementTouchedPaths = touched + return touched + } + // Soft-fail: recovered jobs / no git snapshot — lock out recompute. + job.settlementTouchedPaths = [] + return undefined +} + /** * Join (poll) or wait (follow) on a background job started by * run_terminal_command. @@ -258,12 +288,14 @@ export async function checkJob(params: { const matchWindow = collected + chunk + peekJobLineCarry(job) if (matchWindow.includes(waitFor)) { matched = true - // If the needle is in the pending partial line (carry) that has not - // yet been emitted as a registry output event, force-emit it now so - // the returned events are consistent with matched: true. Without this, - // a needle in an unterminated partial line could be reported as matched - // while being absent from the returned events/outputText. - if (peekJobLineCarry(job).includes(waitFor)) { + // If the needle depends on the carry (not fully present in the + // already-emitted collected + chunk), force-emit the carry so the + // returned events are consistent with matched: true. This covers both + // the case where the needle is entirely in the carry and where it spans + // the boundary between chunk and carry. Without this, a needle in an + // unterminated partial line could be reported as matched while being + // absent from the returned events/outputText. + if (!(collected + chunk).includes(waitFor)) { flushJobLineCarry(job) } } @@ -329,37 +361,6 @@ export async function checkJob(params: { logFile: job.logFile, } - /** - * Resolve the one-shot settlement dirty delta the first time a settled - * observation is returned. Stores `[]` when snapshot/git is missing so - * re-polls stay idempotent and never re-attribute post-settle dirt. - * Returns paths to emit only on the resolving observation (omit on - * subsequent polls and while still running). - */ - const resolveSettlementTouchedPaths = async (): Promise< - string[] | undefined - > => { - if (job.settlementTouchedPaths !== undefined) { - // Already resolved on a prior settled check_job — do not re-emit. - return undefined - } - if ( - job.dirtyBeforePaths !== undefined && - job.projectRoot !== undefined - ) { - const dirtyAfter = await listDirtyPaths(job.projectRoot) - const touched = - dirtyAfter !== null - ? dirtyDelta(new Set(job.dirtyBeforePaths), dirtyAfter) - : [] - job.settlementTouchedPaths = touched - return touched - } - // Soft-fail: recovered jobs / no git snapshot — lock out recompute. - job.settlementTouchedPaths = [] - return undefined - } - if (timedOut && job.status === 'running' && killOnTimeout) { const killResult = killBackgroundJob(jobId, 'SIGTERM') if ('killed' in killResult) { @@ -371,7 +372,7 @@ export async function checkJob(params: { postKillJob?.exitCode ?? killResult.exitCode ?? undefined // Kill settles the job; credit dirty delta on this first settled // observation (same one-shot path as natural finish). - const killTouched = await resolveSettlementTouchedPaths() + const killTouched = await resolveSettlementTouchedPaths(job) const killValue = { ...baseValue, state: postKillState, @@ -407,7 +408,7 @@ export async function checkJob(params: { } const settlementTouched = finished - ? await resolveSettlementTouchedPaths() + ? await resolveSettlementTouchedPaths(job) : undefined const resultValue = settlementTouched !== undefined From d9a992f29e5e228c27789ee0bfc4b04d3402ebf7 Mon Sep 17 00:00:00 2001 From: AnzoBenjamin Date: Sun, 6 Sep 2026 20:40:36 +0300 Subject: [PATCH 3/8] docs(background-jobs): move registryJobId up and shorten its JSDoc Relocate the registryJobId field to right after jobId at the top of the BackgroundJob interface so the two id fields sit together, and trim the multi-line comment to a single concise line. --- sdk/src/tools/background-jobs.ts | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/sdk/src/tools/background-jobs.ts b/sdk/src/tools/background-jobs.ts index 13e8536da0..ae147923ec 100644 --- a/sdk/src/tools/background-jobs.ts +++ b/sdk/src/tools/background-jobs.ts @@ -71,6 +71,8 @@ export function terminateProcessTree( export interface BackgroundJob { jobId: string + /** Registry-side id backing this job (recovered/test jobs are remapped). */ + registryJobId?: string command: string child: ChildProcess logFile: string @@ -122,14 +124,6 @@ export interface BackgroundJob { * before a group-kill (pid reuse guard); undefined on non-Linux hosts. */ childProcessStartTime?: string - /** - * Registry-side id backing this job when it differs from `jobId`. The - * registry allocates its own ids, so a cross-session-recovered job (whose - * `jobId` comes from the on-disk metadata file name) is re-emitted into - * the registry under a fresh id recorded here. Jobs spawned by this - * process use the registry-issued id directly and leave this undefined. - */ - registryJobId?: string /** * Project root used for the pre-start dirty snapshot (BACKGROUND start). * In-memory only — not written to recovery metadata. Recovered jobs omit it. From 4b475677da83adae140209bbd32cc987b8afd5da Mon Sep 17 00:00:00 2001 From: AnzoBenjamin Date: Sun, 6 Sep 2026 22:25:21 +0300 Subject: [PATCH 4/8] refactor(background-jobs): collapse dual job ids and unify poll-result building --- .../handlers/tool/check-background-agent.ts | 81 ++++++-- sdk/src/__tests__/check-job.test.ts | 15 +- sdk/src/__tests__/list-jobs.test.ts | 40 ++-- sdk/src/tools/background-jobs.ts | 125 +++++++------ sdk/src/tools/check-job.ts | 173 ++++++++++++------ sdk/src/tools/kill-job.ts | 7 +- sdk/src/tools/list-jobs.ts | 13 +- sdk/src/tools/read-logs.ts | 9 +- 8 files changed, 269 insertions(+), 194 deletions(-) diff --git a/packages/agent-runtime/src/tools/handlers/tool/check-background-agent.ts b/packages/agent-runtime/src/tools/handlers/tool/check-background-agent.ts index f181366639..0651b21b6f 100644 --- a/packages/agent-runtime/src/tools/handlers/tool/check-background-agent.ts +++ b/packages/agent-runtime/src/tools/handlers/tool/check-background-agent.ts @@ -17,7 +17,11 @@ import type { CodebuffToolOutput, } from '@codebuff/common/tools/list' import type { AgentState } from '@codebuff/common/types/session-state' -import type { JobEvent } from '@codebuff/common/util/job-registry' +import type { + JobEvent, + JobSnapshot, + WaitJobResult, +} from '@codebuff/common/util/job-registry' type ToolName = 'check_background_agent' @@ -89,6 +93,54 @@ function eventToSearchString(event: JobEvent): string { return chunkType } +/** + * Build the structured poll/follow result from the raw wait result and the + * wait_for predicate. Computes the common output fields (state, events, + * nextCursor, truncated, dropped, matched, timedOut) shared by poll and + * follow modes. Advances the per-consumer cursor for cursorless polls as a + * side effect. Agent-specific fields (result/error/cancelled) are resolved + * separately by the caller from the core/view. + */ +function buildAgentPollResult(params: { + /** Poll mode yields a JobSnapshot; follow mode yields a WaitJobResult. */ + result: WaitJobResult | JobSnapshot | undefined + predicate: ((event: JobEvent) => boolean) | undefined + fallbackState: string + jobId: string + consumerId: string + cursorOmitted: boolean +}): { + state: string + events: JobEvent[] + nextCursor: number + truncated: boolean + dropped: number + matched: boolean | undefined + timedOut: boolean +} { + const { result, predicate, fallbackState, jobId, consumerId, cursorOmitted } = + params + const events = result?.events ?? [] + // Falls back to 0 rather than echoing the caller's cursor: the owned job + // always yields a result here, and reporting a cursor the core did not + // confirm could pin a consumer past every future event. + const nextCursor = result?.nextCursor ?? 0 + // Only a cursorless poll owns the stored position, and it advances only as + // far as the core confirmed: a follow-mode timeout that returned no events + // confirms the cursor it started from, so events that land later are still + // delivered to this consumer. + if (cursorOmitted) { + advanceBackgroundAgentConsumerCursor(jobId, consumerId, nextCursor) + } + const state = result?.state ?? fallbackState + const dropped = result?.dropped ?? 0 + const truncated = + result && 'truncated' in result ? result.truncated : dropped > 0 + const matched = predicate ? events.some(predicate) : undefined + const timedOut = result && 'timedOut' in result ? result.timedOut : false + return { state, events, nextCursor, truncated, dropped, matched, timedOut } +} + /** * The single not_found message shape, used for an id the unified core no * longer knows about (never allocated, or reclaimed by the settled-job TTL @@ -223,24 +275,15 @@ export const handleCheckBackgroundAgent = (async ({ }) : (snapshotBackgroundAgentJob(jobId, effectiveCursor) ?? undefined) - const events = result?.events ?? [] - // Falls back to 0 rather than echoing the caller's cursor: the owned job - // always yields a result here, and reporting a cursor the core did not - // confirm could pin a consumer past every future event. - const nextCursor = result?.nextCursor ?? 0 - // Only a cursorless poll owns the stored position, and it advances only as - // far as the core confirmed: a follow-mode timeout that returned no events - // confirms the cursor it started from, so events that land later are still - // delivered to this consumer. - if (cursorOmitted) { - advanceBackgroundAgentConsumerCursor(jobId, consumerId, nextCursor) - } - const state = result?.state ?? owned.job.state - const dropped = result?.dropped ?? 0 - const truncated = - result && 'truncated' in result ? result.truncated : dropped > 0 - const matched = predicate ? events.some(predicate) : undefined - const timedOut = result && 'timedOut' in result ? result.timedOut : false + const { state, events, nextCursor, truncated, dropped, matched, timedOut } = + buildAgentPollResult({ + result, + predicate, + fallbackState: owned.job.state, + jobId, + consumerId, + cursorOmitted, + }) // Both the settled error and the settled result are folded into the core // lifecycle, so they are resolved from the core first and fall back to the diff --git a/sdk/src/__tests__/check-job.test.ts b/sdk/src/__tests__/check-job.test.ts index 59da28274a..30b8f93dc6 100644 --- a/sdk/src/__tests__/check-job.test.ts +++ b/sdk/src/__tests__/check-job.test.ts @@ -22,6 +22,7 @@ import { appendBoundedCollected, checkJob, } from '../tools/check-job' +import { listJobs } from '../tools/list-jobs' import { SETTLED_JOB_TTL_MS, jobRegistry, @@ -146,8 +147,7 @@ describe('readNewJobOutput', () => { } expect(peekJobLineCarry(job)).toBe('') - const registryId = job.registryJobId ?? job.jobId - const snapshot = jobRegistry.snapshot(registryId, 0) + const snapshot = jobRegistry.snapshot(job.jobId, 0) const outputEvents = (snapshot?.events ?? []).filter( (event) => event.payload.type === 'output', ) @@ -261,8 +261,7 @@ describe('checkJob', () => { expect(readNewJobOutput(job)).toBe(partial) expect(peekJobLineCarry(job)).toContain('Listening on') - const registryId = job.registryJobId ?? job.jobId - const preMatchSnapshot = jobRegistry.snapshot(registryId, 0) + const preMatchSnapshot = jobRegistry.snapshot(job.jobId, 0) const preMatchOutput = (preMatchSnapshot?.events ?? []) .filter((event) => event.payload.type === 'output') .map((event) => @@ -1023,12 +1022,10 @@ describe('checkJob', () => { // The unified jobRegistry is now the source of truth for live // state/ownership (the pending-background-jobs mirror is the legacy // store M4 removes). A recovered job is re-emitted into the registry - // under a fresh registry id stored on the adapter object, carrying the - // preserved owner. + // under the disk-derived jobId (passed as explicit registry id), carrying + // the preserved owner. const recovered = getBackgroundJob(jobId) - const registryJob = recovered?.registryJobId - ? jobRegistry.get(recovered.registryJobId) - : undefined + const registryJob = jobRegistry.get(jobId) expect(registryJob?.owner).toEqual(owner) }) diff --git a/sdk/src/__tests__/list-jobs.test.ts b/sdk/src/__tests__/list-jobs.test.ts index bf2ab310a4..1a2be12c8d 100644 --- a/sdk/src/__tests__/list-jobs.test.ts +++ b/sdk/src/__tests__/list-jobs.test.ts @@ -301,17 +301,17 @@ describe('listJobs', () => { expect(entry!.tail).toEqual(emitted.slice(-10)) }) - test('dual-id remapped process job: list_jobs exposes user jobId; pending uses adapter cursor; listed id works with check_job', async () => { - // Recovered / __registerJobForTest jobs remap: adapter Map key = user/ - // disk jobId, registryJobId = fresh registry id. list_jobs must reverse- - // resolve the adapter, emit the user-facing id, and honor lastCheckCursor - // so rediscovery works with check_job/kill_job. - const userJobId = 'job-dual-id-user' + test('registered process job: list_jobs exposes jobId; pending uses adapter cursor; listed id works with check_job', async () => { + // After the id collapse, __registerJobForTest passes the adapter's jobId + // as the explicit registry id, so the registry record and adapter Map + // share one key. list_jobs must resolve the adapter, emit the id, and + // honor lastCheckCursor so rediscovery works with check_job/kill_job. + const userJobId = 'job-registered-user' const logFile = path.join(os.tmpdir(), `openbuff-${userJobId}.log`) fs.writeFileSync(logFile, '') const adapter: BackgroundJob = { jobId: userJobId, - command: 'dual-id-cmd', + command: 'registered-cmd', child: { pid: 4242 } as BackgroundJob['child'], logFile, metadataFile: path.join(os.tmpdir(), `openbuff-${userJobId}.json`), @@ -322,16 +322,15 @@ describe('listJobs', () => { owner, } __registerJobForTest(adapter) - const registryJobId = adapter.registryJobId - expect(registryJobId).toBeDefined() - expect(registryJobId).not.toBe(userJobId) + // After collapse: registry id === adapter.jobId (no remapping). + expect(adapter.jobId).toBe(userJobId) - // Emit on the *registry* id (where process output is mirrored). - jobRegistry.emit(registryJobId!, { type: 'output', data: 'line-1\n' }) - jobRegistry.emit(registryJobId!, { type: 'output', data: 'line-2\n' }) - jobRegistry.emit(registryJobId!, { type: 'output', data: 'line-3\n' }) + // Emit on the jobId (where process output is mirrored). + jobRegistry.emit(userJobId, { type: 'output', data: 'line-1\n' }) + jobRegistry.emit(userJobId, { type: 'output', data: 'line-2\n' }) + jobRegistry.emit(userJobId, { type: 'output', data: 'line-3\n' }) - // Advance lastCheckCursor via check_job on the user-facing id. + // Advance lastCheckCursor via check_job on the id. const checkResult = (await checkJob({ jobId: userJobId, owner }))[0] .value as { jobId: string @@ -348,19 +347,18 @@ describe('listJobs', () => { gap: boolean command: string }> - // Must expose user-facing id, never the internal remapped registry id. + // Must expose the id. expect(listed.map((j) => j.jobId)).toContain(userJobId) - expect(listed.map((j) => j.jobId)).not.toContain(registryJobId) const entry = listed.find((j) => j.jobId === userJobId) - expect(entry?.command).toBe('dual-id-cmd') + expect(entry?.command).toBe('registered-cmd') // Cursor advanced by check_job → pending none at adapter cursor; no gap. expect(entry?.pending).toBe('none') expect(entry?.gap).toBe(false) - // New output after the listed cursor should re-bucket pending, still under - // the user-facing id, and check_job must still resolve that listed id. - jobRegistry.emit(registryJobId!, { type: 'output', data: 'line-4\n' }) + // New output after the listed cursor should re-bucket pending, and + // check_job must still resolve that listed id. + jobRegistry.emit(userJobId, { type: 'output', data: 'line-4\n' }) const afterMore = ( value(await listJobs({ owner })).jobs as Array<{ jobId: string diff --git a/sdk/src/tools/background-jobs.ts b/sdk/src/tools/background-jobs.ts index ae147923ec..c6a4afb03e 100644 --- a/sdk/src/tools/background-jobs.ts +++ b/sdk/src/tools/background-jobs.ts @@ -71,8 +71,6 @@ export function terminateProcessTree( export interface BackgroundJob { jobId: string - /** Registry-side id backing this job (recovered/test jobs are remapped). */ - registryJobId?: string command: string child: ChildProcess logFile: string @@ -466,10 +464,6 @@ function writeBackgroundJobMetadata(job: BackgroundJob): void { } } -/** Registry-side id backing `job` (recovered jobs are remapped; see above). */ -function registryJobIdFor(job: BackgroundJob): string { - return job.registryJobId ?? job.jobId -} /** * Fold a terminal lifecycle transition into the registry and mirror it onto @@ -494,7 +488,7 @@ function settleBackgroundJob( if (job.settledAt === undefined) { job.settledAt = Date.now() } - jobRegistry.emit(registryJobIdFor(job), { + jobRegistry.emit(job.jobId, { type: 'lifecycle', state: status, exitCode, @@ -525,7 +519,7 @@ export function pruneSettledJobs(now: number = Date.now()): void { /** Mirror a chunk of streamed log bytes into the registry as an output event. */ function emitJobOutput(job: BackgroundJob, data: string): void { if (data.length === 0) return - jobRegistry.emit(registryJobIdFor(job), { type: 'output', data }) + jobRegistry.emit(job.jobId, { type: 'output', data }) } /** @@ -832,31 +826,13 @@ function resolveRestampedOwner( } /** - * Resolve a process adapter for a *registry* job id (list_jobs iteration). - * - * The adapter Map is keyed by the user-facing jobId. Live spawns use the same - * id for both Map key and registry. Recovered / `__registerJobForTest` jobs - * remap: Map key = disk/user jobId, `registryJobId` = fresh registry id. A - * direct `jobs.get(registryId)` therefore misses remapped adapters — reverse- - * scan by `registryJobId ?? jobId` so list_jobs can read `lastCheckCursor` / - * lineCarry and expose the user-facing `adapter.jobId` that check_job/kill_job - * resolve via `getBackgroundJob`. + * Read-only adapter lookup by jobId (no recovery, no ownership changes). + * list_jobs uses this to read `lastCheckCursor` / `lineCarry` from the + * adapter without triggering recovery or ownership restamping. */ -export function getBackgroundJobForRegistryId( - registryJobId: string, -): BackgroundJob | undefined { +export function getBackgroundJobAdapter(jobId: string): BackgroundJob | undefined { pruneSettledJobs() - const direct = jobs.get(registryJobId) - if (direct !== undefined) { - const backing = direct.registryJobId ?? direct.jobId - if (backing === registryJobId) return direct - } - for (const job of jobs.values()) { - if ((job.registryJobId ?? job.jobId) === registryJobId) { - return job - } - } - return undefined + return jobs.get(jobId) } export function getBackgroundJob( @@ -878,13 +854,12 @@ export function getBackgroundJob( // this, the cached-job early return skips the re-stamp and the trusted // caller is locked out of its own job by assertOwned. Gate on the // REGISTRY record's current owner being unknown (SEC-2). - const registryJobId = existing.registryJobId ?? existing.jobId - const registryJob = jobRegistry.get(registryJobId) + const registryJob = jobRegistry.get(jobId) const upgraded = registryJob ? resolveRestampedOwner(registryJob.owner, opts?.restampOwner) : undefined if (upgraded) { - jobRegistry.restampOwner(registryJobId, upgraded) + jobRegistry.restampOwner(jobId, upgraded) existing.owner = upgraded } return existing @@ -903,28 +878,42 @@ export function getBackgroundJob( return undefined } - jobs.set(jobId, recovered) // Re-emit the recovered job into the registry, which is the live source - // of truth for state/ownership. The registry allocates its own ids, so - // the disk-derived jobId is remapped onto a fresh registry id stored on - // the adapter object. Cross-session re-attach: a supplied restampOwner - // UPGRADES ownership only when the disk metadata carries a placeholder / - // missing owner. A job already stamped with a real owner keeps it, so a - // re-attaching run can never launder ownership of another session's job - // (assertOwned then refuses it as foreign) — this mirrors the cached-job - // branch above (SEC-2). Same-process callers pass no opts, so the - // original owner is preserved either way. + // of truth for state/ownership. The disk-derived jobId is passed as the + // explicit registry id so the registry record and adapter Map share one + // key. Cross-session re-attach: a supplied restampOwner UPGRADES ownership + // only when the disk metadata carries a placeholder / missing owner. A job + // already stamped with a real owner keeps it, so a re-attaching run can + // never launder ownership of another session's job (assertOwned then + // refuses it as foreign) — this mirrors the cached-job branch above + // (SEC-2). Same-process callers pass no opts, so the original owner is + // preserved either way. const owner = resolveRestampedOwner(recovered.owner, opts?.restampOwner) ?? recovered.owner ?? UNKNOWN_JOB_OWNER recovered.owner = owner - const registryJobId = jobRegistry.create({ - kind: 'process', - label: recovered.command, - owner, - }).jobId - recovered.registryJobId = registryJobId + // Pass the disk-derived jobId as the explicit registry id so the + // registry record and adapter Map share one key. On collision (another + // job with the same id already registered), fall back to a fresh + // registry-allocated id (preserving old behavior for that rare case). + let registryJobId: string + try { + registryJobId = jobRegistry.create({ + kind: 'process', + label: recovered.command, + owner, + jobId: recovered.jobId, + }).jobId + } catch { + registryJobId = jobRegistry.create({ + kind: 'process', + label: recovered.command, + owner, + }).jobId + } + recovered.jobId = registryJobId + jobs.set(registryJobId, recovered) jobRegistry.start(registryJobId) if (recovered.status !== 'running') { // A settled recovered job is folded straight into its terminal state so @@ -1233,15 +1222,27 @@ export function readNewJobOutput(job: BackgroundJob): string { /** Test-only: register a job backed by an existing log file (no real process). */ export function __registerJobForTest(job: BackgroundJob): void { - jobs.set(job.jobId, job) - // Mirror the job into the registry (under a fresh registry id, like a - // recovered job) so registry-backed lookups see the same state. - const registryJobId = jobRegistry.create({ - kind: 'process', - label: job.command, - owner: job.owner ?? UNKNOWN_JOB_OWNER, - }).jobId - job.registryJobId = registryJobId + // Pass the adapter's jobId as the explicit registry id so the registry + // record and adapter Map share one key. On collision (another job with + // the same id already registered), fall back to a fresh registry-allocated + // id (preserving old behavior for that rare case). + let registryJobId: string + try { + registryJobId = jobRegistry.create({ + kind: 'process', + label: job.command, + owner: job.owner ?? UNKNOWN_JOB_OWNER, + jobId: job.jobId, + }).jobId + } catch { + registryJobId = jobRegistry.create({ + kind: 'process', + label: job.command, + owner: job.owner ?? UNKNOWN_JOB_OWNER, + }).jobId + } + job.jobId = registryJobId + jobs.set(registryJobId, job) jobRegistry.start(registryJobId) if (job.status !== 'running') { jobRegistry.emit(registryJobId, { @@ -1253,9 +1254,8 @@ export function __registerJobForTest(job: BackgroundJob): void { } /** - * Test-only: set/create a Map adapter keyed by registry jobId (production id shape). - * Used only by tests so list_jobs can observe a same-id lastCheckCursor without - * `__registerJobForTest`'s dual-id remapping. + * Test-only: set/create a Map adapter keyed by jobId (production id shape). + * Used only by tests so list_jobs can observe a same-id lastCheckCursor. */ export function __setLastCheckCursorForTest( jobId: string, @@ -1271,7 +1271,6 @@ export function __setLastCheckCursorForTest( // registry row under the same jobId as production live spawns). jobs.set(jobId, { jobId, - registryJobId: jobId, command: 'test', child: {} as ChildProcess, logFile: path.join(os.tmpdir(), `openbuff-${jobId}.log`), diff --git a/sdk/src/tools/check-job.ts b/sdk/src/tools/check-job.ts index c70dd3b874..1d3e5c1d6d 100644 --- a/sdk/src/tools/check-job.ts +++ b/sdk/src/tools/check-job.ts @@ -27,11 +27,6 @@ const POLL_INTERVAL_MS = 200 export const CHECK_JOB_POLL_ACCUMULATION_CAP = CHECK_JOB_OUTPUT_LIMIT * 2 const COLLECTED_TAIL_KEEP = Math.floor(CHECK_JOB_OUTPUT_LIMIT / 4) -/** Registry-side id backing this adapter job (recovered jobs are remapped). */ -function registryIdFor(job: { jobId: string; registryJobId?: string }): string { - return job.registryJobId ?? job.jobId -} - /** True when a job can no longer produce output. */ function jobSettled(job: { status: string }): boolean { return job.status !== 'running' @@ -144,6 +139,104 @@ async function resolveSettlementTouchedPaths(job: { return undefined } +/** + * Resolve the follow deadline for a check_job call. The deadline is computed + * at ENTRY, before any registry calls: both getBackgroundJob (on recovery) + * and assertOwned invoke sweep() → Date.now(), so computing the deadline + * afterward would base it on a later clock read and could push the follow + * window out indefinitely (a mocked or fast-advancing clock would then never + * satisfy `Date.now() >= deadline`). + */ +function resolveCheckJobWaitBounds(params: { + timeoutSeconds?: number +}): { timeoutMs: number; deadline: number } { + const timeoutMs = Math.max(0, (params.timeoutSeconds ?? 0) * 1000) + return { timeoutMs, deadline: Date.now() + timeoutMs } +} + +/** + * Build the structured poll/follow result from the raw follow state. Computes + * the common output fields (state, events, nextCursor, truncated, dropped, + * matched, exitCode) shared by poll and follow modes. Advances the per-adapter + * consumer cursor (lastCheckCursor) as a side effect. The presentation-level + * output cap (boundEventsToOutputTail) is applied here; the caller folds the + * returned `timedOut` into its kill/return logic. + */ +function buildCheckJobPollResult(params: { + jobId: string + job: { + status: JobState + exitCode?: number | null + logFile: string + lastCheckCursor?: number + } + waitFor?: string + matched: boolean + entryCursor: number + newEvents: JobEvent[] + cursor: number + truncated: boolean +}): { + jobId: string + state: JobState + events: JobEvent[] + nextCursor: number + truncated: boolean + dropped: number + exitCode?: number + matched?: boolean + logFile: string +} { + const { + jobId, + job, + waitFor, + matched, + entryCursor, + newEvents, + cursor, + truncated: truncatedSoFar, + } = params + const registryJob = jobRegistry.get(jobId) + const state = registryJob?.state ?? job.status + const exitCode = registryJob?.exitCode ?? job.exitCode ?? undefined + // Re-snapshot from the entry cursor so the returned `events` cover the + // FULL window [entryCursor, nextCursor) — every `output` event drained + // during this follow, not just the final iteration's batch. This keeps + // `events` consistent with `matched` (computed over the accumulated + // `collected` window) and with `nextCursor`, so the caller receives all + // output it would otherwise never be able to refetch. Bounded by the + // registry's per-job event/byte ring buffer. + const finalSnapshot = jobRegistry.snapshot(jobId, entryCursor) + const rawEvents = finalSnapshot?.events ?? newEvents + const boundedResult = boundEventsToOutputTail( + rawEvents, + CHECK_JOB_OUTPUT_LIMIT, + ) + const events = boundedResult.events + const nextCursor = finalSnapshot?.nextCursor ?? cursor + // Advance the per-adapter consumer cursor so the next check_job does not + // re-serve these events. Mirrored-by-live-drainer is not the same as + // consumed-by-check_job; only this advance marks consumption. + job.lastCheckCursor = nextCursor + const finalDropped = finalSnapshot?.dropped ?? 0 + const finalTruncated = + truncatedSoFar || + (finalSnapshot?.truncated ?? false) || + boundedResult.truncated + return { + jobId, + state, + events, + nextCursor, + truncated: finalTruncated, + dropped: finalDropped, + ...(exitCode !== undefined && exitCode !== null ? { exitCode } : {}), + ...(waitFor ? { matched } : {}), + logFile: job.logFile, + } +} + /** * Join (poll) or wait (follow) on a background job started by * run_terminal_command. @@ -181,7 +274,6 @@ export async function checkJob(params: { owner: BackgroundJobOwner }): Promise> { const { jobId, wait_for: waitFor, owner } = params - const timeoutMs = Math.max(0, (params.timeout_seconds ?? 0) * 1000) // Observation must be non-destructive by default. Callers can explicitly // request termination when a follow timeout represents a hard deadline. const killOnTimeout = params.kill_on_timeout ?? false @@ -190,7 +282,9 @@ export async function checkJob(params: { // so computing the deadline afterward would base it on a later clock read and // could push the follow window out indefinitely (a mocked or fast-advancing // clock would then never satisfy `Date.now() >= deadline`). - const deadline = Date.now() + timeoutMs + const { timeoutMs, deadline } = resolveCheckJobWaitBounds({ + timeoutSeconds: params.timeout_seconds, + }) // Cross-session recovery re-stamps the registry record with THIS trusted // owner (never a model-supplied one). @@ -210,8 +304,7 @@ export async function checkJob(params: { // Ownership gate: the registry is the source of truth for who owns this // job. A foreign job is refused with the SAME generic not_found error as // an unknown id so the caller cannot probe for other sessions' jobs. - const registryJobId = registryIdFor(job) - const ownership = jobRegistry.assertOwned(registryJobId, owner) + const ownership = jobRegistry.assertOwned(jobId, owner) if (!ownership.ok) { return [ { @@ -258,7 +351,7 @@ export async function checkJob(params: { // Recovered and test-registered jobs have no interval, so they still drain // here. if (!job.hasLiveDrainer) readNewJobOutput(job) - const snapshot = jobRegistry.snapshot(registryJobId, cursor) + const snapshot = jobRegistry.snapshot(jobId, cursor) const newEvents = snapshot?.events ?? [] if (snapshot) { cursor = snapshot.nextCursor @@ -305,9 +398,6 @@ export async function checkJob(params: { collected = appendBoundedCollected(collected, chunk) const finished = jobSettled(job) if (matched || finished || Date.now() >= deadline) { - const registryJob = jobRegistry.get(registryJobId) - const state = registryJob?.state ?? job.status - const exitCode = registryJob?.exitCode ?? job.exitCode ?? undefined // The follow-timeout fired (deadline reached, pattern NOT matched, job // NOT finished, and still running) and only in follow mode (timeoutMs > 0). // Poll mode (timeoutMs === 0) must never kill even though its deadline @@ -315,58 +405,23 @@ export async function checkJob(params: { // there — but guard with timeoutMs > 0 to be explicit and safe. const timedOut = timeoutMs > 0 && !matched && !finished && Date.now() >= deadline - // Re-snapshot from the entry cursor so the returned `events` cover the - // FULL window [entryCursor, nextCursor) — every `output` event drained - // during this follow, not just the final iteration's batch. This keeps - // `events` consistent with `matched` (computed over the accumulated - // `collected` window) and with `nextCursor`, so the caller receives all - // output it would otherwise never be able to refetch. Bounded by the - // registry's per-job event/byte ring buffer. - const finalSnapshot = jobRegistry.snapshot(registryJobId, entryCursor) - const rawEvents = finalSnapshot?.events ?? newEvents - const boundedResult = boundEventsToOutputTail( - rawEvents, - CHECK_JOB_OUTPUT_LIMIT, - ) - const events = boundedResult.events - const nextCursor = finalSnapshot?.nextCursor ?? cursor - // Advance the per-adapter consumer cursor so the next check_job does not - // re-serve these events. Mirrored-by-live-drainer is not the same as - // consumed-by-check_job; only this advance marks consumption. - job.lastCheckCursor = nextCursor - const finalDropped = finalSnapshot?.dropped ?? 0 - const finalTruncated = - truncated || - (finalSnapshot?.truncated ?? false) || - boundedResult.truncated - const baseValue: { - jobId: string - state: JobState - events: JobEvent[] - nextCursor: number - truncated: boolean - dropped: number - exitCode?: number - matched?: boolean - logFile: string - } = { + const baseValue = buildCheckJobPollResult({ jobId, - state, - events, - nextCursor, - truncated: finalTruncated, - dropped: finalDropped, - ...(exitCode !== undefined && exitCode !== null ? { exitCode } : {}), - ...(waitFor ? { matched } : {}), - logFile: job.logFile, - } + job, + waitFor, + matched, + entryCursor, + newEvents, + cursor, + truncated, + }) if (timedOut && job.status === 'running' && killOnTimeout) { const killResult = killBackgroundJob(jobId, 'SIGTERM') if ('killed' in killResult) { // killBackgroundJob updates the in-memory job status; prefer the // fresh kill-result state/exitCode over the stale local snapshot. - const postKillJob = jobRegistry.get(registryJobId) + const postKillJob = jobRegistry.get(jobId) const postKillState = postKillJob?.state ?? killResult.status const postKillExitCode = postKillJob?.exitCode ?? killResult.exitCode ?? undefined @@ -441,7 +496,7 @@ export async function checkJob(params: { // before ever reaching this line, so wait() never blocks a poll. No // predicate is passed and the return value is ignored. const remaining = deadline - Date.now() - await jobRegistry.wait(registryJobId, { + await jobRegistry.wait(jobId, { timeoutMs: Math.min(POLL_INTERVAL_MS, remaining), cursor, }) diff --git a/sdk/src/tools/kill-job.ts b/sdk/src/tools/kill-job.ts index 36c7f2cae8..5c623a591e 100644 --- a/sdk/src/tools/kill-job.ts +++ b/sdk/src/tools/kill-job.ts @@ -5,11 +5,6 @@ import { getBackgroundJob, killBackgroundJob } from './background-jobs' import type { BackgroundJobOwner } from './background-jobs' import type { CodebuffToolOutput } from '../../../common/src/tools/list' -/** Registry-side id backing this adapter job (recovered jobs are remapped). */ -function registryIdFor(job: { jobId: string; registryJobId?: string }): string { - return job.registryJobId ?? job.jobId -} - export async function killJob(params: { jobId: string signal?: 'SIGTERM' | 'SIGKILL' @@ -38,7 +33,7 @@ export async function killJob(params: { // Ownership gate BEFORE any kill path: a foreign job is refused with the // same generic not_found error as an unknown id (no existence leak), and // terminateProcessTree is never reached for another session's job. - const ownership = jobRegistry.assertOwned(registryIdFor(job), params.owner) + const ownership = jobRegistry.assertOwned(job.jobId, params.owner) if (!ownership.ok) { return [ { diff --git a/sdk/src/tools/list-jobs.ts b/sdk/src/tools/list-jobs.ts index 8017b75e2a..d7ed11cf20 100644 --- a/sdk/src/tools/list-jobs.ts +++ b/sdk/src/tools/list-jobs.ts @@ -13,7 +13,7 @@ import { } from '@codebuff/common/util/job-registry' import { - getBackgroundJobForRegistryId, + getBackgroundJobAdapter, peekJobLineCarry, } from './background-jobs' @@ -61,12 +61,7 @@ export async function listJobs(params: { })) const selected = selectListJobsRows(candidates) const rows: ListJobsViewRow[] = selected.rows.map(({ entry }) => { - // Resolve process adapters by *registry* id (not a direct Map get on the - // registry id alone). Live spawns share one id; recovered / - // `__registerJobForTest` remaps (Map key = user/disk jobId, - // registryJobId = fresh registry id). Reverse-scan so lastCheckCursor and - // lineCarry are visible, and emit the user-facing adapter.jobId so - // rediscovered ids work with check_job/kill_job. + // Resolve process adapters by jobId (Map key = registry id = jobId). // // `pending` line buckets count only registry events with // `payload.type === 'output'` relative to the process adapter's @@ -77,7 +72,7 @@ export async function listJobs(params: { // ring truncation at the snapshot cursor for any kind. // // The lineCarry +1 counting rationale lives on `countPendingOutputLines`. - const adapter = getBackgroundJobForRegistryId(entry.jobId) + const adapter = getBackgroundJobAdapter(entry.jobId) const cursor = adapter?.lastCheckCursor ?? 0 const snap = jobRegistry.snapshot(entry.jobId, cursor) const lineCarry = @@ -89,8 +84,6 @@ export async function listJobs(params: { }), ) const gap = snap?.truncated ?? false - // Prefer user-facing adapter.jobId when remapped; agents/no-adapter keep - // the registry id (the only id they have). const row: ListJobsViewRow = { jobId: adapter?.jobId ?? entry.jobId, kind: entry.kind, diff --git a/sdk/src/tools/read-logs.ts b/sdk/src/tools/read-logs.ts index 1aff08163b..f048d831f4 100644 --- a/sdk/src/tools/read-logs.ts +++ b/sdk/src/tools/read-logs.ts @@ -38,11 +38,6 @@ const MAX_TAIL_SCAN_CHARS_MULTIPLE = 8 */ const BACKGROUND_JOB_FILE_PATTERN = /^openbuff-(.+)\.(log|json)$/ -/** Registry-side id backing this adapter job (recovered jobs are remapped). */ -function registryIdFor(job: { jobId: string; registryJobId?: string }): string { - return job.registryJobId ?? job.jobId -} - type ReadLogsParams = { cwd: string path?: string @@ -90,7 +85,7 @@ export async function readLogs( // Ownership gate before serving the log tail: a foreign job is refused // with the same generic not_found error as an unknown id. - const ownership = jobRegistry.assertOwned(registryIdFor(job), params.owner) + const ownership = jobRegistry.assertOwned(job.jobId, params.owner) if (!ownership.ok) { return [ { @@ -205,7 +200,7 @@ export async function readLogs( if (jobFileMatch) { const jobId = jobFileMatch[1] const job = getBackgroundJob(jobId) - if (job && !jobRegistry.assertOwned(registryIdFor(job), params.owner).ok) { + if (job && !jobRegistry.assertOwned(job.jobId, params.owner).ok) { return [ { type: 'json', From 618d0b87714e1006aa8172b247b455a3f1929b53 Mon Sep 17 00:00:00 2001 From: AnzoBenjamin Date: Sun, 6 Sep 2026 23:27:18 +0300 Subject: [PATCH 5/8] refactor(background-jobs): fail closed on id collisions and drop remap indirection Cross-session recovery and __registerJobForTest previously fell back to a fresh registry-allocated id on a collision, desyncing the adapter from the on-disk log/metadata filenames and making the caller's requested id unresolvable. Recovery now returns undefined and test registration now throws so the registry id, adapter Map key, jobId, and on-disk file names stay one and the same (single-id invariant); list-jobs accordingly emits entry.jobId directly instead of the adapter remap indirection. --- sdk/src/tools/background-jobs.ts | 76 ++++++++++++++------------------ sdk/src/tools/list-jobs.ts | 4 +- 2 files changed, 37 insertions(+), 43 deletions(-) diff --git a/sdk/src/tools/background-jobs.ts b/sdk/src/tools/background-jobs.ts index c6a4afb03e..e3441e987b 100644 --- a/sdk/src/tools/background-jobs.ts +++ b/sdk/src/tools/background-jobs.ts @@ -894,32 +894,28 @@ export function getBackgroundJob( UNKNOWN_JOB_OWNER recovered.owner = owner // Pass the disk-derived jobId as the explicit registry id so the - // registry record and adapter Map share one key. On collision (another - // job with the same id already registered), fall back to a fresh - // registry-allocated id (preserving old behavior for that rare case). - let registryJobId: string - try { - registryJobId = jobRegistry.create({ - kind: 'process', - label: recovered.command, - owner, - jobId: recovered.jobId, - }).jobId - } catch { - registryJobId = jobRegistry.create({ - kind: 'process', - label: recovered.command, - owner, - }).jobId + // registry record and adapter Map share one key (single-id invariant). + // A collision means a live job already owns this id — a cross-session + // recovery must NOT silently remap onto a fresh id (that would desync the + // adapter from the on-disk log/metadata file names and make the caller's + // requested id unresolvable), so refuse the recovery and leave the id + // pointing at the live record. getBackgroundJob reports not-found. + if (jobRegistry.get(recovered.jobId) !== undefined) { + return undefined } - recovered.jobId = registryJobId - jobs.set(registryJobId, recovered) - jobRegistry.start(registryJobId) + jobRegistry.create({ + kind: 'process', + label: recovered.command, + owner, + jobId: recovered.jobId, + }) + jobs.set(recovered.jobId, recovered) + jobRegistry.start(recovered.jobId) if (recovered.status !== 'running') { // A settled recovered job is folded straight into its terminal state so // the re-attaching run can serve its final output/exit code from the // registry. - jobRegistry.emit(registryJobId, { + jobRegistry.emit(recovered.jobId, { type: 'lifecycle', state: recovered.status, exitCode: recovered.exitCode, @@ -1223,29 +1219,25 @@ export function readNewJobOutput(job: BackgroundJob): string { /** Test-only: register a job backed by an existing log file (no real process). */ export function __registerJobForTest(job: BackgroundJob): void { // Pass the adapter's jobId as the explicit registry id so the registry - // record and adapter Map share one key. On collision (another job with - // the same id already registered), fall back to a fresh registry-allocated - // id (preserving old behavior for that rare case). - let registryJobId: string - try { - registryJobId = jobRegistry.create({ - kind: 'process', - label: job.command, - owner: job.owner ?? UNKNOWN_JOB_OWNER, - jobId: job.jobId, - }).jobId - } catch { - registryJobId = jobRegistry.create({ - kind: 'process', - label: job.command, - owner: job.owner ?? UNKNOWN_JOB_OWNER, - }).jobId + // record and adapter Map share one key (single-id invariant). A collision + // means the test registered two adapters under one id — that is a test bug + // and must fail loudly instead of silently remapping onto a fresh id (which + // would break every assertion made on the original jobId). + if (jobRegistry.get(job.jobId) !== undefined) { + throw new Error( + `__registerJobForTest: job id '${job.jobId}' is already registered`, + ) } - job.jobId = registryJobId - jobs.set(registryJobId, job) - jobRegistry.start(registryJobId) + jobRegistry.create({ + kind: 'process', + label: job.command, + owner: job.owner ?? UNKNOWN_JOB_OWNER, + jobId: job.jobId, + }) + jobs.set(job.jobId, job) + jobRegistry.start(job.jobId) if (job.status !== 'running') { - jobRegistry.emit(registryJobId, { + jobRegistry.emit(job.jobId, { type: 'lifecycle', state: job.status, exitCode: job.exitCode, diff --git a/sdk/src/tools/list-jobs.ts b/sdk/src/tools/list-jobs.ts index d7ed11cf20..6cc9ac5adc 100644 --- a/sdk/src/tools/list-jobs.ts +++ b/sdk/src/tools/list-jobs.ts @@ -85,7 +85,9 @@ export async function listJobs(params: { ) const gap = snap?.truncated ?? false const row: ListJobsViewRow = { - jobId: adapter?.jobId ?? entry.jobId, + // Single-id invariant: the adapter Map key, the registry id, and the + // user-facing jobId are the same string, so emit entry.jobId directly. + jobId: entry.jobId, kind: entry.kind, command: entry.label, status: entry.state, From eba44abc08b69e183c16e622f9518a13635ba9fd Mon Sep 17 00:00:00 2001 From: AnzoBenjamin Date: Mon, 7 Sep 2026 01:56:38 +0300 Subject: [PATCH 6/8] fix(background-jobs): preserve foreign log files on failed spawn; polish list-jobs startBackgroundJob's failed-spawn cleanup now unlinks the temp log only when this spawn created it (fail-closed removeFileIfPresent), so O_EXCL EEXIST or symlink-thrown errors leave another owner's openbuff-job-*.log untouched. Also renames listJobs' shadowing owner local to scopeOwner and merges its duplicate background-jobs imports. --- sdk/src/tools/background-jobs.ts | 23 +++++++++++++++-------- sdk/src/tools/list-jobs.ts | 10 ++++++---- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/sdk/src/tools/background-jobs.ts b/sdk/src/tools/background-jobs.ts index e3441e987b..34c448eda6 100644 --- a/sdk/src/tools/background-jobs.ts +++ b/sdk/src/tools/background-jobs.ts @@ -464,7 +464,6 @@ function writeBackgroundJobMetadata(job: BackgroundJob): void { } } - /** * Fold a terminal lifecycle transition into the registry and mirror it onto * the adapter object + the write-only disk projection. All terminal paths @@ -623,11 +622,17 @@ export function startBackgroundJob(params: { // safeCreateJobLogFile/safeWriteJobMetadata also use O_EXCL + O_NOFOLLOW so // pre-created regular files and TOCTOU symlink swaps are rejected at open(). let outFd: number | undefined + // True only once safeCreateJobLogFile has returned, i.e. this spawn + // exclusively created logFile via O_CREAT|O_EXCL. Throws from + // rejectIfSymlink or O_EXCL EEXIST leave it false: the path then holds a + // foreign/pre-existing file this process must never delete. + let logFileCreatedByThisSpawn = false let child: ChildProcess try { rejectIfSymlink(logFile) rejectIfSymlink(metadataFile) outFd = safeCreateJobLogFile(logFile) + logFileCreatedByThisSpawn = true child = spawn(shell, [...shellArgs, command], { cwd, env, @@ -645,13 +650,15 @@ export function startBackgroundJob(params: { // already closed } } - // The log file was created exclusively for this spawn (O_EXCL); on a failed - // spawn nobody else can own it, so remove it best-effort rather than leaving - // an empty temp file behind until the 24h+ orphan sweep. - try { - fs.unlinkSync(logFile) - } catch { - // best-effort; the orphan sweep will clean it up if removal fails + // Remove the log file ONLY when this spawn created it (safeCreateJobLogFile + // succeeded). If the throw came from rejectIfSymlink or from O_EXCL EEXIST, + // the path belongs to a foreign/pre-existing file this process did NOT + // create, and deleting it would clobber another owner's file in the shared + // temp dir. removeFileIfPresent re-lstats immediately before unlinking so + // a TOCTOU symlink swap fails closed; a failed removal still leaves the + // empty file for the 24h+ orphan sweep. + if (logFileCreatedByThisSpawn) { + removeFileIfPresent(logFile) } // Fold the failed spawn into the registry so the freshly-created id does // not linger as a queued job (queued only transitions via running). diff --git a/sdk/src/tools/list-jobs.ts b/sdk/src/tools/list-jobs.ts index 6cc9ac5adc..cfece28db0 100644 --- a/sdk/src/tools/list-jobs.ts +++ b/sdk/src/tools/list-jobs.ts @@ -15,9 +15,8 @@ import { import { getBackgroundJobAdapter, peekJobLineCarry, + type BackgroundJobOwner, } from './background-jobs' - -import type { BackgroundJobOwner } from './background-jobs' import type { CodebuffToolOutput } from '../../../common/src/tools/list' /** Last ≤10 non-empty output lines from buffered events (terminal peek only). */ @@ -40,7 +39,10 @@ export async function listJobs(params: { */ owner: BackgroundJobOwner }): Promise> { - const owner = { + // Narrow the trusted owner to the registry's ownership key: list_jobs is + // scoped by (clientSessionId, rootRunId) only; parentRunId/parentAgentId are + // diagnostic and deliberately excluded from the ownership filter. + const scopeOwner = { clientSessionId: params.owner.clientSessionId, rootRunId: params.owner.rootRunId, } @@ -54,7 +56,7 @@ export async function listJobs(params: { // the same fallback the row build below uses, keeping order/tie-break // parity), so adapter reverse-resolution, snapshot, pending, and tail work // runs only for the ≤LIST_JOBS_MAX_ROWS rows actually emitted. - const candidates = jobRegistry.list(owner).map((entry) => ({ + const candidates = jobRegistry.list(scopeOwner).map((entry) => ({ entry, status: entry.state, startedAt: entry.startedAt ?? entry.createdAt, From ecf7942b5a21fa104a9c254528b98021ffe04048 Mon Sep 17 00:00:00 2001 From: AnzoBenjamin Date: Mon, 7 Sep 2026 08:17:34 +0300 Subject: [PATCH 7/8] test(background-jobs): pin foreign log preservation on EEXIST spawn Force a registry-issued jobId whose openbuff-.log already exists so the O_EXCL create in startBackgroundJob throws EEXIST; asserts the failed-spawn cleanup preserves the foreign file (logFileCreatedByThisSpawn guard), locking in the reviewer-cleared security fix. --- .../__tests__/run-terminal-command.test.ts | 68 ++++++++++++++++++- 1 file changed, 66 insertions(+), 2 deletions(-) diff --git a/sdk/src/__tests__/run-terminal-command.test.ts b/sdk/src/__tests__/run-terminal-command.test.ts index 1256799d6f..79f681f4da 100644 --- a/sdk/src/__tests__/run-terminal-command.test.ts +++ b/sdk/src/__tests__/run-terminal-command.test.ts @@ -2,11 +2,17 @@ import { spawnSync } from 'child_process' import fs from 'fs' import os from 'os' import path from 'path' -import { describe, expect, it } from 'bun:test' +import { afterEach, describe, expect, it, spyOn } from 'bun:test' +import { jobRegistry } from '@codebuff/common/util/job-registry' import { getOwnedTempRoots } from '@codebuff/common/util/project-path-containment' -import { getBackgroundJob, killBackgroundJob } from '../tools/background-jobs' +import { + __clearJobsForTest, + getBackgroundJob, + killBackgroundJob, + startBackgroundJob, +} from '../tools/background-jobs' import { findWindowsBash, runTerminalCommand, @@ -472,3 +478,61 @@ describe('runTerminalCommand BACKGROUND dirty snapshot at start', () => { } }) }) + +describe('startBackgroundJob failed-spawn log file preservation', () => { + const FORCED_ID = 'job-test-forced-eexist' + const FOREIGN_CONTENT = 'foreign log data that must survive\n' + let foreignLogFile: string | undefined + let createSpy: { mockRestore(): void } | undefined + + afterEach(() => { + createSpy?.mockRestore() + createSpy = undefined + if (foreignLogFile !== undefined) { + fs.rmSync(foreignLogFile, { force: true }) + foreignLogFile = undefined + } + __clearJobsForTest() + }) + + it('preserves a pre-existing foreign log when creation hits EEXIST', () => { + foreignLogFile = path.join(os.tmpdir(), `openbuff-${FORCED_ID}.log`) + fs.writeFileSync(foreignLogFile, FOREIGN_CONTENT) + + // Force the registry to allocate exactly the id whose log file already + // exists, so safeCreateJobLogFile's O_EXCL create fails with EEXIST and + // the catch block runs before spawn is ever reached. + const realCreate = jobRegistry.create.bind(jobRegistry) + const spy = spyOn(jobRegistry, 'create').mockImplementation((opts) => { + realCreate({ ...opts, jobId: FORCED_ID }) + return jobRegistry.get(FORCED_ID)! + }) + createSpy = spy + + try { + expect(() => + startBackgroundJob({ + command: 'sleep 30', + shell: 'sh', + shellArgs: ['-c'], + cwd: process.cwd(), + env: { ...process.env }, + owner: { + clientSessionId: 'session-eexist', + rootRunId: 'root-eexist', + parentRunId: 'parent-eexist', + parentAgentId: 'agent-eexist', + }, + }), + ).toThrow() + } finally { + spy.mockRestore() + createSpy = undefined + } + + // The failed-spawn cleanup must not delete a pre-existing/foreign log + // file this spawn never created (logFileCreatedByThisSpawn guard). + expect(fs.existsSync(foreignLogFile)).toBe(true) + expect(fs.readFileSync(foreignLogFile, 'utf8')).toBe(FOREIGN_CONTENT) + }) +}) From 1c789f544d0e3919fd16b3fbbe6561e8ec2de17f Mon Sep 17 00:00:00 2001 From: AnzoBenjamin Date: Mon, 7 Sep 2026 09:26:46 +0300 Subject: [PATCH 8/8] test(terminal): ensure child cleanup on failure; dedupe git-repo helper The background-job tests now kill their detached sleep 30 children in finally blocks guarded on jobId, so an assertion failure no longer leaks the process; the identical initTempGitRepo helpers in the SYNC and BACKGROUND dirty-delta describes are consolidated into one shared prefix-parameterized helper. Behavior asserted by every test is unchanged. --- .../__tests__/run-terminal-command.test.ts | 179 +++++++++--------- 1 file changed, 88 insertions(+), 91 deletions(-) diff --git a/sdk/src/__tests__/run-terminal-command.test.ts b/sdk/src/__tests__/run-terminal-command.test.ts index 79f681f4da..92099e57ba 100644 --- a/sdk/src/__tests__/run-terminal-command.test.ts +++ b/sdk/src/__tests__/run-terminal-command.test.ts @@ -89,55 +89,67 @@ describe('runTerminalCommand cwd containment', () => { parentRunId: 'parent-1', parentAgentId: 'agent-1', } - const result = await runTerminalCommand({ - command: 'sleep 30', - process_type: 'BACKGROUND', - cwd: process.cwd(), - projectRoot: process.cwd(), - timeout_seconds: 5, - signal: controller.signal, - owner, - }) - const value = result[0].value as { jobId?: string; detached?: boolean } - expect(value.detached).toBe(false) - expect(value.jobId).toBeDefined() - - const runningJob = getBackgroundJob(value.jobId!) - expect(runningJob?.owner).toEqual(owner) - expect( - JSON.parse(fs.readFileSync(runningJob!.metadataFile, 'utf8')).owner, - ).toEqual(owner) - - controller.abort() - await new Promise((resolve) => setTimeout(resolve, 20)) - const job = getBackgroundJob(value.jobId!) - // An abort-initiated kill is an intentional stop, recorded as 'stopped' - // (distinct from an error/non-zero natural exit). - expect(job?.status).toBe('stopped') - killBackgroundJob(value.jobId!, 'SIGKILL') + let value: { jobId?: string; detached?: boolean } | undefined + try { + const result = await runTerminalCommand({ + command: 'sleep 30', + process_type: 'BACKGROUND', + cwd: process.cwd(), + projectRoot: process.cwd(), + timeout_seconds: 5, + signal: controller.signal, + owner, + }) + value = result[0].value as { jobId?: string; detached?: boolean } + expect(value.detached).toBe(false) + expect(value.jobId).toBeDefined() + + const runningJob = getBackgroundJob(value.jobId!) + expect(runningJob?.owner).toEqual(owner) + expect( + JSON.parse(fs.readFileSync(runningJob!.metadataFile, 'utf8')).owner, + ).toEqual(owner) + + controller.abort() + await new Promise((resolve) => setTimeout(resolve, 20)) + const job = getBackgroundJob(value.jobId!) + // An abort-initiated kill is an intentional stop, recorded as 'stopped' + // (distinct from an error/non-zero natural exit). + expect(job?.status).toBe('stopped') + } finally { + if (value?.jobId !== undefined) { + killBackgroundJob(value.jobId, 'SIGKILL') + } + } }) it('terminates a background job that exceeds the bounded log quota', async () => { - const result = await runTerminalCommand({ - command: 'yes x | head -c 12000000; sleep 30', - process_type: 'BACKGROUND', - cwd: process.cwd(), - projectRoot: process.cwd(), - timeout_seconds: 5, - }) - const value = result[0].value as { jobId?: string } - expect(value.jobId).toBeDefined() - - const deadline = Date.now() + 5_000 - let job = getBackgroundJob(value.jobId!) - while (job?.status === 'running' && Date.now() < deadline) { - await new Promise((resolve) => setTimeout(resolve, 25)) - job = getBackgroundJob(value.jobId!) - } + let value: { jobId?: string } | undefined + try { + const result = await runTerminalCommand({ + command: 'yes x | head -c 12000000; sleep 30', + process_type: 'BACKGROUND', + cwd: process.cwd(), + projectRoot: process.cwd(), + timeout_seconds: 5, + }) + value = result[0].value as { jobId?: string } + expect(value.jobId).toBeDefined() + + const deadline = Date.now() + 5_000 + let job = getBackgroundJob(value.jobId!) + while (job?.status === 'running' && Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, 25)) + job = getBackgroundJob(value.jobId!) + } - expect(job?.status).toBe('error') - expect(fs.statSync(job!.logFile).size).toBeLessThanOrEqual(10 * 1024 * 1024) - killBackgroundJob(value.jobId!, 'SIGKILL') + expect(job?.status).toBe('error') + expect(fs.statSync(job!.logFile).size).toBeLessThanOrEqual(10 * 1024 * 1024) + } finally { + if (value?.jobId !== undefined) { + killBackgroundJob(value.jobId, 'SIGKILL') + } + } }) it('accepts the project root itself as cwd', async () => { @@ -298,28 +310,23 @@ describe('runTerminalCommand cwd containment', () => { }) }) -describe('runTerminalCommand SYNC dirty-delta touchedPaths', () => { - function initTempGitRepo(): string { - const projectRoot = fs.mkdtempSync( - path.join(os.tmpdir(), 'terminal-dirty-'), - ) - const run = (args: string[]) => - spawnSync('git', args, { - cwd: projectRoot, - encoding: 'utf8', - }) - expect(run(['init']).status).toBe(0) - run(['config', 'user.email', 'test@example.com']) - run(['config', 'user.name', 'Test']) - // Optional initial commit keeps porcelain stable across git versions. - fs.writeFileSync(path.join(projectRoot, 'README'), 'seed\n') - run(['add', 'README']) - run(['commit', '-m', 'seed']) - return projectRoot - } +function initTempGitRepo(prefix: string): string { + const projectRoot = fs.mkdtempSync(path.join(os.tmpdir(), prefix)) + const run = (args: string[]) => + spawnSync('git', args, { cwd: projectRoot, encoding: 'utf8' }) + expect(run(['init']).status).toBe(0) + run(['config', 'user.email', 'test@example.com']) + run(['config', 'user.name', 'Test']) + // Optional initial commit keeps porcelain stable across git versions. + fs.writeFileSync(path.join(projectRoot, 'README'), 'seed\n') + run(['add', 'README']) + run(['commit', '-m', 'seed']) + return projectRoot +} +describe('runTerminalCommand SYNC dirty-delta touchedPaths', () => { it('reports newly created project files in touchedPaths', async () => { - const projectRoot = initTempGitRepo() + const projectRoot = initTempGitRepo('terminal-dirty-') try { // Pre-existing dirt must not appear in the delta. fs.writeFileSync(path.join(projectRoot, 'already-dirty.txt'), 'old\n') @@ -346,7 +353,7 @@ describe('runTerminalCommand SYNC dirty-delta touchedPaths', () => { }) it('attributes paths relative to projectRoot when cwd is a subdirectory', async () => { - const projectRoot = initTempGitRepo() + const projectRoot = initTempGitRepo('terminal-dirty-') try { const sub = path.join(projectRoot, 'pkg') fs.mkdirSync(sub) @@ -396,26 +403,13 @@ describe('runTerminalCommand SYNC dirty-delta touchedPaths', () => { }) describe('runTerminalCommand BACKGROUND dirty snapshot at start', () => { - function initTempGitRepo(): string { - const projectRoot = fs.mkdtempSync( - path.join(os.tmpdir(), 'terminal-bg-dirty-'), - ) - const run = (args: string[]) => - spawnSync('git', args, { - cwd: projectRoot, - encoding: 'utf8', - }) - expect(run(['init']).status).toBe(0) - run(['config', 'user.email', 'test@example.com']) - run(['config', 'user.name', 'Test']) - fs.writeFileSync(path.join(projectRoot, 'README'), 'seed\n') - run(['add', 'README']) - run(['commit', '-m', 'seed']) - return projectRoot - } - it('stores pre-start dirty snapshot on the job without emitting touchedPaths', async () => { - const projectRoot = initTempGitRepo() + const projectRoot = initTempGitRepo('terminal-bg-dirty-') + let value: { + jobId?: string + touchedPaths?: string[] + backgroundProcessStatus?: string + } | undefined try { fs.writeFileSync(path.join(projectRoot, 'already-dirty.txt'), 'old\n') @@ -426,7 +420,7 @@ describe('runTerminalCommand BACKGROUND dirty snapshot at start', () => { projectRoot, timeout_seconds: 5, }) - const value = result[0].value as { + value = result[0].value as { jobId?: string touchedPaths?: string[] backgroundProcessStatus?: string @@ -442,9 +436,10 @@ describe('runTerminalCommand BACKGROUND dirty snapshot at start', () => { expect(job?.dirtyBeforePaths).toContain('already-dirty.txt') expect(job?.dirtyBeforePaths).not.toContain('created-by-bg.txt') expect(job?.settlementTouchedPaths).toBeUndefined() - - killBackgroundJob(value.jobId!, 'SIGKILL') } finally { + if (value?.jobId !== undefined) { + killBackgroundJob(value.jobId, 'SIGKILL') + } fs.rmSync(projectRoot, { recursive: true, force: true }) } }) @@ -453,6 +448,7 @@ describe('runTerminalCommand BACKGROUND dirty snapshot at start', () => { const projectRoot = fs.mkdtempSync( path.join(os.tmpdir(), 'terminal-bg-nongit-'), ) + let value: { jobId?: string; touchedPaths?: string[] } | undefined try { const result = await runTerminalCommand({ command: 'sleep 30', @@ -461,7 +457,7 @@ describe('runTerminalCommand BACKGROUND dirty snapshot at start', () => { projectRoot, timeout_seconds: 5, }) - const value = result[0].value as { + value = result[0].value as { jobId?: string touchedPaths?: string[] } @@ -471,9 +467,10 @@ describe('runTerminalCommand BACKGROUND dirty snapshot at start', () => { const job = getBackgroundJob(value.jobId!) expect(job?.projectRoot).toBe(projectRoot) expect(job?.dirtyBeforePaths).toBeUndefined() - - killBackgroundJob(value.jobId!, 'SIGKILL') } finally { + if (value?.jobId !== undefined) { + killBackgroundJob(value.jobId, 'SIGKILL') + } fs.rmSync(projectRoot, { recursive: true, force: true }) } })