fix(node-core): harden dictionary v1 responses and abort timed-out queries

packages/node-core/CHANGELOG.md

@@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased]
 ### Fixed
 - Dictionary v1 queries kept every parsed query document in memory: each batch's query text is new, and both `graphql-tag`'s cache and Apollo Client's `QueryManager.transformCache` retained it, so a long sync's memory grew without bound. Queries are now posted as plain GraphQL over HTTP and nothing is parsed or cached (#3048)
+- Dictionary v1 queries threw a `TypeError` on a `null` body, `{data: null}` or `errors` that isn't an array, and their errors dropped the response body, so the `distinct` fallback missed non-standard error responses. The error now keeps the body and includes it in its message (#3051)
+- A timed-out dictionary v1 query kept running in the background with its connection and response body. It is now aborted a second after the query times out (#3051)
 
 ### Changed
 - `DictionaryV1` sends queries with a protected `query()` method instead of an Apollo `client`, and `@apollo/client` is no longer a dependency (#3048)

packages/node-core/src/indexer/dictionary/v1/dictionaryV1.spec.ts

@@ -342,11 +342,23 @@ describe('Dictionary V1 queries', () => {
     fetchSpy = jest.spyOn(global, 'fetch').mockResolvedValue(new Response('<html>Forbidden</html>', {status: 403}));
 
     await expect(queryableDictionary().runQuery('query{_metadata{lastProcessedHeight}}')).rejects.toThrow(
-      'Dictionary returned HTTP 403 without a JSON body'
+      'Dictionary returned HTTP 403 without a JSON body: <html>Forbidden</html>'
     );
   });
 
-  it('retries a query without distinct when the dictionary does not support it', async () => {
+  it.each([null, {data: null}, {data: []}])('throws when the dictionary answers %j', async (body) => {
+    fetchSpy = jest.spyOn(global, 'fetch').mockResolvedValue(jsonResponse(body));
+
+    await expect(queryableDictionary().runQuery('query{_metadata{lastProcessedHeight}}')).rejects.toThrow(
+      'Dictionary returned HTTP 200 without data'
+    );
+  });
+
+  it.each<[string, () => Response]>([
+    ['GraphQL errors', () => jsonResponse({errors: distinctUnsupported}, 400)],
+    ['errors that are not an array', () => jsonResponse({errors: distinctUnsupported[0].message}, 400)],
+    ['a plain-text body', () => new Response(distinctUnsupported[0].message, {status: 400})],
+  ])('retries a query without distinct when the dictionary reports it unsupported in %s', async (_, unsupported) => {
     const queries: string[] = [];
     fetchSpy = jest.spyOn(global, 'fetch').mockImplementation((_url, init) => {
       const {query} = JSON.parse(String(init?.body)) as {query: string};
@@ -355,7 +367,7 @@ describe('Dictionary V1 queries', () => {
         return Promise.resolve(jsonResponse({data: {_metadata: metadata}}));
       }
       if (query.includes('distinct')) {
-        return Promise.resolve(jsonResponse({errors: distinctUnsupported}, 400));
+        return Promise.resolve(unsupported());
       }
       return Promise.resolve(
         jsonResponse({data: {_metadata: metadata, events: {nodes: [{blockHeight: '150'}, {blockHeight: '170'}]}}})
@@ -369,4 +381,34 @@ describe('Dictionary V1 queries', () => {
     const batchQueries = queries.filter((query) => query.includes('events'));
     expect(batchQueries.map((query) => query.includes('distinct'))).toEqual([true, false]);
   });
+
+  it('aborts a request after callers have timed out on it', async () => {
+    let signal: AbortSignal | undefined;
+    fetchSpy = jest.spyOn(global, 'fetch').mockImplementation(
+      (_url, init) =>
+        new Promise<Response>((_resolve, reject) => {
+          signal = init?.signal ?? undefined;
+          signal?.addEventListener('abort', () => reject(new Error('aborted')));
+        })
+    );
+    const dictionary = new QueryableDictionaryV1(
+      DICTIONARY_ENDPOINT,
+      DICTIONARY_CHAINID,
+      new NodeConfig({
+        subquery: 'asdf',
+        subqueryName: 'asdf',
+        networkEndpoint: {'wss://polkadot.api.onfinality.io/public-ws': {}},
+        dictionaryTimeout: 0.1,
+      }),
+      HAPPY_PATH_CONDITIONS
+    );
+
+    // Callers still get `timeout()`'s error, before the request is aborted
+    await expect((dictionary as any).init()).rejects.toThrow('Dictionary metadata query timeout in 0.1 seconds');
+    assert(signal);
+    expect(signal.aborted).toBe(false);
+
+    await new Promise((resolve) => signal?.addEventListener('abort', resolve));
+    expect(signal.reason).toMatchObject({name: 'TimeoutError'});
+  });
 });

packages/node-core/src/indexer/dictionary/v1/dictionaryV1.ts

@@ -19,14 +19,16 @@ type GraphQLError = {message: string; [key: string]: unknown};
 type DictionaryV1BatchResult = {_metadata: DictionaryV1Metadata} & Record<string, {nodes: {blockHeight: string}[]}>;
 
 /**
- * A dictionary query that failed. `graphQLErrors` holds the errors the dictionary returned, if any,
- * which callers inspect to tell an unsupported argument from a failed request.
+ * A dictionary query that failed. `graphQLErrors` holds the errors the dictionary returned, if any, and `body` the
+ * response body when it isn't a GraphQL error response. Callers inspect both to tell an unsupported argument from a
+ * failed request.
  */
 export class DictionaryQueryError extends Error {
   constructor(
     message: string,
     readonly status: number,
-    readonly graphQLErrors: GraphQLError[] = []
+    readonly graphQLErrors: GraphQLError[] = [],
+    readonly body?: unknown
   ) {
     super(message);
     this.name = 'DictionaryQueryError';
@@ -63,30 +65,43 @@ export abstract class DictionaryV1<DS> extends CoreDictionary<
       method: 'POST',
       headers: {'content-type': 'application/json', accept: 'application/json'},
       body: JSON.stringify({query, variables}),
+      // Callers give up on a query through `timeout()`, which doesn't cancel it. Abort it a second later so it doesn't
+      // keep its connection and body, while callers still get `timeout()`'s error.
+      signal: AbortSignal.timeout((this.nodeConfig.dictionaryTimeout + 1) * 1000),
     });
 
-    let body: {data?: T; errors?: GraphQLError[]};
+    const text = await response.text();
+    let body: {data?: unknown; errors?: unknown} | null;
     try {
-      body = (await response.json()) as typeof body;
+      body = JSON.parse(text);
     } catch {
       throw new DictionaryQueryError(
-        `Dictionary returned HTTP ${response.status} without a JSON body`,
-        response.status
+        `Dictionary returned HTTP ${response.status} without a JSON body: ${text.slice(0, 500)}`,
+        response.status,
+        [],
+        text
       );
     }
 
-    if (body.errors?.length) {
+    const errors = body?.errors;
+    if (Array.isArray(errors) && errors.length) {
       throw new DictionaryQueryError(
-        `Dictionary query failed: ${body.errors.map((e) => e.message).join('; ')}`,
+        `Dictionary query failed: ${errors.map((e) => e?.message).join('; ')}`,
         response.status,
-        body.errors
+        errors
       );
     }
-    if (!response.ok || body.data === undefined) {
-      throw new DictionaryQueryError(`Dictionary returned HTTP ${response.status} without data`, response.status);
+    const data = body?.data;
+    if (!response.ok || typeof data !== 'object' || data === null || Array.isArray(data)) {
+      throw new DictionaryQueryError(
+        `Dictionary returned HTTP ${response.status} without data: ${text.slice(0, 500)}`,
+        response.status,
+        [],
+        body
+      );
     }
 
-    return body.data;
+    return data as T;
   }
 
   protected async init(): Promise<void> {