Skip to content

fix(apps): re-resolve the artifact-manager proxy when its client id goes stale - #185

Draft
nilsmechtel wants to merge 2 commits into
mainfrom
fix/artifact-manager-stale-handle
Draft

nilsmechtel wants to merge 2 commits into
mainfrom
fix/artifact-manager-stale-handle

Conversation

@nilsmechtel

@nilsmechtel nilsmechtel commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

On 2026-09-14 the KTH worker stopped being able to do anything involving an artifact — deploy_app, list_apps and get_app_manifest all hung until they timed out — while every status call kept answering normally, so the worker looked healthy. model-runner was unreachable for about 62 minutes and the only thing that fixed it was restarting the pod.

Nothing had restarted. The Hypha artifact manager simply re-registered itself under a new client id, and the worker was still holding a handle addressed to the old one.

Fixes the framework half of svamp #74. Found, measured and root-caused at KTH by cold-ruff.

Why it could not recover on its own

AppsManager resolves public/artifact-manager once during initialization and keeps that proxy for the life of the process. A hypha_rpc proxy is bound to one client instance of the remote service, not to the service name — so when the artifact manager re-registered (public/marvelous-chameleon-04319715public/incredible-athlete-20303535, with hypha-server at restartCount 0 and two days old), the cached proxy was addressing a client that no longer existed.

hypha_rpc does reconnect the websocket underneath, and logs that it has, which is part of why this reads as healthy. What it does not do is re-resolve cached service proxies.

Every escape route was blocked by the same fault:

  • auto_redeploy needs the artifact read that is broken
  • run_code executes in Ray tasks, so it cannot reach the in-process handle
  • local calls (get_status, get_app_status) never touch the proxy, so monitoring stayed green throughout

This is a different object from the one PR #180 checks. That probe asks whether Hypha still serves the worker's own registration — inbound. This is a cached outbound proxy to somebody else's service, and the two are independent: the artifact-manager handle can be completely dead while the worker's own registration resolves perfectly. #180 would have stayed green through all 62 minutes.

The fix

The proxy is wrapped at the point of resolution rather than at each call site. AppBuilder and every artifact_utils helper are handed this same object, so one wrapper covers all of them and no caller has to remember the rule.

The retry rule is deliberately not "retry transport errors", and it has two axes. First, whether the call can have reached the server:

kind matched on
never_sent failed to send the request, websocket reconnection timed out provably never left the process
maybe_sent TimeoutError, client disconnected, method call timed out, service not found may already have executed

Second, whether repeating it is safe. A never_sent failure is always retried. A maybe_sent failure is retried only for reads (read, list, search, get_file, read_file, list_files); writes — create, commit, edit, delete, publish, discard, put_file, and the vector/PR operations — re-raise instead, because a timeout cancels nothing at the far end and replaying a commit would create a second version snapshot.

Retrying reads is what takes the observed fault to zero failed calls rather than one: the call that hung was services.artifact-manager.read, with the upload_app write before it already succeeded and deploy_app failing afterwards at manifest load.

Reads are repeatable but not side-effect-free, which needed checking rather than assuming. silent defaults to False and read increments a view count, so a naive retry double-counts a view. Verified against the live service (50 methods, 2026-09-15): read, list and get_file accept silent; read_file and list_files do not, so passing it to those would turn a recoverable timeout into a TypeError. So silent=True is injected only on a maybe_sent retry of one of the three that accept it. A never_sent retry is not silenced — nothing was counted the first time, so the retry is the first real view.

Anything else propagates untouched: an application-level error is not evidence that the handle is stale.

Concurrent callers all fail against the same dead proxy, so a generation counter means a single eviction produces one get_service call rather than a burst of them.

Residual cost, so this does not read as zero: the first call still pays its full timeout (~30 s) before the retry fires, and a write that times out still surfaces one failure rather than being replayed. That is the right trade against 62 minutes, but it is not transparent. Removing even the 30 s needs a periodic re-resolve probe, which this PR does not add; cold-ruff measured the numbers that make it attractive (a dead client id called from a fresh connection errors in 0.01 s, while the same id through a cached proxy hangs for the full timeout; a re-resolve costs 0.03 s), and that belongs in the health-check work tracked by #3.

Tests

New file tests/apps/test_artifact_manager_reconnect.py, 15 tests: the classifier in both directions, pass-through of healthy calls and of ordinary application errors, the send-side retry, the timeout path refreshing the handle without replaying the call, the next call succeeding, single re-resolve under concurrency, a failed re-resolve surfacing rather than being swallowed, the retried read being silenced while a send-side retry is not, a read that does not accept silent being retried unsilenced, and one test that the manager actually installs the wrapper — the fix is inert if that line regresses.

Full suite in the worker image, same scope both sides: origin/main 209 passed / 4 failed / 24 skipped, this branch 224 / 4 / 24. The 4 failures are pre-existing on main (tests/apps/cellpose/test_metadata_and_glob.py, _FakeArtifact.ls() keyword mismatch in app code) and untouched here.

Not validated live

Unit-tested only. Reproducing this on purpose means making the artifact manager re-register under a new client id, which is not something to induce on a shared Hypha server. The natural validation is the next time it happens by itself: the worker should log Re-resolved the artifact manager service proxy. and recover without a pod restart.

Same fault class as annotation-broker #3, fixed there in 0.9.3 (31e5681); this applies that pattern worker-side.

…oes stale

AppsManager resolves public/artifact-manager once and caches the proxy for the
process lifetime. A hypha_rpc proxy is pinned to one client instance of the
remote service, and that instance can change client id with nothing having
restarted — at KTH the artifact manager re-registered from
public/marvelous-chameleon-04319715 to public/incredible-athlete-20303535 with
hypha-server at restartCount 0 and two days old. hypha_rpc reconnects the
websocket underneath but does not re-resolve cached proxies, so every
artifact-backed call (deploy_app, list_apps, get_app_manifest) hung to timeout
while every local call (get_status, get_app_status) stayed green.

Nothing recovered it in-process: auto_redeploy needs the artifact read that is
broken, and run_code executes in Ray tasks so it cannot reach the handle.
model-runner was down ~62 minutes and only a pod restart cleared it.

The proxy is now wrapped at the point of resolution rather than at each call
site, so AppBuilder and every artifact_utils helper are covered without any
caller having to remember the rule.

The retry rule is deliberately not "retry transport errors". Failures are split
by whether the call can have reached the server:

  never_sent  "failed to send the request" / "websocket reconnection timed out"
              provably never left the process -> re-resolve and retry once,
              safe even for a write
  maybe_sent  timeout / "client disconnected" / "method call timed out"
              may already have executed -> re-resolve but re-raise, because
              replaying a create or commit would double-execute it

So a stale handle costs one visible failure instead of an indefinite outage.
Eliminating even that one failure needs a periodic re-resolve probe, which this
does not add — see the issue for the measurement that makes it cheap.

Refs: svamp #74, and the same fault class as annotation-broker #3, fixed
there in 0.9.3 (31e5681).
The first pass re-raised everything that might already have executed, which was
right for writes and wrong for the fault actually observed. The call that hung
in the #74 outage was services.artifact-manager.read; the upload_app write
before it had already succeeded and deploy_app failed afterwards at manifest
load. So the measured signature is a read timing out, and re-raising it left a
stale handle still costing one failed deploy_app.

Reads are safe to repeat however the first attempt failed, so they are now
retried on either failure kind. Writes are unchanged: create, commit, edit,
delete, publish, discard, put_file and the vector/PR operations still re-raise
on a maybe-sent failure rather than risk double-execution.

Reads are repeatable but not side-effect-free, which is the part that needed
checking rather than assuming. silent defaults to False and read increments a
view count, so a naive retry double-counts. Verified against the live service
(50 methods, 2026-09-15): read, list and get_file accept silent; read_file and
list_files do not, so passing it to those would turn a recoverable timeout into
a TypeError. silent=True is injected only on a maybe-sent retry of one of the
three that accept it — the first attempt still counts as a real view, and a
send-side retry is the first view rather than a repeat so it is not silenced.

Classification and the silent wrinkle were measured by cold-ruff on the live
artifact manager and re-verified here against the service schema.

Refs: svamp #74
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant