fix(effects): use Foundry core duration expiry - #144
Conversation
DimitroffVodka
left a comment
There was a problem hiding this comment.
Review: NEEDS-FIXES at 739d04dcc81f451a90f8ec79a6f2b8df4f6f9521
Do not merge this head yet. The exact PR tree passes 1,668 tests and npm run verify (0 errors, 1,065 warnings), matching the green CI. The missing cases below still reproduce.
Standards / sibling-path consistency
No hard documented-standard violations found.
- P2 — Owner and non-owner applications use different clocks.
scripts/effects/BreakOnDamageSD.mjs:125–132creates the source item directly, bypassing the new normalization inscripts/shared/combat-socket.mjs:581–585, which its non-owner branch calls. Measured through both publicapplySpellEffectbranches: identical source data retained legacyrounds: 2, start: nullfor the owner, but received canonical rounds,turnStart, and combat/time anchors for the non-owner. Reuse the timing helper in the owner branch and test parity. This is an incomplete shared-path fix, not a request for a new abstraction.
Spec / correctness
-
P1 — Concurrent expiry corrupts another spell's tracking. The new hook's lock is per cast (
scripts/effects/duration-spell.mjs:622–632), whileendDurationSpellcaptures an index in the caster's shared array before awaiting deletion (:322–331) and later splices that stale index (:535–537). Measured: with A/B expiring and C still active, starting both handlers and completing A's deletion first left B tracked, C untracked, and C's Effect item still present. The probe included the realhandleEffectDeletedcleanup. Foundry's hooks do not await listeners, andgetFlagreturns the stored array, so this is a reachable race. Serialize the caster's tracker mutation and remove by identity from fresh state, rather than trusting a pre-await index. -
P2 — One expired item deletes still-running siblings.
handleDurationEffectUpdateends the entire cast atscripts/effects/duration-spell.mjs:614–628;endDurationSpellthen deletes every linked item at:334–379. Issue #140 calls for reacting to expiry for that target. Measured: expiring a short linked item also deleted a sibling with 12 seconds remaining, then cleared the cast. Delete/unlink the expired item without prematurely ending independently timed siblings; define and test when whole-cast cleanup should occur. -
P2 — A late target extends an existing cast. Both
scripts/shared/combat-socket.mjs:581–585and the direct fallback atscripts/effects/duration-spell.mjs:1053–1057stamp the current time/turn and full source duration instead of the original cast's anchor. Once linked,:719–721bypasses the original expiry. Measured throughstartDurationSpell→addTargetToDurationSpell→ combat update, using the installed Foundry combat-duration calculation: a two-round cast at round 3 was due at round 5; its first target entered at round 4, still had one round remaining at round 5, and kept the tracker alive. Preserve the cast-time timing anchor for late entrants in both paths. -
P2 — Round-zero casts still create competing clocks.
scripts/shared/duration-basis.mjs:57–63treats an unstarted encounter as outside combat, but:142–144treats the same encounter as round-based. Measured: the same two-round cast at round 0 produces an Active Effect lasting 12 seconds and tracker expiry at round 3. With Shadowdark's zero combat time increments, linked and unlinked versions can end differently. Apply one explicit unstarted-combat rule across effect, tracker, and summon timing; add a cross-helper regression. -
P2 — Summon cleanup can precede the linked effect's expiry. Code-read, conditional on a timed Effect item being linked to the summoning cast:
scripts/combat/damage-card-actions.mjs:893–898still registers a separate whole-round deadline, andscripts/combat/combat-settings-app.mjs:294–329,347–350deletes tokens at the round boundary. The new effect timing is anchored to a specific combatant'sturnStart. For a mid-round cast, that summon path can delete tokens before the core expiry event. Make linked summon cleanup use the same authoritative lifetime; retain legacy fallback only where no core clock exists.
Evidence: five in-memory Node probes exercised the pinned modules; document persistence/socket transport were mocked. The late-target probe also invoked the installed Foundry _prepareCombatBasedDuration method. No live-world mutation was performed, and these probes are not a substitute for a live lifecycle regression after fixes. The summon finding is explicitly code-read rather than live-reproduced.
Worst case: an unrelated active spell loses tracking while its Effect item remains, so cleanup and the tracker no longer describe the same state.
Verdict: NEEDS-FIXES — no merge consent for this head. No repository code was changed.
Route duration registry writes through the elected GM and serialize per-caster mutations without holding the queue over document deletion. Keep independent target effects, late-entry clocks and duration overrides intact. Preserve the originating scene across relays and clean both persisted and cached summon expiry tracking. Cover concurrent expiry, owner/socket parity, authority checks, late entry, per-target damage expiry and off-canvas synthetic targets with regressions.
DimitroffVodka
left a comment
There was a problem hiding this comment.
Fixes delivered and verified
Ready for merge at 7508d6a. This supersedes the NEEDS-FIXES verdict for 739d04d.
Resolved
- Concurrent cast cleanup no longer removes unrelated registry entries; owner and secondary-GM mutations reach the elected GM's per-caster queue.
- Independently timed target effects survive sibling expiry; an expiring target cannot receive an extra damage tick because another target is still running.
- Initial/late and owner/socket application paths preserve the original cast anchor and configured duration overrides. Legacy add/remove/end selectors consistently choose the latest cast.
- Core-timed summons defer to their linked effect; cleanup updates both persisted and cached expiry tracking.
- Relayed target operations and cleanup retain the originating scene, including off-canvas synthetic actors, regions and summons.
Verification
npm run test:all: 1,694 tests plus 2 roller regressions passed.npm run verify: passed, 0 lint errors / 1,065 repository warnings. Binding, import, named-export and script-path gates passed; flag and registration snapshots regenerated.- Foundry 14.368 candidate-module probes passed for concurrent expiry, two-GM serialization, late-target timing, and off-canvas add/remove/re-add/end. The latter used real socketlib requests between two clients and real synthetic actor/region/token documents. Test hooks, handlers and disposable documents were restored/removed. This was structured runtime verification, not a full visual gameplay run.
- Exact-head GitHub
verify-and-testpassed: https://github.com/DimitroffVodka/shadowdark-extras/actions/runs/35343425247
The corrective worktree is clean. Unrelated changes in the primary checkout were left alone. Not merged.
Closes #140
Summary
duration+ fullstartdata, with explicitturnStartexpiry during combat and seconds outside combatduration.expiredupdate while retaining the existing cleanup path for summons, templates, side effects, and unlinkingupdateCombatwithout dealing an extra tick on the core expiry eventVerification
npm test— 1,668 passed, 0 failednpm run verify— passed (0 errors; existing repository warnings remain)turnStartupdateWorldTimeremoved the parent item once and cleared the trackerturnStartbehavior