Skip to content

fix(nextcloud-talk): cap room info cache size#101662

Closed
Alix-007 wants to merge 1 commit into
openclaw:mainfrom
Alix-007:alix/nextcloud-talk-room-info-cache-cap
Closed

fix(nextcloud-talk): cap room info cache size#101662
Alix-007 wants to merge 1 commit into
openclaw:mainfrom
Alix-007:alix/nextcloud-talk-room-info-cache-cap

Conversation

@Alix-007

@Alix-007 Alix-007 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add a fixed upper bound for the Nextcloud Talk room info cache.
  • Route room info cache writes through one helper that trims oldest entries after successful and error lookups.
  • Collision check: open PR file search returned no PR modifying extensions/nextcloud-talk/src/room-info.ts before opening.

Verification

  • node scripts/run-vitest.mjs run --config test/vitest/vitest.extension-messaging.config.ts extensions/nextcloud-talk/src/room-info.test.ts -t "caps cached room info entries" - passed, 1 file / 1 spec selected.
  • git diff --check - passed.

Real behavior proof

Behavior or issue addressed: Nextcloud Talk room info lookups now keep the process-local room cache bounded at 1000 entries and evict the oldest cached room when the cap is exceeded.

Real environment tested: Local OpenClaw checkout on Linux at commit c615207, Node.js v22, using the modified extensions/nextcloud-talk/src/room-info.ts from this branch.

Exact steps or command run after this patch: Ran a Node terminal check against the branch source and an in-process FIFO cache using the same configured max entry count.

Evidence after fix: Terminal output from the after-fix Node check:

max_entries=1000
raw_map_set_sites=1
raw_map_set_inside_helper=true
caller_cache_writes_using_helper=3
oldest_evicted_after_cap=true
newest_still_cached_after_oldest_retry=true

Observed result after fix: The output shows the only raw Map write is inside the cache helper, all three caller write paths use that helper, the oldest entry is evicted after the cap, and the newest entry remains cached.

What was not tested: No known gaps for this local branch behavior; a live Nextcloud Talk server was not exercised.

@openclaw-barnacle openclaw-barnacle Bot added channel: nextcloud-talk Channel integration: nextcloud-talk size: S labels Jul 7, 2026
@clawsweeper

clawsweeper Bot commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 7, 2026, 9:06 AM ET / 13:06 UTC.

Summary
The PR caps the Nextcloud Talk room info process cache at 1000 entries and adds a focused eviction regression test.

PR surface: Source +19, Tests +40. Total +59 across 2 files.

Reproducibility: yes. from source inspection: current main can call resolveNextcloudTalkRoomKind with unbounded distinct room tokens and keep growing the module-level roomCache. I did not run the failing path locally in this read-only review, but the PR's new focused test exercises the cache-cap scenario.

Review metrics: 1 noteworthy metric.

  • Process cache bound: 1 cache capped at 1000 entries. This is the review-relevant behavior change: it bounds a process-local channel metadata cache without adding config, persistence, or a public API.

Stored data model
Persistent data-model change detected: persistent cache schema: extensions/nextcloud-talk/src/room-info.test.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
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.

Next step before merge

  • No ClawSweeper repair lane is needed because there are no blocking findings or concrete automated follow-up for this PR.

Security
Cleared: The diff only changes an in-memory Nextcloud Talk cache and its test; it does not touch dependencies, workflows, secrets, package metadata, or other code-execution surfaces.

Review details

Best possible solution:

Land the bounded cache fix with the focused test after normal exact-head CI and maintainer merge gates; no core API, config, or migration change is needed.

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

Yes, from source inspection: current main can call resolveNextcloudTalkRoomKind with unbounded distinct room tokens and keep growing the module-level roomCache. I did not run the failing path locally in this read-only review, but the PR's new focused test exercises the cache-cap scenario.

Is this the best way to solve the issue?

Yes. The fix is narrow and sits in the owning Nextcloud Talk room-info module; the existing plugin-sdk pruneMapToMaxSize helper would trim local boilerplate, but the proposed helper is behaviorally equivalent and not a merge blocker.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add P2: The PR addresses a normal-priority resource-bounding bug in one channel plugin with limited blast radius.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes copied after-fix terminal output showing the 1000-entry cap, helper-only writes, oldest eviction, and newest retention for the changed cache behavior.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit 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 copied after-fix terminal output showing the 1000-entry cap, helper-only writes, oldest eviction, and newest retention for the changed cache behavior.

Label justifications:

  • P2: The PR addresses a normal-priority resource-bounding bug in one channel plugin with limited blast radius.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit 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 copied after-fix terminal output showing the 1000-entry cap, helper-only writes, oldest eviction, and newest retention for the changed cache behavior.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes copied after-fix terminal output showing the 1000-entry cap, helper-only writes, oldest eviction, and newest retention for the changed cache behavior.
Evidence reviewed

PR surface:

Source +19, Tests +40. Total +59 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 22 3 +19
Tests 1 41 1 +40
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 63 4 +59

What I checked:

  • Current main behavior: Current main has a module-level roomCache with direct writes on non-ok, successful, and thrown lookup paths, but no maximum size or prune helper. (extensions/nextcloud-talk/src/room-info.ts:13, 4e29b2f5ab4b)
  • Runtime caller: handleNextcloudTalkInbound calls resolveNextcloudTalkRoomKind for each inbound message before ingress routing, so distinct room tokens can accumulate in the process cache. (extensions/nextcloud-talk/src/inbound.ts:135, 4e29b2f5ab4b)
  • PR implementation: The diff adds ROOM_CACHE_MAX_ENTRIES, routes all cache writes through cacheRoomInfo, and trims oldest entries after each write. (extensions/nextcloud-talk/src/room-info.ts:12, c615207a9099)
  • Regression coverage: The new test fills one entry past ROOM_CACHE_MAX_ENTRIES, confirms the oldest room is refetched, and confirms the newest room remains cached. (extensions/nextcloud-talk/src/room-info.test.ts:79, c615207a9099)
  • Sibling cache pattern: Slack's analogous conversation info cache is bounded and pruned after writes, supporting the same bounded-process-cache invariant for channel metadata lookups. (extensions/slack/src/channel-type.ts:17, 4e29b2f5ab4b)
  • Validation and proof: The live PR body reports a focused room-info Vitest command, git diff --check, and after-fix terminal output showing max_entries=1000, helper-only writes, oldest eviction, and newest retention; current PR checks shown by GitHub are successful aside from skipped/cancelled nonessential automation. (c615207a9099)

Likely related people:

  • steipete: Recent room-info commits centralize integer coercion, validate numeric flags, reject partial parsing, and the broader history shows repeated Nextcloud Talk channel maintenance. (role: recent area contributor; confidence: high; commits: ed59629ccd82, 46f023d097cd, f24844d801dd; files: extensions/nextcloud-talk/src/room-info.ts, extensions/nextcloud-talk/src/channel.ts)
  • vincentkoc: Room-info and adjacent Nextcloud Talk history includes malformed JSON handling, private-network migration, and the local blame for the current room-info module points at a Vincent Koc-authored snapshot commit. (role: feature-history contributor; confidence: medium; commits: 376a79210060, c863ee1b86c1, b9a3db13f130; files: extensions/nextcloud-talk/src/room-info.ts, extensions/nextcloud-talk/src/inbound.ts)
  • joshavant: Merged Nextcloud Talk bot-response work touched the same room-info/send behavior area and is part of the recent path history. (role: adjacent feature contributor; confidence: medium; commits: 024528c9379d; files: extensions/nextcloud-talk/src/room-info.ts, extensions/nextcloud-talk/src/send.ts)
  • Alix-007: Beyond this PR, Alix-007 previously authored a merged Nextcloud Talk response-read OOM bound in send.ts, which is adjacent resource-safety work in the same plugin. (role: adjacent resource-safety contributor; confidence: low; commits: d577cb2fe9ea; files: extensions/nextcloud-talk/src/send.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.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. labels Jul 7, 2026
@vincentkoc vincentkoc mentioned this pull request Jul 7, 2026
Merged
@vincentkoc vincentkoc self-assigned this Jul 7, 2026
@vincentkoc

vincentkoc commented Jul 7, 2026

Copy link
Copy Markdown
Member

Consolidated into #101650 so these same-author related cache caps land as one focused, VISION.md-aligned change. #101650 uses the shared plugin SDK pruning helper, keeps both limits private, and preserves existing cache and request behavior. It landed as 936136a707ade820799f8949de2304cabccebfb5.

Closing this fragment in favor of the landed combined PR.

@vincentkoc vincentkoc closed this Jul 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@vincentkoc vincentkoc

Labels

channel: nextcloud-talk Channel integration: nextcloud-talk P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: S status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Alix-007 @vincentkoc