Persist temporary summon expiry and define durable follower categories

[moshehbenavraham]

Fixes #119

Problem

save_char_pets() / load_char_pets() preserved substantial runtime state but not the remaining ePURGEMOB lifetime. A temporary follower such as an illusory decoy could reload without its expiration event and live forever. Every charmed NPC was also implicitly persistent, so ordinary spell summons reloaded across logouts.

Policy

Follower persistence is now explicit (pet_lifetime_kind() in src/utils.c):

• Durable (companions, familiars, mounts, dragon mounts, eidolons, golems, mercenaries, animated dead, lycanthropes, totem spirits, purchased or charmed followers with no spell source): persist until dismissed, killed, or stored.
• Timed control (a charm affect with a duration): the remaining affect duration is saved as before and pauses while the owner is offline.
• Deadline (a live ePURGEMOB event, plus illusory decoys, which never persist without one): an absolute real-time deadline that keeps elapsing offline, across reboots and copyovers.
• Session (any other spell summon: summoned creatures, allies, genies, shamblers, planar allies, and so on): lasts while the owner plays and is never saved. A saved record of one is rejected on restore.

The pet keeper boards only durable and timed-control followers. Reclaiming a stored row whose lifetime has ended (for example a decoy stabled before this policy existed) deletes the row and its objects inside the reclaim transaction and tells the player the keeper released it, so the stable slot is not held forever.

Changes

• Runtime-state record version 4 adds a validated T <kind> <epoch> line bound to the existing pet row identity. The scheduler handle is never persisted. The existing versioned parser and the atomic snapshot contract are unchanged.
• Restore rejects an expired record, a decoy record without a deadline, and a session-summon record; the roomless copy is discarded and logged. Active rows are dropped by the next snapshot; stored rows are deleted on reclaim. Live deadline records get a fresh native ePURGEMOB for the remaining time, and the restore fails closed if that event cannot be scheduled.
• save_char_pets() skips session followers; pet_store_pet() and the keeper's stable store refuse session and deadline followers with a player-facing message.
• pets shows each pet's persistence policy and remaining real time, with a footer explaining offline behavior.
• Help updated in lib/text/help/help.hlp and sql/components/help_pet_entries.sql (PETS and STABLE). docs/systems/SAVE_SYSTEMS_BREAKDOWN.md updated.

Tests

unittests/CuTest/test_pet_lifetime.c covers the record format: deadline round trip with a fresh event, repeated saves agreeing on the absolute deadline, expired restoration rejected with no event, malformed records, missing or stray lifetime lines by version, legacy v3 records restoring as durable, decoys saved as spent without a live event, session summons classified and rejected, keeper acceptance per kind, and status output for every policy.

Test_pet_lifetime_survives_snapshot_restore_and_keeper_release in unittests/CuTest/test_database_persistence.c drives the production path against MariaDB (LUMINARI_TEST_MYSQL_ENABLE=1) with real prototypes and a room: save_char_pets() skips the session summon and stores the absolute deadline; load_char_pets() republishes the deadline follower with a fresh event and the control follower with its affect; an expired active row is refused on reload while the roster still counts as fully restored and the next save_char_pets() drops it; pet_store_pet() refuses deadline and session followers; pet_retrieve_stored() deletes a spent legacy stored decoy and reports the release, and still reclaims a live durable stored follower into the room. Disconnect, reboot, and copyover all reach persistence through these same functions.

https://claude.ai/code/session_01Adu1bF5vNR7Drp3a9oKR97

[coderabbitai]

Warning

Review limit reached

Next included review available in 8 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: da5b4f6e-1891-4cdf-b5d2-ca7ae5e07fb0

📥 Commits

Reviewing files that changed from the base of the PR and between ced0bac and 4add1ca.

📒 Files selected for processing (11)

• CMakeLists.txt
• Makefile.am
• docs/systems/SAVE_SYSTEMS_BREAKDOWN.md
• lib/text/help/help.hlp
• sql/components/help_pet_entries.sql
• src/players.c
• src/spec/spec_mobiles.c
• src/utils.c
• src/utils.h
• unittests/CuTest/test_database_persistence.c
• unittests/CuTest/test_pet_lifetime.c

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[moshehbenavraham]

Verdict: issues found

Adversarial review of head b8481b80e17f3c12009f0bbf1b5c599f77321930 (all 10 files plus save_char_pets / load_char_pets / prepare_saved_pet_row / pet_retrieve_stored / ePURGEMOB / charm-duration / copyover login). CodeRabbit's rate-limit stub is not this review. #120 / pet_save_objs.pet_idnum is out of this diff.

The version-4 T <kind> <epoch> parser, spent-decoy serialization (T 1 <now> rather than durable), and active roster login path are largely what #119 asked for: the scheduler handle is never persisted, malformed blobs still fail parse and retain the row, and an expired active pet is extracted roomless without setting restore_failed so a later save_char_pets snapshot can drop it. Copyover really does save_player_pets() then enter_player_game() -> load_char_pets().

The hole is the shared decoder's expiry path vs stored rows.

Blocker

1. Expired stored deadline pets are never dropped (src/players.c:7248, caller pet_retrieve_stored ~7818 is not in the diff).
   • Claimed: restore rejects an expired record, discards the roomless copy, and the next snapshot drops the row.
   • Actual: true only for PET_STATE_ACTIVE after a successful load plus a later snapshot. pet_retrieve_stored treats prepare_saved_pet_row returning NULL as "The keeper cannot rouse that follower." and ROLLBACKs, so pet_state stays PET_STATE_STORED. save_char_pets only replaces active rows.
   • Why it matters: PET_KEEPER_CAPACITY is 10. stable store accepts any loyal follower, including an illusory decoy. pet_store_surviving_followers() auto-stores every charmed NPC on death, including decoys. After the 120s SPELL_MISLEAD ePURGEMOB elapses, reclaim fails forever and the slot stays occupied. Help says the pet is gone when you return; the stable list still shows it.

Non-blockers

2. restore_pet_lifetime does not verify the fresh ePURGEMOB attached (src/players.c:6363). attach_mud_event is void and frees the event on admission failure; the function still return true and the pet is published. That is session-length immortality on the reconstruct path (a later decoy save would write spent T 1 <now>). Claimed: live records get a fresh native event.

3. New tests never drive the production restore/snapshot/keeper path (unittests/CuTest/test_pet_lifetime.c). They exercise serialize_pet_runtime_state / restore_pet_runtime_state_for_test only (deadline round-trip, repeated serialize, malformed T lines, v3 stray line, spent decoy, PETS strings). They do not call save_char_pets, load_char_pets, prepare_saved_pet_row, or pet_retrieve_stored. Issue #119's disconnect/reboot/copyover coverage is call-graph commentary. Finding 1 is exactly the untested path.

4. Save eligibility is still "every charmed NPC" (pet_lifetime_kind / save_char_pets). #119 asked to define durable categories instead of treating every charmed NPC as implicitly persistent. Magical summons still set the charm bit without ePURGEMOB or a charm affect with duration >= 0 (only SPELL_MISLEAD gets the 120s event), so they classify as PET_LIFETIME_DURABLE and still reload forever. Labels changed; what gets snapshotted did not.

Checked and not wrong

• Durable serialize is T 0 0 (init_pet_runtime_state memset), not stack garbage.
• Parser %n + pet_state_line_is_complete rejects trailing junk; v4 requires T; v3 stray T fails; T 0 5 / T 1 0 / T 2 5 fail validation.
• Absolute epoch + pulse ceiling: repeated saves stay within ~1s; not an immortality leak.
• NPC ePURGEMOB is not in the player event copyover table; pets rebuild from DB only (no double event).
• stop_follower's 12s purge is after the pet leaves the follower list, so it is not snapshotted as a deadline companion.
• Help .hlp and help_pet_entries.sql match aside from SQL pet''s escaping.

[moshehbenavraham]

[blocker] Expired restore is a non-failure extract of a roomless copy. That only lets the next active snapshot drop the row.

pet_retrieve_stored (not in this diff, ~7818) shares this decoder. On NULL it sets "The keeper cannot rouse that follower." and ROLLBACK, so PET_STATE_STORED is unchanged. save_char_pets only DELETEs/replaces PET_STATE_ACTIVE rows.

Claimed: reject expired, discard roomless copy, next snapshot drops the row.

Actual: stored deadline pets (manual stable store, or pet_store_surviving_followers on death, both of which accept decoys) stay in the stable forever after the epoch, occupying one of 10 slots. Help says the pet is gone when you return.

Fix on this path: if lifetime restore fails because the deadline elapsed (or a decoy had no deadline), delete or mark-unretrievable the stored row and its pet_save_objs instead of rolling back to the expired snapshot. Do not set restore_failed for that case on login (active drop-on-snapshot is correct).

[moshehbenavraham]

[non-blocker] attach_mud_event is void; on admission failure it frees the event and returns. This function still return true and the caller publishes the pet.

Claimed: a live record gets a fresh native ePURGEMOB for the remaining time.

Actual: the call is issued; success is not observed. A deadline follower restored without an event is the original #119 bug for that session (a decoy would be saved spent on the next snapshot, so it is not forever).

After attach, require mud_event_is_live(char_has_mud_event(pet, ePURGEMOB)) and return false (discard) if it is not live.

[moshehbenavraham]

[non-blocker] This suite drives serialize_pet_runtime_state / restore_pet_runtime_state_for_test only. Expired is "helper returned false and no event," not "prepare_saved_pet_row extracted a roomless read_mobile copy and the DB row was dropped."

Missing vs the PR/#119 claims:

• save_char_pets / load_char_pets (disconnect, reboot, copyover all go through those)
• keeper retrieve of an expired T 1 <past> stored row (pet_retrieve_stored rollback vs active snapshot drop)
• restore_failed left false on expiry so login can still mark PET_ROSTER_LOADED

The serializer cases here (malformed T lines, v3 stray T, spent decoy T 1 <now>) are real. They would not have caught the stored-row leak.

[moshehbenavraham]

[non-blocker / docs] "lets the next snapshot drop the row" is only true for PET_STATE_ACTIVE after load_char_pets succeeds. Stored rows have no such snapshot: save_char_pets replaces active rows only, and keeper retrieve rolls back on expiry.

Same overclaim is in the PR body and the prepare_saved_pet_row comment at src/players.c:7247. If stored deadline pets are meant to expire, document that reclaim deletes the stored row; if they are meant to pause, pet_lifetime_kind/serialize must not keep a wall-clock epoch while PET_STATE_STORED.

[moshehbenavraham]

Addressed all four review findings in the follow-up commit (branch squashed to one commit on top of current master; no rebase was needed).

1. Expired stored deadline pets were never dropped (blocker). prepare_saved_pet_row() now reports lifetime rejection through an expired out-parameter, distinct from decode failure. pet_retrieve_stored() deletes the spent row and its pet_save_objs inside the already-open FOR UPDATE transaction, commits, and tells the player the keeper released it, so the slot is freed. Separately, pet_store_pet() and the keeper's stable store now refuse deadline and session followers (pet_keeper_accepts()), so new decoys cannot be stabled at all; pet_store_surviving_followers() inherits that through pet_store_pet(). Active rows still drop on the next snapshot as before.

2. restore_pet_lifetime() did not verify the fresh ePURGEMOB attached. After attach_mud_event() it now checks mud_event_is_live(char_has_mud_event(pet, ePURGEMOB)) and fails closed with a SYSERR, so an admission failure rejects the record instead of publishing an immortal follower.

3. Tests never drove the production restore/snapshot/keeper path. Added Test_pet_lifetime_survives_snapshot_restore_and_keeper_release in test_database_persistence.c (runs with LUMINARI_TEST_MYSQL_ENABLE=1, real prototypes and a room): save_char_pets() stores the absolute deadline and skips the session summon; load_char_pets() republishes the deadline follower with a fresh live event and the control follower with its affect; an expired active row is refused on reload while the roster still counts as loaded, and the next save_char_pets() drops it; pet_store_pet() refuses deadline and session followers; pet_retrieve_stored() deletes a spent legacy stored decoy with the release reason and still reclaims a live durable stored follower into the room. Finding 1's exact path is covered.

4. Save eligibility was still every charmed NPC. Added PET_LIFETIME_SESSION: a follower with a spell source (pet_source_spell != 0) that is not in a kept family (companion, familiar, mount, dragon mount, eidolon, golem, mercenary, animated dead, lycanthrope, totem spirit) and has neither a deadline nor timed control. save_char_pets() skips session followers, restore_pet_lifetime() rejects a saved record of one (including legacy rows), the keeper refuses them, and pets labels them "this session only, not saved". Animated dead stay durable because HELP ANIMATE DEAD promises a permanent follower. The kept-family list is one table in src/utils.c if the policy needs adjusting.

Also fixed an int narrowing in pet_lifetime_deadline() flagged by clang-tidy, and updated HELP PETS, HELP STABLE (both help.hlp and sql/components/help_pet_entries.sql, applied to the dev help database) and docs/systems/SAVE_SYSTEMS_BREAKDOWN.md.

Local CI replication (GitHub jobs are not treated as gates):

• make test + make install: 1367 runs, 1366 pass. The single failure is Test_syntax_check_encounter_world_boots_and_cleans_up_once, which forks the server against the local dev database that the in-progress Pets: reconcile schema definitions and strengthen owner/object persistence identities #120 branch has already migrated (SYSERR: Required pet column pet_save_objs.pet_idnum has an incompatible type or nullability); the same reason the local server startup smoke test cannot run here. Not related to this diff, and it passes on CI's isolated database.
• All remaining make test sub-targets, make test-world-tools, make test-protocol, protocol fuzzing (15 s), make test-process-memory, test-character-rename-static/schema: pass.
• Valgrind (--errors-for-leak-kinds=definite) over cutest: 0 errors apart from CuTest's own failure-message strings for the environment failure above.
• ASan/UBSan cutest build (gcc, same flags as CI): no sanitizer errors. With the MariaDB tests enabled (CI's sanitizer job does not enable them) LeakSanitizer reports 11 small leaks in the pre-existing Test_named_eidolon_keeper_restore_preserves_saved_identity path (set_eidolon_descs strdup via pet_retrieve_stored); identical at the previous PR head, so pre-existing and out of scope. Worth a follow-up issue.
• Strict-warning rebuild of changed files (-Wall -Wextra): 0 warnings. pre-commit run clang-format: clean. clang-tidy (CI check set) on src/utils.c: no findings in the changed code. gitleaks: no leaks.

[moshehbenavraham]

Merged to master as 303ba07 (squash of one commit, rebased onto #146 first with no conflicts).

What merged

• Explicit follower persistence policy (pet_lifetime_kind()): durable, timed control, real-time deadline, and session-bound spell summons. Session summons are never snapshotted and their saved records are rejected on restore.
• Runtime-state record version 4 with a validated T <kind> <epoch> deadline line; restored deadlines rebuild a native ePURGEMOB event and fail closed if it cannot be scheduled.
• Keeper boards only durable and timed-control followers; reclaiming a stored row whose lifetime has ended deletes it inside the reclaim transaction and releases the slot.
• pets shows each pet's policy and remaining real time. HELP PETS and HELP STABLE updated in help.hlp and the SQL component (applied to the dev help database); docs/systems/SAVE_SYSTEMS_BREAKDOWN.md updated.
• Unit coverage of the record format plus a MariaDB-backed test through save_char_pets, load_char_pets, pet_store_pet, and pet_retrieve_stored.

Rebased verification: make test on top of #146 ran 1370 tests with 1369 passing; the single failure is the syntax-check boot test against the local dev database, which now trips on pet_data description-column drift from #146's schema contract, not on this change.

Follow-up

• LeakSanitizer: eidolon description strings leak through pet_retrieve_stored in keeper round-trip test #148: pre-existing LeakSanitizer findings in the eidolon keeper round-trip test path (only visible with the MariaDB tests enabled under ASan).
• Expiry gear handling and uncertain-commit reconciliation remain open in docs/ongoing-projects/PET_SYSTEM_REFACTOR_PLAN.md, as noted in the save-systems document.
• The local dev database needs re-aligning with sql/master_schema.sql before the server or the syntax-check boot test will run here.