fix(harness): an author child can never author, capture or loop (v0.76.2) - #275
felix-xtrace wants to merge 7 commits into
Conversation
…6.2) The loop (v0.74.0–v0.76.0): the author child was handed MEMHUB_HARNESS_EXTRACT=0, and Claude Code applied the install's settings.json `env` — where opting in IS MEMHUB_HARNESS_EXTRACT=1 — over it. So the child was sensed like a person's session; its one turn is the create-rule flow, exactly what the classifier flags; and its Stop drained that moment by forking the child. One generation per generation. 7113985 (on main, in no release) refuses on MEMHUB_HARNESS_CHILD — one more environment variable, and an environment variable is what failed. Guards that are not the environment, each tested with the environment saying the opposite: - the spawner chooses the child's session id (--session-id) and writes it to harness/children.jsonl BEFORE the child runs; a listed session's Stop is never sensed, it can never spawn an author, and a moment it stamped is never drained (clears debris from installs that had no list); - capture (flush_turn, flush_session) refuses a listed session by id, not only by MEMHUB_HARNESS_CHILD — the same override put 80 copies of one session on staging; - the child runs with --no-session-persistence: no transcript on disk, so there is nothing for any hook to capture, sense or resume at all; - MEMHUB_HARNESS_DEPTH=1 in the child refuses on its own; - at most MAX_LIVE_DRAINS=2 author passes run on a machine, logged when it trips — the breaker for whatever the next loop looks like; - the child's own hand-off prompt is harness text to the sensor. Also in this change: - cmd_author logged `detail[:60]` after #259 renamed it to `fields`: a NameError after the FIRST moment of every drain, caught by the handler that reports "drain could not start". Every drain authored one moment and told the person the pass had failed. Regression test included. - the child runs with --output-format json and the outcome row records the child id and what the pass spent (measured before this: nowhere). Tests: tests/harness_recursion_test.py (7), fork-lane tests in harness_stop_test.py. With the four guards removed, exactly those four recursion tests fail (mutation-checked). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad630d941b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if str((m.get("state") or {}).get("session_id") or "") in kids: | ||
| # Stamped inside an author child: the loop's own output, left | ||
| # behind by an install that had no registry yet. | ||
| continue |
There was a problem hiding this comment.
On an upgrade from the affected v0.74.0–v0.76.0 releases, existing author-child moments cannot match this condition because those releases never wrote children.jsonl. Consequently, the exact recursive debris this migration claims to clear remains in pending() and can trigger another costly claude -p run or file a self-referential rule; the added test misses this because it manually registers the legacy child first. Legacy moment rows need a migration or another identification mechanism that does not depend on registry entries created only by this version.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1ad0044 — a pre-list child is recognised by its transcript (the hand-off is a human-role message no person's session holds) and its moment quarantined in the author pass; the test no longer registers the child by hand.
| live = live_drains(now) | ||
| if live >= MAX_LIVE_DRAINS: |
There was a problem hiding this comment.
When Stops for multiple sessions overlap, each process can observe the same count below MAX_LIVE_DRAINS before any of them creates its per-session claim, after which all proceed to spawn. Thus the advertised two-pass recursion breaker can be exceeded by an arbitrary number of concurrent Stops, precisely when a runaway or multi-session burst makes the cap important. Atomically reserve one of two global slots before spawning instead of separating the count from the reservation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1ad0044 — the slot file is now the count: MAX_LIVE_DRAINS O_EXCL slots taken before the spawn; test races 8 Stops behind a barrier and exactly 2 spawn.
…r transcript Two Codex findings on #275, both real: - The breaker counted live passes and then claimed: concurrent Stops all counted the same number before any of them claimed, so the cap held for one Stop at a time and not for the burst it exists for. The slot is now the count — MAX_LIVE_DRAINS O_EXCL slot files, taken before the spawn, touched after every authored moment, released in the pass's finally. Test: 8 Stops behind a barrier, exactly 2 spawn; a refused Stop releases its per-session claim; a freed slot is taken by the next Stop. - v0.74.0–v0.76.0 wrote no children list, so the moments their children stamped were on disk with nothing marking them, and the earlier test had registered the child by hand. Their transcripts ARE marked: the hand-off is a human-role message that says CHILD_MARK. The author pass checks that once per session, lists the child, and marks the moment handed with outcome `quarantined` — never authored, never told to the person. Runs in the detached pass, never the Stop hook. Test registers nothing by hand; a person's session that merely quotes the sentence in a tool result is not a child. Both mutation-checked: with each fix reverted, exactly its test fails. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
GitHub Actions
Active
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0a4aeab45
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if now - path.stat().st_mtime > DRAIN_CLAIM_STALE_S: | ||
| path.unlink(missing_ok=True) | ||
| continue # once: a live holder may have just re-taken it |
There was a problem hiding this comment.
Fresh evidence in this revision is that a slot is considered stale after 900 seconds, exactly the same duration as AUTHOR_TIMEOUT_S, while its mtime is refreshed only after run_author returns. If a child reaches its timeout while another Stop runs, that Stop can unlink and reacquire the still-owned slot; the original pass can then unlink the replacement slot in its finally, allowing further passes and defeating the two-pass cap. Use an ownership-safe lock/lease with a heartbeat during the subprocess, or make reclamation occur only beyond the maximum possible live interval.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 20a881f — the slot is a lease now: holder token written into it, release_slot unlinks only a slot still carrying that token, and run_child heartbeats the slot + claim every 30s while the child runs (SLOT_STALE_S 600s, well past any live silence). Test covers the reclaimed-slot case.
…guards # Conflicts: # plugins/memhub-staging/.claude-plugin/plugin.json # plugins/memhub-staging/.codex-plugin/plugin.json # plugins/memhub-staging/.mcp.json
…s, release by token Codex P2 on #275: the slot's stale window equalled AUTHOR_TIMEOUT_S and the holder touched it only between children, so a child at its timeout looked exactly like a dead holder. Another Stop could reclaim the slot, and the first pass's `finally` then unlinked the reclaimer's slot — three passes where the cap says two. Now the slot carries the holder's token (pid + uuid), `release_slot` unlinks only a slot that still carries it, and `run_child` heartbeats the slot and the session claim every HEARTBEAT_S (30s) for as long as the child runs. SLOT_STALE_S is 600s: far beyond the longest silence a live holder can have, short enough that a dead holder frees its slot within ten minutes. Also merges origin/main (#274 removed plugins/memhub-staging; its manifests leave this branch with it). Tests: a slot older than SLOT_STALE_S with a live heartbeat is touched; a slot carrying another token is not released; run_child touches the leases it is handed. Mutation: with the token check removed, the lease test fails. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20a881f1a7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if now - path.stat().st_mtime > SLOT_STALE_S: | ||
| path.unlink(missing_ok=True) | ||
| continue # once: a live holder may have just re-taken it |
There was a problem hiding this comment.
Fresh evidence in this revision is that stale-slot reclamation is not atomic: when multiple Stops observe the same stale slot, they can all pass the stat() check, and after one process unlinks and recreates the slot, another process can unlink that newly acquired lease by pathname and acquire it again. Each process that previously returned the slot will still spawn an author pass, so a burst following a crashed/stale holder can exceed MAX_LIVE_DRAINS by an arbitrary amount and defeat the recursion circuit breaker. Reclamation needs to verify ownership/inode at deletion time or use an atomic stale-file replacement protocol.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0b03682 — no reclamation protocol at all now: the slot (and the per-session claim) is an exclusive advisory lock on an open file, taken by the Stop hook and handed to the detached pass by fd. A dead holder's lock is released by the kernel; nothing is stat'ed or unlinked. Tests cover a holder dying mid-pass, a live holder in another process, and the lock surviving the parent's exit until the child's.
…o the pass by fd Codex, third finding on the breaker (#275): reclaiming a stale slot by stat-then-unlink is not atomic — a burst of Stops that all see the same stale slot can each unlink and re-create it by pathname, and each spawns. Three file-based lease shapes (count-then-claim, stat-then-unlink, token-checked release) each left a race of this kind. So there is no lease file protocol any more. A slot, and the per-session claim, is an exclusive advisory lock (`portable_lock`) on an open file. The Stop hook takes it, hands the fd to the detached author pass (`pass_fds`, `--lease-fds`), keeps its own copy in `_HELD` until it exits, and the pass closes the fd in its `finally`. A pass that dies loses the lock to the kernel. Nobody decides whether a holder is dead; nothing is unlinked; `live_drains` is a probe for the log line, never a decision. The pass's `claude -p` runs with close_fds so it never inherits the lock. Verified end to end: the real detached `harness_stop.py author` inherits both locks (a sibling probe sees the slot held while the hook is alive) and both are free once it exits. Tests: 8 concurrent Stops → exactly 2 spawn; a holder that dies mid-pass frees its slot; a live holder in another process cannot be displaced; a lock survives the parent closing and ends with the child. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b03682801
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if pass_fds and os.name != "nt": | ||
| kwargs["pass_fds"] = tuple(pass_fds) |
There was a problem hiding this comment.
On native Windows, this branch omits pass_fds while Popen still uses close_fds=True, so the detached author process does not inherit either lease descriptor despite receiving their numeric values in --lease-fds. As soon as the Stop hook exits, its _HELD handles close and both the machine-wide slot and per-session claim become available while the author is still running; overlapping Stops can therefore spawn duplicate authors and exceed MAX_LIVE_DRAINS, defeating the quota/recursion breaker specifically on the Windows platform that portable_lock supports. Use an explicit Windows handle-inheritance mechanism or retain the leases through another owner for the pass's lifetime.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4484558 — on Windows the hook now hands the child the lease PATHS and releases after the spawn; the child re-locks them (bounded retry) before authoring and gives the pass up if a sibling won the window. An inherited handle would not have carried a Windows byte-range lock anyway. POSIX keeps the fd hand-off.
Codex, #275: spawn_detached omitted pass_fds on Windows while the Stop hook still closed its copies on exit, so a Windows author pass held neither lease and the cap was void there. An inherited handle would not have helped: a Windows byte-range lock belongs to the process that took it. So on Windows the hook takes the leases only to decide, hands the child their PATHS, and releases them right after the spawn; the child retries the locks for up to LEASE_WAIT_S before authoring anything, and if a sibling Stop won that window it gives the pass up — the moments stay for a later drain — rather than author unlocked. The wait is bounded here, because POSIX flock's blocking mode has no budget at all. POSIX keeps the fd hand-off. Test: a pass handed lease paths holds both while it authors and neither afterwards; with a sibling holding the slot it authors nothing and hands nothing. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4484558827
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if not owner or is_registered_child(owner) or not is_legacy_child(owner): | ||
| return False |
There was a problem hiding this comment.
Fresh evidence in this revision is that registering a legacy child makes subsequent moments from that same child bypass quarantine: cmd_author snapshots the whole batch before the loop, the first moment calls register_child, and this condition then returns False for every later moment with that owner, causing the caller to invoke run_author on the recursive debris. The same path occurs when two drains snapshot the moment before either registers its owner. Thus a legacy child with multiple queued moments can still consume model quota or file self-referential rules; an already registered owner must be treated as a reason to skip/mark the moment handled, not as evidence that it is safe to author.
Useful? React with 👍 / 👎.
This branch was successfully deployed
1 participant
The bug
The harness's author child forked itself, recursively, until the machine ran out of something. v0.74.0–v0.76.0.
run_authorhanded the childMEMHUB_HARNESS_EXTRACT=0. Claude Code applies the install'ssettings.jsonenvover the inherited environment — and opting in isMEMHUB_HARNESS_EXTRACT=1in that file. So the child was sensed like a person's session; its one turn is the create-rule flow, exactly the text the classifier flags; and its Stop drained that moment by forking the child. One generation per generation.pending()selecting by repo across every session's file meant any Stop on the machine would pick a child's moment up too.7113985(#268) refuses onMEMHUB_HARNESS_CHILD. It is onmainand in no release — the marketplace pinsmemhub--v0.76.0, whoseharness_extract.pyhas no child guard. It is also one more environment variable, and an environment variable is what failed.Guards that are not the environment
Each tested with the environment saying the opposite (flag on, child flag unset):
--session-id, verified on 2.1.278 with and without--resume --fork-session) and writes it toharness/children.jsonlbefore the child runs. A listed session's Stop is never sensed, it can never spawn an author, and a moment it stamped is never drained — which also clears debris left by installs that had no list.flush_turn/flush_sessiononly checkedMEMHUB_HARNESS_CHILD; the same override put 80 copies of one session on staging.--no-session-persistence). Every hook that could capture, sense or resume needs atranscript_paththat exists.MEMHUB_HARNESS_DEPTH=1in the child refuses on its own.Also fixed
cmd_authorloggeddetail[:60]after feat(harness): the filed line says which rule and what it catches (ENG-1107, v0.67.0) #259 renamed it tofields→NameErrorafter the first moment, caught by the handler that appends "drain could not start" — shown to the person as a failed pass. Regression test reproduces it on the old line.--output-format json; the outcome row records the child id and what the passspent(previously recorded nowhere).Tests
tests/harness_recursion_test.py(7 new) + fork-lane tests moved intoharness_stop_test.py. Mutation check: with the four guards removed, exactly those four tests fail and the unrelated ones pass. All harness/flush/guard/version/registration/documentation suites green.Not in this PR
The author still forks the owner's session and re-reads its whole context on every call (measured: 331M cache-read tokens over 118 forks). The packet author + replay eval that addresses the cost follows as v0.77.0.
Release
Version 0.76.2 in all manifests. After merge: tag
memhub--v0.76.2→ pin the marketplace. Until then every opted-in install of v0.76.0 has the loop.🤖 Generated with Claude Code