Skip to content

Conversation

@josejefferson
Copy link
Contributor

Fix #417

@Annhiluc Annhiluc requested review from Annhiluc and hsubox76 March 24, 2025 23:31
@Annhiluc Annhiluc requested review from MarkDaoust and removed request for Annhiluc March 25, 2025 16:43
finalResult = result;
});
await this._sendPromise;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean something like this?

await this._sendPromise.catch((error) => {
  this._sendPromise = Promise.resolve();
  throw error;
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes something like that. Could you check to see if this also works?

.then((result) => {
  // ...
  finalResult = result;
})
.catch((e) => {
  // Resets _sendPromise to avoid subsequent calls failing and throw error.
  this._sendPromise = Promise.resolve();
  throw e;
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it works! I've already made the change.

@josejefferson josejefferson requested a review from Annhiluc March 26, 2025 00:06
finalResult = result;
});
await this._sendPromise;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes something like that. Could you check to see if this also works?

.then((result) => {
  // ...
  finalResult = result;
})
.catch((e) => {
  // Resets _sendPromise to avoid subsequent calls failing and throw error.
  this._sendPromise = Promise.resolve();
  throw e;
});

@josejefferson josejefferson requested a review from Annhiluc March 26, 2025 12:01
Copy link
Contributor

@Annhiluc Annhiluc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for making the changes! Could you also help add a unit test for this case in src/methods/chat-session.test.ts?

Something like:

it("generateContent errors should not persist", async () => {
      const generateContentStub = stub(
        generateContentMethods,
        "generateContent",
      ).rejects("generateContent failed");
      const chatSession = new ChatSession("MY_API_KEY", "a-model");
      await expect(chatSession.sendMessage("hello")).to.be.rejected;
      expect(generateContentStub).to.be.calledWith(
        "MY_API_KEY",
        "a-model",
        match.any,
      );

      // Subsequent call should succeed.
      const UpdatedGenerateContentStub = stub(
        generateContentMethods,
        "generateContent",
      );
      await expect(chatSession.sendMessage("hello")).to.not.be.rejected;
      expect(UpdatedGenerateContentStub).to.be.calledWith(
        "MY_API_KEY",
        "a-model",
        match.any,
      );
    });

@josejefferson
Copy link
Contributor Author

Sure! I've added a unit test for this case in src/methods/chat-session.test.ts. Let me know if you need any adjustments or additional test cases. Thanks for your help!

@josejefferson josejefferson requested a review from Annhiluc March 26, 2025 23:48
@Annhiluc Annhiluc merged commit 58f208d into google-gemini:main Mar 26, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

After an API error, all subsequent calls fail with the same error

2 participants