Skip to content

fix(active-memory): gracefully degrade on timeout instead of failing entire reply#69485

Merged
steipete merged 2 commits intoopenclaw:mainfrom
Magicray1217:fix/issue-66849-active-memory
Apr 21, 2026
Merged

fix(active-memory): gracefully degrade on timeout instead of failing entire reply#69485
steipete merged 2 commits intoopenclaw:mainfrom
Magicray1217:fix/issue-66849-active-memory

Conversation

@Magicray1217
Copy link
Copy Markdown
Contributor

Problem\n\nAfter upgrading to 2026.4.14, existing conversations fail with 'Something went wrong'. Gateway logs show repeated active-memory timeouts on the pre-reply path.\n\n## Root cause\n\nThe before_prompt_build hook had no top-level error guard. Any rejection from isSessionActiveMemoryDisabled, persistPluginStatusLines, or maybeResolveActiveRecall (including abort edge cases) propagated up to the plugin framework, surfacing as a fatal error.\n\n## Fix\n\nWrapped the entire before_prompt_build handler in try/catch. On any error, logs a warn-level message and returns undefined so the reply continues without memory context.\n\nAdded test coverage for the simultaneous rejection case.\n\nFixes #66849

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 20, 2026

Greptile Summary

Wraps the entire before_prompt_build event handler in a try/catch so that any rejection from isSessionActiveMemoryDisabled, persistPluginStatusLines, or maybeResolveActiveRecall is caught, logged at warn level, and gracefully degraded to undefined instead of surfacing as a fatal plugin error. A new test verifies the catch-and-warn path.

Confidence Score: 5/5

Safe to merge — the fix is targeted, correct, and consistent with existing error-handling conventions in the file.

Only a P2 style observation remains (possibly superfluous second mock rejection in the new test). No logic, data integrity, or security concerns were found.

No files require special attention.

Prompt To Fix All With AI
This is a comment left during a code review.
Path: extensions/active-memory/index.test.ts
Line: 1151-1152

Comment:
**Potentially unconsumed mock rejection**

The test queues two `mockRejectedValueOnce` entries, but the first failure (`runEmbeddedPiAgent`) will likely cause `maybeResolveActiveRecall` to reject before `updateSessionStore` is ever called, leaving the second mock rejection unconsumed. `vi.clearAllMocks()` in `beforeEach` prevents this from polluting later tests, so there's no reliability risk — but the second mock gives the false impression the test verifies a concurrent-failure scenario. If the intent is truly to test simultaneous parallel rejection, a comment explaining which internal `Promise.all` call triggers both would clarify the setup.

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

Reviews (1): Last reviewed commit: "fix(active-memory): gracefully degrade o..." | Re-trigger Greptile

Comment thread extensions/active-memory/index.test.ts Outdated
Comment on lines +1151 to +1152
runEmbeddedPiAgent.mockRejectedValueOnce(new Error("network reset"));
hoisted.updateSessionStore.mockRejectedValueOnce(new Error("store unavailable"));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Potentially unconsumed mock rejection

The test queues two mockRejectedValueOnce entries, but the first failure (runEmbeddedPiAgent) will likely cause maybeResolveActiveRecall to reject before updateSessionStore is ever called, leaving the second mock rejection unconsumed. vi.clearAllMocks() in beforeEach prevents this from polluting later tests, so there's no reliability risk — but the second mock gives the false impression the test verifies a concurrent-failure scenario. If the intent is truly to test simultaneous parallel rejection, a comment explaining which internal Promise.all call triggers both would clarify the setup.

Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/active-memory/index.test.ts
Line: 1151-1152

Comment:
**Potentially unconsumed mock rejection**

The test queues two `mockRejectedValueOnce` entries, but the first failure (`runEmbeddedPiAgent`) will likely cause `maybeResolveActiveRecall` to reject before `updateSessionStore` is ever called, leaving the second mock rejection unconsumed. `vi.clearAllMocks()` in `beforeEach` prevents this from polluting later tests, so there's no reliability risk — but the second mock gives the false impression the test verifies a concurrent-failure scenario. If the intent is truly to test simultaneous parallel rejection, a comment explaining which internal `Promise.all` call triggers both would clarify the setup.

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

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@steipete steipete force-pushed the fix/issue-66849-active-memory branch from 41e5cd2 to d4b32a6 Compare April 21, 2026 01:43
steipete added a commit to Magicray1217/openclaw that referenced this pull request Apr 21, 2026
@steipete steipete force-pushed the fix/issue-66849-active-memory branch from d4b32a6 to 077ceff Compare April 21, 2026 02:13
@steipete steipete merged commit cbdd6a4 into openclaw:main Apr 21, 2026
91 checks passed
@steipete
Copy link
Copy Markdown
Contributor

Landed after maintainer review and regression coverage update.

  • Gate: pnpm test extensions/active-memory/index.test.ts; pnpm check:changed; GitHub CI green
  • Source SHA: 077ceff
  • Merge SHA: cbdd6a4

Thanks @Magicray1217!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants