Skip to content

fix: improved server side cleanup after ingest #477

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 30, 2025

Conversation

cyclotruc
Copy link
Member

@cyclotruc cyclotruc commented Jul 30, 2025

Refactor repository cleanup from background task to immediate cleanup

Overview

This PR refactors the repository cleanup mechanism from a background cleanup task to immediate cleanup after each processing operation, improving resource management and simplifying the codebase.

Changes Made

🧹 Cleanup Strategy Change

  • Before: Background task periodically scanned and deleted repositories older than 1 hour
  • After: Immediate cleanup after each repository processing (success or failure)

🔧 Key Modifications

src/server/main.py

  • Removed lifespan import and usage from FastAPI app initialization
  • Simplified app setup by removing the lifespan context manager

src/server/query_processor.py

  • Added new _cleanup_repository() function for immediate cleanup
  • Integrated cleanup calls in process_query():
    • Cleanup on processing failure (in exception handler)
    • Cleanup on successful processing completion
  • Uses shutil.rmtree() with proper error handling and logging

src/server/server_utils.py

  • Removed entire background cleanup system:
    • lifespan() context manager
    • _remove_old_repositories() background task
    • _process_folder() helper function
    • _append_line() utility function
  • Cleaned up unused imports (asyncio, shutil, time, contextlib, pathlib)

src/server/server_config.py

  • Removed DELETE_REPO_AFTER constant (no longer needed)

Copy link

github-actions bot commented Jul 30, 2025

⚙️ Preview environment was undeployed.

@NicolasIRAGNE
Copy link
Contributor

lgtm, I think this is not ideal in case of local (eg. dev) non-S3 environments but I don't have a better solution that is cheap to implement so we can go that route now

@NicolasIRAGNE NicolasIRAGNE enabled auto-merge (squash) July 30, 2025 20:19
@MickaelCa MickaelCa force-pushed the fix/server/cleanup branch from 57aea9b to bbdc938 Compare July 30, 2025 23:20
@NicolasIRAGNE NicolasIRAGNE merged commit 2df0eb4 into main Jul 30, 2025
8 of 24 checks passed
@NicolasIRAGNE NicolasIRAGNE deleted the fix/server/cleanup branch July 30, 2025 23:21
@coderamp-ci coderamp-ci bot mentioned this pull request Jul 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants