fix: prevent stack overflow in cache-trace stableStringify on circular refs#38935
fix: prevent stack overflow in cache-trace stableStringify on circular refs#38935MumuTW wants to merge 1 commit intoopenclaw:mainfrom
Conversation
…r refs Add WeakSet-based circular reference detection to stableStringify(), preventing RangeError when digesting messages that contain circular object references (e.g. image attachments near context limit). Fixes openclaw#38859
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 959af8af59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (seen.has(value)) { | ||
| return JSON.stringify("[Circular]"); | ||
| } | ||
| seen.add(value); |
There was a problem hiding this comment.
seen is never cleared after descending into a branch, so any repeated reference is treated as "[Circular]" even when there is no cycle (for example, two fields pointing to the same object). In summarizeMessages(), this changes digests based on object identity rather than message content and can collapse distinct payload shapes to the same fingerprint, reducing cache-trace accuracy for debugging. This was introduced by the new seen.has/seen.add logic and should use recursion-path tracking (or delete from seen on unwind) to mark only true cycles.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR fixes a
Confidence Score: 4/5
Last reviewed commit: 959af8a |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (seen.has(value)) { | ||
| return JSON.stringify("[Circular]"); | ||
| } | ||
| seen.add(value); |
There was a problem hiding this comment.
Shared references produce false "[Circular]" in fingerprints
Because seen is shared across the entire traversal, any object (or array) that is referenced from two different places in the same message — a diamond-shaped graph that is not actually circular — will be serialized as "[Circular]" on its second occurrence. This can silently produce incorrect messageFingerprints for valid data.
For example:
const shared = { type: "text", text: "hello" };
const msg = { role: "user", content: [shared, shared] }; // not circular
stableStringify(msg);
// → {"content":[{"text":"hello","type":"text"},"[Circular]"],"role":"user"}
// instead of the correct:
// → {"content":[{"text":"hello","type":"text"},{"text":"hello","type":"text"}],"role":"user"}Since the fingerprints are only used for diagnostics/tracing (not actual cache-control decisions), the impact is low. However, it's worth being aware of, especially if messageFingerprints are later used for anything more critical. The same trade-off exists in redactImageDataForDiagnostics(), so this is at least consistent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/cache-trace.ts
Line: 120-123
Comment:
**Shared references produce false `"[Circular]"` in fingerprints**
Because `seen` is shared across the entire traversal, any object (or array) that is referenced from **two different places** in the same message — a diamond-shaped graph that is *not* actually circular — will be serialized as `"[Circular]"` on its second occurrence. This can silently produce incorrect `messageFingerprints` for valid data.
For example:
```ts
const shared = { type: "text", text: "hello" };
const msg = { role: "user", content: [shared, shared] }; // not circular
stableStringify(msg);
// → {"content":[{"text":"hello","type":"text"},"[Circular]"],"role":"user"}
// instead of the correct:
// → {"content":[{"text":"hello","type":"text"},{"text":"hello","type":"text"}],"role":"user"}
```
Since the fingerprints are only used for diagnostics/tracing (not actual cache-control decisions), the impact is low. However, it's worth being aware of, especially if `messageFingerprints` are later used for anything more critical. The same trade-off exists in `redactImageDataForDiagnostics()`, so this is at least consistent.
How can I resolve this? If you propose a fix, please make it concise.| expect(lines.length).toBe(1); | ||
| const event = JSON.parse(lines[0]?.trim() ?? "{}") as Record<string, unknown>; | ||
| expect(event.messageCount).toBe(1); | ||
| expect(event.messageFingerprints).toHaveLength(1); | ||
| }); |
There was a problem hiding this comment.
Test could assert "[Circular]" appears in the fingerprint
The test correctly checks that no stack overflow occurs and that a single event line is produced, but it doesn't assert that the circular reference is actually replaced with the "[Circular]" sentinel in the serialized fingerprint. Adding an assertion would more directly confirm the fix works as intended:
expect(event.messageFingerprints).toHaveLength(1);
// Verify the circular reference is replaced rather than causing incorrect output
const fp = event.messageFingerprints as string[];
expect(fp[0]).toBeTruthy(); // fingerprint is a non-empty sha256 hex stringOr, if you want to verify the placeholder itself ends up in the serialized form, you could expose a small unit test on stableStringify directly (via a test-only export or a separate describe block for the helper). Either way, the current test is a solid regression guard — this is just a suggestion to strengthen it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/cache-trace.test.ts
Line: 173-177
Comment:
**Test could assert `"[Circular]"` appears in the fingerprint**
The test correctly checks that no stack overflow occurs and that a single event line is produced, but it doesn't assert that the circular reference is actually replaced with the `"[Circular]"` sentinel in the serialized fingerprint. Adding an assertion would more directly confirm the fix works as intended:
```ts
expect(event.messageFingerprints).toHaveLength(1);
// Verify the circular reference is replaced rather than causing incorrect output
const fp = event.messageFingerprints as string[];
expect(fp[0]).toBeTruthy(); // fingerprint is a non-empty sha256 hex string
```
Or, if you want to verify the placeholder itself ends up in the serialized form, you could expose a small unit test on `stableStringify` directly (via a test-only export or a separate `describe` block for the helper). Either way, the current test is a solid regression guard — this is just a suggestion to strengthen it.
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!
gambletan
left a comment
gambletan
left a comment
There was a problem hiding this comment.
Review: cache-trace stableStringify circular reference fix
Overall: Good fix, clean implementation. ✅
What works well
- Using
WeakSet<object>is the correct choice — it avoids memory leaks since entries are garbage-collected when no longer referenced. - The
[Circular]sentinel is JSON-safe and clearly communicates what happened. - The default parameter
seen: WeakSet<object> = new WeakSet()keeps the public API unchanged — callers don't need to know about the cycle tracking. - Test case correctly creates a real circular reference and verifies no crash.
Minor observations
-
The
seenset is shared across the entire recursive tree. This means if the same object appears in two independent branches (not a cycle, just shared reference), it will be serialized as[Circular]on the second encounter. For a diagnostic tracing function this is fine — stability and crash prevention matter more than perfect fidelity. Just noting this is a deliberate trade-off. -
New
ErrorandUint8Arraywrapper objects are not added toseenbefore recursing (lines ~127–140). These freshly created plain objects can't be circular, so this is correct. Good. -
The test could optionally verify the output content (e.g., that the fingerprint contains
[Circular]), but since the function is internal diagnostic plumbing, verifying "no crash + correct count" is sufficient.
LGTM — clean, minimal fix for a real crash scenario. 👍
mdlmarkham
left a comment
mdlmarkham
left a comment
There was a problem hiding this comment.
Review: Circular Reference Stack Overflow Fix ✅
Verdict: Clean fix for a critical crash. Ready to merge.
The Bug
stableStringify() in cache-trace uses deep recursion. When a message contains circular references (e.g., image attachments with parent back-references), the function recurses infinitely until RangeError: Maximum call stack size exceeded.
The Fix
function stableStringify(value: unknown, seen: WeakSet<object> = new WeakSet()): string {
// ...
if (seen.has(value)) {
return JSON.stringify("[Circular]");
}
seen.add(value);
// ... recurse with seen
}Why WeakSet: Tracks objects only, doesn't prevent GC, O(1) lookup, doesn't leak memory.
Pattern Consistency
This mirrors redactImageDataForDiagnostics() in payload-redaction.ts:
// Same pattern already used elsewhere
const seen = new WeakSet();
if (seen.has(value)) return "[Circular]";Test Coverage
- ✅ Circular reference in messages no longer causes stack overflow
- ✅ Existing tests pass (6/6)
- ✅ Verify output contains
[Circular]placeholder, not crash
Edge Cases
- Array circular refs:
[{ref: parent}, parent]—seencatches the object ✅ - Deep nesting before cycle:
seentracks all visited objects ✅ - Primitives in cycle: WeakSet only tracks objects, primitives are fine ✅
Minor Observation
The fix replaces circular refs with "[Circular]". If downstream consumers parse this string, they'll get a literal string, not an error. This is better than:
- Crashing (old behavior)
- Losing the entire object
If downstream needs to know which field was circular, that would require path tracking — but that's overkill for this use case.
Recommendation: Approve. Critical crash fix, follows established pattern, well-tested.
Co-authored-by: MumuTW <[email protected]>
|
Landed. Thank you @MumuTW. What I did:
SHA hashes:
Thanks again for the fix. |
* main: (133 commits) reduce image size, offer slim image (openclaw#38479) fix(security): harden install base drift cleanup fix(agents): respect explicit provider baseUrl in merge mode (openclaw#39103) fix(agents): apply contextTokens cap for compaction threshold (openclaw#39099) fix(exec): block dangerous override-only env pivots fix(security): stage installs before publish fix(daemon): normalise whitespace in checkTokenDrift to prevent false-positive warning (openclaw#39108) fix(security): harden fs-safe copy writes refactor: dedupe bluebubbles webhook auth test setup refactor: dedupe discord native command test scaffolding refactor: dedupe anthropic probe target test setup refactor: dedupe minimax provider auth test setup refactor: dedupe runtime snapshot test fixtures fix: harden zip extraction writes fix(tests): stabilize diffs localReq headers (supersedes openclaw#39063) fix: harden workspace skill path containment fix(agents): land openclaw#38935 from @MumuTW fix(models): land openclaw#38947 from @davidemanuelDEV fix(gateway): land openclaw#39064 from @Narcooo fix(models-auth): land openclaw#38951 from @MumuTW ...
Co-authored-by: MumuTW <[email protected]>
Co-authored-by: MumuTW <[email protected]>
Co-authored-by: MumuTW <[email protected]>
Co-authored-by: MumuTW <[email protected]>
Co-authored-by: MumuTW <[email protected]>
Co-authored-by: MumuTW <[email protected]>
Co-authored-by: MumuTW <[email protected]>
Co-authored-by: MumuTW <[email protected]>
Co-authored-by: MumuTW <[email protected]>
Co-authored-by: MumuTW <[email protected]> (cherry picked from commit 5effa60)
Co-authored-by: MumuTW <[email protected]> (cherry picked from commit 5effa60)
4 participants
Summary
WeakSet-based circular reference detection tostableStringify()insrc/agents/cache-trace.tsRangeError: Maximum call stack size exceededwhendigest()encounters circular object references in session messages (e.g. image attachments near context limit)redactImageDataForDiagnostics()insrc/agents/payload-redaction.tsTest plan
pnpm formatcleanFixes #38859