s3 transfer stats corrupted when two concurrent transfers share the same (transfer_type, src, dest)
- Dominant language
- Python
- Stars
- 17.3k
- Forks
- 4.6k
- Avg merge
- 1d 2h
- Merged PRs (30d)
- 13
Description
## Describe the bug
`ResultRecorder` (`awscli/customizations/s3/results.py`) tracks in-flight `s3 cp`/`sync`/`mv`/`rm` transfers in two dicts, `_ongoing_progress` and `_ongoing_total_sizes`, keyed by `_get_ongoing_dict_key`:
```python
def _get_ongoing_dict_key(self, result):
...
key_parts = []
for result_property in [result.transfer_type, result.src, result.dest]:
if result_property is not None:
key_parts.append(ensure_text_type(result_property))
return u':'.join(key_parts)
```
The key is built only from `(transfer_type, src, dest)`. Each individual `Future` created by `s3transfer.manager.TransferManager` already has a unique `future.meta.transfer_id` (used elsewhere, e.g. `BaseResultSubscriber._result_kwargs_cache`, as the true per-transfer identity), but this identifier is dropped when the `QueuedResult`/`ProgressResult`/`SuccessResult`/`FailureResult` namedtuples are constructed and never reaches `ResultRecorder`.
**Impact:** if two transfers are ever queued concurrently with the identical `(transfer_type, src, dest)` tuple, their progress/expected-size bookkeeping collides in the same dict slot. Whichever transfer completes (succeeds or fails) first pops the shared entry, silently discarding the *other*, still in-flight transfer's accounting. The practical effect is a corrupted final summary: `bytes_failed_to_transfer`, `expected_bytes_transferred`, and the live progress display can all be wrong — a failed transfer can be reported as if 0 bytes were outstanding, for example.
## Repro (direct reproduction against the shipped `ResultRecorder` class)
```python
from awscli.customizations.s3.results import ResultRecorder, QueuedResult, ProgressResult, SuccessResult, FailureResult
r = ResultRecorder()
q1 = QueuedResult(transfer_type='upload', src='file.txt', dest='s3://bucket/file.txt', total_transfer_size=1000)
q2 = QueuedResult(transfer_type='upload', src='file.txt', dest='s3://bucket/file.txt', total_transfer_size=2000)
r(q1); r(q2)
print(r._ongoing_total_sizes) # {'upload:file.txt:s3://bucket/file.txt': 2000} <- transfer #1's 1000 was overwritten
p1 = ProgressResult(transfer_type='upload', src='file.txt', dest='s3://bucket/file.txt',
bytes_transferred=800, total_transfer_size=1000, timestamp=r.start_time + 1)
r(p1)
s1 = SuccessResult(transfer_type='upload', src='file.txt', dest='s3://bucket/file.txt')
r(s1) # pops the SHARED entry -- transfer #2's bookkeeping is wiped out too
f2 = FailureResult(transfer_type='upload', src='file.txt', dest='s3://bucket/file.txt', exception=Exception('boom'))
r(f2)
print(r.bytes_failed_to_transfer) # 0, but should reflect transfer #2's ~2000 unsent bytes
```
## Severity note (being upfront about scope)
This is a real, verified defect at the `ResultRecorder`/results-pipeline level, confirmed by direct reproduction against the shipped class. I was **not** able to construct a confirmed realistic end-to-end `aws s3 cp/sync` CLI invocation that produces two concurrent transfers sharing the exact same `(transfer_type, src, dest)` — normal `cp`/`sync` usage produces unique `(src, dest)` pairs per file, and the file-generator/dedup logic I reviewed looked sound. So I'm filing this as a real correctness bug in the stats/progress accounting layer (worth fixing defensively, since the underlying `transfer_id` is already available and unused for this exact purpose), rather than claiming a proven high-severity CLI-level data-loss scenario.
## Suggested fix
`future.meta.transfer_id` is already threaded through `BaseResultSubscriber._result_kwargs_cache` — it just isn't propagated onto the `Result` namedtuples or used by `_get_ongoing_dict_key`. I have a PR ready that:
- Adds an optional `transfer_id` field (default `None`, fully backward compatible) to the `BaseResult`-derived result types.
- Populates it from `future.meta.transfer_id` in `BaseResultSubscriber._add_to_result_kwargs_cache`.
- Uses it (when present) to disambiguate `_get_ongoing_dict_key`, so two transfers can never collide even if they share `(transfer_type, src, dest)`.
- Adds a regression test reproducing the exact collision/clobbering scenario above.
## Environment
- `aws-cli` develop branch (current)
- Confirmed `tests/unit/customizations/s3/test_results.py` has no existing coverage of two results sharing an identical `(transfer_type, src, dest)` key.
Contributor guide
Research direction
Start with awscli/customizations/s3/results.py and tests/unit/customizations/s3/test_results.py. Trace how BaseResultSubscriber populates result data and how ResultRecorder keys its in-flight dictionaries, then run the existing results tests. Done when the named regression scenario keeps concurrent transfers distinct and the test covers the corrected accounting.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- aws, python
- Domain
- cli
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100