fix: resolve activity tracker infinite timeout loops #158
Conversation
WalkthroughAdds try/catch wrappers and logging around pre-warning and timeout callbacks, refines proxy get behavior to avoid updating activity for Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App
participant Proxy as sessionManagerActivityProxy
participant Tracker as updateActivityTimestamp
participant Timer as Inactivity Timer
participant SessionMgr as Session Manager
User->>App: call session method
App->>Proxy: access property (get)
alt property is function and not "destroySession"
Proxy->>Tracker: updateActivityTimestamp()
else
note right of Proxy: no activity update
end
Timer->>Proxy: pre-warning trigger
rect #F8F9FF
Proxy->>Proxy: suppress activity updates
Proxy->>SessionMgr: invoke preWarning callback (try/catch)
note right of Proxy: console.error if callback throws
Proxy->>Proxy: clear suppress
end
Timer->>Proxy: timeout trigger
rect #FFF8F8
Proxy->>Proxy: suppress activity updates
Proxy->>SessionMgr: destroySession(active)
Proxy->>SessionMgr: destroySession(insecure if different)
Proxy->>Proxy: invoke onActivityTimeout (try/catch)
note right of Proxy: console.error if callback throws
Proxy->>Proxy: clear suppress
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
lib/utils/activityTracking.ts (4)
12-14: Global suppression flag can unintentionally affect unrelated activity flows; consider scoping or ref-counting.A single module-level boolean suppresses updates across all proxies/sessions. If multiple storages are live, a timeout in one will pause activity tracking globally. Prefer a small ref-count or helper to allow nested suppression and future per-instance scoping.
Example minimal change:
-let suppressActivityUpdate = false; +let suppressActivityUpdateCount = 0; +const withSuppressedActivity = async <T>(fn: () => Promise<T> | T): Promise<T> => { + suppressActivityUpdateCount++; + try { + return await fn(); + } finally { + suppressActivityUpdateCount--; + } +};And use it in the timeout handler (see lines 45-68).
61-66: Harden callback error handling message and surface type (preWarning lacks same guard).The try/catch is good. Consider mirroring this for preWarning (lines 73-80) and clarifying the log to include the timeout type for easier triage.
- console.error("onActivityTimeout callback threw:", err); + console.error("[activityTimeout] onActivityTimeout(timeout) threw:", err);
73-80: Wrap pre-warning callback in try/catch (and optionally suppress activity during callback).Pre-warning handlers can indirectly touch storage and re-arm timers. Guarding them the same way avoids accidental resets and aligns error handling with timeout.
- activityPreWarnTimer = setTimeout( - () => { - storageSettings.onActivityTimeout?.(TimeoutActivityType.preWarning); - }, - storageSettings.activityTimeoutPreWarningMinutes! * 60 * 1000, - ); + activityPreWarnTimer = setTimeout(() => { + try { + // Optional: suppress to avoid re-arming timers from within the callback. + // If you implement ref-counting per earlier comment, replace with ++/--. + suppressActivityUpdate = true; + storageSettings.onActivityTimeout?.(TimeoutActivityType.preWarning); + } catch (err) { + console.error("[activityTimeout] onActivityTimeout(preWarning) threw:", err); + } finally { + suppressActivityUpdate = false; + } + }, storageSettings.activityTimeoutPreWarningMinutes! * 60 * 1000);
112-121: Avoid resetting timers on every property read; only treat method invocations as “activity”.Calling updateActivityTimestamp for any property access (including non-method reads) is surprising and can cause unnecessary churn. Move the update call after value lookup and gate on functions; also skip destroySession to avoid re-arming timers in logout flows if suppression ever fails.
- get(target: SessionManager<T>, prop: string | symbol) { - if (!suppressActivityUpdate) { - updateActivityTimestamp(); - } - const value = target[prop as keyof SessionManager<T>]; - if (typeof value === "function") { - return value.bind(target); - } - return value; - }, + get(target: SessionManager<T>, prop: string | symbol) { + const value = target[prop as keyof SessionManager<T>]; + if (typeof value === "function") { + if (!suppressActivityUpdate && prop !== "destroySession") { + updateActivityTimestamp(); + } + return (value as Function).bind(target); + } + return value; + },lib/utils/activityTracking.test.ts (1)
332-379: Solid regression test covering the “single preWarning + single timeout per cycle” guarantee.Captures the original bug and prevents reintroduction. Two small polish ideas:
- Use advanceTimersByTimeAsync for the pre-warning step for consistency with async handlers.
- Replace magic milliseconds with derived expressions to mirror config.
-vi.advanceTimersByTime(3 * 1000 + 100); +await vi.advanceTimersByTimeAsync(storageSettings.activityTimeoutPreWarningMinutes! * 60 * 1000 + 100);Optionally, add a test where onActivityTimeout throws to ensure it’s swallowed and doesn’t schedule additional timers.
I can add that negative test if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
lib/utils/activityTracking.test.ts(2 hunks)lib/utils/activityTracking.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/utils/activityTracking.test.ts (2)
lib/utils/token/index.ts (2)
setActiveStorage(39-45)getActiveStorage(51-53)lib/sessionManager/index.ts (1)
storageSettings(3-42)
lib/utils/activityTracking.ts (2)
lib/utils/token/index.ts (1)
getInsecureStorage(87-89)lib/sessionManager/index.ts (1)
storageSettings(3-42)
🔇 Additional comments (2)
lib/utils/activityTracking.ts (1)
45-68: Good re-entrancy guard placement; ordering prevents proxy re-trigger during destroy.Setting suppression before accessing destroySession on the (proxied) manager prevents recursive timer setup. The try/finally restores state even on errors. LGTM.
lib/utils/activityTracking.test.ts (1)
181-184: Nice: asserts proxy creation fails fast when session is missing.The cast is acceptable for simulating runtime misuse; the expectation matches the thrown message.
@Koosha-Owji thanks for this. Could you take a look at the nitpick comments |
|
Most of these suggestions weren't needed because the root issue was specifically destroySession causing infinite loops during cleanup. Instead of ref-counting, I just excluded destroySession from triggering activity updates. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/utils/activityTracking.ts (1)
68-82: Bug: pre-warning timer not scheduled when set to 0The truthy check skips scheduling when
activityTimeoutPreWarningMinutesis0. Use an explicit!== undefinedcheck.- if (storageSettings.activityTimeoutPreWarningMinutes) { + if (storageSettings.activityTimeoutPreWarningMinutes !== undefined) {lib/utils/activityTracking.test.ts (1)
216-225: Fix matcher misuse with Promises
toThrowexpects a function, not a Promise. Assert resolution instead.- await expect( - activeStorage.setSessionItem(StorageKeys.accessToken, "token"), - ).resolves.not.toThrow(); + await expect( + activeStorage.setSessionItem(StorageKeys.accessToken, "token"), + ).resolves.toBeUndefined(); @@ - await expect( - activeStorage.removeSessionItem(StorageKeys.accessToken), - ).resolves.not.toThrow(); + await expect( + activeStorage.removeSessionItem(StorageKeys.accessToken), + ).resolves.toBeUndefined(); @@ - await expect(activeStorage.destroySession()).resolves.not.toThrow(); + await expect(activeStorage.destroySession()).resolves.toBeUndefined();
🧹 Nitpick comments (10)
lib/utils/activityTracking.ts (3)
117-121: Track activity on invocation, not on property access (prevents false positives and drift after destructuring)Bind the method and update on call. Also skip updates when suppression is active (see helper below).
- if (typeof value === "function") { - if (prop !== "destroySession") { - updateActivityTimestamp(); - } - return value.bind(target); - } + if (typeof value === "function") { + const bound = (value as unknown as Function).bind(target); + if (prop === "destroySession") { + return bound; + } + return function (...args: unknown[]) { + if (!isActivityUpdatesSuppressed()) { + updateActivityTimestamp(); + } + // Preserve `this` in rare rebind cases + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + return bound.apply(this, args); + }; + }Add these helpers at module scope (outside the selected lines):
let suppressActivityUpdateCount = 0; const isActivityUpdatesSuppressed = () => suppressActivityUpdateCount > 0; const withSuppressedActivity = <R>(fn: () => R): R => { try { suppressActivityUpdateCount++; return fn(); } finally { suppressActivityUpdateCount--; } };
55-63: Avoid re-arming timers during user callbacksWrap callbacks with suppression so any session-manager calls inside handlers don’t retrigger activity while handling the event.
try { - storageSettings.onActivityTimeout?.(TimeoutActivityType.timeout); + withSuppressedActivity(() => { + storageSettings.onActivityTimeout?.(TimeoutActivityType.timeout); + }); } catch (err) {try { - storageSettings.onActivityTimeout?.(TimeoutActivityType.preWarning); + withSuppressedActivity(() => { + storageSettings.onActivityTimeout?.(TimeoutActivityType.preWarning); + }); } catch (err) {Also applies to: 71-78
9-11: Browser-safe timer typesPrefer
ReturnType<typeof setTimeout>for cross-env typings (Node + browser).-let activityPreWarnTimer: NodeJS.Timeout | null = null; -let activityTimer: NodeJS.Timeout | null = null; +let activityPreWarnTimer: ReturnType<typeof setTimeout> | null = null; +let activityTimer: ReturnType<typeof setTimeout> | null = null;lib/utils/activityTracking.test.ts (7)
198-205: Use async timer advancement for consistencyAdvance timers asynchronously to flush microtasks after callbacks.
- vi.advanceTimersByTime(25 * 60 * 1000 + 1000); + await vi.advanceTimersByTimeAsync(25 * 60 * 1000 + 1000);
348-359: Derive test timing from config and use async advancementAvoid magic numbers and ensure correctness if config changes.
- // Advance to pre-warning (3 seconds) - vi.advanceTimersByTime(3 * 1000 + 100); + // Advance to pre-warning + await vi.advanceTimersByTimeAsync( + storageSettings.activityTimeoutPreWarningMinutes! * 60 * 1000 + 100, + ); @@ - // Advance to timeout (6 seconds total) - await vi.advanceTimersByTimeAsync(3 * 1000 + 100); + // Advance to timeout + await vi.advanceTimersByTimeAsync( + (storageSettings.activityTimeoutMinutes! - + storageSettings.activityTimeoutPreWarningMinutes!) * + 60 * + 1000 + + 100, + );
391-396: Align test with invocation-based tracking (if adopted)If you switch to “update on invocation,” assert after calling the method.
- // Accessing method properties should trigger activity updates - const methodRef = activeStorage.getSessionItem; - expect(methodRef).toBeDefined(); - expect(timeoutSpy).toHaveBeenCalledTimes(1); - expect(timeoutSpy).toHaveBeenCalledWith(expect.any(Function), 300000); // 5 minutes + // Calling a method should trigger activity updates + await activeStorage.getSessionItem(StorageKeys.accessToken); + expect(timeoutSpy).toHaveBeenCalledTimes(1); + expect(timeoutSpy).toHaveBeenCalledWith( + expect.any(Function), + 5 * 60 * 1000, + );
411-446: Async timer advancement for error path tooEnsure the pre-warning callback error is flushed before assertions.
- // Advance to pre-warning time - vi.advanceTimersByTime(3 * 1000 + 100); + // Advance to pre-warning time + await vi.advanceTimersByTimeAsync(3 * 1000 + 100);
295-302: Clarify test name (duplicate title)Two tests share the same description. Rename to reflect the inequality check.
-it("should throw error when no activity timeout configured", () => { +it("should throw error when pre-warning minutes >= timeout minutes", () => {
251-252: Use globalThis for portabilitySpy on
globalThis.setTimeoutto work in both node and browser environments.- const timeoutSpy = vi.spyOn(global, "setTimeout"); + const timeoutSpy = vi.spyOn(globalThis, "setTimeout");Also applies to: 388-389
335-381: Optional: add a negative test for re-arming during timeout handlerIf you adopt suppression, add a case where
onActivityTimeout(timeout path) calls a session method and verify no new timers are scheduled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
lib/utils/activityTracking.test.ts(3 hunks)lib/utils/activityTracking.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/utils/activityTracking.ts (1)
lib/sessionManager/index.ts (1)
storageSettings(3-42)
lib/utils/activityTracking.test.ts (3)
lib/utils/token/index.ts (2)
setActiveStorage(39-45)getActiveStorage(51-53)lib/sessionManager/index.ts (2)
storageSettings(3-42)StorageKeys(51-51)lib/utils/activityTracking.ts (1)
sessionManagerActivityProxy(92-126)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
lib/utils/activityTracking.ts (4)
55-63: Optionally suppress activity updates while executing timeout callback.If handlers touch the session manager, they may unintentionally re-arm timers. Wrap the callback with a suppression guard.
Apply within this hunk:
- try { - storageSettings.onActivityTimeout?.(TimeoutActivityType.timeout); - } catch (err) { + try { + withSuppressedActivity(() => { + storageSettings.onActivityTimeout?.(TimeoutActivityType.timeout); + }); + } catch (err) { // Shield callers from exceptions in user callbacks console.error( "[activityTimeout] onActivityTimeout(timeout) threw:", err, ); }Add once in the module (outside this range):
let _suppressCount = 0; const withSuppressedActivity = <R>(fn: () => R): R => { _suppressCount++; try { return fn(); } finally { _suppressCount--; } }; const isActivitySuppressed = () => _suppressCount > 0;
68-79: Mirror the suppression for pre-warning callback.Same rationale as timeout: avoid accidental re-arming from inside the handler.
- () => { - try { - storageSettings.onActivityTimeout?.(TimeoutActivityType.preWarning); - } catch (err) { + () => { + try { + withSuppressedActivity(() => { + storageSettings.onActivityTimeout?.(TimeoutActivityType.preWarning); + }); + } catch (err) { console.error( "[activityTimeout] onActivityTimeout(preWarning) threw:", err, ); } },
113-124: Update timers on invocation, not on method property access; also honor suppression.Triggering on property access can reset timers during mere introspection/destructuring. Prefer updating on actual calls and skip while suppressed.
const proxyHandler = { get(target: SessionManager<T>, prop: string | symbol) { - const value = target[prop as keyof SessionManager<T>]; - if (typeof value === "function") { - if (prop !== "destroySession") { - updateActivityTimestamp(); - } - return value.bind(target); - } - return value; + const value = target[prop as keyof SessionManager<T>]; + if (typeof value === "function") { + if (prop === "destroySession") { + return value.bind(target); + } + const fn = (value as Function).bind(target); + return (...args: unknown[]) => { + if (!isActivitySuppressed()) { + updateActivityTimestamp(); + } + return fn(...args); + }; + } + return value; }, };If you keep the current “property access updates activity” semantics intentionally, consider documenting it to set expectations.
9-10: Cross-env timer typing.
NodeJS.Timeoutbreaks in DOM-only builds. UseReturnType<typeof setTimeout>.-let activityPreWarnTimer: NodeJS.Timeout | null = null; -let activityTimer: NodeJS.Timeout | null = null; +let activityPreWarnTimer: ReturnType<typeof setTimeout> | null = null; +let activityTimer: ReturnType<typeof setTimeout> | null = null;lib/utils/activityTracking.test.ts (2)
396-411: Property access vs. invocation — confirm intended contract.This test encodes “method property access re-arms timers.” If you adopt invocation-only updates (recommended), update this test to call the method instead of just accessing it.
Proposed adjustment if moving to invocation-only:
const timeoutSpy = vi.spyOn(globalThis, "setTimeout").mockClear(); await activeStorage.getSessionItem(StorageKeys.accessToken); expect(timeoutSpy).toHaveBeenCalledWith( expect.any(Function), storageSettings.activityTimeoutMinutes! * 60 * 1000, );
305-333: Add a companion test for timeout-handler exceptions.You cover pre-warning exceptions; add a case where
onActivityTimeoutthrows onTimeoutActivityType.timeoutand assert:
- error is logged
- no additional timers are scheduled
Example:
it("swallows errors from timeout callback without re-arming timers", async () => { storageSettings.activityTimeoutMinutes = 0.05; const throwing = vi.fn((t: TimeoutActivityType) => { if (t === TimeoutActivityType.timeout) throw new Error("boom"); }); storageSettings.onActivityTimeout = throwing; const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {}); setActiveStorage(sessionManager); const active = getActiveStorage()!; await active.getSessionItem(StorageKeys.accessToken); await vi.advanceTimersByTimeAsync(storageSettings.activityTimeoutMinutes! * 60 * 1000 + 10); expect(throwing).toHaveBeenCalledWith(TimeoutActivityType.timeout); expect(consoleSpy).toHaveBeenCalledWith( "[activityTimeout] onActivityTimeout(timeout) threw:", expect.any(Error), ); // No more callbacks afterwards throwing.mockClear(); await vi.advanceTimersByTimeAsync(storageSettings.activityTimeoutMinutes! * 60 * 1000 + 10); expect(throwing).not.toHaveBeenCalled(); consoleSpy.mockRestore(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
lib/utils/activityTracking.test.ts(7 hunks)lib/utils/activityTracking.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-19T20:31:59.197Z
Learnt from: DanielRivers
PR: kinde-oss/js-utils#11
File: lib/utils/token/index.test.ts:59-62
Timestamp: 2024-11-19T20:31:59.197Z
Learning: In `lib/utils/token/index.test.ts`, the test "getInsecureStorage returns active storage when no insecure storage is set" is intentional. It verifies that when insecure storage is not set, `getInsecureStorage()` returns the active storage as a fallback, preferring secure storage but allowing for insecure storage when needed by consumers of the library.
Applied to files:
lib/utils/activityTracking.test.ts
🧬 Code graph analysis (2)
lib/utils/activityTracking.ts (1)
lib/sessionManager/index.ts (1)
storageSettings(3-42)
lib/utils/activityTracking.test.ts (3)
lib/utils/token/index.ts (2)
setActiveStorage(39-45)getActiveStorage(51-53)lib/sessionManager/index.ts (2)
StorageKeys(51-51)storageSettings(3-42)lib/utils/activityTracking.ts (1)
sessionManagerActivityProxy(92-126)
🔇 Additional comments (3)
lib/utils/activityTracking.ts (1)
55-63: Good defensive guard around user callback (timeout).Catching and logging exceptions from
onActivityTimeout(TimeoutActivityType.timeout)is the right call to prevent cascade failures.lib/utils/activityTracking.test.ts (2)
336-394: Solid regression coverage and async timer driving.
- “Once per inactivity cycle” test is precise and guards against the original infinite-loop bug.
- Async timer advancement at pre-warning is correct.
- Verifying no activity updates on
destroySessioncall is on point.- Pre-warning error handling test asserts logging without side effects.
Also applies to: 426-463, 198-209, 414-424
182-188: Good negative-path assertion.Throwing on
setActiveStorage(null as any)exercises proxy creation validation early.
Explain your changes
Problem
When timeout occurred,
destroySession()went through the activity proxy, which calledupdateActivityTimestamp(), creating new timers in an infinite loop.Testing
Added regression test
"should fire timeout callbacks only once per inactivity cycle"that verifies correct behavior and prevents future regressions.Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.