Feature/922 delete inactive players#1490
Conversation
Adds a configurable server setting and daily scheduled command that permanently deletes players inactive beyond a threshold. - New integer server setting `inactive_player_deletion_days` (0 = disabled, and 0 is the default), editable in the admin Server Settings panel. - New `ogamex:scheduler:delete-inactive-players` command, registered to run daily. No-op when the setting is 0. Selects users whose last activity (users.time) is older than the threshold. - Vacation mode does NOT exempt a player from deletion. - Admin accounts are excluded, mirroring the existing "administrators cannot be banned" protection. - New `PlayerService::deleteInactiveAccount()` removes the account and abandons every planet via `PlanetService::abandonPlanet()` so galaxy slots are freed and other players' incoming missions are handled gracefully. Adds a `$force` flag to `abandonPlanet()` to bypass the user-facing guards (last planet / active missions) during full-account deletion. Battle/espionage reports are intentionally preserved (their FK is ON DELETE SET NULL). - Leaves descriptive TODO (lanedirt#146) comments in the abandonment path where the unimplemented "Destroyed Planet" logic will apply. - Adds feature tests covering the disabled no-op, threshold selection, vacation-not-exempt, admin exclusion, and full-wipe data integrity. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Preview deploymentPreview environment for this PR is ready for functional testing. Use one of the test accounts below to log in.
Test accounts (click to expand)
This preview will be automatically destroyed when the PR is closed. |
Geda173
left a comment
Geda173
left a comment
There was a problem hiding this comment.
thanks for your work. I have attached some comments. There isn't anything big that needs changing, it's mostly preference.
Ideally I'd like a test for the case where another player has a fleet in flight toward the inactive player's planet. That's the scenario $force specifically unblocks by skipping the active-missions guard, and what keeps it safe is the planet_id_to === null check in GameMission::process(), which lives outside this feature and has no idea it's load-bearing for it. If that guard ever changes, nothing here would catch it. A test asserting the attacker's fleet turns around and returns home would pin the behaviour down.
There was a problem hiding this comment.
Since this setting permanently deletes accounts with no undo, could we put a floor on it? Right now 1 is accepted and would wipe everyone inactive for a day on the next run and it's an easy slip. I'd be much more comfortable if there was an dialogue box prompting the user to confirm the setting.
| * | ||
| * @return void | ||
| */ | ||
| public function deleteInactiveAccount(): void |
There was a problem hiding this comment.
This overlaps delete() above it quite heavily. Five of the statements are identical. My worry is drift: whoever adds the next player-related table updates one method and not the other. Since delete() is only called from dev commands now, could this become the single implementation with a flag for how planets get handled? The transaction wrapper here is also something delete() lacks, which is another argument for consolidating on this version.
| try { | ||
| // Load with a fresh cache so the deletion acts on the player's current | ||
| // planets, moons and missions rather than any stale cached state. | ||
| $playerServiceFactory->make($user->id, true)->deleteInactiveAccount(); |
There was a problem hiding this comment.
The player and planet factories are shared singletons with no way to clear them, so everything built for each deleted account stays in memory for the whole run. The batching limits query size but not memory. Likely a non-issue for nightly runs; I'd mainly want to know it's been tried against a large backlog, since that first run could be thousands of accounts. Most of the deployments will have only small groups of people, but something main.ogamex.dev with its many inactive accounts could be seriously in trouble.
|
|
||
| // Delete remaining user-scoped records that no database cascade covers. | ||
| Message::where('user_id', $this->getId())->delete(); | ||
| Highscore::where('player_id', $this->getId())->delete(); |
There was a problem hiding this comment.
The highscores table already deletes these rows automatically when the user goes, so this line does nothing.
| $deletedCount++; | ||
| } catch (Throwable $e) { | ||
| $failedCount++; | ||
| $this->error("Failed to delete inactive player #{$user->id}: " . $e->getMessage()); |
There was a problem hiding this comment.
Same pattern as CleanupWreckFields, so take this as optional, but since scheduled output isn't captured anywhere, an account that fails to delete every night would be completely invisible. This being the one job that deletes accounts irreversibly, a Log::error() alongside the console message seems worth it.
| // Admins are excluded to protect the server operator and system accounts, mirroring the | ||
| // existing "administrators cannot be banned" rule. Vacation mode does NOT exempt a player. | ||
| // The users.time column holds the last-activity UNIX timestamp. | ||
| User::withoutRole('admin') |
There was a problem hiding this comment.
This one is more for @lanedirt: Should moderators be excluded alongside admin? It looks like a non-issue today since that role doesn't grant any permissions yet, but if it ever becomes a real staff role those accounts are exactly the infrequent-login users this targets. Cheap to add now: withoutRole(['admin', 'moderator']). There is only a handful of mentions of the role in the codebase and it currently doesn't do anything.
There was a problem hiding this comment.
@Geda173 Yes I agree, since this is being touched already, adding the moderator role exclusion too here is a good idea (even if the role itself is not actually implemented yet as of this moment).
3 participants
Description
This PR implements automatic deletion of players who have been inactive beyond a configurable number of days (issue #922).
OGameX already measures inactivity (
users.time, surfaced throughPlayerService::isInactive()/isLongInactive()), but nothing ever acts on it — abandoned accounts persist forever, inflating the database and leaving dead planets cluttering the galaxy. This change closes that lifecycle gap.What it does:
inactive_player_deletion_days(integer).0disables the feature and is the default; any value> 0is the inactivity threshold in days. Editable in the admin Server Settings panel, backed by a typedSettingsServicegetter and seeded by a migration.ogamex:scheduler:delete-inactive-players(registered inroutes/console.php). It no-ops when the setting is0, otherwise selects users whose last activity predates the threshold and deletes them.PlayerService::deleteInactiveAccount(): it clears the player's own fleet missions, abandons every planet throughPlanetService::abandonPlanet()(freeing galaxy slots, cascading to moons, and nulling other players' incoming missions), then permanently removes the account. Runs inside a transaction.bool $force = falseparameter was added toabandonPlanet()so full-account deletion can bypass the user-facing guards (last remaining planet / active fleet missions). The default keeps existing behavior unchanged.TODO (#146)markers are left in the planet-abandonment path where the not-yet-implemented "Destroyed Planet" logic (Add "Destroyed Planet" logic when abandoning a planet #146) will hook in — no attempt is made to implement Add "Destroyed Planet" logic when abandoning a planet #146 itself.Type of Change:
Related Issues
Fixes #922
Checklist
Before submitting this pull request, ensure all following requirements as outlined in CONTRIBUTING.md are met:
composer run rector— full-tree dry-run reports no changes needed.)composer run cs -- --test; see note on pre-existing unrelated failures below.)composer run stan— 651 files, no errors.)tests/Feature/DeleteInactivePlayersCommandTest.phpwith 5 tests.)php artisan test— 1065 passed, 11,744 assertions.)Additional Information
Design decisions worth reviewer attention:
planet_user_idforeign key is alreadyON DELETE SET NULL(added in Fix FK constraint violation when deleting a player with espionage reports #1326 / the battle-report fix), so deleting a player nulls the reference rather than orphaning or destroying the report — the other party keeps their copy. The deletion logic deliberately does not delete these rows.deleteInactiveAccount()reusesabandonPlanet()rather than the existingPlayerService::delete()(which raw-deletes planet rows), so galaxy slots are freed and in-flight missions are handled correctly.Files changed:
app/Services/SettingsService.php,database/migrations/..._add_inactive_player_deletion_setting.php,app/Http/Controllers/Admin/ServerSettingsController.php,resources/views/ingame/admin/serversettings.blade.php— the setting.app/Services/PlayerService.php(deleteInactiveAccount()),app/Services/PlanetService.php($forceparam +TODO (#146)markers).app/Console/Commands/Scheduler/DeleteInactivePlayers.php,routes/console.php— the scheduled command.tests/Feature/DeleteInactivePlayersCommandTest.php— tests.Breaking changes: None. The feature ships disabled by default (
inactive_player_deletion_days = 0), and the addedabandonPlanet($force = false)parameter is optional, so existing callers are unaffected.Note on Pint: Running
composer run cs -- --testacross the whole tree reports pre-existing style issues in ~19 files that this PR does not modify (e.g.app/Models/Setting.php,app/Models/User.php, severalScheduler/Testcommands). Per the "one issue, one PR" guideline these were left untouched; all files changed by this PR pass Pint cleanly.