Skip to content

Ensure temporary files are cleaned up after upload process#95

Open
Darkshadow0409 wants to merge 1 commit intoindictechcom:masterfrom
Darkshadow0409:pr2-temp-cleanup
Open

Ensure temporary files are cleaned up after upload process#95
Darkshadow0409 wants to merge 1 commit intoindictechcom:masterfrom
Darkshadow0409:pr2-temp-cleanup

Conversation

@Darkshadow0409
Copy link
Copy Markdown

Related Task

https://phabricator.wikimedia.org/T415717

Problem

Temporary files created during the upload process are not always cleaned up, especially in failure scenarios. This can lead to storage issues and resource leaks over time.

Solution

This PR ensures that temporary files are always cleaned up after the upload process by using a finally block.

Changes made:

  • Wrapped synchronous upload logic in a try-finally block
  • Ensured temporary files are deleted using os.remove()
  • Added safety check using os.path.exists()

Benefits

  • Prevents accumulation of temporary files
  • Improves system reliability
  • Ensures proper resource management.

Copilot AI review requested due to automatic review settings March 31, 2026 17:39
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR aims to prevent leaked temporary files created during /api/upload by adding a try/finally cleanup around the synchronous upload path.

Changes:

  • Wrapped the synchronous (<50MB) process_upload(...) call in a try/finally.
  • Added a os.path.exists(...) check followed by os.remove(...) to delete the downloaded temp file.
Comments suppressed due to low confidence (1)

app.py:120

  • This cleanup only applies to the synchronous (<50MB) path. The async Celery path still passes a temp file to upload_image_task and tasks.py currently does not delete it, so temp files can still accumulate (and failures in the task will leak too). To match the PR description (“always cleaned up”), add best-effort deletion in the Celery task (ideally in its own try/finally) once the upload attempt finishes.
            else:
                # Process asynchronously using Celery
                OAuthObj = {
                    "consumer_key": CONSUMER_KEY,
                    "consumer_secret": CONSUMER_SECRET,
                    "key": session['mwoauth_access_token']['key'],
                    "secret": session['mwoauth_access_token']['secret']
                }
                task = upload_image_task.delay(file_path, tr_filename, src_fileext, tr_endpoint, OAuthObj)
                return jsonify({"success": True, "task_id": task.id}), 202

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread app.py
Comment on lines +109 to +110
if os.path.exists(file_path):
os.remove(file_path)
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

The finally cleanup can raise (e.g., FileNotFoundError, PermissionError) and will override/mask the intended Flask response (including successful uploads). Consider making cleanup best-effort by catching OSError (or suppressing FileNotFoundError) and logging instead of letting exceptions propagate from the finally block.

Suggested change
if os.path.exists(file_path):
os.remove(file_path)
try:
if os.path.exists(file_path):
os.remove(file_path)
except OSError as e:
logging.warning("Failed to remove temporary file %s: %s", file_path, e)

Copilot uses AI. Check for mistakes.
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.

2 participants