Skip to content

fix: infinite import#689

Merged
sjfhsjfh merged 10 commits intomainfrom
fix-infinport
Mar 8, 2026
Merged

fix: infinite import#689
sjfhsjfh merged 10 commits intomainfrom
fix-infinport

Conversation

@NuanRMxi
Copy link
Member

@NuanRMxi NuanRMxi commented Mar 6, 2026

When a directory fails to parse multiple times, it should be deleted immediately to prevent the program from crashing indefinitely. This code has a good repair effect on application crashes caused by insufficient memory.
fix again: #430

@NuanRMxi NuanRMxi marked this pull request as draft March 6, 2026 17:27
@NuanRMxi NuanRMxi marked this pull request as ready for review March 6, 2026 17:42
#[serde(default)]
pub collections: Vec<LocalCollection>,
#[serde(default)]
pub import_scan_retry: HashMap<String, u8>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there a need to persist this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to know what path caused the problem when restarting the program next time

continue;
}
let path = entry.path();
if self.import_scan_retry.get(&filename).copied().unwrap_or_default() >= 2 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bump_entry should be combined into this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what? bump_entry?Do you mean bump_retry ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging the bump_retry method into this code block will result in a complete semantic error in the code. Without bump_retry, this code block can never trigger it

@YuevUwU
Copy link
Contributor

YuevUwU commented Mar 7, 2026

Explanation

Details

To know how many times it was retried each game starting.

The player will encounter:

  • Import a large chart file (unzipped size)
  • Freeze or Crash due to low memory
  • User will end the game by any behaviour including force reboot
  • We can't guarantee all of our file is complete
  • [Verify needed] The interrupted file writing will cause fix_info inferring, it will try to load the whole file content to memory to parse and get metadata

Reproduce Method (before this PR)

  • Import a large chart (unzipped size)
  • Ctrl-C or kill Phira
  • Start Phira, you will see a black screen
  • If the task behind the black screen didn't complete, it won't enter the main game
  • If we entered the game successfully, we will just see an incomplete chart -- a useless chart

Solution Idea

  • Find the chart file takes long time (Make a profiler)
  • Block it to be import

Requirements

  • Known that player or os will kill the process
  • The profiler will satisfy
    • It can mark who is potential murderer
    • If the scanning task is successful, we consider the chart is not the murderer
    • The mark must be retraceable
      • Reason: if the scanning task won't end, the terminating of Phira will cause you can't do the next step to block the murderer. The Drop post-process also can't guarantee to run. The cleanup workflow may wait until next startup.
    • This is why to persist data
  • What we want to catch is unexpected terminating.

Current Profiler Idea

  • Mark each chart before its reading task.
  • Record the mark to a persistent file
    • Current: Save the mark of each chart to data.json. We assume the file writing won't fail.
  • If this murdurer mark is found the next time you open the app, avoid scanning it
    • Current: Remove it
  • If the chart load successfully, even if fs error, remove the murdurer mark

Consideration

  • The unexpected termininate during scanning may due to reasonable reason, for example, user accidentally start Phira and immediately exit.
    • Current: Mark it with a retry counter. Block the reading of the chart file if found that retried MAX_RETRIES
    • Current: MAX_RETRIES=2

Expected Behaviour (With this PR)

  • Player import a large file, and force reboot
  • Enter Phira. Player found that the black screen, and can't enter the game.
  • Player falls to pieces, and attempt to restart Phira multiple time
  • When player starts the game 3 times,
    and if there is only one murderer,
    and he force kill the game on the loading state of murderer charts
    • Enter the game successfully, and other charts shouldn't be hurt
    • (So we shouldn't hint the player "start and kill" as fast. They should be wait for little time, determined by their data/ folder size, to let those medium chart can be seen as safe charts, and won't remove them, unless add a freeze state as blocking flag for those murdurer charts)

Tested on AVD with 2GB RAM and chart in 430, based on the non-latest branch, with cold reboot.

Style suggestion: (insignificant)

  • Clarify the intention of the constant 2 in (many) if-block (MAX_RETRIES?)
  • Prefer to use .with_context(|| format! instead of map_err(|e| anyhow!, only one place using the former style
  • Other refactoring determined by the whole structure

@NuanRMxi
Copy link
Member Author

NuanRMxi commented Mar 7, 2026

The code already has deduplication logic: charts that are already recorded in data.json will not be scanned again. Therefore, this change should theoretically only affect charts being imported. Regardless of when the program exits, it can only delete files that are not yet present in data.json, i.e., charts that are still in the import process.

@NuanRMxi NuanRMxi requested a review from Mivik March 7, 2026 15:11
Copy link
Contributor

@sjfhsjfh sjfhsjfh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sjfhsjfh sjfhsjfh merged commit ca4487b into main Mar 8, 2026
6 of 8 checks passed
@sjfhsjfh sjfhsjfh deleted the fix-infinport branch March 8, 2026 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants