mirror of
https://github.com/AutoMaker-Org/automaker.git
synced 2026-03-17 22:13:08 +00:00
Fix: Dev server detection bug fixes. Settings sync bug fixes. Cli provider fixes. Terminal background/foreground colors (#791)
* Changes from fix/dev-server-state-bug * feat: Add configurable max turns setting with user overrides. Address pr comments * fix: Update default behaviors and improve state management across server and UI * feat: Extract branch sync logic to separate service. Fix settings sync bug. Address pr comments * refactor: Extract magic numbers to named constants and improve branch tracking logic - Add DEFAULT_MAX_TURNS (1000) and MAX_ALLOWED_TURNS (2000) constants to settings-helpers - Replace hardcoded 1000 values with DEFAULT_MAX_TURNS constant throughout codebase - Improve max turns validation with explicit Number.isFinite check - Update getTrackingBranch to split on first slash instead of last for better remote parsing - Change isBranchCheckedOut return type from boolean to string|null to return worktree path - Add comments explaining skipFetch parameter in worktree creation - Fix cleanup order in AgentExecutor finally block to run before logging ``` * feat: Add comment refresh and improve model sync in PR dialog
This commit is contained in:
@@ -19,6 +19,10 @@ const logger = createLogger('DevServerService');
|
||||
// Maximum scrollback buffer size (characters) - matches TerminalService pattern
|
||||
const MAX_SCROLLBACK_SIZE = 50000; // ~50KB per dev server
|
||||
|
||||
// Timeout (ms) before falling back to the allocated port if URL detection hasn't succeeded.
|
||||
// This handles cases where the dev server output format is not recognized by any pattern.
|
||||
const URL_DETECTION_TIMEOUT_MS = 30_000;
|
||||
|
||||
// URL patterns for detecting full URLs from dev server output.
|
||||
// Defined once at module level to avoid reallocation on every call to detectUrlFromOutput.
|
||||
// Ordered from most specific (framework-specific) to least specific.
|
||||
@@ -88,6 +92,8 @@ const OUTPUT_BATCH_SIZE = 4096; // Smaller batches for lower latency
|
||||
|
||||
export interface DevServerInfo {
|
||||
worktreePath: string;
|
||||
/** The port originally reserved by findAvailablePort() – never mutated after startDevServer sets it */
|
||||
allocatedPort: number;
|
||||
port: number;
|
||||
url: string;
|
||||
process: ChildProcess | null;
|
||||
@@ -102,6 +108,8 @@ export interface DevServerInfo {
|
||||
stopping: boolean;
|
||||
// Flag to indicate if URL has been detected from output
|
||||
urlDetected: boolean;
|
||||
// Timer for URL detection timeout fallback
|
||||
urlDetectionTimeout: NodeJS.Timeout | null;
|
||||
}
|
||||
|
||||
// Port allocation starts at 3001 to avoid conflicts with common dev ports
|
||||
@@ -124,6 +132,32 @@ class DevServerService {
|
||||
this.emitter = emitter;
|
||||
}
|
||||
|
||||
/**
|
||||
* Prune a stale server entry whose process has exited without cleanup.
|
||||
* Clears any pending timers, removes the port from allocatedPorts, deletes
|
||||
* the entry from runningServers, and emits the "dev-server:stopped" event
|
||||
* so all callers consistently notify the frontend when pruning entries.
|
||||
*
|
||||
* @param worktreePath - The key used in runningServers
|
||||
* @param server - The DevServerInfo entry to prune
|
||||
*/
|
||||
private pruneStaleServer(worktreePath: string, server: DevServerInfo): void {
|
||||
if (server.flushTimeout) clearTimeout(server.flushTimeout);
|
||||
if (server.urlDetectionTimeout) clearTimeout(server.urlDetectionTimeout);
|
||||
// Use allocatedPort (immutable) to free the reserved slot; server.port may have
|
||||
// been mutated by detectUrlFromOutput to reflect the actual detected port.
|
||||
this.allocatedPorts.delete(server.allocatedPort);
|
||||
this.runningServers.delete(worktreePath);
|
||||
if (this.emitter) {
|
||||
this.emitter.emit('dev-server:stopped', {
|
||||
worktreePath,
|
||||
port: server.port, // Report the externally-visible (detected) port
|
||||
exitCode: server.process?.exitCode ?? null,
|
||||
timestamp: new Date().toISOString(),
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Append data to scrollback buffer with size limit enforcement
|
||||
* Evicts oldest data when buffer exceeds MAX_SCROLLBACK_SIZE
|
||||
@@ -253,6 +287,12 @@ class DevServerService {
|
||||
server.url = detectedUrl;
|
||||
server.urlDetected = true;
|
||||
|
||||
// Clear the URL detection timeout since we found the URL
|
||||
if (server.urlDetectionTimeout) {
|
||||
clearTimeout(server.urlDetectionTimeout);
|
||||
server.urlDetectionTimeout = null;
|
||||
}
|
||||
|
||||
// Update the port to match the detected URL's actual port
|
||||
const detectedPort = this.extractPortFromUrl(detectedUrl);
|
||||
if (detectedPort && detectedPort !== server.port) {
|
||||
@@ -291,6 +331,12 @@ class DevServerService {
|
||||
server.url = detectedUrl;
|
||||
server.urlDetected = true;
|
||||
|
||||
// Clear the URL detection timeout since we found the port
|
||||
if (server.urlDetectionTimeout) {
|
||||
clearTimeout(server.urlDetectionTimeout);
|
||||
server.urlDetectionTimeout = null;
|
||||
}
|
||||
|
||||
if (detectedPort !== server.port) {
|
||||
logger.info(
|
||||
`Port mismatch: allocated ${server.port}, detected ${detectedPort} from ${description}`
|
||||
@@ -660,6 +706,7 @@ class DevServerService {
|
||||
const hostname = process.env.HOSTNAME || 'localhost';
|
||||
const serverInfo: DevServerInfo = {
|
||||
worktreePath,
|
||||
allocatedPort: port, // Immutable: records which port we reserved; never changed after this point
|
||||
port,
|
||||
url: `http://${hostname}:${port}`, // Initial URL, may be updated by detectUrlFromOutput
|
||||
process: devProcess,
|
||||
@@ -669,6 +716,7 @@ class DevServerService {
|
||||
flushTimeout: null,
|
||||
stopping: false,
|
||||
urlDetected: false, // Will be set to true when actual URL is detected from output
|
||||
urlDetectionTimeout: null, // Will be set after server starts successfully
|
||||
};
|
||||
|
||||
// Capture stdout with buffer management and event emission
|
||||
@@ -692,18 +740,24 @@ class DevServerService {
|
||||
serverInfo.flushTimeout = null;
|
||||
}
|
||||
|
||||
// Clear URL detection timeout to prevent stale fallback emission
|
||||
if (serverInfo.urlDetectionTimeout) {
|
||||
clearTimeout(serverInfo.urlDetectionTimeout);
|
||||
serverInfo.urlDetectionTimeout = null;
|
||||
}
|
||||
|
||||
// Emit stopped event (only if not already stopping - prevents duplicate events)
|
||||
if (this.emitter && !serverInfo.stopping) {
|
||||
this.emitter.emit('dev-server:stopped', {
|
||||
worktreePath,
|
||||
port,
|
||||
port: serverInfo.port, // Use the detected port (may differ from allocated port if detectUrlFromOutput updated it)
|
||||
exitCode,
|
||||
error: errorMessage,
|
||||
timestamp: new Date().toISOString(),
|
||||
});
|
||||
}
|
||||
|
||||
this.allocatedPorts.delete(port);
|
||||
this.allocatedPorts.delete(serverInfo.allocatedPort);
|
||||
this.runningServers.delete(worktreePath);
|
||||
};
|
||||
|
||||
@@ -749,6 +803,43 @@ class DevServerService {
|
||||
});
|
||||
}
|
||||
|
||||
// Set up URL detection timeout fallback.
|
||||
// If URL detection hasn't succeeded after URL_DETECTION_TIMEOUT_MS, check if
|
||||
// the allocated port is actually in use (server probably started successfully)
|
||||
// and emit a url-detected event with the allocated port as fallback.
|
||||
// Also re-scan the scrollback buffer in case the URL was printed before
|
||||
// our patterns could match (e.g., it was split across multiple data chunks).
|
||||
serverInfo.urlDetectionTimeout = setTimeout(() => {
|
||||
serverInfo.urlDetectionTimeout = null;
|
||||
|
||||
// Only run fallback if server is still running and URL wasn't detected
|
||||
if (serverInfo.stopping || serverInfo.urlDetected || !this.runningServers.has(worktreePath)) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Re-scan the entire scrollback buffer for URL patterns
|
||||
// This catches cases where the URL was split across multiple output chunks
|
||||
logger.info(`URL detection timeout for ${worktreePath}, re-scanning scrollback buffer`);
|
||||
this.detectUrlFromOutput(serverInfo, serverInfo.scrollbackBuffer);
|
||||
|
||||
// If still not detected after full rescan, use the allocated port as fallback
|
||||
if (!serverInfo.urlDetected) {
|
||||
logger.info(`URL detection fallback: using allocated port ${port} for ${worktreePath}`);
|
||||
const fallbackUrl = `http://${hostname}:${port}`;
|
||||
serverInfo.url = fallbackUrl;
|
||||
serverInfo.urlDetected = true;
|
||||
|
||||
if (this.emitter) {
|
||||
this.emitter.emit('dev-server:url-detected', {
|
||||
worktreePath,
|
||||
url: fallbackUrl,
|
||||
port,
|
||||
timestamp: new Date().toISOString(),
|
||||
});
|
||||
}
|
||||
}
|
||||
}, URL_DETECTION_TIMEOUT_MS);
|
||||
|
||||
return {
|
||||
success: true,
|
||||
result: {
|
||||
@@ -794,6 +885,12 @@ class DevServerService {
|
||||
server.flushTimeout = null;
|
||||
}
|
||||
|
||||
// Clean up URL detection timeout
|
||||
if (server.urlDetectionTimeout) {
|
||||
clearTimeout(server.urlDetectionTimeout);
|
||||
server.urlDetectionTimeout = null;
|
||||
}
|
||||
|
||||
// Clear any pending output buffer
|
||||
server.outputBuffer = '';
|
||||
|
||||
@@ -812,8 +909,10 @@ class DevServerService {
|
||||
server.process.kill('SIGTERM');
|
||||
}
|
||||
|
||||
// Free the port
|
||||
this.allocatedPorts.delete(server.port);
|
||||
// Free the originally-reserved port slot (allocatedPort is immutable and always
|
||||
// matches what was added to allocatedPorts in startDevServer; server.port may
|
||||
// have been updated by detectUrlFromOutput to the actual detected port).
|
||||
this.allocatedPorts.delete(server.allocatedPort);
|
||||
this.runningServers.delete(worktreePath);
|
||||
|
||||
return {
|
||||
@@ -827,6 +926,7 @@ class DevServerService {
|
||||
|
||||
/**
|
||||
* List all running dev servers
|
||||
* Also verifies that each server's process is still alive, removing stale entries
|
||||
*/
|
||||
listDevServers(): {
|
||||
success: boolean;
|
||||
@@ -836,14 +936,37 @@ class DevServerService {
|
||||
port: number;
|
||||
url: string;
|
||||
urlDetected: boolean;
|
||||
startedAt: string;
|
||||
}>;
|
||||
};
|
||||
} {
|
||||
// Prune any servers whose process has died without us being notified
|
||||
// This handles edge cases where the process exited but the 'exit' event was missed
|
||||
const stalePaths: string[] = [];
|
||||
for (const [worktreePath, server] of this.runningServers) {
|
||||
// Check if exitCode is a number (not null/undefined) - indicates process has exited
|
||||
if (server.process && typeof server.process.exitCode === 'number') {
|
||||
logger.info(
|
||||
`Pruning stale server entry for ${worktreePath} (process exited with code ${server.process.exitCode})`
|
||||
);
|
||||
stalePaths.push(worktreePath);
|
||||
}
|
||||
}
|
||||
for (const stalePath of stalePaths) {
|
||||
const server = this.runningServers.get(stalePath);
|
||||
if (server) {
|
||||
// Delegate to the shared helper so timers, ports, and the stopped event
|
||||
// are all handled consistently with isRunning and getServerInfo.
|
||||
this.pruneStaleServer(stalePath, server);
|
||||
}
|
||||
}
|
||||
|
||||
const servers = Array.from(this.runningServers.values()).map((s) => ({
|
||||
worktreePath: s.worktreePath,
|
||||
port: s.port,
|
||||
url: s.url,
|
||||
urlDetected: s.urlDetected,
|
||||
startedAt: s.startedAt.toISOString(),
|
||||
}));
|
||||
|
||||
return {
|
||||
@@ -853,17 +976,33 @@ class DevServerService {
|
||||
}
|
||||
|
||||
/**
|
||||
* Check if a worktree has a running dev server
|
||||
* Check if a worktree has a running dev server.
|
||||
* Also prunes stale entries where the process has exited.
|
||||
*/
|
||||
isRunning(worktreePath: string): boolean {
|
||||
return this.runningServers.has(worktreePath);
|
||||
const server = this.runningServers.get(worktreePath);
|
||||
if (!server) return false;
|
||||
// Prune stale entry if the process has exited
|
||||
if (server.process && typeof server.process.exitCode === 'number') {
|
||||
this.pruneStaleServer(worktreePath, server);
|
||||
return false;
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
/**
|
||||
* Get info for a specific worktree's dev server
|
||||
* Get info for a specific worktree's dev server.
|
||||
* Also prunes stale entries where the process has exited.
|
||||
*/
|
||||
getServerInfo(worktreePath: string): DevServerInfo | undefined {
|
||||
return this.runningServers.get(worktreePath);
|
||||
const server = this.runningServers.get(worktreePath);
|
||||
if (!server) return undefined;
|
||||
// Prune stale entry if the process has exited
|
||||
if (server.process && typeof server.process.exitCode === 'number') {
|
||||
this.pruneStaleServer(worktreePath, server);
|
||||
return undefined;
|
||||
}
|
||||
return server;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -891,6 +1030,15 @@ class DevServerService {
|
||||
};
|
||||
}
|
||||
|
||||
// Prune stale entry if the process has been killed or has exited
|
||||
if (server.process && (server.process.killed || server.process.exitCode != null)) {
|
||||
this.pruneStaleServer(worktreePath, server);
|
||||
return {
|
||||
success: false,
|
||||
error: `No dev server running for worktree: ${worktreePath}`,
|
||||
};
|
||||
}
|
||||
|
||||
return {
|
||||
success: true,
|
||||
result: {
|
||||
|
||||
Reference in New Issue
Block a user