[codex] Use uploaded cache endpoint for Runner packages#482
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a helper method _package_url_from_ref in CacheStorage to dynamically resolve and normalize the package URL from the uploaded object's reference, specifically handling the translation of grpc+tls:// to grpcs://. It also adds corresponding unit tests. The feedback suggests a cleaner and safer way to replace the protocol prefix using str.replace instead of manual string slicing.
| if endpoint.startswith("grpc+tls://"): | ||
| endpoint = f"grpcs://{endpoint[len('grpc+tls://'):]}" |
There was a problem hiding this comment.
Using manual string slicing with a hardcoded length calculation like endpoint[len('grpc+tls://'):] is more error-prone and less readable than using str.replace with a limit of 1. If the prefix string is ever updated in the future, having to update both the startswith check and the slice length calculation can easily lead to bugs.
Using endpoint.replace("grpc+tls://", "grpcs://", 1) is cleaner and safer.
| if endpoint.startswith("grpc+tls://"): | |
| endpoint = f"grpcs://{endpoint[len('grpc+tls://'):]}" | |
| if endpoint.startswith("grpc+tls://"): | |
| endpoint = endpoint.replace("grpc+tls://", "grpcs://", 1) |
02eabc5 to
065524f
Compare
Summary
Root Cause
Runner uploaded packages through object-cache, but rebuilt the package URL from the configured cache service endpoint instead of the returned ObjectRef endpoint. With multiple object-cache pods behind one ClusterIP service, executor-manager could download from a different cache pod and fail to find the package.
Verification