Skip to content

fix(apps): treat absence from the Serve status report as unknown - #177

Draft
nilsmechtel wants to merge 1 commit into
mainfrom
fix/monitor-status-gap
Draft

fix(apps): treat absence from the Serve status report as unknown#177
nilsmechtel wants to merge 1 commit into
mainfrom
fix/monitor-status-gap

Conversation

@nilsmechtel

Copy link
Copy Markdown
Collaborator

A momentary gap in Ray's status report restarts a healthy app. monitor_applications computed unhealthy = application is None or state in ("DEPLOY_FAILED", "UNHEALTHY"), which reads "Ray did not mention this app" as if it were "Ray says this app is broken". Those are different claims — the first can simply mean the status report was incomplete for a tick — and acting on it tears down a running app and destroys whatever it was in the middle of. There was nothing to absorb it either: the backoff only starts after the first fire, so the very first such observation redeployed immediately.

What changed

Absence is tolerated for _MISSING_FROM_STATUS_TOLERANCE (3) consecutive ticks — roughly 30 s at the default monitor interval — logged at INFO each time, with the counter cleared the moment the app reappears in any state. The tolerance is finite on purpose: never redeploying a genuinely dead app is its own outage, so unknown has to converge rather than become a permanent veto.

The stale-replica branch added in #50 deletes an app expecting the next tick to see it missing and redeploy fresh replicas. That gap is known rather than unknown, so it is marked and skips the tolerance — otherwise this PR would have tripled the recovery latency of a path that is already degraded.

_fire_redeploy now takes a required reason and logs it. An "absent from the report" restart and a genuine UNHEALTHY restart previously produced byte-identical warning lines, which is why nobody can say how often the first already happens in production. That was the second half of the issue and it is what makes the next incident readable.

Tests

tests/apps/test_monitor_missing_from_status.py, 10 tests. Both behavioural halves were positive-controlled rather than asserted:

  • Setting the tolerance to 1 (the old fire-on-first-absence behaviour) fails 2 tests.
  • Removing the reason from the log line fails 4, one of which shows the two warning lines coming out byte-identical — the production symptom verbatim.
  • Dropping the deliberate-delete marker fails the exemption test.

One of those 10 asserts 1 < _MISSING_FROM_STATUS_TOLERANCE < 100 directly, because every other test counts its ticks off that constant: at 1 the loops run zero iterations and the suite would pass vacuously while the bug was back.

Full run on this branch: 187 passed (tests/_app tests/apps tests/worker, --noconftest; the 32 errors are the pre-existing model-runner/cellpose fixture errors that flag needs).

Not yet done

No version bump — per CLAUDE.md that lands as a separate commit just before this leaves draft. This touches bioengine/**, so it also wants a dev-image run on a live cluster before it is marked ready; it can share one image with #175 and #176.

Closes svamp issue #12.

monitor_applications computed

    unhealthy = application is None or state in ("DEPLOY_FAILED", "UNHEALTHY")

which reads "Ray did not mention this app" as "Ray says this app is
broken". Those are different claims: the first can simply mean the
status report was momentarily incomplete, and acting on it tears down a
healthy app and destroys whatever it was in the middle of. The first
such observation fired a redeploy immediately -- the backoff only starts
after the first fire, so there was nothing to absorb it.

Absence is now tolerated for _MISSING_FROM_STATUS_TOLERANCE (3)
consecutive ticks, logged at INFO, and the counter is cleared the moment
the app reappears in any state. The tolerance is deliberately finite:
never redeploying a genuinely dead app would be its own outage, so
unknown must still converge.

The stale-replica branch deletes an app expecting the next tick to
redeploy it. That gap is known rather than unknown, so it is marked and
skips the tolerance instead of tripling that path's recovery latency.

_fire_redeploy now takes a required reason and logs it. An "absent from
the report" restart and a genuine UNHEALTHY restart previously produced
byte-identical log lines, which is why nobody could say how often the
first already happened.

Refs: svamp issue #12
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