fix: address release API review feedback

kvinwang · kvinwang · commit 445cd1ad7506 · 2026-07-20T21:18:31.000-07:00
diff --git a/dstack/crates/dstackup/src/cli.rs b/dstack/crates/dstackup/src/cli.rs
@@ -22,8 +22,8 @@ pub(crate) struct Cli {
     #[arg(long, global = true)]
     pub(crate) host: Option<String>,
 
-    /// Base URL of the GitHub-compatible releases API. The repository path is
-    /// appended automatically (useful for an API proxy or a test server).
+    /// Base URL of the GitHub-compatible releases API, including its `/repos`
+    /// prefix. The owner/repository path is appended automatically.
     #[arg(long, global = true, default_value = DEFAULT_RELEASE_API_BASE_URL)]
     pub(crate) release_api_base_url: String,
 
diff --git a/dstack/crates/dstackup/src/image.rs b/dstack/crates/dstackup/src/image.rs
@@ -628,7 +628,7 @@ async fn fetch_release(version: Option<&str>, release_api_base_url: &str) -> Res
             });
     }
 
-    let api_base = release_api_base_url.trim_end_matches('/');
+    let api_base = release_api_base_url.trim().trim_end_matches('/');
     let list_url = format!("{api_base}/{REPO}/releases?per_page=100");
     let releases: Vec<Release> = client
         .get(&list_url)
@@ -672,7 +672,7 @@ fn tagged_release_location(
         version.to_string(),
         format!(
             "{}/{repo}/releases/tags/{tag_prefix}{version}",
-            release_api_base_url.trim_end_matches('/')
+            release_api_base_url.trim().trim_end_matches('/')
         ),
         releases_url,
     ))
@@ -889,7 +889,8 @@ mod tests {
 
     #[test]
     fn release_api_base_url_is_configurable_and_trailing_slash_safe() {
-        let (_, url, _) = tagged_release_location("0.6.0", "http://127.0.0.1:1234/api/").unwrap();
+        let (_, url, _) =
+            tagged_release_location("0.6.0", "  http://127.0.0.1:1234/api/  ").unwrap();
         assert_eq!(
             url,
             "http://127.0.0.1:1234/api/Dstack-TEE/dstack/releases/tags/guest-os-v0.6.0"
diff --git a/tools/mock-github-releases.py b/tools/mock-github-releases.py
@@ -1,4 +1,8 @@
 #!/usr/bin/env python3
+# SPDX-FileCopyrightText: © 2026 Phala Network <dstack@phala.network>
+#
+# SPDX-License-Identifier: Apache-2.0
+
 """Local GitHub Releases API overlay for dstackup development.
 
 Locally published releases shadow GitHub. Requests not satisfied locally are
@@ -22,12 +26,16 @@
 
 
 class State:
+    """Thread-safe-by-snapshot in-memory release overlay."""
+
     def __init__(self, asset_dir: Path):
+        """Create an empty overlay and its local asset directory."""
         self.asset_dir = asset_dir.resolve()
         self.asset_dir.mkdir(parents=True, exist_ok=True)
         self.releases = {}  # repo -> tag -> release; insertion order is newest first
 
     def publish(self, repo, release, public_base):
+        """Publish a release as the newest local release for a repository."""
         tag = release.get("tag_name")
         if not isinstance(tag, str) or not tag:
             raise ValueError("tag_name must be a non-empty string")
@@ -49,19 +57,28 @@ def publish(self, repo, release, public_base):
                     digest=f"sha256:{digest}",
                 )
             if not asset.get("name") or not asset.get("browser_download_url"):
-                raise ValueError("each asset needs name and browser_download_url, or local_path")
+                raise ValueError(
+                    "each asset needs name and browser_download_url, or local_path"
+                )
             assets.append(asset)
         release = dict(release, assets=assets)
-        releases = self.releases.setdefault(repo, {})
-        releases.pop(tag, None)
-        self.releases[repo] = {tag: release, **releases}
+        # Build a new snapshot rather than mutating one observed by a concurrent
+        # GET handler. Assignment is atomic under CPython's GIL.
+        releases = self.releases.get(repo, {})
+        self.releases[repo] = {
+            tag: release,
+            **{old_tag: old for old_tag, old in releases.items() if old_tag != tag},
+        }
         return release
 
 
 class Handler(BaseHTTPRequestHandler):
+    """Serve the local release overlay and relay misses to GitHub."""
+
     server_version = "dstack-release-mock/1"
 
     def json_response(self, status, value):
+        """Send a JSON response with a fixed content length."""
         body = json.dumps(value, indent=2).encode()
         self.send_response(status)
         self.send_header("content-type", "application/json")
@@ -70,9 +87,14 @@ def json_response(self, status, value):
         self.wfile.write(body)
 
     def do_POST(self):
+        """Publish a release through the local administration endpoint."""
         # POST /__admin/repos/{owner}/{repo}/releases
         parts = self.path.strip("/").split("/")
-        if len(parts) != 5 or parts[:2] != ["__admin", "repos"] or parts[4:] != ["releases"]:
+        if (
+            len(parts) != 5
+            or parts[:2] != ["__admin", "repos"]
+            or parts[4:] != ["releases"]
+        ):
             self.send_error(404)
             return
         try:
@@ -87,6 +109,7 @@ def do_POST(self):
             self.json_response(400, {"error": str(error)})
 
     def do_GET(self):
+        """Serve an asset or a GitHub-compatible releases API request."""
         path = urlsplit(self.path).path
         if path.startswith("/__assets/"):
             self.serve_asset(unquote(path.removeprefix("/__assets/")))
@@ -102,7 +125,9 @@ def do_GET(self):
                 upstream = self.fetch_upstream_json()
                 if isinstance(upstream, list):
                     local_tags = set(local)
-                    upstream = [r for r in upstream if r.get("tag_name") not in local_tags]
+                    upstream = [
+                        r for r in upstream if r.get("tag_name") not in local_tags
+                    ]
                 else:
                     upstream = []
                 self.json_response(200, list(local.values()) + upstream)
@@ -118,6 +143,7 @@ def do_GET(self):
         self.relay()
 
     def serve_asset(self, name):
+        """Serve one basename-only file from the configured asset directory."""
         if not name or Path(name).name != name:
             self.send_error(400, "invalid asset name")
             return
@@ -126,65 +152,92 @@ def serve_asset(self, name):
             self.send_error(404)
             return
         self.send_response(200)
-        self.send_header("content-type", mimetypes.guess_type(name)[0] or "application/octet-stream")
+        self.send_header(
+            "content-type", mimetypes.guess_type(name)[0] or "application/octet-stream"
+        )
         self.send_header("content-length", str(path.stat().st_size))
         self.end_headers()
         with path.open("rb") as source:
             shutil.copyfileobj(source, self.wfile)
 
     def upstream_request(self):
+        """Build the corresponding authenticated GitHub API request."""
         url = UPSTREAM + self.path
-        headers = {"user-agent": "dstack-release-mock", "accept": "application/vnd.github+json"}
+        headers = {
+            "user-agent": "dstack-release-mock",
+            "accept": "application/vnd.github+json",
+        }
         auth = self.headers.get("authorization")
         if auth:
             headers["authorization"] = auth
         return urllib.request.Request(url, headers=headers)
 
     def fetch_upstream_json(self):
+        """Fetch upstream JSON for merging, returning an empty list on failure."""
         try:
-            with urllib.request.urlopen(self.upstream_request(), timeout=30) as response:
+            with urllib.request.urlopen(
+                self.upstream_request(), timeout=30
+            ) as response:
                 return json.load(response)
         except (urllib.error.URLError, json.JSONDecodeError):
             return []
 
     def relay(self):
+        """Relay the current request and its HTTP status to GitHub."""
         try:
-            with urllib.request.urlopen(self.upstream_request(), timeout=30) as response:
+            with urllib.request.urlopen(
+                self.upstream_request(), timeout=30
+            ) as response:
                 body = response.read()
                 self.send_response(response.status)
-                self.send_header("content-type", response.headers.get("content-type", "application/json"))
+                self.send_header(
+                    "content-type",
+                    response.headers.get("content-type", "application/json"),
+                )
                 self.send_header("content-length", str(len(body)))
                 self.end_headers()
                 self.wfile.write(body)
         except urllib.error.HTTPError as error:
             body = error.read()
             self.send_response(error.code)
-            self.send_header("content-type", error.headers.get("content-type", "application/json"))
+            self.send_header(
+                "content-type", error.headers.get("content-type", "application/json")
+            )
             self.send_header("content-length", str(len(body)))
             self.end_headers()
             self.wfile.write(body)
         except urllib.error.URLError as error:
             self.json_response(502, {"error": f"upstream request failed: {error}"})
 
     def log_message(self, fmt, *args):
+        """Write HTTP access logs to stderr."""
         sys.stderr.write("%s - %s\n" % (self.address_string(), fmt % args))
 
 
 class Server(ThreadingHTTPServer):
+    """Threading HTTP server carrying shared overlay state."""
+
     def __init__(self, address, state):
+        """Bind the server and attach its overlay state."""
         super().__init__(address, Handler)
         self.state = state
 
 
 def main():
+    """Run the local releases API overlay until interrupted."""
     parser = argparse.ArgumentParser(description=__doc__)
     parser.add_argument("--listen", default="127.0.0.1")
     parser.add_argument("--port", type=int, default=8000)
-    parser.add_argument("--asset-dir", type=Path, default=Path("/tmp/dstack-release-mock-assets"))
+    parser.add_argument(
+        "--asset-dir", type=Path, default=Path("/tmp/dstack-release-mock-assets")
+    )
     args = parser.parse_args()
     server = Server((args.listen, args.port), State(args.asset_dir))
     print(f"release API: http://{args.listen}:{args.port}/repos", flush=True)
-    print(f"publish API: http://{args.listen}:{args.port}/__admin/repos/OWNER/REPO/releases", flush=True)
+    print(
+        f"publish API: http://{args.listen}:{args.port}/__admin/repos/OWNER/REPO/releases",
+        flush=True,
+    )
     try:
         server.serve_forever()
     except KeyboardInterrupt:
