Skip to content

refactor(heartbeat): document scheduled_retry lock semantic at the predicate site - #1334

Merged
allyblockcast[bot] merged 2 commits into
masterfrom
blo25517-lock-semantic-comment
Aug 12, 2026
Merged

refactor(heartbeat): document scheduled_retry lock semantic at the predicate site#1334
allyblockcast[bot] merged 2 commits into
masterfrom
blo25517-lock-semantic-comment

Conversation

@allyblockcast

@allyblockcast allyblockcast Bot commented Aug 12, 2026

Copy link
Copy Markdown

Thinking Path

  • Paperclip is the open source app people use to manage AI agents for work
  • Heartbeat's scheduled-retry path decides whether a parked (scheduled_retry) run keeps holding an issue's execution lock (issues.executionRunId)
  • Two commits, 8446c1011 and 76304affd, landed ~15 minutes apart with contradictory answers to that question, each green on its own base, and merged red together — this broke master (BLO-25517)
  • The semantic conflict was already reconciled and shipped test-only in commit 8f55b3b7, but the reconciled rule (narrower predicate for the OpenCode-recovery reasons, not a global lock-retention change) was never written down at the function that implements it, only near an unrelated code path
  • This pull request adds that doc comment directly above requiresIssueExecutionRetryLock / requiresInProgressIssueRetry in heartbeat.ts, naming both conflicting commits and explaining why session_unavailable / zero_token_session_reset still appear in the lock-conflict predicate without holding the lock while parked
  • The benefit is a future edit to either commit's territory (OpenCode recovery vs. general retry-lock binding) has the tradeoff and the two SHAs to check, right where the logic lives, instead of re-deriving it from test names

Linked Issues or Issue Description

Refs: BLO-25517

What Changed

  • Added a doc comment above requiresIssueExecutionRetryLock explaining the 8446c1011 (default: scheduled_retry does not hold the issue lock) vs. 76304affd (PR [codex] Recover unavailable OpenCode sessions (BLO-18012) #1245, BLO-18012 OpenCode recovery) tension and how the current code reconciles it: the lock guard from 76304affd is enforced only at scheduling-time claim-conflict checking (issueExecutionRetryLockAvailable), not by holding the lock while parked.
  • Added a short pointer comment above requiresInProgressIssueRetry cross-referencing the same explanation for why session_unavailable/zero_token_session_reset are excluded there.
  • No behavior change — comments only.

Verification

  • Doc-only change; no logic touched. Confirmed via git worktree add against origin/master (head 12c1e0eb2) that the edited functions retain identical bodies/logic, only comments added.
  • The behavior this documents is already covered by server/src/__tests__/heartbeat-retry-scheduling.test.ts ("schedules %s retries for an assigned todo issue while leaving its execution lock free until claim", session_unavailable / zero_token_session_reset cases) and heartbeat-finalize-cancelled-skip-dispatch.test.ts, both currently green — verified via 7+ consecutive success General tests (server 3/4) merge-group runs on current master (e.g. run 31590276027).
  • This PR's own merge-group General tests (server 3/4) run is the direct verifying signal for BLO-25517's last open acceptance-criteria item (lock semantic documented at the predicate site).

Risks

  • Low risk: comment-only change, no production logic modified.

Model Used

  • Claude Sonnet 5 (claude-sonnet-5), Anthropic. Standard reasoning mode, tool use (Bash, Read, Edit) for repo investigation and git worktree verification. No extended thinking mode used.

Checklist

  • I have included a thinking path that traces from project context to this change
  • I have specified the model used (with version and capability details)
  • I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
  • I have searched GitHub for duplicate or related PRs and linked them above
  • I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
  • I have run tests locally and they pass (comment-only change; relying on CI + prior verified green runs, see Verification)
  • I have added or updated tests where applicable (no test-relevant behavior changed)
  • If this change affects the UI, I have included before/after screenshots (N/A)
  • I have updated relevant documentation to reflect my changes
  • I have considered and documented any risks above
  • All Paperclip CI gates are green (pending this PR's own run)
  • Greptile is 5/5 with no open P2s, recommendations, or follow-ups
  • I will address all Greptile and reviewer comments before requesting merge

…ate site

Names the two commits that landed within ~15 minutes of each other with
contradictory execution-lock semantics for a scheduled_retry run
(8446c10, 76304af), and the narrower predicate that reconciles them,
directly at requiresIssueExecutionRetryLock instead of only near the
unrelated BLO-18106 code path.

Refs: BLO-25517

Co-Authored-By: Paperclip <noreply@paperclip.ing>
@allyblockcast

allyblockcast Bot commented Aug 12, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-18012
🔗 Paperclip issue: BLO-25517

1 similar comment
@allyblockcast

allyblockcast Bot commented Aug 12, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-18012
🔗 Paperclip issue: BLO-25517

@allyblockcast

allyblockcast Bot commented Aug 12, 2026

Copy link
Copy Markdown
Author

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

  • PR is titled docs: but includes source code changes (server/src/services/heartbeat.ts). Please retitle as fix:, feat:, or refactor: so the right gates run, or remove the source code changes if this is genuinely a docs: PR.

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

@allyblockcast allyblockcast Bot changed the title docs(heartbeat): document scheduled_retry lock semantic at the predicate site refactor(heartbeat): document scheduled_retry lock semantic at the predicate site Aug 12, 2026
@allyblockcast

allyblockcast Bot commented Aug 12, 2026

Copy link
Copy Markdown
Author

Confirmed — check-pr-test-coverage.mjs mismatch check is path-based and doesn't distinguish comment-only edits from behavior changes, so touching server/src/services/heartbeat.ts under a docs: prefix correctly trips it. Retitled to refactor(heartbeat): ..., which is in SKIP_TEST_PREFIXES (no test-file requirement, matches this being a comment-only change) but not in NO_SOURCE_CODE_PREFIXES (won't false-flag for touching a .ts file). No commit changes needed — this was a title-gate mismatch, not a diff issue.

Title mismatch (docs: with source file changes) flagged by
commitperclip's test-coverage gate — fixed by retitling PR #1334 to
refactor(heartbeat): ..., which requires a new commit (title-only
edits don't fire pull_request_target's synchronize trigger).
@allyblockcast
allyblockcast Bot added this pull request to the merge queue Aug 12, 2026
Merged via the queue into master with commit 7fc8b9e Aug 12, 2026
19 checks passed
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.

0 participants