[Bug] memos-local-plugin: skill rebuild overwrites trial outcomes recorded while it awaits the LLM (lost trials_attempted/trials_passed, undone promotions/archivals) #2556

Pre-submission checklist | 提交前检查

• I have searched existing issues and this hasn't been mentioned before | 我已搜索现有问题，确认此问题尚未被提及
• I have read the project documentation and confirmed this issue doesn't already exist | 我已阅读项目文档并确认此问题尚未存在
• This issue is specific to MemOS and not a general software issue | 该问题是针对 MemOS 的，而不是一般软件问题

Bug Description | 问题描述

runSkill (core/skill/skill.ts) snapshots every skill row once per run (buildSkillIndex, L75). It then awaits an LLM crystallize call (runCrystallize, L134) and an embed call (buildSkillRow, L184), and finally writes a full row derived from that snapshot (repos.skills.upsert(row), L212). The packager copies trialsAttempted/trialsPassed from the snapshot (core/skill/packager.ts L84-85 → L112-113).

While the run is suspended in either await, a reward.updated event can resolve pending skill trials. resolveTrialsForReward (core/skill/subscriber.ts L221, called at L176) calls applySkillFeedback (core/skill/skill.ts L285), which synchronously reads the skill, applies trial.pass/trial.fail, and upserts new trials_attempted, trials_passed, eta and possibly status (a promotion or archival from applyTrial in core/skill/lifecycle.ts). When the rebuild resumes, its upsert writes the stale counters back.

Because skills.upsert is INSERT OR REPLACE (core/storage/repos/skills.ts L63-64, core/storage/tx.ts L76), the stale row replaces the whole record. The trial outcome is lost permanently: the skill_trials row stays resolved (pass/fail), so it's never re-applied, but the skill's counters never include it. Any promotion or archival that the trial caused is undone too. A skill archived mid-run is re-written as candidate, because buildSkillIndex filters archived rows only at snapshot time (L354). The same full-row replace also discards exposure_count, usage_count and the last_* timestamps written by other paths during the await.

Why this is not two feedback calls racing. applySkillFeedback itself is synchronous end-to-end (better-sqlite3, no await between getById L291 and upsert L305), so two feedback applications can't interleave inside one process. The lost update needs a writer that holds a row across an await, and the rebuild path is that writer.

How this was found: by reading the code. It has not been reproduced at runtime, and no lost update has been observed on a running install. Paths are relative to apps/memos-local-plugin/; line numbers are at main 64388b50, and the cited files are identical on dev-v2.0.36 0e58d0e9.

How to Reproduce | 如何重现

Requires algorithm.lightweightMemory.enabled: false. The default is true, and in that mode the skill subscriber is not attached (core/pipeline/deps.ts L252-261).

Expected interleaving, in a single process:

1. A skill run starts (any trigger: l2.policy.induced, l2.policy.updated, or a reward-scoped run) and snapshots skill S at trials_attempted=3, trials_passed=2. Eligibility picks rebuild for S (core/skill/eligibility.ts, policy.updatedAt > skill.updatedAt).
2. The run hits await runCrystallize(...).
3. reward.updated for an episode in which S had a pending trial arrives. resolveTrialsForReward → skillTrials.resolve → applySkillFeedback(S, "trial.pass") → DB row is now 4/3, with a new η and possibly status=active. triggerRun only queues, because a run is in flight (subscriber.ts L108-121).
4. The run resumes and upserts S with 3/2, status=candidate, and η recomputed from the stale row. The trial is gone.

To exercise it deterministically, use an LLM stub that holds runCrystallize open while a reward.updated event resolves a pending trial for the skill being rebuilt, then release the stub and compare trials_attempted with the skill_trials rows. These steps have not been run.

Environment | 环境信息

• memos-local-plugin source at main 64388b50 and dev-v2.0.36 0e58d0e9
• Not specific to an OS or Node version; better-sqlite3 is synchronous, and the window comes from the awaits in runSkill

Additional Context | 其他信息

• status: "candidate" and the η recompute on rebuild are intentional (comments at skill.ts L201-206). The defect is that they're computed from a row that can be minutes old, so feedback committed meanwhile is erased.
• The doc comments assert atomicity that isn't there: core/skill/lifecycle.ts L16 ("the orchestrator calls these helpers inside a tx") and core/skill/skill.ts L20-21 ("every write is a repo.upsert call that the transaction wrapper keeps atomic"). No transaction wraps these writes, and a transaction couldn't span the awaits anyway.
• The per-policy reward scheduling with cooldown (subscriber.ts L123-157) reduces how often a run is in flight, but doesn't remove the window. Runs triggered by l2.policy.* events, or a reward run for the same policy, still overlap trial resolution.
• skills.bumpTrial / updateTrials (core/storage/repos/skills.ts L69-74, L131+) is a column-scoped trial writer that has no callers.
• Not reachable in lightweight mode (algorithm.lightweightMemory.enabled), where the skill subscriber isn't attached (core/pipeline/deps.ts L252-261).

Detection. A skill whose counter is below its resolved trial rows has lost an update (> is legitimate, < should be impossible):

SELECT s.id, s.name, s.trials_attempted, r.resolved
FROM skills s
JOIN (SELECT skill_id, count(*) AS resolved
      FROM skill_trials WHERE status IN ('pass','fail')
      GROUP BY skill_id) r ON r.skill_id = s.id
WHERE s.trials_attempted < r.resolved;

Related but distinct: #2548 (trials never recorded), #2366 (trial recording bypassed at turn start), #2363 (the same INSERT OR REPLACE leaving FTS orphans).

Suggested direction. Would appreciate maintainer input on which fits the intended design:

1. Split ownership of lifecycle columns. Feedback writes trials_attempted, trials_passed, eta, status and updated_at through a column-scoped UPDATE (the existing bumpTrial/updateTrials shape). The rebuild upsert switches from INSERT OR REPLACE to ON CONFLICT(id) DO UPDATE and keeps skills.<col> for columns it doesn't own (trial counters and exposure/usage counters). If the rebuild needs to reset η or status, it does so by deriving from the current row, as in (2).
2. Re-read before write. Immediately before repos.skills.upsert(row) (synchronously, with no await between), re-read the existing row and carry over trials_* (and recompute η from the fresh row). This closes the window in-process with a small diff. Using BEGIN IMMEDIATE around the read+write would also cover multiple processes sharing one DB.

Option 1 also fixes the exposure/usage-counter overwrite, and it avoids the delete-and-reinsert path that #2363 ran into, since an ON CONFLICT DO UPDATE updates the row in place.

Happy to contribute a PR once maintainers indicate the preferred direction.

Willingness to Implement | 实现意愿

• I'm willing to implement this myself | 我愿意自己解决
• I would like someone else to implement this | 我希望其他人来解决