Repository navigation
fix(tests): treat ESRCH as a vanished process in release gate probe - #2505
jasondeng1997 wants to merge 1 commit into
Conversation
The parent-term cancellation check races with PDEATHSIG cleanup: a dying descendant can vanish between opening /proc/<pid>/stat and reading it, which raises ProcessLookupError (ESRCH) instead of FileNotFoundError. Observed on CI (unit 3.12): 8122 passed, only test_upgrade_command_owner_finishes_cleanup_after_gate_cancellation[parent-term] failed. Catch OSError so a reaped process simply counts as absent.
| try: | ||
| stat = Path(f"/proc/{pid}/stat").read_text() | ||
| except FileNotFoundError: | ||
| except OSError: |
There was a problem hiding this comment.
[P2] 放宽到 OSError 会把读失败当成「进程已消失」,让存活后代的断言静默通过
ESRCH 这个诊断本身是对的:我在 WSL 上复现了同一条路径(open('/proc/<pid>/stat') 后 kill 该子进程再 read()),三次读取均抛 OSError/ProcessLookupError(errno 3),Path.read_text() 同样如此。所以补 ProcessLookupError 是必要的。
问题在捕获范围。_process_exists() 直接以 _process_identity(pid) == identity 作为「仍存活」判据,而本行现在把 EACCES、EMFILE、EIO、EBADF 一律变成 return None,等价于「该 pid 不是记录中的进程」。后果是本文件第 938 行那条泄漏断言(assert time.monotonic() < deadline, "Detached descendants survived gate termination")与第 951 行 assert not any(...) 在读取失败时直接放行,而这两条恰恰是用来证明 gate 终止后没有残留后代的。
仓库内同类读取的既有写法是窄口径:
src/powercontext/source_management/background.py:50:except (FileNotFoundError, ProcessLookupError): return Falsesrc/powercontext/source_management/durable.py:82:except (FileNotFoundError, ProcessLookupError): return True
建议改为 except (FileNotFoundError, ProcessLookupError):,与生产侧保持一致;若确实想容忍其它 OSError,请在 return None 前区分「进程不存在」与「读取失败」,后者应让断言失败而不是当作已消失。
There was a problem hiding this comment.
Confirmed on 3948f2e with real Linux processes, without mocking Path.read_text(): after recording a live child's start time, I lowered RLIMIT_NOFILE to 64 and exhausted the remaining descriptors. os.kill(pid, 0) still succeeded, but _process_exists(pid, identity) returned False, so the cancellation test's assert not any(...) passed for that live child. The parent commit instead raised OSError(EMFILE); restoring descriptors made the same child detectable again.
I also reproduced ProcessLookupError(ESRCH) by opening /proc/<pid>/stat, terminating and reaping the child, then reading the open file. Please catch (FileNotFoundError, ProcessLookupError) so this race is handled without turning unrelated probe failures into evidence of successful cleanup.
There was a problem hiding this comment.
Update: the narrow ESRCH fix is now on master via #2396, in commit 9862ef4b. _process_identity() already catches (FileNotFoundError, ProcessLookupError) there, handling the exit race while allowing unrelated read failures to surface.
This PR is still at 3948f2ee with the broad OSError catch and the previously demonstrated live-process false negative. Since the intended fix has already landed, I suggest closing this PR as superseded; retaining its broad catch when reconciling with master would weaken the assertion again.
hidb4ai
left a comment
There was a problem hiding this comment.
The ESRCH fix is needed, but catching every OSError weakens the descendant-cleanup assertion. I confirmed a live-process false negative with real descriptor exhaustion and added the reproduction to the existing P2 thread. Please limit the absent-process case to FileNotFoundError and ProcessLookupError.
hireview001
left a comment
There was a problem hiding this comment.
- CI failed: CI result (failure).
- CI failed: Upgrade release gate (failure).
- CI failed: CI preflight (failure).
- CI failed: Main (failure). Failed steps: CI preflight / Check License Header; Upgrade release gate / Require the selected parallel acceptance workflow; CI result / Require successful selected checks.
knqiufan
left a comment
There was a problem hiding this comment.
The existing process-probe finding remains reproducible: an unrelated OSError such as EMFILE is treated as a vanished process. Please address the existing thread before merging.
|
Closing as superseded by #2396, which has already merged the targeted ESRCH fix into master. The process probe now catches FileNotFoundError and ProcessLookupError while preserving failures for unrelated OSError exceptions, so this PR is no longer needed. |
Flaky test: ESRCH race in the release gate process probe
test_upgrade_command_owner_finishes_cleanup_after_gate_cancellation[parent-term]failed once on CI (unit 3.12, run 42148875675) with:8122 other tests passed, so this is not a regression in behaviour — it is a race in the probe helper: PDEATHSIG cleanup reaps the descendant between the
openand thereadof/proc/<pid>/stat, andreadthen fails withProcessLookupError(ESRCH) rather thanFileNotFoundError. The helper already intends to treat a vanished pid as "absent" (PID-reuse guard), so catchingOSErrorinstead ofFileNotFoundErrormakes it do exactly that.One-line change, no behaviour change beyond removing the flake.