Skip to content

feat: add extheartbeat shared heartbeat watchdog#150

Merged
joshiste merged 1 commit into
mainfrom
feat/extheartbeat
Jul 1, 2026
Merged

feat: add extheartbeat shared heartbeat watchdog#150
joshiste merged 1 commit into
mainfrom
feat/extheartbeat

Conversation

@joshiste

@joshiste joshiste commented Jul 1, 2026

Copy link
Copy Markdown
Member

Why

action_kit_sdk and preflight_kit_sdk each carried a byte-identical heartbeat package (a channel-based watchdog: Monitor + Notify + RecordHeartbeat/Stop). That duplication recently caused the same concurrency bug to be fixed twiceaction-kit#457 and preflight-kit#52 both had to make Stop idempotent and RecordHeartbeat a non-blocking, closed-safe send. That's the textbook signal to deduplicate.

What

Add extheartbeat here — the (already-fixed) concurrency-safe watchdog — so both SDKs can share one implementation and it can't diverge again. extension-kit is already a dependency of both SDKs, so this adds no new module coupling.

The API is unchanged from the copies both SDKs already use: Notify(ch, interval, timeout) *Monitor, (*Monitor).RecordHeartbeat(), (*Monitor).Stop().

Follow-up (after this is tagged)

Two SDK PRs will bump to the new extension-kit, switch their internals to extheartbeat, and delete their local heartbeat packages (clean removal — no external consumers). The action-kit PR will also fold in the same re-Start monitor-leak fix that preflight-kit#52 got.

Verification

go build, go vet, go test -race ./extheartbeat/ pass — includes the concurrency test (concurrent RecordHeartbeat vs repeated Stop) plus the original timeout tests.

action-kit and preflight-kit each carried an identical copy of a
channel-based heartbeat monitor — which recently required the same
concurrency fix (idempotent Stop, non-blocking closed-safe RecordHeartbeat)
to be applied twice. Extract it here as extheartbeat so both SDKs can share
one implementation and it can't diverge again. extension-kit is already a
dependency of both SDKs, so this adds no new module coupling.
@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown

Claude finished @joshiste's task in 2m 20s —— View job


Claude finished reviewing this PR

  • Check for previously unresolved review threads by Claude (none exist — no prior Claude reviews on this PR)
  • Gather context (diff, CLAUDE.md, changed files)
  • Review extheartbeat/heartbeat.go
  • Review extheartbeat/heartbeat_test.go
  • Review CHANGELOG.md
  • Post findings

Summary

This is a clean, faithful port of the already-fixed heartbeat watchdog from action_kit_sdk/preflight_kit_sdk. I reviewed it for concurrency bugs, security implications, and correctness — no issues found.

Concurrency review of heartbeat.go:

  • Stop() is properly idempotent (mutex-guarded closed check before close(h.pulse)), so concurrent/repeated Stop() calls can't double-close.
  • RecordHeartbeat() holds the same mutex before checking closed and doing the non-blocking send, so it can never race with Stop() to send on a closed channel (the exact bug the two linked SDK PRs fixed).
  • The Notify goroutine's close(signal) calls on the timeout path and the "pulse channel closed" path are mutually exclusive branches of the same select, so signal can't be double-closed either.
  • TestMonitor_concurrent_stop_and_record exercises this correctly (50 concurrent RecordHeartbeat vs 5 concurrent Stop, then a post-Stop RecordHeartbeat/Stop to confirm no panic).

Other observations (not blocking, not new to this PR):

  • If a caller never reads from ch, the goroutine can block forever on signal <- time.Now() in the timeout branch — this is inherited behavior from the existing SDK implementations, not something introduced here, and the PR explicitly preserves the existing API/behavior.
  • No new module dependencies: zerolog and testify are already in go.mod.

Verification note: I was not able to execute go build/go vet/go test -race myself in this sandboxed session (non-interactive Bash commands beyond basic read-only git operations require approval that isn't available here). The logic was reviewed statically and matches the described, already-verified concurrency fix; nothing in the diff looks structurally likely to fail those checks.

No blocking issues found — LGTM.

@joshiste

joshiste commented Jul 1, 2026

Copy link
Copy Markdown
Member Author

Re: the "blocks forever if a caller never reads from ch" observation — deliberately leaving it as-is here, and it's not reachable with the real callers:

  • Both SDKs pass a buffered (size 1), actively-drained channel (for range ch), and the monitor goroutine is the only thing that closes ch (right after the timeout send). So at the moment of signal <- time.Now() the reader is guaranteed still alive and a buffer slot is free — the send always completes. A never-read channel is a caller bug that none of the callers commit.
  • The tempting fix (non-blocking select { case signal <- ...: default: }) would be worse: it can silently drop the timeout notification, which is what triggers StopAction/CancelPreflight — i.e. a missed action-stop — and it would break TestHeartbeat_timeout_should_close_channel (which asserts exactly one real timeout value is delivered).
  • A non-dropping, non-blocking design requires the watchdog to own the channel buffering (hand back a receive-only channel, or add a ctx/done to abort the send) — an API/behavior change out of scope for this pure extraction.

Also confirmed the Windows build (the CI job is gated to push on main, so it runs post-merge): GOOS=windows build+vet pass for both amd64 and arm64; the package is pure sync/time/channels/zerolog with no OS-specific code.

Leaving as-is; if we want to harden the no-reader case it should be a separate, deliberate PR with its own tests.

@joshiste joshiste merged commit 3f35abb into main Jul 1, 2026
5 checks passed
@joshiste joshiste deleted the feat/extheartbeat branch July 1, 2026 13:00
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 1, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant