From 1d61d1d7963a7f926b940fa900aabcad7b999a9b Mon Sep 17 00:00:00 2001 From: Chad Palmer Date: Mon, 5 Oct 2026 19:58:43 +0000 Subject: [PATCH] fix-large-document-tag-processing - Fixed issue where large document metadata tag updates timed out. --- application/single_app/config.py | 2 +- application/single_app/functions_documents.py | 39 ++++- .../single_app/route_backend_documents.py | 108 +++++++------- .../js/workspace/workspace-documents.js | 32 ++++- ...CUMENT_METADATA_CHUNK_SYNC_BATCHING_FIX.md | 38 +++++ docs/explanation/release_notes.md | 10 ++ ...t_document_metadata_chunk_sync_batching.py | 136 ++++++++++++++++++ 7 files changed, 296 insertions(+), 69 deletions(-) create mode 100644 docs/explanation/fixes/DOCUMENT_METADATA_CHUNK_SYNC_BATCHING_FIX.md create mode 100644 functional_tests/test_document_metadata_chunk_sync_batching.py diff --git a/application/single_app/config.py b/application/single_app/config.py index 5a2d077a7..632b5360e 100644 --- a/application/single_app/config.py +++ b/application/single_app/config.py @@ -98,7 +98,7 @@ EXECUTOR_TYPE = 'thread' EXECUTOR_MAX_WORKERS = 30 SESSION_TYPE = 'filesystem' -VERSION = "0.261.050" +VERSION = "0.261.051" IS_DEVELOPMENT = is_development_env_enabled() # Opt-out for deployments where App Service Easy Auth is active but the platform diff --git a/application/single_app/functions_documents.py b/application/single_app/functions_documents.py index d3a879016..d690a6fbb 100644 --- a/application/single_app/functions_documents.py +++ b/application/single_app/functions_documents.py @@ -3308,6 +3308,19 @@ def update_document(**kwargs): group_id = kwargs.get('group_id') public_workspace_id = kwargs.get('public_workspace_id') num_chunks_increment = kwargs.pop('num_chunks_increment', 0) + chunk_sync_limit = kwargs.pop('chunk_sync_limit', None) + chunk_sync_offset = max(int(kwargs.pop('chunk_sync_offset', 0) or 0), 0) + requested_chunk_sync_fields = set(kwargs.pop('chunk_sync_fields', []) or []) + supported_chunk_sync_fields = {'title', 'authors', 'file_name', 'document_classification', 'tags', 'shared_group_ids'} + requested_chunk_sync_fields = requested_chunk_sync_fields.intersection(supported_chunk_sync_fields) + chunk_sync_result = { + 'required': False, + 'complete': True, + 'fields': [], + 'total': 0, + 'processed': 0, + 'next_offset': None, + } if not document_id or not user_id: # Cannot proceed without these identifiers @@ -3460,11 +3473,13 @@ def update_document(**kwargs): else: existing_document['percentage_complete'] = new_percentage + updated_fields_requiring_chunk_sync.update(requested_chunk_sync_fields) + # 4. Propagate relevant changes to search index chunks # This happens regardless of 'update_occurred' flag because the *intent* from kwargs might trigger it, # even if the main doc update didn't happen (e.g., only percentage changed). # However, it's better to only do this if the relevant fields *actually* changed. - if update_occurred and updated_fields_requiring_chunk_sync: + if updated_fields_requiring_chunk_sync: try: chunks_to_update = get_all_chunks( document_id, @@ -3472,7 +3487,18 @@ def update_document(**kwargs): group_id=group_id, public_workspace_id=public_workspace_id ) - for chunk in chunks_to_update: + total_chunks = len(chunks_to_update) + chunk_sync_result['required'] = total_chunks > 0 + chunk_sync_result['fields'] = sorted(updated_fields_requiring_chunk_sync) + chunk_sync_result['total'] = total_chunks + + if chunk_sync_limit is None: + chunk_sync_end = total_chunks + else: + chunk_sync_limit = max(int(chunk_sync_limit), 1) + chunk_sync_end = min(total_chunks, chunk_sync_offset + chunk_sync_limit) + + for chunk in chunks_to_update[chunk_sync_offset:chunk_sync_end]: chunk_updates = {} if 'title' in updated_fields_requiring_chunk_sync: chunk_updates['title'] = existing_document.get('title') @@ -3502,10 +3528,15 @@ def update_document(**kwargs): update_params['shared_group_ids'] = existing_document.get('shared_group_ids') update_chunk_metadata(**update_params) + chunk_sync_result['processed'] += 1 + + if chunk_sync_end < total_chunks: + chunk_sync_result['complete'] = False + chunk_sync_result['next_offset'] = chunk_sync_end add_file_task_to_file_processing_log( document_id=document_id, user_id=public_workspace_id if is_public_workspace else (group_id if is_group else user_id), - content=f"Propagated updates for fields {updated_fields_requiring_chunk_sync} to search chunks." + content=f"Propagated updates for fields {updated_fields_requiring_chunk_sync} to {chunk_sync_result['processed']} of {total_chunks} search chunks." ) except Exception as chunk_sync_error: # Log error but don't necessarily fail the whole document update @@ -3526,6 +3557,8 @@ def update_document(**kwargs): operation='document_updated', ) + return {'updated': update_occurred, 'chunk_sync': chunk_sync_result} + except CosmosResourceNotFoundError as e: # Error already logged where it was first detected print(f"Document {document_id} not found or access denied: {e}") diff --git a/application/single_app/route_backend_documents.py b/application/single_app/route_backend_documents.py index d85c95588..d6d7ea773 100644 --- a/application/single_app/route_backend_documents.py +++ b/application/single_app/route_backend_documents.py @@ -1044,82 +1044,58 @@ def api_patch_user_document(document_id): if not user_id: return jsonify({'error': 'User not authenticated'}), 401 - data = request.get_json() # new metadata values from the client + data = request.get_json() or {} # new metadata values from the client + chunk_sync_state = data.get('_metadata_chunk_sync') if isinstance(data.get('_metadata_chunk_sync'), dict) else {} + try: + chunk_sync_offset = max(int(chunk_sync_state.get('next_offset', 0) or 0), 0) + except (TypeError, ValueError): + return jsonify({'error': 'Invalid metadata chunk sync state'}), 400 + chunk_sync_fields = chunk_sync_state.get('fields') if isinstance(chunk_sync_state.get('fields'), list) else [] + is_chunk_sync_continuation = chunk_sync_offset > 0 and bool(chunk_sync_fields) # Track which fields were updated updated_fields = {} + document_updates = {} # Update allowed fields # You can decide which fields can be updated from the client if 'title' in data: - update_document( - document_id=document_id, - user_id=user_id, - title=data['title'] - ) + document_updates['title'] = data['title'] updated_fields['title'] = data['title'] if 'abstract' in data: - update_document( - document_id=document_id, - user_id=user_id, - abstract=data['abstract'] - ) + document_updates['abstract'] = data['abstract'] updated_fields['abstract'] = data['abstract'] if 'keywords' in data: # Expect a list or a comma-delimited string if isinstance(data['keywords'], list): - update_document( - document_id=document_id, - user_id=user_id, - keywords=data['keywords'] - ) + document_updates['keywords'] = data['keywords'] updated_fields['keywords'] = data['keywords'] else: # if client sends a comma-separated string of keywords keywords_list = [kw.strip() for kw in data['keywords'].split(',')] - update_document( - document_id=document_id, - user_id=user_id, - keywords=keywords_list - ) + document_updates['keywords'] = keywords_list updated_fields['keywords'] = keywords_list if 'publication_date' in data: - update_document( - document_id=document_id, - user_id=user_id, - publication_date=data['publication_date'] - ) + document_updates['publication_date'] = data['publication_date'] updated_fields['publication_date'] = data['publication_date'] if 'document_classification' in data: - update_document( - document_id=document_id, - user_id=user_id, - document_classification=data['document_classification'] - ) + document_updates['document_classification'] = data['document_classification'] updated_fields['document_classification'] = data['document_classification'] # Add authors if you want to allow editing that if 'authors' in data: # if you want a list, or just store a string # here is one approach: if isinstance(data['authors'], list): - update_document( - document_id=document_id, - user_id=user_id, - authors=data['authors'] - ) + document_updates['authors'] = data['authors'] updated_fields['authors'] = data['authors'] else: authors_list = [data['authors']] - update_document( - document_id=document_id, - user_id=user_id, - authors=authors_list - ) + document_updates['authors'] = authors_list updated_fields['authors'] = authors_list # Handle tags with validation and chunk propagation if 'tags' in data: - from functions_documents import validate_tags, propagate_tags_to_chunks, get_or_create_tag_definition + from functions_documents import validate_tags, get_or_create_tag_definition # Validate and normalize tags tags_input = data['tags'] if isinstance(data['tags'], list) else [] @@ -1133,24 +1109,30 @@ def api_patch_user_document(document_id): get_or_create_tag_definition(user_id, tag, workspace_type='personal') # Update document with normalized tags - update_document( - document_id=document_id, - user_id=user_id, - tags=normalized_tags - ) + document_updates['tags'] = normalized_tags updated_fields['tags'] = normalized_tags - - # Propagate tags to all chunks immediately - try: - propagate_tags_to_chunks(document_id, normalized_tags, user_id) - except Exception as propagate_error: - debug_print(f"Warning: Failed to propagate tags to chunks: {propagate_error}") - # Continue - document tags are updated, chunk sync will be retried later # Save updates back to Cosmos try: + update_result = {'chunk_sync': {'complete': True}} + if document_updates: + update_result = update_document( + document_id=document_id, + user_id=user_id, + chunk_sync_limit=75, + chunk_sync_offset=chunk_sync_offset, + chunk_sync_fields=chunk_sync_fields, + **document_updates, + ) or update_result + + if 'tags' in updated_fields and not is_chunk_sync_continuation: + try: + propagate_tags_to_blob_metadata(document_id, updated_fields['tags'], user_id) + except Exception as blob_metadata_error: + debug_print(f"Warning: Failed to propagate tags to blob metadata: {blob_metadata_error}") + # Log the metadata update transaction if any fields were updated - if updated_fields: + if updated_fields and not is_chunk_sync_continuation: # Get document details for logging doc_response = get_document(user_id, document_id) doc = None @@ -1172,10 +1154,20 @@ def api_patch_user_document(document_id): updated_fields=updated_fields, file_type=doc.get('file_type') ) - - return jsonify({'message': 'Document metadata updated successfully'}), 200 + + chunk_sync = update_result.get('chunk_sync', {}) if isinstance(update_result, dict) else {} + response_payload = {'message': 'Document metadata updated successfully'} + if chunk_sync.get('required'): + response_payload['metadata_chunk_sync'] = chunk_sync + + return jsonify(response_payload), 200 except Exception as e: - return jsonify({'error': str(e)}), 500 + log_event( + '[DOCUMENT_METADATA] Failed to update personal document metadata', + {'document_id': document_id, 'error_type': type(e).__name__}, + debug_only=True, + ) + return jsonify({'error': 'Unable to update document metadata'}), 500 @bp.route('/api/documents/', methods=['DELETE']) @swagger_route(security=get_auth_security()) diff --git a/application/single_app/static/js/workspace/workspace-documents.js b/application/single_app/static/js/workspace/workspace-documents.js index 92c1de3f3..667b8c60b 100644 --- a/application/single_app/static/js/workspace/workspace-documents.js +++ b/application/single_app/static/js/workspace/workspace-documents.js @@ -802,6 +802,29 @@ function renderWorkspaceDocumentView() { window.renderWorkspaceDocumentView = renderWorkspaceDocumentView; +async function patchDocumentMetadata(documentId, payload) { + let chunkSync = null; + do { + const requestPayload = { ...payload }; + if (chunkSync && !chunkSync.complete) { + requestPayload._metadata_chunk_sync = chunkSync; + } + + const response = await fetch(`/api/documents/${documentId}`, { + method: "PATCH", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify(requestPayload), + }); + + const responseData = await response.json().catch(() => ({})); + if (!response.ok) { + throw responseData.error ? responseData : { error: `Server responded with status ${response.status}` }; + } + + chunkSync = responseData.metadata_chunk_sync || null; + } while (chunkSync && !chunkSync.complete); +} + function getDocumentDeleteModalContent(documentCount) { if (documentCount === 1) { return { @@ -1428,13 +1451,8 @@ if (docMetadataForm && docMetadataModalEl) { // Check both exist payload.document_classification = selectedClassification; } - fetch(`/api/documents/${docId}`, { - method: "PATCH", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify(payload), - }) - .then(r => r.ok ? r.json() : r.json().then(err => Promise.reject(err))) - .then(updatedDoc => { + patchDocumentMetadata(docId, payload) + .then(() => { if (docMetadataModalEl) docMetadataModalEl.hide(); fetchUserDocuments(); // Refresh the table loadWorkspaceTags(); // Refresh tag counts and grid view diff --git a/docs/explanation/fixes/DOCUMENT_METADATA_CHUNK_SYNC_BATCHING_FIX.md b/docs/explanation/fixes/DOCUMENT_METADATA_CHUNK_SYNC_BATCHING_FIX.md new file mode 100644 index 000000000..5dbe0f24f --- /dev/null +++ b/docs/explanation/fixes/DOCUMENT_METADATA_CHUNK_SYNC_BATCHING_FIX.md @@ -0,0 +1,38 @@ +# Document Metadata Chunk Sync Batching Fix + +Fixed/Implemented in version: **0.261.051** + +## Issue Description + +Saving metadata for a large uploaded document could time out after users added or selected tags such as `bills`. The browser then attempted to parse the timeout response as JSON, which surfaced as an invalid JSON error instead of a useful completion path. + +## Root Cause Analysis + +The personal document metadata PATCH route updated the document record, then synchronously propagated tag metadata to every search chunk. The shared document update helper also propagated changed tag metadata to every chunk, so tag saves could perform duplicate full-document chunk updates during a single request. + +## Technical Details + +Files modified: + +- `application/single_app/functions_documents.py` +- `application/single_app/route_backend_documents.py` +- `application/single_app/static/js/workspace/workspace-documents.js` +- `functional_tests/test_document_metadata_chunk_sync_batching.py` +- `application/single_app/config.py` + +Code changes summary: + +- Added bounded chunk-sync support to `update_document()` with `chunk_sync_limit`, `chunk_sync_offset`, and `chunk_sync_fields`. +- Updated the personal metadata route to apply metadata changes once and return chunk-sync progress when large chunk updates need continuation. +- Updated the workspace metadata form to continue the same PATCH request until chunk synchronization completes. +- Removed the duplicate immediate search chunk tag propagation call from the personal metadata route while preserving quick blob metadata tag updates. + +## Validation + +Validation coverage: + +- `functional_tests/test_document_metadata_chunk_sync_batching.py` verifies that a large metadata sync returns a continuation after a bounded batch and resumes from the saved offset without requiring the document metadata to change again. + +Expected user impact: + +- Large XML and other high-chunk-count documents can finish metadata/tag saves across multiple shorter requests instead of timing out during one long request. \ No newline at end of file diff --git a/docs/explanation/release_notes.md b/docs/explanation/release_notes.md index fb5ce3f8a..7242f157b 100644 --- a/docs/explanation/release_notes.md +++ b/docs/explanation/release_notes.md @@ -2,6 +2,16 @@ For feature-focused and fix-focused drill-downs by version, see [Features by Version](https://github.com/microsoft/simplechat/tree/main/docs/explanation/features) and [Fixes by Version](https://github.com/microsoft/simplechat/tree/main/docs/explanation/fixes). +### **(v0.261.051)** + +#### Bug Fixes + +* **Large Document Metadata Saves Continue Across Batches** + * Saving tags or other metadata for large uploaded documents now synchronizes search chunk metadata in bounded batches instead of holding one long request until every chunk is updated. + * The workspace metadata dialog continues the save through short JSON PATCH requests until the chunk sync is complete, avoiding timeout HTML responses that previously appeared in the browser as invalid JSON errors. + * Tag saves no longer run duplicate full-document search chunk propagation, while blob metadata tag updates are still preserved. + * (Ref: document metadata updates, tag propagation, `functions_documents.py`, `route_backend_documents.py`, `workspace-documents.js`, [Document Metadata Chunk Sync Batching Fix](fixes/DOCUMENT_METADATA_CHUNK_SYNC_BATCHING_FIX.md)) + ### **(v0.261.050)** #### Bug Fixes diff --git a/functional_tests/test_document_metadata_chunk_sync_batching.py b/functional_tests/test_document_metadata_chunk_sync_batching.py new file mode 100644 index 000000000..c9368bc07 --- /dev/null +++ b/functional_tests/test_document_metadata_chunk_sync_batching.py @@ -0,0 +1,136 @@ +#!/usr/bin/env python3 +""" +Functional test for document metadata chunk sync batching. +Version: 0.261.051 +Implemented in: 0.261.051 + +This test ensures that large document metadata updates can synchronize search +chunk metadata in resumable batches instead of requiring one long request. +""" + +import os +import sys +import ast +import json +import traceback +from copy import deepcopy +from datetime import datetime, timezone + +REPO_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +APP_ROOT = os.path.join(REPO_ROOT, 'application', 'single_app') +sys.path.append(REPO_ROOT) +sys.path.append(APP_ROOT) + +from test_support.versioning import assert_app_version_at_least + + +class FakeDocumentsContainer: + def __init__(self, document): + self.document = document + + def query_items(self, query, parameters, enable_cross_partition_query=True): + return [self.document] + + +class FakeCosmosResourceNotFoundError(Exception): + def __init__(self, message=None, status=None): + super().__init__(message) + self.status = status + + +def load_update_document_for_test(namespace): + """Load the real update_document source into a controlled test namespace.""" + source_path = os.path.join(APP_ROOT, 'functions_documents.py') + with open(source_path, 'r', encoding='utf-8') as source_file: + module_ast = ast.parse(source_file.read(), filename=source_path) + + update_function = next( + node for node in module_ast.body + if isinstance(node, ast.FunctionDef) and node.name == 'update_document' + ) + function_module = ast.Module(body=[update_function], type_ignores=[]) + ast.fix_missing_locations(function_module) + exec(compile(function_module, source_path, 'exec'), namespace) + return namespace['update_document'] + + +def test_metadata_chunk_sync_batches_and_resumes(): + """Test chunk metadata synchronization continuation across batches.""" + print("Testing document metadata chunk sync batching...") + assert_app_version_at_least("0.261.051") + + document = { + 'id': 'doc-large-xml', + 'user_id': 'user-123', + 'title': 'Large XML', + 'tags': [], + 'status': 'Processing Complete', + 'percentage_complete': 100, + } + chunks = [{'id': f'chunk-{index}'} for index in range(5)] + chunk_updates = [] + upserted_documents = [] + namespace = { + 'datetime': datetime, + 'timezone': timezone, + 'json': json, + 'traceback': traceback, + 'CosmosResourceNotFoundError': FakeCosmosResourceNotFoundError, + 'cosmos_user_documents_container': FakeDocumentsContainer(document), + 'cosmos_group_documents_container': FakeDocumentsContainer(document), + 'cosmos_public_documents_container': FakeDocumentsContainer(document), + 'add_file_task_to_file_processing_log': lambda **kwargs: None, + 'calculate_processing_percentage': lambda doc: doc.get('percentage_complete', 0), + 'get_all_chunks': lambda document_id, user_id, group_id=None, public_workspace_id=None: chunks, + 'ensure_list': lambda value: value if isinstance(value, list) else [value], + 'update_chunk_metadata': lambda **kwargs: chunk_updates.append(kwargs), + '_upsert_document_and_sync_access_index': ( + lambda container, doc, operation: upserted_documents.append(deepcopy(doc)) + ), + } + update_document = load_update_document_for_test(namespace) + + first_result = update_document( + document_id='doc-large-xml', + user_id='user-123', + tags=['bills'], + chunk_sync_limit=2, + ) + + assert first_result['updated'] is True + assert first_result['chunk_sync']['complete'] is False + assert first_result['chunk_sync']['next_offset'] == 2 + assert first_result['chunk_sync']['processed'] == 2 + assert first_result['chunk_sync']['total'] == 5 + assert [update['chunk_id'] for update in chunk_updates] == ['chunk-0', 'chunk-1'] + assert upserted_documents[-1]['tags'] == ['bills'] + + second_result = update_document( + document_id='doc-large-xml', + user_id='user-123', + tags=['bills'], + chunk_sync_limit=10, + chunk_sync_offset=2, + chunk_sync_fields=['tags'], + ) + + assert second_result['updated'] is False + assert second_result['chunk_sync']['complete'] is True + assert second_result['chunk_sync']['next_offset'] is None + assert second_result['chunk_sync']['processed'] == 3 + assert [update['chunk_id'] for update in chunk_updates] == [ + 'chunk-0', + 'chunk-1', + 'chunk-2', + 'chunk-3', + 'chunk-4', + ] + assert all(update['document_tags'] == ['bills'] for update in chunk_updates) + + print("Document metadata chunk sync batching test passed.") + return True + + +if __name__ == "__main__": + success = test_metadata_chunk_sync_batches_and_resumes() + sys.exit(0 if success else 1) \ No newline at end of file