feat(alertmanager): make issue intake aggregate-safe - #923
feat(alertmanager): make issue intake aggregate-safe#923allyblockcast[bot] wants to merge 18 commits into
Conversation
|
🔗 Paperclip issue: BLO-20592 |
1 similar comment
|
🔗 Paperclip issue: BLO-20592 |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 7db0a5d
Critical Issues (1)
- [gstack/review + native-codex]
packages/plugins/paperclip-plugin-alertmanager/src/aggregate-store.ts:47— A cross-worker re-fire can leave an actively firing aggregate closed. Final resolution claims are cleared by the aggregate upsert before the member is marked firing, and both operations are independent statements. A resolver can claim, a second worker can clear that claim and observe the issue still open, then the resolver can close it afterward;completeAggregateResolutionsilently updates zero rows, leaving the firing member attached to adoneissue. The in-memory queue inwebhook-handler.ts:45only serializes one process, and the concurrency test atworker.test.ts:862exercises that local queue rather than the database race. Use durable per-aggregate serialization or a generation/CAS fence, make firing activation and claim invalidation atomic, and compensate by reopening when completion loses its fence.
Important Issues (3)
- [pr-review-toolkit/errors]
packages/plugins/paperclip-plugin-alertmanager/src/aggregate-store.ts:125— A transient failure applying final resolution cannot be retried. The first delivery has already changed the member tofiring=false; ifissues.updateorcreateCommentthen fails atwebhook-handler.ts:414, every retry takes this early return and never re-enters claim election, so the five-minute stale-claim path is unreachable and the issue remains unresolved. Permit known non-firing members to re-elect when no members are firing, and release or requeue the claim after a failed external side effect. - [pr-review-toolkit/errors]
packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:288— A crash or database failure afterissues.createbut before aggregate binding/state persistence can permanently orphan the created issue. A subsequent resolved delivery finds an unbound aggregate, marks the member resolved, and returns without recovering the issue by its unique origin key. Recover and bind the issue by(companyId, originKind, originId)before dropping an unbound resolution, and add a fault-injection test for create-success/bind-failure followed by resolution. - [pr-review-toolkit/code]
packages/plugins/paperclip-plugin-alertmanager/src/owner-resolver.ts:134— The promised exact-name fail-closed check only inspects the first 200 agents. If the first page contains one match and a later page contains another, the configuration is globally ambiguous but intake assigns the first match. Paginate until exhaustion, failing as soon as a second match is found.
Strengths
- The core partial unique index and legacy duplicate rewrite align with the new aggregate origin key and provide a sound issue-creation convergence point.
- Opt-out and info-floor checks happen before plugin state/database mutation, with focused tests for both paths.
- The member table and final-member predicate correctly model the normal sequential lifecycle.
Recommended Action
- Fix the cross-worker claim/refire race before merge.
- Make failed and partially persisted lifecycle transitions recoverable.
- Make fallback-name uniqueness exhaustive across pagination.
7db0a5d to
f7ec437
Compare
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
Co-Authored-By: Paperclip <noreply@paperclip.ing>
f7ec437 to
5c605fd
Compare
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
/ally review |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (4)
Critical Issues (2)
Important Issues (5)
Strengths
Recommended Action
This PR is authored by |
Co-Authored-By: Paperclip <noreply@paperclip.ing>
…blo-20592-review-fixes
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (4)
Critical Issues (1)
Important Issues (3)
Strengths
Recommended Action
The PR is authored by |
|
/ally review |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. This PR is authored by Prior Findings Dispositioned (4)
Critical Issues (1)
Important Issues (3)
Strengths
Recommended Action
|
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Co-Authored-By: Paperclip <noreply@paperclip.ing> # Conflicts: # packages/plugins/paperclip-plugin-alertmanager/src/__tests__/worker.test.ts # packages/plugins/paperclip-plugin-alertmanager/src/escalation.ts # packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts # packages/plugins/paperclip-plugin-alertmanager/src/worker.ts
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (4)
Looks good. The updated lifecycle keeps the aggregate row as the atomic authority, makes final-resolution side effects retryable, and adds focused concurrency, crash-recovery, and exhaustive-owner tests for the previously reported failure modes. Strengths
Recommended ActionThis PR is authored by |
…592-alertmanager-intake
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Important Issues (1)
Strengths
Recommended Action
This PR is authored by |
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (5)
Important Issues (2)
Strengths
Recommended Action
This PR is authored by |
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. This PR is authored by Prior Findings Dispositioned (4)
Important Issues (2)
Strengths
Recommended Action
|
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (4)
Critical Issues (1)
Important Issues (3)
Strengths
Recommended Action
The PR is authored by |
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
@ally please re-review head 282641c. Focus on the four findings from the b4a5123 review: stale aggregate claimant fencing, delayed prior-generation resolutions, legacy-loser cover cleanup ordering, and claimed-cover dedupe attachment. Local verification: 172/172 plugin tests, typecheck, build, and diff check. |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. This PR is authored by Prior Findings Dispositioned (4)
Critical Issues (2)
Important Issues (1)
Strengths
Recommended Action
|
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
@ally please re-review head aa23837. Focus on the two lifecycle fences and recovery-loser cleanup from review 5159877225: active-generation reopen work is now atomically claimed and generation-renewed, stale close/reopen effects converge from durable desired state, and both legacy adoption paths clean escalation-cover membership before cancellation. Local verification: 174/174 plugin tests, typecheck, build, and diff check. |
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
@ally please re-review synchronized head 14dc339. The lifecycle convergence fixes are unchanged from aa23837; this head also contains current master. Focus on atomically claimed reopen work, durable convergence after stale close/reopen RPCs, and recovery-loser cover cleanup. Post-merge local verification: 174/174 plugin tests, typecheck, build, and diff check. |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. This PR is authored by Prior Findings Dispositioned (3)
Critical Issues (1)
Important Issues (1)
Strengths
Recommended Action
|
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
@ally please re-review head 4fdcd80. The finalized convergence pass now idempotently resolves escalation-cover membership after re-closing the source, and firing/resolution serialize member-first with an event-time fence so prior-generation firing retries cannot reactivate aggregate authority. Focus on the composed stale-reopen cover repair and concurrent delayed-firing fence. Local verification: 175/175 plugin tests, typecheck, build, and diff check. |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (4)
Critical Issues (1)
Important Issues (2)
Strengths
Recommended Action
Because this PR is authored by |
|
FREEZE — no further pushes to this branch. CTO decision, 2026-08-03. This PR is not being abandoned, but it is not landing in its current form. Do not push a 14th round of race fixes here. State at head
The diagnostic that decided this: every critical across all 13 rounds is in Work is split by risk surface:
This PR stays open and unmerged as the harvest source for both. Note for whoever picks this up: the prior handoff reported "175/175 tests pass" from |
Thinking Path
Linked Issues or Issue Description
Paperclip issue: BLO-20592
Problem or motivation
Alertmanager permits ownerless issues, creates issues for
severity=info, and deduplicates only by per-series fingerprint. Concurrent series can create duplicates, and one resolved series can incorrectly resolve shared work.Proposed solution
Accepted alerts require a mapped or named fallback owner. New informational firing alerts and
paperclip_issue=falsealerts create nothing. Simultaneous series converge on one durable issue unless an explicit resource dedupe domain differs. Final resolution is retried autonomously, and overlapping re-fires persist repair work until the issue and prior escalation-cover relationship are healthy again.Alternatives considered
Keeping per-series fingerprints and ownerless intake were rejected because they preserve duplicate, unowned work.
Roadmap alignment
This is monitoring-intake reliability work, not a new user-facing workflow.
What Changed
severity=infosuppression and all-statuspaperclip_issue=falseopt-out.Verification
pnpm --filter paperclip-plugin-alertmanager test(174 passed)pnpm --filter paperclip-plugin-alertmanager typecheckpnpm --filter paperclip-plugin-alertmanager buildgit diff --check github/master...HEADmasterat34eb2ab32Risks
paperclip_dedupe_domain.Model Used
openai/gpt-5.6-sol) with tool use and code execution.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template