refactor(worker): rename cleanupLocalMcpActivity to cleanupRunResourcesActivity#277
Conversation
…esActivity The activity cleans up all run-scoped Docker resources (containers and volumes), not just local MCP servers. Rename to accurately reflect its broader scope. Signed-off-by: Aseem Shrey <LuD1161@users.noreply.github.com>
![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: 0ed690406a
ℹ️ 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".
| registerLocalMcpActivity, | ||
| registerRemoteMcpActivity, | ||
| cleanupLocalMcpActivity, | ||
| cleanupRunResourcesActivity, |
There was a problem hiding this comment.
Renaming cleanupLocalMcpActivity to cleanupRunResourcesActivity changes the Temporal activity type and removes the old handler from worker registration. For workflows that were already running before this deploy, any pending/retried cleanupLocalMcpActivity task will now be polled by a worker that no longer serves that type, leading to repeated activity failures/timeouts for those executions. Keep a temporary alias for the legacy activity name (or gate with workflow versioning) until pre-rename runs are drained.
Useful? React with 👍 / 👎.
…esActivity - Renamed cleanupLocalMcpActivity → cleanupRunResourcesActivity - Renamed CleanupLocalMcpActivityInput → CleanupRunResourcesActivityInput - The activity cleans up all run-scoped Docker resources (containers + volumes), not just local MCP servers - The old name was a misnomer from when only MCP containers existed - No behavioral changes, purely a naming fix Applied from PR #277 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: betterclever <paliwal.pranjal83@gmail.com>
**Problem:** Containers were only cleaned up if registered in Redis tool registry, causing orphans when registration failed (network errors, crashes, etc.) **Solution:** 1. Added fallback container cleanup using name pattern matching - Primary: Clean containers from tool registry (existing) - Fallback: Clean containers matching "mcp-server-*" pattern (new) - Deduplicates and cleans all found containers 2. Explicitly set autoRemove=false for MCP containers - Prevents race condition where container auto-removes before cleanup - Ensures containers persist for manual cleanup by workflow - Added STUDIO_RUN_ID env var for better identification **Impact:** - Orphaned containers are now caught by name pattern fallback - Two-layer defense: registry tracking + name pattern matching - Debug flag (SKIP_CONTAINER_CLEANUP) preserved for E2E testing - Volume cleanup unchanged (already uses labels correctly) **Testing:** - TypeScript compilation passes - Maintains backward compatibility with existing cleanup flow Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: betterclever <paliwal.pranjal83@gmail.com>
Signed-off-by: betterclever <paliwal.pranjal83@gmail.com>
2 participants
Summary
1. Refactoring (Original PR Scope)
cleanupLocalMcpActivity→cleanupRunResourcesActivityCleanupLocalMcpActivityInput→CleanupRunResourcesActivityInput2. Orphan Prevention Fixes (Added)
Problem: Containers were orphaned when registration failed (network errors, Redis unavailable, crashes)
Solution:
✅ Added dual-layer container cleanup:
✅ Explicitly set
autoRemove: falsefor MCP containersSTUDIO_RUN_IDenv var for better identification✅ Preserved
SKIP_CONTAINER_CLEANUPdebug flag for E2E testingImpact:
Test plan
finallyblock