Skip to content

fix(test): reduce startup-heavy hotspot retention#52381

Merged
vincentkoc merged 1 commit intomainfrom
fix/test-memory-hotspots
Mar 22, 2026
Merged

fix(test): reduce startup-heavy hotspot retention#52381
vincentkoc merged 1 commit intomainfrom
fix/test-memory-hotspots

Conversation

@vincentkoc
Copy link
Copy Markdown
Member

Summary

  • Problem: several small hotspot tests were paying very high startup/import costs and inflating shared worker RSS, especially src/infra/outbound/message-action-runner.threading.test.ts, extensions/msteams/src/monitor.test.ts, extensions/feishu/index.test.ts, extensions/bluebubbles/src/setup-surface.test.ts, and src/plugins/provider-wizard.test.ts.
  • Why it matters: these files distort shared-lane memory, make traces look leakier than they are, and slow targeted/local verification.
  • What changed: split two heavy test seams into lighter helpers, trimmed one plugin-entry test import graph, and added extension singletonIsolated scheduler support so startup-heavy extension tests can be peeled out of the shared lane.
  • What did NOT change (scope boundary): no product behavior changes; this only adjusts test/runtime seams and test scheduling.

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor required for the fix
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration
  • Skills / tool execution
  • Auth / tokens
  • Memory / storage
  • Integrations
  • API / contracts
  • UI / DX
  • CI/CD / infra

Linked Issue/PR

  • Closes #
  • Related #

User-visible / Behavior Changes

None.

Security Impact (required)

  • New permissions/capabilities? (Yes/No) No
  • Secrets/tokens handling changed? (Yes/No) No
  • New/changed network calls? (Yes/No) No
  • Command/tool execution surface changed? (Yes/No) No
  • Data access scope changed? (Yes/No) No
  • If any Yes, explain risk + mitigation:

Repro + Verification

Environment

  • OS: macOS
  • Runtime/container: Node 22 / pnpm
  • Model/provider: N/A
  • Integration/channel (if any): Feishu, MS Teams, BlueBubbles test surfaces
  • Relevant config (redacted): default local test env

Steps

  1. Run the hotspot tests with OPENCLAW_TEST_MEMORY_TRACE=1 OPENCLAW_TEST_WORKERS=1 pnpm test -- <file>.
  2. Compare startup time / peak RSS before and after the seam split or scheduler change.
  3. Re-run targeted tests, pnpm build, and pnpm check.

Expected

  • Startup-heavy tests use lighter seams where possible.
  • Remaining import-heavy extension tests are isolated out of the shared extensions worker lane.
  • No product behavior regressions.

Actual

  • src/infra/outbound/message-action-runner.threading.test.ts: 22.88s / 518.6 MiB -> 2.39s / 222.4 MiB
  • extensions/msteams/src/monitor.test.ts: 19.56s / 584.8 MiB -> 1.20s / 250.2 MiB
  • src/plugins/provider-wizard.test.ts: 317.8 MiB -> 237.0 MiB, and now isolated from shared unit-fast
  • extensions/feishu/index.test.ts and extensions/bluebubbles/src/setup-surface.test.ts remain startup-heavy, but are now isolated from the shared extensions lane
  • Heap snapshots for extensions/msteams/src/monitor.test.ts showed no meaningful retained growth, so that case was startup/import cost rather than a real leak

Evidence

Attach at least one:

  • Failing test/log before + passing after
  • Trace/log snippets
  • Screenshot/recording
  • Perf numbers (if relevant)

Human Verification (required)

What you personally verified (not just CI), and how:

  • Verified scenarios:
    • targeted memory-traced reruns for the hotspot files
    • targeted tests for the new seams and the extension include-file config
    • pnpm build
    • pnpm check
  • Edge cases checked:
    • explicit thread id still wins over auto-threading
    • replyTo still flows into thread resolution
    • extension include-file config rejects malformed JSON shape
    • MS Teams timeout helper behavior unchanged
  • What you did not verify:
    • full pnpm test

Review Conversations

  • I replied to or resolved every bot review conversation I addressed in this PR.
  • I left unresolved only the conversations that still need reviewer or maintainer judgment.

If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.

Compatibility / Migration

  • Backward compatible? (Yes/No) Yes
  • Config/env changes? (Yes/No) No
  • Migration needed? (Yes/No) No
  • If yes, exact upgrade steps:

Failure Recovery (if this breaks)

  • How to disable/revert this change quickly: revert commit 4d7706e64c
  • Files/config to restore: test runner manifest/scheduler files and the two extracted helper modules
  • Known bad symptoms reviewers should watch for: missing extension tests in the shared lane, or threading helper tests no longer covering route/session-key injection

Risks and Mitigations

  • Risk: extension singleton scheduling could accidentally exclude tests from the shared lane without rerunning them elsewhere.
    • Mitigation: scripts/test-parallel.mjs now creates explicit isolated extension entries, and test/vitest-extensions-config.test.ts covers the include-file behavior.
  • Risk: splitting helpers could weaken coverage if tests stop exercising important paths.
    • Mitigation: the threading test still verifies route/session-key injection semantics, and the MS Teams timeout test still validates the real exported helper behavior.

@vincentkoc vincentkoc self-assigned this Mar 22, 2026
@openclaw-barnacle openclaw-barnacle bot added channel: msteams Channel integration: msteams scripts Repository scripts channel: feishu Channel integration: feishu size: L maintainer Maintainer-authored PR labels Mar 22, 2026
@vincentkoc vincentkoc marked this pull request as ready for review March 22, 2026 16:57
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 22, 2026

Greptile Summary

This PR reduces startup time and RSS for five identified hotspot test files by splitting two heavy modules into lighter seams and adding a new singletonIsolated scheduling tier for the extensions test runner.

Key changes:

  • message-action-threading.ts (new): extracts resolveAndApplyOutboundThreadId and prepareOutboundMirrorRoute from message-action-runner.ts; behavior is identical — callers pre-resolve resolveAutoThreadId from getChannelPlugin(channel), which is functionally equivalent to the old inline lookup.
  • webhook-timeouts.ts (new): extracts applyMSTeamsWebhookTimeouts and its constants/type from monitor.ts so the timeout test no longer drags in the full MS Teams monitor import graph.
  • vitest.extensions.config.ts: adds loadIncludePatternsFromEnv to accept an explicit file-list via OPENCLAW_VITEST_INCLUDE_FILE; falls back to the existing extensions/**/*.test.ts glob when the variable is absent, preserving local-dev behavior. The new function is well-covered by test/vitest-extensions-config.test.ts.
  • scripts/test-parallel.mjs + test-runner-manifest.mjs: introduces extensions.singletonIsolated scheduling so startup-heavy extension tests (feishu, monitor, bluebubbles) each get their own --pool=forks run rather than inflating the shared extensions lane.
  • One minor edge case in the runner: when extensionSharedCandidateFiles.length === 0 (all extension tests isolated), OPENCLAW_VITEST_INCLUDE_FILE is not set and the shared-lane run falls back to the global glob, potentially double-executing isolated tests. Easily guarded but low probability with the current manifest.

Confidence Score: 5/5

  • Safe to merge — no product behavior changes, pnpm build and pnpm check confirmed passing, and the refactoring is mechanically straightforward with preserved semantics.
  • All threading logic is a direct extraction with identical branching; the only delta is callers now pre-resolve resolveAutoThreadId rather than passing channel to the helper. The new test seams cover session-key injection, thread-id propagation, and replyTo forwarding. The one flagged edge case (fallback glob when all extensions are isolated) is a non-blocking P2 with a trivially low probability of occurring given the current manifest.
  • Light attention on scripts/test-parallel.mjs around the extensionSharedCandidateFiles.length === 0 fallback; all other files are straightforward extractions.

Comments Outside Diff (1)

  1. scripts/test-parallel.mjs, line 145-148 (link)

    P2 Shared-lane fallback when all extensions are isolated

    When extensionSingletonIsolatedFiles accounts for every extension test file, extensionSharedCandidateFiles.length is 0, the env block is omitted, and OPENCLAW_VITEST_INCLUDE_FILE is never set. loadIncludePatternsFromEnv() then returns null, so the shared extensions run falls back to the extensions/**/*.test.ts glob and would re-run all the singletonIsolated tests a second time.

    This is unlikely with the current manifest (only 3 isolated entries) but could silently double-execute isolated tests if the manifest grows. A safe guard would be to skip the shared-lane entry entirely when there are no candidates:

    const extensionSingletonExcludedFileSet = new Set(extensionSingletonIsolatedFiles);
    const extensionSharedCandidateFiles = allKnownTestFiles.filter(
      (file) => file.startsWith("extensions/") && !extensionSingletonExcludedFileSet.has(file),
    );
    // … later, when building baseRuns:
    // wrap the whole "extensions" entry in `extensionSharedCandidateFiles.length > 0 ? [...] : []`
    // so the shared lane is skipped entirely rather than falling back to the broad glob.
    
    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: scripts/test-parallel.mjs
    Line: 145-148
    
    Comment:
    **Shared-lane fallback when all extensions are isolated**
    
    When `extensionSingletonIsolatedFiles` accounts for every extension test file, `extensionSharedCandidateFiles.length` is 0, the `env` block is omitted, and `OPENCLAW_VITEST_INCLUDE_FILE` is never set. `loadIncludePatternsFromEnv()` then returns `null`, so the shared extensions run falls back to the `extensions/**/*.test.ts` glob and would re-run all the singletonIsolated tests a second time.
    
    This is unlikely with the current manifest (only 3 isolated entries) but could silently double-execute isolated tests if the manifest grows. A safe guard would be to skip the shared-lane entry entirely when there are no candidates:
    
    ```
    const extensionSingletonExcludedFileSet = new Set(extensionSingletonIsolatedFiles);
    const extensionSharedCandidateFiles = allKnownTestFiles.filter(
      (file) => file.startsWith("extensions/") && !extensionSingletonExcludedFileSet.has(file),
    );
    // … later, when building baseRuns:
    // wrap the whole "extensions" entry in `extensionSharedCandidateFiles.length > 0 ? [...] : []`
    // so the shared lane is skipped entirely rather than falling back to the broad glob.
    ```
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: scripts/test-parallel.mjs
Line: 145-148

Comment:
**Shared-lane fallback when all extensions are isolated**

When `extensionSingletonIsolatedFiles` accounts for every extension test file, `extensionSharedCandidateFiles.length` is 0, the `env` block is omitted, and `OPENCLAW_VITEST_INCLUDE_FILE` is never set. `loadIncludePatternsFromEnv()` then returns `null`, so the shared extensions run falls back to the `extensions/**/*.test.ts` glob and would re-run all the singletonIsolated tests a second time.

This is unlikely with the current manifest (only 3 isolated entries) but could silently double-execute isolated tests if the manifest grows. A safe guard would be to skip the shared-lane entry entirely when there are no candidates:

```
const extensionSingletonExcludedFileSet = new Set(extensionSingletonIsolatedFiles);
const extensionSharedCandidateFiles = allKnownTestFiles.filter(
  (file) => file.startsWith("extensions/") && !extensionSingletonExcludedFileSet.has(file),
);
// … later, when building baseRuns:
// wrap the whole "extensions" entry in `extensionSharedCandidateFiles.length > 0 ? [...] : []`
// so the shared lane is skipped entirely rather than falling back to the broad glob.
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "Merge branch 'main' into fix/test-memory..." | Re-trigger Greptile

@vincentkoc vincentkoc force-pushed the fix/test-memory-hotspots branch from 84d3c15 to fa1f376 Compare March 22, 2026 19:17
@vincentkoc vincentkoc merged commit dbd26e4 into main Mar 22, 2026
35 of 39 checks passed
@vincentkoc vincentkoc deleted the fix/test-memory-hotspots branch March 22, 2026 19:28
frankekn pushed a commit to artwalker/openclaw that referenced this pull request Mar 23, 2026
alexey-pelykh pushed a commit to remoteclaw/remoteclaw that referenced this pull request Mar 24, 2026
alexey-pelykh added a commit to remoteclaw/remoteclaw that referenced this pull request Mar 24, 2026
* test: add Feishu bot-menu lifecycle regression

(cherry picked from commit 0e825ec)

* fix(feishu): harden webhook signature compare

(cherry picked from commit 223ae42)

* refactor: move feishu zalo zalouser to setup wizard

(cherry picked from commit 40be12d)

* Plugin SDK: add legacy message discovery helper

(cherry picked from commit 4c36436)

* refactor: rename setup helper surfaces

(cherry picked from commit 53ccc78)

* Feishu: split setup adapter helpers

(cherry picked from commit 61bcdcc)

* refactor: share feishu webhook monitor harness

(cherry picked from commit 6464149)

* fix(ci): reset deep test runtime state

(cherry picked from commit 83a267e)

* Plugins: internalize matrix and feishu SDK imports

(cherry picked from commit 889bb8a)

* Feishu: break plugin-sdk setup cycle

(cherry picked from commit 9b6859e)

* fix(test): reduce startup-heavy hotspot retention (openclaw#52381)

(cherry picked from commit dbd26e4)

* feat(feishu): structured cards with identity header, note footer, and streaming enhancements (openclaw#29938)

Verified:
- pnpm build
- pnpm check
- pnpm test:macmini

Co-authored-by: nszhsl <[email protected]>
Co-authored-by: Tak Hoffman <[email protected]>
(cherry picked from commit df3a247)

* test: dedupe feishu startup preflight waits

(cherry picked from commit 1ea5bba)

* test: flatten feishu docx hoisted mocks

(cherry picked from commit 285f903)

* Feishu: move outbound session routing behind plugin boundary

(cherry picked from commit 2a02337)

* fix(test): split feishu bot helpers

(cherry picked from commit 383d5ac)

* Security: preserve Feishu reaction chat type (openclaw#44088)

* Feishu: preserve looked-up chat type

* Feishu: fail closed on ambiguous reaction chats

* Feishu: cover reaction chat type fallback

* Changelog: note Feishu reaction hardening

* Feishu: fail closed without resolved chat type

* Feishu: normalize reaction chat type at runtime

(cherry picked from commit 3e730c0)

* test: reduce feishu reply dispatcher duplication

(cherry picked from commit 3ffb9f1)

* test: add Feishu ACP failure lifecycle regression

(cherry picked from commit 628b55a)

* Feishu: lazy-load runtime-heavy channel paths

(cherry picked from commit 66a8c25)

* fix(test): repair extensions lane regressions

(cherry picked from commit 75ab4db)

* test: add Feishu card-action lifecycle regression

(cherry picked from commit 7d50e7f)

* tests(feishu): inject client runtime seam

(cherry picked from commit 8448f48)

* test: dedupe feishu media account setup

(cherry picked from commit 8ca510a)

* test: dedupe feishu config schema checks

(cherry picked from commit 9a14696)

* refactor: share feishu reaction client setup

(cherry picked from commit a14a326)

* test: reuse feishu streaming merge helper

(cherry picked from commit a474a9c)

* test: dedupe feishu account resolution fixtures

(cherry picked from commit a7e5925)

* test: dedupe feishu probe fixtures

(cherry picked from commit b23bfef)

* test: add Feishu broadcast lifecycle regression

(cherry picked from commit c7cebd6)

* fix: adapt cherry-picks for fork TS strictness

Remove upstream-only files (runtime-api, setup-wizard, message-tool-legacy)
and fix import paths for fork's module structure.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* fix: remove setup-core.ts (DTS rootDir violation) and fix test mock path

- Remove setup-core.ts that caused DTS build failure (extensions/ outside rootDir)
- Update helpers.test.ts mock path from onboarding.js to setup.js (matching rename)

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* fix: remove extensions/ exports from plugin-sdk (DTS rootDir) and broken test

- Remove feishuOnboardingAdapter, zaloOnboardingAdapter, zalouserOnboardingAdapter
  re-exports from plugin-sdk/* (these pull extensions/ into DTS rootDir scope)
- Remove vitest-extensions-config.test.ts (tests non-existent function)

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

---------

Co-authored-by: Tak Hoffman <[email protected]>
Co-authored-by: Peter Steinberger <[email protected]>
Co-authored-by: Gustavo Madeira Santana <[email protected]>
Co-authored-by: Vincent Koc <[email protected]>
Co-authored-by: songlei <[email protected]>
Co-authored-by: nszhsl <[email protected]>
Co-authored-by: huntharo <[email protected]>
Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
furaul pushed a commit to furaul/openclaw that referenced this pull request Mar 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

channel: feishu Channel integration: feishu channel: msteams Channel integration: msteams maintainer Maintainer-authored PR scripts Repository scripts size: L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant