fix: create a fresh default polling strategy per request (#1149)

ilbertt · web-flow · commit 5835985ff0c2 · 2025-09-18T14:43:17.000+02:00
Setting the `defaultStrategy()` on the `DEFAULT_POLLING_OPTIONS` statically results in calling the `timeout` strategy in the chain and starting the timeout. This makes all the read state requests after update calls fail after the configured `FIVE_MINUTES_IN_MSEC`, no matter whether the polling already started or not. This PR fixes the bug by removing the `defaultStrategy()` on the `DEFAULT_POLLING_OPTIONS` and setting it in the `pollForResponse` call. Ideally, the strategy property in the `pollingOptions` should be a function that returns a `PollStrategy`, but that was unfortunately changed in 880f18e#diff-8e88073ceeb119a7b39f3e6616db4c23f5d3c0e923c194f1f0e5a1a6d54f5835L99. It would require us to make a breaking change, which is not viable at the moment. Additionally, removes the `PollStrategyFactory` type, which was not used anywhere.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,8 @@
 
 ## [Unreleased]
 
+- fix(agent): create a fresh default polling strategy per request.
+- fix(agent): remove the unused `PollStrategyFactory` type.
 - fix(agent): remove the `nonce` from the `ActorConfig` type. This field must be used through the `CallConfig` type instead.
 
 ## [4.0.3] - 2025-09-16
diff --git a/packages/agent/src/actor-polling-options.test.ts b/packages/agent/src/actor-polling-options.test.ts
@@ -0,0 +1,135 @@
+import { IDL } from '@dfinity/candid';
+import { Principal } from '@dfinity/principal';
+import { type Agent } from './agent/api.ts';
+import { RequestId } from './request_id.ts';
+import { type LookupPathStatus, type LookupPathResultFound } from './certificate.ts';
+
+// Track strategy creations and invocations
+const instantiatedStrategies: jest.Mock[] = [];
+jest.mock('./polling/strategy.ts', () => {
+  return {
+    defaultStrategy: jest.fn(() => {
+      const fn = jest.fn(async () => {
+        // no-op strategy used in tests
+      });
+      instantiatedStrategies.push(fn);
+      return fn;
+    }),
+  };
+});
+
+const textEncoder = new TextEncoder();
+const textDecoder = new TextDecoder();
+
+const statusesByRequestId = new Map<RequestId, string[]>();
+const replyByRequestId = new Map<RequestId, Uint8Array>();
+
+jest.mock('./certificate.ts', () => {
+  return {
+    lookupResultToBuffer: (res: { value: Uint8Array }) => res?.value,
+    Certificate: {
+      create: jest.fn(async () => {
+        return {
+          lookup_path: (path: [string, RequestId, string]): LookupPathResultFound => {
+            // Path shape: ['request_status', requestIdBytes, 'status'|'reject_code'|'reject_message'|'error_code'|'reply']
+            const requestIdBytes = path[1];
+            const lastPathElement = path[path.length - 1] as string | Uint8Array;
+            const lastPathElementStr =
+              typeof lastPathElement === 'string'
+                ? lastPathElement
+                : textDecoder.decode(lastPathElement);
+
+            if (lastPathElementStr === 'status') {
+              const q = statusesByRequestId.get(requestIdBytes) ?? [];
+              const current = q.length > 0 ? q.shift()! : 'replied';
+              statusesByRequestId.set(requestIdBytes, q);
+              return {
+                status: 'Found' as LookupPathStatus.Found,
+                value: textEncoder.encode(current),
+              };
+            }
+            if (lastPathElementStr === 'reply') {
+              return {
+                status: 'Found' as LookupPathStatus.Found,
+                value: replyByRequestId.get(requestIdBytes)!,
+              };
+            }
+            throw new Error(`Unexpected lastPathElementStr ${lastPathElementStr}`);
+          },
+        } as const;
+      }),
+    },
+  };
+});
+
+describe('Actor default polling options are not reused across calls', () => {
+  beforeEach(() => {
+    instantiatedStrategies.length = 0;
+    statusesByRequestId.clear();
+    replyByRequestId.clear();
+    jest.resetModules();
+  });
+
+  it('instantiates a fresh defaultStrategy per update call when using DEFAULT_POLLING_OPTIONS', async () => {
+    const { Actor } = await import('./actor.ts');
+    const defaultStrategy = (await import('./polling/strategy.ts')).defaultStrategy as jest.Mock;
+
+    const canisterId = Principal.anonymous();
+
+    const requestIdA = new Uint8Array([1, 2, 3]) as RequestId;
+    const requestIdB = new Uint8Array([4, 5, 6]) as RequestId;
+    statusesByRequestId.set(requestIdA, ['processing', 'replied']);
+    statusesByRequestId.set(requestIdB, ['unknown', 'replied']);
+
+    const expectedReplyArgA = IDL.encode([IDL.Text], ['okA']);
+    const expectedReplyArgB = IDL.encode([IDL.Text], ['okB']);
+    replyByRequestId.set(requestIdA, expectedReplyArgA);
+    replyByRequestId.set(requestIdB, expectedReplyArgB);
+
+    // Fake Agent that forces polling (202) and provides readState
+    let callCount = 0;
+    const fakeAgent = {
+      rootKey: new Uint8Array([1]),
+      call: async () => {
+        const requestId = callCount === 0 ? requestIdA : requestIdB;
+        callCount += 1;
+        return {
+          requestId,
+          response: { status: 202 },
+          reply: replyByRequestId.get(requestId)!,
+          requestDetails: {},
+        } as unknown as {
+          requestId: Uint8Array;
+          response: { status: number };
+          requestDetails: object;
+        };
+      },
+      readState: async () => ({ certificate: new Uint8Array([0]) }),
+    } as unknown as Agent;
+
+    // Simple update method to trigger poll
+    const actorInterface = () =>
+      IDL.Service({
+        upd: IDL.Func([IDL.Text], [IDL.Text]),
+      });
+
+    const actor = Actor.createActor(actorInterface, {
+      canisterId,
+      // Critically, no pollingOptions override; Actor uses DEFAULT_POLLING_OPTIONS
+      // which must not carry a pre-instantiated strategy
+      agent: fakeAgent,
+    });
+
+    const outA = await actor.upd('x');
+    const outB = await actor.upd('y');
+    expect(outA).toBe('okA');
+    expect(outB).toBe('okB');
+
+    // defaultStrategy should have been created once per call, not shared
+    expect(defaultStrategy.mock.calls.length).toBe(2);
+    // Each created strategy used at least once
+    expect(instantiatedStrategies.length).toBe(2);
+    expect(instantiatedStrategies[0].mock.calls.length).toBe(1);
+    expect(instantiatedStrategies[1].mock.calls.length).toBe(1);
+  });
+});
diff --git a/packages/agent/src/polling/index.test.ts b/packages/agent/src/polling/index.test.ts
@@ -0,0 +1,115 @@
+import { Principal } from '@dfinity/principal';
+import { type Agent } from '../agent/api.ts';
+import { type RequestId } from '../request_id.ts';
+import { type LookupPathResultFound, type LookupPathStatus } from '../certificate.ts';
+
+// Mock the strategy module to observe instantiation and usage
+const instantiatedStrategies: jest.Mock[] = [];
+jest.mock('./strategy.ts', () => {
+  return {
+    // Each call should create a fresh, stateful strategy function
+    defaultStrategy: jest.fn(() => {
+      const strategyFn = jest.fn(async () => {
+        // no-op strategy used in tests
+      });
+      instantiatedStrategies.push(strategyFn);
+      return strategyFn;
+    }),
+  };
+});
+
+const textEncoder = new TextEncoder();
+const textDecoder = new TextDecoder();
+
+// Map a requestId key to a queue of statuses to emit across polls
+const statusesByRequestKey = new Map<RequestId, string[]>();
+const replyByRequestKey = new Map<RequestId, Uint8Array>();
+
+const mockAgent = {
+  rootKey: new Uint8Array([1]),
+  readState: async () => ({ certificate: new Uint8Array([0]) }),
+} as unknown as Agent;
+
+jest.mock('../certificate.ts', () => {
+  return {
+    // Simplified adapter used by polling/index.ts
+    lookupResultToBuffer: (res: LookupPathResultFound) => res.value,
+    Certificate: {
+      create: jest.fn(async () => {
+        return {
+          lookup_path: (path: [string, RequestId, string]): LookupPathResultFound => {
+            // Path shape: ['request_status', requestIdBytes, 'status'|'reject_code'|'reject_message'|'error_code'|'reply']
+            const requestIdBytes = path[1];
+            const lastPathElement = path[path.length - 1] as string | Uint8Array;
+            const lastPathElementStr =
+              typeof lastPathElement === 'string'
+                ? lastPathElement
+                : textDecoder.decode(lastPathElement);
+
+            if (lastPathElementStr === 'status') {
+              const q = statusesByRequestKey.get(requestIdBytes) ?? [];
+              const current = q.length > 0 ? q.shift()! : 'replied';
+              statusesByRequestKey.set(requestIdBytes, q);
+              return {
+                status: 'Found' as LookupPathStatus.Found,
+                value: textEncoder.encode(current),
+              };
+            }
+            if (lastPathElementStr === 'reply') {
+              return {
+                status: 'Found' as LookupPathStatus.Found,
+                value: replyByRequestKey.get(requestIdBytes)!,
+              };
+            }
+            throw new Error(`Unexpected lastPathElementStr ${lastPathElementStr}`);
+          },
+        } as const;
+      }),
+    },
+  };
+});
+
+describe('pollForResponse strategy lifecycle', () => {
+  beforeEach(() => {
+    instantiatedStrategies.length = 0;
+    statusesByRequestKey.clear();
+    replyByRequestKey.clear();
+    jest.resetModules();
+  });
+
+  it('creates a fresh default strategy per request and reuses it across retries', async () => {
+    // We need to import the module here to make sure the mock is applied
+    const { pollForResponse, defaultStrategy } = await import('./index.ts');
+
+    const canisterId = Principal.anonymous();
+
+    // Request A: simulate three polls: processing -> unknown -> replied
+    const requestIdA = new Uint8Array([1, 2, 3]) as RequestId;
+    statusesByRequestKey.set(requestIdA, ['processing', 'unknown', 'replied']);
+    replyByRequestKey.set(requestIdA, new Uint8Array([42]));
+
+    // Request B: simulate two polls: unknown -> replied
+    const requestIdB = new Uint8Array([9, 8, 7]) as RequestId;
+    statusesByRequestKey.set(requestIdB, ['unknown', 'replied']);
+    replyByRequestKey.set(requestIdB, new Uint8Array([99]));
+
+    // First call
+    const responseA = await pollForResponse(mockAgent, canisterId, requestIdA);
+    expect(responseA.reply).toEqual(new Uint8Array([42]));
+
+    // Second independent call
+    const responseB = await pollForResponse(mockAgent, canisterId, requestIdB);
+    expect(responseB.reply).toEqual(new Uint8Array([99]));
+
+    // Assert that defaultStrategy has been instantiated once per request (not per retry)
+    const defaultStrategyMock = defaultStrategy as jest.Mock;
+    expect(defaultStrategyMock.mock.calls.length).toBe(2);
+
+    // And that each created strategy function was invoked during its own request
+    expect(instantiatedStrategies.length).toBe(2);
+    // Request A had two non-replied statuses, so strategy called at least twice
+    expect(instantiatedStrategies[0].mock.calls.length).toBe(2);
+    // Request B had one non-replied status (unknown), so strategy called once
+    expect(instantiatedStrategies[1].mock.calls.length).toBe(1);
+  });
+});
diff --git a/packages/agent/src/polling/index.ts b/packages/agent/src/polling/index.ts
@@ -31,8 +31,6 @@ export type PollStrategy = (
   status: RequestStatusResponseStatus,
 ) => Promise<void>;
 
-export type PollStrategyFactory = () => PollStrategy;
-
 interface SignedReadStateRequestWithExpiry extends ReadStateRequest {
   body: {
     content: Pick<ReadStateRequest, 'request_type' | 'ingress_expiry'>;
@@ -46,7 +44,7 @@ export interface PollingOptions {
   /**
    * A polling strategy that dictates how much and often we should poll the
    * read_state endpoint to get the result of an update call.
-   * @default defaultStrategy()
+   * @default {@link defaultStrategy}
    */
   strategy?: PollStrategy;
 
@@ -69,7 +67,6 @@ export interface PollingOptions {
 }
 
 export const DEFAULT_POLLING_OPTIONS: PollingOptions = {
-  strategy: defaultStrategy(),
   preSignReadStateRequest: false,
 };
 
@@ -187,8 +184,11 @@ export async function pollForResponse(
       // Execute the polling strategy, then retry.
       const strategy = options.strategy ?? defaultStrategy();
       await strategy(canisterId, requestId, status);
+
       return pollForResponse(agent, canisterId, requestId, {
         ...options,
+        // Pass over either the strategy already provided or the new one created above
+        strategy,
         request: currentRequest,
       });
     }
diff --git a/packages/agent/src/polling/strategy.ts b/packages/agent/src/polling/strategy.ts
@@ -15,6 +15,9 @@ const FIVE_MINUTES_IN_MSEC = 5 * 60 * 1000;
 /**
  * A best practices polling strategy: wait 2 seconds before the first poll, then 1 second
  * with an exponential backoff factor of 1.2. Timeout after 5 minutes.
+ *
+ * Note that calling this function will create the strategy chain described above and already start the 5 minutes timeout.
+ * You should only call this function when you want to start the polling, and not before, to avoid exhausting the 5 minutes timeout in advance.
  */
 export function defaultStrategy(): PollStrategy {
   return chain(conditionalDelay(once(), 1000), backoff(1000, 1.2), timeout(FIVE_MINUTES_IN_MSEC));
