Skip to content

Search should be aborted when reaching timeout #1922

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Strift opened this issue Apr 15, 2025 · 2 comments · May be fixed by #1926
Open

Search should be aborted when reaching timeout #1922

Strift opened this issue Apr 15, 2025 · 2 comments · May be fixed by #1926
Assignees
Labels
bug Something isn't working

Comments

@Strift
Copy link
Collaborator

Strift commented Apr 15, 2025

Description

The following test is failing:

 FAIL  tests/search.test.ts > Test on abortable search > Master key: search should be aborted when reaching timeout
AssertionError: expected value to not resolve

- Expected: 
Symbol(<not resolved>)

+ Received: 
{
  "status": "available",
}

 ❯ Function.rejects tests/utils/meilisearch-test-utils.ts:124:18
    122|     }
    123| 
    124|     vitestAssert.fail(
       |                  ^
    125|       resolvedValue,
    126|       NOT_RESOLVED,
 ❯ tests/search.test.ts:1439:19

Screenshots or Logs
https://github.com/meilisearch/meilisearch-js/actions/runs/14459420962/job/40549120917

Environment (please complete the following information):

  • Meilisearch version: v1.14.0
  • meilisearch-js version: v0.50
@Strift Strift added the bug Something isn't working label Apr 15, 2025
@flevi29 flevi29 self-assigned this Apr 15, 2025
@flevi29
Copy link
Collaborator

flevi29 commented Apr 19, 2025

I couldn't reproduce it with the following test:

import { describe, test } from "vitest";
import {
  MeiliSearch,
  MeiliSearchRequestTimeOutError,
  MeiliSearchRequestError,
} from "../src/index.js";
import { assert, HOST, MASTER_KEY } from "./utils/meilisearch-test-utils.js";

describe("timeout", () => {
  test.concurrent.for(Array.from({ length: 10000 }, (_, i) => i))(
    "with timeout >= 1",
    async () => {
      const timeout = 1;
      const ms = new MeiliSearch({ host: HOST, apiKey: MASTER_KEY, timeout });

      const { cause } = await assert.rejects(
        ms.health(),
        MeiliSearchRequestError,
      );
      assert.instanceOf(cause, MeiliSearchRequestTimeOutError);
      assert.strictEqual(cause.cause.timeout, timeout);
    },
  );
});
 Test Files  1 passed (1)
      Tests  10000 passed (10000)
   Start at  08:46:20
   Duration  10.73s (transform 175ms, setup 0ms, collect 504ms, tests 9.56s, environment 0ms, prepare 121ms)

I believe it's related to the event loop once many different things are happening on it. One millisecond in setTimeout doesn't guarantee one actual millisecond, quite far from it. This means that fetch can resolve sooner than the abort signal is dispatched.

There's nothing we can do here as long as fetch itself doesn't allow us to meddle with its intrinsic timeout mechanism.

If we want to guarantee that a timeout will successfully occur, we need to mock fetch and make it wait for the abort signal indefinitely.

@flevi29 flevi29 closed this as completed Apr 19, 2025
@flevi29
Copy link
Collaborator

flevi29 commented Apr 19, 2025

Actually, a PR should close this that implements the mocked fetch version of the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants