Skip to content

fix(witan-core): size the connect-retry budget against a measured restart - #175

Merged
blarghmatey merged 2 commits into
mainfrom
worktree-unavailable-budget-fix
Aug 5, 2026
Merged

fix(witan-core): size the connect-retry budget against a measured restart#175
blarghmatey merged 2 commits into
mainfrom
worktree-unavailable-budget-fix

Conversation

@blarghmatey

Copy link
Copy Markdown
Member

What are the relevant tickets?

Follow-up to #174, from validating that change against the CI deployment (the post-merge validation named in ol-infrastructure#5215). Same witan task lineage: tk-omnigraph-server-actor-token-hot-reload-...-0e878a.

Description (What does it do?)

#174's retry budget was shorter than the outage it exists to absorb, so it did not absorb it. The validation caught it.

I measured two real restarts of the omnigraph-server Deployment in CI — one triggered by adding a token to the actor-tokens map, one by removing it — from the old container being killed to the new pod reporting Ready:

restart A   killed 21:20:21Z -> Ready 21:21:22Z    61s
restart B   killed 21:33:36Z -> Ready 21:34:28Z    52s

            merged budget (#174)                  ~42s   <-- gives up mid-restart

The framing is what made it wrong. As an attempt count, 12 attempts of capped exponential backoff sums to ~42s — a number you can only evaluate by doing the arithmetic and then comparing it against something external. The test did the arithmetic and compared it to >= 40, a threshold with no provenance. It passed while being wrong, which is the worst thing a test can do.

So the budget is now a wall-clock deadline (_UNAVAILABLE_MAX_WAIT = 150.0) rather than an attempt count. The value states the goal directly and can be checked against field data at a glance. The delay cap goes 5s → 10s so the longer deadline isn't paid for with dozens of subprocess spawns against a dead endpoint.

One behavioral detail: the deadline starts at the first connect failure, not at _execute entry, so a call that already spent time on unrelated drift retries still gets the full restart-length window.

How can this be tested?

cd packages/witan-core && uv sync --frozen --group test && uv run pytest — 197 passed, 1 skipped. uvx ruff@0.15.15 check . / format --check . clean at the version CI pins.

The replacement test, test_budget_outlasts_a_real_measured_restart, drives _execute against a fake clock and a stub server that comes back after the measured 61s, then asserts the call survived with headroom — rather than asserting the schedule sums past a made-up floor.

I verified the new test actually catches the shipped bug, which the old one did not:

$ sed -i 's/_UNAVAILABLE_MAX_WAIT = 150.0/_UNAVAILABLE_MAX_WAIT = 42.5/' witan_core/omnigraph.py
$ uv run pytest -k budget_outlasts
FAILED tests/test_omnigraph.py::test_budget_outlasts_a_real_measured_restart

The remaining tests are updated for the deadline model — the fake clock has to advance on sleep, since a no-op sleep would spin forever against a wall-clock budget.

Additional Context

The mechanism from ol-infrastructure#5215 itself validated cleanly end to end, in both directions — that part needed no change:

time Vault K8s Secret server accepts probe token
21:10:02 +act-restart-probe svc-witan-ci 401
21:11:09 +act-restart-probe svc-witan-ci 401 ← bug reproduced
21:20:21 VSO synced, restartedAt stamped
21:23:01 +act-restart-probe +act-restart-probe 422 (authenticated; 401 for an unknown token)
21:24:25 probe removed
21:35 svc-witan-ci svc-witan-ci 401 (revoked)

Propagation was 11.5 min, inside the 15m refreshAfter. The throwaway entry was added and removed by direct Vault write and is fully cleaned up; Vault, the K8s Secret, and Pulumi state all agree on svc-witan-ci only.

Worth a separate look, not changed here: 30s of each ~60s outage is the full terminationGracePeriodSeconds, burned exactly every time because omnigraph-server never exits on SIGTERM and gets SIGKILLed at the deadline. Lowering the grace period would roughly halve the outage, but it's a judgment call about in-flight writes that belongs to whoever owns the data tier rather than to this PR.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Gu1MY5CuMBu7YttU2RbFMk

…tart

Validating the merged change against the CI deployment showed the retry
budget was shorter than the outage it exists to absorb, so it did not
actually absorb it.

Two real restarts of the omnigraph-server Deployment were measured, one
triggered by adding a token to the actor-tokens map and one by removing
it, from the old container being killed to the new pod reporting Ready:
61s and 52s. The budget was ~42s.

The framing is what made it wrong. As an attempt count, 12 attempts of
capped exponential backoff sums to ~42s — a number you can only evaluate
by doing the arithmetic and then comparing it to something. Its test did
the arithmetic and compared it to 40, a threshold with no provenance,
so it passed while being wrong. Make the budget a wall-clock deadline
(_UNAVAILABLE_MAX_WAIT = 150s) so the value states the goal directly and
can be checked against field data at a glance, and raise the delay cap to
10s so the deadline is not paid for with dozens of subprocess spawns.

The deadline starts at the FIRST connect failure rather than at entry, so
a call that already spent time on unrelated drift retries still gets the
full restart-length window.

The replacement test drives _execute against a fake clock and a server
that comes back after the measured 61s, asserting the call survives with
headroom — not that the schedule sums past a made-up floor. Verified it
fails when the budget is set back to 42.5s, which the old test did not.

For anyone tuning this later: 30s of the ~60s is the full
terminationGracePeriodSeconds, burned exactly every time because the
server never exits on SIGTERM and is SIGKILLed at the deadline; the rest
is the binary opening its S3-backed graphs before readiness can pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Gu1MY5CuMBu7YttU2RbFMk
Copilot AI review requested due to automatic review settings August 3, 2026 21:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates witan-core’s omnigraph-server connect-failure retry logic to use a wall-clock deadline (measured from the first connect failure) sized to outlast observed CI Deployment restart outages, and refreshes the test suite and changelog to match the new model.

Changes:

  • Replace the prior attempt-count-based “server unavailable” retry budget with a wall-clock deadline (_UNAVAILABLE_MAX_WAIT = 150.0) and raise the delay cap to 10s to avoid excessive subprocess churn.
  • Update tests to drive the retry loop with a fake monotonic clock and add a regression test asserting the budget outlasts a measured 61s restart outage.
  • Update CHANGELOG.md to document the new deadline-based retry semantics and the rationale.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
packages/witan-core/witan_core/omnigraph.py Switch connect-failure retry from attempt-count to a wall-clock deadline; increase max backoff delay.
packages/witan-core/tests/test_omnigraph.py Introduce fake clock helpers and new regression tests aligned with the deadline model.
packages/witan-core/CHANGELOG.md Document the deadline-based retry budget and measured restart data motivating the change.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/witan-core/witan_core/omnigraph.py Outdated
…cond

Copilot caught this on review, and it is the same species of bug as the
one this PR exists to fix: the constant did not mean what it said.

The loop skipped the final sleep whenever the next backoff would
overshoot the deadline, and raised instead. With a 10s delay cap that
silently cut the effective window to 145.5s of the configured 150s, and
— worse than the 4.5s — it meant no attempt ever happened AT the
deadline. A server returning at T-1s, comfortably inside the window by
any reading of _UNAVAILABLE_MAX_WAIT, was missed.

Clamp the last sleep to the time remaining instead of skipping it, so
the budget is spent in full and the deadline itself gets one more
attempt. Tracks elapsed-since-first-failure rather than an absolute
deadline, which makes both the comparison and the error message read
directly.

The existing budget test had encoded the shortfall as acceptable
(`sum(sleeps) > MAX_WAIT - MAX_DELAY`) — a tolerance that existed only
to accommodate the bug. It now asserts the full budget is spent, and a
new test pins the behaviour that actually matters: a server coming back
1s before the deadline is caught. Both were verified to fail against the
previous overshoot-skips logic.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Gu1MY5CuMBu7YttU2RbFMk
@blarghmatey
blarghmatey merged commit 541b31f into main Aug 5, 2026
10 checks passed
@blarghmatey
blarghmatey deleted the worktree-unavailable-budget-fix branch August 5, 2026 12:29
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.

2 participants