fix: match session webhook PID to parent when wrapper scripts are used#101
fix: match session webhook PID to parent when wrapper scripts are used#101Spirotot wants to merge 1 commit intohappier-dev:devfrom
Conversation
When the daemon spawns a session through a wrapper script (e.g. a Node.js entrypoint that spawns the actual binary as a child process), the daemon tracks the wrapper's PID while the session reports the binary's PID via webhook. This mismatch causes: 1. The session to be registered as "externally-started" instead of correlating with the daemon-spawned entry 2. A ~90-second webhook timeout on the wrapper PID 3. Delayed spawn response to the client When an unknown PID reports via webhook, check if its parent PID (PPID) matches any daemon-tracked PID. If found, re-key the tracked session to the actual PID and resolve pending awaiters immediately. Uses a lightweight ps call (Unix only, <10ms, 1s timeout guard). On Windows the check is skipped and existing behavior is preserved.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
Greptile SummaryFixes daemon session tracking when wrapper scripts spawn the actual binary. When a session webhook reports an unknown PID, checks if its parent PID (PPID) matches a daemon-tracked session, then re-keys the tracking to the actual process PID and resolves pending awaiters immediately.
Important: Missing test coverage for the new PPID matching logic. Should add tests to verify re-keying behavior, awaiter resolution, and error handling. Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| apps/cli/src/daemon/sessions/onHappySessionWebhook.ts | adds PPID matching to fix wrapper script session tracking, logic is sound but needs test coverage and input validation |
Sequence Diagram
sequenceDiagram
participant Daemon
participant Wrapper as Wrapper Script<br/>(PID 1000)
participant Binary as Session Binary<br/>(PID 1001)
Note over Daemon: Tracks session by PID 1000
Daemon->>Wrapper: spawn wrapper script
activate Wrapper
Wrapper->>Binary: spawn actual binary
activate Binary
Wrapper-->>Daemon: returns (tracked as PID 1000)
Note over Daemon: Awaiting webhook from PID 1000
Binary->>Daemon: webhook with PID 1001
Note over Daemon: PID 1001 not in tracked map!
Daemon->>Daemon: getParentPid(1001) → 1000
Daemon->>Daemon: find session for PID 1000
Note over Daemon: Match found! Re-key session
Daemon->>Daemon: delete session[1000]
Daemon->>Daemon: set session[1001]
Daemon->>Daemon: resolve awaiter immediately
deactivate Binary
deactivate Wrapper
Last reviewed commit: a6aec5c
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function getParentPid(pid: number): number | null { | ||
| if (process.platform === 'win32') return null; | ||
| try { | ||
| const stdout = execSync(`ps -o ppid= -p ${pid}`, { encoding: 'utf-8', timeout: 1000 }); |
There was a problem hiding this comment.
add input validation to ensure pid is a number before using in shell command
| function getParentPid(pid: number): number | null { | |
| if (process.platform === 'win32') return null; | |
| try { | |
| const stdout = execSync(`ps -o ppid= -p ${pid}`, { encoding: 'utf-8', timeout: 1000 }); | |
| function getParentPid(pid: number): number | null { | |
| if (process.platform === 'win32') return null; | |
| // Runtime validation for shell command safety | |
| if (!Number.isInteger(pid) || pid <= 0) return null; | |
| try { |
| // PID not in tracked map. Check if this is a child of a tracked PID — | ||
| // this happens when a wrapper script (e.g. Node.js entrypoint) spawns | ||
| // the actual session binary as a child process, causing a PID mismatch | ||
| // between what the daemon spawned and what the session reports. | ||
| const ppid = getParentPid(pid); | ||
| const parentSession = ppid ? pidToTrackedSession.get(ppid) : null; | ||
|
|
||
| if (parentSession && parentSession.startedBy === 'daemon') { | ||
| // Re-key the tracked session from wrapper PID to actual session PID | ||
| pidToTrackedSession.delete(ppid); | ||
| parentSession.pid = pid; | ||
| parentSession.happySessionId = sessionId; | ||
| parentSession.happySessionMetadataFromLocalWebhook = normalizedMetadata; | ||
| pidToTrackedSession.set(pid, parentSession); | ||
| logger.debug(`[DAEMON RUN] Re-keyed daemon session from wrapper PID ${ppid} to actual PID ${pid}`); | ||
|
|
||
| // Resolve any awaiter that was waiting on the wrapper PID | ||
| const awaiter = pidToAwaiter.get(ppid); | ||
| if (awaiter) { | ||
| pidToAwaiter.delete(ppid); | ||
| awaiter(parentSession); | ||
| logger.debug(`[DAEMON RUN] Resolved session awaiter via parent PID ${ppid}`); | ||
| } |
There was a problem hiding this comment.
add test coverage for the parent PID matching logic - should verify:
- child process correctly re-keyed when PPID matches daemon-spawned session
- awaiter resolved when parent PID match found
- falls back to externally-started when PPID doesn't match
- handles
getParentPidreturning null gracefully
1 participant
Summary
Implementation
getParentPid()helper usingps -o ppid= -p <pid>(Unix only, <10ms, 1s timeout guard)Context
Discovered with compiled binary deployments where a Node.js entrypoint wrapper spawns the actual Happier binary. The daemon tracks the wrapper's PID, but the session webhook reports the binary's PID. This causes a PID mismatch that results in a 90-second timeout before the spawn response reaches the client, even though the session itself works fine.
Test plan