Repository navigation
Served-profile children and background threads run with their own profile's env/scope; kanban never kills a recycled PID #111617
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
7b8a44e
1337754
8317ddb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -256,17 +256,41 @@ def _pid_alive(pid: Optional[int]) -> bool: | |
| return True | ||
|
|
||
|
|
||
| def _worker_alive(pid: Optional[int], started_at: Optional[int]) -> bool: | ||
| """True when ``pid`` is live AND is still the worker we spawned. ``started_at`` is the start-time | ||
| fingerprint recorded by ``_set_worker_pid``; after a reboot (or any PID recycle) an unrelated | ||
| process can own the number, so bare existence is never enough to extend a claim or to signal. | ||
| A legacy row without a fingerprint keeps the existence answer: killing it is the pre-fingerprint | ||
| behaviour and the row is rewritten with a fingerprint on its next spawn.""" | ||
| return _kb._pid_alive(pid) and not _pid_recycled(pid, started_at) | ||
|
|
||
|
|
||
| def _pid_recycled(pid: Optional[int], started_at: Optional[int]) -> bool: | ||
| """True when a live ``pid`` is NOT the process fingerprinted at spawn (or the fingerprint can no | ||
| longer be read). Signalling it would hit a stranger. ``None`` fingerprint = legacy row, never recycled.""" | ||
| if started_at is None or not pid: | ||
| return False | ||
| from gateway.status import _start_times_agree, get_process_start_time | ||
| current = get_process_start_time(int(pid)) | ||
| if current is None: | ||
| return True | ||
| try: | ||
| return not _start_times_agree(current, started_at) | ||
| except (TypeError, ValueError): | ||
| return True | ||
|
|
||
|
|
||
| def _kill_fn(signal_fn) -> Optional[Callable[[int, int], None]]: | ||
| """``signal_fn`` test hook, else ``os.kill`` when the platform has one.""" | ||
| if signal_fn is not None: | ||
| return signal_fn | ||
| return os.kill if hasattr(os, "kill") else None | ||
|
|
||
|
|
||
| def _poll_worker_exit(pid: int) -> bool: | ||
| def _poll_worker_exit(pid: int, started_at: Optional[int] = None) -> bool: | ||
| """Poll ~5 s (10 x 0.5 s) for ``pid`` to die; True once it is gone.""" | ||
| for _ in range(10): | ||
| if not _kb._pid_alive(pid): | ||
| if not _worker_alive(pid, started_at): | ||
| return True | ||
| time.sleep(0.5) | ||
| return False | ||
|
|
@@ -287,8 +311,11 @@ def _terminate_reclaimed_worker( | |
| claim_lock: Optional[str], | ||
| *, | ||
| signal_fn=None, | ||
| started_at: Optional[int] = None, | ||
| ) -> dict[str, Any]: | ||
| """Best-effort host-local worker termination for reclaim paths.""" | ||
| """Best-effort host-local worker termination for reclaim paths. ``started_at`` is the spawn-time | ||
| fingerprint: when the live process no longer matches it, the PID was recycled and nothing is | ||
| signalled — the worker is gone, which is what the reclaim wanted (``terminated`` = True).""" | ||
| info: dict[str, Any] = { | ||
| "prev_pid": int(pid) if pid else None, | ||
| "host_local": False, | ||
|
|
@@ -305,6 +332,10 @@ def _terminate_reclaimed_worker( | |
| kill = _kill_fn(signal_fn) | ||
| if kill is None: | ||
| return info | ||
| if _kb._pid_alive(pid) and _pid_recycled(pid, started_at): | ||
| info["terminated"] = True | ||
| info["pid_recycled"] = True | ||
| return info | ||
|
|
||
| info["termination_attempted"] = True | ||
| try: | ||
|
|
@@ -317,14 +348,14 @@ def _terminate_reclaimed_worker( | |
| except OSError: | ||
| return info | ||
|
|
||
| if _poll_worker_exit(pid): | ||
| if _poll_worker_exit(pid, started_at): | ||
| info["terminated"] = True | ||
| return info | ||
| if _kb._pid_alive(pid): | ||
| if _worker_alive(pid, started_at): | ||
| if not _sigkill(kill, pid): | ||
| return info | ||
| info["sigkill"] = True | ||
| info["terminated"] = not _kb._pid_alive(pid) | ||
| info["terminated"] = not _worker_alive(pid, started_at) | ||
| return info | ||
|
|
||
|
|
||
|
|
@@ -427,7 +458,7 @@ def enforce_max_runtime(conn: sqlite3.Connection, *, signal_fn=None) -> list[str | |
| host_prefix = _kb._host_prefix() | ||
|
|
||
| rows = conn.execute( | ||
| "SELECT t.id, t.worker_pid, " | ||
| "SELECT t.id, t.worker_pid, t.worker_started_at, " | ||
| " COALESCE(r.started_at, t.started_at) AS active_started_at, " | ||
| " t.max_runtime_seconds, t.claim_lock " | ||
| "FROM tasks t " | ||
|
|
@@ -449,16 +480,18 @@ def enforce_max_runtime(conn: sqlite3.Connection, *, signal_fn=None) -> list[str | |
|
|
||
| pid = int(row["worker_pid"]) | ||
| tid = row["id"] | ||
| started_at = _kb._row_get(row, "worker_started_at") | ||
| # SIGTERM then SIGKILL after 5 s grace; workers wanting a cleaner | ||
| # shutdown install their own SIGTERM handler. | ||
| # shutdown install their own SIGTERM handler. A recycled PID (fingerprint | ||
| # mismatch) is never signalled: the worker is already gone. | ||
| killed = False | ||
| kill = _kill_fn(signal_fn) | ||
| if kill is not None: | ||
| if kill is not None and not (_kb._pid_alive(pid) and _pid_recycled(pid, started_at)): | ||
| with contextlib.suppress(ProcessLookupError, OSError): | ||
| kill(pid, signal.SIGTERM) | ||
| # Short polling wait — no time.sleep on the write txn. | ||
| _poll_worker_exit(pid) | ||
| if _kb._pid_alive(pid): | ||
| _poll_worker_exit(pid, started_at) | ||
| if _worker_alive(pid, started_at): | ||
| killed = _sigkill(kill, pid) | ||
|
|
||
| error = f"elapsed {int(elapsed)}s > limit {limit}s" | ||
|
|
@@ -529,7 +562,7 @@ def detect_stale_running( | |
| reclaimed: list[str] = [] | ||
|
|
||
| rows = conn.execute( | ||
| "SELECT t.id, t.worker_pid, t.last_heartbeat_at, t.claim_lock, " | ||
| "SELECT t.id, t.worker_pid, t.worker_started_at, t.last_heartbeat_at, t.claim_lock, " | ||
| " COALESCE(r.started_at, t.started_at) AS active_started_at " | ||
| "FROM tasks t " | ||
| "LEFT JOIN task_runs r ON r.id = t.current_run_id " | ||
|
|
@@ -552,7 +585,8 @@ def detect_stale_running( | |
| tid = row["id"] | ||
| lock = row["claim_lock"] or "" | ||
|
|
||
| termination = _kb._terminate_reclaimed_worker(pid, lock, signal_fn=signal_fn) | ||
| termination = _kb._terminate_reclaimed_worker( | ||
| pid, lock, signal_fn=signal_fn, started_at=_kb._row_get(row, "worker_started_at")) | ||
|
|
||
| # Never release a claim while our own worker is still alive: that would | ||
| # spawn a duplicate beside it. Hold the claim and retry next tick. | ||
|
|
@@ -615,14 +649,14 @@ def reconcile_orphaned_running(conn: sqlite3.Connection) -> list[str]: | |
| now = int(time.time()) | ||
| reconciled: list[str] = [] | ||
| rows = conn.execute( | ||
| "SELECT id, claim_lock, claim_expires, worker_pid FROM tasks " | ||
| "SELECT id, claim_lock, claim_expires, worker_pid, worker_started_at FROM tasks " | ||
| "WHERE status = 'running' " | ||
| " AND (claim_lock IS NULL OR claim_expires IS NULL)" | ||
| ).fetchall() | ||
| for row in rows: | ||
| tid = row["id"] | ||
| pid = row["worker_pid"] | ||
| if pid and _kb._pid_alive(pid): | ||
| if pid and _worker_alive(pid, _kb._row_get(row, "worker_started_at")): | ||
| # Never requeue beside a live process. Retry next tick. | ||
| _kb._log.debug( | ||
| "kanban reconcile: task %s has broken claim bookkeeping but " | ||
|
|
@@ -807,7 +841,7 @@ def _reclaim_dead_workers(conn: sqlite3.Connection) -> _CrashSweep: | |
| sweep = _CrashSweep() | ||
| with _kb.write_txn(conn): | ||
| rows = conn.execute( | ||
| "SELECT id, worker_pid, claim_lock, started_at, assignee " | ||
| "SELECT id, worker_pid, worker_started_at, claim_lock, started_at, assignee " | ||
| "FROM tasks " | ||
| "WHERE status = 'running' AND worker_pid IS NOT NULL" | ||
| ).fetchall() | ||
|
|
@@ -821,7 +855,7 @@ def _reclaim_dead_workers(conn: sqlite3.Connection) -> _CrashSweep: | |
| started_at = _kb._row_get(row, "started_at") | ||
| if started_at is not None and time.time() - started_at < _kb._resolve_crash_grace_seconds(): | ||
| continue | ||
| if _kb._pid_alive(row["worker_pid"]): | ||
| if _worker_alive(row["worker_pid"], _kb._row_get(row, "worker_started_at")): | ||
| continue | ||
|
|
||
| pid = int(row["worker_pid"]) | ||
|
|
@@ -1098,13 +1132,18 @@ def _record_task_failure( | |
|
|
||
|
|
||
| def _set_worker_pid(conn: sqlite3.Connection, task_id: str, pid: int) -> None: | ||
| """Record the spawned child's pid + emit a ``spawned`` event carrying it.""" | ||
| """Record the spawned child's pid + its start-time fingerprint, and emit a ``spawned`` event | ||
| carrying them. The fingerprint is what lets every later liveness/kill decision tell OUR worker | ||
| from a process that recycled the PID after a reboot.""" | ||
| from gateway.status import get_process_start_time | ||
| started_at = get_process_start_time(int(pid)) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1 — this persisted fingerprint is not reboot-stable on Linux, so it does not prove the threat model in the docstring.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed on head 42e01d9 (#112685, |
||
| with _kb.write_txn(conn): | ||
| conn.execute("UPDATE tasks SET worker_pid = ? WHERE id = ?", (int(pid), task_id)) | ||
| conn.execute("UPDATE tasks SET worker_pid = ?, worker_started_at = ? WHERE id = ?", | ||
| (int(pid), started_at, task_id)) | ||
| run_id = _kb._current_run_id(conn, task_id) | ||
| if run_id is not None: | ||
| conn.execute("UPDATE task_runs SET worker_pid = ? WHERE id = ?", (int(pid), run_id)) | ||
| _kb._append_event(conn, task_id, "spawned", {"pid": int(pid)}, run_id=run_id) | ||
| _kb._append_event(conn, task_id, "spawned", {"pid": int(pid), "started_at": started_at}, run_id=run_id) | ||
|
|
||
|
|
||
| def _clear_failure_counter(conn: sqlite3.Connection, task_id: str) -> None: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1 —
Noneis safe only for truly legacy rows, but_set_worker_pid()can create it today.get_process_start_time()is optional; if a new spawn's lookup fails, this PR persistsworker_started_at=NULL, then this branch deliberately treats the live PID as our worker and the timeout/reclaim paths can signal it by bare number. That recreates the #89614/#99558 authority failure for a post-migration row. Preserve legacy compatibility separately, but a new unverified spawn must fail closed for destructive signaling. Add a test that forces fingerprint capture toNone, then presents a live/reused PID and proves TERM/KILL is never issued.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed on head 42e01d9 (#112685,
9cb9fd0d0cb9c): a failed capture persists'unverified', never NULL — held while live, never signalled by timeout/stale-claim/manual reclaim/archive/reaper, reclaimed once gone; NULL stays legacy-only —…::test_unverified_fingerprint_capture_never_authorizes_a_signal(red on base).