[GH-ISSUE #6078] [Bug] ADB bugreport staging finalization loses late cancellation #3715

New issue

Closed

opened 2026-10-31 00:43:19 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 00:43:19 +00:00

Owner

Originally created by @coygeek on GitHub (Oct 21, 2026).
Original GitHub issue: https://github.com/morluto/rea/issues/6078

Before submitting

I searched the open and closed issues for a related report.
I removed credentials, tokens, private paths, and sensitive target data.

Area

Evidence, artifacts, or reconstruction

Observed behavior

The public collect-adb-bugreport CLI loses cancellation when it arrives during final staging removal and that removal fails once. The CLI's final close successfully retries removal, but the final JSON reports unreadable_output rather than cancelled. The published archive remains readable and staging is gone.

A control with the same interruption and successful removal returns cancelled. Additional provider/service controls isolate the missing decision: the provider returns ok after cancellation during successful removal, while the shared service correctly converts that success to cancellation. With a removal failure, both retain a cleanup error whose primary resolves to unreadable_output after retry. This report does not claim that clean late cancellation produces successful public CLI/MCP output.

Reproduction

In a built source checkout with locked dependencies and the repository-pinned Node.js, save this test as tests/process-global/adbFinalizationCancellation.test.ts and run:

node node_modules/vitest/vitest.mjs run tests/process-global/adbFinalizationCancellation.test.ts --maxWorkers=1

The two reduced CLI cases both ran successfully as observed-behavior assertions. A combined boundary run also covered an uninterrupted provider control and the provider/service distinction. ADB command output is simulated; publication, hashing, directory identity checks, removal and close retry are real. The wrapper emits SIGINT through the CLI's installed listener when final staging removal starts. It injects only one removal failure; subsequent cleanup executes normally. No device is required. These assertions encode the current incorrect result in the fault case.

import { basename, join } from 'node:path';
import { rm, writeFile, readFile, readdir } from 'node:fs/promises';
import { afterEach, expect, it, vi } from 'vitest';
import { execFileBytes, execFileOutput } from '../../src/process/ExecFileOutput.js';
import { createTestTempDirectory } from '../fixtures/temporaryDirectory.js';
vi.mock('../../src/process/ExecFileOutput.js', async (load) => ({
  ...(await load<typeof import('../../src/process/ExecFileOutput.js')>()),
  execFileBytes: vi.fn(), execFileOutput: vi.fn(),
}));
vi.mock('node:fs/promises', async (load) => {
  const actual = await load<typeof import('node:fs/promises')>();
  return { ...actual, rm: vi.fn(actual.rm) };
});
afterEach(() => vi.restoreAllMocks());
it.each([false,true])('ADB public CLI late cancellation with staging fault=%s',async cleanupFault=>{
  const { serveCli } = await import('../boundary/cli/cliServeHarness.js');
  const root = await createTestTempDirectory('rea-adb-cli-finalization-');
  const binary = join(root,'adb');
  const exitCode = process.exitCode;
  const fs = await vi.importActual<typeof import('node:fs/promises')>('node:fs/promises');
  let interrupted = false;
  vi.mocked(execFileOutput).mockImplementation(async()=>{
    const stdout = `Android Debug Bridge version 1.0.41\nVersion 34.0.5-test\nInstalled as ${binary}\n`;
    return {stdout,stderr:'',stdoutBytes:Buffer.byteLength(stdout),stderrBytes:0};
  });
  vi.mocked(execFileBytes).mockImplementation(async(_binary,args)=>{
    const path=join(args[3]!,'report.zip');
    await writeFile(path,'bugreport-bytes');
    return {stdout:Buffer.from(`Bug report copied to ${path}\n`),stderr:Buffer.alloc(0)};
  });
  vi.mocked(rm).mockImplementation(async(path,options)=>{
    if(basename(String(path)).startsWith('.rea-bugreport-')&&!interrupted){
      interrupted=true; process.emit('SIGINT');
      if(cleanupFault) throw new Error('controlled staging failure');
    }
    return fs.rm(path,options);
  });
  try{
    const run=await serveCli(['collect-adb-bugreport','device','--output-directory',root,'--json'],{REA_ADB_PATH:binary,REA_LOG_LEVEL:'silent',HOME:root});
    const output=JSON.parse(run.stdout);
    expect(interrupted).toBe(true);
    expect(output.code).toBe(cleanupFault?'unreadable_output':'cancelled');
    expect(await readFile(join(root,'report.zip'),'utf8')).toBe('bugreport-bytes');
    expect(await readdir(root)).toEqual(['report.zip']);
    console.log(JSON.stringify({case:'adb-cli',cleanupFault,code:output.code,archiveRetained:true,stagingRemoved:true}));
  }finally{process.exitCode=exitCode;}
});

Expected behavior

Check the signal after final staging cleanup and before deciding success or selecting a cleanup primary. A cancellation observed in this interval should preserve its exact reason and completed bugreport facts. If removal remains incomplete, retain cleanup_incomplete and its owner; once retry confirms release, the primary should resolve to cancelled. Keep any genuinely earlier protocol, publication or digest failure authoritative.

The lifecycle contract requires cancellation through awaited cleanup and a final signal check. The testing contract explicitly says a downstream guard does not repair the provider's own completion contract. The existing ADB runtime-root owner already makes that final cancellation decision before composing cleanup. The committed archive must remain available; cancellation is not a request to erase completed publication.

Impact and scope

P2: the cleanup-failure branch reports an output failure for a cancelled acquisition after cleanup has actually recovered, sending callers toward the wrong recovery action. The clean-removal branch is compensated by the shared service. The deterministic CLI interleavings each ran once in the reduced run and also passed in the combined run; natural occurrence frequency is unknown. A caller that retains its own interruption state can recognize the cancellation separately and inspect the retained archive receipt. No archive loss, device mutation or persistent staging leak was observed. Physical ADB/device behavior and MCP transport were not exercised.

REA version

rea-agents 6.7.0; source revision 52415975942a4b9c53a817915b313ccc404f280e.

Runtime and platform

macOS 27.0.1 arm64; Node.js v24.18.0; locked Vitest 5.0.3. Real source CLI dispatcher with simulated ADB command replies.

Target and artifact context

Synthetic 15-byte bugreport archive written by the simulated producer into REA-owned staging and published to the selected directory. No Android device or application was used.

Interface

CLI

Earliest failure stage

Cleanup, cancellation, or lifecycle

Relevant output or evidence

Observed public CLI results:

{"case":"adb-cli","cleanupFault":false,"code":"cancelled","archiveRetained":true,"stagingRemoved":true}
{"case":"adb-cli","cleanupFault":true,"code":"unreadable_output","archiveRetained":true,"stagingRemoved":true}

Independent source-boundary controls observed ok: true from the cancelled clean provider, cancelled with the exact caller reason and completed Evidence from the clean service, and cleanup_incomplete changing to unreadable_output after successful provider close for the removal-fault case. Both archived bytes and staging absence were read back.

Typed outcome or recovery expected

Preserve AnalysisCancelledError("collect_adb_bugreport") with the caller reason and completed archive path, byte count, digest and original ADB-reported path. While staging cleanup is unresolved, keep its existing typed owner and cleanup diagnostics; successful retry should reveal the cancellation primary.

Analysis or suspected cause

Source-derived: collectBugreport finalization samples no signal after removeOwnedOutputDirectory. On successful acquisition plus removal failure it constructs AnalysisOutputError as the cleanup primary. The shared provider workflow compensates only successful provider results, so an already-constructed failure bypasses that check. CLI finalization awaits close and projects the recovered primary; it correctly preserves an existing failure and therefore cannot reconstruct the omitted cancellation decision.

This requires a cancellation decision at the staging owner, with completed observation retention and cleanup composition. Changing the shared adapter's failure precedence would risk hiding real earlier failures.

Additional context

Related #2220 / PR #2221 establish collision-safe archive publication; #4598 concerns a preceding physical digest failure being replaced by cancellation; PR #4443 concerns descriptor/hash finalization. This interval occurs after the digest completes. #3927 / PR #4274 explain the existing shared success guard. #4939 concerns a different CLI close-signal omission; this command already supplies its signal. #5971, #5978, #5981 and #6045–#6047 cover different Android/ADB admission, mutation, aggregation or descriptor owners. #6048 and #6049 concern other completion owners.

Fetched upstream main f86206535f8ec71014781f9d432a84b01e183e5d; the bugreport method and relevant shared completion logic are unchanged. That newer revision was source-compared, not executed. No fix or regression range was established.

Originally created by @coygeek on GitHub (Oct 21, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/6078 ### Before submitting - [x] I searched the open and closed issues for a related report. - [x] I removed credentials, tokens, private paths, and sensitive target data. ### Area Evidence, artifacts, or reconstruction ### Observed behavior The public `collect-adb-bugreport` CLI loses cancellation when it arrives during final staging removal and that removal fails once. The CLI's final close successfully retries removal, but the final JSON reports `unreadable_output` rather than `cancelled`. The published archive remains readable and staging is gone. A control with the same interruption and successful removal returns `cancelled`. Additional provider/service controls isolate the missing decision: the provider returns `ok` after cancellation during successful removal, while the shared service correctly converts that success to cancellation. With a removal failure, both retain a cleanup error whose primary resolves to `unreadable_output` after retry. This report does not claim that clean late cancellation produces successful public CLI/MCP output. ### Reproduction In a built source checkout with locked dependencies and the repository-pinned Node.js, save this test as `tests/process-global/adbFinalizationCancellation.test.ts` and run: ```sh node node_modules/vitest/vitest.mjs run tests/process-global/adbFinalizationCancellation.test.ts --maxWorkers=1 ``` The two reduced CLI cases both ran successfully as observed-behavior assertions. A combined boundary run also covered an uninterrupted provider control and the provider/service distinction. ADB command output is simulated; publication, hashing, directory identity checks, removal and close retry are real. The wrapper emits SIGINT through the CLI's installed listener when final staging removal starts. It injects only one removal failure; subsequent cleanup executes normally. No device is required. These assertions encode the current incorrect result in the fault case. ```ts import { basename, join } from 'node:path'; import { rm, writeFile, readFile, readdir } from 'node:fs/promises'; import { afterEach, expect, it, vi } from 'vitest'; import { execFileBytes, execFileOutput } from '../../src/process/ExecFileOutput.js'; import { createTestTempDirectory } from '../fixtures/temporaryDirectory.js'; vi.mock('../../src/process/ExecFileOutput.js', async (load) => ({ ...(await load<typeof import('../../src/process/ExecFileOutput.js')>()), execFileBytes: vi.fn(), execFileOutput: vi.fn(), })); vi.mock('node:fs/promises', async (load) => { const actual = await load<typeof import('node:fs/promises')>(); return { ...actual, rm: vi.fn(actual.rm) }; }); afterEach(() => vi.restoreAllMocks()); it.each([false,true])('ADB public CLI late cancellation with staging fault=%s',async cleanupFault=>{ const { serveCli } = await import('../boundary/cli/cliServeHarness.js'); const root = await createTestTempDirectory('rea-adb-cli-finalization-'); const binary = join(root,'adb'); const exitCode = process.exitCode; const fs = await vi.importActual<typeof import('node:fs/promises')>('node:fs/promises'); let interrupted = false; vi.mocked(execFileOutput).mockImplementation(async()=>{ const stdout = `Android Debug Bridge version 1.0.41\nVersion 34.0.5-test\nInstalled as ${binary}\n`; return {stdout,stderr:'',stdoutBytes:Buffer.byteLength(stdout),stderrBytes:0}; }); vi.mocked(execFileBytes).mockImplementation(async(_binary,args)=>{ const path=join(args[3]!,'report.zip'); await writeFile(path,'bugreport-bytes'); return {stdout:Buffer.from(`Bug report copied to ${path}\n`),stderr:Buffer.alloc(0)}; }); vi.mocked(rm).mockImplementation(async(path,options)=>{ if(basename(String(path)).startsWith('.rea-bugreport-')&&!interrupted){ interrupted=true; process.emit('SIGINT'); if(cleanupFault) throw new Error('controlled staging failure'); } return fs.rm(path,options); }); try{ const run=await serveCli(['collect-adb-bugreport','device','--output-directory',root,'--json'],{REA_ADB_PATH:binary,REA_LOG_LEVEL:'silent',HOME:root}); const output=JSON.parse(run.stdout); expect(interrupted).toBe(true); expect(output.code).toBe(cleanupFault?'unreadable_output':'cancelled'); expect(await readFile(join(root,'report.zip'),'utf8')).toBe('bugreport-bytes'); expect(await readdir(root)).toEqual(['report.zip']); console.log(JSON.stringify({case:'adb-cli',cleanupFault,code:output.code,archiveRetained:true,stagingRemoved:true})); }finally{process.exitCode=exitCode;} }); ``` ### Expected behavior Check the signal after final staging cleanup and before deciding success or selecting a cleanup primary. A cancellation observed in this interval should preserve its exact reason and completed bugreport facts. If removal remains incomplete, retain `cleanup_incomplete` and its owner; once retry confirms release, the primary should resolve to `cancelled`. Keep any genuinely earlier protocol, publication or digest failure authoritative. The [lifecycle contract](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/docs/tool-design.md#L101-L113) requires cancellation through awaited cleanup and a final signal check. The [testing contract](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/docs/testing.md#L321-L326) explicitly says a downstream guard does not repair the provider's own completion contract. [The existing ADB runtime-root owner](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/adb/AdbProvider.ts#L1053-L1077) already makes that final cancellation decision before composing cleanup. The committed archive must remain available; cancellation is not a request to erase completed publication. ### Impact and scope P2: the cleanup-failure branch reports an output failure for a cancelled acquisition after cleanup has actually recovered, sending callers toward the wrong recovery action. The clean-removal branch is compensated by the shared service. The deterministic CLI interleavings each ran once in the reduced run and also passed in the combined run; natural occurrence frequency is unknown. A caller that retains its own interruption state can recognize the cancellation separately and inspect the retained archive receipt. No archive loss, device mutation or persistent staging leak was observed. Physical ADB/device behavior and MCP transport were not exercised. ### REA version rea-agents 6.7.0; source revision `52415975942a4b9c53a817915b313ccc404f280e`. ### Runtime and platform macOS 27.0.1 arm64; Node.js v24.18.0; locked Vitest 5.0.3. Real source CLI dispatcher with simulated ADB command replies. ### Target and artifact context Synthetic 15-byte bugreport archive written by the simulated producer into REA-owned staging and published to the selected directory. No Android device or application was used. ### Interface CLI ### Earliest failure stage Cleanup, cancellation, or lifecycle ### Relevant output or evidence Observed public CLI results: ```jsonl {"case":"adb-cli","cleanupFault":false,"code":"cancelled","archiveRetained":true,"stagingRemoved":true} {"case":"adb-cli","cleanupFault":true,"code":"unreadable_output","archiveRetained":true,"stagingRemoved":true} ``` Independent source-boundary controls observed `ok: true` from the cancelled clean provider, `cancelled` with the exact caller reason and completed Evidence from the clean service, and `cleanup_incomplete` changing to `unreadable_output` after successful provider close for the removal-fault case. Both archived bytes and staging absence were read back. ### Typed outcome or recovery expected Preserve `AnalysisCancelledError("collect_adb_bugreport")` with the caller reason and completed archive path, byte count, digest and original ADB-reported path. While staging cleanup is unresolved, keep its existing typed owner and cleanup diagnostics; successful retry should reveal the cancellation primary. ### Analysis or suspected cause Source-derived: [collectBugreport finalization](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/adb/AdbProvider.ts#L3182-L3215) samples no signal after `removeOwnedOutputDirectory`. On successful acquisition plus removal failure it constructs `AnalysisOutputError` as the cleanup primary. [The shared provider workflow](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/application/ProviderOperationExecution.ts#L78-L111) compensates only successful provider results, so an already-constructed failure bypasses that check. [CLI finalization](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/cli/finishAnalysisCommand.ts#L27-L82) awaits close and projects the recovered primary; it correctly preserves an existing failure and therefore cannot reconstruct the omitted cancellation decision. This requires a cancellation decision at the staging owner, with completed observation retention and cleanup composition. Changing the shared adapter's failure precedence would risk hiding real earlier failures. ### Additional context Related #2220 / PR #2221 establish collision-safe archive publication; #4598 concerns a preceding physical digest failure being replaced by cancellation; PR #4443 concerns descriptor/hash finalization. This interval occurs after the digest completes. #3927 / PR #4274 explain the existing shared success guard. #4939 concerns a different CLI close-signal omission; this command already supplies its signal. #5971, #5978, #5981 and #6045–#6047 cover different Android/ADB admission, mutation, aggregation or descriptor owners. #6048 and #6049 concern other completion owners. Fetched upstream main `f86206535f8ec71014781f9d432a84b01e183e5d`; the bugreport method and relevant shared completion logic are unchanged. That newer revision was source-compared, not executed. No fix or regression range was established.

bsakaguchi closed this issue

2026-10-31 00:43:19 +00:00

bsakaguchi referenced this issue

2026-11-01 21:10:57 +00:00

[GH-ISSUE #3796] [Tracker] Open feature requests by area (October 2026) #1471