Skip to content

refactor: use shared extheartbeat and fix re-Start monitor leak#458

Merged
joshiste merged 1 commit into
mainfrom
refactor/use-extheartbeat
Jul 1, 2026
Merged

refactor: use shared extheartbeat and fix re-Start monitor leak#458
joshiste merged 1 commit into
mainfrom
refactor/use-extheartbeat

Conversation

@joshiste

@joshiste joshiste commented Jul 1, 2026

Copy link
Copy Markdown
Member

What

Switches action_kit_sdk to the shared extheartbeat watchdog from extension-kit (extension-kit#150, released as v1.10.7) and deletes the local action_kit_sdk/heartbeat package.

That package was a byte-identical copy of preflight-kit's, and the same concurrency bug had to be fixed in both (action-kit#457, preflight-kit#52). Sharing one implementation stops it diverging again. extension-kit was already a dependency, so no new module coupling.

Also folds in the re-Start monitor leak fix (matching preflight-kit#52): monitorHeartbeatWithCallback now Swap-and-Stops any monitor already registered for the execution id, so a repeated Start no longer leaks the previous monitor's two goroutines.

API impact

None. The local heartbeat package was SDK-internal — imported only by action_sdk.go, not by extensions. Clean removal (no shim), agreed.

Verification

  • extension-kit bumped to v1.10.7; local heartbeat package removed.
  • go build ./..., go vet ./..., go test -race ./... pass (module go/action_kit_sdk).
  • Added a goroutine-leak regression test for repeated Start (fails without the Swap fix); existing stop/heartbeat race tests still pass.

Switch action_kit_sdk to extension-kit's extheartbeat watchdog (v1.10.7)
and delete the local heartbeat package — it was a byte-identical copy of
preflight-kit's, and the same concurrency fix had to be applied to both.
Sharing one implementation stops it diverging again.

Also fold in the re-Start monitor leak fix (matching preflight-kit#52):
monitorHeartbeatWithCallback now Swap-and-Stops any monitor already
registered for the execution id, so a repeated Start no longer leaks the
previous monitor's goroutines. Adds a goroutine-leak regression test.
@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown

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


Review

  • Check previously unresolved review threads for fixes (none exist)
  • Gather context (diff, action_sdk.go, extension-kit's extheartbeat source)
  • Review new changes for bugs/concurrency/security issues
  • Post final review

Summary: Clean, well-scoped refactor. Verified the swap-and-stop fix against the actual extheartbeat source in extension-kit (fetched from steadybit/extension-kit@main):

  • extheartbeat.Notify/Monitor.Stop/RecordHeartbeat semantics match what action_sdk.go expects (idempotent Stop, non-blocking RecordHeartbeat, Stop closes the channel which unblocks the for range ch consumer goroutine in monitorHeartbeatWithCallback) — so no goroutine leak from either monitor's internal goroutine or the local consumer goroutine.
  • The heartbeatMonitors.Swap(...) + prev.Stop() in action_sdk.go:261-263 correctly closes the old monitor's ch, which lets the old consumer goroutine (go func() { for range ch { callback() } }()) exit — this is what fixes the leak on repeated Start.
  • Checked for a new race between concurrent Start (Swap) and Stop (LoadAndDelete) on the same execution id: both orderings converge safely (no double-stop, no leaked entry) because Stop() is idempotent and sync.Map.Swap/LoadAndDelete are atomic — this matches the pre-existing concurrency model, not a regression.
  • The new regression test (action_sdk_test.go:16-26) is meaningful: without the Swap-and-Stop fix, 50 iterations would leak ~98 goroutines, well outside the base+4 threshold, so this test fails deterministically on the old code and passes on the new code.
  • Confirmed no other code in the repo still imports the deleted action_kit_sdk/heartbeat package.
  • go.mod/go.sum bump to extension-kit v1.10.7 is consistent with the new extheartbeat dependency.

No bugs, race conditions, or security issues found in this change.

Note: I was unable to run go build/go vet/go test -race myself — the sandboxed environment blocks go toolchain invocations pending approval that isn't available in this automated context. The PR description states these were run and passed locally; you may want to confirm via CI. If you'd like me to be able to run these, add Bash(go build:*), Bash(go vet:*), and Bash(go test:*) to --allowedTools.

@sonarqubecloud

sonarqubecloud Bot commented Jul 1, 2026

Copy link
Copy Markdown

@joshiste joshiste merged commit a73e700 into main Jul 1, 2026
11 checks passed
@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