fix(studio): recover stalled Hub downloads over HTTP#6858
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an automatic fallback mechanism that retries failed XET downloads over HTTP when available, along with comprehensive unit tests and registry updates to support reclaiming active slots. The review feedback highlights a critical bug in _try_http_retry where early returns on missing metadata or non-XET transport leave the job stuck in a "running" state, blocking future operations. Additionally, the feedback suggests resolving encapsulation violations by replacing direct access to DownloadRegistry's private attributes with a new public update_job_transport method.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
I am having trouble creating individual review comments. Click here to see my feedback.
studio/backend/hub/services/download_lifecycle.py (339-350)
If _try_http_retry returns early because original_metadata is None or the original transport is not XET, the job will remain stuck in the "running" state forever. This is because finalize_worker_exit was called with defer_error = True (which deferred transitioning the job to "error"), and returning False here skips spawning the HTTP worker without transitioning the job state. This will block any future download or delete operations on this repository.
We should transition the job to "error" (or "cancelled" if a cancel was requested) using _set_retry_failure_state before returning False.
original_metadata = registry.get_job_metadata(key)
if original_metadata is None:
logger.debug("%s XET retry skipped for %s; metadata unavailable", log_prefix, label)
_set_retry_failure_state(
registry,
key,
"XET retry skipped: metadata unavailable",
repo_type = repo_type,
repo_id = repo_id,
fallback_variant = download_registry.variant_from_key(key),
fallback_transport = download_registry.TRANSPORT_XET,
logger = logger,
)
return False
if original_metadata.transport != download_registry.TRANSPORT_XET:
logger.debug(
"%s XET retry skipped for %s; original transport was %s",
log_prefix,
label,
original_metadata.transport,
)
_set_retry_failure_state(
registry,
key,
f"XET retry skipped: original transport was {original_metadata.transport}",
repo_type = repo_type,
repo_id = repo_id,
fallback_variant = variant,
fallback_transport = original_metadata.transport,
logger = logger,
)
return FalseReferences
- When designing API schemas or internal helpers that support advanced transport modes (e.g., Xet), it is safe to default to the advanced mode if a fallback mechanism is implemented to automatically downgrade to a standard protocol (e.g., HTTP) when the required dependency is missing, avoiding hard failures (like 400 Bad Request) for clients.
studio/backend/hub/services/download_lifecycle.py (279-288)
Directly accessing private attributes of DownloadRegistry (such as _lock and _metadata) from another module violates encapsulation. It is cleaner to delegate this responsibility to a public method on DownloadRegistry itself.
def _set_job_transport(
registry: download_registry.DownloadRegistry,
key: str,
transport: str,
) -> None:
registry.update_job_transport(key, transport)studio/backend/hub/utils/download_registry.py (851-882)
To support updating the job transport without violating encapsulation from other modules, we can add a public update_job_transport method to DownloadRegistry.
def set_error_unless_cancelled(
self,
key: str,
error: str,
) -> tuple[JobState, Optional[DownloadMetadata]]:
key = normalize_job_key(key)
with self._lock:
current = self._jobs.get(key, DownloadState("idle")).state
has_pending_cancel = key in self._pending_cancel
pending_generation = self._pending_cancel.get(key)
metadata = self._metadata.get(key)
should_cancel = (
current == "cancelling"
or (
has_pending_cancel
and self._generation_matches_locked(key, pending_generation)
)
)
terminal_state: JobState = "cancelled" if should_cancel else "error"
self._put_terminal_job_locked(
key,
terminal_state,
None if should_cancel else error,
)
self._pending_cancel.pop(key, None)
repo = _repo_of_key(key)
active = self._repo_active.get(repo)
if active is not None:
active.discard(key)
if not active:
self._repo_active.pop(repo, None)
return terminal_state, metadata
def update_job_transport(self, key: str, transport: str) -> None:
from dataclasses import replace
key = normalize_job_key(key)
with self._lock:
metadata = self._metadata.get(key)
if metadata is None or metadata.transport == transport:
return
self._metadata[key] = replace(metadata, transport = transport)
09df149 to
a8581e0
Compare
|
Thanks, I addressed both points.
I also added focused lifecycle tests for both skipped retry cases. |
![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: 38c25d6732
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 026b6a872a
ℹ️ 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".
026b6a8 to
0f9ba79
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f9ba79031
ℹ️ 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".
0f9ba79 to
4e343ea
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e343eafcf
ℹ️ 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".
4e343ea to
ce00309
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b47dfacb0b
ℹ️ 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".
b47dfac to
18bc4ed
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18bc4ed437
ℹ️ 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".
| else 0 | ||
| ) | ||
| generation = registry.current_generation(key) | ||
| registry.release_active_slot(key) |
There was a problem hiding this comment.
Fresh evidence beyond the prior same-variant delete fix is that _delete_cached_model_blocking() decides whether to preserve shared GGUF companion blobs through has_active_peer_variant(), which only scans _repo_active. After this line removes the failed variant from _repo_active while it remains running, deleting a different GGUF variant in the same repo can see no active peer, delete shared companion .incomplete blobs with companions=True, and then the waiting HTTP retry can reclaim the slot and write the same companion concurrently or have its partial removed. Keep the retrying job in whatever structure sibling-delete protection uses, or make has_active_peer_variant() include these running retry-wait jobs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The has_active_peer_variant / _repo_active asymmetry is real but inert here: the at-risk companion is an XET-written partial that a resume purges anyway, finalized blobs are protected by the deletion ref-count walk, and the release-to-reclaim window is microseconds. No data loss or misled resume, so leaving it as is; can scan _jobs there later for defense in depth if wanted.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18bc4ed437
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| else 0 | ||
| ) | ||
| generation = registry.current_generation(key) | ||
| registry.release_active_slot(key) |
There was a problem hiding this comment.
When an XET variant fails while a sibling is still active, this removes the failed key from _repo_active but leaves its job state as running until the HTTP retry can reclaim the slot. During that wait, active_download_refs() only enumerates _repo_active, so /api/hub/active-downloads drops the retry even though it still blocks deletion and will later spawn work; after a page reload the frontend cannot adopt or cancel that backend-running retry. Keep a visible active ref for the waiting retry, or teach the active-download listing to include these active _jobs that were temporarily released from the repo guard.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4ccd05c: active_job_refs now also includes active _jobs that were briefly released from _repo_active during an XET to HTTP retry handoff, so the waiting retry stays listed in /api/hub/active-downloads and can be adopted or cancelled after a reload. Mirrors the existing delete-guard scan; the normal case is unchanged.
…d as active downloads for PR unslothai#6858
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9a013a3c0
ℹ️ 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".
| label, | ||
| conflict_state, | ||
| ) | ||
| time.sleep(0.05) |
There was a problem hiding this comment.
When an XET retry is parked in this wait loop behind an active sibling, it has already dropped its process and active-slot guard, so DownloadRegistry.terminate_all() does not include it in the shutdown snapshot; after shutdown kills the sibling and its watcher clears _repo_active, this loop can reclaim the slot and spawn a fresh HTTP worker after the shutdown cleanup has already run. In that shutdown/restart window the new worker is not killed by terminate_all() and the released retry may also lose the cancel-marker/orphan breadcrumb path if the process exits before it registers, so the retry loop needs to observe shutdown or terminate_all() needs to settle these no-process active jobs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ca1790b: terminate_all now settles no-process active jobs (arms a pending cancel and marks them cancelling), so an XET to HTTP retry parked in the reclaim wait loop bails at its cancel_requested check instead of spawning a fresh worker after shutdown cleanup, and any retry racing past that check has its new worker killed on register.
…cannot spawn after cleanup for PR unslothai#6858
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca1790b9ea
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self._pending_cancel[key] = self._generations.get(key) | ||
| self._jobs[key] = DownloadState("cancelling") |
There was a problem hiding this comment.
Fresh evidence beyond the earlier shutdown fix is that run_lifespan_shutdown only awaits terminate_all(), and this branch merely records the parked retry's cancel intent in memory. When an XET→HTTP retry has no registered process/breadcrumb (for example, both sibling XET workers have already failed and are sleeping in the reclaim loop), shutdown can return before the daemon watcher wakes and calls _set_retry_failure_state, so the backend exits without writing the XET cancel marker and the next launch loses the resumable/cancelled state for the partials. Please synchronously persist/settle these no-process jobs here, as the live-process shutdown path does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b09d6c5: terminate_all now persists the cancel marker synchronously for each settled no-live-worker job (collected under the lock, written after it, mirroring the reaped path), so the resumable XET cancel state is recorded even if shutdown returns before the watcher wakes.
| # check has its freshly spawned worker killed on register, instead of | ||
| # surviving this shutdown snapshot. | ||
| for key, job in list(self._jobs.items()): | ||
| if job.state not in _ACTIVE_STATES or key in self._processes: |
There was a problem hiding this comment.
Fresh evidence beyond the earlier shutdown fix is that terminate_all() builds its live snapshot only from processes where proc.poll() is None, while this new settling loop skips any key still present in _processes. If an XET worker has already exited with an error but its watcher has not yet called drop_process, shutdown leaves the job running; after run_lifespan_shutdown() returns, the watcher can process that error and enter _try_http_retry(), spawning an HTTP worker that was not in the shutdown snapshot. Treat already-exited registered processes as settled/cancelling before returning, not just process-less jobs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b09d6c5: terminate_all now skips only keys with a live process (live_keys), so a registered worker that already exited with an error but has not been dropped is settled (cancelling + pending cancel) instead of left running to spawn a retry after shutdown. A cleanly exited worker (rc 0) is left alone to avoid a stale marker.
…their cancel markers for PR unslothai#6858
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b09d6c5872
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if proc is not None and proc.poll() == 0: | ||
| continue | ||
| self._pending_cancel[key] = self._generations.get(key) | ||
| self._jobs[key] = DownloadState("cancelling") | ||
| settled_no_proc.append(self._metadata.get(key)) |
There was a problem hiding this comment.
When a plain HTTP worker exits nonzero just before shutdown, it is absent from live but still present in _processes; this new block treats every nonzero poll() result as a shutdown cancel and adds it to settled_no_proc, which immediately writes a cancel marker below. After restart, idle_status will see that marker and report the HTTP failure as cancelled/resumable rather than leaving it as an error/no-marker case. Please restrict this path to the parked XET retry handoff (or at least skip metadata.transport == "http").
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the latest commit: the exited-registered-worker settling now skips metadata.transport == http, so a plain HTTP worker that failed on its own keeps its error status instead of a cancel marker. Only exited XET workers (the ones that could spawn a post-shutdown HTTP retry) are settled; the parked no-process XET retry is still settled and persisted as before.
| other_variant = self._active_job_variant_locked(key) | ||
| if other_variant is None or other_variant == variant: | ||
| return True | ||
| for key, job in self._jobs.items(): |
There was a problem hiding this comment.
This new released-job scan makes begin_delete() aware of a retry that has left _repo_active, but the delete flow later uses has_active_peer_variant() to decide whether shared GGUF companions can be removed, and that helper still only scans _repo_active. If a Q4 retry is between release_active_slot() and its HTTP reclaim while the user deletes Q5, begin_delete(Q5) is allowed, has_active_peer_variant(Q5) returns false, and the delete can pass companions=True and remove the shared mmproj/companion while the Q4 retry is still active or about to spawn. Please apply the same released-job visibility to the peer check before companion deletion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the latest commit: has_active_peer_variant now also scans released-but-active jobs (mirroring _delete_blocked_by_active_locked), so a Q4 retry between release_active_slot and its reclaim blocks companion deletion when deleting a different variant like Q5.
…for released retry peers for PR unslothai#6858
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
3 participants
Problem
Studio Hub downloads can get stuck in
errorafter a non-cancelled XET worker failure even when the same repo or GGUF variant can continue over the normal HTTP transport. A plain HTTP failure should still stay terminal, and user cancellation should still win over any recovery attempt.Fix
Add a narrow XET-to-HTTP recovery path in the shared download lifecycle. When a failed XET worker is eligible for recovery, the watcher reclaims the same registry key over HTTP, preserves the registry generation used by cancel requests, recomputes completed blob bytes from the current cache state, and registers the replacement worker through the existing lifecycle path.
The retry handoff stays nonterminal while the replacement claim is prepared. If another same-repo XET variant is still active, the failed key releases only its active-slot guard and waits instead of publishing
error; this prevents deadlock when sibling XET variants fail around the same time. Delete admission still checks running job state, so the waiting variant cannot be deleted while it is temporarily outside the active-slot set. If the repo is deleting, retry spawn fails, or cancellation wins, the registry resolves the final state atomically.The retry claim uses HTTP metadata for conflict checks once the retry owns the active slot, so a new XET sibling cannot run concurrently with the HTTP retry and corrupt shared companion blobs. Cancel-marker provenance is tracked separately in active metadata and worker breadcrumbs, so cancellation before HTTP spawn, before HTTP registration, after registration, during shutdown, or during restart reaping still writes an XET cancel marker.
Plain HTTP failures remain outside this fix.
Testing
set PYTHONPATH=.\\studio\\backend && set UNSLOTH_COMPILE_DISABLE=1 && python -m pytest studio/backend/hub/tests/test_download_lifecycle.py -q --tb=short137 passedResult: 17 passed.
Closes #5979