Skip to content

perf(cookies): parallelize browser strategy queries#491

Merged
mherod merged 1 commit intomainfrom
perf/parallelize-strategy-queries-485
Mar 15, 2026
Merged

perf(cookies): parallelize browser strategy queries#491
mherod merged 1 commit intomainfrom
perf/parallelize-strategy-queries-485

Conversation

@mherod
Copy link
Copy Markdown
Owner

@mherod mherod commented Mar 15, 2026

Summary

  • Replace sequential for...of await loop with Promise.allSettled in batchQueryCookies so Chrome, Firefox, and Safari are queried in parallel
  • Each strategy targets independent browser databases with zero data dependency
  • Error handling behavior unchanged (continueOnError flag respected)

Closes #485

Test plan

  • Type-check passes
  • Biome lint passes (225 files, 0 errors)
  • All 588 tests pass
  • Pre-commit and pre-push hooks pass

Replace sequential for-of loop with Promise.allSettled so
Chrome, Firefox, and Safari are queried in parallel. Each
strategy targets independent databases with zero data
dependency.

Fixes #485
Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

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

The parallelization change is correct and well-structured.

What the PR does well:

👍 Promise.allSettled is the right primitive — it guarantees all three browser strategies are always attempted, even if one fails early. Using Promise.all here would have short-circuited on the first failure when continueOnError = false.

👍 Error handling semantics are preserved — the continueOnError flag is threaded through both the inner queryStrategy calls and the new outer settlement loop.

👍 Independent I/O is the correct justification — Chrome, Firefox, and Safari each read from completely separate database files, so parallel access is safe and there are no shared resources or ordering constraints to worry about.

One minor observation worth noting (non-blocking):

🔵 "rejected" branch in the outer loop is a narrow path when continueOnError = falsequeryStrategy already catches internally and re-throws when continueOnError = false, so Promise.allSettled captures that throw as a rejected result. The outer else { throw result.reason } then re-throws it. This is functionally correct, but it means an uncaught exception in queryStrategy (e.g., from something outside the try/catch block like the strategy constructor throwing) would also land here. The current code handles that correctly, so this is purely an observation rather than a bug.

🔵 No direct unit tests for the parallel behavior in batchQueryCookies itself — existing tests mock out batchQueryCookies entirely. A test verifying that all three strategies are invoked concurrently (e.g., using spies to detect overlapping execution) would add confidence, but the existing integration path through batchGetCookies covers the happy path.

Overall the change is minimal, well-reasoned, and the test plan confirms all 588 tests pass. Ready to merge.

@mherod mherod merged commit 9f973f6 into main Mar 15, 2026
15 checks passed
@mherod mherod deleted the perf/parallelize-strategy-queries-485 branch March 15, 2026 17:46
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.

perf(cookies): parallelize browser strategy queries in batchQueryCookies

1 participant