-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Switch to persistent Chroma HTTP server #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Greptile OverviewGreptile SummaryReplaces MCP subprocess approach with persistent HTTP server using Key architectural changes:
Integration points:
Configuration added:
Previous review comments about undefined Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant WS as WorkerService
participant CSM as ChromaServerManager
participant Server as Chroma Server (npx)
participant CS as ChromaSync
participant GS as GracefulShutdown
Note over WS: Worker Startup
WS->>CSM: getInstance(config)
CSM-->>WS: singleton instance
WS->>CSM: start()
CSM->>CSM: Check existing server (heartbeat)
alt Server already running
CSM-->>WS: reuse existing
else No server found
CSM->>Server: spawn npx chroma run
CSM->>Server: Poll heartbeat
Server-->>CSM: ready
end
WS->>CSM: waitForReady(60s)
CSM-->>WS: server ready
Note over CS: First Vector Operation
CS->>CSM: isServerReachable()
CSM->>Server: GET /api/v2/heartbeat
Server-->>CSM: 200 OK
CSM-->>CS: true
CS->>Server: HTTP operations (add/query)
Note over WS: Worker Shutdown
WS->>GS: performGracefulShutdown()
GS->>CSM: stop()
alt Unix/Linux/macOS
CSM->>Server: kill(-pid, SIGTERM)
else Windows
CSM->>Server: kill(SIGTERM)
end
alt Graceful exit
Server-->>CSM: exit event
else Timeout (5s)
CSM->>Server: kill(SIGKILL)
end
CSM-->>GS: stopped
GS-->>WS: shutdown complete
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, 1 comment
| } else { | ||
| // Unix: kill the process group to ensure all children are killed | ||
| try { | ||
| process.kill(-pid!, 'SIGTERM'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pid could be undefined if the process was never assigned a PID. The non-null assertion on undefined will cause process.kill(-undefined!) to pass NaN to kill().
| process.kill(-pid!, 'SIGTERM'); | |
| if (pid !== undefined) { | |
| try { | |
| process.kill(-pid, 'SIGTERM'); | |
| } catch (err) { | |
| // Process group kill failed, try direct kill | |
| proc.kill('SIGTERM'); | |
| } | |
| } else { | |
| proc.kill('SIGTERM'); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/services/sync/ChromaServerManager.ts
Line: 215:215
Comment:
`pid` could be undefined if the process was never assigned a PID. The non-null assertion on undefined will cause `process.kill(-undefined!)` to pass `NaN` to kill().
```suggestion
if (pid !== undefined) {
try {
process.kill(-pid, 'SIGTERM');
} catch (err) {
// Process group kill failed, try direct kill
proc.kill('SIGTERM');
}
} else {
proc.kill('SIGTERM');
}
```
How can I resolve this? If you propose a fix, please make it concise.4c5d517 to
2f3f50b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 file reviewed, 1 comment
- Create standalone dist/sdk/ module with parseObservations, buildObservationPrompt, parseSummary - Add package.json exports for 'claude-mem/sdk' and 'claude-mem/modes/*' - Add TypeScript declarations Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…propagation (Issue thedotmack#737) (thedotmack#806) * Fix zombie process accumulation (Issue thedotmack#737) Problem: Claude haiku subprocesses spawned by the SDK weren't terminating properly, causing zombie process accumulation (user reported 155 processes consuming 51GB RAM). Root causes: 1. SDK's SpawnedProcess interface hides subprocess PIDs 2. deleteSession() didn't verify subprocess exit 3. abort() was fire-and-forget with no confirmation 4. No mechanism to track or clean up orphaned processes Solution: - Add ProcessRegistry module to track spawned Claude subprocesses - Use SDK's spawnClaudeCodeProcess option to capture PIDs via custom spawn - Pass signal parameter to enable AbortController integration - Wait for subprocess exit in deleteSession() with 5s timeout - Escalate to SIGKILL if graceful exit fails - Add orphan reaper running every 5 minutes as safety net Files changed: - src/services/worker/ProcessRegistry.ts (new): PID registry and reaper - src/services/worker/SDKAgent.ts: Use custom spawn to capture PIDs - src/services/worker/SessionManager.ts: Verify subprocess exit on delete - src/services/worker-service.ts: Start/stop orphan reaper Fixes thedotmack#737 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: address code review feedback - Replace busy-wait polling with event-based proc.once('exit') - Detect and warn about multiple processes per session (race condition) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: bigphoot <bigphoot@gmail.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…dotmack#794) (thedotmack#813) The isDirectChild() function failed to match files when the API used absolute paths (/Users/x/project/app/api) but the database stored relative paths (app/api/router.py). This caused all folder CLAUDE.md files to incorrectly show "No recent activity". Changes: - Create shared path-utils module with proper path normalization - Implement suffix matching strategy for mixed path formats - Update SessionSearch.ts to use shared utilities - Update regenerate-claude-md.ts to use shared utilities (was using outdated broken logic) - Prevent spurious directory creation from malformed paths - Add comprehensive test coverage for path matching edge cases This is the proper fix for thedotmack#794, replacing PR thedotmack#809 which only masked the bug by skipping file creation when "no activity" was shown. Co-authored-by: bigphoot <bigphoot@gmail.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace MCP subprocess approach with persistent Chroma HTTP server for improved performance and reliability. This re-enables Chroma on Windows by eliminating the subprocess spawning that caused console popups. Changes: - NEW: ChromaServerManager.ts - Manages local Chroma server lifecycle via `npx chroma run` - REFACTOR: ChromaSync.ts - Uses chromadb npm package's ChromaClient instead of MCP subprocess (removes Windows disabling) - UPDATE: worker-service.ts - Starts Chroma server on initialization - UPDATE: GracefulShutdown.ts - Stops Chroma server on shutdown - UPDATE: SettingsDefaultsManager.ts - New Chroma configuration options - UPDATE: build-hooks.js - Mark optional chromadb deps as external Benefits: - Eliminates subprocess spawn latency on first query - Single server process instead of per-operation subprocesses - No Python/uvx dependency for local mode - Re-enables Chroma vector search on Windows - Future-ready for cloud-hosted Chroma (claude-mem pro) - Cross-platform: Linux, macOS, Windows Configuration: CLAUDE_MEM_CHROMA_MODE=local|remote CLAUDE_MEM_CHROMA_HOST=127.0.0.1 CLAUDE_MEM_CHROMA_PORT=8000 CLAUDE_MEM_CHROMA_SSL=false Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Updated chromadb from ^1.9.2 to ^3.2.2 (includes CLI binary) - Changed heartbeat endpoint from /api/v1 to /api/v2 The 1.9.x version did not include the CLI, causing `npx chroma run` to fail. Version 3.2.2 includes the chroma CLI and uses the v2 API. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Added @chroma-core/default-embed dependency for local embeddings - Updated ChromaSync to use DefaultEmbeddingFunction with collections - Added isServerReachable() async method for reliable server detection - Fixed start() to detect and reuse existing Chroma servers - Updated build script to externalize native ONNX binaries - Added runtime dependency to plugin/package.json The embedding function uses all-MiniLM-L6-v2 model locally via ONNX, eliminating need for external embedding API calls. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Wire tenant, database, and API key settings into ChromaSync for remote/pro mode. In remote mode: - Passes tenant and database to ChromaClient for data isolation - Adds Authorization header when API key is configured - Logs tenant isolation connection details Local mode unchanged - uses default_tenant without explicit params. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Claude Code's plugin discovery looks for plugin.json at the marketplace root level in .claude-plugin/, not nested inside plugin/.claude-plugin/. Without this file at the root level, skills and commands are not discovered. This matches the structure of working plugins like claude-research-team. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
c5d1730 to
8dd4d15
Compare
Summary
chromadbnpm package's built-inChromaClientandnpx chroma runChanges
ChromaServerManager.ts- Manages local Chroma server lifecycleChromaSync.ts- Uses ChromaClient instead of MCPworker-service.ts- Starts server on initializationGracefulShutdown.ts- Stops server on shutdownSettingsDefaultsManager.ts- New Chroma configuration optionsBenefits
Configuration
Test Plan
🤖 Generated with Claude Code