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

Offen
  • Stimmt etwas nicht?
#730 2 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen

Maintainer antworten meist innerhalb von 1 Tag

Dieses Issue hat noch niemand übernommen.

  • #733 von @deanrie — ohne Merge geschlossen

Bewertung

Schwierigkeit
4/5
Geschätzter Aufwand
3-5 Tage
Anfängerfreundlichkeit
52/100
Issue-Typ
Bug
Klarheit
Größtenteils klar
Aktivitätsstatus
Aktiv
Tech-Stack
node.js, typescript
Bereich
backend, devtools

Rechercherichtung

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.

Vom Indexierungsmodell aus dem Issue-Text verfasst.

Beschreibung

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.

Vorherrschende Sprache
TypeScript
Sterne
14.9k
Forks
1.5k
Ø Merge
3 Std. 22 Min.
Gemergte PRs (30 T.)
146

Entwicklungsumgebung

Dieses Projekt bietet weder Dev-Container noch Dockerfile noch Beitragsleitfaden – die Einrichtung liegt bei Ihnen. Beginnen Sie mit der README; die allgemeinen Schritte stehen in unserem Leitfaden für den ersten Beitrag.

Erste Schritte

  1. Lesen Sie das ganze Issue und danach den Beitragsleitfaden des Projekts.
  2. Schreiben Sie ins Issue, dass Sie es übernehmen — das erspart doppelte Arbeit.
  3. Forken Sie das Repository und arbeiten Sie in einem Branch.
  4. Öffnen Sie einen Pull Request, der die Issue-Nummer nennt.

Mehr aus cursor/plugins

Alle Issues in cursor/plugins

Ähnliche Issues

Weitere Issues zu TypeScript

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.