[GH-ISSUE #2657] fix(binary): restored session retains a retired client's unavailable operations #958

New issue

Closed

opened 2026-10-15 18:29:54 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-15 18:29:54 +00:00

Owner

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

Summary

After canceling a target switch during provider cleanup, I can query the restored target successfully, but discovery still describes that operation as unavailable until I retry it. The restored analysis run carries the previous client's failure reason into both its provider capability group and its tool-availability remediation. This affects callers that choose operations from the documented current session status.

Steps to reproduce

Run this source test from the checkout with its pinned toolchain and locked dependencies already prepared. It uses two tiny analysis-database fixtures and an inert selected provider, with production registry and composite routing. The test pauses an ordinary asynchronous close, cancels the switch, releases cleanup, and observes the restored session before retrying. No provider engine is needed. This was executed at the shared application boundary; a public MCP transport reproduction was not executed.

Save the following as src/application/binary/restoredAvailability.repro.test.ts:

import { mkdir, writeFile, rm } from "node:fs/promises";
import { join } from "node:path";
import { expect, it } from "vitest";
import { createCacheProvider, createDeferred, createTestBinarySession } from "../../../tests/fixtures/binarySession.js";
import { observed } from "../../../tests/fixtures/analysisExecution.js";
import { err, ok } from "../../domain/result.js";
import { buildCapabilityInventory } from "../CapabilityInventory.js";
import { ProviderAdapterError } from "../../domain/providerAdapterError.js";

it("observes retired-client availability after cancellation with composite routing", async () => {
  const directory = process.env.REA_REPRO_DIRECTORY!;
  await mkdir(directory, { recursive: true });
  const first = join(directory, "first.hop");
  const second = join(directory, "second.hop");
  await writeFile(first, "one");
  await writeFile(second, "two");
  const provider = createCacheProvider([]);
  const closing = createDeferred<null>();
  const release = createDeferred<null>();
  const clients: { closed: number; fail: boolean; calls: number }[] = [];
  provider.createClient = () => {
    const state = { closed: 0, fail: clients.length === 0, calls: 0 };
    clients.push(state);
    return {
      execute: (operation) => {
        state.calls++;
        return Promise.resolve(state.fail
          ? err(new ProviderAdapterError("fixture", operation))
          : observed("restored-name"));
      },
      close: async () => {
        state.closed++;
        if (clients.length === 1 && state.closed === 1) {
          closing.resolve(null);
          await release.promise;
        }
        return ok(null);
      },
    };
  };
  const session = createTestBinarySession(provider);
  const facts = () => {
    const status = session.status() as any;
    const group = status.capabilities.flatMap((p: any) => p.groups)
      .find((g: any) => g.operations.includes("address_name"));
    return { open: status.open, available: group.available, reason: group.reason, run: status.analysis_run.run_id };
  };
  try {
    expect((await session.open(first)).ok).toBe(true);
    expect(clients).toHaveLength(0); // Composite health does not instantiate a child.
    expect((await session.execute("address_name", {})).ok).toBe(false);
    const before = facts();
    const originalTarget = session.activeTarget();
    expect(before.available).toBe(false);
    const controller = new AbortController();
    const switching = session.open(second, { signal: controller.signal });
    await closing.promise;
    controller.abort();
    release.resolve(null);
    expect(await switching).toMatchObject({ ok: false, error: { _tag: "AnalysisCancelledError" } });
    expect(session.activeTarget()).toEqual(originalTarget);
    expect(clients).toHaveLength(1);
    expect(clients[0]?.closed).toBe(1);
    const restored = facts();
    expect(restored.run).not.toBe(before.run);
    expect(restored.available).toBe(false);
    expect(restored.reason).toBe(before.reason);
    expect(buildCapabilityInventory(session.status(), { processCaptureEnabled: false }).find(t => t.name === "address_name")).toMatchObject({ available: false, reason: "provider_unavailable", remediation: before.reason });
    expect((await session.execute("address_name", {})).ok).toBe(true);
    expect(clients).toHaveLength(2);
    expect(clients[1]?.calls).toBe(1);
    const retried = facts();
    expect(retried.available).toBe(true);
    expect(retried.reason).toBe(null);
    clients[1]!.fail = true;
    expect((await session.execute("address_name", {})).ok).toBe(false);
    expect(facts().available).toBe(false);
    expect((await session.open(second)).ok).toBe(true);
    const switched = facts();
    expect(switched.available).toBe(true);
    expect(switched.reason).toBe(null);
    console.log(JSON.stringify({ before: { available: before.available, reason: before.reason }, restored: { open: restored.open, newRun: restored.run !== before.run, available: restored.available, reason: restored.reason }, retry: { available: retried.available, reason: retried.reason }, genuineFailure: { available: false }, successfulSwitch: { available: switched.available, reason: switched.reason }, childClients: clients.length }));
  } finally {
    release.resolve(null);
    await session.close();
    await rm(directory, { recursive: true, force: true });
  }
});

Run only that test:

REA_REPRO_DIRECTORY="$(mktemp -d)" npm run test:local -- src/application/binary/restoredAvailability.repro.test.ts

The test asserts the current defective result so it completes successfully while printing the mismatch and its controls. A restoration regression check should instead require current availability immediately after restoration.

Expected behavior

Session discovery should describe the restored current route. A runtime error recorded from a closed client should not continue to mark the replacement run's callable operation unavailable. A real failure on the replacement client should still affect discovery. The contract promises current target/provider/capability status, and callers are instructed to choose operations from that status.

Actual behavior

The executed fixture restored the original target with a different analysis run ID and retained the exact previous error. The derived tool inventory reported address_name as unavailable with reason provider_unavailable, and its remediation repeated the retired client's error. A direct query then succeeded on a new child client and cleared that operation's marker. A genuine failure on that child marked it unavailable again. A subsequent successful target switch reset the marker.

{"before":{"available":false,"reason":"ProviderAdapterError: Provider fixture adapter failed during address_name"},"restored":{"open":true,"newRun":true,"available":false,"reason":"ProviderAdapterError: Provider fixture adapter failed during address_name"},"retry":{"available":true,"reason":null},"genuineFailure":{"available":false},"successfulSwitch":{"available":true,"reason":null},"childClients":2}

One focused test passed after correcting an initial fixture assertion that compared the input path with a canonicalized path. The final test compares the complete original and restored target identities. No provider-runtime success or failure is inferred from the inert fixture.

Evidence

Expected source: src/contracts/sessionToolContracts.ts:80-84 defines binary_session as complete current capability status. docs/mcp-contracts.md:277-286 directs callers to choose callable operations from current tool availability; lines 313-317 distinguish current route capabilities from candidate capabilities.
Failure source: The inline focused fixture directly observed the stale capability group and its derived tool-availability entry. src/application/binary/BinarySession.ts:290-295,483-490,827-855 reaches restoration after cancellation during successful cleanup and installs a replacement without resetting the recorded runtime failures. src/application/binary/BinarySessionStatus.ts:45-60 overlays the old reason; src/application/CapabilityInventory.ts:621-626 projects it into tool availability.
Evidence provenance: observed
Local verification: reproduced
Reproduction completeness: complete

The fixture directly exercises the shared session and availability-inventory components used by MCP. Public cancellation forwarding and status delivery were checked from source at src/server/registerSessionTools.ts:189-218 and src/server/registerSessionStatusTool.ts:66-99,108-121. No MCP server was started.

Restoration check

Repeat the cancellation sequence with the same supported inputs. After cleanup restores the original target on a new run, the operation's capability group should report available: true with reason: null; its derived tool availability should also be available without the retired error. Preserve the cancellation result and original target identity. A new failure on the restored child must still mark the operation unavailable, a successful direct retry must clear its marker, and a successful switch must continue to reset runtime availability.

Runtime or environment

Source revision a2d8b94dd5. Node 24.18.0 and npm 11.16.0. The focused source test ran against this checkout with Vitest 5.0.3. No released package or real provider runtime was executed.

Current upstream recheck at 3ae826ceba found the relevant cancellation, restoration, execution-admission, and status-projection behavior unchanged, so the defect remains present by source inspection. The focused fixture was not rerun at this revision. The latest published release is rea-agents 6.4.0; it was not executed.

Related: #2528 covers a stale Frida adapter-load failure after recovery; #2590 covers reusing a dead child during same-target reopening; #1166 covers availability for missing or unsupported targets. Each has a distinct trigger and restoration check.

Triage assessment

Impact level: P2
Assessment status: supported
Impact basis: The restored current session advertises a callable operation as unavailable and propagates the retired failure into operation-selection guidance. The effect is bounded to restoration after a recorded runtime failure; dispatch itself remains callable, and no data loss was observed.
Workaround status: available
Workaround basis: The fixture directly queried the affected operation on the restored client, received a successful result, and observed its availability marker clear. This bypasses the inaccurate discovery guidance without reopening or discarding the session.

Originally created by @coygeek on GitHub (Oct 13, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/2657 ## Summary After canceling a target switch during provider cleanup, I can query the restored target successfully, but discovery still describes that operation as unavailable until I retry it. The restored analysis run carries the previous client's failure reason into both its provider capability group and its tool-availability remediation. This affects callers that choose operations from the documented current session status. ## Steps to reproduce Run this source test from the checkout with its pinned toolchain and locked dependencies already prepared. It uses two tiny analysis-database fixtures and an inert selected provider, with production registry and composite routing. The test pauses an ordinary asynchronous close, cancels the switch, releases cleanup, and observes the restored session before retrying. No provider engine is needed. This was executed at the shared application boundary; a public MCP transport reproduction was not executed. Save the following as `src/application/binary/restoredAvailability.repro.test.ts`: ```typescript import { mkdir, writeFile, rm } from "node:fs/promises"; import { join } from "node:path"; import { expect, it } from "vitest"; import { createCacheProvider, createDeferred, createTestBinarySession } from "../../../tests/fixtures/binarySession.js"; import { observed } from "../../../tests/fixtures/analysisExecution.js"; import { err, ok } from "../../domain/result.js"; import { buildCapabilityInventory } from "../CapabilityInventory.js"; import { ProviderAdapterError } from "../../domain/providerAdapterError.js"; it("observes retired-client availability after cancellation with composite routing", async () => { const directory = process.env.REA_REPRO_DIRECTORY!; await mkdir(directory, { recursive: true }); const first = join(directory, "first.hop"); const second = join(directory, "second.hop"); await writeFile(first, "one"); await writeFile(second, "two"); const provider = createCacheProvider([]); const closing = createDeferred<null>(); const release = createDeferred<null>(); const clients: { closed: number; fail: boolean; calls: number }[] = []; provider.createClient = () => { const state = { closed: 0, fail: clients.length === 0, calls: 0 }; clients.push(state); return { execute: (operation) => { state.calls++; return Promise.resolve(state.fail ? err(new ProviderAdapterError("fixture", operation)) : observed("restored-name")); }, close: async () => { state.closed++; if (clients.length === 1 && state.closed === 1) { closing.resolve(null); await release.promise; } return ok(null); }, }; }; const session = createTestBinarySession(provider); const facts = () => { const status = session.status() as any; const group = status.capabilities.flatMap((p: any) => p.groups) .find((g: any) => g.operations.includes("address_name")); return { open: status.open, available: group.available, reason: group.reason, run: status.analysis_run.run_id }; }; try { expect((await session.open(first)).ok).toBe(true); expect(clients).toHaveLength(0); // Composite health does not instantiate a child. expect((await session.execute("address_name", {})).ok).toBe(false); const before = facts(); const originalTarget = session.activeTarget(); expect(before.available).toBe(false); const controller = new AbortController(); const switching = session.open(second, { signal: controller.signal }); await closing.promise; controller.abort(); release.resolve(null); expect(await switching).toMatchObject({ ok: false, error: { _tag: "AnalysisCancelledError" } }); expect(session.activeTarget()).toEqual(originalTarget); expect(clients).toHaveLength(1); expect(clients[0]?.closed).toBe(1); const restored = facts(); expect(restored.run).not.toBe(before.run); expect(restored.available).toBe(false); expect(restored.reason).toBe(before.reason); expect(buildCapabilityInventory(session.status(), { processCaptureEnabled: false }).find(t => t.name === "address_name")).toMatchObject({ available: false, reason: "provider_unavailable", remediation: before.reason }); expect((await session.execute("address_name", {})).ok).toBe(true); expect(clients).toHaveLength(2); expect(clients[1]?.calls).toBe(1); const retried = facts(); expect(retried.available).toBe(true); expect(retried.reason).toBe(null); clients[1]!.fail = true; expect((await session.execute("address_name", {})).ok).toBe(false); expect(facts().available).toBe(false); expect((await session.open(second)).ok).toBe(true); const switched = facts(); expect(switched.available).toBe(true); expect(switched.reason).toBe(null); console.log(JSON.stringify({ before: { available: before.available, reason: before.reason }, restored: { open: restored.open, newRun: restored.run !== before.run, available: restored.available, reason: restored.reason }, retry: { available: retried.available, reason: retried.reason }, genuineFailure: { available: false }, successfulSwitch: { available: switched.available, reason: switched.reason }, childClients: clients.length })); } finally { release.resolve(null); await session.close(); await rm(directory, { recursive: true, force: true }); } }); ``` Run only that test: ```sh REA_REPRO_DIRECTORY="$(mktemp -d)" npm run test:local -- src/application/binary/restoredAvailability.repro.test.ts ``` The test asserts the current defective result so it completes successfully while printing the mismatch and its controls. A restoration regression check should instead require current availability immediately after restoration. ## Expected behavior Session discovery should describe the restored current route. A runtime error recorded from a closed client should not continue to mark the replacement run's callable operation unavailable. A real failure on the replacement client should still affect discovery. The contract promises current target/provider/capability status, and callers are instructed to choose operations from that status. ## Actual behavior The executed fixture restored the original target with a different analysis run ID and retained the exact previous error. The derived tool inventory reported `address_name` as unavailable with reason `provider_unavailable`, and its remediation repeated the retired client's error. A direct query then succeeded on a new child client and cleared that operation's marker. A genuine failure on that child marked it unavailable again. A subsequent successful target switch reset the marker. ```json {"before":{"available":false,"reason":"ProviderAdapterError: Provider fixture adapter failed during address_name"},"restored":{"open":true,"newRun":true,"available":false,"reason":"ProviderAdapterError: Provider fixture adapter failed during address_name"},"retry":{"available":true,"reason":null},"genuineFailure":{"available":false},"successfulSwitch":{"available":true,"reason":null},"childClients":2} ``` One focused test passed after correcting an initial fixture assertion that compared the input path with a canonicalized path. The final test compares the complete original and restored target identities. No provider-runtime success or failure is inferred from the inert fixture. ## Evidence - Expected source: `src/contracts/sessionToolContracts.ts:80-84` defines `binary_session` as complete current capability status. `docs/mcp-contracts.md:277-286` directs callers to choose callable operations from current tool availability; lines 313-317 distinguish current route capabilities from candidate capabilities. - Failure source: The inline focused fixture directly observed the stale capability group and its derived tool-availability entry. `src/application/binary/BinarySession.ts:290-295,483-490,827-855` reaches restoration after cancellation during successful cleanup and installs a replacement without resetting the recorded runtime failures. `src/application/binary/BinarySessionStatus.ts:45-60` overlays the old reason; `src/application/CapabilityInventory.ts:621-626` projects it into tool availability. - Evidence provenance: observed - Local verification: reproduced - Reproduction completeness: complete The fixture directly exercises the shared session and availability-inventory components used by MCP. Public cancellation forwarding and status delivery were checked from source at `src/server/registerSessionTools.ts:189-218` and `src/server/registerSessionStatusTool.ts:66-99,108-121`. No MCP server was started. ## Restoration check Repeat the cancellation sequence with the same supported inputs. After cleanup restores the original target on a new run, the operation's capability group should report `available: true` with `reason: null`; its derived tool availability should also be available without the retired error. Preserve the cancellation result and original target identity. A new failure on the restored child must still mark the operation unavailable, a successful direct retry must clear its marker, and a successful switch must continue to reset runtime availability. ## Runtime or environment Source revision a2d8b94dd50dab8d966239dd63ab84718aa8b4b4. Node 24.18.0 and npm 11.16.0. The focused source test ran against this checkout with Vitest 5.0.3. No released package or real provider runtime was executed. Current upstream recheck at 3ae826ceba6f25b08bce50bcb51001209bc16b57 found the relevant cancellation, restoration, execution-admission, and status-projection behavior unchanged, so the defect remains present by source inspection. The focused fixture was not rerun at this revision. The latest published release is rea-agents 6.4.0; it was not executed. Related: #2528 covers a stale Frida adapter-load failure after recovery; #2590 covers reusing a dead child during same-target reopening; #1166 covers availability for missing or unsupported targets. Each has a distinct trigger and restoration check. ## Triage assessment - Impact level: P2 - Assessment status: supported - Impact basis: The restored current session advertises a callable operation as unavailable and propagates the retired failure into operation-selection guidance. The effect is bounded to restoration after a recorded runtime failure; dispatch itself remains callable, and no data loss was observed. - Workaround status: available - Workaround basis: The fixture directly queried the affected operation on the restored client, received a successful result, and observed its availability marker clear. This bypasses the inaccurate discovery guidance without reopening or discarding the session.

bsakaguchi

2026-10-15 18:29:54 +00:00