Skip to content

fix: do not expire Infinity gcTime via setTimeout overflow - #25

Open
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/f003-gctime-overflow
Open

fix: do not expire Infinity gcTime via setTimeout overflow#25
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/f003-gctime-overflow

Conversation

@SebTardif

Copy link
Copy Markdown

What Problem This Solves

Fixes an issue where consumers that call createRouter({ gcTime: Infinity }), set preloadGcTime: Infinity, or pass a retention longer than the host timer limit (for example 30 * 24 * 60 * 60 * 1000) would schedule a 1ms garbage-collection timer. Node and browsers coerce setTimeout delays outside the signed 32-bit range to 1ms, so the unused match is re-checked every millisecond and Node prints TimeoutOverflowWarning. staleTime: Infinity is not affected because it is a comparison, not a timer.

Why This Change Was Made

scheduleGc now skips a timer when gcTime or preloadGcTime is not finite, so Infinity keeps the cached match until it is replaced or stop() runs. Finite values above 2^31-1 ms are clamped to that limit. The existing callback still reschedules when the match is younger than gcTime, so a 30-day retention can expire later instead of spinning 1ms timers. No public exports, types, history adapter shape, or loader signature changed.

User Impact

gcTime: Infinity and preloadGcTime: Infinity keep unused cached matches without overflowing the host timer. A 30-day (or other overflow) retention no longer emits TimeoutOverflowWarning or wake the event loop every millisecond. Default 30-minute GC is unchanged. Apps that never pass Infinity or a delay above about 24.8 days behave as before.

Evidence

Live node against the built dist/index.js on this branch, macOS 26.6.2, Node v26.7.0. The same public sequence (createRouter({ gcTime }), navigate("chat"), navigate("fast"), then 20ms) was run against the unpatched bundle from upstream/main and the patched bundle.

Unpatched dist/index.js (gcTime: Infinity, 16 overflow warnings in 20ms):

$ node /tmp/proof-uirouter-f003.mjs /tmp/uirouter-f003-unpatched.js Infinity
(node:41839) TimeoutOverflowWarning: Infinity does not fit into a 32-bit signed integer.
Timeout duration was set to 1.
GCTIME Infinity
WARNINGS 16
WARNING0 TimeoutOverflowWarning: Infinity does not fit into a 32-bit signed integer.
CACHED_AFTER_NAV 1
CACHED_AFTER_20MS 1
CACHED_ROUTE chat
ACTIVE_ROUTE fast

Unpatched dist/index.js (gcTime: 30 * 24 * 60 * 60 * 1000):

$ node /tmp/proof-uirouter-f003.mjs /tmp/uirouter-f003-unpatched.js 30d
(node:41840) TimeoutOverflowWarning: 2592000000 does not fit into a 32-bit signed integer.
Timeout duration was set to 1.
GCTIME 2592000000
WARNINGS 18
WARNING0 TimeoutOverflowWarning: 2592000000 does not fit into a 32-bit signed integer.
CACHED_AFTER_NAV 1
CACHED_AFTER_20MS 1
CACHED_ROUTE chat
ACTIVE_ROUTE fast

Patched dist/index.js (gcTime: Infinity, no overflow warning):

$ node /tmp/proof-uirouter-f003.mjs /tmp/oc-pr-uirouter-F003/dist/index.js Infinity
GCTIME Infinity
WARNINGS 0
WARNING0 none
CACHED_AFTER_NAV 1
CACHED_AFTER_20MS 1
CACHED_ROUTE chat
ACTIVE_ROUTE fast

Patched dist/index.js (gcTime: 2592000000, no overflow warning):

$ node /tmp/proof-uirouter-f003.mjs /tmp/oc-pr-uirouter-F003/dist/index.js 30d
GCTIME 2592000000
WARNINGS 0
WARNING0 none
CACHED_AFTER_NAV 1
CACHED_AFTER_20MS 1
CACHED_ROUTE chat
ACTIVE_ROUTE fast

pnpm run check passed locally (format, typecheck, lint, 23 tests, pack/import).

This timer path has been in scheduleGc since f047b64a87a5 (2026-06-20, refactor: finalize router match loading). Same class of host-timer overflow: TanStack Query #6287 (TimeoutOverflowWarning in Query.scheduleGc). Adjacent but different: #20 (stale navigation cancellation), #21 (loader redirect hop cap), #24 (history listener context).

Real behavior proof

  • Behavior or issue addressed: createRouter({ gcTime: Infinity }) and a 30-day gcTime no longer overflow setTimeout into a 1ms GC timer (TimeoutOverflowWarning). Unused cached matches stay cached without a 1ms reschedule loop.

  • Real environment tested: macOS 26.6.2 arm64, Node v26.7.0, @openclaw/uirouter built from fix/f003-gctime-overflow at /tmp/oc-pr-uirouter-F003.

  • Exact steps or command run after this patch:

    cd /tmp/oc-pr-uirouter-F003
    node /tmp/proof-uirouter-f003.mjs /tmp/oc-pr-uirouter-F003/dist/index.js Infinity
    node /tmp/proof-uirouter-f003.mjs /tmp/oc-pr-uirouter-F003/dist/index.js 30d
  • Evidence after fix: terminal output from the patched dist/index.js:

    $ node /tmp/proof-uirouter-f003.mjs /tmp/oc-pr-uirouter-F003/dist/index.js Infinity
    GCTIME Infinity
    WARNINGS 0
    WARNING0 none
    CACHED_AFTER_NAV 1
    CACHED_AFTER_20MS 1
    CACHED_ROUTE chat
    ACTIVE_ROUTE fast
    $ node /tmp/proof-uirouter-f003.mjs /tmp/oc-pr-uirouter-F003/dist/index.js 30d
    GCTIME 2592000000
    WARNINGS 0
    WARNING0 none
    CACHED_AFTER_NAV 1
    CACHED_AFTER_20MS 1
    CACHED_ROUTE chat
    ACTIVE_ROUTE fast
  • Observed result after fix: After createRouter({ gcTime: Infinity }) (and the 30-day value) plus navigate("chat") then navigate("fast"), Node emitted 0 TimeoutOverflowWarning events and cachedMatches still held chat after 20ms. The same commands against the unpatched bundle emitted 16 and 18 overflow warnings (Timeout duration was set to 1).

  • What was not tested: A browser setTimeout in a running OpenClaw UI shell, and a per-route gcTime: Infinity override (same scheduleGc helper).

Host setTimeout coerces Infinity and delays above 2^31-1 ms to 1ms.
createRouter({ gcTime: Infinity }) and a 30-day gcTime therefore
scheduled a 1ms GC timer (TimeoutOverflowWarning) and rescheduled
it every millisecond.

Skip non-finite gcTime so those matches stay cached. Clamp finite
overflows to 2^31-1 ms so the existing reschedule path can expire
them later.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@SebTardif
SebTardif requested a review from a team as a code owner August 30, 2026 01:43
@clawsweeper

clawsweeper Bot commented Aug 30, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 30, 2026
@clawsweeper

clawsweeper Bot commented Aug 30, 2026

Copy link
Copy Markdown

Codex review: blocked before merge. Reviewed September 4, 2026, 5:54 PM ET / 21:54 UTC.

ClawSweeper review

What this changes

This PR prevents route-cache garbage collection from passing infinite or overly large retention periods to host timers, with documentation and regression tests.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

Blocked before merge - 3 items remain

Keep this PR open: it fixes the timer-overflow problem with convincing real-runtime evidence, but its new non-finite guard also turns NaN and -Infinity retention values from expiring values into permanent cached matches. The same P1 finding remains on the unchanged PR head.

Priority: P1
Reviewed head: b981a9c73817a4b9ab5b1a538bec9d95d4e2f934

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The overflow fix is well demonstrated, but a narrow P1 compatibility defect remains in the changed retention guard.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The changed production owner is the cache garbage-collection scheduler; the PR body provides a real built-package Node run that navigates from one route to another and records zero overflow warnings for Infinity and a 30-day retention after the patch.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The changed production owner is the cache garbage-collection scheduler; the PR body provides a real built-package Node run that navigates from one route to another and records zero overflow warnings for Infinity and a 30-day retention after the patch.
Evidence reviewed 5 items Introduced compatibility regression: The introduced Number.isFinite(gcTime) return applies to all non-finite numbers, not only positive Infinity. It returns before the existing remaining &lt;= 0 removal path for -Infinity and before the former short host-timer path for NaN.
Current-main behavior remains unfixed: Current main still computes remaining and passes it directly to the host timer, so the central overflow defect remains necessary work rather than being implemented on main.
Regression coverage gap: The added tests cover positive Infinity and a finite 30-day value, but do not cover NaN or -Infinity, the two values newly retained by the broad guard.
Findings 1 actionable finding [P1] Limit permanent retention to positive Infinity
Security None None.

How this fits together

The UI router receives application navigation and preload requests and maintains active and cached route matches. When a match becomes unused, its configured retention period drives garbage collection that either retains or removes the cached match.

flowchart LR
  A[Navigation or preload] --> B[Cached route match]
  B --> C[Garbage collection scheduler]
  C --> D{Retention duration}
  D --> E[Host timer]
  D --> F[Retain or remove cache entry]
  E --> F
Loading

Before merge

  • Limit permanent retention to positive Infinity (P1) - Number.isFinite is false for NaN and -Infinity as well as positive Infinity. Before this hunk, -Infinity reached immediate removal and NaN reached the timer callback; this return retains both cached matches indefinitely. Use gcTime === Infinity and cover both cached and preloaded cases. This was also visible on the prior reviewed head.
  • Resolve merge risk (P1) - Merging the broad non-finite guard changes existing gcTime: -Infinity and gcTime: NaN behavior to indefinite cache retention, which can preserve stale route data and consume memory unexpectedly.
  • Complete next step (P2) - Change the non-finite guard to gcTime === Infinity and add cached and preloaded regression coverage for NaN and -Infinity.

Findings

  • [P1] Limit permanent retention to positive Infinity — src/loading.ts:103-105
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus test delta production +8/-1, tests +125, docs +5 Most of the 138 added lines are focused regression coverage, but the changed production conditional needs the missing invalid-number cases.

Merge-risk options

Maintainer options:

  1. Restrict permanent retention to positive Infinity (recommended)
    Replace the broad non-finite return with a positive-Infinity check and add focused expiry regressions before merge.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Use `gcTime === Infinity` for permanent retention and add cached/preloaded regressions for `NaN` and `-Infinity` expiry.

Technical review

Best possible solution:

Special-case only gcTime === Infinity, retain clamping for large positive finite values, and add cached and preloaded tests proving that NaN and -Infinity still expire.

Do we have a high-confidence way to reproduce the issue?

Yes. Current-main source sends configured retention values directly to setTimeout, and the supplied before-and-after Node trace exercises the public navigation sequence that exposes the overflow.

Is this the best way to solve the issue?

No. Clamping large finite values is appropriate, but treating every non-finite number as positive Infinity changes established expiry behavior; only positive Infinity should bypass scheduling.

Full review comments:

  • [P1] Limit permanent retention to positive Infinity — src/loading.ts:103-105
    Number.isFinite is false for NaN and -Infinity as well as positive Infinity. Before this hunk, -Infinity reached immediate removal and NaN reached the timer callback; this return retains both cached matches indefinitely. Use gcTime === Infinity and cover both cached and preloaded cases. This was also visible on the prior reviewed head.
    Confidence: 0.99
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is incorrect
Overall confidence: 0.99

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against ea06377b0e80.

Labels

Label justifications:

  • P1: Configured infinite retention currently causes a rapid timer loop, and the proposed fix has a P1 compatibility regression that must be repaired before users can rely on it.
  • merge-risk: 🚨 compatibility: The introduced guard changes prior expiry behavior for existing numeric inputs NaN and -Infinity.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The changed production owner is the cache garbage-collection scheduler; the PR body provides a real built-package Node run that navigates from one route to another and records zero overflow warnings for Infinity and a 30-day retention after the patch.
  • proof: sufficient: Contributor real behavior proof is sufficient. The changed production owner is the cache garbage-collection scheduler; the PR body provides a real built-package Node run that navigates from one route to another and records zero overflow warnings for Infinity and a 30-day retention after the patch.

Evidence

Acceptance criteria:

  • [P1] pnpm run test -- test/router-loading.test.ts.
  • [P1] pnpm run check.
  • [P1] git diff --check.

What I checked:

  • Introduced compatibility regression: The introduced Number.isFinite(gcTime) return applies to all non-finite numbers, not only positive Infinity. It returns before the existing remaining &lt;= 0 removal path for -Infinity and before the former short host-timer path for NaN. (src/loading.ts:103, b981a9c73817)
  • Current-main behavior remains unfixed: Current main still computes remaining and passes it directly to the host timer, so the central overflow defect remains necessary work rather than being implemented on main. (src/loading.ts:100, ea06377b0e80)
  • Regression coverage gap: The added tests cover positive Infinity and a finite 30-day value, but do not cover NaN or -Infinity, the two values newly retained by the broad guard. (test/router-loading.test.ts:403, b981a9c73817)
  • Real behavior proof: The PR body supplies before-and-after Node traces against built package output: Infinity and a 30-day value produce zero timeout-overflow warnings after the patch while retaining the expected cached match. (b981a9c73817)
  • Release and branch provenance: The repository state identifies v0.1.1 at f5ce7c0; the PR head is a later unmerged commit, and no local release tag or branch contains it. (package.json:3, b981a9c73817)

Likely related people:

  • Shakker: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Change the non-finite guard to a positive-Infinity check and add cached/preloaded tests for NaN and -Infinity expiry.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (12 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-31T04:33:21.103Z sha b981a9c :: needs changes before merge. :: [P1] Restrict non-expiring retention to positive Infinity
  • reviewed 2026-08-31T10:06:09.156Z sha b981a9c :: needs changes before merge. :: [P1] Preserve expiry for negative Infinity and NaN
  • reviewed 2026-09-01T10:04:07.246Z sha b981a9c :: needs changes before merge. :: [P1] Restrict permanent retention to positive Infinity
  • reviewed 2026-09-01T22:06:31.395Z sha b981a9c :: needs changes before merge. :: [P1] Restrict permanent retention to positive Infinity
  • reviewed 2026-09-02T17:19:22.012Z sha b981a9c :: needs changes before merge. :: [P1] Restrict permanent retention to positive Infinity
  • reviewed 2026-09-03T06:03:57.933Z sha b981a9c :: blocked before merge. :: [P1] Restrict permanent retention to positive Infinity
  • reviewed 2026-09-03T14:00:45.148Z sha b981a9c :: blocked before merge. :: [P1] Limit permanent retention to positive Infinity
  • reviewed 2026-09-04T03:59:50.797Z sha b981a9c :: blocked before merge. :: [P1] Limit permanent retention to positive Infinity

@clawsweeper clawsweeper Bot added P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 session-state 🚨 Merging this PR could lose, corrupt, stale, or mis-associate session or agent state. and removed P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 session-state 🚨 Merging this PR could lose, corrupt, stale, or mis-associate session or agent state. labels Aug 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P1 Urgent regression or broken agent/channel workflow affecting real users now. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant