Skip to content

resolveCloakBrowserVersion() re-resolves and re-reads cloakbrowser/package.json on every status call, once per profile` #211

Description

@rajarshidattapy

Description

resolveCloakBrowserVersion() does a module resolution, a synchronous file read, and a JSON parse on every invocation, with no caching:

// src/browser/runtime/local-cloak/session-manager.ts:14
export function resolveCloakBrowserVersion(): string | undefined {
  try {
    const entryPath = fileURLToPath(import.meta.resolve('cloakbrowser'));
    const pkg = JSON.parse(fs.readFileSync(path.join(findPackageRoot(entryPath), 'package.json'), 'utf-8'));
    return typeof pkg.version === 'string' ? pkg.version : undefined;
  } catch {
    return undefined;
  }
}

The value is the installed npm package version. It cannot change while the process is alive.

Impact

It is called more often than it looks. status() calls it directly, and profileStatuses() calls it inside the .map(), so once per profile:

// src/browser/runtime/local-cloak/provider.ts:24
runtimeVersion: resolveCloakBrowserVersion(),

// src/browser/runtime/local-cloak/session-manager.ts:126 — inside [...this.profiles.entries()].map(...)
runtimeVersion: resolveCloakBrowserVersion(),

One status() call with N active profiles performs N+1 resolve-read-parse cycles, all returning the same string. webcmd doctor and every runtime status poll pay this. In the daemon, where status is polled repeatedly, it is pure repeated synchronous I/O on the event loop.

The previous implementation cached this per runtime; the caching was dropped somewhere in the local-cloak refactor.

Suggested fix

Memoize in a module-level variable. The one wrinkle: undefined is a legitimate result (the catch path, when cloakbrowser is not resolvable), so a plain if (cached !== undefined) guard would retry the failing path on every call and preserve the bug in exactly the situation where the file reads are most likely to be slow. Use a sentinel or a separate resolved flag:

const UNRESOLVED = Symbol('unresolved');
let cachedVersion: string | undefined | typeof UNRESOLVED = UNRESOLVED;

export function resolveCloakBrowserVersion(): string | undefined {
  if (cachedVersion !== UNRESOLVED) return cachedVersion;
  try {
    const entryPath = fileURLToPath(import.meta.resolve('cloakbrowser'));
    const pkg = JSON.parse(fs.readFileSync(path.join(findPackageRoot(entryPath), 'package.json'), 'utf-8'));
    cachedVersion = typeof pkg.version === 'string' ? pkg.version : undefined;
  } catch {
    cachedVersion = undefined;
  }
  return cachedVersion;
}

If any test needs to observe a changed version, expose a small reset used only by tests rather than leaving the function uncached.

Verification

Checked against main (53310bf) and upstream/main; the function and both call sites are identical on each. resolveCloakBrowserVersion has exactly two non-test callers, provider.ts:24 and session-manager.ts:126.


Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions