fix(wrapper): revert aggressive retry logic causing message latency#287
Merged
khaliqgant merged 3 commits intomainfrom Jan 24, 2026
Merged
fix(wrapper): revert aggressive retry logic causing message latency#287khaliqgant merged 3 commits intomainfrom
khaliqgant merged 3 commits intomainfrom
Conversation
The exponential backoff retry logic introduced in a23bffa caused message delivery delays of 2-4 minutes when injections failed. This was too aggressive for real-time agent communication. Changes: - Removed MAX_INJECTION_RETRIES (5) and INJECTION_RETRY_BASE_MS (2000) - Reverted to immediate failure reporting without retry loops - Fixed logError to always output (was incorrectly gated by debug flag) Messages now fail immediately when injection fails, allowing the system to recover faster rather than blocking in exponential backoff loops. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 My Senior Dev — Analysis Complete👤 For @khaliqgant📁 Expert in View your contributor analytics → 📊 6 files reviewed • 1 high risk • 3 need attention 🚨 High Risk:
🚀 Open Interactive Review →The full interface unlocks features not available in GitHub:
💬 Chat here: 📖 View all 12 personas & slash commandsYou can interact with me by mentioning In PR comments or on any line of code:
Slash commands:
AI Personas (mention to get their perspective):
For the best experience, view this PR on myseniordev.com — includes AI chat, file annotations, and interactive reviews. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
- Record complete investigation process for 2-4 minute message latency regression - Document root cause: commit a23bffa's exponential retry backoff (2000ms × 2^n) - Record strategy evaluation: 5 retry approaches analyzed, full revert chosen - Track implementation decisions and credential blocker resolution - Confidence: 90% - Fix verified, expected to restore 30s baseline latency Trajectory: traj_i2h6krqx2iun Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Resolve merge conflict in .trajectories/index.json by combining trajectory entries from both branches: - Preserved latency fix trajectories (traj_1b1dj40sl6jl, traj_i2h6krqx2iun) - Incorporated main branch trajectories (socket path fixes, legacy symlink fixes, path traversal validation, includeWorkflowConventions feature) All latency fix changes remain intact: - Removed aggressive retry logic from relay-pty-orchestrator - Reduced default latency values for faster message injection Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (this.config.debug) { | ||
| console.error(`[relay-pty-orchestrator:${this.config.name}] ERROR: ${message}`); | ||
| } | ||
| console.error(`[relay-pty-orchestrator:${this.config.name}] ERROR: ${message}`); |
Check warning
Code scanning / CodeQL
Log injection Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the problem, we should sanitize any user-controlled value before including it in log messages, particularly values that become part of the log “prefix” such as this.config.name. For plain-text logs, the primary concern is newline (\n, \r) and possibly other non-printable/control characters that can break log structure. A simple and robust approach is to derive a sanitized version of the agent name once in the RelayPtyOrchestrator constructor (e.g., this.safeName), where we strip or replace newline and carriage-return characters (and optionally other control characters) and then consistently use that safe value in all logging methods. This preserves existing behavior while preventing forged log lines.
The best minimal change here is:
- In
packages/wrapper/src/relay-pty-orchestrator.ts, add a private field (e.g.,private readonly safeName: string;). - In the constructor, after validating
config.name, computethis.safeName = config.name.replace(/[\r\n]/g, ' ')(or similar), which removes or neutralizes any line breaks but keeps the name readable. - Update the
logandlogErrormethods to usethis.safeNameinstead ofthis.config.namein the log prefix. - Leave all other behavior unchanged; no new external dependencies are required.
This single change addresses all three variants, because they all flow through the same name → RelayPtyOrchestrator → logError path.
Suggested changeset
1
packages/wrapper/src/relay-pty-orchestrator.ts
| @@ -227,6 +227,9 @@ | ||
| private memoryMonitor: AgentMemoryMonitor; | ||
| private memoryAlertHandler: ((alert: MemoryAlert) => void) | null = null; | ||
|
|
||
| // Sanitized agent name used for logging to prevent log injection | ||
| private readonly safeName: string; | ||
|
|
||
| // Note: sessionEndProcessed and lastSummaryRawContent are inherited from BaseWrapper | ||
|
|
||
| constructor(config: RelayPtyOrchestratorConfig) { | ||
| @@ -238,6 +241,9 @@ | ||
| throw new Error(`Invalid agent name: "${config.name}" contains path traversal characters`); | ||
| } | ||
|
|
||
| // Sanitize agent name for safe logging (remove line breaks) | ||
| this.safeName = config.name.replace(/[\r\n]/g, ' '); | ||
|
|
||
| // Get project paths (used for logs and local mode) | ||
| const projectPaths = getProjectPaths(config.cwd); | ||
|
|
||
| @@ -321,7 +327,7 @@ | ||
| */ | ||
| private log(message: string): void { | ||
| if (this.config.debug) { | ||
| console.log(`[relay-pty-orchestrator:${this.config.name}] ${message}`); | ||
| console.log(`[relay-pty-orchestrator:${this.safeName}] ${message}`); | ||
| } | ||
| } | ||
|
|
||
| @@ -329,7 +335,7 @@ | ||
| * Error log - always outputs (errors are important) | ||
| */ | ||
| private logError(message: string): void { | ||
| console.error(`[relay-pty-orchestrator:${this.config.name}] ERROR: ${message}`); | ||
| console.error(`[relay-pty-orchestrator:${this.safeName}] ERROR: ${message}`); | ||
| } | ||
|
|
||
| /** |
Copilot is powered by AI and may make mistakes. Always verify output.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Revert commit a23bffa's exponential backoff retry logic that caused 2-4 minute message injection delays.
Root Cause
Changes
Impact
Files Modified
Test Plan
Fixes the 2-4 minute message delivery regression reported on Jan 24.