feat: persistent message store with crash recovery - #17
Conversation
…) (#2) * feat(messaging): add in-memory message store Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com> * fix(messaging): fix import ordering and add thread cleanup test Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com> * feat(messaging): add send_message MCP tool Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com> * feat(messaging): add receive_messages MCP tool Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com> * feat: add DATA_DIR constant to config.ts for persistent message store (issue eclipse-che#8) * feat: add PVC manifest and register in kustomization (issue eclipse-che#8) * feat: mount PVC and set CHE_MCP_DATA_DIR env var in deployment (issue eclipse-che#8) * feat: replace in-memory Map with file-backed persistent store (issue eclipse-che#8) * test: add crash-recovery persistence tests for message store (issue eclipse-che#8) * test: update tool adapter tests to use initStore with file-backed store * fix: lint and type-check fixups after persistence store integration --------- Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a persistent messaging inbox store with send/receive functionality, exposed as new MCP tools ( ChangesMessaging Store Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant SendMessageTool
participant ReceiveMessagesTool
participant MessageStore
participant Disk
Client->>SendMessageTool: send_message(from, to, body, thread_id)
SendMessageTool->>MessageStore: sendMessage(from, to, body, thread_id)
MessageStore->>Disk: flushToDisk (write temp, rename)
MessageStore-->>SendMessageTool: message_id, thread_id
SendMessageTool-->>Client: message_id, thread_id
Client->>ReceiveMessagesTool: receive_messages(session_id, thread_id)
ReceiveMessagesTool->>MessageStore: receiveMessages(session_id, thread_id)
MessageStore->>Disk: flushToDisk (update inbox)
MessageStore-->>ReceiveMessagesTool: messages
ReceiveMessagesTool-->>Client: messages
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/tools.ts (1)
593-634: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding
destructiveHinttoreceive_messages.
receive_messagesconsumes (deletes) messages after reading, which is a destructive operation. Adding{ destructiveHint: true }as the fourth argument toserver.tool()would signal this to MCP clients, consistent with howdelete_workspaceandstop_agentare annotated.♻️ Proposed fix
async ({ session_id, thread_id }) => { try { const result = receiveMessagesTool({ session_id, thread_id }); return { content: [{ type: 'text', text: JSON.stringify(result) }] }; } catch (error) { return toolError(error); } }, + { destructiveHint: true }, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tools.ts` around lines 593 - 634, The receive_messages tool in tools.ts is destructive because it consumes inbox messages after reading, so update the server.tool() registration for receive_messages to include the destructiveHint flag. Use the existing send_message/receive_messages definitions as the anchor, and add the hint in the tool metadata consistently with other destructive tools like delete_workspace and stop_agent.deploy/deployment.yaml (1)
28-32: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winVolume and env wiring is correct.
Cross-file verification passed: PVC name
che-mcp-server-datamatchesdeploy/pvc.yaml, mount path/datamatchesCHE_MCP_DATA_DIR, and volume namedatais consistent betweenvolumeMountsandvolumes.Two pre-existing concerns worth noting (not introduced by this PR):
Missing
securityContext(Checkov CKV_K8S_20, CKV_K8S_23): The container runs withoutallowPrivilegeEscalation: falseor a non-rootrunAsUser. Consider adding asecurityContextblock to the container spec.Replica scaling constraint:
replicas: 1with aReadWriteOncePVC means scaling beyond 1 replica will cause additional pods to fail PVC mounting. If horizontal scaling is planned, considerReadWriteMany(requires a supporting storage class) or switching to a shared data store.Also applies to: 45-48
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/deployment.yaml` around lines 28 - 32, The deployment wiring is fine, but the review calls out two existing hardening/scaling gaps in the container spec: add a securityContext to the workload defined under the Deployment’s container template (for example, enforce non-root/disable privilege escalation), and avoid relying on replicas: 1 with a ReadWriteOnce PVC if future horizontal scaling is expected. Update the Deployment definition and related storage settings only if you plan to support more than one replica.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/messaging/store.ts`:
- Around line 19-35: `initStore` needs stronger crash-recovery and reload
handling. Always clear `inboxes` before loading from `dataFile`, then wrap the
restore path in validation/error handling so a corrupted `dataFile` or promoted
`tmpFile` cannot crash startup. In `initStore`, only `renameSync(tmpFile,
dataFile)` after confirming the tmp contents are valid JSON, and guard the
`JSON.parse` load path with a fallback that clears state instead of throwing.
Use the existing `dataFile`, `tmpFile`, and `inboxes` flow in `initStore` to
keep the fix localized.
---
Nitpick comments:
In `@deploy/deployment.yaml`:
- Around line 28-32: The deployment wiring is fine, but the review calls out two
existing hardening/scaling gaps in the container spec: add a securityContext to
the workload defined under the Deployment’s container template (for example,
enforce non-root/disable privilege escalation), and avoid relying on replicas: 1
with a ReadWriteOnce PVC if future horizontal scaling is expected. Update the
Deployment definition and related storage settings only if you plan to support
more than one replica.
In `@src/tools.ts`:
- Around line 593-634: The receive_messages tool in tools.ts is destructive
because it consumes inbox messages after reading, so update the server.tool()
registration for receive_messages to include the destructiveHint flag. Use the
existing send_message/receive_messages definitions as the anchor, and add the
hint in the tool metadata consistently with other destructive tools like
delete_workspace and stop_agent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ca2fed9-2b54-45a5-89f5-1ad08aaf124a
📒 Files selected for processing (13)
deploy/deployment.yamldeploy/kustomization.yamldeploy/pvc.yamlsrc/config.tssrc/kube/exec.tssrc/messaging/store.tssrc/tools.tssrc/tools/receive-messages.tssrc/tools/send-message.tstests/messaging/store-persistence.test.tstests/messaging/store.test.tstests/tools/receive-messages.test.tstests/tools/send-message.test.ts
- initStore: clear inboxes before load, validate tmp file JSON before promoting, handle corrupted data file gracefully (start empty) - receive_messages: add destructiveHint since it consumes messages - deployment: add securityContext (allowPrivilegeEscalation: false, runAsNonRoot: true) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
server.tool() signature is (name, desc, schema, extras, handler), not (name, desc, schema, handler, extras). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/messaging/store.ts (1)
112-117: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
flushToDiskfailure leaves in-memory state inconsistent with disk.
sendMessageandreceiveMessagesmutateinboxesbefore callingflushToDisk. IfwriteFileSyncorrenameSyncthrows (disk full, I/O error), the in-memory state is already updated but the disk retains stale data. The exception propagates to the caller, but on the next restartinitStoreloads the old disk state — silently losing the messages that were "sent" or "received" in-memory.Consider wrapping the flush in a try/catch and rolling back the in-memory mutation on failure, or restructuring so the serialized state is computed before mutation.
🔧 Suggested approach: snapshot before mutate, rollback on flush failure
function flushToDisk(): void { if (dataFile === '') return; const json = JSON.stringify(Array.from(inboxes.entries())); - writeFileSync(tmpFile, json, 'utf8'); - renameSync(tmpFile, dataFile); + try { + writeFileSync(tmpFile, json, 'utf8'); + renameSync(tmpFile, dataFile); + } catch (e) { + // Disk write failed — in-memory state is now ahead of disk. + // Log the error so operators can investigate; the next successful + // flush will reconcile disk with memory. + console.error('flushToDisk failed:', e); + } }Alternatively, snapshot the inbox state in
sendMessage/receiveMessagesbefore mutation and restore it ifflushToDiskthrows:export function sendMessage(from: string, to: string, body: string, thread_id?: string) { // ... build message ... const prevInbox = [...(inboxes.get(to) ?? [])]; inbox.push(message); inboxes.set(to, inbox); try { flushToDisk(); } catch (e) { inboxes.set(to, prevInbox); // rollback throw e; } return { message_id, thread_id: resolvedThreadId }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/messaging/store.ts` around lines 112 - 117, flushToDisk currently updates disk after in-memory mutations in sendMessage and receiveMessages, so a write/rename failure leaves inboxes diverged from persisted state. Update the sendMessage/receiveMessages flow to snapshot the prior inbox state (or otherwise stage the change) before mutating, then wrap the flushToDisk call in try/catch and restore the snapshot on failure before rethrowing. Keep the fix centered around the existing flushToDisk helper and the sendMessage/receiveMessages mutations to ensure in-memory state only commits when persistence succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/messaging/store.ts`:
- Around line 112-117: flushToDisk currently updates disk after in-memory
mutations in sendMessage and receiveMessages, so a write/rename failure leaves
inboxes diverged from persisted state. Update the sendMessage/receiveMessages
flow to snapshot the prior inbox state (or otherwise stage the change) before
mutating, then wrap the flushToDisk call in try/catch and restore the snapshot
on failure before rethrowing. Keep the fix centered around the existing
flushToDisk helper and the sendMessage/receiveMessages mutations to ensure
in-memory state only commits when persistence succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0dfda1f-7659-4da1-80b1-3773a38b7789
📒 Files selected for processing (3)
deploy/deployment.yamlsrc/messaging/store.tssrc/tools.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- deploy/deployment.yaml
- src/tools.ts
Wrap writeFileSync/renameSync in try/catch so a disk failure doesn't propagate to callers. In-memory state stays current; the next successful flush reconciles disk. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
|
Addressed the Chose the simpler approach (log and continue) over snapshot-and-rollback — this is a messaging store where eventual consistency on next flush is acceptable, and rollback would add complexity to both Re: image build skipped — the |
1 participant
Summary
send_messageandreceive_messagesMCP tools for inter-agent communicationDATA_DIRconfig constant for persistent storageChanges
src/messaging/store.ts— message store with file persistence and thread managementsrc/tools/send-message.ts— MCP tool to send messages between agentssrc/tools/receive-messages.ts— MCP tool to receive messages from a threadsrc/tools.ts— register new toolssrc/config.ts— addDATA_DIRconstantdeploy/pvc.yaml— PersistentVolumeClaim for message storagedeploy/deployment.yaml— volume mount for data directoryTest plan
tests/messaging/store.test.ts)tests/messaging/store-persistence.test.ts)tests/tools/send-message.test.ts,tests/tools/receive-messages.test.ts)🤖 Generated with Claude Code
Summary by CodeRabbit