Repository navigation
Conversation
A failed refresh left the previous entry in the tree cache, so a later cached read for a recycled pid returned the dead process's branch. Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Verdict: approve — the fix is needed, correct, and ready to merge into its current target. No blocking findings.
Reviewed node-agent#1047, head e3f05c402c65b52ad20343bb16edf81ecd033c2e, against feat/host-profile-gaps at 4f207ac6f20b9e814db815e3edf755c0726ae307.
The failure is real on that target: a process-event refresh can fail after same-command PID reuse with unknown replacement ancestry, leaving the predecessor's branch available to subsequent cached enrichment. pkg/containerwatcher/v2/event_enricher.go:34–42 refreshes after process events and permits cached reads for other events. Removing the exact container/PID entry at pkg/processtree/process_tree_manager.go:94 preserves the existing error and prevents reuse of that invalid branch. The pinned LRU synchronizes removal internally; the change adds no API, configuration, dependency, or data-format change. Independent code and architecture reviews found no introduced blocker.
Related history:
- #1043 discussion documents this same-command gap; the maintainer explicitly invited the follow-up. Its command check does not supersede this fix.
- Merged #876 repairs creator-node PID reuse and exit cleanup, and #1027 repairs host ancestry. Neither invalidates this separate manager cache on reconstruction failure. #936 optimized the manager without repairing this error path.
- Open #1048 addresses the separate pre-existing creator-lock race in branch traversal; it complements this change.
- #1014 proposes cache migration. #1018's decision keeps production on Hashicorp pending performance evidence; this PR respects that boundary. Closed #521 had approval and a suggestion to close, with no documented rejection of this approach.
Searches covered open/closed/merged PRs and issues using processtree, process tree, PID reuse, GetContainerProcessTree, process_tree_manager.go, cached branch, stale/process, and refresh/cache variants, capped at 50 results per query. No duplicate or superseding invalidation fix was identified; keyword search is not exhaustive. No applicable prior rejection was found.
Validation used a network-isolated Bubblewrap sandbox with cleared environment, no credentials, dropped capabilities, read-only source/dependencies, and temporary writable build storage:
- Target implementation plus the new test, via Go overlay: fails as expected at test line 195 because the cached read incorrectly succeeds.
- PR head:
go test -mod=readonly -count=1 ./pkg/processtree/...passes. go test -mod=readonly -race -run TestManager_GetContainerProcessTree_FailedRefreshDropsCachedBranch -count=10 ./pkg/processtree: passes, with no race reports.go vet -mod=readonly ./pkg/processtree/...: passes.- Formatting and
git diff --check: clean.
GitHub validation succeeded at the reviewed head, including the non-race test step; its race step was skipped. Component tests, benchmark and CodeQL also report success. No live PID-wrap/cluster reproduction or general concurrency-coherence guarantee is claimed. Gopls returned invalid imported types, so semantic caller analysis was unavailable; callers were inspected directly. Concurrent cache publication and broader PID identity handling remain outside this bounded fix.
Overview
Targets #1043's branch, as discussed there.
When a tree refresh fails,
GetContainerProcessTreekept the old cache entry, so a later cached read for a recycled PID returned the dead process's branch. #1043's comm check misses this when both processes have the same comm, like twopython3workers.Fix: drop the cache entry when the lookup fails (one line). The new test
TestManager_GetContainerProcessTree_FailedRefreshDropsCachedBranchfails on 4f207ac and passes with the fix.The case is narrow: it needs PID reuse within the 1 min cache TTL and a parent the tree hasn't seen yet.
How to Test