fix(review): guard placeholder in scanner reconcile + test dual-write path
Round-1 review follow-ups (PR #857): - scanner.py: skip the rename-reconcile when the existing metadata point is a placeholder. reconcile_document_path only touches real chunks, so a not-yet- indexed file would just incur a 0-point set_payload; the real index writes the current path anyway. - test_sharing_state.py: add a dedup-hit case where the file was renamed AND the user is new to the ACL, asserting both set_payload writes fire (file_path/title and acl_principals). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
bded41de5d
commit
a03bc0af66
@@ -587,13 +587,18 @@ async def scan_user_documents(
|
|||||||
)
|
)
|
||||||
)
|
)
|
||||||
file_queued += 1
|
file_queued += 1
|
||||||
elif existing_metadata is not None:
|
elif existing_metadata is not None and not existing_metadata.get(
|
||||||
|
"is_placeholder", False
|
||||||
|
):
|
||||||
# Unchanged content (not re-queued) but the file may have
|
# Unchanged content (not re-queued) but the file may have
|
||||||
# been renamed/moved: a rename keeps the fileid while
|
# been renamed/moved: a rename keeps the fileid while
|
||||||
# changing the path, and the dedup miss here means the
|
# changing the path, and the dedup miss here means the
|
||||||
# etag changed without a modified_at bump. Refresh the
|
# etag changed without a modified_at bump. Refresh the
|
||||||
# stale path/title metadata without re-embedding. No-op
|
# stale path/title metadata without re-embedding. No-op
|
||||||
# when the path is unchanged.
|
# when the path is unchanged. Skip placeholders: reconcile
|
||||||
|
# only touches real chunks, so a not-yet-indexed file would
|
||||||
|
# just incur a 0-point set_payload (the real index writes
|
||||||
|
# the current path anyway).
|
||||||
try:
|
try:
|
||||||
await reconcile_document_path(
|
await reconcile_document_path(
|
||||||
file_id,
|
file_id,
|
||||||
|
|||||||
@@ -210,6 +210,31 @@ class TestClaimExistingIndex:
|
|||||||
assert payload["file_path"] == "/a/new.pdf"
|
assert payload["file_path"] == "/a/new.pdf"
|
||||||
assert payload["title"] == "new.pdf"
|
assert payload["title"] == "new.pdf"
|
||||||
|
|
||||||
|
async def test_dedup_hit_reconciles_and_grants_new_principal(self, client) -> None:
|
||||||
|
# Renamed file (stale path) AND a user not yet in the ACL: both writes
|
||||||
|
# fire — one set_payload for file_path/title, one for acl_principals.
|
||||||
|
client.scroll.return_value = (
|
||||||
|
[
|
||||||
|
_point(
|
||||||
|
{
|
||||||
|
payload_keys.EMBEDDING_IDENTITY: _MODEL,
|
||||||
|
ss.ACL_PRINCIPALS_KEY: ["user:alice"],
|
||||||
|
"file_path": "/a/old.pdf",
|
||||||
|
}
|
||||||
|
)
|
||||||
|
],
|
||||||
|
None,
|
||||||
|
)
|
||||||
|
claimed = await ss.claim_existing_index(
|
||||||
|
"42", "file", "abc", "bob", current_path="/a/new.pdf"
|
||||||
|
)
|
||||||
|
assert claimed is True
|
||||||
|
assert client.set_payload.await_count == 2
|
||||||
|
payloads = [c.kwargs["payload"] for c in client.set_payload.await_args_list]
|
||||||
|
# One write refreshes the path/title, the other unions the new principal.
|
||||||
|
assert {"file_path": "/a/new.pdf", "title": "new.pdf"} in payloads
|
||||||
|
assert {ss.ACL_PRINCIPALS_KEY: ["user:alice", "user:bob"]} in payloads
|
||||||
|
|
||||||
async def test_dedup_hit_without_current_path_skips_reconcile(self, client) -> None:
|
async def test_dedup_hit_without_current_path_skips_reconcile(self, client) -> None:
|
||||||
# No current_path (non-file callers) -> never touches file_path/title.
|
# No current_path (non-file callers) -> never touches file_path/title.
|
||||||
client.scroll.return_value = (
|
client.scroll.return_value = (
|
||||||
|
|||||||
Reference in New Issue
Block a user