fix(F-871): a named profile is no longer walked to -2 by a dead browser's leftovers, and the answer says when it is - #105
Merged
Conversation
Whether a Chrome profile was busy was decided by `Path.exists()` over `SingletonLock`, `SingletonSocket` and `SingletonCookie`. None of those names means what that assumed. Chrome's lock is a SYMLINK whose target is the string `<hostname>-<pid>` -- a claim about a pid, not a path -- so `exists()`, which follows symlinks, reported the one artefact that names an owner as ABSENT; while `SingletonSocket` points into a per-launch /tmp directory that a killed browser never cleans up, so its target outlived the browser and answered "busy" forever. After F-860's reaper killed a Chrome a failed spawn had leaked, the next `spawn_browser` on the same NAMED profile therefore read the residue as a running browser and walked the caller to `<name>-2` -- a different, freshly cloned identity for a profile that exists precisely to keep its cookies and logins -- and nothing in the answer said so. Measured on the 2.1.5 release gate (run 34911829422, job 104200794999): three warmup attempts all passing `user_data_dir="ci-warmup"`, the third running on `ci-warmup-2`. The question gets one home, `embedded/profile_lock.py`, which reads the lock the way Chromium's own ParseProcessSingletonLock does: a lock naming a dead pid is orphaned and holds nothing, exactly as Chrome concludes before unlinking it and starting. The socket and cookie are not consulted at all -- Chrome writes them after the lock, so a live browser always has one. A live browser's lock is now visible for the first time as well, so a held profile can no longer be handed to a second Chrome. When a walk does happen the answer says so: the selection dict gains `requested_user_data_dir`, `walked_to` and `walk_reason`, present only when the caller did not get what they asked for, and surfacing through the existing `spawn_diagnostics.profile_selection` with no second diagnostics home. Deliberately NOT done: the reaper still leaves the artefacts alone. Deleting them would cover only the locks we killed -- not a crash, an OOM kill or a reboot -- and Chromium already unlinks an orphaned lock, so it would be a second way to do something already done, from a failure handler racing a browser that is still dying. clone_storage.py ratchets DOWN 1057 -> 1055 (cap == actual). Four SOFT goldens that wrote the literal bytes "lock" into a SingletonLock and called it a running browser are updated in place, with the reason inline: those bytes are what Chromium calls an INVALID lockfile. tests/fakes.py gains the ONE artefact writer (`write_singleton` / `held_profile`). Finding: audit/stage2/finding_F871_stale_singletonlock_walks_named_profile.md
… the warning Six nits from the review of ca06c2a, no behaviour outside them. 1. `profile_lock`'s "two witnesses, in order, and no third" was POSIX-only. An **On Windows** paragraph now says what is true there: Chrome writes no `Singleton*` at all, it takes a `lockfile` with FILE_FLAG_DELETE_ON_CLOSE that the kernel removes even on a hard kill, so witness 2 is vacuous and the process scan carries the answer alone. Its mere PRESENCE is deliberately not promoted to a third witness: a presence test is the exact reasoning this finding condemns, and a power loss leaves a `lockfile` behind forever with no pid in it to check. 2. The error direction was not uniform. `_pid_alive` counted an unreadable pid as ALIVE (toward held) while a FAILING process scan resolved toward free -- covered by the lock on POSIX, covered by nothing on Windows. `_browser_pids` now distinguishes "could not be asked" (`None`) from "asked, nothing running" (`()`), and where there is no second witness the former is a `Hold(None, ...)`: one extra walk is survivable, two browsers on one profile is not. `tests/test_profile_pid_check.py` pinned main's non-uniform answer; what it exists to guard (the failure is survived, not raised) is untouched and it is now expressed per-platform, with the reasoning in its docstring. 3. `except TypeError: continue` was the str-vs-Path signature probe carried over from main, and it silently turned a TypeError raised INSIDE a working scan into "no browsers". The probe is kept, the exception is remembered, and exhausting both forms now warns and returns "never really asked". 4. "One home" was overstated: `clone_storage._REGENERABLE_PROFILE_NAMES` still names all three artefacts (plus `lockfile`) for the different question of what a clone must not copy and what a trim may delete. The module docstring and the CLAUDE.md row now claim the one home for INTERPRETING them. 5. `walk_reason` sat quietly in the selection dict beside an unconditional, prominent `warning` about the named profile persisting -- a model reads the loud field and never learns it was handed a different profile. The reason is now PREPENDED to that same warning ("NOT the profile you asked for: ... is in use (...), so this spawn got ... -- a DIFFERENT profile, freshly cloned, with none of the cookies or logins the requested one holds."), keeping the standing advice after it. Same field set, no second diagnostics home. Pinned both ways: a walk leads the warning, no walk leaves it byte-for-byte as it was. Finding section 6 no longer calls this an open product call. 6. `_lock_content` stripped the plain-file branch but not the symlink branch, for two forms its own docstring calls equivalent. Both strip now. profile_lock.py 196 -> 251 LOC (leaf, under the 1000 default); browser_management.py 438 -> 453; clone_storage.py unchanged at 1055/1055 (cap == actual). Unit lane 2416 passed, 1 skipped.
The substitution warning promised "a DIFFERENT profile, freshly cloned". Only the FIRST walk to a given name clones: `_next_available_explicit_dir` returns the first non-busy `<name>-N`, and `resolve_profile_selection` skips the copy when that directory already exists -- which is exactly the CI shape, where `ci-warmup-2` pre-existed. A second walk therefore hands back whatever an earlier one left there, and telling the caller it is fresh is a claim the code does not make good on. The warning now says "a DIFFERENT profile, either a fresh clone of the master snapshot or one an earlier walk left behind, with none of the cookies or logins the requested one holds", and the pin asserts that wording AND that the old promise is gone. The same overclaim is corrected where it was repeated: the finding's severity line and its section 6 quote, the CHANGELOG entry, and `test_stateful_i18n`'s barrier docstring (prose only -- that file is the integration tier and its assertions are untouched).
CHANGELOG: the F-871 entry is placed below the two F-872 entries; every Unreleased entry from main is kept.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect (F-871)
A NAMED profile silently became a different profile. In CI run 34911829422 (integration Linux) three spawn attempts all passing
user_data_dir="ci-warmup"ran onci-warmup,ci-warmup, thenci-warmup-2after an F-860 spawn-leak reap. A named profile exists precisely to keep cookies and logins; nothing in the answer said the caller had been handed another one.Root cause, measured:
clone_storagedecided "busy" withPath.exists()overSingletonLock/SingletonSocket/SingletonCookie. Chrome'sSingletonLockis a DANGLING symlink whose target is the string<hostname>-<pid>, soexists()never saw the one artefact that names an owner, whileSingletonSocketpoints into a per-launch temp dir a KILLED Chrome never cleans, so its target outlives the process and reads as "in use". Full write-up:audit/stage2/finding_F871_stale_singletonlock_walks_named_profile.md.The fix
New leaf
embedded/profile_lock.py, THE one home for interpreting Chrome's profile-hold artefacts:profile_hold(profile_dir) -> Hold(pid, reason) | Nonereads the lock the way Chromium does (lexists+readlink, split on the last-, host must match, pid must be alive), with the process-table scan as the other witness. On Windows, where Chrome writes noSingleton*and itslockfileis delete-on-close, the scan is the only witness, and a scan that cannot be asked resolves toward "held" (one extra walk is survivable; two browsers on one profile is not). When a walk does happen,spawn_browser's answer now leads itswarningwith the substitution and the reason, andprofile_selectioncarries the requested dir, the walked-to dir, andwalk_reason.clone_storage.py1057 → 1055 (grandfather row ratcheted down);browser_manager.pyandprocess_cleanup.pyuntouched.Evidence
assert 'occupied-2' == 'occupied'(a residual socket walked a free profile) andassert 'occupied' == 'occupied-2'(a live lock did not walk); verified by the reviewer against main's code.Not fixed here (named)
-2directories are not migrated; the four JS-aspect{"error": ...}returns are F-872's business, not this branch's.