Skip to content

fix(kanban): trust a worker PID only inside the claim's PID namespace - #104125

Open
rodrigogs wants to merge 3 commits into
NousResearch:mainfrom
rodrigogs:fix/kanban-pid-check-same-namespace
Open

rodrigogs wants to merge 3 commits into
NousResearch:mainfrom
rodrigogs:fix/kanban-pid-check-same-namespace

Conversation

@rodrigogs

@rodrigogs rodrigogs commented Sep 6, 2026 •

Copy link
Copy Markdown

What does this PR do?

A claim is <hostname>:<pid> (_claimer_id, hermes_cli/kanban_db.py:1063). Every reclaim
pass that probes a PID first asked whether the claim's host prefix was its own (_host_prefix,
:1081) and then treated "no such PID here" as "worker dead": _reclaim_dead_workers
(hermes_cli/kanban_db_dispatch.py:835), release_stale_claims (kanban_db.py:2311),
_terminate_reclaimed_worker (kanban_db_dispatch.py:285, then SIGTERM/SIGKILL) and
enforce_max_runtime (:442). _pid_alive (:209) reads /proc/<pid>, which can only answer for
the caller's PID namespace.

Hostname and PID namespace are independent in containers. Compose services that join one
network namespace (network_mode: "service:x") share hostname; unless pid: is also set,
each keeps its own PID namespace, where the same PID number is an unrelated process or
nothing at all.

Observed on such a stack at 03:19:28Z on 2026-09-05: a dispatch from the webui container
found four running claims carrying its own hostname, saw none of the gateway's worker PIDs,
and closed all four runs as crashed "pid N not alive" (_classify_dead_worker). The workers
were alive; 3h29m of work was lost and respawned. The dashboard's POST /dispatch runs the
same passes from the dashboard container on every click of its nudge button, so this is not a
dry-run-only problem. The mirror-image error is equally reachable: a PID number that does
exist locally makes a foreign claim look alive forever, so release_stale_claims keeps
extending it — or a SIGTERM lands on whatever local process holds that number.

The fix records where a claim was made and refuses to trust a PID probe from anywhere else.
The owner is a new leaf sibling, hermes_cli/kanban_db_pidns.py:

LocalPidNamespace (kanban_db_pidns.py:28) / _local_pid_namespace() (:73) this process' namespace identity as a tri-state: supported=False (no /proc/self/ns/pid: macOS, Windows), supported=True with the inode id (Linux, resolved), supported=True with id=None (Linux, but readlink failed). A definite answer is cached per process; a failed lookup is not, so a transient /proc problem is retried on the next call
tasks.claim_pidns new nullable column (SCHEMA_SQL kanban_db.py:877 + _LATER_TASK_COLUMNS kanban_db_connect.py:809), written by _claim_and_open_run (kanban_db.py:2105) in the same CAS update as claim_lock; the claimed event payload gains pidns. It is claim state: every release path that nulls claim_lock nulls it too (10 sites in kanban_db.py, 6 in kanban_db_dispatch.py, one in kanban_swarm.py, and the dashboard's _set_status_direct release UPDATE in plugins/kanban/dashboard/plugin_api.py), so a claim written later by an older binary starts from NULL instead of inheriting the previous claimer's namespace
_claim_pid_checkable(lock, ns, host_prefix=) (kanban_db_pidns.py:91) the single predicate every PID probe and every signal goes through. Host prefix differs → not checkable. No namespace identity on this platform → hostname-only trust, as before. Local identity supported but our own lookup failed → not checkable. Claim recorded no namespace (pre-upgrade row, older writer) while ours is known → not checkable. Both known → equality

A claim that is not checkable is treated exactly like a foreign host's: not marked crashed,
not extended as "pid alive", not signalled, and released by release_stale_claims once the
TTL expires. The reclaimed payload gains pid_checkable next to host_local so an operator
can see why a claim waited for the TTL (_record_reclaim, kanban_db.py:2404, merges as
{**termination, **payload} so the caller's claim-derived values win over the termination
report's same-named "how far did the signal get" keys).

The NULL cases fail closed on purpose. A NULL stored namespace is the mixed-version
boundary: rows claimed before the column existed and claims authored by an older binary
sharing the board. On Linux the reader knows its own namespace, and an older writer sharing
our hostname is indistinguishable from a sibling container sharing it, so hostname is not
authority there either — trusting it would move the cross-namespace destructive path to the
rollout boundary. Likewise a Linux process whose own /proc/self/ns/pid read failed has
unknown authority, not the compatibility case: absence of provenance cannot become provenance
because the lookup failed. The hostname-only fallback survives only where the local platform
has no namespace identity at all.

Every _terminate_reclaimed_worker caller passes the claim's namespace through, and the
parameter is now required (claim_pidns keyword, no default), so no path is left with
hostname-only PID trust: release_stale_claims, the operator's hermes kanban reclaim via
reclaim_task (kanban_db.py:2456), invalidate_descendants_for_parent_reopen (:3389 — its
terminations entries become (worker_pid, claim_lock, claim_pidns)), detect_stale_running
(kanban_db_dispatch.py:536) and the dashboard's _set_status_direct
(plugins/kanban/dashboard/plugin_api.py:669, whose prev SELECT reads the column and whose
release UPDATE nulls it with the same CASE WHEN ? = 'running' guard as claim_lock,
claim_expires and worker_pid — a drag off Running was the one release path that left the
stamp behind until this round; the two dashboard tests now assert the column is NULL after
the move).
enforce_max_runtime and _reclaim_dead_workers skip rows they cannot probe instead of
filtering on the host prefix. kanban_transfer._scrub_local_state nulls claim_pidns along
with the rest of the local claim state, and export_board (kanban_transfer.py:115) now runs
kanban_db_connect.migrate_schema (kanban_db_connect.py:834) on its snapshot before scrubbing:
nothing on the export path opens the source through connect, so the first post-upgrade backup
of a board this release had not touched yet failed with no such column: claim_pidns.

Commit train. Two commits, each meant to be green on its own:

  1. refactor(kanban): move the claim PID-probe gate into kanban_db_pidns — behaviour-neutral.
    Creates kanban_db_pidns.py with _claim_pid_checkable(lock, host_prefix=) reducing to the
    startswith(host_prefix) test the four passes made inline, and routes them through it.
    3 files, +49/−6; no schema, payload, config or test change; kanban_db._host_prefix is
    still read at call time so it stays the patch point.
  2. fix(kanban): trust a worker PID only inside the claim's PID namespace — everything above:
    the column and migration, stamping and clearing, the tri-state identity, the fail-closed
    predicate, the required kwarg, the export migration, docs and tests.

Hosted receipts, stated plainly: CI/Docker/Nix run against the branch head only, so the
refactor commit has no hosted check-runs attached to its object (gh api repos/NousResearch/hermes-agent/commits/66484e28.../check-runs → 0). What exists for commit 1
is the local receipt below (the eleven pre-existing suites the PR touches, unchanged, all
green at that SHA). I have not pushed it as a throwaway head to manufacture hosted runs; if
the acceptance gate needs an exact-object hosted receipt for commit 1, say so and I will
push it as a separate branch head so the runs attach, or squash the train to one commit.

Base: the merge base is 693641a (upstream/main at push time). main has moved on since
(17 commits at the time of this revision); none of them touch any of the 14 files this PR
changes (git log 693641aa8..upstream/main -- <the 14 paths> is empty), so the branch was
not rebased for this round and the diff applies as-is. It will be rebased if a file moves.

Why a namespace stamp and not heartbeat freshness. PR #50118 by GoliathInteractive
"fix(kanban): trust fresh heartbeat over pid miss" is real prior art on this exact root cause
("bounded heartbeat freshness knob for PID namespace / container visibility mismatch"). It was
closed by its own author; the hermes-sweeper review confirmed the premise —
detect_crashed_workers() proceeds from a false _pid_alive() result without consulting
run-owned heartbeat state — and objected only to a new HERMES_KANBAN_CRASH_HEARTBEAT_FRESH_SECONDS
env var and a duplicate test. It is the cheaper change: no schema, no call sites. I chose the
stamp anyway, for two reasons. First, heartbeat freshness tells you a worker went quiet; the
namespace tells you whether the PID answer you got means anything at all — and only the second
can stop a cross-namespace SIGTERM, which enforce_max_runtime, detect_stale_running, the
operator reclaim and the dashboard all send. Second, heartbeat freshness applied to an
unverifiable claim makes recovery slower than what this PR settles for: release_stale_claims
already computes heartbeat_stale against DEFAULT_CLAIM_HEARTBEAT_MAX_STALE_SECONDS (1 h); if
a fresh heartbeat could authorise extending a claim whose PID this process cannot check, a
foreign worker that dies between heartbeats would hold its claim until an hour past its last
heartbeat instead of the 15-minute claim TTL. The two remain complementary the other way round:
inside one already-qualified namespace, a fresh current-run heartbeat vetoing a "pid not alive"
crash is a real improvement, and nothing here stands in its way.

Consequence for container replacement, stated plainly. With a stable hostname, a
recreated gateway reclaimed its predecessor's dead claims on the first tick. The predecessor's
namespace is gone, so those claims now wait for the claim TTL (claim_expires, 15 min default)
like any other unverifiable claim, then are released without a signal, with
pid_checkable: false on the reclaimed event. Pre-upgrade running claims and claims from
an older binary sharing the board take the same path on Linux. That is the pre-hostname:
recovery latency and the safe floor; the alternative is what happened above. Stacks that want
first-tick recovery and a safe dashboard nudge share one PID namespace
(pid: "service:gateway" on webui/dashboard), which makes the ids equal and re-enables the
fast path — now documented in kanban.md and docker.md. #59799 by chrismaddern (point 2)
proposes a foreign-claim grace period for faster reclaim of unverifiable claims; this change
routes namespace mismatches onto exactly the path it would speed up, and adds no fast path of
its own.

Interaction with #104144 (capacity_deferred): on a capped board whose cap is filled by
dead, non-checkable claims, the two changes together can keep the "dispatcher stuck" warning
quiet for up to the claim TTL — the claims are not reclaimed until claim_expires, and until
then the cap legitimately reads as full. Mitigation is the TTL release itself, and
hermes kanban dispatch --json shows capacity_deferred and the reclaim counters so the state
is visible. Both bodies name this; neither PR duplicates the other's code.

Deliberately unchanged: the claim-lock string format — workers compare
HERMES_KANBAN_CLAIM_LOCK for equality on heartbeat (tools/kanban_tools.py) and the string
appears in dashboards and events; folding the namespace into _host_prefix or _claimer_id
would make every claim from a recreated container foreign to all passes and change what
host_local means everywhere. reconcile_orphaned_running keeps its logic (its release UPDATE
only gains the claim_pidns = NULL clear); its rows have no valid claim to qualify — the
separate #82622 / #96629 gap. heartbeat_claim and the live-claim extension helper keep
their logic. task_runs gets no column. No new env var or
config key. The dashboard's GET /runs/{run_id}/inspect still psutil-probes the worker PID
from the dashboard's own namespace and so can report the same lie ("no such process") — it is
read-only and destroys nothing; making its output namespace-aware is explicit follow-up, not
authoritative liveness.

Related Issue

Related: #59799 (point 2: foreign-host claims after container replacement). Prior art on the
same root cause: #50118 (author-closed; discussed above). The read-only dry-run half of the same
incident is #94916, fixed by #104119; the two are independent and can land in either order
(both touch dispatch, so the second may need a textual rebase). Interaction with #104144
discussed above.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/kanban_db_pidns.py (new): LocalPidNamespace, _parse_pid_namespace_link,
    _resolve_local_pid_namespace, _local_pid_namespace, _pid_namespace_id,
    _claim_pid_checkable.
  • hermes_cli/kanban_db.py: SCHEMA_SQL gains claim_pidns TEXT; _claim_and_open_run stamps
    it and adds pidns to the claimed payload; release_stale_claims selects it, gates
    extend/probe on the predicate and adds pid_checkable to the reclaimed payload;
    _record_reclaim merges {**termination, **payload}; reclaim_task and
    invalidate_descendants_for_parent_reopen pass it through (3-tuple terminations); every
    release UPDATE that nulls claim_lock nulls claim_pidns.
  • hermes_cli/kanban_db_connect.py: ("claim_pidns", "claim_pidns TEXT") in _LATER_TASK_COLUMNS
    with a comment stating the fail-closed semantics; migrate_schema(conn) extracted from the
    first-open init so export can run the same pass on a snapshot.
  • hermes_cli/kanban_db_dispatch.py: _terminate_reclaimed_worker(..., *, claim_pidns, ...)
    (required) gates every signal on the predicate and reports pid_checkable;
    _reclaim_dead_workers, enforce_max_runtime, detect_stale_running select claim_pidns
    and use it; release UPDATEs null the column.
  • hermes_cli/kanban_swarm.py: the root-task completion UPDATE nulls claim_pidns.
  • hermes_cli/kanban_transfer.py: export_board migrates the snapshot before
    _scrub_local_state, which nulls claim_pidns.
  • plugins/kanban/dashboard/plugin_api.py: _set_status_direct reads claim_pidns in its
    prev SELECT, forwards it with the PID, and its release UPDATE nulls claim_pidns beside
    claim_lock / claim_expires / worker_pid (same CASE WHEN ? = 'running' guard).
  • website/docs/user-guide/features/kanban.md (claimed / reclaimed event rows, single-host
    note with the namespace rule and the pid: "service:gateway" remedy),
    website/docs/user-guide/features/kanban-worker-lanes.md (stale-claim / crashed-worker
    bullets), website/docs/user-guide/docker.md (dashboard PID-namespace note).
  • tests/hermes_cli/test_kanban_claim_pid_namespace.py (new, 26 cases; see the note under
    "How to Test" on why this exceeds the 1–2-per-fix bar);
    tests/hermes_cli/test_kanban_transfer.py (+1: export of a board this release has not
    opened); tests/hermes_cli/test_kanban_core_functionality.py (the reclaim fixture stamps the
    namespace like a real claimer); tests/hermes_cli/test_kanban_parent_reopen_invalidation.py
    (3-tuple shape).

How to Test

  1. Two containers, same hostname, no pid: sharing, one board volume. Spawn a worker from
    A; from B run hermes kanban dispatch. Before: the task is closed crashed "pid N not
    alive". After: still running, no crashed event, run still open.
  2. Same, with pid: "service:A" on B: B reclaims A's genuinely dead workers on the first
    tick (namespace ids equal).
  3. Recreate A while a claim is running: the claim is released once claim_expires passes and
    the reclaimed payload shows host_local: true, pid_checkable: false.
  4. From B, drag a running card out of Running in the dashboard, or run
    hermes kanban reclaim <id>: the card is released and no signal is sent to A's PID
    number.
  5. hermes kanban boards export <slug> on a board no post-upgrade command has opened: writes
    the archive instead of dying with no such column: claim_pidns.
  6. scripts/run_tests.sh -j 1 tests/hermes_cli/test_kanban_claim_pid_namespace.py tests/hermes_cli/test_kanban_transfer.py
    and
    scripts/run_tests.sh -j 1 tests/hermes_cli/test_kanban_db.py tests/hermes_cli/test_kanban_core_functionality.py tests/hermes_cli/test_kanban_reclaim_claim_lock_guard.py tests/hermes_cli/test_kanban_parent_reopen_invalidation.py tests/hermes_cli/test_kanban_worker_lifecycle_hooks.py tests/hermes_cli/test_kanban_review_lifecycle_complete.py tests/hermes_cli/test_signal_handler_kanban_worker.py tests/tools/test_kanban_tools.py tests/gateway/test_kanban_reconcile_orphans.py tests/plugins/test_kanban_dashboard_plugin.py tests/hermes_cli/test_kanban_cli_dispatch_passthrough.py tests/hermes_cli/test_kanban_notify.py tests/hermes_cli/test_kanban_swarm.py

tests/hermes_cli/test_kanban_claim_pid_namespace.py — 26 cases. Identity: the readlink text →
inode parser as a pure mapping; the real /proc/self/ns/pid read (linux_only); a Linux
readlink that raises PermissionError yields LocalPidNamespace(None, True), is not
checkable, is not cached, and resolves once the read works again (linux_only); no identity
off Linux keeps hostname-only semantics (macos_only and windows_only, one marked test
each). Predicate: the checkable matrix; unsupported platform keeps hostname semantics; own
lookup failed fails closed. Behaviour: the namespace stamped on the row and in the claimed
event; the incident — a dead-looking PID from another namespace is not marked crashed and
its run stays open — against its control, where a dead PID in the same namespace still crashes
(and the release clears the stamp); rollout (a) a migrated live claim with claim_pidns = NULL
is neither probed nor signalled from a sibling and is released by the TTL with
pid_checkable: false; rollout (b) the real older-writer sequence — this binary claims and
releases (the stamp must be gone), an older binary's _claim_and_open_run shape then claims
without touching the column — and the crash sweep and operator reclaim neither probe nor
signal; a Linux reader whose own lookup failed neither probes nor signals a fully stamped claim
and releases it at the TTL; a TTL-expired foreign claim is neither extended nor signalled;
the reclaimed payload describes the claim rather than the kill, with and without a
worker_pid; a live same-namespace worker is still extended; no signal crosses a namespace
from enforce_max_runtime, detect_stale_running, the operator reclaim_task or the
dashboard's _set_status_direct, each paired with a same-namespace control that still sends
SIGTERM (the two dashboard cases also assert the row's claim_pidns is NULL after the
move); the additive migration on a pre-column board. Namespace identity is monkeypatched
as data (_local_pid_namespace, never sys.platform), so the suite is deterministic on any
host; the two linux_only cases read the real /proc.

On the count versus AGENTS.md's "1–2 invariant tests per fix": this fix has more than one
invariant surface — a tri-state identity across three platforms, one predicate matrix, and
five independent signal paths (crash sweep, stale release, max-runtime, operator reclaim,
dashboard) that each had their own inline hostname check and each needed a cross-namespace
negative and a same-namespace positive control, plus the two rollout-boundary sequences. The
26 cases are those contracts written one per row; none asserts SQL text, call order or
internal structure, so none is a change-detector. If the maintainer prefers the bar read
literally, the five per-path negative/positive pairs collapse into two parametrized cases
without losing a contract — I have kept them separate so a failure names the path.

Receipts on the final head (macOS host, scripts/run_tests.sh -j 1, one file per process):
the two files above — 43 passed, 0 failed, 3 skipped (the two linux_only and the
windows_only cases; 23 + 20); the other thirteen suites — 202 passed, 0 failed, 2 skipped
(31/1sk, 23/1sk, 2, 4, 5, 22, 2, 32, 9, 38, 2, 27, 5 in the order listed). At commit 1 alone,
the eleven pre-existing suites the PR touches — 187 passed, 0 failed, 2 skipped, with no test
change (local receipt only; see "Hosted receipts" above).

Checklist

Code

Documentation & Housekeeping

  • Docstrings updated (_claimer_id, _claim_and_open_run, _record_reclaim,
    _terminate_reclaimed_worker, _worker_survived_termination, detect_crashed_workers,
    _reclaim_dead_workers, enforce_max_runtime, detect_stale_running,
    invalidate_descendants_for_parent_reopen, migrate_schema, the kanban_db_pidns module)
  • User docs updated: kanban.md, kanban-worker-lanes.md, docker.md (see Changes Made)
  • cli-config.yaml.example — N/A (no config keys, and no new HERMES_* env var)
  • CONTRIBUTING.md / AGENTS.md — N/A
  • Cross-platform: LocalPidNamespace.supported is False off Linux, where behaviour is unchanged
  • Tool descriptions/schemas — N/A
  • Schema change is additive and nullable; old boards migrate in place via
    _LATER_TASK_COLUMNS (and export snapshots via migrate_schema); no Task field or
    _TASK_OPTIONAL_COLUMNS entry is needed

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed exact head d94bcb014c0fe0245843b1ab28387f791b5c5550 against base/current main 245e48008fa814b3251f50755eb656bd9fb86cb1: one commit, merge base exactly current main, behind by 0. I traced claim creation → additive migration → stale/crash/max-runtime/reclaim/parent-reopen → dashboard direct status transitions, read the namespace matrix and migration regressions, checked the related work/review history, and verified exact-head CI, Docker, and Nix are green (34019908798, 34019908125, 34019908173; 37 check runs and no failure/action_required/cancelled conclusion surfaced).

The core direction is right: hostname is not PID authority, and carrying namespace identity into every mutating PID probe/signal is the correct shape. There are two blockers before this is safe to land.

1. claim_pidns IS NULL still re-enables the exact cross-namespace destructive path during rollout

_claim_pid_checkable() returns True immediately for a NULL stored namespace. The additive migration does not backfill active claims, and an older CLI sharing the DB can continue authoring NULL after a newer process has migrated the schema. On Linux the reader does know its own PID namespace, but a same-hostname sibling then gets license to call _pid_alive() / _terminate_reclaimed_worker() on an unqualified PID.

The new regression actually pins this behavior: test_legacy_claim_with_null_pidns_keeps_hostname_semantics switches the local namespace, makes _pid_alive() return false, and still expects detect_crashed_workers() to reclaim the legacy NULL claim. That is the same failure class this PR is meant to eliminate, moved to the mixed-version boundary. The mirror hazard remains too: a reused PID number can still authorize a signal to the wrong local process.

Required: fail closed when the local PID namespace is knowable but the claim namespace is unknown. Preserve hostname-only fallback only when the local platform cannot identify namespaces (macOS/Windows). In other words: host mismatch → not checkable; local namespace unavailable → preserve current hostname semantics; local namespace known + stored namespace missing → not checkable; both known → equality. Add rollout regressions for (a) a migrated live NULL claim and (b) an older-writer NULL claim showing they are neither PID-probed nor signalled and instead reach the TTL-safe release path. If faster recovery of unknown legacy claims is wanted, it needs another trusted liveness proof; unknown namespace identity cannot certify PID ownership.

2. This adds new behavior to an already-over-threshold god file

Current root AGENTS.md is explicit: a file past ~2,000 lines is the signal to split along <stem>_<topic> FIRST, in its own commit, and new behavior belongs in a new/topical sibling rather than being appended to the facade. This diff adds the namespace resolver/cache/predicate plus claim/reclaim behavior directly to hermes_cli/kanban_db.py; the same diff has hunks beyond line 3,500. kanban_db_dispatch.py already exists as an extracted sibling, so the repository has already established this decomposition boundary.

Required: move the PID-namespace / claim-provenance owner into an appropriate topical sibling in a behavior-neutral first commit, preserving the existing late-bound/monkeypatch contract, then apply the behavior on top in a second commit. Both surviving commits need their own green receipts; the current single commit is green, but landing new commits changes that proof object.

Interlocks / credit

  • #50118 by GoliathInteractive is real prior art on the same false-PID premise. Its heartbeat idea remains complementary inside an already-qualified namespace, but it cannot fence a cross-namespace signal. Please retain that contributor credit rather than treating the earlier work as wrong or irrelevant.
  • #59799 by chrismaddern is the earlier container-replacement / foreign-claim report. Its faster foreign-claim recovery proposal is complementary to this safe qualification work, not a duplicate.
  • #104119 is the dry-run half of the same production incident, not a substitute. The two fixes have no semantic prerequisite and can land in either order, though both touch dispatch and may require a textual rebase. Until both land, one side stays open: #104119 alone leaves real dispatch/dashboard/operator PID authority unsafe; #104125 alone leaves --dry-run mutating durable board state.
  • #94916 remains the broader dry-run lifecycle-honesty issue; this PR should not claim to close that class.

The deliberately deferred GET /runs/{run_id}/inspect psutil probe is the observation-side mirror of this defect: from a sibling dashboard namespace it can still report a live worker as absent. I do not think that read-only diagnostic mismatch blocks this mutation-safety repair once the two blockers above are closed, but it should stay visible as follow-up so the UI does not keep presenting an authority claim the mutators correctly refuse to trust.

Outside those blockers, the test shape is strong: same-namespace positive controls, cross-namespace negative controls, operator/dashboard signalling, TTL release, event provenance, and migration are all covered, and the exact current commit train is green. This is careful work on a nasty production class; closing the rollout authority gap and respecting the decomposition boundary would make the safety model substantially cleaner. 🚀

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cron Cron scheduler and job management comp/plugins Plugin system and bundled plugins labels Sep 6, 2026
@rodrigogs
rodrigogs force-pushed the fix/kanban-pid-check-same-namespace branch from d94bcb0 to b0bd32f Compare September 6, 2026 19:39
@rodrigogs

Copy link
Copy Markdown
Author

@andrexibiza — both blockers closed. The head was rewritten from one commit into the two-commit shape you asked for (behavior-neutral extraction first, behavior second), so this reply cites symbols, not SHAs.

Blocker 2 — the owner moved out of kanban_db.py (commit 1, behavior-neutral). The PID-namespace identity now lives in a leaf sibling hermes_cli/kanban_db_pidns.py — _parse_pid_namespace_link, _pid_namespace_id, _claim_pid_checkable and the _PID_NS_ID / _PID_NS_RESOLVED cache — along the same <stem>_<topic> boundary that already produced kanban_db_dispatch.py. kanban_db.py keeps the schema column and the stamping in _claim_and_open_run; both facades call through the module (kanban_db._kbpidns._claim_pid_checkable(...), kanban_db_dispatch._kbp._claim_pid_checkable(...)), so there is exactly one patch point: kanban_db_pidns._*. The late-bound/monkeypatch contract for the reclaim entry points is untouched: kanban_db._terminate_reclaimed_worker, kanban_db._pid_alive, kanban_db_dispatch._kill_fn, kanban_db._record_reclaim keep their signatures and patch surfaces. kanban_db_pidns is a leaf — its only dependency on the facade (_host_prefix for the non-hoisted default) is a lazy import inside _claim_pid_checkable, so kanban_db stays the single owner of claim-lock host identity and existing hostname patches keep working.

Blocker 1 — a NULL stored namespace now fails closed (commit 2). The _claim_pid_checkable matrix is now: host mismatch → not checkable; local namespace unavailable (macOS/Windows, no /proc) → hostname-only fallback preserved; local known + claim_pidns IS NULL → not checkable; both known → equality. The NULL case is the mixed-version boundary (pre-upgrade rows plus older writers that never stamp), and hostname is not authority there: a sibling container sharing our hostname is indistinguishable from an older same-host writer, so reclaiming on hostname alone would re-enable the exact cross-namespace destructive path, moved to the rollout boundary.

Rollout regressions, replacing test_legacy_claim_with_null_pidns_keeps_hostname_semantics (which pinned the old behavior you flagged):

  • test_migrated_null_pidns_claim_is_not_crashed_and_released_by_ttl — a pre-upgrade row read from a sibling container: detect_crashed_workers neither probes (the _pid_alive recorder never fires) nor signals, the row stays running, and release_stale_claims reclaims it only after claim_expires lapses — TTL-safe release, with pid_checkable: false in the reclaimed payload.
  • test_older_writer_null_pidns_claim_is_never_signalled — an older writer's NULL claim read from the same host and same namespace: still neither probed nor signalled by the crash sweep, and reclaim_task releases the claim without a SIGTERM.

The one pre-existing suite test that simulated a claim without a namespace (test_reclaim_task_resets_running_to_ready in test_kanban_core_functionality.py) is adapted: its raw-SQL claim now stamps claim_pidns the way _claim_and_open_run does, so the operator-reclaim signal path it exists to pin is still exercised — the un-stamped variant is precisely the older-writer case the new regressions cover.

Receipts, same venv as the reviewed head: commit 1 → test_kanban_claim_pid_namespace.py + test_kanban_parent_reopen_invalidation.py = 23 passed, 1 skipped (extraction is green alone); after commit 2 → pid-namespace + parent-reopen + core-functionality + reclaim-guard + db + worker-image = 85 passed, 2 skipped; schema/repair/transfer/boards = 52 passed. Interlocks noted: #50118's heartbeat idea stays complementary inside a qualified namespace (credited), #59799 and #104119 remain open siblings, and no claim is made on closing #94916's class.

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review of exact head b0bd32f03bdeb26e443dd0af4c360ac66c06dbd9, after the response to my review of d94bcb014c0fe0245843b1ab28387f791b5c5550. I re-read the rewritten two-commit train, the full current diff, rollout tests, every live _terminate_reclaimed_worker caller I could find, #50118/#59799/#104119, and the current execution receipts.

The rewrite materially improves both original concerns: claim_pidns IS NULL now fails closed when the reader has a namespace identity, the claim-provenance helper has a bounded kanban_db_pidns.py owner, operator/dashboard/TTL paths carry the namespace, and the current head is green in CI/Docker/Nix (34055595063, 34055594750, 34055594762). The rollout regressions are particularly useful. I still see two landing blockers.

1. Linux namespace-resolution failure still fails open to hostname-only PID authority

kanban_db_pidns._pid_namespace_id() returns None not only on platforms without /proc, but also on Linux when os.readlink("/proc/self/ns/pid") raises any OSError. _claim_pid_checkable() then treats every local_ns is None as the safe compatibility case and returns True for a matching hostname.

That collapses two different states:

  • platform has no PID-namespace identity mechanism → compatibility fallback;
  • Linux should have an identity, but this process failed to obtain it → unknown authority.

The second state is exactly where this class must fail closed. A Linux container with /proc unmounted/restricted/transiently unreadable can share a hostname with another PID namespace; under this head, that lookup failure restores the old permission to probe, extend, or signal the sibling's PID number. The new tests mask the distinction by monkeypatching _pid_namespace_id() directly to None and calling that “macOS/Windows”.

Required: preserve the reason for None (e.g. unsupported | known(ns) | resolution_failed) or make the predicate platform-aware: unsupported non-Linux may retain hostname compatibility, but Linux namespace lookup failure must make the PID uncheckable. Add a deterministic regression where the Linux namespace lookup itself fails and prove no _pid_alive probe and no SIGTERM/SIGKILL is authorized. This is the other side of the same proof boundary: absence of provenance cannot become provenance merely because the reader failed to resolve it.

2. The surviving two-commit train still does not satisfy the decomposition / every-commit-green gate

Commit 1, 16b5c21e2a78b4e5243111cf2663d978f48f7ff4, is titled and described as a behavior-neutral extraction, but its own diff already adds claim_pidns to the schema, stamps it during _claim_and_open_run, rewires stale/crash/max-runtime/reclaim/dashboard paths, and blocks a same-host/different-PID-namespace claim whenever both IDs are known. That is the core semantic repair, not a neutral move of an existing owner.

More importantly, that surviving commit has no hosted workflow runs at all. The current head is green, but the train is therefore not 2/2 green. A local targeted receipt for commit 1 is useful development evidence, not an exact-object hosted acceptance receipt.

Required: make the first surviving commit genuinely behavior-neutral (for example, extract the existing hostname-only claim/PID qualification into the bounded owner without changing semantics), then layer namespace provenance/fail-closed behavior in the next commit; and obtain green hosted CI/Docker/Nix for every surviving commit. Rebase first: the merge base is still 245e48008fa814b3251f50755eb656bd9fb86cb1, while current main is 6b2d4faf36f107c364048e2fe17eaa02154e9ea9 — 218 main commits beyond that merge base — so any new receipts must belong to the rebased exact objects, not this stale train.

One contract cleanup belongs with that rewrite: the PR body still says legacy claim_pidns IS NULL keeps hostname-only semantics and still locates _claim_pid_checkable in kanban_db.py; kanban_db_connect.py likewise comments that legacy NULL keeps hostname trust. Those statements now contradict the intended head behavior and should be updated before landing.

Interlocks / provenance

  • #50118 by GoliathInteractive remains genuine prior art on the same false-PID premise. Heartbeat freshness is complementary after PID authority is qualified; it is not a substitute for the namespace fence.
  • #59799 by chrismaddern remains complementary recovery work for unverifiable/foreign claims; this PR deliberately routes mismatches onto the slower safe path that work could later accelerate.
  • #104119 is the read-only dry-run sibling from the same incident, not a duplicate or prerequisite. Either can land first semantically; both must land for the incident's two independent destructive surfaces to be closed.
  • The read-only dashboard GET /runs/{run_id}/inspect PID probe remains the observation-side mirror. I still do not consider that a blocker to this mutation-safety repair, but it should remain explicit follow-up rather than being mistaken for authoritative liveness.

I did not find a remaining live _terminate_reclaimed_worker call site in the current tree that this patch leaves at hostname-only signaling; the namespace propagation across reclaim, stale-running, parent-reopen, and dashboard direct status transitions is coherent. The claim event provenance, transfer scrub, same-namespace positive controls, mixed-version NULL regressions, and post-commit termination coverage are all strong work. Close the Linux-resolution fail-open edge and make the rewritten train actually neutral-first + every-commit green, and this becomes a much cleaner authority boundary. 🚀

@rodrigogs
rodrigogs force-pushed the fix/kanban-pid-check-same-namespace branch from b0bd32f to bbdecab Compare September 7, 2026 00:37
@rodrigogs

Copy link
Copy Markdown
Author

Thanks for both passes. The train is rewritten and rebased onto current main; here is how each point closed.

Linux namespace-resolution failure (19:49Z blocker 1). kanban_db_pidns now resolves the local identity as a tri-state, LocalPidNamespace(id, supported): supported=False is "this platform has no /proc/self/ns/pid" (macOS, Windows), supported=True with an id is Linux resolved, and supported=True with id=None is Linux where os.readlink raised. _claim_pid_checkable keeps hostname-only trust only for the first state; in the third it returns False for every host-local claim, stamped or not, so a reader that could not resolve itself neither probes nor signals anything and the claims go to the TTL path. A failed lookup is also not cached, so a transient /proc problem is retried on the next call rather than freezing recovery for the process lifetime. Regressions: test_linux_namespace_lookup_failure_is_not_the_unsupported_platform (linux_only, monkeypatches os.readlink to raise PermissionError, asserts the tri-state, the False, and the un-cached recovery) and test_failed_local_namespace_lookup_neither_probes_nor_signals (fully stamped claim, reader with id=None: detect_crashed_workers makes no _pid_alive call and no signal; release_stale_claims releases at the TTL with pid_checkable: false). The bare skipif is gone: the "no identity off Linux" contract is one macos_only and one windows_only test, and the fixture patches _local_pid_namespace as data rather than calling id=None "macOS".

Neutral-first train, every commit green, rebase (19:49Z blocker 2). Commit 1 is now refactor(kanban): move the claim PID-probe gate into kanban_db_pidns: it creates the sibling with a _claim_pid_checkable(lock, host_prefix=) that is exactly the startswith(host_prefix) test the four passes made inline and routes them through it — 3 files, +49/-6, no schema, payload, config or test change, kanban_db._host_prefix still read at call time. The eleven pre-existing suites this PR touches pass unchanged at that commit (187 passed, 0 failed, 2 skipped via scripts/run_tests.sh). Commit 2, fix(kanban): trust a worker PID only inside the claim's PID namespace, carries the column and migration, stamping, the tri-state identity, the fail-closed predicate and the tests. Merge base is current main; hosted receipts will attach to these objects once pushed.

Contract cleanup. The body now locates the predicate in kanban_db_pidns and states the fail-closed matrix instead of "NULL keeps hostname-only semantics"; the _LATER_TASK_COLUMNS comment in kanban_db_connect says NULL fails closed where the reader can identify its namespace; and _terminate_reclaimed_worker makes claim_pidns a required keyword with a docstring that says None means no signal on Linux, not hostname-only behaviour, so no future caller can fall back by omission.

Two related gaps surfaced while re-splitting and are fixed in commit 2. claim_pidns was never cleared on release, so an older binary claiming a row this binary had claimed and released would inherit the previous namespace and its PID would read as checkable — the rollout-boundary case again; every release UPDATE that nulls claim_lock now nulls claim_pidns, and rollout regression (b) reproduces the real sequence (new claim, release, old-shape claim that never touches the column) instead of hand-nulling the column. And hermes kanban boards export scrubbed a raw snapshot that never went through connect, so the first post-upgrade backup of an untouched board failed on the new column; export_board now runs kanban_db_connect.migrate_schema on the snapshot first, with a transfer test. User docs (kanban event table, single-host note, worker-lanes failure modes, docker dashboard note) now carry the namespace rule and the pid: "service:gateway" remedy.

Credit and interlocks are kept as you framed them: #50118 remains prior art whose heartbeat idea is complementary inside a qualified namespace, #59799 remains complementary recovery work, #104119 is the independent dry-run half of the incident, and the read-only GET /runs/{run_id}/inspect probe stays explicit follow-up.

@rodrigogs rodrigogs changed the title fix(kanban): trust a worker PID check only inside the claimer's PID namespace fix(kanban): trust a worker PID only inside the claim's PID namespace Sep 7, 2026
@rodrigogs
rodrigogs force-pushed the fix/kanban-pid-check-same-namespace branch from bbdecab to 215df1c Compare September 7, 2026 19:51
@rodrigogs

Copy link
Copy Markdown
Author

Head amended (fix commit only; the extraction commit is byte-identical): the dashboard's _set_status_direct was the one release path that still kept claim_pidns, so a card dragged off Running now clears it like every other release, with two regression assertions in the same tests. Two corrections to my earlier reply, now stated in the body: (1) hosted CI attaches only to the head, so the extraction commit has a local scripts/run_tests.sh receipt and no check-runs of its own — say if you want it pushed as a separate head or the train squashed; (2) the merge base is unchanged since the last push; main moved on since, none of the moved commits touch these files, and I will rebase before merge if you prefer a current base.

@rodrigogs

Copy link
Copy Markdown
Author

Rebased onto upstream/main (5dea46d13d); this PR had gone CONFLICTING.

What the force-push overwrites: the previous remote head 215df1c72 is replaced by 201ad4b4d (2 commits, 14 files). Review-comment line anchors on this PR move with it — citing symbols below instead of SHAs so this survives the next rebase too.

One conflicted region, in website/docs/user-guide/features/kanban.md at the end of Out of scope. Two things landed in the same hunk:

  1. Upstream rewrote the same paragraph this PR extends (the crash-detection path assumes PIDs are host-local). Resolution: kept this PR's text, which states the rule the code implements — a worker PID is trusted only on this host and in this PID namespace (claim_pidns, stamped on every claim) — plus the added paragraph about one Docker host (network_mode: "service:gateway" shares a hostname but not a PID namespace; unverifiable claims are released on claim_expires with pid_checkable: false).
  2. The other half of the hunk was upstream deleting the stale ## Design spec section (part of 0c0875b746, which removed the orphaned spec PDF). That deletion is upstream's call and unrelated to this PR, so the resolution keeps upstream's deletion — the section stays gone rather than being resurrected by the conflict.

Nothing else moved; the auto-merged surfaces carry through as-is: hermes_cli/kanban_db_pidns.py, hermes_cli/kanban_db.py, hermes_cli/kanban_db_dispatch.py, hermes_cli/kanban_db_connect.py, hermes_cli/kanban_swarm.py, hermes_cli/kanban_transfer.py, plugins/kanban/dashboard/plugin_api.py.

Verified on the rebased head (scripts/run_tests.sh): tests/hermes_cli/test_kanban_claim_pid_namespace.py + test_kanban_core_functionality.py + test_kanban_transfer.py + test_kanban_parent_reopen_invalidation.py → 72 passed, 2 skipped, 0 failed.

GitHub now reports this PR mergeable: true. No new ask.

@rodrigogs
rodrigogs force-pushed the fix/kanban-pid-check-same-namespace branch from 201ad4b to e36e41a Compare September 14, 2026 05:50
@rodrigogs

Copy link
Copy Markdown
Author

Rebased onto upstream/main — the conflict was in request_review, and it was resolved by keeping upstream's rewrite plus this PR's column release.

What the force-push overwrites: remote head 201ad4b4d4 → e36e41afc6, still 2 commits.

Upstream rewrote request_review since this branch was cut (nested try: / with write_txn(...), plus _merge_completion_prose_artifacts, staged_copies, and the implementer / _prior_reviewer provenance block), so the commit's claim_pidns = NULL had no anchor line left to attach to. The resolution takes upstream's function verbatim and re-adds exactly that column to its UPDATE tasks ... SET status = 'review' statement. No line of upstream's block was dropped, and no other site of this PR was rewritten.

Verified that this is not a silent content loss, because the obvious wrong resolution (git checkout --ours on the whole file) quietly discards 23 of the PR's 24 column writes and still completes the rebase:

  • git range-diff reports commit 2 with one hunk difference, and it is the indentation shift caused by upstream's now-deeper nesting.
  • 11 of 13 files are byte-identical to the pre-rebase branch. The two that differ (kanban_db_dispatch.py, features/kanban.md) auto-merged with upstream's newer edits, and every pre-rebase line carrying this PR's change survives (0 missing).
  • claim_pidns sites: 24/24 in kanban_db.py (upstream has none), and the counts in kanban_db_pidns, kanban_db_dispatch, plugin_api.py, kanban_transfer, kanban_swarm are unchanged.
  • git show --stat of the rebased commit: 14 files, 985 insertions / 97 deletions — identical to the pre-rebase commit's stat.

Suites on the rebased head, real imports against the tree:

  • tests/hermes_cli/test_kanban_claim_pid_namespace.py → 24 passed, 2 skipped
  • test_kanban_core_functionality.py + test_kanban_parent_reopen_invalidation.py + test_kanban_transfer.py → 48 passed

GitHub now reports this PR mergeable: true (it was CONFLICTING / DIRTY). The standing re-gate request is unchanged — no new ask.

Every reclaim pass that reads worker_pid as evidence (crash sweep, stale
claim release, max-runtime kill, operator/dashboard reclaim) repeated the
same inline startswith(host_prefix) test before probing or signalling. It
now lives in one predicate, kanban_db_pidns._claim_pid_checkable, a leaf
sibling on the same <stem>_<topic> boundary that produced
kanban_db_dispatch.py; kanban_db.py is past the ~2k-line bounded-owner
threshold, so claim provenance gets its own owner before it grows.

Behaviour-neutral: the predicate is exactly the hostname-prefix check it
replaces, there is no schema, payload or config change, and
kanban_db._host_prefix is still read at call time so it stays the patch
point. release_stale_claims keeps computing host_local for the reclaimed
payload; only its probe gate goes through the predicate.
A claim lock is <hostname>:<pid>, and every reclaim pass took a matching
hostname as licence to read "no such PID here" as "worker dead". Hostname
and PID namespace are independent in containers: Compose services joined
with network_mode: "service:x" share one hostname, and without pid: each
keeps its own PID namespace, where the same number is an unrelated
process. A dispatch from the dashboard container found four running claims
with its own hostname, saw none of the gateway's worker PIDs, and closed
all four live runs as crashed; the mirror error (a PID number that exists
locally) would extend a dead foreign claim forever or SIGTERM a bystander.

tasks.claim_pidns (additive, nullable; migrated via _LATER_TASK_COLUMNS)
records the namespace a claim was made in, stamped by _claim_and_open_run
in the same CAS update as the lock and cleared wherever claim_lock is, so
a claim written later by an older binary starts from NULL instead of
inheriting the previous claimer's namespace. kanban_db_pidns owns the
identity (LocalPidNamespace: unsupported / known / lookup failed) and
_claim_pid_checkable, the predicate every PID probe and every signal now
goes through: host prefix must match; no namespace identity on this
platform (macOS/Windows) keeps hostname-only trust; a failed lookup of our
own namespace on Linux, or a claim with no recorded namespace, fails
closed; otherwise the ids must be equal. A claim that is not checkable is
neither probed, extended nor signalled and is released by the claim TTL;
the reclaimed payload reports pid_checkable beside host_local (caller's
values win the merge in _record_reclaim). _terminate_reclaimed_worker now
requires claim_pidns so no caller falls back to hostname-only signalling.

Board export runs migrate_schema on its snapshot before scrubbing, so the
first post-upgrade backup of a board this release has not opened no longer
fails on the new column. Docs: kanban event table (claimed.pidns,
reclaimed.pid_checkable), the single-host note, worker-lanes failure
modes, and the docker dashboard note gain the namespace rule and the
pid: "service:gateway" remedy.
@rodrigogs
rodrigogs force-pushed the fix/kanban-pid-check-same-namespace branch from e36e41a to f6c58e9 Compare September 30, 2026 07:22

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact-head re-review: three remaining P1 findings

Reviewed f6c58e9931d4712c7d75fe8bcf8399f69732072f, including the full diff, the two surviving commits, the earlier review/author discussion, active-claim transitions, archive, and the closed-run terminal-worker reaper. Rechecked the contributor branch immediately before submission: it still points to this SHA. This head is not ready to land. The findings below are also attached inline.

Reconciliation of the earlier reviews

The earlier NULL-namespace and Linux-resolution-failure concerns are addressed in the current predicate: an unsupported platform is distinct from a supported platform whose lookup failed, and Linux unknown provenance returns false. The bounded kanban_db_pidns.py owner is present. I also inspected the actual extraction commit 7d6b99d: its three-file +49/−6 diff really does extract the hostname predicate before introducing namespace semantics. Those code-shape concerns should not be repeated as unresolved. The author’s tri-state and release-path response represents real progress.

What remains is lifecycle integration and the other direction of mixed-version writing—not another request to redo that extraction.

P1 — Running-task archive commits its state, then raises before termination and cleanup

The helper now requires claim_pidns, but archive_task neither selects the namespace into its snapshot nor passes it to the post-commit call. Every running-task archive therefore reaches TypeError: _terminate_reclaimed_worker() missing 1 required keyword-only argument: 'claim_pidns'.

This is a partial-success failure, not a harmless rejected operation: the transaction has already archived the task, closed the run, and cleared the task’s PID/claim fields. No termination occurs; archive_worker_termination, recompute_ready, and _cleanup_workspace are skipped. Retrying archive returns false because the task is already archived. The normal caller has no successful retry path for the skipped work.

Snapshot claim_pidns with the original lock/PID/fingerprint inside the winning archive transaction, and forward that original value after commit. Test the real archive entry point with the real termination helper for same-namespace and different/unknown-namespace cases; also verify that cleanup and the termination audit event complete. Passing an unconditional None would conceal the TypeError while disabling legitimate Linux cleanup.

P1 — Terminal-worker reaping is broken and has no run-owned namespace provenance

The second missed caller is _reap_terminal_worker_row. For an eligible live local worker, it invokes the newly required-argument helper without claim_pidns. reap_terminal_workers catches that exception per row, logs it at debug, and returns no reaped worker. This silently disables cleanup of the live post-completion processes that this sweep exists to terminate.

There is a second branch of the same integration gap: the reaper still gates only on hostname, probes the PID, and clears the closed run’s retained PID/fingerprint when the local visibility check says false. A sibling PID namespace can therefore erase the owner’s cleanup evidence without proving that the retained worker is dead. This is a durable mutation, unlike the explicitly deferred read-only dashboard inspect endpoint.

The namespace currently lives only on tasks and is cleared on terminal transitions. A closed task_runs row retains its worker identity but not its namespace. Consequently, just adding the keyword at this call cannot safely finish the integration: the current task may already have no claim or may belong to a later run.

Retain namespace provenance with the run-owned worker identity, qualify the reaper’s probes and evidence-clearing branch, and pass that retained namespace to termination. Unknown/different-namespace observations must not discard the owner’s retained worker evidence. Cover both a live same-namespace terminal worker and a same-hostname foreign-namespace visibility miss.

P1 — An older release can leave a stale namespace stamp that an older claim inherits

The new claim-stamping contract assumes a later old writer starts from NULL because releases clear the stamp. That is true only when the release also runs new code. A pre-column client’s release SQL clears claim_lock, expiry and PID but cannot clear claim_pidns; the subsequent old-shape claim does not write that column either.

Therefore a task previously stamped by namespace A can be released by an older client and then claimed by an older client in namespace B while still carrying A’s stamp. A new reader in A accepts that inherited value in the equality predicate. The unknown old-writer claim has acquired false provenance; the crash sweep can again treat a local visibility miss as proof that the foreign worker died. The fresh-NULL fail-closed branch never runs.

The existing test_older_writer_null_pidns_claim_is_never_signalled in the new namespace suite first calls new kb.reclaim_task, so it excludes precisely this case.

Make invalidation of claim provenance hold at a boundary older writers cannot skip—for example, database-enforced clearing when a claim is released—or use an equivalently verifiable association with the exact claim generation. Add the old-release → old-claim regression, not just new-release → old-claim. It must yield uncheckable provenance rather than inheriting the previous claimant’s authority.

Executed checks and limits of the evidence

I executed isolated copies of the relevant current-head function bodies with temporary SQLite and stubbed process operations. These are source-excerpt checks, not a full repository pytest run or a real-container integration receipt; no real process was inspected or signalled. Observed results:

running archive:
  TypeError: missing required keyword-only argument 'claim_pidns'
  persisted status=archived; task worker_pid=NULL
  termination helper never entered; cleanup never called; retry=False

terminal reaper, live local worker:
  same TypeError swallowed by per-row handler
  reaped=[]; retained worker_pid unchanged; termination helper never entered

terminal reaper, local PID visibility miss:
  reaped=[]; retained run worker_pid and worker_started_at both cleared

older-client release followed by older-client claim:
  previous namespace stamp survives
  current predicate returns checkable=True for the inherited stamp
  fresh-NULL control returns False

The terminal reaper’s provenance omission is a remaining class-closure gap; its newly broken helper call and the archive exception are direct regressions from this signature change. Please run repository-level coverage for these real entry points rather than mocks that replace the helper with a permissive signature.

Current CI is not the earlier green receipt

For this exact head, CI 36683521696, Docker 36683521061, and Nix 36683520841 all report action_required, not success. The exact extraction commit’s check-runs endpoint currently reports total_count: 0. This is missing acceptance evidence, not a claim that an executed CI test failed. Earlier-head green runs and the author’s local receipts do not certify these objects.

Keep the existing credit to #50118 and the recovery/dry-run interlocks intact. The needed repair is to carry trustworthy provenance through the complete worker lifecycle and preserve it across mixed-version transitions, while keeping the already-correct active-claim fence.

Comment thread hermes_cli/kanban_db.py
cur = conn.execute(
"UPDATE tasks SET status = 'archived', "
" claim_lock = NULL, claim_expires = NULL, worker_pid = NULL, worker_started_at = NULL "
" claim_lock = NULL, claim_expires = NULL, worker_pid = NULL, worker_started_at = NULL, claim_pidns = NULL "

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Preserve the archive snapshot’s namespace and pass it to termination

This UPDATE clears the stamp, but archive_task still SELECTs only status/lock/PID/fingerprint and calls _terminate_reclaimed_worker(prev_pid, prev_lock, signal_fn=..., started_at=...) after commit. The new required claim_pidns keyword makes every running-task archive raise after status=archived, run closure, and claim/PID clearing have committed. The worker is not terminated, the termination event and downstream cleanup are skipped, and a retry returns false because the card is already archived.

Exact caller: archive_task. Capture claim_pidns with the original row inside the winning transaction and pass that saved value post-commit; do not use the cleared row or an unconditional None. An isolated execution of this function body with temporary SQLite reproduced the post-commit TypeError, zero termination/cleanup calls, and false retry. Add real-helper archive coverage for same-namespace and uncheckable claims.

pid: Optional[int],
claim_lock: Optional[str],
*,
claim_pidns: Optional[str],

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Integrate the closed-run terminal reaper with this required namespace

_reap_terminal_worker_row is another live caller that does not pass this keyword: caller and outer exception handler. Eligible live local workers now raise TypeError here; reap_terminal_workers swallows it per row and silently performs no cleanup. The source-excerpt/SQLite check reproduced reaped=[], the swallowed missing-keyword error, and unchanged retained PID.

The repair also needs run-owned namespace provenance: this sweep reads closed task_runs, while this PR stores the namespace only on tasks and clears it at release. The reaper still probes by hostname and clears the retained run PID/fingerprint on a local visibility miss, including a sibling-namespace miss. Merely passing None disables valid Linux cleanup; looking up the current task’s stamp can borrow a later run’s identity. Retain the namespace alongside the run worker identity, gate both probing and evidence-clearing, and pass it here. Test a live same-namespace terminal worker and a foreign-namespace miss that must preserve its cleanup evidence.

Comment thread hermes_cli/kanban_db.py
Also stamps ``claim_pidns`` (``kanban_db_pidns._pid_namespace_id``) beside the lock:
only a process in that namespace can read ``worker_pid`` as evidence. It is
claim state and is cleared wherever ``claim_lock`` is, so a claim written
later by a binary that does not know the column starts from NULL instead of

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Clear provenance when an older client releases the claim too

The guarantee in this comment currently depends on the release executing new code. A pre-column client clears claim_lock/expiry/PID without mentioning claim_pidns; a subsequent older claim also leaves that column untouched. The new reader can therefore see a claim authored in namespace B still stamped with namespace A from the previous claim, and _claim_pid_checkable in A returns true rather than taking the unknown-provenance branch. That reopens the false-crash path during the mixed-version rollout this PR explicitly supports.

The new older-writer test covers only new release → old claim because it calls current kb.reclaim_task first. The isolated SQLite check with old release → old claim retained A’s stamp and the current predicate accepted it; the fresh-NULL control correctly rejected it. Enforce stamp invalidation at the database/claim-generation boundary so an older release cannot omit it, and add this missing transition regression. More new-code release UPDATEs alone cannot establish that invariant.

@rodrigogs

Copy link
Copy Markdown
Author

I have prepared the three lifecycle fixes for re-review. Before force-pushing: I will replace this PR's current remote two-commit PID-namespace train only, preserving its history and adding the final cleanup commit; no other branch or production checkout is involved.

  • archive_task snapshots and forwards claim_pidns before clearing task state, so termination, the audit event, readiness recomputation, and workspace cleanup all complete.
  • _claim_and_open_run retains claim namespace provenance on task_runs; _reap_terminal_worker_row now qualifies the closed run before any probe, evidence clearing, or signal.
  • clear_claim_pidns_when_lock_released clears stale provenance at the database boundary, including an old-client release followed by an old-client claim.

Verified with scripts/run_tests.sh across the namespace, terminal-worker reaper, archive, core Kanban, parent-invalidation, and transfer suites: 132 passed, 0 failed (3 platform-skipped). The regression coverage exercises same/foreign namespace archive, foreign terminal-run evidence preservation, and the old-release → old-claim path.

@rodrigogs

Copy link
Copy Markdown
Author

Force-push complete; the current head contains the focused lifecycle repair. archive_task snapshots claim_pidns before the state transition; _claim_and_open_run retains it on task_runs; _reap_terminal_worker_row refuses foreign/unknown namespace evidence before probing, clearing, or signalling; and clear_claim_pidns_when_lock_released fences old-release → old-claim inheritance.

tests/hermes_cli/test_kanban_claim_pid_namespace.py, test_kanban_terminal_worker_reaper.py, test_kanban_db.py, test_kanban_core_functionality.py, test_kanban_parent_reopen_invalidation.py, and test_kanban_transfer.py pass together: 132 passed, 0 failed (3 host-platform skips). GitHub currently reports this fork PR mergeable; as usual for this repository's fork PRs, no hosted status checks are attached. Re-review requested.

@andrexibiza andrexibiza left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at 41df251683e90d559efaee0e1ab39bc16962caf6.

The three P1s from the previous f6c58e9 pass are repaired at this head:

  • archive_task now snapshots and forwards claim_pidns before clearing task state.
  • terminal-worker cleanup now retains namespace provenance on task_runs and checks it before probing/clearing/signalling.
  • the DB trigger closes the old-release → old-claim stale-provenance window.

Two P1 authority holes remain. Both are the same class this PR is closing: a PID observation from one namespace is still being allowed to authorize mutation of a claim created in another namespace.

%%{init: {'theme':'dark','themeVariables':{'background':'#0d1117','primaryTextColor':'#e6edf3','lineColor':'#94a3b8'}}}%%
flowchart TB
subgraph CORE[" "]
direction TB
    A["claim created<br/>PID namespace A"]:::normal --> B["tasks.claim_pidns = A"]:::durable
    B --> C{"PID-consuming path"}:::authority
    C --> D["reclaim / crash / timeout<br/>namespace-qualified"]:::good
    C --> E["reconcile_orphaned_running"]:::bad
    C --> F["_claim_is_live<br/>complete / request-review"]:::bad
    D --> G["_claim_pid_checkable"]:::authority
    E --> H["_worker_alive(pid)<br/>without claim_pidns"]:::bad
    F --> H
    H --> I["namespace B cannot see A's PID<br/>false 'dead' / claim mutation"]:::bad

end
CORE ~~~ Legend
Legend["Legend<br/>Blue: entry / incoming<br/>Purple: ownership / policy<br/>Teal: durable state<br/>Green: success<br/>Red: failure"]
style CORE fill:#0d1117,stroke:#30363d,color:#e6edf3
style Legend fill:#1e293b,stroke:#94a3b8,color:#f8fafc,font-size:11px
classDef durable fill:#134e4a,stroke:#2dd4bf,color:#f0fdfa,stroke-width:2px

    click G "https://github.com/NousResearch/hermes-agent/blob/41df251683e90d559efaee0e1ab39bc16962caf6/hermes_cli/kanban_db_pidns.py#L91-L127"
    click E "https://github.com/NousResearch/hermes-agent/blob/41df251683e90d559efaee0e1ab39bc16962caf6/hermes_cli/kanban_db_dispatch.py#L883-L946"
    click F "https://github.com/NousResearch/hermes-agent/blob/41df251683e90d559efaee0e1ab39bc16962caf6/hermes_cli/kanban_db.py#L2779-L2790"

    classDef normal fill:#172554,stroke:#60a5fa,color:#eff6ff,stroke-width:2px
    classDef authority fill:#3b0764,stroke:#c084fc,color:#faf5ff,stroke-width:2px
    classDef good fill:#052e16,stroke:#4ade80,color:#f0fdf4,stroke-width:2px
    classDef bad fill:#450a0a,stroke:#f87171,color:#fef2f2,stroke-width:2px
click A "https://github.com/NousResearch/hermes-agent/blob/41df251683e90d559efaee0e1ab39bc16962caf6/hermes_cli/kanban_db_pidns.py#L83-L88" "Pinned namespace-provenance source from existing review"
click B "https://github.com/NousResearch/hermes-agent/blob/41df251683e90d559efaee0e1ab39bc16962caf6/hermes_cli/kanban_db_pidns.py#L12-L18" "Pinned namespace-provenance source from existing review"
click C "https://github.com/NousResearch/hermes-agent/blob/41df251683e90d559efaee0e1ab39bc16962caf6/hermes_cli/kanban_db_pidns.py#L91-L127" "Pinned namespace-provenance source from existing review"
click D "https://github.com/NousResearch/hermes-agent/blob/41df251683e90d559efaee0e1ab39bc16962caf6/hermes_cli/kanban_db_pidns.py#L91-L127" "Pinned namespace-provenance source from existing review"
click H "https://github.com/NousResearch/hermes-agent/blob/41df251683e90d559efaee0e1ab39bc16962caf6/hermes_cli/kanban_db.py#L2779-L2790" "Pinned namespace-provenance source from existing review"
click I "https://github.com/NousResearch/hermes-agent/pull/104125#pullrequestreview-5416546071" "Recorded foreign-namespace outcome and its evidence limits"
Loading

P1 — orphan reconciliation still treats foreign PID invisibility as death

reconcile_orphaned_running() is in the default dispatch path (reconcile_orphans=True). It selects broken running rows, calls _worker_alive(worker_pid, ...) directly, and if that probe is false requeues the task and closes its run. It never selects or checks claim_pidns.

That reproduces the original authority error for a broken row: gateway namespace A owns a live worker; dashboard namespace B shares the hostname but cannot see A's PID; B's reconciliation probe returns false; B requeues the still-live task and clears its claim. The next spawn can then run beside the original worker.

The repair needs the same namespace qualification before this probe/mutation. If the PID is not checkable from this namespace, automatic reconciliation cannot use its absence as death evidence. Add same-namespace/dead and foreign-namespace/live controls to tests/gateway/test_kanban_reconcile_orphans.py.

P1 — the live-claim fence used by complete/request-review is still namespace-blind

_claim_is_live() still defines liveness as _worker_alive(worker_pid, worker_started_at) and does not receive claim_pidns. Both complete_task() and request_review() use that result to decide whether an unowned caller may clear the running claim.

From a sibling PID namespace, a live worker can therefore look dead and the guard drops. A claimless CLI/orchestrator caller can close the run or move it to review while the real worker continues. This is exactly the #111764 invariant the existing live-claim tests are meant to preserve, now broken at the container boundary this PR introduces.

Make the guard namespace-aware and fail closed: an uncheckable claimed PID must remain protected unless the caller supplies expected_run_id or an explicit force=True override. Extend test_kanban_complete_live_claim_guard.py with foreign-namespace cases for both completion and review handoff.

Reconciliation / acceptance

The latest cleanup commit does close the three prior P1s; I am not carrying those forward.

Hosted evidence is still not green. At this exact head GitHub has CI, Docker, and Nix runs attached, all action_required. The middle commit f6c58e9 has the same three action_required runs, while the extraction commit 7d6b99d has no hosted runs at all. That does not satisfy the per-commit green gate.

The branch is also currently 3 commits ahead / 3484 commits behind main from merge base 9a0a1625; kanban_db.py and kanban_db_dispatch.py have both changed on main since that merge base. Final acceptance needs validation on a current base, not just the Sept. 28 snapshot.

History note: the Oct. 4 update from f6c58e9 to 41df251 is a one-commit fast-forward (ahead 1, behind 0). GitHub records no Oct. 4 force-push event, so that update did not rewrite the previously reviewed commits even though the comments called it a force-push.

cur = conn.execute(
"UPDATE tasks SET status = 'ready', claim_lock = NULL, "
"claim_expires = NULL, worker_pid = NULL, worker_started_at = NULL, "
"claim_expires = NULL, worker_pid = NULL, worker_started_at = NULL, claim_pidns = NULL, "

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — namespace-blind orphan reconciliation. This clear is reached after line 903 calls _worker_alive() without qualifying the row's PID namespace. A live gateway worker in namespace A can be invisible to a dashboard dispatcher in namespace B; this path then treats that invisibility as death, requeues the task, closes the run, and clears the provenance here. dispatch_once(..., reconcile_orphans=True) enables this pass by default. Select/check claim_pidns before the PID probe; if the PID is not checkable from this namespace, its absence cannot authorize automatic requeue. Add foreign-namespace/live and same-namespace/dead controls.

Comment thread hermes_cli/kanban_db.py
claim_lock = NULL,
claim_expires= NULL,
worker_pid = NULL,
claim_pidns = NULL,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — complete_task still has a namespace-blind live-claim fence. This new clear is protected only by _claim_is_live() above, but that helper does not read claim_pidns; it calls _worker_alive(worker_pid, ...) directly. From a sibling PID namespace a live worker can look dead, so an unowned caller can pass the guard and close the live run here. request_review() shares the same helper and the same failure. Make _claim_is_live namespace-aware and fail closed for uncheckable claimed PIDs unless ownership (expected_run_id) or explicit force=True is present; extend the existing live-claim guard tests across both surfaces.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants