Skip to content

fix(fpc): memory leak, nil guards, thread safety, cleanups - #122

Open
Pedro-DAS wants to merge 9 commits into
gustavoeenriquez:fpcfrom
Pedro-DAS:pr/essential-fixes
Open

fix(fpc): memory leak, nil guards, thread safety, cleanups#122
Pedro-DAS wants to merge 9 commits into
gustavoeenriquez:fpcfrom
Pedro-DAS:pr/essential-fixes

Conversation

@Pedro-DAS

Copy link
Copy Markdown

Correcciones esenciales para MakerAi FPC

Este PR incluye 5 correcciones identificadas mediante auditoría multi-modelo del código fuente:

1. Memory leak: TAiChat.Destroy no libera tool instances

Problema: El destructor de TAiChat liberaba ~8 campos propios pero omitía 11 tool instances (FSpeechTool, FImageTool, FVideoTool, FWebSearchTool, FVisionTool, FPdfTool, FReportTool, FShellTool, FTextEditorTool, FComputerUseTool, FAiFunctions). Cada conexión con tools perdía memoria.
Impacto: Acumulación de memoria progresiva al usar herramientas.
Demo: test_tool_leak.pas

2. AV potencial: TAiMediaFile sin nil guards

Problema: GetBase64, GetBytes, SaveToFile, ToString accedían a FContent.Memory/Size sin verificar Assigned(FContent), causando Access Violation si no se había cargado contenido.
Impacto: Crash al manipular TAiMediaFile vacío.
Demo: test_media_nil.pas

3. Race condition: LogDebug sin sincronización

Problema: LogDebug escribía al archivo desde múltiples hilos sin TCriticalSection, corrompiendo el log bajo concurrencia.
Impacto: Diagnóstico imposible en entornos multi-thread.
Demo: test_log_threadsafe.pas

4. Código muerto: variables sin usar

Problema: demo_rag_graph.pas declaraba Nodes (TNodeArray) y PathNode (TAiRagGraphNode) sin usarlos.
Impacto: Warnings de compilación.

5. Documentación desactualizada

Problema: CLAUDE.md marcaba Chat, Agents, RAG, MCPClient, MCPServer como 'Empty' y Tools como 'Stubs' cuando ya están implementados.
Impacto: Confusión sobre el estado real del port.

@Pedro-DAS
Pedro-DAS force-pushed the pr/essential-fixes branch from 2ad9e7f to d299c55 Compare July 11, 2026 03:53
Add FreeAndNil for FSpeechTool, FImageTool, FVideoTool, FWebSearchTool,
FVisionTool, FPdfTool, FReportTool, FShellTool, FTextEditorTool,
FComputerUseTool, and FAiFunctions before inherited Destroy.

These tool fields were being leaked because Destroy only freed
FTools, FMemory, etc. but not the tool instances.
…, ToString

Si FContent no está asignado, se crea TMemoryStream antes de usar.
En SaveToFile se retorna temprano si no hay contenido.
Eliminadas declaraciones de Nodes (TNodeArray) y PathNode (TAiRagGraphNode)
que no se usaban en el cuerpo del demo.
- Agregada variable unit-level LogDebugCS
- Cuerpo de LogDebug envuelto en Enter/Leave del CS
- Inicialización y finalización del CS en bloques initialization/finalization
Source/Tools, Source/Chat/, Source/Agents/, Source/RAG/,
Source/MCPClient/, Source/MCPServer pasan de Stubs/Empty a Implemented.
@Pedro-DAS
Pedro-DAS force-pushed the pr/essential-fixes branch from d299c55 to 981218b Compare July 11, 2026 03:53
@Sergio87Felix

This comment has been minimized.

@gustavoeenriquez gustavoeenriquez left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

¡Hola Pedro! Muchas gracias por el PR y por el nivel de detalle de la auditoría — contribuciones así le aportan muchísimo al port FPC. 🙏

Revisé los 5 puntos contra la rama fpc actual (1fb0148); va el veredicto por punto:

#3 LogDebug + TCriticalSection — aceptado. LogDebug se llama desde TToolCallThread (varias tool calls en paralelo), así que la sincronización es necesaria. SyncObjs ya está en el uses y la unidad no tenía sección initialization previa; compila limpio.

#4 Variables muertas en demo_rag_graph.pas — aceptado.

#5 CLAUDE.md — aceptado con un ajuste. Tienes razón: esos módulos ya están implementados. Pero quedó el texto "— to be ported" en la columna de notas de las 6 filas, que ahora resulta contradictorio. ¿Puedes actualizar también esas notas en el mismo cambio?

🔴 #1 FreeAndNil de tool instances en TAiChat.Destroy — aquí sí te pido cambios. En el diseño del framework (igual que en el Delphi original), FSpeechTool, FImageTool, …, FAiFunctions son referencias no propietarias: se asignan externamente vía los setters (que llaman FreeNotification) y Notification las pone en nil cuando su dueño las destruye. TAiChat referencia las tools pero no las posee. Con el FreeAndNil en el destructor:

  • una tool compartida entre dos chats se destruye cuando muere el primero;
  • si el chat se libera dinámicamente, el form/datamodule dueño de la tool queda con una referencia colgante → AV.

No es un memory leak del framework: si en test_tool_leak.pas las tools se crean con nil como owner y no se liberan, el leak está en el código llamador — la corrección iría en el test/demo (liberarlas o crearlas con owner). ¿Puedes retirar este bloque del PR? Si te interesa un modo "el chat es dueño de sus tools", lo conversamos aparte como cambio de diseño explícito (p. ej. una propiedad OwnsTools), no como fix silencioso en el destructor.

🟡 #2 Nil guards en TAiMediaFile — opcional. El constructor ya crea FContent y nada lo anula durante la vida del objeto, así que los guards hoy son código defensivo no alcanzable. No estorban, pero si los dejas: (a) corrige la indentación de los FContent.Clear; que quedaron desalineados en 4 sitios, y (b) en SaveToFile preferiría una excepción clara antes que un Exit silencioso.

Con #1 retirado y los ajustes menores, lo mergeo enseguida. ¡Gracias de nuevo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants