fix(watchdog): alert on sustained goroutine breach, add hard ceiling (HUB-3) #104

bot · 2026-10-17T19:38:05Z

bot commented

2026-10-17 19:38:05 +00:00

Do not merge without Curry's OK.

Sentry: POKEBEDROCK-HUB-3 (watchdog: goroutine count high). 337 events in 7d, 959 lifetime. Over Sep 17 to Oct 17: goroutines median 956, p90 1,315, max 1,820, never ≥2,000. 607 of 664 episodes were a single 30s check. That points to short load spikes, not a leak.

Change

Sustained breach: the soft limit (GoroutineThreshold + GoroutinesPerPlayer × players) must be breached for SustainChecks consecutive checks (default 3, i.e. 90s) before a Sentry alert. Transient breaches log at Warn. The streak resets on a healthy check.
Hard ceiling: new goroutine_ceiling condition (default 3000) alerts on the first check, regardless of players or SustainChecks. It also caps the soft limit. The ceiling and the soft pileup are mutually exclusive, so a 4k pile-up gives one goroutine alert.
Per-player default: GoroutinesPerPlayer = 0 now means the default (12) on top of the 800 base. A negative value opts out and keeps the fixed threshold. The hub needs no host config edit.
Stable fingerprint ["watchdog", <condition>], so counts in the message don't split issues. This opens one new Sentry issue for goroutine_pileup. Leave HUB-3 open until the new one is confirmed.
New config keys SustainChecks and GoroutineHardCeiling are wired through config.go / loadWatchdog / config.example.toml.

Tests

go vet ./... and go test ./... pass; go test -race ./pokebedrock/watchdog/... passes. Two existing cases were updated (the deadlock signature now expects goroutine_ceiling; the fixed-threshold opt-out now uses -1). New cases: defaults, clamp to ceiling, ceiling bypasses gate, sustain gate + reset, SustainChecks=1, stall/heap not gated, ceiling < threshold raised.

Rollout

This repo has no PR CI; release.yml runs vet+test on main. It takes effect only after merge (v0.1.64) and a hub restart on ns1018631, and both need Curry's OK. One restart picks up both #103 and this. After 24h, re-tune GoroutinesPerPlayer from the first goroutines.txt attachment.

Risk: low; this only changes monitoring. Worst case, a real slow leak under 3000 alerts 90s later than it does today.

Stacked follow-up for HUB-2 (heap profile on heap alerts) is coming as a separate PR based on this branch.

**Do not merge without Curry's OK.** Sentry: POKEBEDROCK-HUB-3 (`watchdog: goroutine count high`). 337 events in 7d, 959 lifetime. Over Sep 17 to Oct 17: goroutines median 956, p90 1,315, max 1,820, never ≥2,000. 607 of 664 episodes were a single 30s check. That points to short load spikes, not a leak. ## Change - **Sustained breach:** the soft limit (`GoroutineThreshold + GoroutinesPerPlayer × players`) must be breached for `SustainChecks` consecutive checks (default **3**, i.e. 90s) before a Sentry alert. Transient breaches log at Warn. The streak resets on a healthy check. - **Hard ceiling:** new `goroutine_ceiling` condition (default **3000**) alerts on the first check, regardless of players or SustainChecks. It also caps the soft limit. The ceiling and the soft pileup are mutually exclusive, so a 4k pile-up gives one goroutine alert. - **Per-player default:** `GoroutinesPerPlayer = 0` now means the default (**12**) on top of the 800 base. A negative value opts out and keeps the fixed threshold. The hub needs no host config edit. - **Stable fingerprint** `["watchdog", <condition>]`, so counts in the message don't split issues. This opens one new Sentry issue for `goroutine_pileup`. Leave HUB-3 open until the new one is confirmed. - New config keys `SustainChecks` and `GoroutineHardCeiling` are wired through `config.go` / `loadWatchdog` / `config.example.toml`. ## Tests `go vet ./...` and `go test ./...` pass; `go test -race ./pokebedrock/watchdog/...` passes. Two existing cases were updated (the deadlock signature now expects `goroutine_ceiling`; the fixed-threshold opt-out now uses -1). New cases: defaults, clamp to ceiling, ceiling bypasses gate, sustain gate + reset, SustainChecks=1, stall/heap not gated, ceiling < threshold raised. ## Rollout This repo has no PR CI; release.yml runs vet+test on main. It takes effect only after merge (v0.1.64) **and** a hub restart on ns1018631, and both need Curry's OK. One restart picks up both #103 and this. After 24h, re-tune GoroutinesPerPlayer from the first `goroutines.txt` attachment. Risk: low; this only changes monitoring. Worst case, a real slow leak under 3000 alerts 90s later than it does today. Stacked follow-up for HUB-2 (heap profile on heap alerts) is coming as a separate PR based on this branch.

bot added 1 commit

2026-10-17 19:38:05 +00:00

fix(watchdog): alert on sustained goroutine breach, add hard ceiling (HUB-3) 3ca70e36c3

- Soft goroutine limit must be breached for SustainChecks (default 3)
  consecutive checks before a Sentry alert; transient spikes log at Warn.
- New goroutine_ceiling condition (default 3000) alerts on the first check
  regardless of players or SustainChecks; also caps the soft limit.
- GoroutinesPerPlayer 0 now means the default (12); negative opts out.
- Stable Sentry fingerprint per watchdog condition.
- Tests for defaults, clamp, ceiling, sustain gate and reset.

bot referenced this pull request

2026-10-17 19:38:36 +00:00

fix(watchdog): attach heap profile and memstats to heap_pressure alerts (HUB-2) #105

bot reviewed

2026-10-17 21:48:54 +00:00

bot left a comment

Verdict: no blockers. Fine to merge once Curry OKs it. Posted as COMMENT because the PR is bot-authored, so it still needs a human approval.

I checked it locally because this repo has no PR CI. At 3ca70e36, go vet ./pokebedrock/watchdog/, go test -race ./pokebedrock/watchdog/ and go build ./... all pass.

The sustain gate, the ceiling-vs-soft exclusivity and the clamp all hold up when I trace them. Notes below, most important first:

The fingerprint change re-groups every watchdog condition, not just the goroutine one. SetFingerprint({"watchdog", cond}) runs for world_stall and heap_pressure as well. After the restart, stall and heap events open new Sentry issues too, including the heap events HUB-2 / #105 are chasing, and goroutine_ceiling gets its own issue. The body says only goroutine_pileup opens a new issue. Please fix the body, and leave HUB-2 open until the new heap issue is confirmed, not just HUB-3 (inline).
The ceiling-below-threshold clamp doesn't do what its comment says. If you raise the ceiling to exactly GoroutineThreshold, every soft breach (>= threshold) becomes a ceiling breach, and goroutineLimit is capped at that same value. So the sustain gate and per-player headroom both go dead, which is the outcome the comment says it prevents. Either clamp to GoroutineThreshold + 1 or above (or log a config warning), or reword the comment to say a misconfigured ceiling disables the gate. TestCeilingBelowThresholdRaised locks in the current behavior, so update it with the fix (inline).
Deployed configs change meaning silently. Any host whose TOML still says GoroutinesPerPlayer = 0 used to mean a fixed threshold and now means 12 per player. The body states this is intended. Still, call it out in the release notes for v0.1.64 so nobody reads an old config as "fixed" (inline on config.example.toml).
A ceiling check resets the soft streak. A goroutine_ceiling check yields no condGoroutines alert, so breached stays false and the streak goes to 0. With a sequence like 2.9k, 2.95k, 3.1k, 2.9k, the soft alert has to rebuild 3 checks after the ceiling fired. That's harmless since the ceiling already paged, but say so in the gateSustained doc, or count a ceiling check as a breach so the streak tells the truth (inline).
Nit: goroutine_streak is reported in the Sentry context of stall and heap alerts too, where it means nothing. Only set it for the goroutine conditions.
Nit: the @param / @returns style isn't idiomatic godoc, but the file already uses it, so this is consistent. No change needed.

Rollout is unchanged from the body: merge (v0.1.64) plus a hub restart on ns1018631, both on Curry's call.

**Verdict: no blockers. Fine to merge once Curry OKs it.** Posted as COMMENT because the PR is bot-authored, so it still needs a human approval. I checked it locally because this repo has no PR CI. At `3ca70e36`, `go vet ./pokebedrock/watchdog/`, `go test -race ./pokebedrock/watchdog/` and `go build ./...` all pass. The sustain gate, the ceiling-vs-soft exclusivity and the clamp all hold up when I trace them. Notes below, most important first: 1. **The fingerprint change re-groups every watchdog condition, not just the goroutine one.** `SetFingerprint({"watchdog", cond})` runs for `world_stall` and `heap_pressure` as well. After the restart, stall and heap events open new Sentry issues too, including the heap events HUB-2 / #105 are chasing, and `goroutine_ceiling` gets its own issue. The body says only `goroutine_pileup` opens a new issue. Please fix the body, and leave HUB-2 open until the new heap issue is confirmed, not just HUB-3 (inline). 2. **The ceiling-below-threshold clamp doesn't do what its comment says.** If you raise the ceiling *to exactly* `GoroutineThreshold`, every soft breach (`>= threshold`) becomes a ceiling breach, and `goroutineLimit` is capped at that same value. So the sustain gate and per-player headroom both go dead, which is the outcome the comment says it prevents. Either clamp to `GoroutineThreshold + 1` or above (or log a config warning), or reword the comment to say a misconfigured ceiling disables the gate. `TestCeilingBelowThresholdRaised` locks in the current behavior, so update it with the fix (inline). 3. **Deployed configs change meaning silently.** Any host whose TOML still says `GoroutinesPerPlayer = 0` used to mean a fixed threshold and now means 12 per player. The body states this is intended. Still, call it out in the release notes for v0.1.64 so nobody reads an old config as "fixed" (inline on `config.example.toml`). 4. **A ceiling check resets the soft streak.** A `goroutine_ceiling` check yields no `condGoroutines` alert, so `breached` stays false and the streak goes to 0. With a sequence like 2.9k, 2.95k, 3.1k, 2.9k, the soft alert has to rebuild 3 checks after the ceiling fired. That's harmless since the ceiling already paged, but say so in the `gateSustained` doc, or count a ceiling check as a breach so the streak tells the truth (inline). 5. Nit: `goroutine_streak` is reported in the Sentry context of stall and heap alerts too, where it means nothing. Only set it for the goroutine conditions. 6. Nit: the `@param` / `@returns` style isn't idiomatic godoc, but the file already uses it, so this is consistent. No change needed. Rollout is unchanged from the body: merge (v0.1.64) plus a hub restart on ns1018631, both on Curry's call.

config.example.toml

					
				@ -74,3 +74,3 @@

				WorldExecTimeout = "10s" # Max time a probe world task may take before the world owner is considered stalled (login-blocking deadlock). Config key kept for deployed configs.

				GoroutineThreshold = 800 # Goroutine count at/above which an alert is raised.

				GoroutinesPerPlayer = 0 # Extra goroutines per online player. Limit = GoroutineThreshold + N*players. 0 keeps the fixed threshold.

				GoroutinesPerPlayer = 12 # Extra goroutines per online player. Limit = min(GoroutineThreshold + N*players, GoroutineHardCeiling). 0 = default (12), negative = fixed threshold.

Semantics flip for deployed configs: an existing GoroutinesPerPlayer = 0 used to mean a fixed threshold and now means 12 per player. Put this in the v0.1.64 release notes.

Semantics flip for deployed configs: an existing `GoroutinesPerPlayer = 0` used to mean a fixed threshold and now means 12 per player. Put this in the v0.1.64 release notes.

pokebedrock/watchdog/watchdog.go

					
				@ -99,0 +129,4 @@

					// A ceiling below the base threshold would turn every soft breach into a

					// ceiling breach; keep it at least the base.

					if c.GoroutineHardCeiling < c.GoroutineThreshold {

						c.GoroutineHardCeiling = c.GoroutineThreshold

Raising the ceiling to exactly the threshold makes every soft breach (>= threshold) a ceiling breach, and goroutineLimit caps at the same value. So the sustain gate and per-player headroom go dead, which is the case this comment says it prevents. Either clamp above the threshold and log a config warning, or reword the comment.

Raising the ceiling to *exactly* the threshold makes every soft breach (`>= threshold`) a ceiling breach, and `goroutineLimit` caps at the same value. So the sustain gate and per-player headroom go dead, which is the case this comment says it prevents. Either clamp above the threshold and log a config warning, or reword the comment.

pokebedrock/watchdog/watchdog.go

					
				@ -237,0 +323,4 @@

						a.message += fmt.Sprintf(" (sustained %d checks)", w.goroutineStreak)

						out = append(out, a)

					}

					if !breached {

A ceiling-level check has no condGoroutines alert, so this resets the streak. The soft alert then needs 3 fresh checks after the ceiling fired. That's harmless, but either document it or count a ceiling check as a breach.

A ceiling-level check has no `condGoroutines` alert, so this resets the streak. The soft alert then needs 3 fresh checks after the ceiling fired. That's harmless, but either document it or count a ceiling check as a breach.

pokebedrock/watchdog/watchdog.go

					
				@ -286,2 +380,4 @@

						scope.SetLevel(a.level)

						scope.SetTag("watchdog_condition", string(a.cond))

						// Counts in the message must not split issues: one issue per condition.

						scope.SetFingerprint([]string{"watchdog", string(a.cond)})

This fingerprint applies to every condition, so world_stall and heap_pressure re-group into new Sentry issues too (HUB-2 included), and goroutine_ceiling gets its own. Please fix the PR body, which says only goroutine_pileup opens a new issue.

This fingerprint applies to every condition, so `world_stall` and `heap_pressure` re-group into new Sentry issues too (HUB-2 included), and `goroutine_ceiling` gets its own. Please fix the PR body, which says only goroutine_pileup opens a new issue.

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin fix/HUB-3-watchdog-sustained-breach:fix/HUB-3-watchdog-sustained-breach

git switch fix/HUB-3-watchdog-sustained-breach

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main

git merge --no-ff fix/HUB-3-watchdog-sustained-breach

git switch fix/HUB-3-watchdog-sustained-breach

git rebase main

git switch main

git merge --ff-only fix/HUB-3-watchdog-sustained-breach

git switch fix/HUB-3-watchdog-sustained-breach

git rebase main

git switch main

git merge --no-ff fix/HUB-3-watchdog-sustained-breach

git switch main

git merge --squash fix/HUB-3-watchdog-sustained-breach