aws / aws/aws-cli

s3 transfer stats corrupted when two concurrent transfers share the same (transfer_type, src, dest)

Open
#10,589 0 comments 0 reactions 0 assignees View on GitHub
needs-triage
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.