fix(plugin): add timeout protection to auto-recall in openclaw-plugin#750
fix(plugin): add timeout protection to auto-recall in openclaw-plugin#750Jah-yee wants to merge 1 commit intovolcengine:mainfrom
Conversation
Fixes issue volcengine#748 - The auto-recall logic in examples/openclaw-plugin/index.ts was missing timeout protection, causing the agent to hang indefinitely if OpenViking HTTP calls hang during transient connection issues. This fix: - Imports withTimeout from process-manager.js - Adds AUTO_RECALL_TIMEOUT_MS constant (5000ms) - Wraps the entire auto-recall block in withTimeout - Updates catch block to mention 'timed out' for clarity This is the same fix that was applied to openclaw-memory-plugin in PR volcengine#688.
|
same to #752, close this pr |
qin-ptr
left a comment
There was a problem hiding this comment.
Review Summary
The code correctly fixes the timeout issue described in #748 by wrapping the auto-recall logic with withTimeout(). However, there is a significant inconsistency between the PR description and the actual implementation regarding configuration fields.
Blocking Issue
- PR description claims to have added config field and UI support, but the code only adds a hardcoded local constant
Non-blocking Suggestions
- Consider making the timeout value configurable via config schema
- Improve error message accuracy by distinguishing timeout errors from other failures
🤖 I am a bot owned by @qin-ctx.
| @@ -430,6 +431,8 @@ const contextEnginePlugin = { | |||
|
|
|||
| const prependContextParts: string[] = []; | |||
|
|
|||
There was a problem hiding this comment.
[Design] (blocking)
PR description claims:
"Added DEFAULT_AUTO_RECALL_TIMEOUT_MS (5000ms) to config"
"Added autoRecallTimeoutMs config field with UI support"
But the actual code only adds a hardcoded local constant AUTO_RECALL_TIMEOUT_MS here. The config schema (memoryOpenVikingConfigSchema) was not modified, and there is no UI support added.
This inconsistency is misleading for reviewers and future maintainers. Please either:
- Update the PR description to remove claims about config/UI changes, or
- Actually implement the configurable timeout by adding
autoRecallTimeoutMsto the config schema
The constant name also doesn't match: AUTO_RECALL_TIMEOUT_MS vs DEFAULT_AUTO_RECALL_TIMEOUT_MS.
| @@ -430,6 +431,8 @@ const contextEnginePlugin = { | |||
|
|
|||
| const prependContextParts: string[] = []; | |||
|
|
|||
There was a problem hiding this comment.
[Suggestion] (non-blocking)
The timeout value is hardcoded to 5000ms. Different deployment environments (high-latency networks, slow servers) may require different timeout values.
Consider adding a config field like:
autoRecallTimeoutMs: Type.Number({
default: 5000,
description: "Timeout for auto-recall in ms"
})Then use cfg.autoRecallTimeoutMs here instead of the hardcoded constant.
| } | ||
| })(), | ||
| AUTO_RECALL_TIMEOUT_MS, | ||
| `openviking: auto-recall timed out after ${AUTO_RECALL_TIMEOUT_MS}ms`, |
There was a problem hiding this comment.
[Suggestion] (non-blocking)
The error message says "failed or timed out" for all errors, even when the error is not caused by timeout. This could be misleading when debugging.
Consider checking the error message or type:
catch (err) {
const msg = String(err);
const isTimeout = msg.includes('timed out');
api.logger.warn(
`openviking: auto-recall ${isTimeout ? 'timed out' : 'failed'}: ${msg}`
);
}
Summary
Fixes
Fixes issue #748
Testing
The fix follows the same pattern as PR #688 which fixed the same issue in the original openclaw-memory-plugin location.