fix: don't double increment session usage count in BrowserCrawler

[barjin]

Changes to the session handling in #1709 caused the same session in BrowserCrawler to be marked as good with two different calls. This removes the additional call (keeping only the one in BasicCrawler), aligning the BrowserCrawler behavior with the other crawler instances.

Closes #2851

[Copilot]

Pull Request Overview

This PR resolves an issue where sessions in BrowserCrawler were being marked as good twice, leading to a double increment of the session usage count. The changes remove the duplicate call to session.markGood in BrowserCrawler and add a new test to verify the correct handling.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
test/core/crawlers/browser_crawler.test.ts	Added a test to verify that the session usage count increments correctly.
packages/browser-crawler/src/internals/browser-crawler.ts	Removed the duplicate call to session.markGood to prevent double increment.

[barjin]

Since usageCount is private, should we even test for this? This whole is rather a code quality nit to me than a huge fix with user impact.

[janbuchar]

Testing private stuff does not make much sense. However, maybe we could add a read only getter for that property - it might be helpful for introspection and what not

[barjin]

The latest commit adds public getters for the Session fields that appear in the Session.getState() anyway.

While I like that we got rid of many ts-ignore-errors in the tests (indicative of the fact that these fields shouldn't have been private), I'm not completely sold on the code quality (e.g. underscoring some private fields - those with the getters - while keeping the rest as is). To a naked eye, it looks quite arbitrary and IMO bit messy.

Do you think this is acceptable way to achieve this, or you have a better pattern for those in mind?

[B4nan]

I got a feeling there was a reason for this duplicity, but no idea what exactly 🙃 Have you checked the git history when it was added?

[barjin]

when it was added?

It's been there since the advent of BrowserCrawler since 2020.

The reason was that originally BasicCrawler and BrowserCrawler were touching different session instances:

BasicCrawler.runHandler(context) {
  const session = newSession();
  const context = { ... session ...  };
  await BrowserCrawler.runHandler(context) {
     ...
     context.session = newSession();
     ...
     context.session.markGood();
  }

  session.markGood();
}

Note that the session in the BasicCrawler handler was not referencing the same object as context.session at the end of the function.

I fixed this (made all BasicCrawler references to session explicitly into context.session in #1709). Back then I missed the rogue markGood call in BrowserCrawler. This PR fixes this omission.

[janbuchar]

Nice detective work @barjin!