Skip to content

processes: sort the process list on converted values (fix #3830) - #3831

Merged
nicolargo merged 2 commits into
nicolargo:developfrom
santhiprakash:fix/3830-sort-on-converted-fields
Oct 31, 2026
Merged

nicolargo merged 2 commits into
nicolargo:developfrom
santhiprakash:fix/3830-sort-on-converted-fields

Conversation

@santhiprakash

Copy link
Copy Markdown
Contributor

Description

Problem. With --sort-processes io_counters, /api/5/processlist returns the process with the most disk IO at the bottom of the list; with --sort-processes cpu_times the list comes back in CPU% order. Reported and diagnosed in #3830.

Root cause. GlancesProcesses.update() sorts at processes.py:689 on the raw as_dict() rows, before the per-process loop runs get_io_counters() and the namedtuple→dict conversion. At sort time io_counters is still a pio namedtuple — psutil's pio(read_count, write_count, read_bytes, write_bytes, …) puts the syscall counts where _sort_io_counters expects the previous-cycle byte totals, so a heavy writer scores count − bytes ≪ 0 and sinks to the bottom. cpu_times is a pcputimes namedtuple with no .get(), so _sort_cpu_times raises inside sorted() and sort_stats silently falls back to cpu_percent for the whole list.

The TUI hides this because _apply_live_sort re-sorts the post-conversion snapshot every cycle; the REST API and WebUI serve the engine order directly (get_list()), so they expose the pre-conversion sort.

Fix. In update():

  • A first pass now materializes everything a sort key can point at — get_pid_time_and_status, get_io_counters, maybe_add_cached_stats (cached username/cmdline also only exist after this pass), and cpu_times._asdict() — before sort_stats runs.
  • update_processcount moves ahead of that pass since it counts the raw status strings (the pass truncates status to one letter).
  • The extended-stats loop still runs after the sort, so position keeps matching the displayed order (why the early sort exists, Process widget is broken in Homepage since a few dev release #3400). is_extended_pid (v5 pin) is order-independent.

Verification.

  • New tests/test_processes_sort_order.py (4 tests): fake build_process_list rows in the raw psutil shapes. All 4 fail on the pre-fix code (writer last / CPU% order) and pass after.
  • Live check, glances -w --sort-processes io_counters: /api/5/processlist is strictly monotonic desc on read−read_old+write−write_old across 371 rows; a busy writer rises to position 0 within one cycle. POST /api/5/processes/sort/cpu_times → monotonic desc on user+system (Spearman vs cpu% order 0.992, no longer identical). username also monotonic.
  • pytest tests/test_processes_extended.py test_sort_missing_values.py test_processes_sort_order.py test_alerts.py test_routes.py test_plugin_processlist.py test_program_aggregation.py test_curses.py → 569 passed. Full pytest tests/ → 4223 passed; the only failures are pre-existing environment gaps (test_plugin_containers needs the docker package; test_webui_browser needs a Playwright browser), both red on a clean tree.
  • pre-commit run on the touched files: all hooks pass.

Risks. One extra pass over the process list per cycle (~O(n) dict ops, trivial next to the psutil calls). get_io_counters still runs exactly once per process per cycle, so io_old accounting is unchanged.

Resume

AI-assisted. I wrote and verified this change.

update() sorted the raw as_dict() rows at line 689, before the
per-process loop ran get_io_counters() and the namedtuple->dict
conversion. At that point io_counters is still a pio namedtuple
(indexes 2/3 are read/write *counts*, not the previous-cycle byte
totals the sort lambda subtracts) and cpu_times a pcputimes
namedtuple without .get() — so --sort-processes io_counters ordered
on garbage that pushes the busiest process to the bottom of
/api/5/processlist, and cpu_times sorting raised inside sorted()
and silently fell back to cpu_percent order.

Do a first pass that materializes the metadata and converts the
fields the sort keys can point at (io_counters list, cpu_times dict,
cached username/cmdline), then sort on that. The extended-stats loop
keeps running after the sort so position still matches the
displayed order (issue nicolargo#3400). update_processcount moves ahead of
the metadata pass since it counts the raw status strings.

Fixes nicolargo#3830

Copy link
Copy Markdown
Owner

Thanks for the clear diagnosis and the regression tests.

Summary. The sort used to run on raw psutil rows, before the per-process conversion. io_counters was still a pio namedtuple, so the sort read syscall counts where it expected byte totals, and cpu_times raised and silently fell back to cpu_percent. The PR splits the per-process loop in two: the metadata/IO/cache/cpu_times conversion now runs before sort_stats, and the extended-stats pass stays after the sort, so cursor/position still matches the displayed order (#3400).

CPU impact: negligible.

  • No extra psutil calls. get_pid_time_and_status, get_io_counters and maybe_add_cached_stats are moved, not duplicated, and still run once per process per cycle.
  • cpu_times._asdict() isn't new work: namedtuple_to_dict in update_list() already did it. It now finds a dict already in place.
  • The only real overhead is one extra Python loop. A micro-benchmark of the added loop gives about 0.4 µs/process including _asdict (well under 0.2 ms/cycle for 500 processes).
  • I also benchmarked the whole update(), base vs PR, on about 480 live processes with cpu_percent, io_counters and cpu_times sort keys. The results are identical within noise (about 33–43 ms/cycle on both sides).
  • A small gain with the cpu_times sort: no more exception followed by a second full sort.

Minor, non-blocking:

  • extended_process now carries the converted status (one letter) and io_counters (list) instead of the raw values. None of _EXTENDED_KEYS, the TUI or the WebUI reads them, so there's no visible change.
  • The post-loop copy of cmdline into extended_process is now redundant, since cmdline is merged before the extended grab. It's harmless.
  • The comment in glances/plugins/processlist/model.py (around lines 192–194) still says cmdline is merged "after the extended grab". It's worth updating to match.

LGTM overall.


Generated by Claude Code

Review on nicolargo#3831: since the pre-sort pass merges the cached attrs
(cmdline, username) before the extended grab, the processlist model
comment claiming cmdline was merged after was stale, and the post-loop
copy into extended_process was redundant -- namedtuple_to_dict(proc)
already carries it. Update the comment in model.py (and the same claim
in the test docstring) and delete the dead copy.
@santhiprakash

Copy link
Copy Markdown
Contributor Author

Done in 141cb10 — thanks for the close read.

You asked for the stale-comment fix: model.py no longer claims cmdline is merged after the extended grab; it now says it reaches extended_process with the rest of the process dict, since the engine merges the cached attrs in the pre-sort pass.

Since you flagged the post-loop copy as redundant, I removed it rather than leaving dead code — extended_process = namedtuple_to_dict(proc) already carries cmdline, and the copy only ever ran right after that rebuild. Same stale claim fixed in test_the_pinned_process_carries_its_command_line's docstring. The only edge that changes: a pinned process whose cmdline was never readable (cache miss and the as_dict fallback failing) now omits the key instead of publishing "cmdline": null — every consumer goes through payload.get("cmdline"), so no visible difference. I left status/io_counters in extended_process as is — same conclusion, nothing reads them.

Regression check: test_the_pinned_process_carries_its_command_line still green; pytest tests/test_processes_extended.py tests/test_processes_sort_order.py tests/test_plugin_processlist.py tests/test_plugin_processlist_render_curses.py → 175 pass, plus the routes/curses/processcount suites → 437 pass; ruff clean. Checks on the new head: snyk + Codacy are running; the GH Actions workflow still needs maintainer approval to run (first-contributor gate).

@nicolargo
nicolargo merged commit 5c6d2de into nicolargo:develop Oct 31, 2026
1 of 2 checks passed
@nicolargo

Copy link
Copy Markdown
Owner

Thanks for the PR @santhiprakash

Just merged into develop.

@nicolargo nicolargo added this to the Glances 5.0.0 milestone Oct 31, 2026
@santhiprakash
santhiprakash deleted the fix/3830-sort-on-converted-fields branch November 1, 2026 09:58
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.

2 participants