Skip to content

fix(memory): run nightly consolidation off the px-mind tick (#291) - #293

Closed
adrianwedd wants to merge 1 commit into
feat/consolidation-health-visibilityfrom
fix/291-consolidation-background-job
Closed

adrianwedd wants to merge 1 commit into
feat/consolidation-health-visibilityfrom
fix/291-consolidation-background-job

Conversation

@adrianwedd

Copy link
Copy Markdown
Owner

Fixes #291.

Stacked PR. Based on feat/consolidation-health-visibility (#289), which
gave consolidation a health component. Retarget this to master once #289
merges.
The diff below is against #289's tip, not master.

#289 made the nightly consolidation failure visible. This makes it
survivable: three defects were reinforcing each other so that success was
structurally impossible under load, and fixing any one alone would not have
helped.

  • The pass ran inline on px-mind's ~60s awareness tick, while its declared
    deadline is 600s — twice px-mind's own 300s staleness window. Honouring the
    budget and keeping the mind loop alive were mutually exclusive.
  • So it carried an ad-hoc timeout=180 that silently overrode the declared
    600s. The tighter number always wins, so the declared budget was never once
    reachable: every live failure measured exactly 180.1s, every success 30–65s.
  • And attempt 2 could not be spent. memory.MAX_ATTEMPTS_PER_DAY promised
    two tries a night while _TYPE_QUOTAS["consolidate"] was 1 and
    _TYPE_COOLDOWNS["consolidate"] was 20 hours. Attempt 1 consumed the only
    slot attempt 2 could ever have used.

Scheduling semantics: before → after

before after
Where it runs inline on px-mind's awareness tick daemon thread (px-mind-consolidation) inside px-mind
px-mind during a run blocked for the whole call — no awareness, no reflection, no battery check keeps ticking; the tick only supervises
Effective deadline 180s (ad-hoc override) 600s — brain._DEADLINE_S["consolidate"], the declared value
run_claude_session(timeout=) int = 300 default int | None = None — None means "use the kind's declared deadline"
Attempts/night 2 promised, 1 spendable 2 promised, 2 spendable
_TYPE_QUOTAS["consolidate"] 1 2 (== memory.MAX_ATTEMPTS_PER_DAY)
_TYPE_COOLDOWNS["consolidate"] 72000s (20h) 2400s (40min, == memory.RETRY_SPACING_S)
Retry spacing none — attempt 2 was re-asked on the very next tick and refused ≥40min, enforced by memory.consolidation_due()
30-min global cooldown attempt 2 landed inside it spaced past it; consolidate stays out of _GLOBAL_COOLDOWN_EXEMPT
"Running right now" not representable state/consolidation_job.json, pid + heartbeat keyed
A stuck/abandoned run invisible reported once (overran, abandoned), no new health status enum value
Marker after a px-mind restart n/a detected as stale, cleared, recorded as a failure
Success / correct skip marks the date done unchanged
Window, meta file shape 02:00–06:00 Hobart, state/consolidation_meta.json unchanged (plus a last_attempt_ts field)

Why a daemon thread + a state-file job record

The brief asked for the smallest existing repo pattern. This is two of them
composed, no new machinery:

  • wander._ExploringRefresher — the in-daemon threading.Thread(daemon=True)
    idiom, for work that must not block its owner's loop.
  • px-mind's own /proc/{pid} single-instance guard — the liveness idiom,
    for deciding whether a marker on disk still has an owner.

Alternatives considered and rejected:

The thread dying with its process is a feature, not a limitation: it is what
makes "a marker whose pid is gone" unambiguously a lie rather than a maybe.

Proof that px-mind cannot be stalled

tests/test_consolidation_background_job.py::test_tick_stays_responsive_while_consolidation_exceeds_300s

The worker blocks on a threading.Event the test owns and never sets during
the loop
. The tick is then called five more times with the job marker
backdated past 600s, and each iteration asserts:

assert not gate.is_set()
assert worker.is_alive(), (
    f"tick {i} returned only because the worker had finished — "
    "it must return while the worker is still running")

Reaching the end of that loop is the proof: a tick that waited on the worker
could not return at all, because the only thing that can release the worker is
the test itself, after the loop. It is deliberately structural rather than a
stopwatch
— this Pi is the live robot and routinely sits above a load average
of 10 (26 while writing this), where a single fsync has been measured taking
tens of seconds. A wall-clock budget would have proved nothing extra and failed
for the wrong reason; an earlier draft of this test did exactly that (a tick
measured at 35.5s that was not blocked on anything).

Supporting assertions in the same file:

  • test_no_duplicate_concurrent_consolidation — three further ticks while the
    worker runs start nothing new (len(calls) == 1, same thread object), and
    claim_consolidation_job() independently returns False, so the guard does
    not rest on a process-local variable alone.
  • test_the_tick_heartbeats_the_marker_while_the_worker_runs — the heartbeat is
    written by the tick, not the worker (the worker is blocked in ask_brain
    and could not beat if it wanted to).
  • test_an_overrunning_worker_is_reported_exactly_once — four ticks past
    JOB_OVERRUN_AFTER_S produce consecutive_failures == 1, not four.
  • test_a_marker_from_a_dead_owner_is_not_a_running_claim — a marker with a
    dead pid is cleared and recorded as abandoned, and the next tick is free to
    run; no persisted "running" marker survives a restart as a false claim.
  • test_a_marker_with_a_silent_heartbeat_is_stale — a live pid is not enough;
    a process that exists and stopped ticking is stale, and so is a marker with no
    heartbeat at all (absent-is-stale, so an unreadable marker can never block a
    night forever).

Proof that the declared 600s is what reaches the brain

Three layers, no live call and no real wait anywhere:

  1. test_consolidate_passes_no_ad_hoc_timeout — memory.consolidate() calls
    run_claude_session with no timeout kwarg at all.
  2. test_run_claude_session_defaults_to_the_declared_deadline — the default is
    None, and None is what arrives at ask_brain; brain.deadline_for_kind ("consolidate") == 600.
  3. test_the_declared_600s_is_what_reaches_the_request — ask_brain is driven
    with session_state stubbed validated and tmux_claude.inject stubbed to
    capture-then-fail, so it writes the real request into conftest's tmp mailbox
    and returns immediately. The assertion is on the number in that file:
    580 <= request["deadline"] - before <= 601. A window rather than an
    equality because ask_brain deducts validation/lock wait from the budget.

Other callers passing ad-hoc timeouts for classified kinds (listed, not fixed)

Caller passes declared note
src/pxh/mind.py:3498 timeout=600 self_debug = 900 real mismatch — the override is tighter and wins, same shape as the bug fixed here
bin/px-blog:739 timeout=300 blog = 300 matches; redundant, drifts silently if the table changes
bin/tool-blog:59 timeout=300 blog = 300 same
bin/tool-research:48 timeout=300 research = 300 same
bin/tool-compose:49 timeout=300 compose = 300 same
src/pxh/vision.py timeout_s=CLAUDE_TIMEOUT → ask_brain("describe_scene") 60 deliberate: pinned against wander's outer DESCRIBE_SCENE_TIMEOUT budget by tests/test_wander.py
bin/px-evolve ×4 timeout=… — moot; evolve is not brain-routed and raises ColdStartForbidden

Only the self_debug one is a live defect. Out of scope here; left for a
follow-up so this PR stays about #291.

Invariants preserved

  • Resident-only Claude. No new Claude process, no cold fallback. The worker
    reaches the same ask_brain → spark-brain mailbox as before, through the
    same single-flight FileLock; the only change is which thread blocks on it.
    python tools/check_resident_claude.py → clean.
  • Single-flight. Nothing here calls ask_brain concurrently — there is one
    worker at a time, enforced twice over (process-local thread handle and the
    pid-keyed marker under a FileLock).
  • No new health status enum value. "Running" is the job marker's business;
    health.py still reports only what finished. bin/px-motd renders the
    in-flight hint additively (long-term memory (consolidating now for 3m) last formed 20h ago) and ignores a marker whose heartbeat has gone quiet.
  • Existing consolidation contract. 02:00–06:00 Hobart, ≤2 attempts/day,
    state/consolidation_meta.json, success-or-correct-skip marks the date done —
    all still pinned, in tests/test_memory.py and
    test_the_window_and_meta_shape_are_unchanged.

Tests

All inert: no service touched, no tmux session reached, no Claude call, no
restart. Every duration is synthetic — the "600s" is asserted as a plumbed
number and the ">300s run" is an Event plus a backdated marker. There is no
manufactured live 600s brain call anywhere.

$ pytest -q -m "not live" \
    tests/test_consolidation_background_job.py \      # new (17)
    tests/test_mind_consolidation_health.py \         # updated for the async tick
    tests/test_health_memory_formation.py \           # +4 for the job marker in px-motd
    tests/test_memory.py \                            # updated for retry spacing
    tests/test_claude_session.py \                    # quota/cooldown pinned to memory.py
    tests/test_mind_utils.py \
    tests/test_resident_routing.py \
    tests/test_resident_only_invariant.py

308 passed in 131.68s

$ python tools/check_resident_claude.py
clean

One unrelated red flag seen along the way, for the record:
tests/test_health.py::test_overall_is_the_worst_component failed once inside a
915s combined run (assert 'stale' == 'ok') and passes in isolation — that
run was starved enough for px-mind's 300s window to elapse between
record_success and read_health. Nothing here touches health.py's status
derivation; it is the repo's known "flaky suite under load" class, and CI is the
gate.

Run targeted (-m "not live") rather than as a full suite — this checkout is on
the robot itself and CI is the gate.

Filed separately

#292 — 82 orphaned state/health/tmp*.tmp files (2026-08-06 → 2026-08-23).
Investigated while here; the cause is atomic_write's temp file surviving a
SIGKILL (all are mode 0600, i.e. pre-chmod; complete-but-unrenamed at
169/338 bytes or empty at 0), which no code inside the dying process can clean
up — the remedy is a sweeper, which is a design decision rather than a tiny
proven fix. None have appeared since 2026-08-23, i.e. since the #286/#219
restart fixes landed. The live files were not deleted — they are the
evidence.

Consolidation could not succeed under load, and the three reasons were
mutually reinforcing — fixing any one alone would not have helped.

It ran inline on px-mind's ~60s awareness tick while its declared deadline
is 600s, twice px-mind's own 300s staleness window. Honouring the budget and
keeping the mind loop alive were mutually exclusive, so the call carried an
ad-hoc timeout=180 that silently overrode the declared 600s: every live
failure timed out at exactly 180.1s while every success took 30-65s. And
attempt 2 was unspendable — memory.MAX_ATTEMPTS_PER_DAY promised two tries
while _TYPE_QUOTAS["consolidate"] was 1 and its cooldown 20h.

- mind: `_consolidation_tick` now only supervises. It reaps a finished
  worker, heartbeats the in-flight marker, clears what a previous process
  left behind, and starts at most one `px-mind-consolidation` daemon thread.
  It always returns promptly; awareness, reflection and the battery check
  run behind it.
- memory: `state/consolidation_job.json`, keyed on pid + heartbeat. The
  worker dies with its process, so a marker outliving its owner is always a
  lie — a restart cannot leave a false in-progress claim, and the cleanup is
  recorded as a failure rather than silently reset. An unfinished run past
  JOB_OVERRUN_AFTER_S is reported once, not every 60s.
- memory: `consolidate()` passes no `timeout`, and `run_claude_session`'s
  default is now None, so brain._DEADLINE_S is the single deadline source
  and the declared 600s is what reaches `ask_brain`.
- claude_session: consolidate quota 1 -> 2 (== MAX_ATTEMPTS_PER_DAY) and
  cooldown 72000 -> 2400 (== RETRY_SPACING_S). 40min spaces the retry *past*
  the 30-min global cooldown rather than exempting it from one — nobody is
  waiting on a 3am retry.
- px-motd renders an in-flight run additively; no new health status value.

Resident-only Claude is untouched: same ask_brain, same single-flight lock,
same mailbox, no second process and no cold fallback. Only which thread
blocks on it changed.

Tests are inert and every duration is synthetic: the ">300s run" is a
threading.Event plus a backdated marker, and the 600s deadline is asserted
as a plumbed number read out of the request file. No live brain call.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ThqC6Gq4mZyXWnnvGC2a57
@adrianwedd
adrianwedd deleted the branch feat/consolidation-health-visibility August 25, 2026 00:47
@adrianwedd adrianwedd closed this Aug 25, 2026
@adrianwedd

Copy link
Copy Markdown
Owner Author

Superseded by #294. GitHub auto-closed this PR when feat/consolidation-health-visibility (#289) was deleted on squash-merge, and a closed PR whose base branch is gone can be neither retargeted nor reopened. Same branch, same commits, rebased onto master.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant