Skip to content

fix(kanban): release respawn guard for closed PRs - #84017

Open
jared-voss wants to merge 1 commit into
NousResearch:mainfrom
jared-voss:fix/active-pr-respawn-guard
Open

fix(kanban): release respawn guard for closed PRs#84017
jared-voss wants to merge 1 commit into
NousResearch:mainfrom
jared-voss:fix/active-pr-respawn-guard

Conversation

@jared-voss

Copy link
Copy Markdown

Summary

  • resolve live GitHub PR state before applying the Kanban active_pr respawn guard
  • fail closed when GitHub state cannot be resolved
  • cache successful and failed lookups for 10 minutes with a bounded process-local cache

Test plan

  • scripts/run_tests.sh tests/hermes_cli/test_kanban_db.py (34 passed, 1 skipped)
  • .venv/bin/ruff check hermes_cli/kanban_db.py tests/hermes_cli/test_kanban_db.py
  • live resolver smoke: open NousResearch PR -> open; merged Replay PR feat(skills): add content size limits for agent-created skills #4391 -> merged
  • .venv/bin/ty check ... reports 67 pre-existing diagnostics in these legacy files; none point to the added lines

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cron Cron scheduler and job management labels Aug 11, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #72555: both address stale active_pr respawn guards for closed or merged PRs, but this PR makes live GitHub lookups mandatory while #72555 is the earlier broader guard-semantics approach. Maintainer selection is needed.

@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(kanban): release respawn guard for closed PRs

  1. hermes_cli/kanban_db.py_RESPAWN_GUARD_PR_STATE_CACHE is a module-global dict mutated without a lock: the expired-entry cleanup (for cache_key in expired: _RESPAWN_GUARD_PR_STATE_CACHE.pop(...)) will raise RuntimeError: dictionary changed size during iteration if check_respawn_guard ever runs concurrently (multiple lanes/threads, gateway + CLI in one process). Either document the single-threaded assumption next to the global or guard with a threading.Lock.

  2. Eviction is insertion-ordered, not LRU: re-assigning an existing key (_RESPAWN_GUARD_PR_STATE_CACHE[key] = ...) does not move it to the end of the dict, so a hot URL that keeps getting refreshed sits at its original (old) position and is evicted first, while cold entries survive. collections.OrderedDict.move_to_end (or delete-then-reinsert on hit) would make the cap evict the least-recently-used entry.

  3. Fail-closed semantics: a GitHub outage or a machine without gh caches None for 10 min per URL, so tasks stay blocked. That's the intended duplicate-PR safety trade-off, but "gh not installed" is permanent while "PR state unresolvable" is transient — distinguishing the two (e.g. cache the failure type, or skip live lookups when gh is known-absent) would avoid silently blocking tasks forever on a host that never had gh.

  4. Minor: the 10-min cache also applies to a successfully-resolved "open" — a PR merged seconds ago still suppresses respawn for up to 10 min. Acceptable, just worth stating in the docstring alongside the window.

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

Labels

comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants