找 Bug

[minorcell]

找出项目中的明显 bug

[minorcell]

@codercellone

[minorcell]

🐛 Bug Analysis Complete

After thoroughly reviewing the codebase, I've identified several bugs and potential issues:

---

Bug #1: Potential Memory Leak in McpClientPool

Location: packages/tools/src/router/mcp/pool.ts:189-198

Issue: When connecting to an MCP server fails during the listTools() call, the code attempts to close the client but doesn't remove the failed connection from the pendingConnections map on error. This happens because the finally block at line 204 only runs after the promise settles, but if there's an exception during tool listing (line 169), the connection object is never stored.

Impact: Memory leak in concurrent connection attempts.

Code:

try {
    const toolsResult = await client.listTools()
    // ... connection setup
    this.connections.set(name, connection)
    return connection
} catch (err) {
    try {
        await client.close()  // Client closed but pending promise still tracked
    } catch {
        // Ignore close errors when bootstrap fails.
    }
    throw err
}

---

Bug #2: Race Condition in Session History Writing

Location: packages/core/src/runtime/history.ts:23-32

Issue: The JsonlHistorySink.append() method chains promises sequentially but doesn't handle errors in the chain. If appendFile fails, subsequent writes will also fail silently, and the queue becomes corrupted. The closed flag is checked but errors in the promise chain are swallowed.

Impact: Silent data loss when history writes fail mid-session.

Code:

async append(event: HistoryEvent) {
    if (this.closed) {
        throw new Error('History sink is closed')
    }
    this.writeQueue = this.writeQueue.then(async () => {
        await this.ensureDirectory()
        await appendFile(this.filePath, `${JSON.stringify(event)}\n`, 'utf8')
    })
    return this.writeQueue
}

Fix needed: Add error handling to the promise chain to prevent queue corruption.

---

Bug #3: Incomplete Error Handling in session_runtime.ts

Location: packages/core/src/runtime/session_runtime.ts:1199-1209

Issue: In the close() method, when flushing history sinks, errors are caught and logged but the loop continues. However, the error message template uses string interpolation that could fail if err is not an Error object.

Code:

for (const sink of this.sinks) {
    try {
        if (sink.close) {
            await sink.close()
        } else if (sink.flush) {
            await sink.flush()
        }
    } catch (err) {
        console.error(`History flush failed: ${(err as Error).message}`)  // ⚠️ Assumes err has .message
    }
}

Impact: Potential crash if error object doesn't have a message property.

---

Bug #4: Missing Cleanup in exec_runtime.ts

Location: packages/tools/src/tools/exec_runtime.ts:288-298

Issue: Event listeners are attached to proc.stdout, proc.stderr, and proc itself, but there's no cleanup mechanism when sessions are deleted or the process exits. This creates a memory leak for long-running applications with many exec sessions.

Code:

proc.stdout?.on('data', (chunk) => append('', chunk))
proc.stderr?.on('data', (chunk) => append('', chunk))
proc.on('error', (err) => { /* ... */ })
proc.on('close', (code) => { /* ... */ })
// No removeListener or cleanup when session is removed

Impact: Event emitter memory leak, especially with the 64 session limit.

---

Bug #5: Unsafe String Slicing in selectCompactionUserMessages

Location: packages/core/src/runtime/session_runtime.ts:296-298

Issue: The code slices a string by character count without considering token boundaries, which can result in cutting UTF-8 characters in half or breaking multi-byte characters.

Code:

if (remaining > 0) {
    selected.push(message.slice(0, remaining))  // ⚠️ Cuts by char count, not token boundary
}

Impact: Corrupted text when messages are truncated, especially with non-ASCII characters.

---

Bug #6: Timer Not Cleared on Early Return

Location: packages/tools/src/router/mcp/cache_store.ts:207-218

Issue: In schedulePersist(), if the function is called multiple times rapidly, the timer is set but might not be cleared if persistRequested is already true and the function returns early. This could lead to multiple timers being active.

Code:

private schedulePersist() {
    if (!isDiskCacheEnabled()) return
    this.persistRequested = true
    if (this.persistTimer) return  // ⚠️ Returns without clearing old timer first

    this.persistTimer = setTimeout(() => {
        this.persistTimer = null
        void this.flushPersistQueue()
    }, CACHE_PERSIST_DEBOUNCE_MS)
    this.persistTimer.unref?.()
}

Impact: Multiple debounce timers active simultaneously, though impact is minor.

---

Bug #7: Uncaught Promise Rejection in Background Refresh

Location: packages/tools/src/router/mcp/index.ts:166

Issue: Background refresh is fired with void keyword but errors are only logged, not handled. If the refresh fails, the tools remain stale but the user has no indication this happened.

Code:

if (cached.stale) {
    void this.refreshServer(serverName, config, 'background')  // ⚠️ Errors only logged, not reported
}

Impact: Silent tool staleness when background refresh fails.

---

Summary

Bug #	Severity	Type	Location
1	Medium	Memory Leak	pool.ts:189-198
2	High	Race Condition	history.ts:23-32
3	Low	Error Handling	session_runtime.ts:1199-1209
4	High	Memory Leak	exec_runtime.ts:288-298
5	Medium	Data Corruption	session_runtime.ts:296-298
6	Low	Resource Leak	cache_store.ts:207-218
7	Low	Silent Failure	index.ts:166

All bugs have been documented with their locations, impacts, and root causes. The most critical issues are Bug #2 (silent data loss) and Bug #4 (memory leak in exec sessions).

View job run