[fix] Retry a lost storage-unit request instead of failing the job - #171
[fix] Retry a lost storage-unit request instead of failing the job#171adoda wants to merge 3 commits into
Conversation
CLA Signature Guide@adoda , thanks for your pull request. The following commit(s) are not associated with a signed Contributor License Agreement (CLA).
To sign CLA, click here. To check if your email is configured correctly, refer to the FAQs. Once you've signed the CLA or updating your email, please comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfa75f2ee9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| self._put_to_single_storage_unit, | ||
| group.global_indexes, | ||
| storage_data, | ||
| target_storage_unit=su_id, | ||
| data_parser=data_parser, |
There was a problem hiding this comment.
When a PUT is committed by the storage unit but its reply is lost, this retry path sends data_parser again with a new request ID, and _handle_put executes the callable a second time. The supported public APIs currently accept arbitrary parsers without requiring idempotence, so parsers that consume references, perform external writes, or generate nondeterministic values can duplicate side effects or silently overwrite the first result. Deduplicate PUTs at the unit or avoid replaying parser-backed PUTs unless this constraint is enforced at the public API.
AGENTS.md reference: AGENTS.md:L17-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 613bba8. kv_put / kv_batch_put / put document a contract for data_parser covering keys, element count and ordering, but never side effects, so treating replay-safety as an unwritten requirement was wrong.
Rather than dedupe at the unit, which needs per-request state there, a parser-backed put is now sent once: put_data passes max_attempts=1 when data_parser is not None, so it fails exactly as it did before this change. The retry stays in force for the ordinary put, which is a plain overwrite of the same global indexes.
Tests: test_parser_backed_put_is_not_replayed asserts one attempt, test_put_without_a_parser_is_still_retried asserts the retry is not lost for the ordinary path.
| endpoint = self._describe_storage_unit(target_storage_unit) | ||
| for attempt in range(1, TQ_SIMPLE_STORAGE_MAX_ATTEMPTS + 1): | ||
| try: | ||
| return await make_request() |
There was a problem hiding this comment.
If TQ_SIMPLE_STORAGE_MAX_ATTEMPTS is configured as 0 or a negative value, this loop never runs and _request_with_retry returns None. For PUTs, asyncio.gather therefore reports success and put_data notifies the controller even though no payload was sent, leaving published metadata that points to missing data. Validate the new configuration as at least one attempt before serving requests.
AGENTS.md reference: AGENTS.md:L17-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 177667f. Reproduced first: with the count patched to 0, _request_with_retry returned None without raising, and for a put that reads as success, after which put_data would notify the controller with metadata for data that was never sent.
The attempt count is now floored at one where it is consumed, attempts_allowed = max(1, ...), so every caller gets one attempt regardless of how the value was configured. I put the floor at the use site rather than at import so it also covers the internal max_attempts override, and so it is testable without reloading the module.
Test: test_a_nonpositive_attempt_count_still_issues_one_request.
A storage unit stops answering and every client routed to it fails when its own recv timeout expires. Raising that timeout (400s to 1800s in our deployment) changed nothing. Evidence from one such failure: the unit's node was alive and serving other traffic throughout, a TCP connect to its put_get_socket succeeded, and the unit's own counters showed it fully healthy (1025 GET_DATA served, 9.6ms p99, 1.33GB RSS). It had served exactly one GET_DATA fewer than its cohort. So the unit never saw the request that timed out; it was lost between the two ends, not queued behind slow work. Ascend#168 protected the worker thread from dying, which is a different cause of the same symptom; here the thread was intact. That loss is invisible by construction. ZMQ connect is asynchronous and SNDHWM is 0, so a DEALER accepts send() into an unbounded local queue for a peer it has not reached yet. The message sits there and the caller only learns anything when its own RCVTIMEO expires, which is why no timeout value can distinguish a lost request from a slow one. Lowering SNDHWM would not help either, it only trades silent queuing for silent dropping. Retry the request on a new socket and TCP connection, which is the part that matters, up to TQ_SIMPLE_STORAGE_MAX_ATTEMPTS (default 3). Only a missing answer is retried: zmq.error.Again now raises StorageUnitTimeout, while an error the unit actually reported still surfaces on the first attempt. Replaying an attempt is safe: put is keyed by global index and overwrites, get is read-only. Make the residual failure self-diagnosing, so a next occurrence does not need another round of manual probing. After the last attempt, ask the unit for its own counters over a fresh socket with a short timeout. That probe is served by the same worker thread as put and get, so an answer proves the unit is serving and the request was lost in flight, while silence means the unit itself stopped. The failure log now carries that verdict plus tcp reachability, the unit's op counts and RSS, and the shape of the request that failed. Log volume is unchanged in the steady state. A recovered request logs one line and skips the diagnosis entirely; storage units log a request only above TQ_STORAGE_SLOW_REQUEST_SECONDS (5s) or TQ_STORAGE_LARGE_PAYLOAD_MB (256MB), both far above the single-digit-millisecond norm, so tripping either one is itself the finding. The put_data failure log no longer dumps every routed unit id, which on a large job was thousands of them per line, matching what get_data already does. Tests cover recovery on retry, the bounded attempt count, that reported errors are not retried, and each diagnosis verdict. Signed-off-by: jathonzhang <jathonzhang@tencent.com>
Codex review on Ascend#171: with TQ_SIMPLE_STORAGE_MAX_ATTEMPTS set to 0 or a negative value the retry loop never ran, so _request_with_retry returned None without raising. For a put that reads as success, and put_data then notified the controller, publishing metadata that points at data which was never sent. Floor the attempt count where it is consumed, so every caller gets one attempt regardless of how the value was configured. Signed-off-by: jathonzhang <jathonzhang@tencent.com>
|
Codex review on Ascend#171: a put whose reply was lost has already been committed by the unit, so a retry re-runs data_parser there a second time. Replaying the write itself is harmless, it overwrites the same global indexes, but the parser is not: kv_put, kv_batch_put and put accept an arbitrary callable and constrain only its keys, element count and ordering, never its side effects. A parser that consumes references or writes externally would see those effects duplicated. Send a parser-backed put once, so it fails exactly as it did before this series, and keep the retry for the ordinary put, which is a plain overwrite. Signed-off-by: jathonzhang <jathonzhang@tencent.com>
Codex review on Ascend#171: with TQ_SIMPLE_STORAGE_MAX_ATTEMPTS set to 0 or a negative value the retry loop never ran, so _request_with_retry returned None without raising. For a put that reads as success, and put_data then notified the controller, publishing metadata that points at data which was never sent. Floor the attempt count where it is consumed, so every caller gets one attempt regardless of how the value was configured. Signed-off-by: jathonzhang <jathonzhang@tencent.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Codex review on Ascend#171: a put whose reply was lost has already been committed by the unit, so a retry re-runs data_parser there a second time. Replaying the write itself is harmless, it overwrites the same global indexes, but the parser is not: kv_put, kv_batch_put and put accept an arbitrary callable and constrain only its keys, element count and ordering, never its side effects. A parser that consumes references or writes externally would see those effects duplicated. Send a parser-backed put once, so it fails exactly as it did before this series, and keep the retry for the ordinary put, which is a plain overwrite. Signed-off-by: jathonzhang <jathonzhang@tencent.com> Co-authored-by: Cursor <cursoragent@cursor.com>
bfa75f2 to
613bba8
Compare
CLA Signature Guide@adoda , thanks for your pull request. The following commit(s) are not associated with a signed Contributor License Agreement (CLA).
To sign CLA, click here. To check if your email is configured correctly, refer to the FAQs. Once you've signed the CLA or updating your email, please comment |
|
/check-cla |
CLA Signature Guide@adoda , thanks for your pull request. The following commit(s) are not associated with a signed Contributor License Agreement (CLA).
To sign CLA, click here. To check if your email is configured correctly, refer to the FAQs. Once you've signed the CLA or updating your email, please comment |
|
/check-cla |
CLA Signature Passadoda, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
|
/check-cla |
613bba8 to
28d7a13
Compare
CLA Signature Passadoda, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
1 similar comment
CLA Signature Passadoda, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
2 participants
Problem
A storage unit stops answering and every client routed to it fails when its own recv timeout
expires. Raising the timeout (400s to 1800s in our deployment) changed nothing.
Evidence from one such failure: the unit's node was alive and serving other traffic throughout, a
TCP connect to its
put_get_socketsucceeded, and the unit's own counters showed it fully healthy(1025 GET_DATA served, 9.6ms p99, 1.33GB RSS). It had served exactly one GET_DATA fewer than its
cohort. So the unit never saw the request that timed out; it was lost between the two ends, not
queued behind slow work. #168 protected the worker thread from dying, which is a different cause of
the same symptom; here the thread was intact.
That loss is invisible by construction. ZMQ connect is asynchronous and SNDHWM is 0, so a DEALER
accepts
send()into an unbounded local queue for a peer it has not reached yet. The caller onlylearns anything when its own RCVTIMEO expires, which is why no timeout value can distinguish a lost
request from a slow one. Lowering SNDHWM would not help either, it only trades silent queuing for
silent dropping.
Fix
TQ_SIMPLE_STORAGE_MAX_ATTEMPTS(default 3, floored at 1).A fresh socket and TCP connection is the part that matters. Only a missing answer is retried:
zmq.error.Againnow raisesStorageUnitTimeout, while an error the unit reported surfaces onthe first attempt. Replaying is safe where it is used: get is read-only and put overwrites the
same global indexes. A put carrying a
data_parseris the exception and is sent once, because aretry would re-run the parser on the unit and the public API does not constrain its side
effects.
over a fresh socket with a short timeout. That probe is served by the same worker thread as put
and get, so an answer proves the request was lost in flight and silence means the unit itself
stopped. The failure log carries that verdict plus tcp reachability, the unit's op counts and RSS,
and the shape of the request that failed.
diagnosis entirely; storage units log a request only above
TQ_STORAGE_SLOW_REQUEST_SECONDS(5s)or
TQ_STORAGE_LARGE_PAYLOAD_MB(256MB), both far above the single-digit-millisecond norm, sotripping either one is itself the finding. The
put_datafailure log no longer dumps everyrouted unit id, matching what
get_dataalready does.Tests
tests/test_storage_request_retry.py: 13 passed. Covers recovery on retry, the bounded attemptcount, that a nonpositive count still issues one request, that errors reported by the unit are not
retried, that a parser-backed put is not replayed while the ordinary put still is, the retry log
naming endpoint and request shape, each diagnosis verdict (lost in flight / unit not serving /
endpoint unreachable / unknown unit), and the payload-size estimate for both the put and get shapes.
Full non-e2e run: 519 passed, 8 skipped, plus
python -m compileall -q transfer_queue tutorial tests.test_yuanrong_storage_client_e2e.pyis excluded locally, it needs the optionalopenyuanrong-datasystemdependency.