Skip to content

Conversation

@alexcos20
Copy link
Member

Fixes #1186

Changes proposed in this PR:

  • add image cleanup

@alexcos20 alexcos20 self-assigned this Feb 3, 2026
@alexcos20
Copy link
Member Author

/run-security-scan

@alexcos20 alexcos20 marked this pull request as ready for review February 3, 2026 13:59
Copy link
Member Author

@alexcos20 alexcos20 left a comment

Choose a reason for hiding this comment

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

AI automated code review (Gemini 3).

Overall risk: medium

Summary:
This pull request introduces a new feature to the C2D (Compute-to-Data) engine for automated Docker image cleanup. It adds imageRetentionDays and imageCleanupInterval as configurable options for Docker compute environments. A new docker_images table is added to the SQLite database to track the last usage timestamp of Docker images. The C2DEngineDocker class is enhanced to record image usage, retrieve old images based on retention settings, and periodically remove them using dockerode. Comprehensive integration tests have been added to validate the database operations and the engine's cleanup logic, including conditional tests with a real Docker daemon. Additionally, a concurrency check was added to the InternalLoop of the C2D engine.

Comments:
• [INFO][style] The comment (24 hours) for imageCleanupInterval is accurate, but removing (ms) from the end would prevent confusion since the value 86400 is in seconds, not milliseconds. It's consistent with the schema and code using seconds.
• [WARNING][bug] The comment for imageCleanupInterval indicates Default: 86400 ms (24 hours). This is inconsistent with schemas.ts and env.md, where the default value 86400 is defined in seconds. The code also uses this value as seconds before multiplying by 1000 for setInterval. Please change ms to seconds here to maintain consistency and accuracy.
• [BUG][bug] The initial default value for private cleanupInterval: number = 86400000 // 24 hours is set in milliseconds. However, the configuration clusterConfig.connection.imageCleanupInterval (line 81) is expected in seconds (86400), and setInterval correctly multiplies it by 1000. To ensure consistency, this initial default should also be in seconds: private cleanupInterval: number = 86400 // 24 hours.
• [INFO][style] Consider grouping private isInternalLoopRunning: boolean = false; and private imageCleanupTimer: NodeJS.Timeout | null = null; closer to other related properties like private cronTimer and private cronTime for better readability, or group all timer-related properties together.
• [INFO][other] The setTimeout for initial cleanup runs after 60 seconds. This is reasonable, but it might be useful to make this initial delay configurable or tie it to imageCleanupInterval (e.g., a fraction of the interval) for very short intervals to avoid immediate heavy load or for very long ones to not wait too long for the first cleanup.
• [INFO][performance] The InternalLoop now processes jobs only if jobs.length > 0. This is good. However, if there are a large number of jobs, Promise.all(promises) could still lead to a high number of concurrent Docker operations. While this might be intended, consider if there's a need for a concurrency limit (e.g., using p-limit or a custom queue) if the number of jobs could scale very high and overwhelm the Docker daemon or system resources.

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.

Automatic Storage Cleanup to Prevent Disk Exhaustion

2 participants