Keep every raid kill awaiting a difficulty - #13
Merged
Conversation
A single pending slot silently discarded the first kill when two raid targets died before a difficulty was confirmed, which is exactly when a raid confirms several at once: the second kill overwrote the first, and only the second was ever credited. This fix existed on this fork before the upstream sync and was parked during the PR itsspin#72 merge, because upstream had meanwhile extended the same slot with the kill's occurred_at, zone, character and evidence. Reverting to the old target -> seconds map would have dropped that context, so the kills are now held as an ordered target -> PendingRaidKill map that carries all of it. Confirming a difficulty credits every waiting kill with its own context, not whichever fight happened to be current when the confirmation arrived. The ordering keeps protocol v1 honest: pendingRaidTarget still names the first kill still awaiting confirmation, exactly as it always did, and pendingRaidTargets is added beside it for renderers that want the full list. The test kills two raid targets ten minutes apart and asserts each is credited with its own kill time and evidence. Verified to fail when the pending kills share one context, which is the bug being fixed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The new test pinned both kill times as UTC strings, which only held in the timezone it was written in: the log lines carry no zone, so the stored instant follows whatever machine runs the suite. CI is UTC and this desktop is UTC-4, so the runner read 20:00:01Z where the test expected 00:00:01Z. It asserts the ten minutes between the two kills instead, which is the property that actually distinguishes per-kill context from a shared slot and holds in any zone. Verified passing under EDT, UTC and Asia/Tokyo, and still failing when the pending kills share one context. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Re-applies the multi-kill fix that was parked during the upstream sync (#12), now ported onto upstream's structure.
The bug
A single pending slot silently discarded the first kill when two raid targets died before a difficulty was confirmed — which is exactly when a raid confirms several at once. The second kill overwrote the first, and only the second was ever credited.
This is a cross-platform engine bug that predates the Linux branch, and upstream still has it. Worth sending them separately.
Why it needed porting rather than restoring
The original fix held a plain
target -> secondsmap. Upstream's itsspin#72 meanwhile extended the same single slot with the kill'soccurred_at,zone,characterandevidence, because a confirmation can arrive minutes after the kill, by which time the session has moved on.Restoring the old map would have silently dropped that context. So the pending kills are now an ordered
target -> PendingRaidKillmap carrying all of it, and confirming a difficulty credits each waiting kill with its own context rather than whichever fight was current when the confirmation arrived.Protocol compatibility
Insertion order keeps protocol v1 honest:
pendingRaidTargetstill names the first kill awaiting confirmation, exactly as before.pendingRaidTargetsis added beside it, and the renderer pluralises only when there is more than one.Verification
The test kills two raid targets ten minutes apart, confirms one difficulty, and asserts each is credited with its own kill time and evidence.
That test was verified to fail when the pending kills share one context — mutating the engine to the shared-slot behaviour makes Nagafen inherit Vox's kill time:
so the assertions detect the bug being fixed rather than passing vacuously.
ALL PASStest_desktop_worker: 26 passingtsc --noEmit(renderer): clean🤖 Generated with Claude Code