activeloopai / activeloopai/hivemind

skillopt: worker lock is per-machine — concurrent improvements to the same skill from two machines can silently lose an edit

Aperta
#340 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub
Lingua principale
TypeScript
Stelle
1.6k
Fork
107
Merge medio
17h 30m
PR unite (30g)
6

Descrizione

`tryAcquireWorkerLock(projectKey)` (`src/skillify/state.ts:193`) is a **local lockfile** under the state dir. It correctly serializes SkillOpt workers on one machine — including careful TOCTOU handling for the stale-lock-as-directory case (lines 204-223).

It does **not** serialize across machines, and the skills table is org-wide.

**Scenario.** Skill `X--alice` is at v7. Two engineers on different machines invoke it and both react negatively within the same window:

1. Worker A (machine 1) acquires its local lock, reads current row → v7, proposes edit E1.
2. Worker B (machine 2) acquires *its own* local lock — unaware of A — reads current row → v7, proposes edit E2.
3. Both call `publishImprovedSkill`, both compute `version = 7 + 1`.

Result: two rows at v8 for the same `(project_key, name)`. Readers use `ORDER BY version DESC LIMIT 1` (`src/skillify/skills-table.ts:14-17`), so **one of the two edits is silently discarded** — and which one wins depends on the row ordering for the tie, which isn't specified. Both edits were grounded in genuine confirmed failures; one just disappears.

The append-only design means nothing is destroyed on disk (both rows persist), so this is a **lost update at the read layer**, not data loss. That also makes it cheap to detect after the fact.

**Options:**
1. **Conditional publish (compare-and-set):** re-read the max version immediately before insert and fail/retry if it moved. Doesn't fully close the race without a unique constraint, but shrinks the window a lot.
2. **Unique index on `(project_key, name, version)`** so the second insert fails loudly, then retry the propose against the new current body (the natural fix — the loser's edit gets re-proposed against v8 instead of vanishing).
3. **Deterministic tie-break** on the read side (e.g. `ORDER BY version DESC, updated_at DESC, id ASC`) so at minimum the winner is stable and reproducible — same rationale as the recall query's tie-break (`src/hooks/shared/recall-query.ts:27`).
4. Accept it and document it: concurrent improvements to the same skill are rare, and the next invocation's judge will catch a still-failing skill anyway (the loop is self-healing over time).

Option 3 is cheap and worth doing on its own; option 2 is the real fix.

Note this is only reachable for **org-scope** skills invoked by multiple people — the local lock already covers the common single-user case.

Guida per i contributori

Nessuna guida per i contributori indicizzata per questo repository

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.