fix(shared): make ApiManager refresh paths actually replace the stale api

packages/shared/src/services/pendulum/apiManager.test.ts

@@ -1,5 +1,6 @@
-import { afterEach, describe, expect, it } from "bun:test";
-import type { NetworkConfig } from "./apiManager";
+import { afterEach, describe, expect, it, mock } from "bun:test";
+import type { ApiPromise } from "@polkadot/api";
+import { type API, ApiManager, type NetworkConfig, type SubstrateApiNetwork } from "./apiManager";
 
 const ORIGINAL_ENV = { ...process.env };
 
@@ -53,3 +54,103 @@ describe("ApiManager network configuration", () => {
     ]);
   });
 });
+
+// Exposes the private members needed to drive the manager without real WS connections.
+type TestableApiManager = {
+  apiInstances: Map<string, API>;
+  previousSpecVersions: Map<string, number>;
+  connectApi: (networkName: SubstrateApiNetwork, wsUrlIndex?: number) => Promise<API>;
+  populateApi: ApiManager["populateApi"];
+  getApi: ApiManager["getApi"];
+};
+
+function createManager(): TestableApiManager {
+  return new (ApiManager as unknown as new () => TestableApiManager)();
+}
+
+function createFakeApi(getSpecVersion: () => number): { instance: API; disconnect: ReturnType<typeof mock> } {
+  const disconnect = mock(() => Promise.resolve());
+  const api = {
+    call: {
+      core: {
+        version: () => Promise.resolve({ toHuman: () => ({ specVersion: getSpecVersion() }) })
+      }
+    },
+    disconnect,
+    isConnected: true,
+    isReady: Promise.resolve()
+  };
+  return { disconnect, instance: { api: api as unknown as ApiPromise, decimals: 12, ss58Format: 42 } };
+}
+
+describe("ApiManager api refresh", () => {
+  function setupManager() {
+    const manager = createManager();
+    let onChainSpecVersion = 1;
+    const fakes: ReturnType<typeof createFakeApi>[] = [];
+
+    // Mimics the real connectApi: returns a fresh instance reading the current on-chain
+    // spec version and records that version as the previous one.
+    const connectApiMock = mock((networkName: SubstrateApiNetwork) => {
+      const fake = createFakeApi(() => onChainSpecVersion);
+      fakes.push(fake);
+      manager.previousSpecVersions.set(networkName, onChainSpecVersion);
+      return Promise.resolve(fake.instance);
+    });
+    manager.connectApi = connectApiMock;
+
+    return { connectApiMock, fakes, manager, setSpecVersion: (version: number) => (onChainSpecVersion = version) };
+  }
+
+  it("populateApi is idempotent and reuses the cached instance", async () => {
+    const { fakes, manager } = setupManager();
+
+    const first = await manager.populateApi("pendulum");
+    const second = await manager.populateApi("pendulum");
+
+    expect(second).toBe(first);
+    expect(manager.connectApi).toHaveBeenCalledTimes(1);
+    expect(fakes[0].disconnect).not.toHaveBeenCalled();
+  });
+
+  it("getApi replaces and disconnects the cached instance when the spec version changes", async () => {
+    const { fakes, manager, setSpecVersion } = setupManager();
+
+    const initial = await manager.populateApi("pendulum");
+
+    // Simulate a runtime upgrade on chain
+    setSpecVersion(2);
+
+    const refreshed = await manager.getApi("pendulum");
+
+    expect(refreshed).not.toBe(initial);
+    expect(manager.connectApi).toHaveBeenCalledTimes(2);
+    expect(fakes[0].disconnect).toHaveBeenCalledTimes(1);
+    expect(manager.apiInstances.get("pendulum-0")).toBe(refreshed);
+  });
+
+  it("keeps the cached instance when the refresh reconnect fails", async () => {
+    const { connectApiMock, fakes, manager, setSpecVersion } = setupManager();
+
+    const initial = await manager.populateApi("pendulum");
+
+    setSpecVersion(2);
+    connectApiMock.mockImplementationOnce(() => Promise.reject(new Error("reconnect failed")));
+
+    await expect(manager.getApi("pendulum")).rejects.toThrow("reconnect failed");
+
+    expect(manager.apiInstances.get("pendulum-0")).toBe(initial);
+    expect(fakes[0].disconnect).not.toHaveBeenCalled();
+  });
+
+  it("getApi with forceRefresh replaces and disconnects the cached instance", async () => {
+    const { fakes, manager } = setupManager();
+
+    const initial = await manager.populateApi("pendulum");
+    const refreshed = await manager.getApi("pendulum", true);
+
+    expect(refreshed).not.toBe(initial);
+    expect(fakes[0].disconnect).toHaveBeenCalledTimes(1);
+    expect(manager.apiInstances.get("pendulum-0")).toBe(refreshed);
+  });
+});

packages/shared/src/services/pendulum/apiManager.ts

@@ -107,12 +107,42 @@ export class ApiManager {
     if (existingInstance) {
       return existingInstance;
     }
+    return await this.createAndCacheApi(networkName, index);
+  }
+
+  /**
+   * Discards the cached API instance (disconnecting it) and creates a fresh one.
+   * Unlike populateApi, this always replaces the cached instance.
+   */
+  public async refreshApi(networkName: SubstrateApiNetwork, wsUrlIndex?: number): Promise<API> {
+    const index = wsUrlIndex ?? 0;
+    const instanceKey = this.generateInstanceKey(networkName, index);
+    const staleInstance = this.apiInstances.get(instanceKey);
+
+    // Create the replacement before dropping the stale instance so a failed reconnect
+    // doesn't leave the network without a cached api.
+    const newApi = await this.createAndCacheApi(networkName, index);
+
+    if (staleInstance) {
+      try {
+        await staleInstance.api.disconnect();
+      } catch (error) {
+        logger.current.warn(`Failed to disconnect stale API instance for ${instanceKey}: ${error}`);
+      }
+    }
+
+    return newApi;
+  }
+
+  private async createAndCacheApi(networkName: SubstrateApiNetwork, index: number): Promise<API> {
     const newApi = await this.connectApi(networkName, index);
-    this.apiInstances.set(instanceKey, newApi);
 
     if (!newApi.api.isConnected) await newApi.api.connect();
     await newApi.api.isReady;
 
+    // Cache only once the api is ready so concurrent readers never observe a connecting instance.
+    this.apiInstances.set(this.generateInstanceKey(networkName, index), newApi);
+
     return newApi;
   }
 
@@ -127,16 +157,20 @@ export class ApiManager {
     const instanceKey = this.generateInstanceKey(networkName, 0);
     const apiInstance = this.apiInstances.get(instanceKey);
 
-    if (!apiInstance || forceRefresh) {
+    if (!apiInstance) {
       return await this.populateApi(networkName);
     }
 
+    if (forceRefresh) {
+      return await this.refreshApi(networkName);
+    }
+
     const currentSpecVersion = await this.getSpecVersion(apiInstance.api);
     const previousSpecVersion = this.previousSpecVersions.get(networkName) ?? 0;
 
     if (currentSpecVersion !== previousSpecVersion) {
       logger.current.info(`Spec version changed for ${networkName}, refreshing the api...`);
-      return await this.populateApi(networkName);
+      return await this.refreshApi(networkName);
     }
 
     return apiInstance;
@@ -235,10 +269,12 @@ export class ApiManager {
         );
 
         try {
-          await this.populateApi(networkName);
+          const refreshedApiInstance = await this.refreshApi(networkName);
+          // Rebuild the extrinsic against the refreshed api; the original call is bound to the stale registry.
+          const retryCall = createCall(refreshedApiInstance.api);
           const nonce = await this.getNonce(senderKeypair, networkName);
           return new Promise((resolve, reject) => {
-            call.signAndSend(senderKeypair, { nonce }, (submissionResult: ISubmittableResult) => {
+            retryCall.signAndSend(senderKeypair, { nonce }, (submissionResult: ISubmittableResult) => {
               const { status, dispatchError } = submissionResult;
 
               if (dispatchError) {