Repository navigation
fix(odin): Shut down cleanly with running actions and orphaned extractor processes (EDG-1033) - #590
vikramlc-cognite wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a clean shutdown mechanism and a parent-process watchdog for the unstable extractor package. On shutdown, running actions are cancelled and given up to 10 seconds to finish, with any unfinished actions reported as canceled before a final check-in is sent. Additionally, a watchdog thread is introduced to monitor the parent process and shut down the extractor gracefully if the parent process dies. Feedback on these changes highlights a potential issue on Windows where psutil.pid_exists does not guard against PID reuse, which could prevent the watchdog from detecting that the parent process has terminated. It is recommended to instantiate and cache a psutil.Process object to reliably check if the original parent process is still running.
| def _parent_is_alive(parent_pid: int) -> bool: | ||
| """ | ||
| Check whether the process with the given PID, which spawned this process, is still running. | ||
|
|
||
| On POSIX an orphaned process is re-parented, so a changed parent PID means the original parent is gone. Windows | ||
| does not re-parent, so there the parent is looked up by PID (psutil also guards against PID reuse). | ||
| """ | ||
| if sys.platform != "win32": | ||
| return os.getppid() == parent_pid | ||
| return psutil.pid_exists(parent_pid) # pragma: no cover - Windows only |
There was a problem hiding this comment.
On Windows, psutil.pid_exists(parent_pid) only checks if any process with the given PID exists. It does not guard against PID reuse, which is a common occurrence on Windows. If the parent process dies and its PID is recycled, the watchdog will incorrectly assume the parent is still alive, leaving the orphan extractor process running indefinitely.
To reliably guard against PID reuse, we should instantiate a psutil.Process object once and use its is_running() method, which matches both the PID and the process creation time. We can cache this process object on the function itself to keep the implementation self-contained and efficient.
def _parent_is_alive(parent_pid: int) -> bool:
"""
Check whether the process with the given PID, which spawned this process, is still running.
On POSIX an orphaned process is re-parented, so a changed parent PID means the original parent is gone. Windows
does not re-parent, so there the parent is looked up by PID (using a cached psutil.Process to guard against PID reuse).
"""
if sys.platform != "win32":
return os.getppid() == parent_pid
parent_proc = getattr(_parent_is_alive, "_parent_proc", None)
if parent_proc is None or parent_proc.pid != parent_pid:
try:
parent_proc = psutil.Process(parent_pid)
_parent_is_alive._parent_proc = parent_proc
except psutil.NoSuchProcess:
return False
return parent_proc.is_running()
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## EDG-1036-prompt-action-status #590 +/- ##
=================================================================
+ Coverage 84.75% 84.93% +0.18%
=================================================================
Files 46 46
Lines 4761 4819 +58
=================================================================
+ Hits 4035 4093 +58
Misses 726 726
🚀 New features to boost your workflow:
|
…tor processes - On exit, cancel running actions, wait a bounded time for them, report canceled for any still running, and flush with a fresh token and timeout so the final statuses are actually sent. - Add a parent-process watchdog so the extractor process shuts down gracefully when the runtime process is killed. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
ef29281 to
e046cd1
Compare
Summary
On Ctrl+C or a service stop, running actions now get a final status that is actually sent, and the extractor process shuts itself down if its runtime process is killed.
Type of change
What changed
base.py__exit__:stop(), then cancel running action tokens and join their threads (bounded, 10 s), then queuecanceled("Extractor shut down while the action was running") for any action that did not stop, then the final flush with a fresh token and a 10 s bound. Previously the flush reused the already-cancelled token, so nothing was sent.getppidon POSIX,psutil.pid_existson Windows) every second and shuts down gracefully when it is gone. Only started when the runtime passes aparent_pid.tests/test_unstable/test_clean_shutdown.py; CHANGELOG.Why it changed
runningat Odin until the next start (thenfailedwith no message). Afterkill -9of the runtime the extractor child kept running and checking in, so a restart gave two processes on one integration.What to focus on during review
canceled; a tiny race could report twice).__exit__now always does a final check-in over HTTP, even when idle. Check any test or tool that counts check-ins on exit.Test evidence
tests/test_unstable/test_clean_shutdown.py: 7 tests (finalcanceledreaches the client on__exit__, stuck action reported with the shutdown message in under 5 s, bounded flush when the post hangs, watchdog cancels when the parent disappears, no watchdog withoutparent_pid)uv run pytest tests/test_unstable: same 1 failed + 48 environment errors asmaster; passed count 279 -> 286ruff,mypy,pre-commit run --all-filespassrunning(the bug reproduced).Risks and unknowns
prctl(PR_SET_PDEATHSIG)not used (SIGTERM would skip the graceful status report).Rollout and rollback
Checklist