activeloopai / activeloopai/hivemind

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

Ouverte
#340 0 commentaires 0 réactions 0 personnes assignées Voir sur GitHub
Langage dominant
TypeScript
Étoiles
1.6k
Forks
107
Merge moyen
17 h 30 min
PR mergées (30 j)
6

Description

`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.

Guide de contribution

Aucun guide de contribution indexé pour ce dépôt

Évaluation

Cette issue n'a pas encore été évaluée.

Recevez les nouvelles issues par e-mail

Un résumé court des issues GitHub adaptées aux débutants.