pstack orch: stale-lock takeover can be lost (unlink-then-create TOCTOU); frontier SHAs use local branches

オープン
  • 何か問題がありますか？
#730 コメント 2 件 リアクション 0 件 担当者 0 名 GitHub で見る

メンテナーはふだん 1 日以内に返信

まだ誰も着手していません。

  • #733 @deanrie による — マージされずにクローズ

評価

難易度
4/5
見積もり時間
3〜5日
初心者へのやさしさ
52/100
issue の種類
バグ
明瞭さ
おおむね明確
活発さ
活発
技術スタック
node.js, typescript
領域
backend, devtools

調査の方向性

Start with skills/poteto-mode/scripts/orch/store.ts, reading acquireLock and the frontier mapping around the locations cited in the issue. Trace how stale lock holders are read and how the PR head SHA is represented in the snapshot. Done means stale takeover preserves mutual exclusion and frontier SHAs identify the PR head; the issue suggests a two-process test for the lock behavior.

索引モデルが issue の本文から書いたものです。

説明

Summary

acquireLock in the pstack orchestrator store takes over a stale lock by unlink-then-create, which has a TOCTOU window: two processes that both see the same dead holder can end up both holding the lock.

  • Version: pstack 0.15.28 (harness/pstack/claude-code)
  • File: skills/poteto-mode/scripts/orch/store.ts, acquireLock (~L376–405)
const takeOver = async (): Promise<void> => {
  await unlink(path);          // (1) both processes can reach here
  try {
    await create();            // (2) one wins; the loser's unlink may land later
  } catch (retryError) { ... }
};

Interleaving that loses mutual exclusion

step P1 P2
1 reads holder, decides it is dead
2 reads holder, decides it is dead
3 unlink(path)
4 create() → holds lock
5 unlink(path) → deletes P1's fresh lock
6 create() → also "holds" lock

Both processes then proceed to write the store, so one writer's update is lost.

Suggested fix

Make takeover atomic rather than unlink-then-create. Either:

  1. Lock directory — mkdir the lock path (atomic; EEXIST is the contention signal) and rmdir on release. Changes the lock artifact from a file to a directory, and the reader of holderIsDead needs to read the pid from a file inside it.
  2. Rename-based takeover — rename(path, path + ".stale." + pid): only one process can win the rename; then read the renamed file to report the previous holder, and create() the new lock. On EEXIST, report the current holder.

A two-process test (spawn two children that both observe a dead pid and assert exactly one acquireLock resolves) would pin the behaviour.

Related finding in the same file: frontier SHAs come from local branches

skills/poteto-mode/scripts/orch/store.ts (~L1229–1235):

return chain.map((row) => ({
  pr: row.pr,
  branches: row.branch,
  sha: branchSha({ branch: row.branch, repo }),   // local branch, not PR head
  state: row.state,
}));

The frontier's verification identity is taken from the local branch, while the thing being verified is the PR head. If a local branch is stale or missing, the recorded SHA silently describes a commit that is not under review — a false verification identity rather than an error.

Suggested fix: source the SHA from the PR head OID already present in the snapshot, and represent "no local branch" as an explicit unknown instead of substituting a branch tip.

Happy to send PRs for either; filing first in case the lock artifact shape (file vs directory) is a compatibility constraint I cannot see.

主要言語
TypeScript
スター
14.9k
フォーク
1.5k
平均マージ
3時間 18分
マージ済み PR（30日）
144

環境構築

このプロジェクトには開発コンテナ、Dockerfile、コントリビューションガイドがありません。まず README を読み、一般的な手順ははじめてのコントリビューションガイドを参照してください。

はじめの一歩

  1. issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
  2. 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
  3. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

cursor/plugins のほかの issue

cursor/plugins の issue をすべて見る

似ている issue

TypeScript の issue をもっと見る

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。