loggerd: widen upload PUT read timeout so a slow-but-successful upload isn't retried forever - #39023
Open
cipherprofessor wants to merge 1 commit into
Conversation
…d isn't retried forever do_upload's PUT to the blob store used a flat 10s timeout. For larger qcamera.ts files over slow/cellular connections, the server receives and stores the file successfully but takes longer than 10s to ack, so requests.put raises ReadTimeout even though the upload already succeeded. Since a file is only tagged uploaded on success, this makes the uploader retry the same already-uploaded file forever. Adds PUT_TIMEOUT = (10, 60), a (connect, read) tuple that keeps the connect side fail-fast while giving the read side more room. The upload-url GET a few lines above is left untouched -- the issue's own traceback shows the timeout originating from the blob store host, not comma's own API, which this GET hits instead. Fixes commaai#34941.
Contributor
Process replay diff reportReplays driving segments through this PR and compares the behavior to master. ✅ 0 changed, 66 passed, 0 errors |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #34941.
Root cause
do_upload()'s PUT to the blob store (system/loggerd/uploader.py) used a flattimeout=10. For largerqcamera.tsfiles over slow/cellular connections, the server receives and stores the file successfully, but takes longer than 10s to send back its ack —requests.put()raisesReadTimeouteven though the upload already succeeded (confirmed uploaded, visible in useradmin per the report). Since a file is only tagged as uploaded (viasetxattr) on success, the uploader retries the same already-uploaded file forever.Fix
Added
PUT_TIMEOUT = (10, 60), a(connect, read)timeout tuple — keeps the connect side fail-fast (an unreachable host should still fail quickly) while giving the read side more room for a slow-but-successful upload to ack. Left the upload-URL GET a few lines above untouched: the issue's own traceback shows the timeout originating fromcommadata2.blob.core.windows.net(the blob store), not comma's own API, which the GET hits instead — a categorically different, small metadata request with no comparable delay pattern.Testing
Added
test_upload_put_uses_widened_read_timeouttosystem/loggerd/tests/test_uploader.py: mocksrequests.put, temporarily disables the test suite's defaultfake_uploadshort-circuit, callsdo_upload()directly on a real generated test file, and asserts the mock was called withtimeout=PUT_TIMEOUT.I wasn't able to run the full test suite end-to-end in my environment — importing
uploader.pyneedsopenpilot.common.params, which loads a nativelibparams_c.dylibbuilt via the project's full SCons graph, and I didn't have a way to build that here (no Linux environment available to me). I did buildcereal.messaging's own Cython extension (msgq.ipc_pyx) from scratch and confirmed it imports cleanly, and both edited files passpython3 -m py_compile. As a substitute for the full test run, I wrote a standalone script (not part of this diff) that extracts the realPUT_TIMEOUTvalue directly from this file's source and exercises it against the actualrequestslibrary, using a real local HTTP server that receives a full PUT body and then deliberately delays its response past the old 10s timeout but within the new 60s one: the old value reproduces a realReadTimeoutat exactly 10.0s (matching the reported mechanism), the new value succeeds. Happy to adjust the test or timeout value if there's a preferred approach here.