Skip to content

feat(desktop): name the dev-profile lock holder in conflict messages - #3953

Open
rbalachandar wants to merge 2 commits into
apache:mainfrom
rbalachandar:fix/dev-profile-owner-name
Open

feat(desktop): name the dev-profile lock holder in conflict messages#3953
rbalachandar wants to merge 2 commits into
apache:mainfrom
rbalachandar:fix/dev-profile-owner-name

Conversation

@rbalachandar

Copy link
Copy Markdown
Contributor

Summary

Refs #3539 (follow-up to #3359's shared 'Maka Dev' profile owner gate). When a launch loses the single-instance lock, all three loser surfaces — the main-process loser dialog, the absorbed outcome in dev.mjs / start-dev-app.mjs, and the plain-loser stderr in dev-app-runtime.mjs — named the conflict only generically.

This resolves the holder from Chromium's own SingletonLock record (a <hostname>-<pid> symlink target in the user-data dir), never from the process table:

  • The symlink alone is not evidence — it survives SIGKILL — so a holder is reported only while its PID is still alive (kill(pid, 0), with EPERM counted as alive).
  • A hostname mismatch is reported as another machine (shared homes).
  • Any inconsistency (unreadable lock, malformed target, dead PID, module not built) degrades to undefined, and every caller falls back to the existing generic wording.
  • An explicit --user-data-dir wins over the shared default, matching launch behavior.
  • The message is hedged ("appears to be PID N"), which also covers the inherent PID-reuse race.

New packages/core/dev-single-instance-owner (parse + liveness probe + describe), exported via the package map and lazy-loaded by the launcher scripts.

This covers the "surface the actual owner" half of the issue's post-spawn handshake option; the atomic-reservation half remains open.

Verification

  • npm run build — pass
  • packages/core suite: 665/665
  • apps/desktop suite (dist/main + dev-app-runtime scripts): 1577/1577 stable run; dev-app-runtime.test.mjs 25/25 including the five new conflict-detail tests
  • npm run lint, npm run format:check, npm run typecheck — pass (exit 0)
  • Note: a handful of deadline-based desktop tests (oauth round guard, MCP import) flake under load on this machine; a clean rerun is fully green and they are untouched by this change

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code — implementation review, rebase onto current main, verification runs; human contributor of record reviewed and owns the change.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above

Closes the observability half of the shared 'Maka Dev' profile launch
race (apache#3539). When a launch loses the single-instance lock, the loser
surfaces named the conflict only generically. Resolve the holder from
Chromium's own SingletonLock record (a <hostname>-<pid> symlink target
in the user-data dir), never from the process table: the symlink alone
is not evidence — it survives SIGKILL — so a holder is reported only
while its PID is still alive (kill(pid, 0), EPERM counts as alive), and
a hostname mismatch is reported as another machine for shared homes.

- packages/core/dev-single-instance-owner: parse + liveness + describe,
  undefined on any inconsistency so every caller degrades to the
  existing generic wording.
- All three loser surfaces name the holder: the main-process loser
  dialog, the absorbed outcome in dev.mjs / start-dev-app.mjs, and the
  plain-loser stderr in dev-app-runtime.mjs. An explicit --user-data-dir
  wins over the shared default, matching launch behavior.

The message is hedged ("appears to be"), which also covers the inherent
PID-reuse race. The race's atomic-reservation half stays open.
@Astro-Han

Copy link
Copy Markdown
Contributor

I reviewed this PR at exact head ef1391344a1553360a61ebeccdc2fef916ef68f3 (base 2d10b520, merge-base 38f0a275, 10 files +395−14, 2 commits).

Standards: NO-GO — 1×P1 + 1×P2 + 2×P3

  • P1 — required test gate is red. The exact-head test run 33002208160 fails in prompt-rail, and the head commit ef139134 directly modifies that failing setup at apps/desktop/e2e/prompt-rail.spec.ts:289-293. Per CONTRIBUTING.md:38, a passing test is required before merge.
  • P2 — OS effects cross the pure-core boundary. packages/core/src/dev-single-instance-owner.ts:27-29,67-72,126-128 adds filesystem, hostname, and PID-liveness effects to a core package defined as pure contracts by ARCHITECTURE.md:69 / README.md:195; apps/desktop/README.md:120,131,136 assigns OS-facing behavior to Desktop main. Parsing can stay pure and shared, but lock reading and liveness should live in Desktop client-local code.
  • P3 — feature commit AI trailer missing. The PR discloses substantive Claude Code use, but a3263382 lacks Generated-by: Claude Code (CONTRIBUTING.md:34).
  • P3 — repair commit AI trailer missing. ef139134 likewise lacks the required trailer.

Other checks: branch/title conform, CONTRIBUTING.md:83 does not govern intermediate commit subjects, and trailing whitespace is tooling-enforced.

Spec: NO-GO — 3×P2 + 1×P3

  • P2 — foreign-host locks are validated against the local process table. packages/core/src/dev-single-instance-owner.ts:107-116 calls liveness(record.pid) before comparing hostname. A shared-home record from another machine is dropped when that numeric PID is absent locally, or falsely validated when an unrelated local process happens to have the same PID; describeDevProfileOwner():120-123 can then name a remote holder that was never verified. This violates dev: close the shared Maka Dev profile launch race (atomic reservation / ownership handshake) #3539’s requirement not to treat a remote hostname as a local PID and to degrade inconsistent data. Test 72-80 encodes the wrong order; direct repro shows both outcomes and counts two local liveness calls.
  • P2 — plain-loser lookup uses a macOS path on cross-platform plain launches. Plain launch spans platforms (dev-app-runtime.mjs:412-430), but the default lookup at 224-237 uses macOS-only DEV_USER_DATA_DIR at 74-76. Without an explicit --user-data-dir=, Linux Electron locks its platform userData while stderr inspects ~/Library/Application Support/Maka Dev; Windows has no POSIX SingletonLock. The promised plain-loser surface therefore cannot name the actual owner off macOS.
  • P2 — malformed/path-shaped targets are converted into plausible owners. parseDevProfileLockTarget():43-58 discards all preceding path components and validates no hostname characters. dev: close the shared Maka Dev profile launch race (atomic reservation / ownership handshake) #3539 authorizes only the bare <hostname>-<pid> lock record and says inconsistency should fall back. Inputs like /tmp/other-box-739 can be emitted as owners if an unrelated local PID is alive; test 42-47 explicitly blesses this widening.
  • P3 — unrelated scope creep weakens test(desktop): prompt-rail focused-turn eviction flakes in CI #3121 coverage. apps/desktop/e2e/prompt-rail.spec.ts:292-293 replaces an exact tail turn-prompt-rail-120 check with any visible last mounted turn. This can let broken scroll-to-bottom setup pass, does not fix the hosted CI, and is unrelated to dev: close the shared Maka Dev profile launch race (atomic reservation / ownership handshake) #3539.

No missing macOS loser surface was sustained; atomic reservation/handshake remains intentionally outside this naming scope.

Other checks: core 665/665 PASS, launcher 25/25 PASS, format 1690 PASS, changed-file Biome PASS; git diff --check fails only on prompt-rail trailing whitespace. Hosted audit and windows_recovery are SUCCESS; test run 33002208160 is FAILURE at the head-modified prompt-rail test (1 failed / 68 passed / 1 skipped). OPEN / MERGEABLE / BLOCKED / REVIEW_REQUIRED, no reviews/comments, head did not drift.

What I did not check: full local Desktop suite beyond the checks noted.

Gate: exact head has P1/P2/P3 findings in both axes and a failing required test check; it cannot be approved or merged until the liveness/hostname order, cross-platform plain path, target parsing, test-weakening, and trailer/gate issues are fixed and test turns green.


Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants