Track healthy duration before resetting restart streak

The stability reset currently keys off now - record.lastRestartAt >= STABLE_THRESHOLD_MS, which means a channel can be unhealthy for a long time, become healthy for a single check, and immediately zero consecutiveRestarts. In that flapping scenario, the restart streak is cleared without actually being stable for 5 minutes, so escalation (onBeforeRestart) can be repeatedly delayed or skipped and Discord clean-restart requests may never trigger as intended.

Useful? React with 👍 / 👎.

Fixed in 68fd7d6 — added healthySince?: number to RestartRecord.

The stability window now measures continuous uptime from the first healthy observation after a restart, not elapsed time since the last restart. healthySince is set on the first healthy check after a restart and cleared immediately when the channel goes unhealthy — so a brief healthy→unhealthy flap cannot satisfy the 5-minute window and prematurely zero consecutiveRestarts.

New test: "does not reset consecutive restart counter on brief healthy→unhealthy flap" covers this exact scenario. 32/32 passing.

onBeforeRestart fires before confirming restart succeeded. The callback is invoked at lines 212-218, before the try block. If stopChannel or startChannel throws, record.consecutiveRestarts is not updated (stays at, e.g., 2). On the next check interval, once the cooldown expires, the same threshold is reached and onBeforeRestart fires again — even though no lifecycle was ever actually started.

The immediate practical consequence is that forceCleanRestart is re-set to true on every failed retry cycle at or above the threshold, rather than exactly once per restart decision. As long as the callback stays idempotent this won't cause a functional breakage, but it violates the documented contract and could be surprising if the callback ever gains additional side-effects.

Suggested fix: Move the escalation call inside the try block (right before startChannel), or update the record on failure:

try {
  if (status.running) {
    await channelManager.stopChannel(channelId as ChannelId, accountId);
  }
  channelManager.resetRestartAttempts(channelId as ChannelId, accountId);

  if (nextConsecutive >= ESCALATION_THRESHOLD && onBeforeRestart) {
    onBeforeRestart({ channelId: channelId as ChannelId, accountId, consecutiveRestarts: nextConsecutive });
  }

  await channelManager.startChannel(channelId as ChannelId, accountId);
  record.lastRestartAt = now;
  record.restartsThisHour.push({ at: now });
  record.consecutiveRestarts = nextConsecutive;
  restartRecords.set(key, record);
} catch (err) { … }

This is a comment left during a code review.
Path: src/gateway/channel-health-monitor.ts
Line: 212-218

Comment:
`onBeforeRestart` fires before confirming restart succeeded. The callback is invoked at lines 212-218, *before* the try block. If `stopChannel` or `startChannel` throws, `record.consecutiveRestarts` is not updated (stays at, e.g., `2`). On the next check interval, once the cooldown expires, the same threshold is reached and `onBeforeRestart` fires again — even though no lifecycle was ever actually started.

The immediate practical consequence is that `forceCleanRestart` is re-set to `true` on every failed retry cycle at or above the threshold, rather than exactly once per restart decision. As long as the callback stays idempotent this won't cause a functional breakage, but it violates the documented contract and could be surprising if the callback ever gains additional side-effects.

**Suggested fix**: Move the escalation call inside the try block (right before `startChannel`), or update the record on failure:

```typescript
try {
  if (status.running) {
    await channelManager.stopChannel(channelId as ChannelId, accountId);
  }
  channelManager.resetRestartAttempts(channelId as ChannelId, accountId);

  if (nextConsecutive >= ESCALATION_THRESHOLD && onBeforeRestart) {
    onBeforeRestart({ channelId: channelId as ChannelId, accountId, consecutiveRestarts: nextConsecutive });
  }

  await channelManager.startChannel(channelId as ChannelId, accountId);
  record.lastRestartAt = now;
  record.restartsThisHour.push({ at: now });
  record.consecutiveRestarts = nextConsecutive;
  restartRecords.set(key, record);
} catch (err) { … }
```

How can I resolve this? If you propose a fix, please make it concise.

Fixed — addressed in commit 5979119a73 via a different approach than suggested but with the same end result.

Rather than moving onBeforeRestart inside the try block, record.consecutiveRestarts is now incremented optimistically before the try block (set to nextConsecutive and persisted via restartRecords.set before the restart attempt). This means:

• On failure, consecutiveRestarts is already at nextConsecutive, so the next check cycle sees a higher level — the escalation threshold cannot be re-triggered at the same counter value.
• onBeforeRestart still fires pre-try (intentional: the callback is meant to configure the next start, before startChannel runs), but it only fires once per counter level rather than on every failed retry.

Combined with the catch block now setting record.lastRestartAt = now (from the fix to r2909175303), the cooldown guard is also applied on persistent stopChannel/startChannel failures, so rapid-fire re-invocations of the callback are additionally rate-limited.

Fixed in 2e44b5f + 509174b.

Two-part resolution:

1. consecutiveRestarts is now incremented optimistically before the try block — so if stopChannel/startChannel throws, the counter is already past the threshold on the next cycle and onBeforeRestart cannot re-fire at the same threshold level (contract preserved).
2. The catch block now sets record.lastRestartAt = now and persists via restartRecords.set, so the exponential cooldown applies on persistent failure cycles and retries no longer happen every check interval.

50/50 tests passing.

Fixed. consecutiveRestarts is now incremented optimistically before the try block (commit 5979119), so if stopChannel/startChannel throws: (1) the escalation counter is already updated — onBeforeRestart will not re-fire at the same threshold level on the next cooldown expiry; (2) the catch block also sets record.lastRestartAt = now (commit 2e44b5f) so the cooldown guard properly applies on persistent failure cycles. The callback contract is now upheld.

The module-level persistentStates Map grows for every account that ever calls getOrCreatePersistentState. There is currently no path that removes an entry — not markStable, not an account-deletion event, nothing. For a long-running gateway process where Discord accounts are added and later removed or disabled, this is a slow memory leak.

Consider adding an evictPersistentState(accountId: string) export (analogous to markStable) that the server can call when an account is fully removed from the config, similar to how unregisterGateway cleans up the gateway registry:

export function evictPersistentState(accountId: string): void {
  persistentStates.delete(accountId);
}

This would also make the module easier to test in isolation (tests currently share the same Map instance across describe blocks unless they use unique accountId strings).

This is a comment left during a code review.
Path: src/discord/monitor/provider.lifecycle.ts
Line: 25-48

Comment:
The module-level `persistentStates` Map grows for every account that ever calls `getOrCreatePersistentState`. There is currently no path that removes an entry — not `markStable`, not an account-deletion event, nothing. For a long-running gateway process where Discord accounts are added and later removed or disabled, this is a slow memory leak.

Consider adding an `evictPersistentState(accountId: string)` export (analogous to `markStable`) that the server can call when an account is fully removed from the config, similar to how `unregisterGateway` cleans up the gateway registry:

```typescript
export function evictPersistentState(accountId: string): void {
  persistentStates.delete(accountId);
}
```

This would also make the module easier to test in isolation (tests currently share the same Map instance across describe blocks unless they use unique `accountId` strings).

How can I resolve this? If you propose a fix, please make it concise.

Fixed — evictPersistentState(accountId: string) export was added to src/discord/monitor/provider.lifecycle.ts alongside markStable. server.impl.ts now calls evictDiscordPersistentState(accountId) for each account that is present in prevDiscordAccountIds but absent from the current config on hot reload, mirroring the unregisterGateway cleanup pattern. The Map entry is also cleared on full Discord removal.

Fixed in ba3a20d (local: evictPersistentState added to src/discord/monitor/provider.lifecycle.ts).

export function evictPersistentState(accountId: string): void {
  persistentStates.delete(accountId);
}

evictDiscordPersistentState is now called from server.impl.ts on hot reload whenever a Discord account is removed from the config. Also added a test-isolation note: tests should use unique accountId strings per describe block (or call evictPersistentState in afterEach) to avoid state bleed across describe blocks in the shared Map.

Fixed in 5979119. Added evictDiscordPersistentState(accountId: string) export (calls persistentStates.delete(accountId)). It is wired in server.impl.ts during hot reload: when prevDiscordAccountIds contains IDs no longer in the new config, each removed account is evicted. Three scenarios handled: single-account token shape (preserves "default"), multi-account shape (evicts only removed accounts), Discord fully removed (evicts all). Tests cover the eviction path.

Derive eviction set from explicit Discord accounts

nextDiscordAccountIds is built with listDiscordAccountIds, which falls back to an implicit "default" account when no Discord accounts are configured. On hot reloads that remove Discord (or remove the default account), this keeps default in the set and prevents evictDiscordPersistentState("default"), so stale consecutiveHelloStalls/forceCleanRestart state survives account removal and can affect a later re-add of that account.

Useful? React with 👍 / 👎.

Fixed in 2e44b5f.

listDiscordAccountIds is replaced by the new listConfiguredDiscordAccountIds export (backed by listConfiguredAccountIds from the account-helpers factory) in both the server.impl.ts eviction path and the import. listConfiguredAccountIds returns an empty array when no Discord accounts are configured rather than falling back to ["default"], so removing Discord entirely now correctly evicts all previous account entries.

Apply cooldown bookkeeping when restart attempts fail

This increments consecutiveRestarts before the restart attempt, but failed attempts do not update lastRestartAt; the cooldown check is based on now - record.lastRestartAt, so when stopChannel/startChannel throws, retries can still happen every check interval instead of backing off. In persistent failure scenarios, that defeats the new exponential cooldown behavior and can repeatedly trigger escalation callbacks.

Useful? React with 👍 / 👎.

Fixed in 2e44b5f.

The catch block now sets record.lastRestartAt = now (and persists via restartRecords.set) on failure, so the exponential backoff applies on persistent failure cycles. consecutiveRestarts was already incremented optimistically before the try block, so the backoff exponent was correct — this closes the remaining gap where the cooldown guard (now - record.lastRestartAt <= effectiveCooldownMs) could be bypassed on repeated stopChannel/startChannel failures.

Preserve implicit default account during Discord eviction

This cleanup computes nextDiscordAccountIds via listConfiguredDiscordAccountIds(...), but that helper returns only explicit channels.discord.accounts.* keys (empty when using the supported single-account channels.discord.token shape from DiscordConfig). In that common configuration, a Discord hot reload will see the running default account in prevDiscordAccountIds but not in nextDiscordAccountIds, so it evicts persistent state for an account that still exists, resetting consecutiveHelloStalls/forceCleanRestart and weakening the new restart-escalation behavior after every reload.

Useful? React with 👍 / 👎.

Fixed in 368bf82a4.

listConfiguredDiscordAccountIds only knows about explicit channels.discord.accounts.* keys, so the single-account channels.discord.token shape produced an empty set — causing "default" to be evicted on every hot reload.

Fix: check whether Discord is actually configured (channels.discord.token exists or listConfiguredDiscordAccountIds returns any IDs). When configured, use listDiscordAccountIds, which correctly resolves "default" for the token shape. When Discord is fully removed, fall back to an empty set so all previous entries are properly evicted.

All three scenarios now handled correctly:

• Single-account token shape → "default" preserved, no spurious eviction ✅
• Multi-account shape → only removed accounts evicted ✅
• Discord fully removed → all previous accounts evicted ✅

50/50 tests passing.

Include env-backed default account in Discord eviction set

hasDiscordConfig only checks prepared.config.channels?.discord?.token and explicit accounts.* keys, so it evaluates false when Discord is configured via DISCORD_BOT_TOKEN only. In that supported setup (resolveDiscordToken falls back to process.env.DISCORD_BOT_TOKEN for default), hot reload computes an empty nextDiscordAccountIds set and evicts default even though the account is still active, which resets consecutiveHelloStalls/forceCleanRestart and weakens the new restart-escalation flow after each reload.

Useful? React with 👍 / 👎.

Fixed — added || !!process.env.DISCORD_BOT_TOKEN to hasDiscordConfig so the env-backed default account is retained in the eviction set. All four scenarios now handled:

• channels.discord.token in config → ✅ default preserved
• Explicit channels.discord.accounts.* → ✅ only removed accounts evicted
• DISCORD_BOT_TOKEN env-var only → ✅ default preserved, no spurious eviction
• Discord fully removed (no config, no env) → ✅ all previous accounts evicted

46/46 tests passing (channel-health-monitor + channel-health-policy).

Fixed in e7c8a7e — added || process.env.DISCORD_BOT_TOKEN to the hasDiscordConfig guard in server.impl.ts. This covers the env-var-only path (resolveDiscordToken falls back to process.env.DISCORD_BOT_TOKEN for the implicit "default" account) so that "default" is correctly preserved in nextDiscordAccountIds during hot reloads when Discord is configured solely via the environment variable, preventing spurious eviction of consecutiveHelloStalls/forceCleanRestart state. All three configurations now handled correctly: token in config, explicit accounts, or env-var only.

Fixed — added process.env.DISCORD_BOT_TOKEN to the hasDiscordConfig guard. When Discord is configured via env-only, hot reloads now correctly treat the default account as active and skip eviction. All tests passing.

Fixed in dbb2934.

The listConfiguredDiscordAccountIds-only approach had a gap: it returns [] for the single-account channels.discord.token shape, causing "default" to be spuriously evicted on every hot reload.

Fix applied in server.impl.ts:

const configuredIds = listConfiguredDiscordAccountIds(prepared.config);
const hasDiscordConfig = !!(
  (configuredIds.length > 0) ||
  prepared.config.channels?.discord?.token ||
  process.env.DISCORD_BOT_TOKEN
);
// When configured, preserve "default" for single-account token shape
const nextDiscordAccountIds = new Set(
  hasDiscordConfig ? (configuredIds.length > 0 ? configuredIds : ["default"]) : [],
);

All three scenarios now handled correctly:

• Single-account token shape → "default" preserved, no spurious eviction ✅
• DISCORD_BOT_TOKEN env-var-only path → "default" preserved ✅
• Multi-account shape → only removed accounts evicted ✅
• Discord fully removed → all previous accounts evicted ✅

50/50 tests passing.

Snapshot old Discord accounts before activating new config

prevDiscordAccountIds is computed from getRuntimeSnapshot() after activateRuntimeSecrets(nextConfig, { activate: true }), but getRuntimeSnapshot() derives account IDs from loadConfig() (see src/gateway/server-channels.ts), so this already reflects the new config rather than the pre-reload runtime. In an account-removal hot reload, removed IDs are absent from prevDiscordAccountIds, so evictDiscordPersistentState is never called for them and stale consecutiveHelloStalls/forceCleanRestart state can leak into a later re-add of that account.

Useful? React with 👍 / 👎.

Fixed — moved the prevDiscordAccountIds snapshot to before activateRuntimeSecrets(...) is called, so it captures the pre-reload account list. Removed accounts are now correctly included in the eviction set and evictDiscordPersistentState fires for them on hot reload. Also fixed the broken import paths (../discord/accounts.js → ../../extensions/discord/src/accounts.js) and added a regression test in server.reload.test.ts verifying eviction fires for all removed accounts. Commit: e74e4cbf0.