Skip to content

fix(a2a): make reply deadline config-native - #91686

Open
RuniThomsen wants to merge 1 commit into
NousResearch:mainfrom
runi-services:fix/a2a-reply-timeout-900
Open

RuniThomsen wants to merge 1 commit into
NousResearch:mainfrom
runi-services:fix/a2a-reply-timeout-900

Conversation

@RuniThomsen

Copy link
Copy Markdown

Problem

Hermes A2A has a five-minute inbound reply deadline controlled only through an environment variable. Behavioral timeout configuration belongs in config.yaml, and raising only the reply deadline can leave the fixed orphan watchdog free to fail a still-running routed profile first. That produces disagreement between the direct response and persisted task state.

Change

  • add gateway.platforms.a2a.extra.reply_timeout as the preferred profile-safe configuration path while preserving A2A_REPLY_TIMEOUT as a legacy fallback
  • reject invalid and non-finite timeout values with the safe 300-second fallback
  • use the configured deadline for synchronous replies and task subscriptions
  • derive each orphan deadline from both the inbound reply timeout and the routed agent timeout, plus 60 seconds of grace
  • let TaskStore.fail_orphans accept a per-task timeout resolver without permitting it to shorten the existing floor
  • document the config key on both A2A reference surfaces

The default remains 300 seconds, avoiding a global increase in long-lived synchronous request workers. Deployments that need long turns can opt into 900 seconds per profile.

This overlaps the watchdog symptom in #90158 but additionally supplies the config-native deadline, applies it to reply/subscription waits, rejects non-finite values, and keeps ordinary and routed task deadlines consistent.

Verification

  • four regression slices were observed red before their implementations
  • scripts/run_tests.sh tests/plugins/test_a2a_plugin.py — 111 passed
  • git diff --check — clean

Risk and rollback

Only explicitly configured profiles get a longer request lifetime. Removing reply_timeout restores the 300-second default; reverting the commit restores the environment-only behavior.

@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins area/config Config system, migrations, profiles P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 21, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

The parsing layer is exactly right (inf/nan/negative/garbage all collapse to the safe fallback, and an invalid explicit config still defers to A2A_REPLY_TIMEOUT before the 300s default — a nice precedence nuance), the orphan watchdog now provably outlives both the inbound reply window and any routed agent's own timeout, and the test set covers each of those behaviors at the right layer (adapter, wait path, store).

  1. plugins/platforms/a2a/protocol.py:~751–772 (fail_orphans) — this signature is being extended simultaneously by two open PRs: this one adds timeout_for=, while feat(a2a): return a task id for long-running peer work #91688 adds skip= (live waiter ids) — why it matters: whichever lands second conflicts textually and semantically, and the correct combined behavior is neither alone (skip live tasks AND use per-task limits) — suggestion: coordinate the two merges so the final signature takes both parameters and the watchdog passes both, ideally with one shared test covering skip ∩ per-task-timeout together.

  2. plugins/platforms/a2a/protocol.py:~760–764 — timeout_for(rec) executes adapter code while holding self._lock; today the callback only reads self._agents, but the pattern invites future callbacks that touch the same store — why it matters: a lock-ordering inversion here deadlocks the watchdog thread against task writes — suggestion: snapshot records under lock, compute limits outside, re-check before failing; or at minimum document that timeout_for must never call back into the store.

  3. plugins/platforms/a2a/adapter.py:~495–500 — the watchdog warning lost the effective timeout it used to include (the "timeout %ds" fragment), which was genuinely useful when diagnosing "why did my long task get orphaned" — why it matters: now that per-task limits vary, a single global number would be misleading but no number hides the answer entirely — suggestion: log the tid plus the result of self._orphan_timeout_for(rec).

Nit: the README table says "prefer gateway.platforms.a2a.extra.reply_timeout" but doesn't state that an invalid explicit value falls back to the env var (not straight to 300) — worth half a sentence given how surprising that ordering could be.

— reviewer-a · automated agent review (Hermes week-review)

This branch has not been deployed

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

Labels

area/config Config system, migrations, profiles comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants