charles/claude-hooks

Fork 0

fix(webhook): dedupe assign-events to prevent pivot double-dispatch #93

Merged

code-lead merged 2 commits from dev/92 into main

2026-04-19 17:31:38 +00:00

dev commented

2026-04-19 17:19:29 +00:00

Collaborator

Summary

Adds a 30 s dedup map in handleIssueAssigned keyed by ${repo}#${issue}@${agent} to drop duplicate assign events for the same tuple
When a different agent fires for the same issue within the TTL window, the previous agent's still-queued task is cancelled; if already running, both continue (logged)
New test file webhook-assign-dedup.test.ts covers all three acceptance-criteria scenarios: same-agent dedup, cross-agent queue cancellation, and cross-agent running-task pass-through

Closes #92

## Summary - Adds a 30 s dedup map in `handleIssueAssigned` keyed by `${repo}#${issue}@${agent}` to drop duplicate assign events for the same tuple - When a *different* agent fires for the same issue within the TTL window, the previous agent's still-queued task is cancelled; if already running, both continue (logged) - New test file `webhook-assign-dedup.test.ts` covers all three acceptance-criteria scenarios: same-agent dedup, cross-agent queue cancellation, and cross-agent running-task pass-through Closes #92

dev added 1 commit

2026-04-19 17:19:29 +00:00

fix(webhook): dedupe assign-events to prevent pivot double-dispatch

qa / qa (pull_request) Successful in 2m36s

Details

qa / dockerfile (pull_request) Successful in 9s

Details

dedb4fd88e

Introduces a 30 s dedup guard in handleIssueAssigned keyed by
`${repo}#${issue}@${agent}`. Duplicate fires for the same tuple are
dropped silently. When a different agent fires for the same issue
within the TTL window, the previous agent's still-queued task is
removed from the queue; if that task is already running, both agents
continue (logged).

Closes #92

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

dev requested review from reviewer

2026-04-19 17:22:17 +00:00

reviewer requested changes

2026-04-19 17:24:15 +00:00

Dismissed

reviewer left a comment

Review

CI is green ✅. Logic is correct and all three acceptance-criteria scenarios are covered by the new tests. One real issue before merge:

`_lastIssueDispatch` is an unbounded `Map` — memory leak

File: src/webhook-handlers.ts

const _lastIssueDispatch = new Map<string, LastDispatch>();

This plain Map has no eviction: every issue that ever triggers handleIssueAssigned adds an entry that is never removed in production (resetAssignDedup() only clears it in tests). In a long-running service this leaks memory without bound.

Everywhere else in this file the same pattern uses BoundedMap — _assignDedup uses it, _postMergeRebaseDispatched uses it. This map should too.

Fix: replace the plain Map with a BoundedMap using the same TTL:

const _lastIssueDispatch = new BoundedMap<string, LastDispatch>(500, ASSIGN_DEDUP_TTL_MS);

No other changes needed — the TTL guard inside cancelStaleIssueTask remains correct (BoundedMap may not evict on reads, so the manual check is good belt-and-suspenders).

Note (non-blocking)

The acceptance criteria in issue #92 names webhook-handlers.test.ts as the target test file, but the tests landed in webhook-assign-dedup.test.ts. The scenarios are all covered and Bun picks up both files, so this is functionally fine — just a minor deviation from the spec wording.

## Review CI is green ✅. Logic is correct and all three acceptance-criteria scenarios are covered by the new tests. One real issue before merge: --- ### `_lastIssueDispatch` is an unbounded `Map` — memory leak **File:** `src/webhook-handlers.ts` ```typescript const _lastIssueDispatch = new Map<string, LastDispatch>(); ``` This plain `Map` has no eviction: every issue that ever triggers `handleIssueAssigned` adds an entry that is **never removed** in production (`resetAssignDedup()` only clears it in tests). In a long-running service this leaks memory without bound. Everywhere else in this file the same pattern uses `BoundedMap` — `_assignDedup` uses it, `_postMergeRebaseDispatched` uses it. This map should too. **Fix:** replace the plain `Map` with a `BoundedMap` using the same TTL: ```typescript const _lastIssueDispatch = new BoundedMap<string, LastDispatch>(500, ASSIGN_DEDUP_TTL_MS); ``` No other changes needed — the TTL guard inside `cancelStaleIssueTask` remains correct (BoundedMap may not evict on reads, so the manual check is good belt-and-suspenders). --- ### Note (non-blocking) The acceptance criteria in issue #92 names `webhook-handlers.test.ts` as the target test file, but the tests landed in `webhook-assign-dedup.test.ts`. The scenarios are all covered and Bun picks up both files, so this is functionally fine — just a minor deviation from the spec wording.

dev added 1 commit

2026-04-19 17:25:31 +00:00

fix(webhook): use BoundedMap for _lastIssueDispatch to prevent memory leak

qa / qa (pull_request) Successful in 2m38s

Details

qa / dockerfile (pull_request) Successful in 9s

Details

96d9769ecb

Replaces the unbounded plain Map with BoundedMap (cap 500, TTL 30 s),
consistent with _assignDedup and _postMergeRebaseDispatched. Also updates
resetAssignDedup() to use the iterate-and-delete pattern since BoundedMap
has no .clear() method.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

dev commented

2026-04-19 17:25:43 +00:00

Author

Collaborator

Fixed in 96d9769. Replaced the plain Map with BoundedMap<string, LastDispatch>(500, ASSIGN_DEDUP_TTL_MS) — consistent with _assignDedup and _postMergeRebaseDispatched. Also updated resetAssignDedup() to use the iterate-and-delete pattern instead of .clear() since BoundedMap doesn't expose that method.

Non-blocking note acknowledged — test file name deviates from the spec wording but all scenarios are covered and Bun picks up both files.

Fixed in 96d9769. Replaced the plain `Map` with `BoundedMap<string, LastDispatch>(500, ASSIGN_DEDUP_TTL_MS)` — consistent with `_assignDedup` and `_postMergeRebaseDispatched`. Also updated `resetAssignDedup()` to use the iterate-and-delete pattern instead of `.clear()` since `BoundedMap` doesn't expose that method. Non-blocking note acknowledged — test file name deviates from the spec wording but all scenarios are covered and Bun picks up both files.

dev requested review from reviewer

2026-04-19 17:28:22 +00:00

reviewer approved these changes

2026-04-19 17:30:50 +00:00

reviewer left a comment

Review: APPROVED ✅

CI is green (run #1623, 2m47s, success). All three acceptance criteria from issue #92 are met. Code is correct and clean.

Acceptance criteria verification

Criterion	Status
Dedupe map in `webhook-handlers.ts` keyed by `${repo}#${issue}@${agent}`; TTL 30 s	✅ `_assignDedup` BoundedMap in `webhook-handlers.ts`
`handleIssueAssigned` rejects dispatch if tuple already in map within TTL	✅ `isDupAssign()` guard, returns `null`
Different-agent within TTL: cancel queued task; if running, log and let both run	✅ `cancelStaleIssueTask()` splices from `prevWorker.queue`; else logs
Existing behaviour for single clean assignment preserved	✅ Tested in scenario 4

Test coverage

All five tests pass and cover every scenario specified in the issue: same-agent dedup, cross-agent queue cancellation, cross-agent running-task pass-through, single clean assignment, and unconfigured-agent no-op. Tests are in webhook-assign-dedup.test.ts (a dedicated file is a fine choice; the issue reference to webhook-handlers.test.ts was directional, not prescriptive).

Memory safety

The final commit switches both _assignDedup and _lastIssueDispatch from plain Map to BoundedMap(500, TTL), which caps entries at 500 and auto-evicts stale ones on every write. The manual TTL checks in isDupAssign and cancelStaleIssueTask are correct belt-and-suspenders guards for lazy-eviction scenarios.

Minor observation (non-blocking)

resetAssignDedup() clears both maps by iterating with for...of and calling .delete(k). This is safe in JS, but if BoundedMap exposes a .clear() method it would be simpler. Not blocking — test-only code, CI is green.

No logic bugs, no safety issues, no scope creep. Ship it.

## Review: APPROVED ✅ CI is green (run #1623, 2m47s, `success`). All three acceptance criteria from issue #92 are met. Code is correct and clean. ### Acceptance criteria verification | Criterion | Status | |---|---| | Dedupe map in `webhook-handlers.ts` keyed by `${repo}#${issue}@${agent}`; TTL 30 s | ✅ `_assignDedup` BoundedMap in `webhook-handlers.ts` | | `handleIssueAssigned` rejects dispatch if tuple already in map within TTL | ✅ `isDupAssign()` guard, returns `null` | | Different-agent within TTL: cancel queued task; if running, log and let both run | ✅ `cancelStaleIssueTask()` splices from `prevWorker.queue`; else logs | | Existing behaviour for single clean assignment preserved | ✅ Tested in scenario 4 | ### Test coverage All five tests pass and cover every scenario specified in the issue: same-agent dedup, cross-agent queue cancellation, cross-agent running-task pass-through, single clean assignment, and unconfigured-agent no-op. Tests are in `webhook-assign-dedup.test.ts` (a dedicated file is a fine choice; the issue reference to `webhook-handlers.test.ts` was directional, not prescriptive). ### Memory safety The final commit switches both `_assignDedup` and `_lastIssueDispatch` from plain `Map` to `BoundedMap(500, TTL)`, which caps entries at 500 and auto-evicts stale ones on every write. The manual TTL checks in `isDupAssign` and `cancelStaleIssueTask` are correct belt-and-suspenders guards for lazy-eviction scenarios. ### Minor observation (non-blocking) `resetAssignDedup()` clears both maps by iterating with `for...of` and calling `.delete(k)`. This is safe in JS, but if `BoundedMap` exposes a `.clear()` method it would be simpler. Not blocking — test-only code, CI is green. No logic bugs, no safety issues, no scope creep. Ship it.