From e44be24308842428a4fce9a2e3d7ffce95d3455b Mon Sep 17 00:00:00 2001 From: Alon Date: Thu, 30 Jul 2026 13:15:17 +0300 Subject: [PATCH 01/14] feat(processtree): read process start time from procfs stat field 22 (SUB-7845) Docs-exempt: additive field population with no consumer yet; no documented behaviour changes. Feature doc lands with the streaming attribution work that gives the value meaning. Co-Authored-By: Claude Opus 5 Signed-off-by: Alon --- pkg/processtree/conversion/types.go | 6 ++++ pkg/processtree/feeder/procfs_feeder.go | 28 +++++++++++++++++++ pkg/processtree/feeder/procfs_feeder_test.go | 29 ++++++++++++++++++++ 3 files changed, 63 insertions(+) diff --git a/pkg/processtree/conversion/types.go b/pkg/processtree/conversion/types.go index ce156d2d6b..cb05e2fae3 100644 --- a/pkg/processtree/conversion/types.go +++ b/pkg/processtree/conversion/types.go @@ -28,6 +28,12 @@ type ProcessEvent struct { Cwd string Path string StartTimeNs uint64 // Process start time in nanoseconds for unique identification + // StartTimeWall is the process creation time as wall-clock time, derived by + // the procfs feeder from /proc/stat btime + StartTimeNs. DISPLAY ONLY: btime + // has whole-second resolution, so this value carries up to 1s of skew and + // must never be compared for process identity — StartTimeNs is the sole + // identity source. Zero for every event type except ProcfsEvent. + StartTimeWall time.Time // Container context ContainerID string diff --git a/pkg/processtree/feeder/procfs_feeder.go b/pkg/processtree/feeder/procfs_feeder.go index ab329127cc..180967e483 100644 --- a/pkg/processtree/feeder/procfs_feeder.go +++ b/pkg/processtree/feeder/procfs_feeder.go @@ -15,6 +15,15 @@ import ( "github.com/prometheus/procfs" ) +// ticksPerSecond is USER_HZ, the unit of /proc//stat field 22. Hardcoded to +// match prometheus/procfs's own userHZ constant and the wire contract's +// documentation that start times are exact multiples of 10 ms. +const ticksPerSecond = 100 + +// nsPerTick converts /proc//stat field 22 (clock ticks since boot) to +// boot-relative nanoseconds. Exact: 1e9 / 100 = 10,000,000. +const nsPerTick = uint64(1_000_000_000 / ticksPerSecond) + // ProcfsFeeder implements ProcessEventFeeder by reading process information from /proc filesystem. type ProcfsFeeder struct { subscribers []chan<- conversion.ProcessEvent @@ -26,6 +35,10 @@ type ProcfsFeeder struct { procfsPath string procfs procfs.FS processTreeManager processtree.ProcessTreeManager + // bootTime is /proc/stat's btime, read once at Start. Used only to derive the + // display-only wall-clock start time; it has whole-second resolution, which is + // why boot-relative nanoseconds remain the sole process-identity source. + bootTime time.Time } // procInfo is a helper struct to pass results from worker goroutines. @@ -61,6 +74,13 @@ func (pf *ProcfsFeeder) Start(ctx context.Context) error { } pf.procfs = fs + if stat, err := fs.Stat(); err == nil { + pf.bootTime = time.Unix(int64(stat.BootTime), 0) + } else { + // Wall-clock start times will stay zero; boot-relative identity is unaffected. + fmt.Fprintf(os.Stderr, "procfs feeder: failed to read btime, StartTimeWall disabled: %v\n", err) + } + // Create a cancellable context for graceful shutdown pf.ctx, pf.cancel = context.WithCancel(ctx) @@ -233,6 +253,14 @@ func (pf *ProcfsFeeder) readProcessInfo(pid uint32) (conversion.ProcessEvent, er event.PPID = uint32(stat.PPID) event.Comm = stat.Comm + // Field 22: process creation time in clock ticks since boot. Convert to + // boot-relative nanoseconds (the identity unit on the wire) and, separately, + // to a display-only wall-clock time via btime. + event.StartTimeNs = stat.Starttime * nsPerTick + if !pf.bootTime.IsZero() && event.StartTimeNs != 0 { + event.StartTimeWall = pf.bootTime.Add(time.Duration(event.StartTimeNs)) + } + if status, err := proc.NewStatus(); err == nil { uid := uint32(status.UIDs[1]) gid := uint32(status.GIDs[1]) diff --git a/pkg/processtree/feeder/procfs_feeder_test.go b/pkg/processtree/feeder/procfs_feeder_test.go index fca872b2a5..9e3c218c1a 100644 --- a/pkg/processtree/feeder/procfs_feeder_test.go +++ b/pkg/processtree/feeder/procfs_feeder_test.go @@ -108,6 +108,35 @@ func TestProcfsFeeder_ReadProcessInfo(t *testing.T) { assert.Error(t, err) } +func TestProcfsFeeder_ReadProcessInfo_StartTime(t *testing.T) { + mockManager := processtree.NewProcessTreeManagerMock() + feeder := NewProcfsFeeder(100*time.Millisecond, 10*time.Millisecond, mockManager) + require.NoError(t, feeder.Start(context.Background())) + defer feeder.Stop() + + event, err := feeder.readProcessInfo(uint32(os.Getpid())) + require.NoError(t, err) + + // Boot-relative nanoseconds: non-zero and an exact multiple of 10ms, + // because /proc//stat field 22 is denominated in USER_HZ=100 ticks. + assert.NotZero(t, event.StartTimeNs) + assert.Zero(t, event.StartTimeNs%10_000_000, + "procfs-derived StartTimeNs must be an exact multiple of 10^7 ns") + + // Wall-clock derivation: btime + ticks/HZ. Sanity: within [boot, now]. + assert.False(t, event.StartTimeWall.IsZero()) + assert.True(t, event.StartTimeWall.Before(time.Now().Add(2*time.Second))) + // This process started after boot: wall - bootNs must land near btime. + // (Loose check: StartTimeWall minus the boot-relative duration is in the past.) + assert.True(t, event.StartTimeWall.Add(-time.Duration(event.StartTimeNs)).Before(time.Now())) + + // Sharper check on the arithmetic itself: this is the test binary's own pid, + // so its real creation time is moments ago. A wrong btime or a wrong tick + // scaling would land this decades or hours away rather than minutes. + assert.WithinDuration(t, time.Now(), event.StartTimeWall, 5*time.Minute, + "btime + ticks/HZ must reconstruct the test process's actual start time") +} + func TestProcfsFeeder_GetProcessComm(t *testing.T) { mockManager := processtree.NewProcessTreeManagerMock() feeder := NewProcfsFeeder(100*time.Millisecond, 10*time.Millisecond, mockManager) From 566a48acf78f565168bb8a53af35a6f2b1c77e56 Mon Sep 17 00:00:00 2001 From: Alon Date: Thu, 30 Jul 2026 13:20:40 +0300 Subject: [PATCH 02/14] feat(processtree): carry procfs start time through the event pipeline (SUB-7845) Adds StartTimeWall to events.ProcfsEvent and copies it in the ProcfsTracer and convertProcfsEvent. convertExecEvent/convertForkEvent/convertExitEvent are deliberately untouched: their StartTimeNs is an event wall-clock timestamp (epoch ns, not boot ns) that only feeds pending-exit sort ordering. Docs-exempt: additive field plumbing, no documented behaviour changes. Co-Authored-By: Claude Opus 5 Signed-off-by: Alon --- pkg/containerwatcher/v2/tracers/procfs.go | 1 + pkg/ebpf/events/procfs.go | 40 +++++++++++++--------- pkg/processtree/conversion/convert.go | 11 ++++-- pkg/processtree/conversion/convert_test.go | 26 ++++++++++++++ 4 files changed, 58 insertions(+), 20 deletions(-) create mode 100644 pkg/processtree/conversion/convert_test.go diff --git a/pkg/containerwatcher/v2/tracers/procfs.go b/pkg/containerwatcher/v2/tracers/procfs.go index eefaca660b..2c91a5f028 100644 --- a/pkg/containerwatcher/v2/tracers/procfs.go +++ b/pkg/containerwatcher/v2/tracers/procfs.go @@ -161,6 +161,7 @@ func (pt *ProcfsTracer) handleProcfsEvent(event conversion.ProcessEvent) { Cwd: event.Cwd, Path: event.Path, StartTimeNs: event.StartTimeNs, + StartTimeWall: event.StartTimeWall, ContainerID: event.ContainerID, ContainerMntNs: event.ContainerMntNs, ContainerNetNs: event.ContainerNetNs, diff --git a/pkg/ebpf/events/procfs.go b/pkg/ebpf/events/procfs.go index 60b1f37505..7856969785 100644 --- a/pkg/ebpf/events/procfs.go +++ b/pkg/ebpf/events/procfs.go @@ -1,29 +1,35 @@ package events import ( + "time" + "github.com/inspektor-gadget/inspektor-gadget/pkg/types" "github.com/kubescape/node-agent/pkg/utils" ) // ProcfsEvent represents a procfs event that can be processed by the ordered event queue type ProcfsEvent struct { - Type types.EventType `json:"type"` - Timestamp types.Time `json:"timestamp"` - PID uint32 `json:"pid"` - PPID uint32 `json:"ppid"` - Comm string `json:"comm"` - Pcomm string `json:"pcomm"` - Cmdline string `json:"cmdline"` - Uid *uint32 `json:"uid"` - Gid *uint32 `json:"gid"` - Cwd string `json:"cwd"` - Path string `json:"path"` - StartTimeNs uint64 `json:"start_time_ns"` - ContainerID string `json:"container_id"` - ContainerMntNs uint64 `json:"container_mnt_ns"` - ContainerNetNs uint64 `json:"container_net_ns"` - HostPID int `json:"host_pid"` - HostPPID int `json:"host_ppid"` + Type types.EventType `json:"type"` + Timestamp types.Time `json:"timestamp"` + PID uint32 `json:"pid"` + PPID uint32 `json:"ppid"` + Comm string `json:"comm"` + Pcomm string `json:"pcomm"` + Cmdline string `json:"cmdline"` + Uid *uint32 `json:"uid"` + Gid *uint32 `json:"gid"` + Cwd string `json:"cwd"` + Path string `json:"path"` + StartTimeNs uint64 `json:"start_time_ns"` + // StartTimeWall is the display-only wall-clock creation time derived by the + // procfs feeder from btime + StartTimeNs. Never compare it for process + // identity — StartTimeNs (boot-relative) is the sole identity source. + StartTimeWall time.Time `json:"start_time_wall"` + ContainerID string `json:"container_id"` + ContainerMntNs uint64 `json:"container_mnt_ns"` + ContainerNetNs uint64 `json:"container_net_ns"` + HostPID int `json:"host_pid"` + HostPPID int `json:"host_ppid"` } var _ utils.K8sEvent = (*ProcfsEvent)(nil) diff --git a/pkg/processtree/conversion/convert.go b/pkg/processtree/conversion/convert.go index ff54c6d5c9..4be31dd6fb 100644 --- a/pkg/processtree/conversion/convert.go +++ b/pkg/processtree/conversion/convert.go @@ -131,9 +131,14 @@ func convertProcfsEvent(procfsEvent *events.ProcfsEvent) ProcessEvent { Cwd: procfsEvent.Cwd, Path: procfsEvent.Path, StartTimeNs: procfsEvent.StartTimeNs, - ContainerID: procfsEvent.ContainerID, - HostPID: procfsEvent.HostPID, - HostPPID: procfsEvent.HostPPID, + // Only ProcfsEvent carries a real start time. The exec/fork/exit + // converters above set StartTimeNs from the event's wall-clock timestamp + // — a different clock domain (epoch ns, not boot ns) with different + // semantics — and deliberately leave StartTimeWall zero. + StartTimeWall: procfsEvent.StartTimeWall, + ContainerID: procfsEvent.ContainerID, + HostPID: procfsEvent.HostPID, + HostPPID: procfsEvent.HostPPID, } return event diff --git a/pkg/processtree/conversion/convert_test.go b/pkg/processtree/conversion/convert_test.go new file mode 100644 index 0000000000..6f4b4d1236 --- /dev/null +++ b/pkg/processtree/conversion/convert_test.go @@ -0,0 +1,26 @@ +package conversion + +import ( + "testing" + "time" + + "github.com/inspektor-gadget/inspektor-gadget/pkg/types" + "github.com/kubescape/node-agent/pkg/ebpf/events" + "github.com/kubescape/node-agent/pkg/utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestConvertProcfsEvent_CarriesStartTime(t *testing.T) { + wall := time.Date(2026, 7, 29, 10, 0, 0, 0, time.UTC) + pe := &events.ProcfsEvent{ + Type: types.NORMAL, Timestamp: types.Time(time.Now().UnixNano()), + PID: 42, PPID: 1, Comm: "nginx", + StartTimeNs: 123_450_000_000, // 12345 ticks + StartTimeWall: wall, + } + got, err := ConvertEvent(utils.ProcfsEventType, pe) + require.NoError(t, err) + assert.Equal(t, uint64(123_450_000_000), got.StartTimeNs) + assert.Equal(t, wall, got.StartTimeWall) +} From c99af6e727910c336ef9d20fed7eae0f96393cb9 Mon Sep 17 00:00:00 2001 From: Alon Date: Thu, 30 Jul 2026 13:26:42 +0300 Subject: [PATCH 03/14] feat(processtree): record boot-relative start time in creator side map (SUB-7845) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit handleProcfsEvent stores /proc field 22's boot-relative nanoseconds in a new pid-keyed side map and stamps the derived wall-clock value on the tree node. The side map is the sole identity source; Process.StartTime is display-only and inherits btime's whole-second skew. exitByPid reclaims the side-map entry on both paths — next to processMap.Delete and in the early return where the node is already gone. Docs-exempt: additive, nothing reads the values yet. Co-Authored-By: Claude Opus 5 Signed-off-by: Alon --- pkg/processtree/creator/exit_manager.go | 3 + .../creator/processtree_creator.go | 30 ++++++++ .../creator/processtree_creator_interface.go | 4 + pkg/processtree/creator/starttime_test.go | 74 +++++++++++++++++++ 4 files changed, 111 insertions(+) create mode 100644 pkg/processtree/creator/starttime_test.go diff --git a/pkg/processtree/creator/exit_manager.go b/pkg/processtree/creator/exit_manager.go index 69a29e5ff0..a26c78c6d3 100644 --- a/pkg/processtree/creator/exit_manager.go +++ b/pkg/processtree/creator/exit_manager.go @@ -128,6 +128,7 @@ func (pt *processTreeCreatorImpl) exitByPid(pid uint32) { proc, ok := pt.processMap.Load(pid) if !ok { delete(pt.pendingExits, pid) + delete(pt.pidStartTimeNs, pid) return } @@ -181,5 +182,7 @@ func (pt *processTreeCreatorImpl) exitByPid(pid uint32) { } pt.processMap.Delete(pid) + // The process identity dies with the node it identifies. + delete(pt.pidStartTimeNs, pid) delete(pt.pendingExits, pid) } diff --git a/pkg/processtree/creator/processtree_creator.go b/pkg/processtree/creator/processtree_creator.go index 2f79a3a516..78469756a3 100644 --- a/pkg/processtree/creator/processtree_creator.go +++ b/pkg/processtree/creator/processtree_creator.go @@ -24,6 +24,15 @@ type processTreeCreatorImpl struct { // Exit manager fields pendingExits map[uint32]*pendingExit // PID -> pending exit exitCleanupStopChan chan struct{} + + // pidStartTimeNs maps a live pid to its creation time in nanoseconds since + // boot, sourced EXCLUSIVELY from /proc//stat field 22 (never from + // fork/exec/exit event timestamps, which are epoch wall-clock — a different + // clock domain). This side map is the SOLE process-identity time source; the + // wall-clock Process.StartTime stamped on tree nodes is display-only and + // inherits btime's whole-second skew. Guarded by pt.mutex. Entries are + // deleted in exitByPid together with the tree node. + pidStartTimeNs map[uint32]uint64 } func NewProcessTreeCreator(containerTree containerprocesstree.ContainerProcessTree, config config.Config) ProcessTreeCreator { @@ -39,6 +48,7 @@ func NewProcessTreeCreator(containerTree containerprocesstree.ContainerProcessTr reparentingStrategies: reparentingLogic, containerTree: containerTree, pendingExits: make(map[uint32]*pendingExit), + pidStartTimeNs: make(map[uint32]uint64), config: config, } @@ -98,6 +108,16 @@ func (pt *processTreeCreatorImpl) GetProcessNode(pid int) (*armotypes.Process, e return pt.shallowCopyProcess(proc), nil } +// GetProcessBootTimeNs returns the pid's creation time as nanoseconds since boot +// (CLOCK_BOOTTIME), or 0 when unknown — the process has not been seen by the +// /proc scan or an on-demand read, or it has already exited. Procfs-sourced +// only; see pidStartTimeNs for the identity contract. +func (pt *processTreeCreatorImpl) GetProcessBootTimeNs(pid uint32) uint64 { + pt.mutex.RLock() + defer pt.mutex.RUnlock() + return pt.pidStartTimeNs[pid] +} + // GetPidBranch performs container branch operation (no longer needs to be atomic) func (pt *processTreeCreatorImpl) GetPidBranch(containerTree interface{}, containerID string, targetPID uint32) (armotypes.Process, error) { pt.mutex.RLock() @@ -217,6 +237,16 @@ func (pt *processTreeCreatorImpl) handleProcfsEvent(event conversion.ProcessEven proc.Path = event.Path } + // Start time from /proc//stat field 22. The boot-relative value is the + // identity source and lives only in the side map; the wall-clock value is + // stamped on the node for display and must never be compared for identity. + if event.StartTimeNs != 0 { + pt.pidStartTimeNs[event.PID] = event.StartTimeNs + if !event.StartTimeWall.IsZero() { + proc.StartTime = event.StartTimeWall + } + } + if proc.ChildrenMap == nil { proc.ChildrenMap = make(map[armotypes.CommPID]*armotypes.Process) } diff --git a/pkg/processtree/creator/processtree_creator_interface.go b/pkg/processtree/creator/processtree_creator_interface.go index 01a7f2328d..1a00c34fd3 100644 --- a/pkg/processtree/creator/processtree_creator_interface.go +++ b/pkg/processtree/creator/processtree_creator_interface.go @@ -15,6 +15,10 @@ type ProcessTreeCreator interface { GetProcessMap() *maps.SafeMap[uint32, *armotypes.Process] // Optionally: Query for a process node by PID GetProcessNode(pid int) (*armotypes.Process, error) + // GetProcessBootTimeNs returns the process's creation time as nanoseconds + // since boot (CLOCK_BOOTTIME), sourced exclusively from /proc//stat + // field 22. Returns 0 when unknown (process not yet seen, or already exited). + GetProcessBootTimeNs(pid uint32) uint64 // Start the process tree creator and begin background tasks Start() // Stop the process tree creator and cleanup resources diff --git a/pkg/processtree/creator/starttime_test.go b/pkg/processtree/creator/starttime_test.go new file mode 100644 index 0000000000..aa3cccb923 --- /dev/null +++ b/pkg/processtree/creator/starttime_test.go @@ -0,0 +1,74 @@ +package processtreecreator + +import ( + "testing" + "time" + + "github.com/kubescape/node-agent/pkg/config" + processtreecreatorconfig "github.com/kubescape/node-agent/pkg/processtree/config" + "github.com/kubescape/node-agent/pkg/processtree/conversion" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestHandleProcfsEvent_RecordsStartTime(t *testing.T) { + creator := NewProcessTreeCreator(&mockContainerProcessTree{}, config.Config{}) + wall := time.Date(2026, 7, 29, 10, 0, 0, 0, time.UTC) + creator.FeedEvent(conversion.ProcessEvent{ + Type: conversion.ProcfsEvent, Timestamp: time.Now(), + PID: 100, PPID: 1, Comm: "nginx", + StartTimeNs: 555_550_000_000, StartTimeWall: wall, + }) + + // Boot-ns identity: exposed via the accessor, sourced from the side map. + assert.Equal(t, uint64(555_550_000_000), creator.GetProcessBootTimeNs(100)) + assert.Zero(t, creator.GetProcessBootTimeNs(999), "unknown pid reads as 0") + + // Wall-clock display value on the node. + node, err := creator.GetProcessNode(100) + require.NoError(t, err) + require.NotNil(t, node) + assert.Equal(t, wall, node.StartTime) +} + +func TestExitByPid_DeletesStartTimeEntry(t *testing.T) { + cfg := config.Config{} + cfg.ExitCleanup = processtreecreatorconfig.ExitCleanupConfig{ + MaxPendingExits: 10, CleanupInterval: time.Hour, CleanupDelay: 0, + } + creator := NewProcessTreeCreator(&mockContainerProcessTree{}, cfg).(*processTreeCreatorImpl) + creator.FeedEvent(conversion.ProcessEvent{ + Type: conversion.ProcfsEvent, PID: 100, PPID: 1, Comm: "nginx", + StartTimeNs: 1_000_000_000, + }) + require.Equal(t, uint64(1_000_000_000), creator.GetProcessBootTimeNs(100), + "precondition: the side map must hold the entry before the exit") + + creator.FeedEvent(conversion.ProcessEvent{ + Type: conversion.ExitEvent, PID: 100, Comm: "exit", Timestamp: time.Now(), + }) + creator.mutex.Lock() + creator.exitByPid(100) + creator.mutex.Unlock() + + assert.Zero(t, creator.GetProcessBootTimeNs(100), "side-map entry must die with the node") +} + +// TestExitByPid_DeletesStartTimeEntry_NodeAlreadyGone covers exitByPid's early +// return: the node is absent from processMap, but a stale side-map entry must +// still be reclaimed rather than leaking for the lifetime of the agent. +func TestExitByPid_DeletesStartTimeEntry_NodeAlreadyGone(t *testing.T) { + cfg := config.Config{} + cfg.ExitCleanup = processtreecreatorconfig.ExitCleanupConfig{ + MaxPendingExits: 10, CleanupInterval: time.Hour, CleanupDelay: 0, + } + creator := NewProcessTreeCreator(&mockContainerProcessTree{}, cfg).(*processTreeCreatorImpl) + + creator.mutex.Lock() + creator.pidStartTimeNs[404] = 2_000_000_000 // entry with no corresponding node + creator.exitByPid(404) + creator.mutex.Unlock() + + assert.Zero(t, creator.GetProcessBootTimeNs(404), + "the early-return branch must reclaim the side-map entry too") +} From dab3f87b76328146eedffa3635aed7f25eba25d0 Mon Sep 17 00:00:00 2001 From: Alon Date: Thu, 30 Jul 2026 13:34:23 +0300 Subject: [PATCH 04/14] feat(processtree): on-demand start-time read at fork/exec node creation (SUB-7845) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scan-only population leaves every process shorter than the 30s scan interval at a zero start time, and short-lived processes are both the bulk of beacon-style connections and the drivers of pid churn. ensureStartTime reads the same kernel source as the periodic scan (/proc//stat field 22) when a fork or exec creates a node, so those processes get an identity too. One read per node creation, never per event. A failed read (process already gone) leaves zero rather than guessing. event.StartTimeNs is never consulted: for fork/exec/exit it is the event's epoch wall-clock timestamp, a different clock domain — pinned by TestHandleForkEvent_IgnoresEventStartTimeNs. nsPerTick is defined per-package (feeder and creator); both are package-private and cross-referenced in comments rather than lifted into a shared package. Docs-exempt: additive, nothing reads the values yet. Co-Authored-By: Claude Opus 5 Signed-off-by: Alon --- .../creator/processtree_creator.go | 41 ++++++++++ pkg/processtree/creator/starttime_reader.go | 60 ++++++++++++++ pkg/processtree/creator/starttime_test.go | 79 +++++++++++++++++++ 3 files changed, 180 insertions(+) create mode 100644 pkg/processtree/creator/starttime_reader.go diff --git a/pkg/processtree/creator/processtree_creator.go b/pkg/processtree/creator/processtree_creator.go index 78469756a3..623978a597 100644 --- a/pkg/processtree/creator/processtree_creator.go +++ b/pkg/processtree/creator/processtree_creator.go @@ -3,6 +3,7 @@ package processtreecreator import ( "fmt" "sync" + "time" "github.com/armosec/armoapi-go/armotypes" "github.com/goradd/maps" @@ -33,6 +34,12 @@ type processTreeCreatorImpl struct { // inherits btime's whole-second skew. Guarded by pt.mutex. Entries are // deleted in exitByPid together with the tree node. pidStartTimeNs map[uint32]uint64 + + // readStartTime fetches a pid's boot-relative start time and display-only + // wall time from /proc on demand, so a node created by a fork or exec event + // does not wait up to a full scan interval for its identity. Returns zeros + // when the process is already gone. Injectable for tests. + readStartTime func(pid uint32) (uint64, time.Time) } func NewProcessTreeCreator(containerTree containerprocesstree.ContainerProcessTree, config config.Config) ProcessTreeCreator { @@ -49,6 +56,7 @@ func NewProcessTreeCreator(containerTree containerprocesstree.ContainerProcessTr containerTree: containerTree, pendingExits: make(map[uint32]*pendingExit), pidStartTimeNs: make(map[uint32]uint64), + readStartTime: newProcfsStartTimeReader(), config: config, } @@ -159,6 +167,34 @@ func (pt *processTreeCreatorImpl) UpdatePPID(proc *armotypes.Process, event conv } } +// ensureStartTime gives a fork/exec-created node its process identity without +// waiting up to a full scan interval for the periodic /proc sweep — short-lived +// processes are the population most exposed to pid reuse, and they are exactly +// the ones the scan misses. +// +// One read per node creation, never per event: an exec on a node whose entry +// already exists must not re-read, because creation time cannot change on exec. +// +// event.StartTimeNs is deliberately ignored. For fork and exec events it is the +// event's wall-clock timestamp — epoch nanoseconds, not boot-relative, and +// stamping the event rather than process creation. Only the procfs read may +// populate the side map. A failed read leaves zero, never a guess. +// +// Must be called with pt.mutex held. +func (pt *processTreeCreatorImpl) ensureStartTime(proc *armotypes.Process, pid uint32) { + if _, known := pt.pidStartTimeNs[pid]; known { + return + } + ns, wall := pt.readStartTime(pid) + if ns == 0 { + return + } + pt.pidStartTimeNs[pid] = ns + if !wall.IsZero() && proc.StartTime.IsZero() { + proc.StartTime = wall + } +} + // handleForkEvent handles fork events - only fills properties if they are empty or don't exist func (pt *processTreeCreatorImpl) handleForkEvent(event conversion.ProcessEvent) { pt.mutex.Lock() @@ -193,6 +229,8 @@ func (pt *processTreeCreatorImpl) handleForkEvent(event conversion.ProcessEvent) proc.Path = event.Path } + pt.ensureStartTime(proc, event.PID) + if proc.ChildrenMap == nil { proc.ChildrenMap = make(map[armotypes.CommPID]*armotypes.Process) } @@ -294,6 +332,9 @@ func (pt *processTreeCreatorImpl) handleExecEvent(event conversion.ProcessEvent) if event.Path != "" { proc.Path = event.Path } + + pt.ensureStartTime(proc, event.PID) + if proc.ChildrenMap == nil { proc.ChildrenMap = make(map[armotypes.CommPID]*armotypes.Process) } diff --git a/pkg/processtree/creator/starttime_reader.go b/pkg/processtree/creator/starttime_reader.go new file mode 100644 index 0000000000..579375c8df --- /dev/null +++ b/pkg/processtree/creator/starttime_reader.go @@ -0,0 +1,60 @@ +package processtreecreator + +import ( + "sync" + "time" + + "github.com/prometheus/procfs" +) + +// nsPerTick converts /proc//stat field 22 (clock ticks since boot) to +// boot-relative nanoseconds. USER_HZ is 100, so this is exact: 1e9 / 100. +// The procfs feeder defines the same constant for the periodic scan; both are +// package-private and must stay in agreement. +const nsPerTick = uint64(10_000_000) + +// newProcfsStartTimeReader returns the on-demand equivalent of the periodic +// scan's start-time read: /proc//stat field 22 (creation time in clock +// ticks since boot), converted to boot-relative nanoseconds plus the derived +// display-only wall time. +// +// This is NOT the rejected "derive the start time from an exec event" option. +// That rejection was of event TIMESTAMPS — an exec event stamps the exec +// instant, not process creation, so the value changes when a process re-execs. +// This reads the same kernel source as the periodic scan, at the same +// semantics, just fetched when the node is created instead of waiting up to a +// full scan interval. Without it every process shorter than one scan interval +// carries a zero start time, and short-lived processes are exactly the +// population most exposed to pid reuse. +// +// Returns zeros when the process is already gone (it died before its creation +// event was processed) — the caller keeps the pre-existing zero-value +// behaviour rather than guessing. +func newProcfsStartTimeReader() func(pid uint32) (uint64, time.Time) { + var ( + once sync.Once + bootTime time.Time + ) + return func(pid uint32) (uint64, time.Time) { + proc, err := procfs.NewProc(int(pid)) + if err != nil { + return 0, time.Time{} + } + stat, err := proc.Stat() + if err != nil || stat.Starttime == 0 { + return 0, time.Time{} + } + once.Do(func() { + if fs, err := procfs.NewDefaultFS(); err == nil { + if s, err := fs.Stat(); err == nil { + bootTime = time.Unix(int64(s.BootTime), 0) + } + } + }) + ns := stat.Starttime * nsPerTick + if bootTime.IsZero() { + return ns, time.Time{} + } + return ns, bootTime.Add(time.Duration(ns)) + } +} diff --git a/pkg/processtree/creator/starttime_test.go b/pkg/processtree/creator/starttime_test.go index aa3cccb923..43f4fb40f3 100644 --- a/pkg/processtree/creator/starttime_test.go +++ b/pkg/processtree/creator/starttime_test.go @@ -54,6 +54,85 @@ func TestExitByPid_DeletesStartTimeEntry(t *testing.T) { assert.Zero(t, creator.GetProcessBootTimeNs(100), "side-map entry must die with the node") } +func TestHandleForkEvent_ReadsStartTimeOnDemand(t *testing.T) { + creator := NewProcessTreeCreator(&mockContainerProcessTree{}, config.Config{}).(*processTreeCreatorImpl) + wall := time.Date(2026, 7, 29, 12, 0, 0, 0, time.UTC) + reads := 0 + creator.readStartTime = func(pid uint32) (uint64, time.Time) { + reads++ + require.Equal(t, uint32(100), pid) + return 777_770_000_000, wall + } + + creator.FeedEvent(conversion.ProcessEvent{Type: conversion.ForkEvent, PID: 100, PPID: 1, Comm: "curl"}) + assert.Equal(t, uint64(777_770_000_000), creator.GetProcessBootTimeNs(100), + "a fork-created node must not wait up to a scan interval for its identity") + node, err := creator.GetProcessNode(100) + require.NoError(t, err) + assert.Equal(t, wall, node.StartTime) + assert.Equal(t, 1, reads) + + // Exec on the same node: creation time cannot change on exec — no re-read. + creator.FeedEvent(conversion.ProcessEvent{Type: conversion.ExecEvent, PID: 100, PPID: 1, Comm: "curl", Cmdline: "curl -s"}) + assert.Equal(t, 1, reads, "one read per node creation, never per event") +} + +func TestHandleExecEvent_ReadsStartTimeOnDemand(t *testing.T) { + creator := NewProcessTreeCreator(&mockContainerProcessTree{}, config.Config{}).(*processTreeCreatorImpl) + wall := time.Date(2026, 7, 29, 13, 0, 0, 0, time.UTC) + creator.readStartTime = func(pid uint32) (uint64, time.Time) { return 888_880_000_000, wall } + + // An exec with no preceding fork event still creates the node, so it must + // also acquire an identity rather than waiting for the next scan. + creator.FeedEvent(conversion.ProcessEvent{Type: conversion.ExecEvent, PID: 200, PPID: 1, Comm: "sh"}) + assert.Equal(t, uint64(888_880_000_000), creator.GetProcessBootTimeNs(200)) + node, err := creator.GetProcessNode(200) + require.NoError(t, err) + assert.Equal(t, wall, node.StartTime) +} + +func TestHandleForkEvent_ReadFailureLeavesZero(t *testing.T) { + creator := NewProcessTreeCreator(&mockContainerProcessTree{}, config.Config{}).(*processTreeCreatorImpl) + creator.readStartTime = func(pid uint32) (uint64, time.Time) { return 0, time.Time{} } // process already gone + creator.FeedEvent(conversion.ProcessEvent{Type: conversion.ForkEvent, PID: 100, PPID: 1, Comm: "flash"}) + assert.Zero(t, creator.GetProcessBootTimeNs(100), "failure degrades to the pre-existing zero, never a guess") +} + +// TestHandleForkEvent_IgnoresEventStartTimeNs pins the clock-domain boundary. +// convertForkEvent sets ProcessEvent.StartTimeNs from the event's wall-clock +// timestamp — epoch nanoseconds, not boot-relative, and stamping the fork rather +// than process creation. Letting that value into the side map would make identity +// comparisons wrong by decades while still compiling and still joining correctly +// within a single message. Only the procfs read may populate the side map. +func TestHandleForkEvent_IgnoresEventStartTimeNs(t *testing.T) { + creator := NewProcessTreeCreator(&mockContainerProcessTree{}, config.Config{}).(*processTreeCreatorImpl) + creator.readStartTime = func(pid uint32) (uint64, time.Time) { return 777_770_000_000, time.Time{} } + + const epochNs = uint64(1_753_876_800_000_000_000) // what convertForkEvent would supply + creator.FeedEvent(conversion.ProcessEvent{ + Type: conversion.ForkEvent, PID: 100, PPID: 1, Comm: "curl", StartTimeNs: epochNs, + }) + + assert.Equal(t, uint64(777_770_000_000), creator.GetProcessBootTimeNs(100), + "the side map must hold the procfs boot-relative read, not the event's epoch timestamp") +} + +// TestHandleForkEvent_IgnoresEventStartTimeNs_OnReadFailure is the same boundary +// on the failure path: a failed read must leave zero, never fall back to the +// event's epoch timestamp. +func TestHandleForkEvent_IgnoresEventStartTimeNs_OnReadFailure(t *testing.T) { + creator := NewProcessTreeCreator(&mockContainerProcessTree{}, config.Config{}).(*processTreeCreatorImpl) + creator.readStartTime = func(pid uint32) (uint64, time.Time) { return 0, time.Time{} } + + creator.FeedEvent(conversion.ProcessEvent{ + Type: conversion.ForkEvent, PID: 100, PPID: 1, Comm: "curl", + StartTimeNs: 1_753_876_800_000_000_000, + }) + + assert.Zero(t, creator.GetProcessBootTimeNs(100), + "a failed read must degrade to zero, never to the event's epoch timestamp") +} + // TestExitByPid_DeletesStartTimeEntry_NodeAlreadyGone covers exitByPid's early // return: the node is absent from processMap, but a stale side-map entry must // still be reclaimed rather than leaking for the lifetime of the agent. From 492d4922cd15154a1a4a836478f5ab387bc93f1a Mon Sep 17 00:00:00 2001 From: Alon Date: Thu, 30 Jul 2026 13:38:23 +0300 Subject: [PATCH 05/14] feat(processtree): expose boot-relative start time on the manager (SUB-7845) GetProcessBootTimeNs is the method the network-stream attribution work calls to build a per-connection process reference. The doc comment carries the identity contract: this is the sole identity source, and the wall-clock armotypes.Process.StartTime on tree nodes is display-only. Docs-exempt: additive accessor, no caller yet. Co-Authored-By: Claude Opus 5 Signed-off-by: Alon --- pkg/processtree/process_tree_manager.go | 6 ++++ .../process_tree_manager_interface.go | 11 ++++++++ pkg/processtree/process_tree_manager_mock.go | 17 ++++++++++- pkg/processtree/process_tree_manager_test.go | 28 +++++++++++++++++++ 4 files changed, 61 insertions(+), 1 deletion(-) create mode 100644 pkg/processtree/process_tree_manager_test.go diff --git a/pkg/processtree/process_tree_manager.go b/pkg/processtree/process_tree_manager.go index 989771754e..df8f87ac4b 100644 --- a/pkg/processtree/process_tree_manager.go +++ b/pkg/processtree/process_tree_manager.go @@ -109,6 +109,12 @@ func (ptm *ProcessTreeManagerImpl) GetContainerProcessTree(containerID string, p return containerSubtree, nil } +func (ptm *ProcessTreeManagerImpl) GetProcessBootTimeNs(pid uint32) uint64 { + ptm.mutex.RLock() + defer ptm.mutex.RUnlock() + return ptm.creator.GetProcessBootTimeNs(pid) +} + func (ptm *ProcessTreeManagerImpl) GetPidList() []uint32 { ptm.mutex.RLock() defer ptm.mutex.RUnlock() diff --git a/pkg/processtree/process_tree_manager_interface.go b/pkg/processtree/process_tree_manager_interface.go index fd98715bc2..aa11122288 100644 --- a/pkg/processtree/process_tree_manager_interface.go +++ b/pkg/processtree/process_tree_manager_interface.go @@ -11,4 +11,15 @@ type ProcessTreeManager interface { GetContainerProcessTree(containerID string, pid uint32, useCache bool) (armotypes.Process, error) ReportEvent(eventType utils.EventType, event utils.K8sEvent) error GetPidList() []uint32 + // GetProcessBootTimeNs returns the process's creation time as nanoseconds + // since boot (CLOCK_BOOTTIME), sourced exclusively from /proc//stat + // field 22 — either the periodic scan or the on-demand read performed when a + // fork/exec creates the node. Returns 0 when unknown: the process died + // before its creation event was processed, or it has already exited. + // + // This is the sole process-identity time source. The wall-clock + // armotypes.Process.StartTime carried on tree nodes is display-only and + // inherits btime's whole-second skew, so it must never be compared for + // identity. + GetProcessBootTimeNs(pid uint32) uint64 } diff --git a/pkg/processtree/process_tree_manager_mock.go b/pkg/processtree/process_tree_manager_mock.go index 4726f171fd..8d04552684 100644 --- a/pkg/processtree/process_tree_manager_mock.go +++ b/pkg/processtree/process_tree_manager_mock.go @@ -7,7 +7,8 @@ import ( // ProcessTreeManagerMock implements the ProcessTreeManager interface for testing type ProcessTreeManagerMock struct { - pidList []uint32 + pidList []uint32 + bootTimeNs map[uint32]uint64 } var _ ProcessTreeManager = (*ProcessTreeManagerMock)(nil) @@ -48,3 +49,17 @@ func (m *ProcessTreeManagerMock) ReportEvent(eventType utils.EventType, event ut func (m *ProcessTreeManagerMock) GetPidList() []uint32 { return m.pidList } + +// GetProcessBootTimeNs returns the configured start time for a pid, or 0 for an +// unknown one — matching the real manager's "0 means unknown" contract. +func (m *ProcessTreeManagerMock) GetProcessBootTimeNs(pid uint32) uint64 { + return m.bootTimeNs[pid] +} + +// SetProcessBootTimeNs sets the boot-relative start time the mock reports for a pid. +func (m *ProcessTreeManagerMock) SetProcessBootTimeNs(pid uint32, ns uint64) { + if m.bootTimeNs == nil { + m.bootTimeNs = make(map[uint32]uint64) + } + m.bootTimeNs[pid] = ns +} diff --git a/pkg/processtree/process_tree_manager_test.go b/pkg/processtree/process_tree_manager_test.go new file mode 100644 index 0000000000..3afd6b8900 --- /dev/null +++ b/pkg/processtree/process_tree_manager_test.go @@ -0,0 +1,28 @@ +package processtree + +import ( + "testing" + + "github.com/kubescape/node-agent/pkg/config" + "github.com/kubescape/node-agent/pkg/ebpf/events" + containerprocesstree "github.com/kubescape/node-agent/pkg/processtree/container" + processtreecreator "github.com/kubescape/node-agent/pkg/processtree/creator" + "github.com/kubescape/node-agent/pkg/utils" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestManager_GetProcessBootTimeNs exercises the accessor that the network-stream +// attribution work calls to build a per-connection process reference: a procfs +// event reported through the manager must surface its boot-relative start time. +func TestManager_GetProcessBootTimeNs(t *testing.T) { + creator := processtreecreator.NewProcessTreeCreator(containerprocesstree.NewContainerProcessTree(), config.Config{}) + mgr := NewProcessTreeManager(creator, containerprocesstree.NewContainerProcessTree(), config.Config{}) + + require.NoError(t, mgr.ReportEvent(utils.ProcfsEventType, &events.ProcfsEvent{ + PID: 77, PPID: 1, Comm: "worker", StartTimeNs: 2_340_000_000, + })) + + assert.Equal(t, uint64(2_340_000_000), mgr.GetProcessBootTimeNs(77)) + assert.Zero(t, mgr.GetProcessBootTimeNs(1234), "a pid the tree has never seen reads as 0") +} From c2607cadbc9dfdb38174b6ea184952613318f1fb Mon Sep 17 00:00:00 2001 From: Alon Date: Thu, 30 Jul 2026 13:42:32 +0300 Subject: [PATCH 06/14] feat(processtree): carry StartTime through branch/copy/enrich (SUB-7845) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is the step that makes the populated value visible downstream at all. All three functions build a node from an explicit field list and silently strip anything absent from it, and all three omitted StartTime: - buildBranchToShim — feeds every alert branch and every stream tree - CopyProcess — alert bulk manager's merged tree - EnrichProcess — alert bulk manager's merge of overlapping chains Each site gets its own named test so a future field-strip regression is caught by name rather than showing up as a silently empty value on the wire. Docs-exempt: additive field propagation, no documented behaviour changes. Co-Authored-By: Claude Opus 5 Signed-off-by: Alon --- .../container/container_processtree.go | 1 + .../container/container_processtree_test.go | 49 +++++++++++++++++++ pkg/utils/processtree_merge.go | 4 ++ pkg/utils/processtree_merge_test.go | 28 +++++++++++ 4 files changed, 82 insertions(+) create mode 100644 pkg/utils/processtree_merge_test.go diff --git a/pkg/processtree/container/container_processtree.go b/pkg/processtree/container/container_processtree.go index 67cf1ab8b5..afd55db8cd 100644 --- a/pkg/processtree/container/container_processtree.go +++ b/pkg/processtree/container/container_processtree.go @@ -240,6 +240,7 @@ func (c *containerProcessTreeImpl) buildBranchToShim(targetNode *armotypes.Proce Gid: node.Gid, Cwd: node.Cwd, Path: node.Path, + StartTime: node.StartTime, ChildrenMap: make(map[armotypes.CommPID]*armotypes.Process), } } diff --git a/pkg/processtree/container/container_processtree_test.go b/pkg/processtree/container/container_processtree_test.go index 72dd99f006..d0273ed1a7 100644 --- a/pkg/processtree/container/container_processtree_test.go +++ b/pkg/processtree/container/container_processtree_test.go @@ -2,6 +2,7 @@ package containerprocesstree import ( "testing" + "time" "github.com/armosec/armoapi-go/armotypes" "github.com/goradd/maps" @@ -821,3 +822,51 @@ func TestContainerProcessTreeImpl_IsProcessUnderAnyContainerSubtree_OutsideProce assert.False(t, cpt.IsProcessUnderAnyContainerSubtree(200, createSafeMapFromData(fullTree))) // outside process assert.False(t, cpt.IsProcessUnderAnyContainerSubtree(1, createSafeMapFromData(fullTree))) // init } + +// TestContainerProcessTreeImpl_GetPidBranch_CarriesStartTime pins the third of +// the three copy sites that strip any field they do not explicitly list. +// buildBranchToShim feeds GetContainerProcessTree, so every alert branch and +// every network-stream tree goes through it: if the node literal drops +// StartTime, the value is populated on the tree and still never reaches a +// consumer. +func TestContainerProcessTreeImpl_GetPidBranch_CarriesStartTime(t *testing.T) { + cpt := NewContainerProcessTree().(*containerProcessTreeImpl) + + containerID := "test-container-123" + shimPID := uint32(50) + cpt.containerIdToShimPid[containerID] = shimPID + + nginxStart := time.Date(2026, 7, 29, 10, 0, 0, 0, time.UTC) + workerStart := time.Date(2026, 7, 29, 10, 5, 0, 0, time.UTC) + + nginxWorker := &armotypes.Process{ + PID: 101, + PPID: 100, + Comm: "nginx-worker", + StartTime: workerStart, + ChildrenMap: map[armotypes.CommPID]*armotypes.Process{}, + } + nginxProcess := &armotypes.Process{ + PID: 100, + PPID: 50, // parent is shim + Comm: "nginx", + StartTime: nginxStart, + ChildrenMap: map[armotypes.CommPID]*armotypes.Process{ + {Comm: "nginx-worker", PID: 101}: nginxWorker, + }, + } + fullTree := map[uint32]*armotypes.Process{ + 1: {PID: 1, PPID: 0, Comm: "init"}, + 50: {PID: 50, PPID: 1, Comm: "containerd-shim", ChildrenMap: map[armotypes.CommPID]*armotypes.Process{{Comm: "nginx", PID: 100}: nginxProcess}}, + 100: nginxProcess, + 101: nginxWorker, + } + + result, err := cpt.GetPidBranch(containerID, 101, createSafeMapFromData(fullTree)) + assert.NoError(t, err) + assert.Equal(t, nginxStart, result.StartTime, "branch root must carry the source node's StartTime") + + child, ok := result.ChildrenMap[armotypes.CommPID{Comm: "nginx-worker", PID: 101}] + assert.True(t, ok) + assert.Equal(t, workerStart, child.StartTime, "branch leaf must carry the source node's StartTime") +} diff --git a/pkg/utils/processtree_merge.go b/pkg/utils/processtree_merge.go index 1418a2517e..49002bbdab 100644 --- a/pkg/utils/processtree_merge.go +++ b/pkg/utils/processtree_merge.go @@ -53,6 +53,7 @@ func CopyProcess(src *armotypes.Process) *armotypes.Process { Cwd: src.Cwd, Gid: copyUint32Ptr(src.Gid), Uid: copyUint32Ptr(src.Uid), + StartTime: src.StartTime, ChildrenMap: make(map[armotypes.CommPID]*armotypes.Process), } } @@ -87,6 +88,9 @@ func EnrichProcess(target *armotypes.Process, source *armotypes.Process) { if target.Gid == nil && source.Gid != nil { target.Gid = copyUint32Ptr(source.Gid) } + if target.StartTime.IsZero() && !source.StartTime.IsZero() { + target.StartTime = source.StartTime + } } // copyUint32Ptr creates a copy of a uint32 pointer diff --git a/pkg/utils/processtree_merge_test.go b/pkg/utils/processtree_merge_test.go new file mode 100644 index 0000000000..afb7cc5ea5 --- /dev/null +++ b/pkg/utils/processtree_merge_test.go @@ -0,0 +1,28 @@ +package utils + +import ( + "testing" + "time" + + "github.com/armosec/armoapi-go/armotypes" + "github.com/stretchr/testify/assert" +) + +func TestCopyProcess_CarriesStartTime(t *testing.T) { + wall := time.Date(2026, 7, 29, 10, 0, 0, 0, time.UTC) + src := &armotypes.Process{PID: 5, Comm: "a", StartTime: wall} + assert.Equal(t, wall, CopyProcess(src).StartTime, + "CopyProcess strips any field it does not explicitly list — StartTime must be listed") +} + +func TestEnrichProcess_FillsEmptyStartTime(t *testing.T) { + wall := time.Date(2026, 7, 29, 10, 0, 0, 0, time.UTC) + target := &armotypes.Process{PID: 5} + EnrichProcess(target, &armotypes.Process{PID: 5, StartTime: wall}) + assert.Equal(t, wall, target.StartTime) + + // fill-if-empty: an existing value is never overwritten + other := wall.Add(time.Hour) + EnrichProcess(target, &armotypes.Process{PID: 5, StartTime: other}) + assert.Equal(t, wall, target.StartTime) +} From f04c8d8ce8269b655fbeed13365af519650ea5b2 Mon Sep 17 00:00:00 2001 From: Alon Date: Thu, 30 Jul 2026 13:51:45 +0300 Subject: [PATCH 07/14] test(processtree): end-to-end guard that alert branches carry StartTime (SUB-7845) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Walks the real production path — procfs event -> ReportEvent -> creator -> buildBranchToShim -> GetContainerProcessTree — against a creator-populated tree rather than a hand-built fixture. Verified to fail when the branch builder stops carrying StartTime. Docs-exempt: test-only. Co-Authored-By: Claude Opus 5 Signed-off-by: Alon --- pkg/processtree/process_tree_manager_test.go | 60 ++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/pkg/processtree/process_tree_manager_test.go b/pkg/processtree/process_tree_manager_test.go index 3afd6b8900..ca4d039197 100644 --- a/pkg/processtree/process_tree_manager_test.go +++ b/pkg/processtree/process_tree_manager_test.go @@ -1,8 +1,12 @@ package processtree import ( + "os" "testing" + "time" + containercollection "github.com/inspektor-gadget/inspektor-gadget/pkg/container-collection" + eventtypes "github.com/inspektor-gadget/inspektor-gadget/pkg/types" "github.com/kubescape/node-agent/pkg/config" "github.com/kubescape/node-agent/pkg/ebpf/events" containerprocesstree "github.com/kubescape/node-agent/pkg/processtree/container" @@ -26,3 +30,59 @@ func TestManager_GetProcessBootTimeNs(t *testing.T) { assert.Equal(t, uint64(2_340_000_000), mgr.GetProcessBootTimeNs(77)) assert.Zero(t, mgr.GetProcessBootTimeNs(1234), "a pid the tree has never seen reads as 0") } + +// TestManager_GetContainerProcessTree_CarriesStartTime is the end-to-end guard +// for the trap this change exists to avoid: the start time is populated on the +// creator's node, but every consumer reads it through a copy function that +// strips any field it does not explicitly list. This walks the real production +// path — procfs event -> ProcessTreeManager.ReportEvent -> creator -> +// containerprocesstree.buildBranchToShim -> GetContainerProcessTree — against a +// creator-populated tree rather than a hand-built fixture, so it fails if the +// branch builder ever stops carrying StartTime again. +// +// The container is registered through the real ContainerCallback, which derives +// the shim pid by reading the container pid's parent from /proc. That needs a +// pid that actually exists, so the test uses its own. +func TestManager_GetContainerProcessTree_CarriesStartTime(t *testing.T) { + containerTree := containerprocesstree.NewContainerProcessTree() + creator := processtreecreator.NewProcessTreeCreator(containerTree, config.Config{}) + mgr := NewProcessTreeManager(creator, containerTree, config.Config{}) + + self := uint32(os.Getpid()) + shim := uint32(os.Getppid()) + require.NotEqual(t, self, shim, "test needs a distinct parent pid") + + const containerID = "e2e-start-time-container" + containerTree.ContainerCallback(containercollection.PubSubEvent{ + Type: containercollection.EventTypeAddContainer, + Container: &containercollection.Container{ + Runtime: containercollection.RuntimeMetadata{ + BasicRuntimeMetadata: eventtypes.BasicRuntimeMetadata{ + ContainerID: containerID, + ContainerPID: self, + }, + }, + }, + }) + + shimWall := time.Date(2026, 7, 29, 9, 0, 0, 0, time.UTC) + selfWall := time.Date(2026, 7, 29, 9, 30, 0, 0, time.UTC) + + require.NoError(t, mgr.ReportEvent(utils.ProcfsEventType, &events.ProcfsEvent{ + PID: shim, PPID: 1, Comm: "containerd-shim", + StartTimeNs: 1_110_000_000, StartTimeWall: shimWall, + })) + require.NoError(t, mgr.ReportEvent(utils.ProcfsEventType, &events.ProcfsEvent{ + PID: self, PPID: shim, Comm: "nginx", + StartTimeNs: 2_220_000_000, StartTimeWall: selfWall, + })) + + branch, err := mgr.GetContainerProcessTree(containerID, self, false) + require.NoError(t, err) + assert.Equal(t, self, branch.PID) + assert.Equal(t, selfWall, branch.StartTime, + "the branch handed to alerts must carry the creator-populated StartTime") + + // The boot-relative identity for the same process, from the side map. + assert.Equal(t, uint64(2_220_000_000), mgr.GetProcessBootTimeNs(self)) +} From 3269b4514315831640eb4731ee11284c0b229fb2 Mon Sep 17 00:00:00 2001 From: Alon Date: Thu, 30 Jul 2026 15:00:55 +0300 Subject: [PATCH 08/14] test(processtree): cover the real procfs start-time reader; harden tick pinning (SUB-7845) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found newProcfsStartTimeReader had zero coverage — every on-demand test injects a fake reader — so the creator's nsPerTick could drift from the feeder's and give one process two identities an order of magnitude apart, each internally consistent. Verified: mutating it to 10^6 left the whole suite green. Both conversions are now pinned against an independently read field 22 times the contract's literal 10^7, so drift fails deterministically. The previous checks were weaker than they looked: `ns % 10^7 == 0` only catches a 10x error when ticks%10 != 0, and the wall-clock proximity check loses sensitivity on a freshly booted node. Also in the reader: resolve /proc once instead of re-stating the mount point on every call, and log the two btime/mount failure modes that previously degraded silently. Docs-exempt: tests plus logging and a redundant-syscall removal. Co-Authored-By: Claude Opus 5 Signed-off-by: Alon --- pkg/processtree/creator/starttime_reader.go | 32 ++++++++++++----- pkg/processtree/creator/starttime_test.go | 36 ++++++++++++++++++++ pkg/processtree/feeder/procfs_feeder_test.go | 11 ++++++ 3 files changed, 71 insertions(+), 8 deletions(-) diff --git a/pkg/processtree/creator/starttime_reader.go b/pkg/processtree/creator/starttime_reader.go index 579375c8df..5f0b6daab0 100644 --- a/pkg/processtree/creator/starttime_reader.go +++ b/pkg/processtree/creator/starttime_reader.go @@ -4,6 +4,8 @@ import ( "sync" "time" + "github.com/kubescape/go-logger" + "github.com/kubescape/go-logger/helpers" "github.com/prometheus/procfs" ) @@ -33,10 +35,31 @@ const nsPerTick = uint64(10_000_000) func newProcfsStartTimeReader() func(pid uint32) (uint64, time.Time) { var ( once sync.Once + fs procfs.FS + fsReady bool bootTime time.Time ) return func(pid uint32) (uint64, time.Time) { - proc, err := procfs.NewProc(int(pid)) + // Resolve /proc and btime once, not per read: procfs.NewProc would + // re-stat the mount point on every call. + once.Do(func() { + f, err := procfs.NewDefaultFS() + if err != nil { + logger.L().Warning("processtree: cannot open /proc, on-demand start times disabled", helpers.Error(err)) + return + } + fs, fsReady = f, true + if s, err := f.Stat(); err == nil { + bootTime = time.Unix(int64(s.BootTime), 0) + } else { + // Boot-relative identity is unaffected; only the display value is lost. + logger.L().Warning("processtree: cannot read /proc/stat btime, wall-clock start times disabled", helpers.Error(err)) + } + }) + if !fsReady { + return 0, time.Time{} + } + proc, err := fs.Proc(int(pid)) if err != nil { return 0, time.Time{} } @@ -44,13 +67,6 @@ func newProcfsStartTimeReader() func(pid uint32) (uint64, time.Time) { if err != nil || stat.Starttime == 0 { return 0, time.Time{} } - once.Do(func() { - if fs, err := procfs.NewDefaultFS(); err == nil { - if s, err := fs.Stat(); err == nil { - bootTime = time.Unix(int64(s.BootTime), 0) - } - } - }) ns := stat.Starttime * nsPerTick if bootTime.IsZero() { return ns, time.Time{} diff --git a/pkg/processtree/creator/starttime_test.go b/pkg/processtree/creator/starttime_test.go index 43f4fb40f3..712f49998f 100644 --- a/pkg/processtree/creator/starttime_test.go +++ b/pkg/processtree/creator/starttime_test.go @@ -1,9 +1,12 @@ package processtreecreator import ( + "os" "testing" "time" + "github.com/prometheus/procfs" + "github.com/kubescape/node-agent/pkg/config" processtreecreatorconfig "github.com/kubescape/node-agent/pkg/processtree/config" "github.com/kubescape/node-agent/pkg/processtree/conversion" @@ -151,3 +154,36 @@ func TestExitByPid_DeletesStartTimeEntry_NodeAlreadyGone(t *testing.T) { assert.Zero(t, creator.GetProcessBootTimeNs(404), "the early-return branch must reclaim the side-map entry too") } + +// TestProcfsStartTimeReader_ReadsRealProcess covers newProcfsStartTimeReader +// itself. Every other test on the on-demand path injects a fake reader, so +// without this the real /proc parse and its tick->nanosecond conversion have no +// coverage at all: this package's nsPerTick could drift from the feeder's and +// the same process would get two identities an order of magnitude apart, each +// internally consistent. +// +// The conversion is pinned against an independently read field 22 times the +// contract's literal 10^7, so a drifted constant fails deterministically rather +// than only on machines whose uptime happens to make the error visible. +func TestProcfsStartTimeReader_ReadsRealProcess(t *testing.T) { + read := newProcfsStartTimeReader() + + ns, wall := read(uint32(os.Getpid())) + require.NotZero(t, ns, "the test's own process must be readable from /proc") + + p, err := procfs.NewProc(os.Getpid()) + require.NoError(t, err) + stat, err := p.Stat() + require.NoError(t, err) + assert.Equal(t, stat.Starttime*10_000_000, ns, + "conversion must be exactly field 22 ticks * 10^7 ns (USER_HZ=100)") + + assert.False(t, wall.IsZero(), "btime should be readable, so the display value should be set") + assert.WithinDuration(t, time.Now(), wall, 5*time.Minute, + "btime + boot-relative offset must reconstruct this process's actual start time") + + // A pid that cannot exist degrades to zeros rather than guessing. + nsGone, wallGone := read(uint32(1 << 22)) + assert.Zero(t, nsGone, "an unreadable process must yield zero, never a guess") + assert.True(t, wallGone.IsZero()) +} diff --git a/pkg/processtree/feeder/procfs_feeder_test.go b/pkg/processtree/feeder/procfs_feeder_test.go index 9e3c218c1a..69b7cb2c02 100644 --- a/pkg/processtree/feeder/procfs_feeder_test.go +++ b/pkg/processtree/feeder/procfs_feeder_test.go @@ -8,6 +8,7 @@ import ( "github.com/kubescape/node-agent/pkg/processtree" "github.com/kubescape/node-agent/pkg/processtree/conversion" + "github.com/prometheus/procfs" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -130,6 +131,16 @@ func TestProcfsFeeder_ReadProcessInfo_StartTime(t *testing.T) { // (Loose check: StartTimeWall minus the boot-relative duration is in the past.) assert.True(t, event.StartTimeWall.Add(-time.Duration(event.StartTimeNs)).Before(time.Now())) + // Pin the conversion against an independently read field 22 and the contract's + // literal 10^7, so a drifted ticksPerSecond fails deterministically instead of + // only on machines whose uptime happens to make the error visible. + p, err := procfs.NewProc(os.Getpid()) + require.NoError(t, err) + stat, err := p.Stat() + require.NoError(t, err) + assert.Equal(t, stat.Starttime*10_000_000, event.StartTimeNs, + "conversion must be exactly field 22 ticks * 10^7 ns (USER_HZ=100)") + // Sharper check on the arithmetic itself: this is the test binary's own pid, // so its real creation time is moments ago. A wrong btime or a wrong tick // scaling would land this decades or hours away rather than minutes. From e1e5f396649ea2e2cec3ca8146231a3638da68d8 Mon Sep 17 00:00:00 2001 From: Alon Date: Sun, 2 Aug 2026 10:05:11 +0300 Subject: [PATCH 09/14] fix(processtree): a recycled pid must not inherit the dead process's start time (SUB-7845) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A fork's pid is newborn, so a surviving side-map entry can only belong to a process the kernel already recycled the pid away from. Exits linger for exitCleanup.cleanupDelay (5 minutes by default), so the stale entry is readily reachable: A exits on pid 4242, the kernel hands 4242 to B, B's fork event arrives, and B reports A's creation time. If B lives less than one 30s scan interval — the short-lived population the on-demand read exists for — the periodic scan never corrects it. That is worse than the zero it replaced: a consumer joining on (pid, startTime) concludes A and B are the same process, which is exactly the inference this field exists to prevent. Scoped to the side map this change introduces, so it stays additive. This is NOT pid-reuse hardening (SUB-7846): the shared tree node still carries the dead process's comm, cmdline and path. Docs-exempt: correctness fix to a field with no consumer yet. Co-Authored-By: Claude Opus 5 Signed-off-by: Alon --- .../creator/processtree_creator.go | 10 ++++ pkg/processtree/creator/starttime_test.go | 47 +++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/pkg/processtree/creator/processtree_creator.go b/pkg/processtree/creator/processtree_creator.go index 623978a597..ade463756a 100644 --- a/pkg/processtree/creator/processtree_creator.go +++ b/pkg/processtree/creator/processtree_creator.go @@ -200,6 +200,16 @@ func (pt *processTreeCreatorImpl) handleForkEvent(event conversion.ProcessEvent) pt.mutex.Lock() defer pt.mutex.Unlock() + // A fork's pid is newborn, so a surviving side-map entry can only belong to a + // process the kernel already recycled this pid away from. Drop it so + // ensureStartTime reads this incarnation's own creation time instead of + // inheriting the dead one's — exits linger for exitCleanup.cleanupDelay + // (5 minutes by default), so the stale entry is readily reachable. + // + // Scoped to this side map. This is NOT pid-reuse hardening: the shared tree + // node still carries the dead process's comm, cmdline and path. + delete(pt.pidStartTimeNs, event.PID) + proc, ok := pt.processMap.Load(event.PID) if !ok { proc = pt.getOrCreateProcess(event.PID) diff --git a/pkg/processtree/creator/starttime_test.go b/pkg/processtree/creator/starttime_test.go index 712f49998f..7443b20a07 100644 --- a/pkg/processtree/creator/starttime_test.go +++ b/pkg/processtree/creator/starttime_test.go @@ -187,3 +187,50 @@ func TestProcfsStartTimeReader_ReadsRealProcess(t *testing.T) { assert.Zero(t, nsGone, "an unreadable process must yield zero, never a guess") assert.True(t, wallGone.IsZero()) } + +// TestHandleForkEvent_RecycledPidDoesNotInheritDeadProcessStartTime guards the +// case the start time exists to catch. A fork's pid is newborn by definition, so +// a surviving side-map entry can only mean the kernel recycled the pid. +// +// It is reachable with shipped defaults: exitCleanup.cleanupDelay is 5 minutes, +// so an exited process's tree node AND its side-map entry outlive it by that +// long. Without the guard the recycled pid reports the DEAD process's start +// time, and if the new process lives less than one 30s scan interval — the +// short-lived population the on-demand read was added for — the periodic scan +// never corrects it. That is worse than the zero it replaces: a consumer joining +// on (pid, startTime) concludes the two processes are one. +// +// Scoped to this package's side map only. This is NOT pid-reuse hardening: the +// shared tree node still carries the dead process's comm, cmdline and path. +func TestHandleForkEvent_RecycledPidDoesNotInheritDeadProcessStartTime(t *testing.T) { + cfg := config.Config{} + cfg.ExitCleanup = processtreecreatorconfig.ExitCleanupConfig{ + MaxPendingExits: 1000, + CleanupInterval: 30 * time.Second, + CleanupDelay: 5 * time.Minute, // shipped default + } + creator := NewProcessTreeCreator(&mockContainerProcessTree{}, cfg).(*processTreeCreatorImpl) + + const aStart = uint64(1_000_000_000) // process A, boot+1s + const bStart = uint64(9_000_000_000) // process B, boot+9s + creator.readStartTime = func(pid uint32) (uint64, time.Time) { return bStart, time.Time{} } + + // Process A on pid 4242, seen by the periodic scan. + creator.FeedEvent(conversion.ProcessEvent{ + Type: conversion.ProcfsEvent, PID: 4242, PPID: 1, Comm: "victim", StartTimeNs: aStart, + }) + require.Equal(t, aStart, creator.GetProcessBootTimeNs(4242)) + + // A exits. Cleanup is delayed, so the node and the side-map entry survive. + creator.FeedEvent(conversion.ProcessEvent{ + Type: conversion.ExitEvent, PID: 4242, Comm: "exit", Timestamp: time.Now(), + }) + + // The kernel recycles 4242 for process B; B's fork event arrives. + creator.FeedEvent(conversion.ProcessEvent{ + Type: conversion.ForkEvent, PID: 4242, PPID: 1, Comm: "beacon", + }) + + assert.Equal(t, bStart, creator.GetProcessBootTimeNs(4242), + "a newborn pid must get its OWN start time, not the dead process's") +} From d65ebb2846f359a5bd1002444eba77618fcd37a2 Mon Sep 17 00:00:00 2001 From: Alon Date: Sun, 2 Aug 2026 10:19:43 +0300 Subject: [PATCH 10/14] docs(processtree): document the process start time feature (SUB-7845) Covers the boot-relative vs wall-clock split and why they are not interchangeable, the single tick conversion and the rescaling trap, the two population paths, zero-means-unknown and the accepted coverage gaps, the recycled-pid guard and what it deliberately does not cover, the three copy functions that must carry any new Process field, and the omitempty-on-a-struct detail that makes this a changed wire value rather than a new one. Co-Authored-By: Claude Opus 5 Signed-off-by: Alon --- docs/features/process-start-time.md | 149 ++++++++++++++++++++++++++++ 1 file changed, 149 insertions(+) create mode 100644 docs/features/process-start-time.md diff --git a/docs/features/process-start-time.md b/docs/features/process-start-time.md new file mode 100644 index 0000000000..935945e986 --- /dev/null +++ b/docs/features/process-start-time.md @@ -0,0 +1,149 @@ +# Process start time + +Every process in the process tree can carry a creation time. It exists to tell +apart two processes that share a process id — the kernel recycles ids, and +without a creation time a recycled id looks like the process that held it before. + +There are **two representations, and they are not interchangeable**: + +| Value | Where | Unit | Use for | +|---|---|---|---| +| `ProcessTreeManager.GetProcessBootTimeNs(pid)` | Creator side map | Nanoseconds since boot (`CLOCK_BOOTTIME`) | **Process identity.** The only value that may be compared. | +| `armotypes.Process.StartTime` | Tree node, and every alert's process tree | Wall-clock `time.Time` | **Display only.** Never compare it. | + +## Why two + +The identity value must be exact. The display value is derived through +`/proc/stat`'s `btime`, which has **whole-second resolution**, so it carries up +to a second of skew. Two processes created 40 ms apart can share a wall-clock +start time while their boot-relative values differ correctly. + +So: show `Process.StartTime` to a human, compare `GetProcessBootTimeNs` in code. +Anyone comparing the wall-clock value for identity is wrong by up to the btime +skew, and will be wrong intermittently rather than consistently. + +## Where the value comes from + +`/proc//stat` field 22 (`starttime`), the process's creation time in clock +ticks since boot. It is converted **once**, on the way in: + +``` +boot-relative nanoseconds = ticks × 10,000,000 (USER_HZ = 100) +``` + +Nothing downstream rescales it. A division at emission would compile, parse and +even join correctly *within* a single message, while silently breaking identity +across messages by seven orders of magnitude. If you find yourself scaling this +value on output, that is a bug. + +**The unit implies more precision than the value carries.** USER_HZ is 100, so +every value is an exact multiple of 10 ms. That is fine for distinguishing +recycled process ids; do not treat it as a timestamp. + +**It is never derived from an event timestamp.** `convertExecEvent`, +`convertForkEvent` and `convertExitEvent` set `ProcessEvent.StartTimeNs` from the +event's wall-clock timestamp — epoch nanoseconds, a different clock domain, and +stamping the event instant rather than process creation. That value feeds +pending-exit sort ordering only and must never reach the identity map. An exec +event in particular stamps the exec, so a re-exec would change it. + +## When it gets populated + +Two paths, both reading the same kernel source: + +- **The periodic `/proc` scan** (every 30 s) — covers processes that predate the + agent, so coverage completes within one interval of startup. +- **On demand, when a fork or exec creates a tree node** — without this, every + process shorter than one scan interval would carry zero, and short-lived + processes are both the bulk of beacon-style connections and the drivers of + process-id churn. + +The on-demand read happens **once per node creation, never per event**. An exec +on a node that already has a value does not re-read: creation time cannot change +on exec. + +### Zero means unknown + +A read fails if the process is already gone. The value stays zero rather than +being guessed. Callers must treat `0` as "unknown", never as "created at boot". + +Two populations keep a zero start time, both accepted: + +- Processes that died before their creation event was processed (sub-second + queue latency). +- In Kubernetes mode, pre-existing **host** (non-container) processes — the tree + refuses to create nodes for them at all. + +### Recycled process ids + +A fork's process id is newborn, so a surviving identity entry can only belong to +a process the kernel already recycled that id away from. The fork handler drops +the stale entry before reading, so the new process gets its own creation time. + +This matters because exits linger: `exitCleanup.cleanupDelay` defaults to five +minutes, so a dead process's tree node and its identity entry outlive it by that +long. + +> **This guard covers the identity map only.** It is **not** process-id-reuse +> hardening. During that same window the shared tree node still carries the dead +> process's `comm`, `cmdline` and `path`, so an alert can still name the wrong +> command. That is tracked separately. + +The entry is deleted at the same point the tree node is deleted, so the map +cannot outgrow the tree. + +## Reading it + +```go +// Identity — the only value safe to compare. +ns := processTreeManager.GetProcessBootTimeNs(pid) +if ns == 0 { + // Unknown: not yet scanned, died before its creation event, or a host + // process in Kubernetes mode. Fall back to process-id-only handling. +} + +// Display — carried on every node of every alert's process tree. +node.StartTime // wall clock, up to 1s of skew, never compare +``` + +`Process.StartTime` reaches consumers only because three separate copy functions +were taught to carry it. Each builds a node from an **explicit field list** and +silently drops anything absent from it: + +- `containerprocesstree.buildBranchToShim` — every alert branch and every + network-stream tree +- `utils.CopyProcess` — the alert bulk manager's merged tree +- `utils.EnrichProcess` — the bulk manager's merge of overlapping chains + +Each has a dedicated test asserting the field survives. **If you add a field to +`armotypes.Process` and expect it downstream, you must update all three** — +otherwise it is populated at the source and silently empty at the consumer. + +One builder deliberately still strips it: `utils.CreateProcessTree` +(`pkg/utils/process.go`), used by the malware manager, so malware alerts carry a +zero start time. Tracked separately. + +## A note on the wire + +`armotypes.Process.StartTime` is a `time.Time` tagged `omitempty`, and Go's +encoder ignores `omitempty` on structs. The field has therefore always been +serialised — as `"startTime":"0001-01-01T00:00:00Z"` — so populating it changes +an existing value rather than adding a new one. Nothing branches on it and there +is no hash or equality over `Process`, but consumers do see the change. + +## Testing + +This package cannot be built or tested on macOS: `inspektor-gadget/pkg/utils/host` +excludes darwin and everything under `pkg/processtree` imports it transitively. + +```bash +docker run --rm -v "$PWD":/src -v "$(go env GOMODCACHE)":/go/pkg/mod -w /src \ + -e GOFLAGS=-mod=mod golang:1.25 go test ./pkg/processtree/... ./pkg/utils/... +``` + +The tick conversion is defined once per package (feeder and creator). Both are +pinned in tests against an independently read field 22 times a literal `10^7`, +so if the two ever drift, a test fails deterministically. Weaker forms of that +check are not enough: `ns % 10^7 == 0` only catches a 10× error when +`ticks % 10 != 0`, and comparing the wall-clock value against "now" loses +sensitivity on a freshly booted machine. From a093e8ea758325c377efff6f3c9cdc9af5a7a9e4 Mon Sep 17 00:00:00 2001 From: Alon Date: Sun, 2 Aug 2026 11:03:34 +0300 Subject: [PATCH 11/14] fix(processtree): restamp the display start time when a fork reuses a pid (SUB-7845) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review catch. The recycled-pid guard cleared the boot-relative side-map entry but left the dead process's wall-clock Process.StartTime on the reused node, because ensureStartTime only assigns the display value when it is zero. For exactly the case the guard exists to handle, the identity value and the value shown to a human disagreed. Moved the reset to where reuse is actually detected — an existing node on a fork event — which also covers the narrower case where the side-map entry is already gone but the node survives. The test now asserts the node's display value, not just the accessor; it previously missed this. Also documents that (pid, startTimeNs) is not unique: the 10ms tick quantization leaves a residual collision when a pid is recycled inside one tick, which matters to consumers building a join key from the tuple. Docs-exempt: feature doc updated in the same commit. Co-Authored-By: Claude Opus 5 Signed-off-by: Alon --- docs/features/process-start-time.md | 18 ++++++++++++--- .../creator/processtree_creator.go | 22 ++++++++++--------- pkg/processtree/creator/starttime_test.go | 16 ++++++++++++-- 3 files changed, 41 insertions(+), 15 deletions(-) diff --git a/docs/features/process-start-time.md b/docs/features/process-start-time.md index 935945e986..72d619f20c 100644 --- a/docs/features/process-start-time.md +++ b/docs/features/process-start-time.md @@ -27,7 +27,7 @@ skew, and will be wrong intermittently rather than consistently. `/proc//stat` field 22 (`starttime`), the process's creation time in clock ticks since boot. It is converted **once**, on the way in: -``` +```text boot-relative nanoseconds = ticks × 10,000,000 (USER_HZ = 100) ``` @@ -37,8 +37,20 @@ across messages by seven orders of magnitude. If you find yourself scaling this value on output, that is a bug. **The unit implies more precision than the value carries.** USER_HZ is 100, so -every value is an exact multiple of 10 ms. That is fine for distinguishing -recycled process ids; do not treat it as a timestamp. +every value is an exact multiple of 10 ms. Do not treat it as a timestamp. + +### `(pid, startTimeNs)` is not guaranteed unique + +The 10 ms quantization means two processes created within the same tick share a +start time. For a **join key** built from `(pid, startTimeNs)` — which is what +the network-stream attribution work uses — that leaves a residual collision: a +process id recycled and reused inside the same 10 ms window produces two +incarnations with an identical tuple, and they are indistinguishable. + +This is narrow but real, and it is a limitation of the source rather than of +this code — `/proc` does not expose anything finer. Treat the tuple as a strong +discriminator, not a unique identifier, and do not build logic that assumes +two matching tuples must be the same process. **It is never derived from an event timestamp.** `convertExecEvent`, `convertForkEvent` and `convertExitEvent` set `ProcessEvent.StartTimeNs` from the diff --git a/pkg/processtree/creator/processtree_creator.go b/pkg/processtree/creator/processtree_creator.go index ade463756a..813a662052 100644 --- a/pkg/processtree/creator/processtree_creator.go +++ b/pkg/processtree/creator/processtree_creator.go @@ -200,19 +200,21 @@ func (pt *processTreeCreatorImpl) handleForkEvent(event conversion.ProcessEvent) pt.mutex.Lock() defer pt.mutex.Unlock() - // A fork's pid is newborn, so a surviving side-map entry can only belong to a - // process the kernel already recycled this pid away from. Drop it so - // ensureStartTime reads this incarnation's own creation time instead of - // inheriting the dead one's — exits linger for exitCleanup.cleanupDelay - // (5 minutes by default), so the stale entry is readily reachable. - // - // Scoped to this side map. This is NOT pid-reuse hardening: the shared tree - // node still carries the dead process's comm, cmdline and path. - delete(pt.pidStartTimeNs, event.PID) - proc, ok := pt.processMap.Load(event.PID) if !ok { proc = pt.getOrCreateProcess(event.PID) + } else { + // A fork's pid is newborn, so an existing node belongs to a process the + // kernel already recycled this pid away from. Exits linger for + // exitCleanup.cleanupDelay (5 minutes by default), so this is readily + // reachable. Drop BOTH halves of the stale start time — the identity + // value and the display value must never disagree — and let + // ensureStartTime restamp them for this incarnation. + // + // Scoped to the start time. This is NOT pid-reuse hardening: the node + // still carries the dead process's comm, cmdline and path. + delete(pt.pidStartTimeNs, event.PID) + proc.StartTime = time.Time{} } pt.UpdatePPID(proc, event) diff --git a/pkg/processtree/creator/starttime_test.go b/pkg/processtree/creator/starttime_test.go index 7443b20a07..cc3a34ae56 100644 --- a/pkg/processtree/creator/starttime_test.go +++ b/pkg/processtree/creator/starttime_test.go @@ -213,11 +213,14 @@ func TestHandleForkEvent_RecycledPidDoesNotInheritDeadProcessStartTime(t *testin const aStart = uint64(1_000_000_000) // process A, boot+1s const bStart = uint64(9_000_000_000) // process B, boot+9s - creator.readStartTime = func(pid uint32) (uint64, time.Time) { return bStart, time.Time{} } + aWall := time.Date(2026, 7, 29, 10, 0, 0, 0, time.UTC) + bWall := time.Date(2026, 7, 29, 10, 30, 0, 0, time.UTC) + creator.readStartTime = func(pid uint32) (uint64, time.Time) { return bStart, bWall } // Process A on pid 4242, seen by the periodic scan. creator.FeedEvent(conversion.ProcessEvent{ - Type: conversion.ProcfsEvent, PID: 4242, PPID: 1, Comm: "victim", StartTimeNs: aStart, + Type: conversion.ProcfsEvent, PID: 4242, PPID: 1, Comm: "victim", + StartTimeNs: aStart, StartTimeWall: aWall, }) require.Equal(t, aStart, creator.GetProcessBootTimeNs(4242)) @@ -233,4 +236,13 @@ func TestHandleForkEvent_RecycledPidDoesNotInheritDeadProcessStartTime(t *testin assert.Equal(t, bStart, creator.GetProcessBootTimeNs(4242), "a newborn pid must get its OWN start time, not the dead process's") + + // The node is reused, so its display value must be restamped too. If only the + // side map is cleared, the identity value and the value shown to a human + // disagree for exactly the case this guard exists to handle. + node, err := creator.GetProcessNode(4242) + require.NoError(t, err) + require.NotNil(t, node) + assert.Equal(t, bWall, node.StartTime, + "the reused node must not keep displaying the dead process's start time") } From 183225aa57ccb100799ddca3081e3f9cf9a2da9c Mon Sep 17 00:00:00 2001 From: Alon Date: Sun, 2 Aug 2026 11:08:50 +0300 Subject: [PATCH 12/14] perf(processtree): take the fork path's /proc read outside the tree lock (SUB-7845) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review catch. The design accepted this read under pt.mutex on the basis that it is skipped when the value is already known — but the recycled-pid guard drops the entry on every fork, so the fork path always reads and that shortcut no longer applies to it. The cost analysed and the cost that exists had diverged. Measured: ~7.5us per /proc//stat open-read-parse. Fork runs to thousands per second on a busy node, so at 1,000/s that is ~7.5ms per second of hold time on a lock every alert type contends on, and ~22ms/s at 3,000/s. A fork always needs the value, so reading before the lock is the same number of reads with none of them holding it — a local reordering, not a change to lock scope or discipline. Exec keeps its read inside ensureStartTime, where the skip-when-known check still makes it conditional. TestHandleForkEvent_ReadsStartTimeWithoutHoldingTreeLock pins the property with TryRLock, so a regression fails instead of deadlocking. Docs-exempt: feature doc updated in the same commit. Co-Authored-By: Claude Opus 5 Signed-off-by: Alon --- docs/features/process-start-time.md | 26 ++++++++++++++++ .../creator/processtree_creator.go | 25 ++++++++++++++- pkg/processtree/creator/starttime_test.go | 31 +++++++++++++++++++ 3 files changed, 81 insertions(+), 1 deletion(-) diff --git a/docs/features/process-start-time.md b/docs/features/process-start-time.md index 72d619f20c..fac782193a 100644 --- a/docs/features/process-start-time.md +++ b/docs/features/process-start-time.md @@ -74,6 +74,32 @@ The on-demand read happens **once per node creation, never per event**. An exec on a node that already has a value does not re-read: creation time cannot change on exec. +### Cost, and why the fork read is taken outside the tree lock + +A single `/proc//stat` open-read-parse measures **~7.5 µs** (Go benchmark, +Linux container, warm). Fork is the highest-volume event path on a node — +thousands per second on a busy one — and **every fork performs a read**, because +the recycled-pid guard below drops any stale value before the read decision is +made. + +At 1,000 forks/s that is ~7.5 ms of read time per second; at 3,000/s, ~22 ms/s. +Taken while holding `pt.mutex`, the tree-wide write lock, that would be hold +time blocking every reader of the process tree — and the tree is shared by every +alert type, not just the consumer this field was added for. + +So the fork path reads **before** acquiring the lock and stores the result under +it. Same number of reads, none of them holding the lock; fork contributes zero +added lock hold time. `TestHandleForkEvent_ReadsStartTimeWithoutHoldingTreeLock` +pins this, using `TryRLock` so a regression fails rather than deadlocks. + +Exec keeps its read inside the lock, where the skip-when-already-known check +makes it conditional and it only fires on first sight of a pid. + +The tradeoff: reading before the lock widens the window between the event and +the read. If the pid is recycled inside that window the value belongs to the new +incarnation — the same already-accepted race as before, just slightly wider, and +the reuse hardening is what detects it. + ### Zero means unknown A read fails if the process is already gone. The value stays zero rather than diff --git a/pkg/processtree/creator/processtree_creator.go b/pkg/processtree/creator/processtree_creator.go index 813a662052..ed2531473d 100644 --- a/pkg/processtree/creator/processtree_creator.go +++ b/pkg/processtree/creator/processtree_creator.go @@ -186,6 +186,21 @@ func (pt *processTreeCreatorImpl) ensureStartTime(proc *armotypes.Process, pid u return } ns, wall := pt.readStartTime(pid) + pt.applyStartTime(proc, pid, ns, wall) +} + +// applyStartTime stores an already-read start time. Split out from +// ensureStartTime so the fork path can perform its /proc read before taking +// pt.mutex: fork is the highest-volume event path and always reads (the +// recycled-pid guard drops any stale entry), so reading under the tree-wide +// write lock would serialise a file read into the hot path for every alert +// type. Exec keeps the read inside ensureStartTime, where the +// skip-when-already-known check makes it conditional. +// +// A zero ns means the read failed — leave the pre-existing zero, never guess. +// +// Must be called with pt.mutex held. +func (pt *processTreeCreatorImpl) applyStartTime(proc *armotypes.Process, pid uint32, ns uint64, wall time.Time) { if ns == 0 { return } @@ -197,6 +212,14 @@ func (pt *processTreeCreatorImpl) ensureStartTime(proc *armotypes.Process, pid u // handleForkEvent handles fork events - only fills properties if they are empty or don't exist func (pt *processTreeCreatorImpl) handleForkEvent(event conversion.ProcessEvent) { + // Read the start time BEFORE taking the tree lock. A fork always needs it — + // the pid is newborn, and the guard below drops any stale value — so this is + // the same number of reads as doing it under the lock, with none of them + // holding it. Widening the gap between event and read only widens an + // already-accepted race: if the pid is recycled in between, the value read + // belongs to the current incarnation, which the reuse hardening detects. + startTimeNs, startTimeWall := pt.readStartTime(event.PID) + pt.mutex.Lock() defer pt.mutex.Unlock() @@ -241,7 +264,7 @@ func (pt *processTreeCreatorImpl) handleForkEvent(event conversion.ProcessEvent) proc.Path = event.Path } - pt.ensureStartTime(proc, event.PID) + pt.applyStartTime(proc, event.PID, startTimeNs, startTimeWall) if proc.ChildrenMap == nil { proc.ChildrenMap = make(map[armotypes.CommPID]*armotypes.Process) diff --git a/pkg/processtree/creator/starttime_test.go b/pkg/processtree/creator/starttime_test.go index cc3a34ae56..e2bbab1876 100644 --- a/pkg/processtree/creator/starttime_test.go +++ b/pkg/processtree/creator/starttime_test.go @@ -246,3 +246,34 @@ func TestHandleForkEvent_RecycledPidDoesNotInheritDeadProcessStartTime(t *testin assert.Equal(t, bWall, node.StartTime, "the reused node must not keep displaying the dead process's start time") } + +// TestHandleForkEvent_ReadsStartTimeWithoutHoldingTreeLock pins that the fork +// path's /proc read happens OUTSIDE pt.mutex. +// +// pt.mutex is the tree-wide write lock that every alert type contends on, and +// fork is the highest-volume event path on a node — thousands per second on a +// busy one. Since the recycled-pid guard drops any stale entry, every fork now +// performs a read, so holding the lock across it would serialise a file read +// into the hot path for the whole process tree. +// +// TryRLock rather than RLock: if the write lock were held, RLock from this same +// goroutine would deadlock and the test would hang instead of failing. +func TestHandleForkEvent_ReadsStartTimeWithoutHoldingTreeLock(t *testing.T) { + creator := NewProcessTreeCreator(&mockContainerProcessTree{}, config.Config{}).(*processTreeCreatorImpl) + + readWasLockFree := false + creator.readStartTime = func(pid uint32) (uint64, time.Time) { + if creator.mutex.TryRLock() { + readWasLockFree = true + creator.mutex.RUnlock() + } + return 555_550_000_000, time.Time{} + } + + creator.FeedEvent(conversion.ProcessEvent{Type: conversion.ForkEvent, PID: 100, PPID: 1, Comm: "curl"}) + + assert.True(t, readWasLockFree, + "the fork path's /proc read must not hold the tree-wide write lock") + assert.Equal(t, uint64(555_550_000_000), creator.GetProcessBootTimeNs(100), + "and the value must still be stored") +} From a163e77e47e98c13fe731cd7cffe7f4367e0874d Mon Sep 17 00:00:00 2001 From: Alon Date: Mon, 3 Aug 2026 09:22:56 +0300 Subject: [PATCH 13/14] fix(processtree): gate the recycled-pid wipe on a pending exit (SUB-7845) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Maintainer review. An existing node on a fork does not prove pid reuse — some other path may simply have created it first — so wiping unconditionally discarded a good scan-recorded start time whenever the on-demand read then failed, leaving both values zero. Gating on pendingExits makes the signal precise rather than heuristic, and makes the comment true. Also documents the limitation the same review surfaced: the dead process's pendingExits entry survives the fork, so the delayed cleanup later runs exitByPid on what is by then the live successor's node and the pid reverts to unknown. Verified on this head. That is the pre-existing pid-reuse behaviour and belongs with the reuse hardening, not here. Docs-exempt: feature doc updated in the same commit. Signed-off-by: Alon --- docs/features/process-start-time.md | 30 +++++++++++++++---- .../creator/processtree_creator.go | 22 +++++++++----- pkg/processtree/creator/starttime_test.go | 29 ++++++++++++++++++ 3 files changed, 67 insertions(+), 14 deletions(-) diff --git a/docs/features/process-start-time.md b/docs/features/process-start-time.md index fac782193a..662b471515 100644 --- a/docs/features/process-start-time.md +++ b/docs/features/process-start-time.md @@ -122,13 +122,31 @@ This matters because exits linger: `exitCleanup.cleanupDelay` defaults to five minutes, so a dead process's tree node and its identity entry outlive it by that long. -> **This guard covers the identity map only.** It is **not** process-id-reuse -> hardening. During that same window the shared tree node still carries the dead -> process's `comm`, `cmdline` and `path`, so an alert can still name the wrong -> command. That is tracked separately. +The wipe is gated on the pid actually having a **pending exit**. An existing node +alone does not prove reuse — some other path may simply have created it first — +and wiping on that weaker signal would discard a good scan-recorded value +whenever the on-demand read then fails. -The entry is deleted at the same point the tree node is deleted, so the map -cannot outgrow the tree. +> **This guard covers the identity map only.** It is **not** process-id-reuse +> hardening, and its benefit is temporary. +> +> The dead process's entry in `pendingExits` survives the fork. When the delayed +> cleanup fires — up to `cleanupDelay` after the *original* exit — it runs +> `exitByPid` on that pid and deletes what is by then the **live** successor's +> node, along with the start time just read for it. The pid reverts to reading +> as unknown. +> +> During the same window the node also still carries the dead process's `comm`, +> `cmdline` and `path`, so an alert can name the wrong command. +> +> Both are the pre-existing pid-reuse behaviour, tracked separately. The fix is +> to retire the predecessor when a fork reuses a pid with a pending exit — +> reparenting its children rather than merely dropping the pending entry, which +> would leave them to be inherited by the successor. + +The entry is deleted at the same point the tree node is deleted. Note that per +the above, the node deleted may by then belong to a different process than the +one whose exit scheduled the deletion. ## Reading it diff --git a/pkg/processtree/creator/processtree_creator.go b/pkg/processtree/creator/processtree_creator.go index ed2531473d..608ce0d123 100644 --- a/pkg/processtree/creator/processtree_creator.go +++ b/pkg/processtree/creator/processtree_creator.go @@ -226,16 +226,22 @@ func (pt *processTreeCreatorImpl) handleForkEvent(event conversion.ProcessEvent) proc, ok := pt.processMap.Load(event.PID) if !ok { proc = pt.getOrCreateProcess(event.PID) - } else { - // A fork's pid is newborn, so an existing node belongs to a process the - // kernel already recycled this pid away from. Exits linger for - // exitCleanup.cleanupDelay (5 minutes by default), so this is readily - // reachable. Drop BOTH halves of the stale start time — the identity - // value and the display value must never disagree — and let - // ensureStartTime restamp them for this incarnation. + } else if _, exited := pt.pendingExits[event.PID]; exited { + // The previous holder of this pid exited but its node is still around — + // exits linger for exitCleanup.cleanupDelay, 5 minutes by default — so a + // fork on it means the kernel recycled the pid. Drop BOTH halves of the + // stale start time, the identity value and the display value, so they + // cannot disagree, and let applyStartTime restamp them below. + // + // Gated on a pending exit rather than on the node merely existing: an + // existing node alone only means some other path created it first, and + // wiping on that weaker signal would discard a good scan-recorded value + // whenever the on-demand read then fails. // // Scoped to the start time. This is NOT pid-reuse hardening: the node - // still carries the dead process's comm, cmdline and path. + // still carries the dead process's comm, cmdline and path, and the dead + // process's pendingExits entry still schedules a delayed exitByPid that + // will remove this live successor's node. Both belong to SUB-7846. delete(pt.pidStartTimeNs, event.PID) proc.StartTime = time.Time{} } diff --git a/pkg/processtree/creator/starttime_test.go b/pkg/processtree/creator/starttime_test.go index e2bbab1876..0d02c661db 100644 --- a/pkg/processtree/creator/starttime_test.go +++ b/pkg/processtree/creator/starttime_test.go @@ -277,3 +277,32 @@ func TestHandleForkEvent_ReadsStartTimeWithoutHoldingTreeLock(t *testing.T) { assert.Equal(t, uint64(555_550_000_000), creator.GetProcessBootTimeNs(100), "and the value must still be stored") } + +// A fork on an existing node does not always mean pid reuse — some other path +// may simply have created the node first. Wiping unconditionally destroys a +// good scan-recorded value whenever the on-demand read then fails, so the wipe +// is gated on there actually being a pending exit for the pid. +func TestHandleForkEvent_DoesNotWipeKnownStartTimeWithoutAPendingExit(t *testing.T) { + cfg := config.Config{} + cfg.ExitCleanup = processtreecreatorconfig.ExitCleanupConfig{ + MaxPendingExits: 1000, CleanupInterval: time.Hour, CleanupDelay: time.Minute, + } + creator := NewProcessTreeCreator(&mockContainerProcessTree{}, cfg).(*processTreeCreatorImpl) + creator.readStartTime = func(pid uint32) (uint64, time.Time) { return 0, time.Time{} } // read fails + + wall := time.Date(2026, 8, 3, 9, 0, 0, 0, time.UTC) + creator.FeedEvent(conversion.ProcessEvent{ + Type: conversion.ProcfsEvent, PID: 100, PPID: 1, Comm: "nginx", + StartTimeNs: 1_000_000_000, StartTimeWall: wall, + }) + require.Equal(t, uint64(1_000_000_000), creator.GetProcessBootTimeNs(100)) + + // No exit for this pid, so the node is still nginx's — not a recycled pid. + creator.FeedEvent(conversion.ProcessEvent{Type: conversion.ForkEvent, PID: 100, PPID: 1, Comm: "nginx"}) + + assert.Equal(t, uint64(1_000_000_000), creator.GetProcessBootTimeNs(100), + "a failed read must not destroy a value the scan already recorded") + node, err := creator.GetProcessNode(100) + require.NoError(t, err) + assert.Equal(t, wall, node.StartTime, "nor the display value") +} From e575a268009197c951d8354438c62e0d28a341e9 Mon Sep 17 00:00:00 2001 From: Alon Liwsky <40373481+AlonLiwsky@users.noreply.github.com> Date: Mon, 3 Aug 2026 11:06:16 +0300 Subject: [PATCH 14/14] Update pkg/processtree/creator/processtree_creator.go Co-authored-by: Matthias Bertschy Signed-off-by: Alon Liwsky <40373481+AlonLiwsky@users.noreply.github.com> --- pkg/processtree/creator/processtree_creator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/processtree/creator/processtree_creator.go b/pkg/processtree/creator/processtree_creator.go index 608ce0d123..4d508b2451 100644 --- a/pkg/processtree/creator/processtree_creator.go +++ b/pkg/processtree/creator/processtree_creator.go @@ -205,7 +205,7 @@ func (pt *processTreeCreatorImpl) applyStartTime(proc *armotypes.Process, pid ui return } pt.pidStartTimeNs[pid] = ns - if !wall.IsZero() && proc.StartTime.IsZero() { + if !wall.IsZero() { proc.StartTime = wall } }