fix(tracking-mirror): idempotent setState with marker-based orphan adoption (#109) - #111
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR introduces a marker-based approach to prevent duplicate tracking comments in workflow runs. It embeds a hidden HTML marker ( ChangesTracking Comment Marker and Adoption Logic
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR addresses duplicate workflow tracking comments by making setState() idempotent: each tracking comment body embeds a hidden run marker and the first-touch path scans for an existing marker comment to adopt and reconciles duplicates created by retried POSTs.
Changes:
- Add a per-run hidden HTML marker to tracking-comment bodies and implement marker-based comment discovery.
- Refactor first-touch
setState()into a create-or-adopt flow with pre-scan + post-scan reconciliation and best-effort deletion of duplicates. - Add a new unit test suite covering create/adopt/reconcile scenarios for tracking-mirror.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/workflows/tracking-mirror.ts | Adds marker embedding, marker scanning, and first-touch create/adopt/reconcile logic for idempotent tracking comments. |
| test/workflows/tracking-mirror.test.ts | Adds unit tests for the new marker-based first-touch behavior and scan argument shaping. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/workflows/tracking-mirror.ts`:
- Around line 66-81: findCommentsByMarker currently fetches only one page and
relies on listComments with since (which filters by updated_at), so it can miss
markers on later pages; replace the single listComments call with
deps.octokit.paginate(deps.octokit.rest.issues.listComments, { owner:
row.target_owner, repo: row.target_repo, issue_number: row.target_number,
per_page: 100, since: row.created_at.toISOString() }) and iterate pages/items,
checking each comment.body for runMarker(row.id) and verifying
comment.created_at is >= row.created_at.toISOString(); stop pagination as soon
as you find a match (return the id/created_at) to avoid extra work, and
otherwise return an empty array—update the function findCommentsByMarker to use
this pagination and early-exit logic.
- Around line 78-80: The filtered comment list needs an explicit deterministic
sort so "oldest wins" is stable; after the filter and before the map in the
return expression in tracking-mirror.ts, sort resp.data entries by numeric
created_at (ascending) and use numeric id (ascending) as a tiebreaker so
matches[0] remains the oldest; update the pipeline that currently ends with
.filter(...).map(...) to insert a .sort(...) step that compares created_at
(parse to timestamp) then id (parse to integer) before mapping to { id,
created_at } to ensure deterministic behavior for the code paths that depend on
matches[0].
- Around line 178-184: The new direct GitHub writes in tracking-mirror.ts
(octokit.rest.issues.createComment, updateComment, deleteComment) send
humanMessage directly and bypass
src/utils/github-output-guard.ts:safePostToGitHub/reductSecrets; change these
calls to route through safePostToGitHub (or the module's approved wrapper) so
the body is passed to redactSecrets and the optional LLM scanner before calling
octokit; for createComment and updateComment pass the humanMessage/body via
safePostToGitHub and for deleteComment use the same wrapper or a no-body
safePostToGitHub invocation so all git writes in the file go through the
chokepoint.
In `@test/workflows/tracking-mirror.test.ts`:
- Around line 11-17: Add a test covering the lost-CAS path by setting
mockReservation = { won: false, trackingCommentId: <oldest_id> } and seeding two
pre-scan marker rows so createOrAdoptTrackingComment /
tryAdoptExistingMarkerComment run the branch where another process reserved a
tracking_comment_id; assert that the local canonical winner is not deleted and
that updateComment is called with reservation.trackingCommentId (the other
process's id) rather than a local id, and use the existing mocks
(mockReservation, findByIdMock, tryReserveMock, mergeStateMock) to simulate the
scenario.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: be196eec-ecb8-405d-a71f-0c365f91e9f6
📒 Files selected for processing (2)
src/workflows/tracking-mirror.tstest/workflows/tracking-mirror.test.ts
|
🎉 This PR is included in version 1.11.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
Fixes #109 — duplicate tracking comments on issue/PR threads. Root cause:
octokitv5.0.5 bundles@octokit/plugin-retry, which silently retriescreateCommentPOSTs on transient 5xx. When the upstream commits the comment but times out before responding, the retry posts a second identical comment that the existing CAS reservation cannot reconcile (CAS only protects the in-process race, not server-side duplicates from a single caller).This PR makes
setState()idempotent by embedding a hidden HTML-comment marker (<!-- workflow-run:{id} -->) in every tracking-comment body, then adopting any pre-existing marker comment on first touch and reconciling duplicates after create. The DB row remains the authority for the canonicaltracking_comment_idvia the unchanged CAS reservation.Diagram
flowchart TD subgraph BeforeAndAfter["Before vs After"] direction TB StartA["setState first touch<br/>tracking_comment_id NULL"]:::ctx PostA["POST createComment"]:::ctx RetryA["octokit plugin-retry<br/>silent duplicate POST<br/>on transient 5xx"]:::bug DupeA["2+ comments<br/>only one CAS-reserved<br/>extras are orphan duplicates"]:::bug StartA --> PostA --> RetryA --> DupeA StartB["setState first touch<br/>tracking_comment_id NULL"]:::ctx PreScanB["pre-scan: findCommentsByMarker<br/>since=row.created_at"]:::fix AdoptB["orphan found<br/>adopt + delete extras<br/>skip POST"]:::fix PostB["no orphan<br/>POST createComment<br/>marker embedded in body"]:::fix PostScanB["post-scan: re-list<br/>oldest wins<br/>delete loser duplicates"]:::fix ReserveB["CAS tryReserveTrackingCommentId<br/>row authoritative"]:::fix StartB --> PreScanB PreScanB -->|hit| AdoptB PreScanB -->|miss| PostB --> PostScanB --> ReserveB AdoptB --> ReserveB end classDef ctx fill:#1f2937,stroke:#9ca3af,color:#f9fafb classDef bug fill:#7f1d1d,stroke:#fca5a5,color:#fef2f2 classDef fix fill:#14532d,stroke:#86efac,color:#f0fdf4Changes
src/workflows/tracking-mirror.ts:<!-- workflow-run:{id} -->marker in every comment body viarunMarker()+renderCommentBody().findCommentsByMarker()—listCommentsscoped bysince=row.created_at, filters bodies containing the run marker.createOrAdoptTrackingComment(): pre-scan adopt → POST create → post-scan reconcile (oldest wins, delete extras best-effort) → CAS reservation → re-render against freshest row.tryAdoptExistingMarkerComment()for the pre-scan branch (recovers from pod crash between create and CAS, or octokit retry that already committed before the previous run died).test/workflows/tracking-mirror.test.ts(new, 7 tests):listCommentscall-shape regression lock (since+per_page, nodirection/sort)tracking_comment_idalready setRelated Issues
Test plan
bun run typecheckclean,bun run lintclean (0 errors),bun test test/workflows/tracking-mirror.test.ts7/7 passtest/workflows/tracking-mirror.test.tsmainSummary by CodeRabbit
Bug Fixes
Tests