fix: YMM4のカスタムボイス機能が使えない問題の対策

[sabonerune]

内容

YMM4のカスタムボイス機能が正常に動作しないというXのポストを見かけたため修正しました。
ymm4/faq/ゆっくりボイス/外部の音声合成エンジンで作成した音声ファイルを使用したい

原因の詳細は不明ですが音声ファイルを一時ファイルに書き込みファイル名を変更していることが問題のようです。

• ref: fix: volta用のNode.jsとnpmのバージョンを更新し、ワークフローのパスを修正 #2368

このPRでは書き込み先にファイルが存在しない場合は直接書き込むように変更します。

その他

writeFileSafelyで気になったこと

• ${path}.tmpが既に存在した場合それを上書きしてしまう。ユーザーが作成していないと仮定しても本当に平気だろうか?
• moveFileSync失敗時に作成した一時ファイルの後始末をしていない。

[voicevox-preview-pages]

🚀 プレビュー用ページを作成しました 🚀

• 📝 エディタ
• 📖 Storybook

更新時点でのコミットハッシュ：24ceffe

[Hiroshiba]

プルリクエストありがとうございます！

これ多分どちらかというとVOICEVOX側の課題ではない気がします。
破損を避けるためにファイルをアトミックに保存するのは悪いことではない･･･はず･･･？
（もしかしたら一般的ではないかもですが･･･）

おそらくYMM4側はファイル保存イベントを監視していて、moveイベントは監視していないんだと思います。
ただ今はVOICEVOXだけがmoveを使ってないだけかも知れず、将来的にmove対応することは悪いことではないかもとちょっと思ってます。

ちょっと元ポストのリンクを教えていただいてもよろしいでしょうか 🙇
僕の方からYMM4の作者の方に連絡してみて、どうしたら良いかを聞いてみたいと思います！

---

それはそれとして、

${path}.tmpが既に存在した場合それを上書きしてしまう。ユーザーが作成していないと仮定しても本当に平気だろうか?
moveFileSync失敗時に作成した一時ファイルの後始末をしていない。

この２点は普通に問題だと思います！！
前者はランダムなファイル名にする（.tmp-8桁のアルファベット数字とか）、後者はtry catchすべきだと思います！！
これももしよかったら別プルリクエストで変更修正いただけると嬉しいです！！！

[sabonerune]

@Hiroshiba Xのポストです。
https://x.com/Noel_TKM/status/1872112448322535782
https://x.com/uloed/status/1871148669153792078

これ多分どちらかというとVOICEVOX側の課題ではない気がします。

自分もVOICEVOXの問題ではないという認識です。
ただVOICEVOX単体では問題なくてもそれ以外の要因が絡むと予想外の動作を起こしやすいような気がします。

これももしよかったら別プルリクエストで変更修正いただけると嬉しいです！！！

こちらも後ほどPRを出そうと思います。

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

[Hiroshiba]

ただVOICEVOX単体では問題なくてもそれ以外の要因が絡むと予想外の動作を起こしやすいような気がします。

長いファイルとかだと途中で書き込み失敗した時とかでエラーが発生するとかもありえそうなので、やっぱり基本書き込みが完了した後にmvが良さそうに思います！
特にソングとかはこの傾向がありそう。

だから「どっちにしろ問題は起こりうる」な気がしますね！
ただ今は必ず問題が起こってしまうので、問題そう。
リンクありがとうございます、伺ってみたいと思います！

[Hiroshiba]

ご連絡して、来年１月上旬の次回アップデート時にmvも検知可能なように修正したいとの回答をいただきました！
（饅頭遣いさんありがとうございます 🙏 ）

なのでこのPRは一旦draftにするのもありかもです･･･！
が、hotfixを入れてすぐアップデートするのもありかもしれないですね。どうしようかな･･･。

release-0.22にhotfixするとしたら、writeFileSafelyの挙動を変えるのは影響範囲を精査するのが大変なのでやめて、新しくWRITE_FILE_UNSAFEを作り、音声やラベル？などを保存する経路ではそちらを使うのはどうでしょう？
あるいはWRITE_FILE関数にoptions?: {unsafe?: boolean}引数を持たせて、音声やラベル保存では一時的に{unsafe: true}を渡す設計にするのはどうでしょうか。

これなら実は29日にアップデートがあるので、そのタイミングで取り込めると思います。
一時的で良いと思うので、コメントで// NOTE: YMM4がファイルmvでの変更検知に対応したらunsafeを外すなどと書く感じで！

writeFileSafelyの中身を変える今のPRの実装は、挙動が変わってしまっていてコメントドキュメントのtmpファイル書き込み後、保存先ファイルに上書きするが嘘になってしまっているのと、if文がいくつか入っている部分がシンボリックリンクなどでも正しく動くのか精査する必要があり、レビューに時間がかかるので、迂回ルートを作って影響を最小限にしたいです。
新たにunsafeルートを一時的に作り、unsafeのときのみ挙動を変える形であればすぐ実装できると考えてます。
例えばこのコードをこんな感じとか：

  WRITE_FILE: (_, { filePath, buffer, options }) => {
    let data = new DataView(buffer instanceof Uint8Array ? buffer.buffer : buffer)
    try {
      // NOTE: unsafeはYMM4がファイルmvでの変更検知できないののワークアラウンド。YMM4が対応したら消す。
      if (options?.unsafe) {
        whiteFile(filePath, data)
      }
      else {
        writeFileSafely(filePath, data);
      }
      return success(undefined);
    } catch (e) {
      // throwだと`.code`の情報が消えるのでreturn
      const a = e as SystemError;
      return failure(a.code, a);
    }
  },

[sabonerune]

@Hiroshiba
返答が遅くなってすみません。

ご連絡して、来年１月上旬の次回アップデート時にmvも検知可能なように修正したいとの回答をいただきました！

YMM4に修正が入るならHotfixではなくていいと思います。

長いファイルとかだと途中で書き込み失敗した時とかでエラーが発生するとかもありえそうなので、やっぱり基本書き込みが完了した後にmvが良さそうに思います！

新規ファイルの場合書き込み失敗しても失われるデータはないのでアトミックである必要が必ずしもない気がします。
(破損したファイルが残るがあまり影響はない気がします)

---

stat.isFile()を置いた理由は書き込み先がファイルではない場合moveFileSync()は失敗するだろうという認識で追加しました。
fs.stat()はシンボリックリンクを辿るためシンボリックリンク先がファイルならstat.isFile()はTrueになるはずです。

一方その後のmoveFileSync()ではシンボリックリンク先のファイルを書き換えずにシンボリックリンク自体ををファイルと置き換えるという挙動になっていることに気が付きました。

これはそもどのようにするべきなのでしょうか？

[Hiroshiba]

新規ファイルの場合書き込み失敗しても失われるデータはないのでアトミックである必要が必ずしもない気がします。

手法にもよりそうですが、他には書き込んでる途中にデータ読んじゃうとかもありえるかなぁと思いました！
configファイルとかはVOICEVOXの中でも初回の作成と読み出しが別プロセスなので、気をつけないと起こり得るかもです！

一方その後のmoveFileSync()ではシンボリックリンク先のファイルを書き換えずにシンボリックリンク自体ををファイルと置き換えるという挙動になっていることに気が付きました。
これはそもどのようにするべきなのでしょうか？

これは流石にシンボリックリンクであることを無視して、シンボリックリンク関係なくそのパスにファイルを移動するのが良い気がします！
コメントした意図としては、そもそもifで分岐してもthrow eするだけなので、何もコードを書かないのと同じな気がしたためです。
PRのコードはたぶんこう書いても挙動がほとんど変わらないはず･･･？

export function writeFileSafely(
  path: string,
  data: string | NodeJS.ArrayBufferView,
) {
  // 上書きでないのなら直接書き込む
  fs.writeFileSync(path, data, { flag: "wx" });

  const tmpPath = `${path}.tmp`;
  fs.writeFileSync(tmpPath, data);

  moveFileSync(tmpPath, path, {
    overwrite: true,
  });
}

まあエラーログはwriteFileSyncに失敗したかmoveFileSyncに失敗したかの違いはあるかもですが、コードが簡潔な方が良い･･････かなぁ。難しい！！

---

とりあえず方針としては、いったんYMM4の更新を待ちつつ、unsafe版whiteFileを実装するか考えるのが良さそうかなと思いました！
@sabonerune さん的にunsafe版whiteFileは必要だと感じる、あるいはモチベがあれば、WRITE_FILEにoptions: { unsafe: boolean }をもたせる形の実装をお願いしたいです！！

個人的な今の考えだと、待機が安定かな？と思ってます 🙏

[sabonerune]

YMM4の修正が既に終わっているようなのでCloseします。
WRITE_FILEにoptions: { unsafe: boolean }持たせるのは他のソフトで動かなくて対応が難しそうだという話が出てきてからでも遅くないと思います。