fix(workspace): bound in-memory workspace cache - #306
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change removes skill-read activation tracking and allows reads within advertised skill directories. It also adds a 32-entry LRU cache for workspaces and a regression test for resource resolution after eviction. ChangesSkill read resolution and workspace caching
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change bounds persisted workspace caching with LRU eviction and allows reads from advertised skill directories. The supplied regression coverage and cleanup update indicate no remaining merge-blocking risk. Sequence Diagram(s)sequenceDiagram
participant ReadTool
participant WorkspaceRegistry
participant SkillResolver
ReadTool->>WorkspaceRegistry: resolveReadPath(inputPath)
WorkspaceRegistry->>SkillResolver: resolveSkillReadPath(skills, inputPath)
SkillResolver-->>WorkspaceRegistry: matching skill and absolutePath
WorkspaceRegistry-->>ReadTool: resolved read path
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR bounds the persisted-backed workspace context cache to 32 LRU entries and preserves activated skill-directory state when an evicted workspace is restored.
Confidence Score: 4/5The PR should not merge until concurrent activation persistence can no longer discard a successfully loaded skill directory. The cache and restoration flow is otherwise coherent, but replacing a shared JSON activation list through an unprotected read-modify-write can lose durable state across concurrent store connections; legacy migration coverage is also missing. Files Needing Attention: src/workspace-store.ts, src/oauth-store.test.ts
|
| Filename | Overview |
|---|---|
| src/workspaces.ts | Implements LRU cache refresh and eviction while restoring persisted skill activation into reconstructed contexts. |
| src/workspace-store.ts | Adds durable skill activation, but its JSON read-modify-write can lose concurrent updates across database connections. |
| src/db/migrations.ts | Adds the activation-state column for fresh and existing databases through migration 7. |
| src/db/schema.ts | Extends the typed workspace-session schema with non-null JSON activation state. |
| src/workspaces.test.ts | Verifies eviction and restoration retain access to resources belonging to an activated skill. |
| src/oauth-store.test.ts | Updates the expected migration ledger but does not exercise the legacy upgrade path. |
Sequence Diagram
sequenceDiagram
participant C as Client
participant R as WorkspaceRegistry
participant DB as SQLite store
C->>R: Open/use workspace
R->>DB: Persist session
R->>R: Insert as most-recent cache entry
R->>R: Evict oldest entry above 32
C->>R: Read advertised SKILL.md
R->>DB: Persist activated skill directory
C->>R: Use evicted workspace ID
R->>DB: Load session and activations
R->>R: Rebuild context from current configuration
R-->>C: Restored workspace
Reviews (1): Last reviewed commit: "fix(db): tolerate partial legacy workspa..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| .select({ activatedSkillDirsJson: workspaceSessions.activatedSkillDirsJson }) | ||
| .from(workspaceSessions) | ||
| .where(eq(workspaceSessions.id, id)) | ||
| .get(); | ||
| if (!row) return; | ||
|
|
||
| const activatedSkillDirs = parseActivatedSkillDirs(row.activatedSkillDirsJson); | ||
| if (activatedSkillDirs.includes(skillDir)) return; | ||
| activatedSkillDirs.push(skillDir); | ||
| this.database.db | ||
| .update(workspaceSessions) | ||
| .set({ activatedSkillDirsJson: JSON.stringify(activatedSkillDirs) }) | ||
| .where(eq(workspaceSessions.id, id)) | ||
| .run(); |
There was a problem hiding this comment.
When two registry or store instances sharing a state directory activate different skills for the same workspace concurrently, both can read the same JSON value and the later update can overwrite the earlier activation. After cache eviction or restart, a successfully loaded skill may lose its persisted activation, causing later resource reads to be rejected. Serialize this read-modify-write operation or store activations in a form that supports atomic updates.
Knowledge Base Used: Local persistence
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
bdf6353 to
5d2cfda
Compare
1 participant
#218 correctly identified a second retention layer after the MCP transport work:
WorkspaceRegistrykept every opened workspace context resident even though workspace sessions are already persisted. This caps the persisted-backed cache at 32 entries with LRU restoration byworkspaceId, so old contexts can leave memory without invalidating workspace handles.Skill reads are now derived directly from the current advertised skill catalog: files within an advertised skill directory are readable without tracking a hidden per-workspace activation state. That keeps cache eviction semantically invisible and avoids adding any new database migration or persisted skill state.
This intentionally leaves #218's heap snapshots, health metrics, stale-binding cleanup, and old MCP-session timers out; #201 already removed retained MCP transport sessions.
Summary by CodeRabbit
New Features
SKILL.md.Documentation
Tests