Skip to content

fix: prevent reflection loop with global cross-instance re-entrant guard#369

Merged
rwmjhb merged 2 commits intoCortexReach:masterfrom
ggzeng:fix/reflection-loop-global-guard
Apr 3, 2026
Merged

fix: prevent reflection loop with global cross-instance re-entrant guard#369
rwmjhb merged 2 commits intoCortexReach:masterfrom
ggzeng:fix/reflection-loop-global-guard

Conversation

@ggzeng
Copy link
Copy Markdown
Contributor

@ggzeng ggzeng commented Mar 26, 2026

Problem

Each plugin instance maintained its own re-entrant guard Map. When the runtime re-loaded the plugin during embedded agent turns (e.g. command:new inside a reflection), a new instance would bypass the guard, causing infinite reflection loops.

Solution

Introduce two global guards using Symbol.for + globalThis so all plugin instances share the same state:

  1. Global re-entrant lock — prevents concurrent reflection calls for the same sessionKey across all plugin instances.
  2. Serial loop guard — imposes a 2-minute cooldown per sessionKey between consecutive reflection runs, preventing gateway-level re-triggering chains (session_end → new session → command:new).

Changes

  • Only index.ts modified — no dependency changes.
  • Guards are initialized lazily via Symbol.for keys, safe for multiple loads.
  • Cooldown is configurable via SERIAL_GUARD_COOLDOWN_MS constant (currently 120s).

Previously each plugin instance maintained its own re-entrant guard Map.
When the runtime re-loaded the plugin during embedded agent turns (e.g.
command:new inside a reflection), a new instance would bypass the guard,
causing infinite reflection loops.

This change introduces two global guards using Symbol.for + globalThis
so ALL plugin instances share the same state:

1. **Global re-entrant lock** — prevents concurrent reflection calls for
   the same sessionKey across all plugin instances.
2. **Serial loop guard** — imposes a 2-minute cooldown per sessionKey
   between consecutive reflection runs, preventing gateway-level
   re-triggering chains (session_end → new session → command:new).
Copy link
Copy Markdown
Collaborator

@AliceLJY AliceLJY left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — clean, minimal, targeted fix.

Two global guards via Symbol.for + globalThis is the right approach for cross-instance protection. The 2-minute serial cooldown handles the gateway re-triggering chain well.

+41 -0, single file, no dependencies — exactly the kind of surgical fix this problem needs.

@rwmjhb ready for merge.

@rwmjhb
Copy link
Copy Markdown
Collaborator

rwmjhb commented Mar 30, 2026

Re-checked this more carefully.

The cross-instance Symbol.for + globalThis lock still looks like the right direction. The issue is narrower: the new serial cooldown is currently recorded even when reflection exits early, not only when a reflection run actually completes.

Concretely:

  • cooldown check happens before the body at runMemoryReflection
  • the timestamp is then written unconditionally in finally

That means early-return paths such as:

  • missing session file after recovery
  • empty/unusable conversation
  • other pre-generation exits

still arm the 120s cooldown.

I reproduced this on latest master + this PR with a realistic hook context (cfg present, workspaceDir present, sessionEntry.sessionId present) where the session file was not yet recoverable:

  1. first call reaches missing session file after recovery
  2. second call with the same sessionKey is immediately skipped by the serial guard

On current master, the second call retries normally.

So my updated take is:

  • the global re-entrant lock is good
  • the serial guard is too broad in its current placement
  • I would not merge this as-is unless the cooldown is only recorded after a successful reflection run, or otherwise narrowed to the specific loop case it intends to block

Copy link
Copy Markdown
Collaborator

@AliceLJY AliceLJY left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — changes are clean, on-topic, and well-tested. Approving.

将 cooldown 时间戳的记录从 finally 无条件执行改为仅当 reflection
通过所有前置条件检查(cfg、session file、conversation)后才记录。
避免因前置条件不满足导致的提前退出误触 cooldown,阻塞后续正常重试。

回应 @rwmjhbCortexReach#369 的审查反馈。
@ggzeng
Copy link
Copy Markdown
Contributor Author

ggzeng commented Apr 1, 2026

Addressing @rwmjhb's feedback on serial cooldown placement

Good catch. The issue was that getSerialGuardMap().set(sessionKey, Date.now()) in the finally block fires unconditionally — even when reflection exits early due to missing session file, empty conversation, or other pre-condition failures.

Fix (8dbdcb5)

Added a reflectionRan flag that is set to true only after all pre-condition checks pass (cfg present, session file recovered, conversation usable). The serial cooldown is now only recorded when reflectionRan === true in the finally block.

This means:

  • Pre-condition early exits (missing cfg, session file, conversation) → no cooldown recorded, subsequent calls retry normally
  • Reflection runs (even if it throws during generation/storage) → cooldown recorded, preventing re-trigger loop

The global re-entrant lock cleanup (getGlobalReflectionLock().delete(sessionKey)) remains unconditional in finally since it must always be released regardless of how the function exits.

@rwmjhb rwmjhb merged commit 4878abd into CortexReach:master Apr 3, 2026
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants