fix(system): do not fail memory snapshots on inaccessible processes - #2078
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens get_memory_info() (used for autoscaler CPU/memory snapshots) against psutil.AccessDenied in restricted environments, preventing the periodic system-info task from crashing and silently stopping.
Changes:
- Catch
psutil.AccessDeniedwhen reading the current process’s memory via PSS and fall back to RSS. - Suppress
psutil.AccessDeniedwhen aggregating child process memory usage. - Add unit tests covering both the child-skip and current-process RSS fallback behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/crawlee/_utils/system.py | Adds AccessDenied handling/fallbacks in memory snapshot collection. |
| tests/unit/_utils/test_system.py | Adds regression tests for AccessDenied scenarios in get_memory_info(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
On Linux, get_memory_info reads a process's PSS via memory_full_info, which psutil documents may require elevated privileges. In restricted environments (hardened containers with hidepid/restricted /proc, or an unreadable child subprocess) this raises psutil.AccessDenied, which is not a subclass of psutil.NoSuchProcess and so was not caught by the existing suppress around the child loop; the current-process read was unguarded entirely. The exception propagated out of the recurring system-info task, which has no error handling, silently stopping the autoscaler's CPU/memory snapshots. Suppress AccessDenied alongside NoSuchProcess when summing child memory, and fall back to RSS for the current process when PSS is denied. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
b3464f1 to
cd699de
Compare
|
pushed a revision addressing the latest review and ci feedback. |
|
Same as here #2074 (comment)
|
vdusek
left a comment
There was a problem hiding this comment.
Thanks for the revision. Both of my earlier comments are correctly addressed - the logger.debug for the PSS->RSS fallback, and the child RSS fallback so an unreadable child is no longer dropped.
However, the change made for the Copilot comment about guarding children() introduces a regression: wrapping the entire loop in suppress means one child dying mid-iteration now aborts the loop and drops all remaining children. I verified this locally - a repro with a dead first child and a live second child worth 40 B yields 140 B on master and 100 B on this branch. CI does not catch it because that path is untested.
Details inline.
✍️ Drafted by Claude Code
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
|
@vdusek addressed all new threads, including the child-exit regression test, rebased, and re-requesting review. thank you. |
- Move the fallback into the Linux-only `_get_used_memory`; elsewhere it re-ran the call that had just failed. - Cover a missing or zero `pss` field, not just `AccessDenied`. - Warn once about the degraded estimate instead of a debug record per snapshot, and log unlistable children. - Test the real fallback instead of only a patched `_get_used_memory`.
Inspecting a process can also fail with a bare `OSError` - psutil re-raises `FileNotFoundError` for a live process whose `/proc` entry is missing - which escaped the previous `psutil.AccessDenied` handling. Catch it wherever a process is inspected, and make the PSS to RSS fallback report what actually happened: - Warn about PSS only when psutil exposes no `pss` field at all, and latch that verdict instead of re-parsing `smaps` for every process on every sample. A single denied process falls back to RSS just for itself, and a `smaps` file that parses to zero is no longer reported as a missing metric. - Report a child that cannot be measured at all separately from one that exited. - Cap the estimate at the total memory of the machine, so summed RSS cannot pin the autoscaler in a critically overloaded state. - Lock `LoggerOnce`, which is now reached from the metric sampling thread.
This comment was marked as outdated.
This comment was marked as outdated.
|
@Mantisus could you please check this as well? |
get_memory_info()no longer raises when a process cannot be inspected. In restricted environments (hidepid, hardened containers) it failed on every sample, which meant a traceback in the log every second and an autoscaler running on a stale memory history. Processes that deny inspection, exit mid-measurement, or lose their/procentry are now skipped instead.smapsat all makes psutil aliasmemory_full_infotomemory_info, whose result has nopssfield, so reading it raisedAttributeErroron every sample. The metric is now read defensively, and the verdict is latched: such a machine is detected once instead of being re-probed for every process on every sample.smapsfile can be empty, which psutil parses to a PSS of zero. That was reported as zero bytes used, which the autoscaler reads as free memory. A PSS of zero now falls back to the RSS of the same process, whichmemory_full_info()has already read anyway.LoggerOnceis now thread-safe, since memory metrics are sampled in a worker thread - this PR adds its first caller that runs off the event loop.current_sizeis byte-for-byte whatmasterreports, with no warnings and no RSS fallbacks./proccases, both PSS fallbacks and their warnings, the latch, and a vanished process not being misreported as a denial.✍️ Drafted by Claude Code