Skip to content

fix(thread-ownership): cap mention cache and bound forwarder JSON reads#101743

Closed
hugenshen wants to merge 1 commit into
openclaw:mainfrom
hugenshen:fix/thread-ownership-cap-mentions-bound-json
Closed

fix(thread-ownership): cap mention cache and bound forwarder JSON reads#101743
hugenshen wants to merge 1 commit into
openclaw:mainfrom
hugenshen:fix/thread-ownership-cap-mentions-bound-json

Conversation

@hugenshen

@hugenshen hugenshen commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes two thread-ownership resource bounds in the Slack multi-agent plugin:

  1. mentionedThreads — in-memory mention cache with no size cap under sustained @-mention traffic.
  2. 409 ownership-conflict JSON reads — forwarder conflict responses used unbounded resp.json() instead of the shared bounded provider JSON reader.

Why This Change Was Made

Routes mention tracking through pruneMapToMaxSize (cap 1 000 entries) and replaces the 409 handler's resp.json() call with readProviderJsonResponse from openclaw/plugin-sdk/provider-http.

User Impact

Long-lived gateways with high Slack @-mention volume get bounded mention-cache memory. Oversized ownership-conflict JSON bodies fail closed instead of buffering unbounded data.

Evidence

  • extensions/thread-ownership/index.ts:25–27,59–63 — mentionedThreads Map, MENTIONED_THREADS_MAX_ENTRIES = 1000, prune after each track
  • extensions/thread-ownership/index.ts:203 — readProviderJsonResponse on ownership-conflict responses
  • src/infra/map-size.ts — shared pruneMapToMaxSize helper
  • Diff: production changes in index.ts; regressions in index.test.ts

Behavior proof

1) Node runtime (production cap + pruneMapToMaxSize, no Vitest)

Replays the same key shapes and cap wired in index.ts:

=== thread-ownership mention cache cap (production constants + pruneMapToMaxSize) ===
Node: v22.22.0
size after 1001 inserts: 1000
oldest C000:1111.0001 evicted: true
newest retained: true

2) Call-site regression tests

$ pnpm test extensions/thread-ownership/index.test.ts --reporter=verbose

 ✓ thread-ownership plugin > message_received @-mention tracking > evicts oldest mention cache entries once the cache exceeds its cap 8ms

 Test Files  1 passed (1)
      Tests  22 passed (22)
   Duration  774ms (transform 274ms, setup 128ms, import 522ms, tests 50ms, environment 0ms)

[test] passed 1 Vitest shard in 4.24s

After 1 001 tracked mentions the oldest {channel}:{thread} key is evicted; ownership claim / 409 conflict tests still pass with bounded JSON parsing on the production path.

Proof gap: No redacted live Slack forwarder or production gateway logs in this validation environment.

@clawsweeper

clawsweeper Bot commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Close as superseded: the newer open Thread Ownership follow-up covers the same cache and bounded-conflict-read work, preserves 409 cancellation when the optional body is unreadable, and has stronger runtime proof, while this branch still has the fail-open 409 regression.

Root-cause cluster
Relationship: superseded
Canonical: #101968
Summary: The newer open follow-up PR covers the same Thread Ownership bounded-read and mention-cache problem while preserving 409 cancellation and supplying stronger runtime proof.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Canonical path: Use #101968 as the canonical Thread Ownership hardening path unless maintainers explicitly prefer to repair this branch instead.

So I’m closing this here and keeping the remaining discussion on #101968.

Review details

Best possible solution:

Use #101968 as the canonical Thread Ownership hardening path unless maintainers explicitly prefer to repair this branch instead.

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

Yes at source level: current main has an uncapped mention map and native resp.json() in the 409 branch, while this PR head can throw from readProviderJsonResponse before returning { cancel: true }. I did not run live Slack or forwarder proof in this read-only review.

Is this the best way to solve the issue?

No as submitted: the cache cap uses the right shared helper, but the 409 branch must preserve cancellation when optional owner-body parsing fails. The newer follow-up PR demonstrates the safer shape by catching parse/overflow inside the 409 branch and logging unknown.

Security review:

Security review cleared: The diff adds no dependencies, workflow changes, secret handling, artifact downloads, or supply-chain surface; the blocker is functional message-delivery behavior.

AGENTS.md: found and applied where relevant.

What I checked:

Likely related people:

  • vincentkoc: Current-main blame points to the latest Thread Ownership file introduction, and GitHub path history shows many recent commits on routing, live config, mention matching, and Slack ID canonicalization in the same hook path. (role: recent area contributor; confidence: high; commits: 7ff3f2dbf922, 65ae1e54de31, 834e50f83cf0; files: extensions/thread-ownership/index.ts, extensions/thread-ownership/index.test.ts)
  • steipete: GitHub path history shows recurring Thread Ownership test/config/SDK-boundary work and plugin SDK import-surface changes relevant to this PR's helper usage. (role: feature and SDK boundary contributor; confidence: high; commits: 4fa5092cdc39, 827b0de0ce74, f078d9022622; files: extensions/thread-ownership/index.ts, extensions/thread-ownership/index.test.ts, src/infra/map-size.ts)
  • RomneyDa: GitHub history shows the bounded HTTP body reader consolidation that underlies readProviderJsonResponse, the helper this PR now uses in the 409 path. (role: bounded-reader helper contributor; confidence: medium; commits: 6c53dfa1df99; files: src/agents/provider-http-errors.ts, src/plugin-sdk/provider-http.ts)

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

@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. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jul 7, 2026
@openclaw-barnacle openclaw-barnacle Bot added scripts Repository scripts size: M and removed size: S labels Jul 7, 2026
@hugenshen
hugenshen force-pushed the fix/thread-ownership-cap-mentions-bound-json branch from af45e07 to f6b138c Compare July 7, 2026 15:43
@openclaw-barnacle openclaw-barnacle Bot added size: S and removed scripts Repository scripts size: M labels Jul 7, 2026
@hugenshen

hugenshen commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review — branch rebased to single commit on latest main, proof scripts removed from repo, PR body updated with Real behavior proof (Vitest regression output).

@clawsweeper

clawsweeper Bot commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@hugenshen
hugenshen force-pushed the fix/thread-ownership-cap-mentions-bound-json branch from f6b138c to 0cb28b4 Compare July 8, 2026 02:15
@hugenshen

hugenshen commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Squashed to a single commit and rebased on latest main. Behavior proof is covered by colocated regression tests (no scripts/proof-*.mjs).

Test run

pnpm test extensions/thread-ownership/index.test.ts — 22 passed

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@mushuiyu886 mushuiyu886 mentioned this pull request Jul 8, 2026
Closed
@hugenshen

hugenshen commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot mentioned this pull request Jul 8, 2026
Merged
@hugenshen hugenshen closed this Jul 8, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 17, 2026
Closed
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

extensions: thread-ownership merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S 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

@hugenshen