fix: remediate 10 review findings in communication spine
- Fix createSession race condition with in-flight promise map - Fix listener leak: always cleanup previous subscription per client - Fix REST timeout returning HTTP 200 — now rejects with 504 - Fix fire-and-forget Discord sends — await with error handling - Fix non-null assertion on client.user in Discord plugin - Fix TUI disconnect mid-stream deadlock (reset streaming state) - Add connect_error handler to TUI and Discord plugin - Add connected guard on TUI message submit - Add relayEvent guard for disconnected sockets - Sanitize error messages sent to WebSocket clients - Add error logging/context to AgentService create/prompt/destroy Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -17,18 +17,39 @@ export interface AgentSession {
|
||||
export class AgentService implements OnModuleDestroy {
|
||||
private readonly logger = new Logger(AgentService.name);
|
||||
private readonly sessions = new Map<string, AgentSession>();
|
||||
private readonly creating = new Map<string, Promise<AgentSession>>();
|
||||
|
||||
async createSession(sessionId: string): Promise<AgentSession> {
|
||||
if (this.sessions.has(sessionId)) {
|
||||
return this.sessions.get(sessionId)!;
|
||||
}
|
||||
const existing = this.sessions.get(sessionId);
|
||||
if (existing) return existing;
|
||||
|
||||
const inflight = this.creating.get(sessionId);
|
||||
if (inflight) return inflight;
|
||||
|
||||
const promise = this.doCreateSession(sessionId).finally(() => {
|
||||
this.creating.delete(sessionId);
|
||||
});
|
||||
this.creating.set(sessionId, promise);
|
||||
return promise;
|
||||
}
|
||||
|
||||
private async doCreateSession(sessionId: string): Promise<AgentSession> {
|
||||
this.logger.log(`Creating agent session: ${sessionId}`);
|
||||
|
||||
const { session: piSession } = await createAgentSession({
|
||||
sessionManager: SessionManager.inMemory(),
|
||||
tools: [],
|
||||
});
|
||||
let piSession: PiAgentSession;
|
||||
try {
|
||||
const result = await createAgentSession({
|
||||
sessionManager: SessionManager.inMemory(),
|
||||
tools: [],
|
||||
});
|
||||
piSession = result.session;
|
||||
} catch (err) {
|
||||
this.logger.error(
|
||||
`Failed to create Pi SDK session for ${sessionId}`,
|
||||
err instanceof Error ? err.stack : String(err),
|
||||
);
|
||||
throw new Error(`Agent session creation failed for ${sessionId}: ${String(err)}`);
|
||||
}
|
||||
|
||||
const listeners = new Set<(event: AgentSessionEvent) => void>();
|
||||
|
||||
@@ -64,7 +85,15 @@ export class AgentService implements OnModuleDestroy {
|
||||
if (!session) {
|
||||
throw new Error(`No agent session found: ${sessionId}`);
|
||||
}
|
||||
await session.piSession.prompt(message);
|
||||
try {
|
||||
await session.piSession.prompt(message);
|
||||
} catch (err) {
|
||||
this.logger.error(
|
||||
`Pi SDK prompt failed for session=${sessionId}, messageLength=${message.length}`,
|
||||
err instanceof Error ? err.stack : String(err),
|
||||
);
|
||||
throw err;
|
||||
}
|
||||
}
|
||||
|
||||
onEvent(sessionId: string, listener: (event: AgentSessionEvent) => void): () => void {
|
||||
@@ -78,17 +107,25 @@ export class AgentService implements OnModuleDestroy {
|
||||
|
||||
async destroySession(sessionId: string): Promise<void> {
|
||||
const session = this.sessions.get(sessionId);
|
||||
if (session) {
|
||||
this.logger.log(`Destroying agent session ${sessionId}`);
|
||||
if (!session) return;
|
||||
this.logger.log(`Destroying agent session ${sessionId}`);
|
||||
try {
|
||||
session.unsubscribe();
|
||||
session.listeners.clear();
|
||||
this.sessions.delete(sessionId);
|
||||
} catch (err) {
|
||||
this.logger.error(`Failed to unsubscribe Pi session ${sessionId}`, String(err));
|
||||
}
|
||||
session.listeners.clear();
|
||||
this.sessions.delete(sessionId);
|
||||
}
|
||||
|
||||
async onModuleDestroy(): Promise<void> {
|
||||
this.logger.log('Shutting down all agent sessions');
|
||||
const stops = Array.from(this.sessions.keys()).map((id) => this.destroySession(id));
|
||||
await Promise.allSettled(stops);
|
||||
const results = await Promise.allSettled(stops);
|
||||
for (const result of results) {
|
||||
if (result.status === 'rejected') {
|
||||
this.logger.error('Session shutdown failure', String(result.reason));
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { Controller, Post, Body, Logger } from '@nestjs/common';
|
||||
import { Controller, Post, Body, Logger, HttpException, HttpStatus } from '@nestjs/common';
|
||||
import type { AgentSessionEvent } from '@mariozechner/pi-coding-agent';
|
||||
import { AgentService } from '../agent/agent.service.js';
|
||||
import { v4 as uuid } from 'uuid';
|
||||
@@ -23,32 +23,52 @@ export class ChatController {
|
||||
async chat(@Body() body: ChatRequest): Promise<ChatResponse> {
|
||||
const conversationId = body.conversationId ?? uuid();
|
||||
|
||||
let agentSession = this.agentService.getSession(conversationId);
|
||||
if (!agentSession) {
|
||||
agentSession = await this.agentService.createSession(conversationId);
|
||||
try {
|
||||
let agentSession = this.agentService.getSession(conversationId);
|
||||
if (!agentSession) {
|
||||
agentSession = await this.agentService.createSession(conversationId);
|
||||
}
|
||||
} catch (err) {
|
||||
this.logger.error(
|
||||
`Session creation failed for conversation=${conversationId}`,
|
||||
err instanceof Error ? err.stack : String(err),
|
||||
);
|
||||
throw new HttpException('Agent session unavailable', HttpStatus.SERVICE_UNAVAILABLE);
|
||||
}
|
||||
|
||||
let responseText = '';
|
||||
|
||||
const done = new Promise<void>((resolve) => {
|
||||
const done = new Promise<void>((resolve, reject) => {
|
||||
const timer = setTimeout(() => {
|
||||
cleanup();
|
||||
this.logger.error(`Agent response timed out after 120s for conversation=${conversationId}`);
|
||||
reject(new Error('Agent response timed out'));
|
||||
}, 120_000);
|
||||
|
||||
const cleanup = this.agentService.onEvent(conversationId, (event: AgentSessionEvent) => {
|
||||
if (event.type === 'message_update' && event.assistantMessageEvent.type === 'text_delta') {
|
||||
responseText += event.assistantMessageEvent.delta;
|
||||
}
|
||||
if (event.type === 'agent_end') {
|
||||
clearTimeout(timer);
|
||||
cleanup();
|
||||
resolve();
|
||||
}
|
||||
});
|
||||
|
||||
setTimeout(() => {
|
||||
cleanup();
|
||||
resolve();
|
||||
}, 120_000);
|
||||
});
|
||||
|
||||
await this.agentService.prompt(conversationId, body.content);
|
||||
await done;
|
||||
try {
|
||||
await this.agentService.prompt(conversationId, body.content);
|
||||
await done;
|
||||
} catch (err) {
|
||||
if (err instanceof HttpException) throw err;
|
||||
const message = err instanceof Error ? err.message : String(err);
|
||||
if (message.includes('timed out')) {
|
||||
throw new HttpException('Agent response timed out', HttpStatus.GATEWAY_TIMEOUT);
|
||||
}
|
||||
this.logger.error(`Chat prompt failed for conversation=${conversationId}`, String(err));
|
||||
throw new HttpException('Agent processing failed', HttpStatus.INTERNAL_SERVER_ERROR);
|
||||
}
|
||||
|
||||
return { conversationId, text: responseText };
|
||||
}
|
||||
|
||||
@@ -62,14 +62,26 @@ export class ChatGateway implements OnGatewayInit, OnGatewayConnection, OnGatewa
|
||||
this.logger.log(`Message from ${client.id} in conversation ${conversationId}`);
|
||||
|
||||
// Ensure agent session exists for this conversation
|
||||
let agentSession = this.agentService.getSession(conversationId);
|
||||
if (!agentSession) {
|
||||
agentSession = await this.agentService.createSession(conversationId);
|
||||
try {
|
||||
let agentSession = this.agentService.getSession(conversationId);
|
||||
if (!agentSession) {
|
||||
agentSession = await this.agentService.createSession(conversationId);
|
||||
}
|
||||
} catch (err) {
|
||||
this.logger.error(
|
||||
`Session creation failed for client=${client.id}, conversation=${conversationId}`,
|
||||
err instanceof Error ? err.stack : String(err),
|
||||
);
|
||||
client.emit('error', {
|
||||
conversationId,
|
||||
error: 'Failed to start agent session. Please try again.',
|
||||
});
|
||||
return;
|
||||
}
|
||||
|
||||
// Clean up any previous event subscription for this client
|
||||
// Always clean up previous listener to prevent leak
|
||||
const existing = this.clientSessions.get(client.id);
|
||||
if (existing && existing.conversationId !== conversationId) {
|
||||
if (existing) {
|
||||
existing.cleanup();
|
||||
}
|
||||
|
||||
@@ -87,12 +99,25 @@ export class ChatGateway implements OnGatewayInit, OnGatewayConnection, OnGatewa
|
||||
try {
|
||||
await this.agentService.prompt(conversationId, data.content);
|
||||
} catch (err) {
|
||||
this.logger.error(`Agent prompt failed: ${err}`);
|
||||
client.emit('error', { conversationId, error: String(err) });
|
||||
this.logger.error(
|
||||
`Agent prompt failed for client=${client.id}, conversation=${conversationId}`,
|
||||
err instanceof Error ? err.stack : String(err),
|
||||
);
|
||||
client.emit('error', {
|
||||
conversationId,
|
||||
error: 'The agent failed to process your message. Please try again.',
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
private relayEvent(client: Socket, conversationId: string, event: AgentSessionEvent): void {
|
||||
if (!client.connected) {
|
||||
this.logger.warn(
|
||||
`Dropping event ${event.type} for disconnected client=${client.id}, conversation=${conversationId}`,
|
||||
);
|
||||
return;
|
||||
}
|
||||
|
||||
switch (event.type) {
|
||||
case 'agent_start':
|
||||
client.emit('agent:start', { conversationId });
|
||||
|
||||
Reference in New Issue
Block a user