Skip to content

fix(gateway): serialize inbound uploads so same-name writes do not collide - #1916

Open
Jay0130-a wants to merge 1 commit into
TencentCloud:mainfrom
Jay0130-a:fix/concurrent-inbound-upload-collision
Open

Jay0130-a wants to merge 1 commit into
TencentCloud:mainfrom
Jay0130-a:fix/concurrent-inbound-upload-collision

Conversation

@Jay0130-a

Copy link
Copy Markdown

Fixes #1889.

Problem

write_inbound allocates a destination and writes to it in two separate awaits:

path = await _unique_inbound_path(workspace, stored_name)   # existence check
await workspace.aupload_bytes(path, data)                   # write

_unique_inbound_path only looks for a free name, it does not reserve one.
Two requests that arrive within the same second build the same
{unix_ts}_{display} name, both observe the candidate as absent, and both
upload to it. Both responses succeed and carry the same path, but only the
second payload survives — one document is lost. The dashboard uploads several
attachments concurrently, so this is reachable from the UI.

Fix

Allocate and write under a per-workspace asyncio.Lock, so a colliding upload
observes the file its predecessor just wrote and picks the existing
{stem}-{n}{suffix} fallback. The lock is cached per workspace directory and
rebuilt when the running event loop changes, so a module-level cache never
hands a lock bound to a closed loop to a new one.

Tests

tests/unit/gateway/test_inbound_store.py gains
test_concurrent_same_name_uploads_keep_their_own_bytes, which uploads two
different payloads named report.txt concurrently and asserts two distinct
paths whose contents are the two original payloads.

Before the fix the test fails on the shared path:

AssertionError: assert 1 == 2
 +  where 1 = len({'inbound/1791765927_report.txt'})

After the fix:

$ python -m pytest tests/unit/gateway/test_inbound_store.py -q
8 passed

…llide

`write_inbound` picks a free path with `_unique_inbound_path` (an existence
check) and only then uploads to it. Both steps await, so two concurrent
uploads whose timestamped names collide can observe the same free candidate
and write to the same key — both calls succeed and return the same path, and
the first upload's bytes are silently lost.

Guard allocate+write with a per-workspace lock so a colliding upload sees the
file its predecessor just wrote and takes the `-{n}` suffix instead.

The regression test drives two concurrent uploads of `report.txt` with
different payloads and asserts they get distinct paths and keep their own
bytes; it fails on the current code with a single shared path.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] Concurrent same-name inbound uploads overwrite each other

1 participant