pokebedrock/pokebedrock-hub
4
0
Fork 0
Code Issues 4 Pull requests 3 Projects Releases 60 Packages Activity Actions

fix(watchdog): make HUB-3 goroutine alerts diagnosable and load-aware #103

Merged
smell-of-curry merged 6 commits from fix/hub-3-watchdog-dump into main 2026-10-16 19:07:14 +00:00
Conversation 4 Commits 6 Files changed 5 +104 -9
bot commented 2026-10-13 12:58:00 +00:00
Member
Copy link

Fixes POKEBEDROCK-HUB-3

Root cause

HUB-3 (789 events, release 24378cf, ns1018631) is not a monotonic leak. Over Oct 11-13 the reported count swings both ways within minutes: 1498 -> 1126 -> 832 in 33 min on Oct 11, and 1514 at 11:34Z today after 832 at 10:16Z. It tracks the daily player curve. A leak only grows until restart. The audited spawn sites (infliction/rank/player-details workers, the per-join delayed loaders, NetherNet ConnContext 20s) are bounded or cancelled, and every HTTP client has a timeout. So the fixed 800 threshold fires on normal peak load, because each RakNet/NetherNet session (pion ICE/DTLS/SCTP) owns several goroutines.

We could not confirm this from stacks because the goroutine dump never reached Sentry. Sentry's server-side scrubber replaces contexts.watchdog.goroutine_dump with [Filtered] on every event. The Oct 11 heap blowup (14-33 GB) is a separate signal (heap_pressure) and is not explained by this PR.

Change

  • Send the goroutine dump as a goroutines.txt Sentry attachment (not scrubbed), so the next breach carries real stacks.
  • Record players in the watchdog context and log line.
  • Add optional [Watchdog] GoroutinesPerPlayer headroom (limit = GoroutineThreshold + N*players). It defaults to 0, so alerting behaviour is unchanged until it's configured.

Testing

go build ./... and go test ./pokebedrock/watchdog/ pass locally.

Risks

  • Attachment up to 1 MiB per alert, throttled by the 5-min cooldown. The self-hosted Sentry must accept attachments.
  • The dump may include player names or XUIDs in stack args. It's stored as an attachment, so it isn't scrubbed.
  • No behaviour change unless GoroutinesPerPlayer is set. Suggested value: set it after the first attachment shows per-session goroutine cost.

Do not merge or deploy without Smell of Curry's OK.

Fixes POKEBEDROCK-HUB-3 ## Root cause HUB-3 (789 events, release 24378cf, ns1018631) is not a monotonic leak. Over Oct 11-13 the reported count swings both ways within minutes: 1498 -> 1126 -> 832 in 33 min on Oct 11, and 1514 at 11:34Z today after 832 at 10:16Z. It tracks the daily player curve. A leak only grows until restart. The audited spawn sites (infliction/rank/player-details workers, the per-join delayed loaders, NetherNet ConnContext 20s) are bounded or cancelled, and every HTTP client has a timeout. So the fixed 800 threshold fires on normal peak load, because each RakNet/NetherNet session (pion ICE/DTLS/SCTP) owns several goroutines. We could not confirm this from stacks because the goroutine dump never reached Sentry. Sentry's server-side scrubber replaces `contexts.watchdog.goroutine_dump` with `[Filtered]` on every event. The Oct 11 heap blowup (14-33 GB) is a separate signal (heap_pressure) and is not explained by this PR. ## Change - Send the goroutine dump as a `goroutines.txt` Sentry attachment (not scrubbed), so the next breach carries real stacks. - Record `players` in the watchdog context and log line. - Add optional `[Watchdog] GoroutinesPerPlayer` headroom (limit = GoroutineThreshold + N*players). It defaults to 0, so alerting behaviour is unchanged until it's configured. ## Testing `go build ./...` and `go test ./pokebedrock/watchdog/` pass locally. ## Risks - Attachment up to 1 MiB per alert, throttled by the 5-min cooldown. The self-hosted Sentry must accept attachments. - The dump may include player names or XUIDs in stack args. It's stored as an attachment, so it isn't scrubbed. - No behaviour change unless GoroutinesPerPlayer is set. Suggested value: set it after the first attachment shows per-session goroutine cost. Do not merge or deploy without Smell of Curry's OK.
bot reviewed 2026-10-13 13:00:07 +00:00
bot left a comment
Copy link

Desk review @ adf515f7 — COMMENT (hold for Curry)

Scope is tight and matches the title. Attachment dump for HUB-3 is the right fix for the scrubbed contexts.watchdog.goroutine_dump, players in log/context is useful, and GoroutinesPerPlayer defaulting to 0 keeps alerting behaviour unchanged until configured. poke.srv.PlayerCount is a real *Server method value; nil-safe hook is fine.

Not a full HUB-3 fix on merge alone

With default config this only makes the next breach diagnosable. Peak-load noise at the fixed 800 threshold continues until GoroutinesPerPlayer is set after the first attachment shows per-session cost. PR body already says not to merge/deploy without Smell of Curry's OK — desk will not merge.

Same-repo Forgejo issues

Open hub issues (#93 quest UI, #86 parkour, #77 Discord roles, #55 restart status) do not match this diff. Fixes POKEBEDROCK-HUB-3 is Sentry, not a Forgejo issue — no Fixes: / Could Fix: link comment.

CI

No commit statuses on adf515f7 yet (Forgejo Actions API returned empty for this head). Local claim in the PR body is fine; do not treat as merge-ready from CI alone.

Should fix (inline)

  1. TestEvaluate has no GoroutinesPerPlayer cases — the new limit path is the point of the PR and evaluate is already the pure harness.
  2. Start does not log GoroutinesPerPlayer — operators will want that when it is non-zero.

Nits

  • Sentry watchdog context has goroutines / players but not the computed goroutine_limit; cheap and helps once GPP is live.
  • Negative GoroutinesPerPlayer in config would lower the threshold; clamp at 0 if you care.

Not approving. Not merging.

## Desk review @ `adf515f7` — COMMENT (hold for Curry) Scope is tight and matches the title. Attachment dump for HUB-3 is the right fix for the scrubbed `contexts.watchdog.goroutine_dump`, `players` in log/context is useful, and `GoroutinesPerPlayer` defaulting to 0 keeps alerting behaviour unchanged until configured. `poke.srv.PlayerCount` is a real `*Server` method value; nil-safe hook is fine. ### Not a full HUB-3 fix on merge alone With default config this only makes the next breach diagnosable. Peak-load noise at the fixed 800 threshold continues until `GoroutinesPerPlayer` is set after the first attachment shows per-session cost. PR body already says not to merge/deploy without Smell of Curry's OK — desk will not merge. ### Same-repo Forgejo issues Open hub issues (#93 quest UI, #86 parkour, #77 Discord roles, #55 restart status) do not match this diff. `Fixes POKEBEDROCK-HUB-3` is Sentry, not a Forgejo issue — no `Fixes:` / `Could Fix:` link comment. ### CI No commit statuses on `adf515f7` yet (Forgejo Actions API returned empty for this head). Local claim in the PR body is fine; do not treat as merge-ready from CI alone. ### Should fix (inline) 1. **`TestEvaluate` has no `GoroutinesPerPlayer` cases** — the new limit path is the point of the PR and `evaluate` is already the pure harness. 2. **`Start` does not log `GoroutinesPerPlayer`** — operators will want that when it is non-zero. ### Nits - Sentry watchdog context has `goroutines` / `players` but not the computed `goroutine_limit`; cheap and helps once GPP is live. - Negative `GoroutinesPerPlayer` in config would lower the threshold; clamp at 0 if you care. Not approving. Not merging.
pokebedrock/watchdog/watchdog.go
bot commented 2026-10-13 13:00:07 +00:00
Author
Member
Copy link

Nit: log goroutines_per_player (and maybe that the effective limit is threshold + N*players) on start when non-zero. Right now startup only prints the fixed threshold, so a live GPP config is invisible in the boot line.

Nit: log `goroutines_per_player` (and maybe that the effective limit is `threshold + N*players`) on start when non-zero. Right now startup only prints the fixed threshold, so a live GPP config is invisible in the boot line.
pokebedrock/watchdog/watchdog.go Outdated
@ -259,3 +282,4 @@
ctx := sentry.Context{
"goroutines": s.goroutines,
"players": s.players,
bot commented 2026-10-13 13:00:07 +00:00
Author
Member
Copy link

Nit: include goroutine_limit: w.goroutineLimit(s) in this context (and maybe the log attrs above). Once GPP is set, Sentry events with only raw goroutines/players force a mental multiply to see why it fired or did not.

Nit: include `goroutine_limit: w.goroutineLimit(s)` in this context (and maybe the log attrs above). Once GPP is set, Sentry events with only raw `goroutines`/`players` force a mental multiply to see why it fired or did not.
pokebedrock/watchdog/watchdog_test.go Outdated
bot commented 2026-10-13 13:00:07 +00:00
Author
Member
Copy link

Should fix: add GoroutinesPerPlayer cases here. Example: conf GoroutineThreshold=800, GoroutinesPerPlayer=5, snapshot goroutines=900, players=100 must not alert (limit 1300); goroutines=1300, players=100 must alert; players=0 keeps the fixed threshold. Also one nil/PlayerCount unused path if you exercise playerCount() via runCheck later — at minimum cover evaluate + goroutineLimit.

Should fix: add `GoroutinesPerPlayer` cases here. Example: conf `GoroutineThreshold=800, GoroutinesPerPlayer=5`, snapshot `goroutines=900, players=100` must not alert (limit 1300); `goroutines=1300, players=100` must alert; `players=0` keeps the fixed threshold. Also one nil/`PlayerCount` unused path if you exercise `playerCount()` via `runCheck` later — at minimum cover `evaluate` + `goroutineLimit`.
bot reviewed 2026-10-14 11:11:58 +00:00
bot left a comment
Copy link

Desk review @ cccc9632 — COMMENT (hold for Curry)

Follow-up to review 3137 @ adf515f7. New commits 24628fe3 + cccc9632 address every should-fix and nit from that pass:

  • TestEvaluateGoroutinesPerPlayer / negative clamp / nil PlayerCount hook
  • Start logs goroutines_per_player
  • Sentry watchdog context includes goroutine_limit
  • Negative GoroutinesPerPlayer clamped to 0 in withDefaults

Diff still matches the title: attachment dump for HUB-3 scrubbing, players in log/context, optional scaled limit defaulting to 0. goroutineDump remains capped at 1 MiB. No new findings.

Still not a full HUB-3 fix on merge alone

Default config keeps the fixed 800 threshold; peak-load noise continues until Curry sets GoroutinesPerPlayer after a real attachment. HUB-3 stays watch-only. PR body: do not merge/deploy without Smell of Curry's OK — desk will not merge.

Same-repo Forgejo issues

Open hub issues (#93 quest UI, #86 parkour, #77 Discord roles, #55 restart status) still do not match. Fixes POKEBEDROCK-HUB-3 is Sentry only — no Fixes: / Could Fix: link comment.

CI

No commit statuses on cccc9632 yet (Forgejo statuses API empty for this head). Local go test ./pokebedrock/watchdog/ claim stands; do not treat as merge-ready from CI alone.

Not approving. Not merging.

## Desk review @ `cccc9632` — COMMENT (hold for Curry) Follow-up to review 3137 @ `adf515f7`. New commits `24628fe3` + `cccc9632` address every should-fix and nit from that pass: - `TestEvaluateGoroutinesPerPlayer` / negative clamp / nil `PlayerCount` hook - `Start` logs `goroutines_per_player` - Sentry watchdog context includes `goroutine_limit` - Negative `GoroutinesPerPlayer` clamped to 0 in `withDefaults` Diff still matches the title: attachment dump for HUB-3 scrubbing, `players` in log/context, optional scaled limit defaulting to 0. `goroutineDump` remains capped at 1 MiB. No new findings. ### Still not a full HUB-3 fix on merge alone Default config keeps the fixed 800 threshold; peak-load noise continues until Curry sets `GoroutinesPerPlayer` after a real attachment. HUB-3 stays watch-only. PR body: do not merge/deploy without Smell of Curry's OK — desk will not merge. ### Same-repo Forgejo issues Open hub issues (#93 quest UI, #86 parkour, #77 Discord roles, #55 restart status) still do not match. `Fixes POKEBEDROCK-HUB-3` is Sentry only — no `Fixes:` / `Could Fix:` link comment. ### CI No commit statuses on `cccc9632` yet (Forgejo statuses API empty for this head). Local `go test ./pokebedrock/watchdog/` claim stands; do not treat as merge-ready from CI alone. Not approving. Not merging.
bot reviewed 2026-10-14 11:12:09 +00:00
bot left a comment
Copy link

Desk review @ cccc963 — COMMENT (hold for Curry)

Follow-up to review 3137 @ adf515f7 (now stale). Commits 24628fe + cccc963 address the should-fix and nits:

  • TestEvaluateGoroutinesPerPlayer / negative clamp / nil PlayerCount hook coverage looks correct (scaled limit 800+3*200; GPP=0 keeps fixed threshold).
  • Start logs goroutines_per_player.
  • Sentry watchdog context includes goroutine_limit.
  • Negative GPP clamped in withDefaults.

Local go test ./pokebedrock/watchdog/ passes on this head.

Still hold

Default config is still diagnose-only (attachment dump). Peak-load noise at fixed 800 continues until GoroutinesPerPlayer is set after the first real attachment. PR body already says not to merge/deploy without Smell of Curry's OK — desk will not merge.

Same-repo Forgejo issues

Open hub issues (#93, #86, #77, #55) still do not match. Fixes POKEBEDROCK-HUB-3 remains Sentry-only — no new Fixes: / Could Fix: comment.

CI

Hub Actions is release-on-main only; no commit statuses on this head (same as prior review). Do not treat as merge-ready from CI.

Soft nit (optional)

config.example.toml [Watchdog] still omits GoroutinesPerPlayer — operators copying the example will not see the new knob. Not blocking.

Not approving. Not merging.

## Desk review @ `cccc963` — COMMENT (hold for Curry) Follow-up to review 3137 @ `adf515f7` (now stale). Commits `24628fe` + `cccc963` address the should-fix and nits: - `TestEvaluateGoroutinesPerPlayer` / negative clamp / nil `PlayerCount` hook coverage looks correct (scaled limit 800+3*200; GPP=0 keeps fixed threshold). - `Start` logs `goroutines_per_player`. - Sentry watchdog context includes `goroutine_limit`. - Negative GPP clamped in `withDefaults`. Local `go test ./pokebedrock/watchdog/` passes on this head. ### Still hold Default config is still diagnose-only (attachment dump). Peak-load noise at fixed 800 continues until `GoroutinesPerPlayer` is set after the first real attachment. PR body already says not to merge/deploy without Smell of Curry's OK — desk will not merge. ### Same-repo Forgejo issues Open hub issues (#93, #86, #77, #55) still do not match. `Fixes POKEBEDROCK-HUB-3` remains Sentry-only — no new `Fixes:` / `Could Fix:` comment. ### CI Hub Actions is release-on-`main` only; no commit statuses on this head (same as prior review). Do not treat as merge-ready from CI. ### Soft nit (optional) `config.example.toml` `[Watchdog]` still omits `GoroutinesPerPlayer` — operators copying the example will not see the new knob. Not blocking. Not approving. Not merging.
bot reviewed 2026-10-14 11:13:51 +00:00
bot left a comment
Copy link

Desk review @ cccc9632 — COMMENT (hold for Curry)

Follow-up commits 24628fe / cccc963 address review 3137: TestEvaluateGoroutinesPerPlayer + nil-hook/negative-clamp tests, Start logs goroutines_per_player, Sentry context includes goroutine_limit, and negative GPP is clamped in withDefaults. go test ./pokebedrock/watchdog/ passes on this head.

Still not a full HUB-3 fix on merge alone

Default GoroutinesPerPlayer=0 keeps the fixed 800 threshold. Peak-load noise continues until GPP is set from a real attachment. PR body still requires Smell of Curry's OK — desk will not merge or deploy.

Same-repo Forgejo issues

Open hub issues (#93 quest UI, #86 parkour, #77 Discord roles, #55 restart status) still do not match this diff. Fixes POKEBEDROCK-HUB-3 is Sentry-only — no Fixes: / Could Fix: link comment.

Remaining nit

config.example.toml [Watchdog] still omits GoroutinesPerPlayer (struct field is documented). Worth a one-liner so operators discover the knob.

Not approving. Not merging.

## Desk review @ `cccc9632` — COMMENT (hold for Curry) Follow-up commits `24628fe` / `cccc963` address review 3137: `TestEvaluateGoroutinesPerPlayer` + nil-hook/negative-clamp tests, `Start` logs `goroutines_per_player`, Sentry context includes `goroutine_limit`, and negative GPP is clamped in `withDefaults`. `go test ./pokebedrock/watchdog/` passes on this head. ### Still not a full HUB-3 fix on merge alone Default `GoroutinesPerPlayer=0` keeps the fixed 800 threshold. Peak-load noise continues until GPP is set from a real attachment. PR body still requires Smell of Curry's OK — desk will not merge or deploy. ### Same-repo Forgejo issues Open hub issues (#93 quest UI, #86 parkour, #77 Discord roles, #55 restart status) still do not match this diff. `Fixes POKEBEDROCK-HUB-3` is Sentry-only — no `Fixes:` / `Could Fix:` link comment. ### Remaining nit `config.example.toml` `[Watchdog]` still omits `GoroutinesPerPlayer` (struct field is documented). Worth a one-liner so operators discover the knob. Not approving. Not merging.
pokebedrock/config.go
bot commented 2026-10-14 11:13:51 +00:00
Author
Member
Copy link

Nit: config.example.toml [Watchdog] still has no GoroutinesPerPlayer line. A commented GoroutinesPerPlayer = 0 (or a short note that N adds headroom per online player) would match the struct docs so operators can find the knob after the first attachment dump.

Nit: `config.example.toml` `[Watchdog]` still has no `GoroutinesPerPlayer` line. A commented `GoroutinesPerPlayer = 0` (or a short note that N adds headroom per online player) would match the struct docs so operators can find the knob after the first attachment dump.
Operators copying config.example.toml never saw the headroom knob.

Co-authored-by: Cursor <cursoragent@cursor.com>
smell-of-curry deleted branch fix/hub-3-watchdog-dump 2026-10-16 19:07:14 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
blocked
automation label

  
bug
Something isn't working

  
dependencies
Pull requests that update a dependency file

  
documentation
Improvements or additions to documentation

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
github_actions
Pull requests that update GitHub Actions code

  
go
Pull requests that update Go code

  
good first issue
Good for newcomers

  
help wanted
Extra attention is needed

  
in-progress
automation label

  
invalid
This doesn't seem right

  
question
Further information is requested

  
ready-to-fix
automation label

  
source:discord
automation label

  
source:sentry
automation label

  
source:server-logs
automation label

  
triage-needed
automation label

  
wontfix
This will not be worked on

No labels
blocked
bug
dependencies
documentation
duplicate
enhancement
github_actions
go
good first issue
help wanted
in-progress
invalid
question
ready-to-fix
source:discord
source:sentry
source:server-logs
triage-needed
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
pokebedrock/pokebedrock-hub!103
No description provided.