fix(kanban): delete partial attachment file on OSError during upload#35596
Open
sprmn24 wants to merge 1 commit into
Open
fix(kanban): delete partial attachment file on OSError during upload#35596sprmn24 wants to merge 1 commit into
sprmn24 wants to merge 1 commit into
Conversation
upload_task_attachment() writes the blob directly to dest_path. When an OSError occurs mid-write (disk full, permission denied), the except block converted the error to HTTPException but did not clean up dest_path, leaving a partial file on disk with no matching DB record. Add dest_path.unlink(missing_ok=True) before re-raising so the orphan file is removed on every write failure. Mirrors the cleanup already present in the size-limit (413) path above it.
tonydwb
approved these changes
May 31, 2026
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
Review
A one-line fix that prevents orphaned partial attachment files on OSError during kanban upload — matching the existing cleanup pattern in the size-limit (413) path.
✅ Looks Good
- Correct fix: Exactly matches the existing
dest_path.unlink(missing_ok=True)pattern used by the 413 handler. - Zero risk: Only adds cleanup on error path; happy path is completely untouched.
- Well-documented: Clear PR body explaining the bug and fix.
Reviewed by Hermes Agent (cron job)
tonydwb
approved these changes
May 31, 2026
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
Reviewed Changes
- plugins/kanban/dashboard/plugin_api.py:upload_task_attachment — Added
dest_path.unlink(missing_ok=True)in the OSError handler to clean up partial attachment files.
✅ Looks Good
- Correctness: When an OSError occurs during attachment upload (e.g. disk full, quota exceeded), the partially written file at
dest_pathremains on disk. This adds a cleanup that removes the orphaned file. Usingmissing_ok=Trueavoids a second OSError if the file doesn't exist yet. - Minimal change: Single line addition. Low risk.
- No security concerns: No injection vectors or credential exposure.
- Edge cases:
missing_ok=True(Python 3.8+) handles the case where the file was never created before the error. The exception is still re-raised, so the caller sees the original error.
Reviewed by Hermes Agent
tonydwb
approved these changes
May 31, 2026
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Overview
Clean one-line fix adding missing dest_path.unlink(missing_ok=True) in the except OSError handler of upload_task_attachment(). Prevents partial orphan files on failed uploads — mirrors the existing cleanup in the size-limit (413) path.
Looks Good
- One-line change, precisely targeted
- Uses
missing_ok=True— safe for concurrent access - Mirrors existing pattern in the same function
- Low blast radius
Reviewed by Hermes Agent
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
upload_task_attachment()inplugins/kanban/dashboard/plugin_api.pywritesthe uploaded blob directly to
dest_path. When anOSErroroccurs mid-write(disk full, permission denied, etc.) the handler converts it to HTTP 500 but
does not remove
dest_path, leaving a partial file on disk with no matchingrow in
task_attachments.The size-limit (413) path immediately above already does the right thing:
The
OSErrorpath was missing the same one-line cleanup.Related Issue
Fixes #35338 (follow-up — the attachment upload endpoint introduced in #35395
did not clean up on write failure)
Type of Change
Changes Made
plugins/kanban/dashboard/plugin_api.py: adddest_path.unlink(missing_ok=True)before re-raising in the
except OSErrorhandler ofupload_task_attachment()How to Test
out.writeto raiseOSError)~/.hermes/kanban/attachments/<task_id>/Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs
One-line diff — only the cleanup path changes, happy path is unaffected.