fix(cache): timezone-independent expiry — parse zone-less UTC timestamps as UTC (#208)

[josephkehan-prog]

Closes #208.

What & why

toIsoSeconds() persists timestamps as zone-less UTC ("YYYY-MM-DD HH:MM:SS", matching SQLite's datetime('now')), but the three expiry read sites parsed them back with new Date(str), which treats that form as local time. Every TTL comparison shifted by the host's UTC offset — west-of-UTC hosts serve expired/stale rows as fresh (and mode: fast never marks stale: true inside the shifted window); east-of-UTC hosts expire early. CI never caught it because runners are UTC; on a UTC-4 machine, current main fails the fetch-mode stale-window unit tests out of the box.

Changes

• src/cache/store.ts: new parseUtcTimestamp() next to toIsoSeconds() — re-attaches the UTC marker when the string matches the zone-less format, falls through unchanged otherwise (Z-suffixed/ISO rows keep working). Swapped the three new Date(...) call sites (isExpired, isCacheUsable, getCachedSearch). No schema or stored-format change; existing rows parse correctly immediately.
• tests/unit/cache/store.test.ts: regression suite that flips process.env.TZ to Etc/GMT+8 (UTC-8) at runtime and verifies expiry + stale-window behavior; each test self-skips if the runtime ignores the TZ change (Windows), so UTC CI runners still catch a reintroduction. Verified the suite fails 3/4 against the unfixed code.

Testing

• npx tsc --noEmit clean
• npm run lint passes
• Full unit suite: previously-failing fetch-mode stale-window tests now pass under a non-UTC local clock; also green under TZ=UTC
• New regression tests red on unfixed code, green on fixed

CLA per CONTRIBUTING.md: agreed.

Summary by CodeRabbit

• Bug Fixes

  • Fixed cache expiration checks to interpret UTC timestamps consistently across different local time zones.
  • Improved accuracy for recently expired, active, and stale cached search results.

• Tests

  • Added coverage for timezone-independent cache expiry and stale-result handling.

[coderabbitai]

📝 Walkthrough

Walkthrough

Changes

Cache expiry handling

Layer / File(s)	Summary
UTC timestamp parsing and cache checks src/cache/store.ts	Adds UTC parsing for persisted zone-less timestamps and applies it to cache expiry, usability, and search-result checks.
Timezone-independent expiry tests tests/unit/cache/store.test.ts	Adds UTC-8 coverage for expiration, stale-window acceptance, and stale-window rejection boundaries.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: knockoutez

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the cache expiry timezone fix and matches the main change.
Linked Issues check	✅ Passed	The changes address the timezone-dependent expiry bug, preserve existing formats, and add non-UTC regression coverage as required.
Out of Scope Changes check	✅ Passed	The patch stays focused on cache timestamp parsing and regression tests, with no obvious unrelated changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (2)

tests/unit/cache/store.test.ts (2)

728-733: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use context.skip() instead of a bare return for the unsupported-TZ-flip path.

When tzFlipTookEffect() returns false, these tests silently return and report as "passed" with no assertions, hiding the fact that coverage was skipped on that host. Vitest exposes context.skip(condition, note) (available since v3.1; project is on 4.1.4) which marks the test as skipped in the reporter instead.

♻️ Suggested change

-  it('isExpired stays true for a just-expired row under a UTC-8 clock', () => {
-    if (!tzFlipTookEffect()) return;
+  it('isExpired stays true for a just-expired row under a UTC-8 clock', (ctx) => {
+    ctx.skip(!tzFlipTookEffect(), 'runtime ignores TZ change on this host');
     expect(isExpired(makeCached(zonelessUtc(-60_000)))).toBe(true);
   });

Also applies to: 764-772, 774-788

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/cache/store.test.ts` around lines 728 - 733, Update the tests
using tzFlipTookEffect() so the unsupported timezone-flip path calls Vitest’s
context.skip() with a clear note instead of returning. Ensure each affected test
receives the test context parameter and marks itself skipped when the condition
is false, while preserving normal assertions when it succeeds.

---

722-789: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Consider covering getCachedSearchResults under the TZ flip too.

The PR objectives explicitly call out "search TTL calculations across host time zones," but this suite only exercises isExpired/isCacheUsable. getCachedSearchResults (store.ts Line 450) shares the same parseUtcTimestamp helper, so the risk is lower, but a dedicated regression test would close the gap and directly validate the search-cache path.

Suggested additional test

it('getCachedSearchResults returns stale results correctly under a UTC-8 clock', () => {
  if (!tzFlipTookEffect()) return;
  initDatabase(':memory:');
  cacheSearchResults('tz search', [], ['engine']);
  // manipulate stored expires_at to a zoneless past value, then assert staleness
  closeDatabase();
});

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/cache/store.test.ts` around lines 722 - 789, Extend the
timezone-independent expiry suite with a regression test for
getCachedSearchResults, using the existing tzFlipTookEffect helper and an
in-memory database. Seed search results via cacheSearchResults, set the stored
expires_at to a zoneless UTC value inside the stale window, then assert
getCachedSearchResults reports the results as stale; clean up the database
afterward with closeDatabase.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/unit/cache/store.test.ts`:
- Around line 728-733: Update the tests using tzFlipTookEffect() so the
unsupported timezone-flip path calls Vitest’s context.skip() with a clear note
instead of returning. Ensure each affected test receives the test context
parameter and marks itself skipped when the condition is false, while preserving
normal assertions when it succeeds.
- Around line 722-789: Extend the timezone-independent expiry suite with a
regression test for getCachedSearchResults, using the existing tzFlipTookEffect
helper and an in-memory database. Seed search results via cacheSearchResults,
set the stored expires_at to a zoneless UTC value inside the stale window, then
assert getCachedSearchResults reports the results as stale; clean up the
database afterward with closeDatabase.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 04096d33-fe18-4a1c-875e-e6504347ef2d

📥 Commits

Reviewing files that changed from the base of the PR and between 185afb5 and ff3372f.

📒 Files selected for processing (2)

• src/cache/store.ts
• tests/unit/cache/store.test.ts