Orphaned profile writer lock silently drops the legacy settings.yaml migration (settings lost, welcome notice cannot be acknowledged)

[zwrong]

Environment: @deepseek-ai/dsh 0.1.7-alpha.2 (npm global), Node.js v25.1.0, macOS 15.4.1, profile: web

Summary

An orphaned writer lock (<profile>/package.json.lock left behind by a killed process) makes the one-shot legacy settings.yaml migration fail silently and permanently. The user's settings (theme, model defaults, welcome-notice acknowledgement) are dropped, the welcome notice reappears, and acknowledging it fails with a generic "please try again" that can never succeed.

I believe the migration behavior is a bug; the lock timeout reporting and the UI error copy are related rough edges that made this very hard to diagnose.

What happened

1. A dsh plugin --profile web add ... process was killed (timeout) while it held the profile writer lock. The lock file survived with the dead PID inside:

   $ cat ~/.dsh/profiles/web/package.json.lock
   20702
   $ ps -p 20702   # dead
   

2. On the next dsh web boot, SettingsForms.importLegacyDocument() (in @deepseek-ai/dsh-settings) runs:

   await rename(path, imported);            // settings.yaml → settings.yaml.imported  (succeeds)
   for (const [section, values] of Object.entries(sections ?? {})) {
       try { await this.update(ns, values); }
       catch (error) { this.ownerContext.logger.warn(...); }   // every section throws
   }

   Each update() goes through withFileLock(.../package.json) → waits 2s → atomic-write: timed out waiting for the writer lock at .../package.json.lock → caught → logger.warn → next section. All five sections were dropped.
3. Because the file is renamed before the imports, the migration never runs again ("a partial import never repeats"). The settings are gone for good — I had to notice the missing theme, manually rename .imported back, delete the stale lock, and reboot to recover.
4. Symptoms in the UI:
   • The 内测声明 / welcome notice reappears even though welcomeNoticeVersion was already acknowledged in the old file.
   • Clicking the confirm button shows "暂时无法保存确认状态，请重试。" — retrying can never work, because every settings write hits the same orphaned lock.
   • Raw API response (found by driving the page headlessly) :

     {"code":"settings/rejected","message":"atomic-write: timed out waiting for the writer lock at /Users/<u>/.dsh/profiles/web/package.json.lock","details":{"ns":"ui-settings-general"}}

Repro steps

1. Hold the profile writer lock from any process and kill it (e.g. interrupt dsh plugin add with a SIGKILL), leaving <profile>/package.json.lock behind with a dead PID.
2. Put a legacy ~/.dsh/settings.yaml in place (pre-0.1.6 layout) and start dsh web.
3. Observe: the file is renamed to settings.yaml.imported, no section is imported, nothing is printed to stdout, and the migration will never be retried.

Why I think this is a bug

1. Migration is not failure-atomic. Renaming the source file before knowing the imports succeed converts any transient write failure into permanent, silent data loss. Suggestion: import all sections first and rename only after full success — or rename back on failure — so a later boot can retry.

2. The failure is invisible. Neither logger.info("settings: imported ...") nor the logger.warn for a failed section appeared on stdout in my runs (the success line never appeared either, even when the import worked). A migration that can lose user settings should be loud — or logged to a file.

3. The lock timeout error is not actionable. withFileLock already writes the holder's PID into the lock file, but the timeout error doesn't include it. Not auto-deleting orphan locks is a defensible choice ("orphan recovery is an operator action"), but then please include the PID and whether it is still alive in the error:

   timed out waiting for the writer lock at .../package.json.lock (held by pid 20702, not running); remove the lock file to recover

   Without this, diagnosis took several steps that an ordinary user would not know to perform.

4. The UI swallows the real error. settings/rejected carries a precise message, but the welcome notice shows only a generic "请重试" for a condition that retries cannot fix. Passing the server message through (at least in a details/tooltip) would have made this a 10-second fix.

Current workaround (for anyone hitting this)

LOCK=~/.dsh/profiles/web/package.json.lock
[ -f "$LOCK" ] && ps -p "$(cat "$LOCK")" >/dev/null 2>&1 || rm -f "$LOCK"   # only if the PID is dead
mv ~/.dsh/settings.yaml.imported ~/.dsh/settings.yaml                       # if migration half-ran
# restart dsh web — the migration re-runs and imports everything

Was the rename-before-import behavior intentional? Happy to open a PR for (1) and (3) if it helps.

[PerryLink]

Your diagnosis holds on every point I could check — and yes, the rename-before-import is deliberate (it's documented as such)

1. Your quote is byte-exact, and the ordering is intentional

packages/settings/settings/src/index.ts:238-258:

/** Move the sections of the removed `settings.yaml` into the active profile once the Loader has settled every entry.
 * The document is renamed before the first write, so a partial import never repeats; a section the running
 * composition rejects is logged and remains only in the renamed file. */
private async importLegacyDocument(): Promise<void> {
  const profile = this.ownerContext.profileContext
  const path = join(profile.home, 'settings.yaml')
  if (!existsSync(path)) return
  const imported = `${path}.imported`
  await rename(path, imported)
  const sections = parse(await readFile(imported, 'utf8')) as Record<string, object> | null
  for (const [section, values] of Object.entries(sections ?? {})) {
    const ns = LEGACY_SECTION_ENTRIES[section] ?? section
    try {
      await this.update(ns, values)
    } catch (error) {
      this.ownerContext.logger.warn('settings: section %s of %s was not imported into entry %s', section, imported, ns)
      this.ownerContext.logger.warn(error)
    }
  }
  this.ownerContext.logger.info('settings: imported %s into profile %s', imported, profile.name)
}

So: yes, the rename-before-import is by design, and the doc comment names exactly the property it buys — "a partial import never repeats". But note what that property assumes: that the import cannot fail. Once update() can fail for a transient reason, "never repeats" silently becomes "never recovers". That's the trade you identified, and I think your framing is right.

2. Your lock analysis is confirmed down to the numbers

packages/util/atomic-write/src/index.ts:

const DEFAULT_LOCK_WAIT_MS = 2_000
...
const lockPath = `${filename}.lock`
const deadline = Date.now() + (options?.waitMs ?? DEFAULT_LOCK_WAIT_MS)
...
await writeFile(lockPath, `${process.pid}\n`, { mode: 0o600, flag: 'wx' })
...
if (Date.now() >= deadline) {
  throw new Error(`atomic-write: timed out waiting for the writer lock at ${lockPath}`)
}

• 2 seconds — exactly the timeout you measured.
• The lock file does receive the holder's PID, as you found.
• But nothing ever reads it back. I searched: the file is written and never read. There is no liveness check, no mtime expiry, no sweep. So a lock left behind by a dead process blocks every settings write forever — which is precisely what happened to you.
• And the timeout error carries only the path, never the PID.

⇒ Your suggestion (3) is cheap and well-founded: the PID is already sitting in the file. Something like "…(held by pid 20702, not running); remove the lock file to recover" costs one read plus a process.kill(pid, 0) probe.

3. A second defect you didn't mention: the success line is unconditional

Line 257 logs settings: imported … into profile … after the loop, no matter how many sections failed. In your run that means the code would have logged both "section X was not imported" and "imported …" — actively misleading for anyone reading the log later (and it contradicts your own note that the success line didn't appear; that part is a logging-level question). Worth folding into your report: that line should be conditional, or should report the success/failure counts.

4. On your four suggestions

1. Import first, rename after (or rename back on failure) — right, and it is compatible with the documented intent if you make the rename the commit point: import every section, rename only when all succeeded. You keep "never repeats" for the success path and only relax it for partial failure. The only reason to keep today's order is if re-applying an already-applied section were harmful — for settings values (latest-wins) that looks idempotent, so I don't see the harm.

2. Make it loud — agreed. A migration that can drop user settings should not depend on a warn reaching somebody's console. Surfacing it in the UI is better still.

3. Put the PID (and its liveness) in the lock error — agreed, see above.

4. Pass the server message through to the UI — agreed, and the information is definitely there: packages/api/settings-controller/src/index.ts:267 builds exactly the error you captured —

   return new RemoteError('settings/rejected', messageOf(error), { ns }, { cause: error })

   while the button shows a hard-coded string, packages/client/ui-settings-models/src/client/locales.ts:229:

   welcomeError: '暂时无法保存确认状态，请重试。',

   So the precise message exists server-side and is thrown away client-side, for a condition that retrying can never fix.

5. Why the welcome notice is the thing that broke (it closes the loop)

That acknowledgement is not stored in settings.yaml any more — it is a volatile Config field of the ui-settings-general entry:

welcomeNoticeVersion: Volatile<string | undefined>     // packages/client/ui-settings-general/src/index.ts:11
welcomeNoticeVersion: z.string().volatile(),            // :16

and packages/client/ui-settings-models/README.md:78 describes the flow: "on loopback it compares and writes ui-settings-general.welcomeNoticeVersion through the shared configuration form". Writing a Config field of the live profile is exactly the operation that goes through the profile's package.json writer lock — the same lock your orphaned file is holding. So: orphaned lock → every settings write times out → the acknowledgement can never be recorded → the notice returns forever, with a generic "try again". Everything you observed follows from the one stale file.

6. One thing to change in your report: the PR offer

CONTRIBUTING.md states it plainly:

DeepSeek Harness is still at an early stage and under active development. We are sorry that we cannot accept external pull requests at the moment.

So a PR would not be accepted — posting the diagnosis and the patch in the discussion, as you have, is the right channel (the same document says the team reads Discussions and weighs them when allocating resources). If you want to make it maximally actionable, include the four changed lines for (1) and the two-line change for (3) inline, plus the importLegacyDocument path and line numbers.

Your workaround is also correct as written, and I'd keep the "only if the PID is dead" guard — deleting a live holder's lock is how you get two writers.

[PerryLink]

补一条：我刚把"失败是不可见的"这条查得更深了，结论是比你说的还要糟

我在自己的答复里说了"warn 不该依赖有人去看控制台"。补一条机制——因为它改变了你第 2 条的严重程度：

那些 warn 不是"没打印出来"，而是根本没被记录下来。 默认日志器只挂了一个内存环形缓冲导出器，而默认阈值是 INFO，于是 warn 在进入缓冲之前就被丢掉了：

export const enum LoggerLevel { ERROR = 0, INFO = 1, WARN = 2, DEBUG = 3 }
...
const targetLevel = exporter.levels?.[this.name] ?? exporter.levels?.default ?? this.level ?? LoggerLevel.INFO
if (targetLevel < level) continue

targetLevel = 1，而 warn 的 level = 2 ⇒ 1 < 2 ⇒ 丢弃。（vendor/cordis/src/logger.ts。）而且生产组合里根本没有挂 logger-console（我全仓检索 logger-console，只在 session-telemetry-otel 的测试夹具里出现）。仓库自己也知道这件事——apps/cli/tests/profiles/web/tests/web-failure-matrix.expected.e2e.ts 里写着一句 "WARN is above their default INFO threshold"。而 dsh web 唯一的 stdout 输出就是那行 URL。

所以你观察到的"stdout 什么都没有、成功那行即使成功也不出现"是准确的，而且是轻描淡写：info 那行能过阈值、会落进缓冲，但没有任何东西会去读那个缓冲。

因此第 2 条的最小可靠修法不是调日志级别，而是让 importLegacyDocument 直接往 stderr 写。 顺便：这场迁移在 docs/** 里没有任何文档（git grep "settings.yaml" -- docs 零命中），只在包 README 里；而且没有任何测试覆盖"某个 section 因锁被占住而导入失败"这条路径——这正是你踩到的那个模式。

另外修正我自己对第 4 条的轻描淡写

我上一条说"服务端消息是现成的、被客户端丢掉了"，这话没错，但我把它说得太轻了。查下来它是在更下面一层被丢的：

// packages/client/ui-settings/src/client/config-form.ts:139-143
const response = await this.ctx.remote.settings.mutate(this.spec.namespace, ownedOps, revision)
if (!response.ok) {
  await this.recover(generation)
  return false            // ← response.error（含 atomic-write 原文）在这里被丢弃
}

那个契约只有布尔值（mutate(...): Promise<boolean>），所以把消息透到 UI 需要改返回类型并串起 4 个文件 + 1 个测试，不是改一句文案。（而且当前这个泛化行为被一个测试钉住了：tests/welcome-notice.client.spec.tsx 里断言 settings/rejected 会渲染成那句通用的 welcomeError。）所以第 4 条要做就得当成契约改动来做——这不影响它值得做，但影响它的工作量估计。

还有一个对比，可能对"该等多久"的讨论有用

那把锁的另一个消费者等的是 120 秒（插件管理器在同一个 package.json 上用的是 lockWaitMs: 120000），而这条设置路径用的是默认的 2 秒。2 秒这个默认值是为"读—渲染—改名"这种短操作定的，而这里是一个五段的迁移循环。这不是你现在这个问题的解（孤儿锁等多久都没用），但它说明"超时值本身也偏紧"——如果你要提改进，这条可以一并附上。