diff --git a/links/file_views.py b/links/file_views.py index c2f3d5b..2a03253 100644 --- a/links/file_views.py +++ b/links/file_views.py @@ -23,27 +23,63 @@ from .serializers import FileUploadSerializer logger = logging.getLogger(__name__) +class FileSaveError(Exception): + """Raised when an uploaded file could not be fully persisted to disk.""" + + def _get_upload_folder(): folder = settings.FILE_UPLOADS_FOLDER Path(folder).mkdir(parents=True, exist_ok=True) return folder +def _cleanup_partial(path): + try: + if os.path.exists(path): + os.remove(path) + except OSError: + logger.warning("Failed to clean up partial upload file %s", path, exc_info=True) + + def _save_uploaded_file(f): - """Save an in-memory upload to FILE_UPLOADS_FOLDER and return a FileUpload instance.""" + """Save an in-memory upload to FILE_UPLOADS_FOLDER and return a FileUpload instance. + + Writes to a temporary path first, fsyncs, and verifies the byte count on + disk matches what was uploaded *before* creating the DB record or moving + the file into its final place. This prevents "phantom" records that point + at a missing/truncated file if the underlying storage (e.g. a flaky NFS + mount) acknowledges a write that never actually lands on disk. + """ folder = _get_upload_folder() ext = Path(f.name).suffix.lower() stored_name = f"{secrets.token_hex(16)}{ext}" dest_path = os.path.join(folder, stored_name) - with open(dest_path, 'wb') as dst: - for chunk in f.chunks(): - dst.write(chunk) + tmp_path = f"{dest_path}.part" + try: + with open(tmp_path, 'wb') as dst: + for chunk in f.chunks(): + dst.write(chunk) + dst.flush() + os.fsync(dst.fileno()) + except OSError as exc: + _cleanup_partial(tmp_path) + raise FileSaveError(f"Failed to write \"{f.name}\" to storage: {exc}") from exc + + actual_size = os.path.getsize(tmp_path) + if actual_size != f.size: + _cleanup_partial(tmp_path) + raise FileSaveError( + f"Upload of \"{f.name}\" is incomplete ({actual_size}/{f.size} bytes written)." + ) + + os.rename(tmp_path, dest_path) + mime_type = f.content_type or mimetypes.guess_type(f.name)[0] or 'application/octet-stream' return FileUpload.objects.create( name=f.name, stored_name=stored_name, mime_type=mime_type, - size=f.size, + size=actual_size, ) @@ -132,8 +168,14 @@ class FileUploadView(View): return JsonResponse({'error': 'No files provided'}, status=400) return redirect('file-list') results = [] + errors = [] for f in uploaded: - record = _save_uploaded_file(f) + try: + record = _save_uploaded_file(f) + except FileSaveError as exc: + logger.error("Upload failed for %s: %s", f.name, exc) + errors.append({'name': f.name, 'error': str(exc)}) + continue results.append({ 'id': str(record.pk), 'name': record.name, @@ -149,7 +191,8 @@ class FileUploadView(View): 'is_image': record.is_image, }) if is_ajax: - return JsonResponse({'uploaded': results}) + status_code = 200 if results else 500 + return JsonResponse({'uploaded': results, 'errors': errors}, status=status_code) return redirect('file-list') @@ -264,7 +307,19 @@ class FileUploadViewSet(viewsets.ModelViewSet): uploaded = request.FILES.getlist('files') if not uploaded: return Response({'error': 'No files provided. Use "files" field.'}, status=status.HTTP_400_BAD_REQUEST) - created = [_save_uploaded_file(f) for f in uploaded] + created = [] + errors = [] + for f in uploaded: + try: + created.append(_save_uploaded_file(f)) + except FileSaveError as exc: + logger.error("Upload failed for %s: %s", f.name, exc) + errors.append({'name': f.name, 'error': str(exc)}) + if not created: + return Response( + {'error': 'Failed to save uploaded file(s).', 'errors': errors}, + status=status.HTTP_500_INTERNAL_SERVER_ERROR, + ) serializer = self.get_serializer(created, many=True) return Response(serializer.data, status=status.HTTP_201_CREATED) diff --git a/links/templates/links/files/list.html b/links/templates/links/files/list.html index cc2b869..6d31c03 100644 --- a/links/templates/links/files/list.html +++ b/links/templates/links/files/list.html @@ -302,14 +302,15 @@
- + :title="u.errorMsg || ''" + x-text="u.status === 'uploading' ? u.progress + '%' : (u.errorMsg || u.status)">
{ const id = Date.now() + '_' + Math.random().toString(36).slice(2); - this.uploads.push({ id, name: file.name, progress: 0, status: 'uploading', xhr: null }); + this.uploads.push({ id, name: file.name, progress: 0, status: 'uploading', errorMsg: '', xhr: null }); this._uploadOne(file, id); }); }, @@ -544,6 +545,11 @@ function fileManager() { } catch (e) { /* ignore parse error */ } } else { u.status = 'error'; + try { + const data = JSON.parse(xhr.responseText); + const firstError = data && Array.isArray(data.errors) && data.errors[0]; + if (firstError && firstError.error) u.errorMsg = firstError.error; + } catch (e) { /* ignore parse error, keep generic 'error' */ } } } this._maybeCollapseHud(); @@ -731,4 +737,4 @@ function formatBytes(bytes) { } -{% endblock %} \ No newline at end of file +{% endblock %} diff --git a/static/openapi.yaml b/static/openapi.yaml index 029f274..a6355bc 100644 --- a/static/openapi.yaml +++ b/static/openapi.yaml @@ -417,6 +417,12 @@ paths: $ref: '#/components/schemas/FileUpload' '400': description: No files provided + '500': + description: >- + None of the uploaded files could be fully persisted to storage + (e.g. a storage write failure or truncated upload). The response + body includes an `errors` array with a `name`/`error` entry per + failed file. /api/files/{id}: get: diff --git a/tests/test_files_api.py b/tests/test_files_api.py index f7870da..094bbf4 100644 --- a/tests/test_files_api.py +++ b/tests/test_files_api.py @@ -73,6 +73,28 @@ class TestFilesAPI: r = api_client.post(f"{self.BASE}", {}, format="multipart") assert r.status_code == 400 + def test_upload_write_failure_returns_500_and_no_phantom_record(self, api_client, monkeypatch): + """If the file can't be fully persisted to disk (e.g. storage failure), + no FileUpload DB record should be created — otherwise it points at a + file that will always 404.""" + import os as _os + + from links import file_views + + def _boom(*args, **kwargs): + raise OSError("simulated storage failure") + + monkeypatch.setattr(file_views.os, "fsync", _boom) + + from links.models import FileUpload + + r = api_client.post(f"{self.BASE}", {"files": _png()}, format="multipart") + assert r.status_code == 500 + assert "errors" in r.json() + assert FileUpload.objects.count() == 0 + # No leftover partial file should remain in the uploads folder. + assert list(_os.scandir(file_views._get_upload_folder())) == [] + def test_delete_file(self, api_client): pk = api_client.post( f"{self.BASE}", {"files": _png()}, format="multipart"