fix: prevent cross-KB data loss in sync, improve dedup and hash memory profile

- remove_file_from_knowledge_by_id with delete_file=True now hard-deletes
  the file record, storage blob, and per-file vector collection only
  when no other knowledge base still references that file. If another
  KB still has the file attached, the request degrades to a KB-scoped
  unlink so syncing KB-A can't silently wipe a file from KB-B. This
  endpoint also now deletes the object-storage blob on hard-delete,
  closing the previously-flagged storage leak.
- upload_and_replace_file Step 4 switches from the global file delete
  route to this KB-scoped helper so the same cross-KB safeguard applies
  to replace flows.
- upload_and_replace_file preserves HTTPException from upload and
  remove steps instead of flattening every upstream status/detail into
  a generic 400 via str(exc).
- Frontend syncDirectoryHandler remove loop switches back to
  removeFileFromKnowledgeById now that the backend endpoint handles
  shared files and storage cleanup safely.
- compare_files_for_sync duplicate detection uses collections.Counter
  for O(n) dedup instead of list.count inside a set comprehension
  (O(n^2)), which matters for large directory payloads.
- Empty incoming file_hash now falls through to the size-based
  comparison branch instead of always being treated as changed, so the
  browser can skip hashing large files without every such file being
  flagged as modified.
- Browser calculateFileHash skips files above 100 MB (SubtleCrypto has
  no streaming digest API) and returns an empty hash, letting the
  server fall back to size comparison and avoiding tab OOM on very
  large files.
This commit is contained in:
DrMelone 2026-04-13 00:05:50 +02:00
parent 74163ce963
commit 5aebd6df65
2 changed files with 93 additions and 31 deletions

View file

@ -761,8 +761,13 @@ async def upload_and_replace_file(
db=db,
)
new_file_id = new_file_result['id']
except HTTPException:
# Preserve upstream status/detail (e.g. 413 payload-too-large or
# validation-specific 400 messages) instead of flattening everything
# to a generic 400 with str(exc).
raise
except Exception as e:
log.error(f'Failed to upload new file: {e}')
log.exception(f'Failed to upload new file: {e}')
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail=f'Failed to upload file: {str(e)}',
@ -827,15 +832,24 @@ async def upload_and_replace_file(
detail=f'Failed to add file to knowledge base: {str(e)}',
)
# Step 4: Remove old file. Use the files router delete handler so the
# object storage blob is removed too — the KB-scoped remove_file helper
# drops the DB row and vector entries but not the Storage blob, which
# would accumulate orphans in S3/GCS/local storage over repeated syncs.
# Step 4: Remove old file via the KB-scoped helper. That route now
# hard-deletes the file row, storage blob, and per-file vector collection
# only when no other knowledge base still references the old file —
# otherwise it degrades to a scoped unlink so replace-in-KB-A doesn't
# silently wipe the same file from KB-B.
# Any failure here means "replace" semantics were violated (new file
# added but old file still present); surface the error so callers can
# reconcile instead of silently accumulating duplicates.
try:
await delete_file_by_id_route(id=old_file_id, user=user, db=db)
await remove_file_from_knowledge_by_id(
id=id,
form_data=KnowledgeFileIdForm(file_id=old_file_id),
delete_file=True,
user=user,
db=db,
)
except HTTPException:
raise
except Exception as e:
log.error(f'Failed to remove old file after replacement upload: {e}')
raise HTTPException(
@ -991,18 +1005,42 @@ async def remove_file_from_knowledge_by_id(
pass
if delete_file:
try:
# Remove the file's collection from vector database
file_collection = f'file-{form_data.file_id}'
if VECTOR_DB_CLIENT.has_collection(collection_name=file_collection):
VECTOR_DB_CLIENT.delete_collection(collection_name=file_collection)
except Exception as e:
log.debug('This was most likely caused by bypassing embedding processing')
log.debug(e)
pass
# Only hard-delete the file record, storage blob, and per-file vector
# collection if no other knowledge base still references this file.
# Otherwise the request becomes a KB-scoped unlink so callers (e.g.
# the directory-sync remove loop) can't silently wipe a file that is
# shared with another KB.
remaining_kbs = await Knowledges.get_knowledges_by_file_id(
form_data.file_id, db=db
)
if not remaining_kbs:
try:
# Remove the file's collection from vector database
file_collection = f'file-{form_data.file_id}'
if VECTOR_DB_CLIENT.has_collection(collection_name=file_collection):
VECTOR_DB_CLIENT.delete_collection(collection_name=file_collection)
except Exception as e:
log.debug('This was most likely caused by bypassing embedding processing')
log.debug(e)
pass
# Delete file from database
await Files.delete_file_by_id(form_data.file_id, db=db)
# Delete the object-storage blob before dropping the DB row so we
# still have file.path available; previously this endpoint only
# removed the DB record and orphaned the blob in S3/GCS/local.
try:
Storage.delete_file(file.path)
except Exception as storage_err:
log.warning(
f'Failed to delete storage blob for {form_data.file_id}: {storage_err}'
)
# Delete file from database
await Files.delete_file_by_id(form_data.file_id, db=db)
else:
log.info(
f'File {form_data.file_id} still referenced by {len(remaining_kbs)} '
f'other knowledge base(s); skipping hard-delete.'
)
if knowledge:
return KnowledgeFilesResponse(
@ -1204,12 +1242,14 @@ async def compare_files_for_sync(
# the same existing file for replacement or removal twice, producing
# downstream 404s on the second pass and muddying the success/failure
# counts on the client. Public API robustness shouldn't depend on the
# frontend deduping.
incoming_paths = [incoming.file_path for incoming in form_data.files]
if len(incoming_paths) != len(set(incoming_paths)):
duplicates = sorted(
{path for path in incoming_paths if incoming_paths.count(path) > 1}
)
# frontend deduping. Collect duplicates in a single pass (Counter is O(n))
# — iterating list.count per entry is O(n^2) and becomes a hotspot for
# large directory syncs.
from collections import Counter
path_counts = Counter(incoming.file_path for incoming in form_data.files)
duplicates = sorted(path for path, count in path_counts.items() if count > 1)
if duplicates:
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail=f'Duplicate file_path entries in sync payload: {", ".join(duplicates)}',
@ -1254,8 +1294,12 @@ async def compare_files_for_sync(
# Check if hash is already stored in meta (files uploaded after this feature)
stored_hash = existing_file.meta.get('file_hash') if existing_file.meta else None
if stored_hash:
# Modern file with stored hash - use accurate hash comparison
if stored_hash and incoming_file.file_hash:
# Modern file with stored hash AND client supplied a hash -
# use accurate hash comparison. Empty incoming hash falls
# through to the size-based fallback so the browser can skip
# hashing large files without every such file being flagged
# as changed.
if stored_hash == incoming_file.file_hash:
# File unchanged
unchanged.append(incoming_file.file_path)

View file

@ -392,8 +392,19 @@
return path.split('/').some((part) => part.startsWith('.'));
};
// Calculate SHA-256 hash of a file in the browser
// SubtleCrypto.digest has no streaming API, so hashing requires loading
// the full file into an ArrayBuffer. For very large files that can freeze
// or crash the tab. Skip hashing above this threshold and let the
// backend fall back to size-based comparison (already supported for
// legacy files without a stored hash).
const MAX_BROWSER_HASH_BYTES = 100 * 1024 * 1024; // 100 MB
// Calculate SHA-256 hash of a file in the browser. Returns '' when the
// file exceeds the in-memory hashing threshold.
const calculateFileHash = async (file: File): Promise<string> => {
if (file.size > MAX_BROWSER_HASH_BYTES) {
return '';
}
const buffer = await file.arrayBuffer();
const hashBuffer = await crypto.subtle.digest('SHA-256', buffer);
return Array.from(new Uint8Array(hashBuffer))
@ -566,10 +577,13 @@
// If a file was incorrectly classified as both "new" and "removed"
// (due to filename matching issues), deleting first allows the
// subsequent upload to succeed.
// Use deleteFileById (DELETE /files/{id}) so the object-storage
// blob, the per-file vector collection, and KB associations are
// all removed — removeFileFromKnowledgeById leaves storage blobs
// behind and would accumulate orphans over repeated syncs.
// Use removeFileFromKnowledgeById (the KB-scoped /knowledge/{id}/
// file/remove route). The backend endpoint now hard-deletes the
// file record, storage blob, and per-file vector collection only
// when no other knowledge base references the file — otherwise it
// degrades to a scoped unlink, so syncing KB-A never wipes a file
// that is also attached to KB-B. Global DELETE /files/{id} would
// not have that safeguard.
let removedSucceeded = 0;
let removedFailed = 0;
if (removed_file_ids.length > 0) {
@ -580,7 +594,11 @@
);
for (const fileId of removed_file_ids) {
try {
const res = await deleteFileById(localStorage.token, fileId);
const res = await removeFileFromKnowledgeById(
localStorage.token,
id,
fileId
);
if (res) {
removedSucceeded++;
} else {