Skip to content

fix(gateway): expire same-epoch orphaned drain markers past a max-age (#85433) - #85447

Closed
PRATHAMESH75 wants to merge 1 commit into
NousResearch:mainfrom
PRATHAMESH75:fix/drain-marker-requested-at-ttl
Closed

fix(gateway): expire same-epoch orphaned drain markers past a max-age (#85433)#85447
PRATHAMESH75 wants to merge 1 commit into
NousResearch:mainfrom
PRATHAMESH75:fix/drain-marker-requested-at-ttl

Conversation

@PRATHAMESH75

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #85433: a .drain_request.json marker orphaned without a machine restart wedges the gateway in draining forever — every inbound message on every platform is bounced with "⏳ This agent is draining for a maintenance action…" until someone removes the marker by hand.

Root cause

The NS-570 fix (PR #53050) stamps the marker with an instantiation epoch, so a marker that survives a machine restart on the durable HERMES_HOME volume is recognised as stale. But that defence rests on the assumption that every drain-gated lifecycle action restarts the machine. When a maintenance action finishes without recreating the container and its writer never clears the marker (e.g. a crash between "action done" and "cancel drain"), the epoch still matches, _marker_epoch_is_stale() correctly returns False, and the 1s watcher honours the orphan indefinitely. In the reported incident a Hermes Cloud instance bounced every Telegram message for ~3 days.

The marker already carries requested_at, but no reader ever consulted it — there was no TTL/max-age fallback, so a same-epoch orphan had unbounded lifetime.

The fix

Add a lenient max-age fallback (_DRAIN_MARKER_MAX_AGE_SECONDS = 60 min) alongside the epoch check, consumed by both drain_requested() and drain_notification_suppressed() via a shared _marker_is_inactive():

  • a marker whose requested_at is present, parseable, and older than the bound reads as stale, with a once-per-marker warning log (memoised so the 1s watcher doesn't spam);
  • a marker with no requested_at, a non-string one, or an unparseable one stays honoured — the same fail-safe-toward-quiescing leniency the epoch check already uses for legacy/corrupt markers;
  • a naive (tz-less) timestamp is assumed UTC, matching the writer;
  • a deliberately long drain keeps a sanctioned keep-alive: write_drain_request() is documented idempotent, and re-writing refreshes requested_at.

60 min is deliberately generous — drain-gated actions (auto-update / image migrate / env edit / profile change) complete in minutes. This mirrors the requested_at staleness guard the neighbouring .restart_notify.json marker already applies in gateway/run.py, precisely so "a legitimately old marker should not swallow a fresh action."

The writer-side clean-up (the cloud control plane clearing the marker in a finally) is out of scope for this repo, as the issue notes — the gateway should defend itself regardless, since any writer crash reproduces this.

Tests

tests/gateway/test_external_drain_control.py::TestRequestedAtMaxAge — the same-epoch aged-out orphan reading as absent (the regression), a just-within-bound marker staying active, missing/unparseable requested_at staying active (leniency), naive-timestamp handling, and an aged orphan no longer suppressing the shutdown broadcast. Full test_external_drain_control.py and the neighbouring drain suites pass locally; ruff + windows-footgun gates clean.

Fixes #85433

A .drain_request.json marker orphaned WITHOUT a machine restart wedges the
gateway in draining indefinitely: the NS-570 epoch check (PR NousResearch#53050) only
recognises a marker that outlived a restart, but a maintenance action that
finishes without recreating the container — and whose writer never clears the
marker (e.g. a crash between 'action done' and 'cancel drain') — leaves the
epoch matching, so the 1s watcher honours the orphan forever and bounces every
inbound message with 'draining for a maintenance action' (NousResearch#85433).

The marker already carries requested_at but no reader consulted it. Add a
lenient max-age fallback (60 min) alongside the epoch check: a same-epoch
marker whose requested_at is present, parseable, and older than the bound reads
as stale in drain_requested()/drain_notification_suppressed(), with a
once-per-marker warning. A marker with no/unparseable requested_at stays
honoured (same fail-safe-toward-quiescing leniency as the epoch check), and a
deliberately long drain keeps itself alive by re-writing the marker (idempotent
write refreshes the stamp). Mirrors the requested_at staleness guard the
neighbouring .restart_notify.json marker already applies in gateway/run.py.

Fixes NousResearch#85433
@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/gateway Gateway runner, session dispatch, delivery sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages duplicate This issue or pull request already exists labels Aug 13, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #85444: both implement the same one-hour requested_at expiry for same-epoch orphan drain markers in gateway/drain_control.py. #85444 was opened first.

@PRATHAMESH75

Copy link
Copy Markdown
Contributor Author

Closing this as superseded. Upstream landed a maintainer fix for the same issue (#85433) in 885576671"fix(gateway): expire orphaned drain markers past a max-age so a leaked marker can't wedge the gateway (#85433)" — which is functionally identical to this PR: it adds a requested_at max-age fallback (DRAIN_REQUEST_MAX_AGE_SECONDS = 3600s) consulted by both drain_requested() and drain_notification_suppressed(), with the same fail-safe leniency for missing/unparseable timestamps and the keep-alive refresh contract.

Redundancy verified: the merged TestMarkerMaxAge suite (7 tests covering the same-epoch aged-out orphan, the just-within-bound marker, and missing/unparseable leniency) passes on plain upstream/main with none of this PR's code. Nothing here remains to land, so closing in favour of the merged fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery duplicate This issue or pull request already exists P1 High — major feature broken, no workaround sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Gateway: same-epoch orphaned .drain_request.json has no TTL — agent bounces every message indefinitely ("draining for a maintenance action")

2 participants