Repository navigation
fix(processtree): walk container branches under the creator lock - #1048
Conversation
GetContainerProcessTree walked the process map under the manager lock only, while exit cleanup reparents children under the creator lock only. Route the walk through the creator's existing GetPidBranch so both sides take the same lock. Signed-off-by: DeathGun44 <krishnamewara841@gmail.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Creator-mediated PID branch lookup pkg/processtree/creator/processtree_creator_interface.go, pkg/processtree/process_tree_manager.go, pkg/processtree/creator/processtree_creator.go, pkg/processtree/process_tree_manager_test.go |
The creator interface adds GetPidBranch, and the manager delegates branch retrieval to it. The method comment describes the lock held during the walk. A new test runs lookups while parent processes exit. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Suggested reviewers: matthyx
Merge Risk: 
dc646
-
No merge-blocking issue is established for the locked process-tree lookup change; run the reported race test as part of normal checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title "fix(processtree): walk container branches under the creator lock" matches the main change. The PR switches GetContainerProcessTree to the creator's GetPidBranch, which takes the creator loc… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title "fix(processtree): walk container branches under the creator lock" matches the main change. The PR switches GetContainerProcessTree to the creator's GetPidBranch, which takes the creator lock, so branch reads and exit-cleanup reparenting use the same lock. The raw summary confirms the interface addition, the manager call-site change, the comment update, and the new concurrency test. The title is specific, concise, and in conventional-commit form.
-
-
Finishing Touches: 💡 1
-
🧪 Generate unit tests (beta)
-
- Create a new PR
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Approve: this is a needed, narrowly scoped synchronization fix. Reviewed head dc64635ef02ffe6467ae347d1784963712ef4366 against main at d4bb9dbf3212880c7e7b1cc0d429823f1d9b0b8f.
The target branch walks mutable process nodes under the manager lock, but background exit cleanup independently writes child PPID/Pcomm under the creator lock (creator/exit_manager.go:121–143,309–313). The safe map protects map operations, not the pointed-to fields. Routing the walk through the existing creator accessor holds its read lock through ancestry traversal and branch copying. Lock order remains manager → creator → container, matching event handling; cleanup does not acquire the manager lock. Returned nodes and child maps are copied before unlocking. Cache behavior and error propagation are preserved. No correctness, security, lifecycle, or performance blocker found; both independent review lanes agree (architecture CLEAR).
Related history: #936 removed redundant node-copy allocations, #876 repaired exit lifecycle/PID reuse, #1027 repaired host ancestry, and #1045 added reparenting coverage. None supplies this shared-lock fix. #1047 targets failed-refresh cache invalidation on #1043's branch and is complementary. Closed #666 concerns lazy shim discovery; #667 concerns fork metadata and reparented Pcomm. Neither contains review discussion or an explicit rejection rationale. #521 was approved before a maintainer suggested closure, without a substantive rejection reason. These closures do not establish an objection to this fix. Repository-wide PR/issue searches covered processtree, process tree, GetPidBranch, GetContainerProcessTree, process_tree_manager.go, exit_manager, reparent, race, and creator lock, including open/closed/merged results, capped at 100 per query. The broad race query reached its cap; this is not an exhaustive proof of absence.
Nonblocking test improvement: process_tree_manager_test.go:177 ignores lookup results and relies on timing. Assert a successful branch and observed reparenting to guard against vacuous stress coverage. The interface gains a method already present on the production implementation; external custom implementations must add it. No dependency or data-format change.
Validation in an isolated, credential-free Linux container with Go 1.27.0: go test -mod=readonly -race -count=3 ./pkg/processtree/... and go vet -mod=readonly ./pkg/processtree/... passed. Replacing only the manager file with the exact target-branch version and running go test -mod=readonly -race -count=1 ./pkg/processtree -run '^TestManager_GetContainerProcessTree_ConcurrentExitCleanup$' failed with races between cleanup writes at exit_manager.go:309,311 and branch reads at container_processtree.go:246,248, confirming both necessity and regression coverage. An initial run with Go as container PID 1 passed the manager package but failed parent-PID-sensitive creator tests; the shell-parent rerun above passed all packages. Formatting checks passed. Semantic tooling could not resolve implementations reliably; no complete semantic-analysis result is claimed.
The reviewed commit's build/unit-test job, CodeQL, benchmark, and other component tests passed. Test_19's failed job timed out contacting Docker during cluster setup (exit 124), before executing the test; it is infrastructure failure. Full local agent build, full-repository lint, and live-cluster testing were not repeated.
Existing PR review and inline-comment lists were empty; the bot reported no actionable finding. Final open/head/base/draft/conflict checks are performed immediately before submission. No source branch changes or merge performed.
Overview
GetContainerProcessTreewalks the process map holding only the manager lock (process_tree_manager.go#L87-L91), while exit cleanup reparents children and writesPPIDandPcommholding only the creator lock (exit_manager.go#L309-L311). They run in separate goroutines (event enrichment and the exit cleanup loop), sogo test -raceflags it.The creator already has
GetPidBranch, which takes its own lock. This adds it to theProcessTreeCreatorinterface and calls it from the manager instead. Lock order stays manager then creator, same asReportEvent, and cache hits still return before taking any lock.Pcommis a string, so a racing read isn't safe in Go. I haven't seen a crash or a wrong alert from it though.New test
TestManager_GetContainerProcessTree_ConcurrentExitCleanupreads branches while exit cleanup reparents. It fails with-raceon main and passes with the fix.This touches the same function as #1047, on different lines, so the two don't conflict.
How to Test
Summary by CodeRabbit