Skip to content

fix(discord): prevent voice speaker cache growth#110907

Closed
zhangguiping-xydt wants to merge 1 commit into
openclaw:mainfrom
zhangguiping-xydt:fix/discord-speaker-context-cache-bound
Closed

fix(discord): prevent voice speaker cache growth#110907
zhangguiping-xydt wants to merge 1 commit into
openclaw:mainfrom
zhangguiping-xydt:fix/discord-speaker-context-cache-bound

Conversation

@zhangguiping-xydt

@zhangguiping-xydt zhangguiping-xydt commented Jul 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes an issue where long-running Discord voice accounts could retain an unbounded number of expired speaker-context entries after encountering many unique speakers.

The cache uses a 60-second TTL, but expiry was only checked when the same guild/user key was looked up again. One-time speakers therefore remained in the voice manager's cache for its entire process lifetime.

Why This Change Was Made

The Discord voice speaker-context owner now caps the cache at 5,000 entries and evicts the oldest inserted entry when the cap is exceeded. This matches the fixed-TTL ordering: the oldest entry is also the first one due to expire.

The change does not alter the existing 60-second TTL, Discord REST behavior, permission checks, configuration, or public Plugin SDK surfaces.

User Impact

Discord voice bots that run for long periods or span many guilds no longer accumulate stale speaker identity contexts without a memory bound. Active and recently observed speakers continue to use the existing cache behavior.

Evidence

Exact commit: 3e9b135b2f619343c619a56b2ca31f4e644bd53c

Final pre-push checks:

  • latest checked origin/main: 5fdb0cd24e68cd27c54ec32c821e8d0165b2e7ac
  • current main does not contain a speaker-context cache size bound
  • no open PR touches extensions/discord/src/voice/speaker-context.ts or its test (all current open-PR pages checked)
  • broad open-PR searches for speaker context cache, discord voice cache bound, and one-time speakers cache returned no matches
  • author open PR count: 17 (below the repository limit of 20)

Before: regression proof

The regression drives the public production resolveContext() method through 5,001 unique Discord speaker keys, then looks up the oldest key again. On unpatched main, the oldest entry is still cached, so the client lookup count remains 5,001 instead of reaching 5,002.

$ node scripts/run-vitest.mjs extensions/discord/src/voice/speaker-context.test.ts --reporter verbose

 RUN  v4.1.10 /media/vdc/code/ai/github/openclaw-discord-speaker-context-cache-bound

 ✓ ... > reuses cached speaker context for repeated speaker lookups
 ✓ ... > does not cache speaker context when the cache expiry would exceed Date range
 × ... > evicts the oldest speaker context after the cache reaches its production bound
   → expected "vi.fn()" to be called 5002 times, but got 5001 times

 Test Files  1 failed (1)
      Tests  1 failed | 2 passed (3)
   Duration  7.42s

After: focused regression

The final test also re-reads the newest key after the overflow, proving the oldest entry is evicted while a recent entry remains cached.

$ node scripts/run-vitest.mjs extensions/discord/src/voice/speaker-context.test.ts --reporter verbose

 RUN  v4.1.10 /media/vdc/code/ai/github/openclaw-discord-speaker-context-cache-bound

 ✓ ... > reuses cached speaker context for repeated speaker lookups 497ms
 ✓ ... > does not cache speaker context when the cache expiry would exceed Date range 4ms
 ✓ ... > evicts the oldest speaker context after the cache reaches its production bound 165ms

 Test Files  1 passed (1)
      Tests  3 passed (3)
   Duration  9.72s

Adjacent Discord voice coverage

$ node scripts/run-vitest.mjs \
    extensions/discord/src/voice/speaker-context.test.ts \
    extensions/discord/src/voice/participant-context.test.ts \
    --reporter verbose

 Test Files  2 passed (2)
      Tests  5 passed (5)
   Duration  70.68s

Formatting and lint

$ ./node_modules/.bin/oxfmt --check \
    extensions/discord/src/voice/speaker-context.ts \
    extensions/discord/src/voice/speaker-context.test.ts
Checking formatting...

All matched files use the correct format.
Finished in 184ms on 2 files using 8 threads.

$ OPENCLAW_OXLINT_SKIP_PREPARE=1 node scripts/run-oxlint.mjs \
    extensions/discord/src/voice/speaker-context.ts \
    extensions/discord/src/voice/speaker-context.test.ts
# exit 0

$ git diff --check
# exit 0

The local check:changed fallback passed conflict-marker, max-lines, attribution, wildcard-export, duplicate-coverage, dependency-pin, formatting, Plugin SDK API, deprecated-API, plugin-boundary, package-patch, and temp-creation checks. It then stopped in the repository-wide extension typecheck on existing packages/net-policy/src/ip.ts dependency/type mismatches outside this two-file diff. Blacksmith was unavailable locally and direct AWS Crabbox was not authenticated; the PR's exact-head CI is therefore the authoritative typecheck/lint fan-out.

Review

The repository autoreview helper ran with Codex gpt-5.6-sol for approximately 17 minutes, then exited at the model-engine layer with codex engine failed (1) before returning findings or a clean verdict. It did not report a code finding. An equivalent manual review covered the changed class, the long-lived DiscordVoiceManager owner, voice ingress and participant-roster callers, the Discord entity/thread/presence cache siblings, concurrency behavior, TTL ordering, and adjacent tests.

Best-fix conclusion: keeping the bound inside DiscordVoiceSpeakerContextResolver is the narrow owner-boundary fix. A timer/sweep would add lifecycle work, and reusing the REST entity cache would mix different value and invalidation contracts.

@openclaw-barnacle openclaw-barnacle Bot added channel: discord Channel integration: discord size: XS labels Jul 18, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels Jul 18, 2026
@clawsweeper

clawsweeper Bot commented Jul 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 18, 2026, 3:54 PM ET / 19:54 UTC.

Summary
The PR caps DiscordVoiceSpeakerContextResolver at 5,000 entries, evicts the oldest inserted entry after overflow, and adds a regression test for oldest-entry eviction while retaining a recent entry.

PR surface: Source +10, Tests +21. Total +31 across 2 files.

Reproducibility: yes. at source level: applying the supplied focused test to current main drives 5,001 unique keys and shows that TTL-only lookup cleanup leaves the oldest key cached. This review did not execute contributor code, so the status remains source-reproducible rather than reproduced here.

Review metrics: none identified.

Stored data model
Persistent data-model change detected: persistent cache schema: extensions/discord/src/voice/speaker-context.test.ts, persistent cache schema: extensions/discord/src/voice/speaker-context.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

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

Rank-up moves:

  • [P1] Add redacted after-fix proof from a real Discord voice runtime that demonstrates the cache-bound path; remove private guild, user, endpoint, and credential data before posting it.
  • Update the PR body after adding proof; if automatic review does not restart, ask a maintainer to comment @clawsweeper re-review.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR includes useful before/after terminal output from a mock-backed Vitest test, but no after-fix evidence from a real Discord voice setup; add a redacted live runtime log, transcript, recording, or linked artifact and update the PR body for re-review.

Next step before merge

  • [P1] The remaining blocker is contributor-supplied real behavior proof, which a code-repair lane cannot safely create or verify on the contributor's Discord setup.

Security
Cleared: The two-file diff adds only a local Map capacity check and test coverage; it introduces no dependency, permission, secret, network, or supply-chain change.

Review details

Best possible solution:

Keep the owner-local 5,000-entry insertion-order bound and merge only after a redacted real Discord voice-runtime proof demonstrates the changed cache behavior without exposing private guild, user, endpoint, or credential data.

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

Yes, at source level: applying the supplied focused test to current main drives 5,001 unique keys and shows that TTL-only lookup cleanup leaves the oldest key cached. This review did not execute contributor code, so the status remains source-reproducible rather than reproduced here.

Is this the best way to solve the issue?

Yes. A fixed insertion-order cap in the resolver that owns the transient Map is narrower than adding a lifecycle timer or mixing speaker identity state with another Discord cache, and it preserves the existing TTL and public surfaces.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a bounded Discord voice memory-growth fix with limited, long-running-channel blast radius rather than an immediate confirmed service outage.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR includes useful before/after terminal output from a mock-backed Vitest test, but no after-fix evidence from a real Discord voice setup; add a redacted live runtime log, transcript, recording, or linked artifact and update the PR body for re-review.
Evidence reviewed

PR surface:

Source +10, Tests +21. Total +31 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 10 0 +10
Tests 1 21 0 +21
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 31 0 +31

What I checked:

  • Current-main behavior: Current main keeps only the 60-second TTL; an entry is deleted when that same key is later looked up after expiry, so unique one-time speaker keys have no size bound. (extensions/discord/src/voice/speaker-context.ts:11, 1d492f5c5378)
  • Patch owner boundary: The proposed bound is local to the resolver that owns this transient speaker-context Map; it adds no configuration, Plugin SDK, dependency, or core-runtime surface. (extensions/discord/src/voice/speaker-context.ts:11, 3e9b135b2f61)
  • Regression coverage: The added test resolves 5,001 distinct keys, verifies the oldest lookup fetches again, then verifies the newest key is still cached. (extensions/discord/src/voice/speaker-context.test.ts:58, 3e9b135b2f61)
  • Proof gate remains unmet: The PR body supplies before/after Vitest output whose Discord member client is a vi.fn mock. The preceding ClawSweeper review requested real behavior proof, and the updated body does not add a live Discord voice transcript, diagnostic log, recording, or linked artifact. (3e9b135b2f61)
  • Extension policy fit: The extension-scoped policy keeps plugin-local runtime behavior inside its owning extension and disallows unnecessary core or SDK expansion; this patch stays within that boundary. (extensions/AGENTS.md, 1d492f5c5378)

Likely related people:

  • Unattributed Discord voice area owner: The resolver is instantiated by DiscordVoiceManager, but the available read-only review artifacts did not expose reliable commit/blame provenance for a named current-main owner. (role: current-main area owner; confidence: low; files: extensions/discord/src/voice/speaker-context.ts, extensions/discord/src/voice/manager.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.
Review history (1 earlier review cycle)
  • reviewed 2026-07-18T19:11:41.342Z sha 3e9b135 :: needs real behavior proof before merge. :: none

@zhangguiping-xydt

zhangguiping-xydt commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

Closing because the remaining review gate requires a real Discord voice runtime transcript that exercises the speaker-context path. This environment has neither the shared Discord QA credentials nor permission to dispatch the upstream credentialed QA workflow, and the existing auto-join scenario alone would not exercise the bounded cache. The source-level eviction test is valid, but I cannot honestly supply the requested external-boundary proof. I am replacing this with a separate Discord Gateway queue-bound fix that can be demonstrated through the actual production GatewayPlugin over a real loopback WebSocket connection.

@zhangguiping-xydt

zhangguiping-xydt commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

Replacement submitted as #110954. It remains Discord-specific and bounds a long-lived queue, but its failure boundary is below the external Discord service: the PR includes before/after proof through the production GatewayPlugin over a real loopback WebSocket connection.

@clawsweeper clawsweeper Bot mentioned this pull request Jul 19, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

channel: discord Channel integration: discord P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@zhangguiping-xydt