Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
…state looting_watch only checked the watched script's (eloot's) pause state, never bigshot's own. When something paused bigshot and eloot back to back (e.g. ecleanse pausing a script list for a disarm-recovery cast), looting_watch noticed eloot's pause and returned to its caller immediately, regardless of whether bigshot itself had also just been paused. That let bigshot's thread advance to the next corpse's run_script/Script.kill one iteration early, racing eloot's own independently-resumed thread with no reliable ordering. Add Script.current at the top of the loop - the same idiom lich-5 uses elsewhere (echo, fput, ...) to block on the calling script's own pause via Script#wait_while_paused! - so bigshot's own pause is checked on every iteration, not just at entry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
c610a14 to
0a13d45
Compare
Keep this fix landing as part of the v5.16.0 release the rest of the stacked PRs are shipping, rather than a separate v5.16.1. Also rewrites the v5.16.0 changelog entries (including this one) in plain, player-facing language instead of implementation detail. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Superseded by #2433 - same commit, pushed directly to this repo instead of from my fork so it groups into the bigshot PR stack correctly. |
… paused (#2433) ## Summary - Fixes a rare timing bug in `looting_watch`: it only checked the *watched* loot script's pause state, never bigshot's own. When something (e.g. `ecleanse`) paused bigshot and the loot script back-to-back for a disarm-recovery cast, `looting_watch` would notice the loot script's pause and hand control back immediately - regardless of whether bigshot itself had also just been paused. - That let bigshot advance one extra loot iteration and kill the loot script for the *next* corpse while the *current* one was independently resuming and finishing its own action, with no reliable ordering between the two. Could skip a loot/skin action or cut one off mid-command. Real session log reproducing it is in the linked work unit. - Fix: block on bigshot's own pause at the top of the loop (the same idiom used elsewhere in the script) so it can't race ahead while paused. - Landing as part of v5.16.0 alongside the rest of the stack rather than a separate release, and simplified the v5.16.0 changelog entries to plain language. - Stacked on `docs/bigshot-yard-documentation` (#2417), the tip of the open bigshot PR stack (#2414 -> #2415 -> #2416 -> #2417 -> this). - Replaces #2432, which was opened from a fork and so didn't group into the native stack view; this is the same branch pushed directly to this repo. ## Test plan - [x] New regression spec `spec/bigshot/looting_watch_spec.rb`, following the source-extraction-and-eval technique from `spec/bigshot/priority_spec.rb`. Uses a Mutex/ConditionVariable + Queue-based stub instead of sleep-based polling, so the race is deterministic: - blocks while bigshot is paused even though the loot script is also paused, and only returns once bigshot is unpaused - still breaks immediately on the loot script's own pause once bigshot itself was never paused (no behavior change for the common case) - kill/box-in-hand/rest-flag handling after the loop unaffected - [x] Audited every other `Script.paused?`/`Script.running?` call site in bigshot.lic for the same bug shape - none found; this was the only raw polling loop watching another script's state without also checking its own - [x] `bin/migrate` run first (gameobj-data fixtures) - [x] `bundle exec rspec` - full suite passing - [x] `RBENV_VERSION=4.0.5 rubocop -A` - clean Generated with Claude Code --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Summary
looting_watch: it only checked the watched loot script's pause state, never bigshot's own. When something (e.g.ecleanse) paused bigshot and the loot script back-to-back for a disarm-recovery cast,looting_watchwould notice the loot script's pause and hand control back immediately - regardless of whether bigshot itself had also just been paused.docs/bigshot-yard-documentation(docs(bigshot.lic): v5.16.0 add YARD documentation throughout #2417), the tip of the open bigshot PR stack (feat(bigshot.lic): v5.16.0 migrate room-creature targeting to Creature module #2414 -> feat(bigshot.lic): v5.16.0 add crtrStatus command checks, migrate flying/rooted/frozen/prone #2415 -> feat(bigshot.lic): v5.16.0 add ;bigshot help and unknown-option fallback #2416 -> docs(bigshot.lic): v5.16.0 add YARD documentation throughout #2417 -> this).Test plan
spec/bigshot/looting_watch_spec.rb, following the source-extraction-and-eval technique fromspec/bigshot/priority_spec.rb. Uses a Mutex/ConditionVariable + Queue-based stub instead of sleep-based polling, so the race is deterministic:Script.paused?/Script.running?call site in bigshot.lic for the same bug shape - none found; this was the only raw polling loop watching another script's state without also checking its ownbin/migraterun first (gameobj-data fixtures)bundle exec rspec- full suite passingRBENV_VERSION=4.0.5 rubocop -A- cleanGenerated with Claude Code