Skip to content

Fix Monitor metrics history never being trimmed to max_size#1474

Open
JSap0914 wants to merge 1 commit into
guidance-ai:mainfrom
JSap0914:fix-monitor-metrics-trim
Open

Fix Monitor metrics history never being trimmed to max_size#1474
JSap0914 wants to merge 1 commit into
guidance-ai:mainfrom
JSap0914:fix-monitor-metrics-trim

Conversation

@JSap0914

Copy link
Copy Markdown

The bug

Monitor._monitor_fn is supposed to cap each metric history at max_size
entries:

for metrics in self.metrics_dict.values():
    if len(metrics) > self.max_size:
        metrics = metrics[-self.max_size :]

metrics = metrics[-self.max_size:] just rebinds the local loop variable to a
new sliced list; it never writes back into self.metrics_dict. The stored
lists are therefore never trimmed, so CPU_USAGE/MEM_USAGE/GPU histories grow
without bound for the lifetime of the monitor and max_size has no effect.

The fix

Trim in place with slice assignment so the list object referenced by
metrics_dict is actually shortened, and pull the logic into a small
Monitor._trim_metrics helper so it can be unit-tested directly without
spinning up the async monitoring loop.

Verification

Added tests/unit/test_metrics.py covering both that over-long lists are capped
to the most-recent max_size entries and that the stored list object is mutated
in place.

$ python -m pytest tests/unit/test_metrics.py -q
..                                                                       [100%]
2 passed

Before the fix these tests fail (the lists keep all appended values).

The trim step in Monitor._monitor_fn rebound the loop variable with a
sliced copy (metrics = metrics[-self.max_size:]) instead of mutating the
list stored in metrics_dict, so the histories grew without bound and
max_size had no effect.

Extract the logic into Monitor._trim_metrics and trim in place via slice
assignment so the stored lists are actually capped. Add unit tests for
the trimming behavior.
Copilot AI review requested due to automatic review settings June 16, 2026 08:05

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@ErenAta16 ErenAta16 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Textbook version of the "rebinding a loop variable doesn't mutate the container" bug: metrics = metrics[-self.max_size:] only repoints the local name at a new list, self.metrics_dict[key] still holds the original untrimmed one. The fix uses metrics[:] = ... (slice assignment), which mutates the existing list object in place, so anyone else holding a reference to that same list (like metrics_dict) sees the trim too.

The two tests are the right pair: one checks the actual trimming behavior (keeps the most recent max_size entries), the other checks identity (monitor.metrics_dict[cpu] is stored) specifically to catch a regression back to rebind-instead-of-mutate, which the first test alone wouldn't necessarily catch if someone "fixed" it by reassigning self.metrics_dict[key] = ... instead (that would also produce a correct value, just not the same object, and would matter if anything else was holding a reference to the pre-trim list). Good fix, good tests.

@ErenAta16 ErenAta16 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Classic Python trap and a real bug: metrics = metrics[-self.max_size:] inside the loop only rebinds the local metrics name to a new list — it never assigns back into self.metrics_dict[key], so the dict keeps referencing the original, ever-growing list. Ran the new test_metrics.py — 2 passed, and pulling the trim into a standalone _trim_metrics helper is a good call since it lets the test exercise the actual bug without needing to spin up the async monitoring loop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants