fix(discord): bound REST entity cache to prevent unbounded Map growth by fede-kamel · Pull Request #77952 · openclaw/openclaw

fede-kamel · 2026-05-05T16:51:57Z

Summary

DiscordEntityCache (extensions/discord/src/internal/entity-cache.ts) cached user:<id>, channel:<id>, guild:<id>, and member:<gid>:<uid> REST fetches in a Map with a 30s TTL, but only checked the TTL on an exact-same-key re-fetch inside fetchCached. Different keys never triggered any sweep, so on a long-lived gateway the cache grew unbounded across the bot's lifetime — every distinct user/channel/guild touched stayed retained past its TTL until process restart. A slow leak proportional to entity cardinality.

This change adds:

A throttled write-time sweep (maybeSweepExpired, default once per 30s) that walks the Map and drops expired entries.
A hard maxEntries cap (default 5,000) enforced via FIFO drop using Map insertion order.
Optional constructor params maxEntries / sweepIntervalMs for callers that want to tune.
A read-only size getter for diagnostics.

The existing ttlMs? constructor option, public fetch surface, and invalidateForGatewayEvent semantics are unchanged.

Verification

New colocated entity-cache.test.ts: 4 cases — cap enforcement, post-TTL sweep, no-sweep before interval, ttlMs=0 disables caching. All pass.
oxfmt --check clean on the touched files.
Live demo run included below in Real behavior proof.

Real behavior proof

Behavior or issue addressed: DiscordEntityCache.entries is a Map with read-time TTL check only and no write-time eviction or hard size cap, so on a long-lived gateway the cache grows unbounded as different user:/channel:/guild:/member: keys are touched. The 30s TTL is never enforced for keys that aren't re-fetched.
Real environment tested: local node v22.22.2 runtime (no Discord network); the patched DiscordEntityCache was driven by a fake RequestClient returning { id } for every route, so the cache exercises real eviction code paths under high cardinality without going through any test framework.
Exact steps or command run after this patch: wrote /tmp/discord-cache-demo.ts that imports the patched cache, runs 100,000 unique-key fetches against the cap, then runs a sweep-window scenario with ttlMs=10/sweepIntervalMs=0. Executed via pnpm exec tsx /tmp/discord-cache-demo.ts — direct node runtime output, no mocking framework involved.
Evidence after fix: live console output captured directly from the node runtime:

=== capped run (default maxEntries=5000, ttlMs=60s) ===
inserted 0 size: 1
inserted 10000 size: 5000
inserted 20000 size: 5000
inserted 30000 size: 5000
inserted 40000 size: 5000
inserted 50000 size: 5000
inserted 60000 size: 5000
inserted 70000 size: 5000
inserted 80000 size: 5000
inserted 90000 size: 5000
final size after 100k unique fetches: 5000

=== sweep run (maxEntries=10_000, ttlMs=10ms, sweep=0) ===
size after 5k inserts (TTL 10ms): 456
size after a single insert past TTL window: 1

Observed result after fix: with maxEntries=5000, the cache stays pinned at 5,000 entries across 100,000 unique fetches (oldest-first eviction holds the cap). With ttlMs=10ms and sweepIntervalMs=0, one insert past the TTL window sweeps the prior 456 stale entries down to 1, confirming the write-time expiry sweep works.
What was not tested: behavior against a real Discord gateway with an active bot session — the fix is purely a Map/cache change with no network surface, and the live runtime demo exercises the same eviction code paths under higher cardinality than a real bot would see.

clawsweeper · 2026-05-05T16:55:23Z

Codex review: needs maintainer review before merge. Reviewed May 31, 2026, 2:04 PM ET / 18:04 UTC.

Summary
The PR adds write-time expired-entry sweeping, a 5,000-entry cap, a diagnostic size getter, and focused tests for DiscordEntityCache.

PR surface: Source +42, Tests +77. Total +119 across 2 files.

Reproducibility: yes. via source inspection, but not by executing a repro in this read-only review. Current main only deletes an expired entry for the requested key and never visits other keys or caps the Map, so N unique successful fetches can retain N entries.

Review metrics: 1 noteworthy metric.

Internal cache defaults: 2 added: maxEntries=5,000 and sweepIntervalMs=30,000. These defaults set the memory-versus-refetch behavior maintainers should notice before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

none.

Risk before merge

[P1] Focused tests were not rerun during this read-only review, so required CI remains the validation gate before merge.

Maintainer options:

Decide the mitigation before merge
Land the focused cache cap and sweep with the current safe expiry guards intact after required checks pass; broader Discord cache audits should stay in separate items.
Pause or close
Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

No ClawSweeper repair lane is needed; the current head has no actionable code finding, so normal PR merge gates are the remaining path.

Security
Cleared: The diff only changes private Discord cache behavior and colocated tests; it adds no dependency, workflow, credential, package, or code-execution surface.

Review details

Best possible solution:

Land the focused cache cap and sweep with the current safe expiry guards intact after required checks pass; broader Discord cache audits should stay in separate items.

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

Yes via source inspection, but not by executing a repro in this read-only review. Current main only deletes an expired entry for the requested key and never visits other keys or caps the Map, so N unique successful fetches can retain N entries.

Is this the best way to solve the issue?

Yes, this is the right owner boundary: DiscordEntityCache owns these REST entity objects, and the PR bounds that private map without adding user config or changing the Client fetch API.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against beb499b4d1c0.

Label changes

Label changes:

add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix terminal output from a node runtime driving the patched cache through high-cardinality cap and TTL-sweep scenarios.
add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes after-fix terminal output from a node runtime driving the patched cache through high-cardinality cap and TTL-sweep scenarios.
remove rating: 🦐 gold shrimp: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.
remove merge-risk: 🚨 compatibility: Current PR review selected no merge-risk labels.
remove status: ⏳ waiting on author: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

P2: This is a normal-priority Discord memory-bounding bug fix with limited channel-plugin blast radius.
rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes after-fix terminal output from a node runtime driving the patched cache through high-cardinality cap and TTL-sweep scenarios.
proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix terminal output from a node runtime driving the patched cache through high-cardinality cap and TTL-sweep scenarios.

Evidence reviewed

PR surface:

Source +42, Tests +77. Total +119 across 2 files.

View PR surface stats

Area	Files	Added	Net
Source	1	42	+42
Tests	1	77	+77
Docs	0	0	0
Config	0	0	0
Generated	0	0	0
Other	0	0	0
Total	2	119	+119

What I checked:

Current main still has unbounded unique-key retention: Current main only checks expiry for the requested cache key and sets new keys without sweeping other expired entries or enforcing a maximum size. (extensions/discord/src/internal/entity-cache.ts:84, beb499b4d1c0)
PR head composes sweep and cap with current expiry helpers: The current PR head keeps asDateTimestampMs/resolveExpiresAtMsFromDurationMs and adds maybeSweepExpired plus enforceMaxEntries after successful fetches. (extensions/discord/src/internal/entity-cache.ts:93, 539aa4debbba)
Focused regression tests are present: The new test file covers cap enforcement, expired-entry sweeping, no early sweep before the interval, and ttlMs=0 avoiding cache writes. (extensions/discord/src/internal/entity-cache.test.ts:25, 539aa4debbba)
Caller surface is private to the Discord client: Current main constructs DiscordEntityCache only inside Client and public fetch methods delegate through that private instance, so the PR does not add a user-facing config surface. (extensions/discord/src/internal/client.ts:201, beb499b4d1c0)
Latest release has not shipped the fix: The latest release tag v2026.5.28 still uses simple same-key cache reuse and Date.now()+ttl insertion with no sweep or max-size cap. (extensions/discord/src/internal/entity-cache.ts:84, e93216080aa1)
Timestamp helper contract checked: The number-runtime helpers reject invalid Date timestamps and overflowed expiry calculations; the current PR head now preserves those helpers after the review repair commit. (packages/normalization-core/src/number-coercion.ts:113, beb499b4d1c0)

Likely related people:

steipete: Recent Discord-area history is dominated by Peter Steinberger, and the current PR head includes his focused repair preserving the expiry guards. (role: recent area contributor and PR repair author; confidence: medium; commits: 539aa4debbba, e93216080aa1, 304e2c83c01f; files: extensions/discord/src/internal/entity-cache.ts, extensions/discord/src/internal/client.ts, extensions/discord/src)
vincentkoc: Discord source shortlog and recent commits show substantial adjacent runtime and test work in the same plugin area. (role: recent adjacent Discord owner; confidence: medium; commits: 01a5e492b79c, 381a8e860a9e, 01d49cf32fb1; files: extensions/discord/src)
alkor2000: Current-main blame in this checkout attributes the existing DiscordEntityCache and Client construction lines to this author, though the broad commit title makes ownership confidence lower. (role: current-source provenance signal; confidence: low; commits: 723d09ff852a; files: extensions/discord/src/internal/entity-cache.ts, extensions/discord/src/internal/client.ts)

What the crustacean ranks mean

🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works

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.

fede-kamel · 2026-05-05T17:21:23Z

@byungskers good point. TTL already bounds the working set here (30s window), so under cap pressure FIFO and LRU effectively converge — but the LRU swap is small and worth measuring. Will park it for a follow-up PR with hit-rate telemetry rather than gate this fix on it. Thanks for the review!

The expiry-sweep test relied on wall-clock async scheduling staying under ttlMs=1ms between two consecutive awaits. On a loaded worker that gap can exceed 1ms, letting maybeSweepExpired drop u1 before the size==2 precondition. Switch the two timing-sensitive tests to vitest fake timers so Date.now() is frozen across awaits and advanced explicitly. Addresses review feedback on openclaw#77952.

fede-kamel · 2026-05-06T22:19:16Z

Rebased onto current origin/main (was 372 commits behind). The previous "Real behavior proof" CI failure was a missing-script artifact — scripts/github/real-behavior-proof-check.mjs was added in #77622 after this branch was first pushed, so the workflow could not load on the old base. The script is on HEAD now and CI is re-running.

@bryce-d-greybeard — flagging for review when convenient (Discord REST entity-cache bound).

openclaw-barnacle · 2026-05-31T05:02:25Z

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

DiscordEntityCache held REST fetches (`user:<id>`, `channel:<id>`, `guild:<id>`, `member:<gid>:<uid>`) in a Map with a read-time TTL check on the exact same key. Different keys never triggered any sweep, so the cache grew unbounded for the lifetime of the gateway as the bot touched distinct users/channels/guilds — a slow leak proportional to entity cardinality. - Add a throttled write-time sweep (default once per 30s) that walks the Map and drops expired entries. - Add a hard `maxEntries` cap (default 5,000) with FIFO eviction using Map insertion order. - Expose optional `maxEntries` / `sweepIntervalMs` constructor params and a read-only `size` getter. Existing `ttlMs?`, public `fetchUser`/`fetchChannel`/`fetchGuild`/ `fetchMember` surface, and `invalidateForGatewayEvent` semantics are unchanged.

The expiry-sweep test relied on wall-clock async scheduling staying under ttlMs=1ms between two consecutive awaits. On a loaded worker that gap can exceed 1ms, letting maybeSweepExpired drop u1 before the size==2 precondition. Switch the two timing-sensitive tests to vitest fake timers so Date.now() is frozen across awaits and advanced explicitly. Addresses review feedback on openclaw#77952.

steipete · 2026-05-31T18:08:10Z

Maintainer pass done on head 539aa4d.

Changes:

Rebased the Discord entity-cache bound fix onto current main.
Preserved current main's safe expiry helpers (asDateTimestampMs and resolveExpiresAtMsFromDurationMs) while adding the write-time sweep and max-entry cap.
Dropped the stale CHANGELOG.md edit; release notes can be generated at release time.

Verification:

pnpm test extensions/discord/src/internal/entity-cache.test.ts
pnpm test extensions/discord/src/internal/entity-cache.test.ts extensions/discord/src/internal/client.test.ts
pnpm test src/gateway/server-methods/agent.test.ts -- -t "tracks plugin SDK subagent agent runs through the subagent registry only" (rerun of unrelated failed CI test; passed locally)
/Users/steipete/Projects/agent-scripts/skills/autoreview/scripts/autoreview --mode branch --base origin/main -> clean, no accepted/actionable findings
GitHub CI on 539aa4d: current-head matrix green after rerunning the unrelated checks-node-agentic-gateway-methods flake.

Known note:

The first checks-node-agentic-gateway-methods run failed in an unrelated gateway subagent registry assertion; the exact test passed locally and the GitHub rerun passed.

openclaw-barnacle Bot added channel: discord Channel integration: discord size: S labels May 5, 2026

This comment was marked as low quality.

Sign in to view

fede-kamel force-pushed the security/discord-entity-cache-bound branch from ddb14ac to 2779d23 Compare May 6, 2026 22:03

openclaw-barnacle Bot added the proof: supplied External PR includes structured after-fix real behavior proof. label May 6, 2026

clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 6, 2026

openclaw-barnacle Bot added the stale Marked as stale due to inactivity label May 31, 2026

clawsweeper Bot added rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. and removed proof: sufficient ClawSweeper judged the real behavior proof convincing. labels May 31, 2026

barnacle-openclaw Bot removed the stale Marked as stale due to inactivity label May 31, 2026

steipete self-assigned this May 31, 2026

fede-kamel and others added 3 commits May 31, 2026 18:56

fix(discord): preserve cache expiry guards

539aa4d

steipete force-pushed the security/discord-entity-cache-bound branch from 2779d23 to 539aa4d Compare May 31, 2026 17:57

openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 31, 2026

steipete merged commit 78f2a89 into openclaw:main May 31, 2026
282 of 288 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(discord): bound REST entity cache to prevent unbounded Map growth#77952

fix(discord): bound REST entity cache to prevent unbounded Map growth#77952
steipete merged 3 commits into
openclaw:mainfrom
fede-kamel:security/discord-entity-cache-bound

fede-kamel commented May 5, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented May 5, 2026 •

edited

Loading

Uh oh!

This comment was marked as low quality.

fede-kamel commented May 5, 2026 •

edited

Loading

Uh oh!

fede-kamel commented May 6, 2026

Uh oh!

openclaw-barnacle Bot commented May 31, 2026

Uh oh!

steipete commented May 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

fede-kamel commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Verification

Real behavior proof

Uh oh!

clawsweeper Bot commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as low quality.

fede-kamel commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fede-kamel commented May 6, 2026

Uh oh!

openclaw-barnacle Bot commented May 31, 2026

Uh oh!

steipete commented May 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fede-kamel commented May 5, 2026 •

edited

Loading

clawsweeper Bot commented May 5, 2026 •

edited

Loading

fede-kamel commented May 5, 2026 •

edited

Loading