Skip to content

refactor(coding-agent): represent heartbeat cache validity by presence - #1743

Closed
snimu wants to merge 7 commits into
snimu/derive-connection-modelsfrom
snimu/heartbeat-cache-presence
Closed

refactor(coding-agent): represent heartbeat cache validity by presence#1743
snimu wants to merge 7 commits into
snimu/derive-connection-modelsfrom
snimu/heartbeat-cache-presence

Conversation

@snimu

@snimu snimu commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Status: fully reverted — this PR is now a no-op. A cursor finding revealed the audit premise was wrong: heartbeat_manage intentionally consumed the retained stale snapshot as the routing index for unloaded sessions, and a successful manage synchronously emits heartbeats_changed before its response returns — so collapsing the flag into presence cleared the routing index on every manage. Base behavior restored in 3fb6ee2. The PR remains open only as a link in the stack chain; merge (empty) or close and retarget #1744 to snimu/derive-connection-models.

What was wrong

"Is the cached heartbeat snapshot usable?" was answered twice in the daemon supervisor: by the snapshot's presence AND by a heartbeatSnapshotStale flag, kept aligned by paired writes (audit: duplicate sources of truth, dup-truth.md finding 6).

The fix

The flag is deleted. heartbeats_changed clears the snapshot; a successful list sets it; validity is presence. All list branches behave identically under the mapping stale=true ⇔ absent. One deliberate narrowing: heartbeat_manage no longer routes via a stale-but-present snapshot — benign, because heartbeats_changed only arrives from a live worker whose broadcast immediately triggers client list refreshes that repopulate the cache, with findWorkerForClient as the fallback; disconnected-worker recovery is unaffected (a dead socket can't emit the event). +5/−7.

How it's verified

Reviewer proved the state bijection branch-by-branch, identified and vetted the one behavioral delta (correcting the audit's "stale snapshots were never consumed" claim), and confirmed no ordering race (frames on one socket are ordered). Heartbeats suite 6/6; full CI-style suite: no branch-only failures vs stack base. Two-model implement/review loop, approved first pass.

Stacked on #1741 (test the whole stack at the leaf; merge base-first).

Note: intentionally no Linear ticket for this cleanup stack, so that check stays red.


Note

Low Risk
No functional code changes in the diff; risk is limited to process/stack bookkeeping if an empty merge is mistaken for a behavior change.

Overview
This PR is effectively empty — the diff has no code changes, and the description states the intended refactor was fully reverted in 3fb6ee2a2.

The original goal was to drop heartbeatSnapshotStale in daemon-supervisor and treat cache validity as “snapshot present vs absent,” aligned with clearing on heartbeats_changed and repopulating on a successful list. That was rolled back because heartbeat_manage still relies on a stale-but-present snapshot as the routing index for unloaded sessions; treating stale as absent would clear that index on every manage before clients could refresh.

Current behavior is unchanged: heartbeatSnapshot and heartbeatSnapshotStale remain the dual source of truth in the supervisor. The PR is kept open only for stack linkage; merge as empty or close and retarget downstream work.

Reviewed by Cursor Bugbot for commit e4e12e6. Bugbot is set up for automated code reviews on this repo. Configure here.

Linear ticket: ENG-5661
(ticket linked above)

Note

Refactor coding-agent heartbeat cache validity to use presence

The PR contains only blank line modifications. There are no functional changes to the heartbeat cache logic or any other code.

Macroscope summarized e4e12e6.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 322c572. Configure here.

Comment thread packages/coding-agent/src/modes/daemon/daemon-supervisor.ts Outdated
@snimu

snimu commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Closing: fully reverted after the routing-cache finding invalidated the audit premise (see resolved thread). The branch stays in place as history for the stack; #1744 has been retargeted to snimu/derive-connection-models.

@snimu snimu closed this Aug 26, 2026
@snimu

snimu commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Reopening: GitHub refuses base changes on PRs that are part of a stack, so #1744 cannot be retargeted away from this branch. This PR stays as a no-op link in the chain — merging it (empty diff) is the supported path, and its merge+delete will auto-retarget #1744.

@snimu snimu reopened this Aug 26, 2026
@snimu

snimu commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Closing permanently: this PR is an intentional no-op (C7 was fully reverted after a review finding invalidated the audit premise; the branch carries net-zero changes). The stack is being converted to plain PRs against main, so this placeholder is no longer needed. Ticket ENG-5661 will be cancelled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant