fix(gateway): close restart admission bypasses - #4
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ec7a19fcd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if "restart_command_enabled" not in data: | ||
| restart_command_enabled = True |
There was a problem hiding this comment.
When an operator sets the documented gateway.restart_command_enabled: false in ~/.hermes/config.yaml, load_gateway_config() never copies that nested key into gw_data (unlike write_sessions_json at lines 1106-1112), so this branch treats it as absent and enables /restart. The new control therefore does not work through the production configuration path; the tests call GatewayConfig.from_dict() directly and miss this propagation failure. Copy the nested setting before constructing GatewayConfig and cover the real loader with a temporary HERMES_HOME.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
| if active_work != 0: | ||
| logger.warning( | ||
| "SIGUSR1 restart refused: %d active agent/cron/API run(s)", active_work | ||
| ) | ||
| return False |
There was a problem hiding this comment.
When hermes gateway restart or the update flow sends SIGUSR1 while any work is active, _graceful_restart_via_sigusr1() sends the signal only once and then waits for the process to exit; this refusal provides no acknowledgement, so the caller waits for the full drain timeout and falls through to systemctl restart or SIGTERM—the hard path that can interrupt the active work. Preserve a drain-aware operator path, or make these callers preflight and report a busy refusal instead of timing out and forcing the restart.
AGENTS.md reference: AGENTS.md:L114-L117
Useful? React with 👍 / 👎.
| "SIGUSR1 restart refused: %d active agent/cron/API run(s)", active_work | ||
| ) | ||
| return False | ||
| return bool(runner.request_restart(detached=False, via_service=True)) |
There was a problem hiding this comment.
When an API request or cron tick is admitted immediately after this callback returns, request_restart() has set _restart_requested but _draining is not set until the scheduled restart task reaches stop() after its 50 ms sleep. The API and cron admission gates inspect only _draining/_external_drain_active, so new work can register after the zero-work check and still be interrupted by this supposedly idle-only restart. Make the common admission gates honor the restart flag synchronously, or coordinate admission and counting with a shared lock.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9206bcc8e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Close chat/API/in-process-cron admission before reading counters. The | ||
| # synchronous gate is visible to the cron scheduler thread's can_dispatch | ||
| # callback and is rolled back on every refusal. | ||
| runner._draining = True |
There was a problem hiding this comment.
When SIGUSR1 arrives after cron.scheduler.tick() passes its can_dispatch check at line 3726 but before _submit_with_guard() adds the due job to _running_job_ids at line 3816, setting _draining here cannot stop that already-admitted tick: the strict snapshot sees zero work, accepts the restart, and the cron thread then launches work during teardown. Fresh evidence beyond the earlier admission comment is the unchanged gap between the cron gate and claim, with no second gate or shared lock; coordinate the restart snapshot with the cron claim or recheck admission while claiming.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b40e72fc9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Close chat/API/in-process-cron admission before reading counters. The | ||
| # synchronous gate is visible to the cron scheduler thread's can_dispatch | ||
| # callback and is rolled back on every refusal. | ||
| runner._draining = True |
There was a problem hiding this comment.
When a normal chat handler has passed the _draining check at line 10039 and is suspended in the unconditional topic-root await at line 10272, SIGUSR1 can execute this zero-work admission, set _draining, and schedule shutdown; the handler then resumes, checks only _external_drain_active at line 10289, and installs its running sentinel at line 10321. This starts an agent after the idle-only restart was accepted, potentially force-interrupting it when the drain times out. Fresh evidence beyond the earlier API/cron reports is this unchanged await-to-claim gap; recheck _draining immediately before the final claim or make the claim reject draining atomically.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 442c4b014d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def request_restart(self, *, detached: bool = False, via_service: bool = False) -> bool: | ||
| if self._restart_task_started: | ||
| return False | ||
| self._draining = True |
There was a problem hiding this comment.
/restart with cron admission
When a messaging /restart races a ticker between the slash handler's _active_work_count() snapshot and _submit_with_guard(), the ticker can claim a cron job before this line finally closes admission; shutdown then proceeds with active work and may interrupt that job after the drain timeout. The new _admit_restart_signal() closes this race only for SIGUSR1, so the sibling chat path should close admission and perform the strict count under the same coordination before committing the restart.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
| return None | ||
| # Advance only after the atomic restart gate. A refused claim | ||
| # stays due for the next allowed tick instead of being skipped. | ||
| advance_next_run(job_id) |
There was a problem hiding this comment.
When another process holds .jobs.lock, advance_next_run() can wait for the configured 30-second lock timeout while this code holds _running_lock. A SIGUSR1 callback concurrently calls get_running_job_ids() from the gateway event loop, so it blocks on that lock and freezes all gateway traffic for the same interval. Publish the running claim under _running_lock, then perform the persistent next-run update without holding the running-set lock, rolling back the claim if necessary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4657ad6666
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if "restart_command_enabled" not in data: | ||
| restart_command_enabled = True |
There was a problem hiding this comment.
The ordinary load_gateway_config() path now flattens gateway.restart_command_enabled, but gateway/run.py:21245 still feeds --config YAML directly to GatewayConfig.from_dict(). In that path, a config file containing the documented gateway: {restart_command_enabled: false} falls into this branch because only the top-level key is checked, so /restart remains enabled despite the operator policy; read the same nested key here before defaulting to True.
AGENTS.md reference: AGENTS.md:L54-L57
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebe00f052a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if adapter._draining_response() is not None: | ||
| raise RuntimeError("gateway is draining; pending API work refused") |
There was a problem hiding this comment.
When a Chronos fire request passes the earlier _draining_response() check and a restart closes admission before this guard runs, this RuntimeError escapes _handle_cron_fire() because that call site does not catch it. The client then gets an aiohttp 500 instead of the retryable 503/Retry-After response used for all other drain refusals, so a normal maintenance race can look like a server failure to the scheduler; return/catch the draining response here instead of raising a generic exception.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Superseded by the current-main restart boundary — 2026-08-10This recovery-branch PR is permanently unsuitable for integration:
Outcome: Closing #4 does not declare DTC-03 complete and does not activate anything in runtime. The remaining path is #7 → CI/review → merge → supervised deployment and live canary. |
1 participant
Summary
gateway.restart_command_enabledwith strict fail-closed parsing and operator policy disable;gateway.lockinode during stale PID cleanup, preventing split singleton locks.Incident evidence
/restartinterrupted 8 active agents because the running PID predated the busy gate;systemctl reloadremained an external bypass;gateway.lockwas unlinked while the gateway still held its old fd.Verification