fix: harden the stall defect class found auditing the 1.2.2 fix - #392
Merged
Conversation
Two instances of the same defect class the write-lock deadline work left behind: an await in a scheduler-gated path with nothing bounding it. Reads (count / get_by_id / find_where / find_where_paginated / search) were skipped last round on the reasoning that a read takes no lock and so blocks no writer. True, but incomplete: the cascade drain loop reads on every batch and advances strictly one batch at a time, so a read that never returns stops the whole md -> LanceDB projection. Claimed rows stay `processing` forever (claim_pending_batch only takes `pending`, orphan recovery runs once at startup), and /health keeps reporting healthy because a hang raises nothing. Budget 60s, ~1000x the measured 62ms flat scan over 117k rows. The empty-index-dir sweep ran inside the prune critical section under a docstring contract requiring the write lock. That contract could not hold: the sweep runs via asyncio.to_thread, and a deadline cancels the future, not the thread, so an orphan sweep outlives the lock -- and Path.iterdir is a lazy os.scandir, so it can yield a dir created after the scan began. It could therefore rmdir a directory a concurrent create_index had just made, leaving the table with no FTS index and every search on that kind 500ing. Safety now comes from an age filter (skip dirs younger than 300s), which holds regardless of lock ownership; the sweep moved out of the critical section so a slow filesystem walk can no longer overrun the prune budget. Mutation-verified: moving the table handle back outside the read deadline hangs the new test; moving the sweep back inside the lock fails it; setting the age floor to 0 fails the fresh-dir test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The drain / heartbeat / rebuild loops were plain create_task coroutines. One uncaught exception ended that loop permanently: nothing restarted it, and because the worker holds a strong reference to the task the interpreter never printed "Task exception was never retrieved" either (that fires on GC). The loop's job just stopped happening with zero output. _run_loop had an inner try; its two siblings did not. Each loop now runs under _supervise: log, wait, restart with escalating backoff (5s / 15s / 45s), then request process exit via SIGTERM so a restarting supervisor (systemd Restart=always, Docker restart: unless-stopped, a k8s Deployment) can recover it. SIGTERM rather than os._exit so the ASGI server runs its graceful-shutdown path. A done-callback is the last-resort observer for the supervisor itself ending unexpectedly. Separately, the fallback rebuild reset the same counter the health verdict reads, so the optimize-failure threshold was effectively unreachable: a table failing 100% of the time cycled 1..5 -> 0 -> 1.. and the threshold value existed only during the sub-second rebuild, ~1% observable against a 30s scrape. cascade.healthy stayed green while the table never reclaimed a version. The rate limiter moves to failures_since_fallback; only a successful optimize clears the alert streak. Same shape as the run7 cross-kind max() masking bug -- a remediation path refreshing the signal meant to report it. Mutation-verified: dropping the restart budget, removing the exit request, and restoring the counter reset each turn the corresponding test red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Acquisition polls with LOCK_NB instead of blocking inside a worker thread. A blocking flock could be neither bounded nor cancelled: cancelling the awaiting coroutine leaves the thread to acquire the lock later with nobody left to release it, which is strictly worse than waiting. The wait itself is correct by design -- the second process is supposed to wait, then find the migration already done -- and flock is released by the kernel on process exit, so a dead holder never wedges it. What was wrong is that it had no upper bound and emitted nothing: a server startup landing on a held lock looked like a hang whose last log line was lifespan_provider_startup name=lancedb. It now logs memory_root_lock_waiting on first contention, reports how long it waited on success, and gives up after timeout_seconds (default 300s, generous because the legitimate holder is an O(rows) migration). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
test_lap_append_during_handler_no_loss asserted no loss after _wait_path_done, whose settle window is 0.1s. That is a bet that the filesystem event for the appends which landed *during* a handler invocation has already been delivered — a terminal row does not mean the file is fully projected, because the handler read the md at whatever length it had then, marked the row done, and the rest arrive on a later event. The bet holds on macOS/fsevents and lost on a loaded Linux runner (md=30 lance=17), failing the assertion for a reason unrelated to the behaviour under test. Waits for quiescence instead: terminal row + empty pending queue + a projected count unchanged across three consecutive polls. Strictly stronger than the old condition, and real loss still fails — the count just converges below the md entry count and stays there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rebuild_indexes dropped every index and recreated it. The docstring justified that with "LanceDB transparently falls back to brute-force scan", which is true for vector search and false for FTS: with no inverted index a BM25 query raises "Cannot perform full text search unless an INVERTED index has been created". The recall legs are gathered without return_exceptions, so one failing leg fails the whole search request -- every keyword search landing in the window returned 500. Measured: 55 failures across 3 rebuilds with drop+create, 0 with create_index(replace=True). Replacing also collapses the live fragment set identically (7 index files back to 4 after 25 optimize beats), so nothing the rebuild existed for is lost. Only indexes on columns that are no longer indexed at all are still dropped -- nothing queries those. A rebuild that loses the manifest race is now retried rather than deferred to the next 12h sweep. Lance marks the conflict Retryable and means it: another process committed first. Retries are recorded as a deadline on the kind (10min / 30min / 3h) and picked up by the rebuild loop, not slept through -- the loop walks kinds sequentially, so sleeping would park every later kind behind the backoff (7 kinds x 3h outlasts the cadence itself). The loop tick is min(60s, cadence) so a shorter configured interval is not quantised. Removes the empty-index-dir sweep. cleanup_older_than deletes the files under a superseded _indices/<uuid>/ but leaves the directory, and everos was removing those with its own rmdir. No LanceDB contract says an empty index dir is garbage, so this is being raised upstream instead. Note it is a separate gap from index *files* not being reclaimed under delete_unverified=False (260MB retained on a 19k-row soak table) -- solving that still leaves the empty dirs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rebuild-bound and lock-wait entries were swallowed into the Removed section when the husk-sweep entry was inserted above them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gloryfromca
force-pushed
the
fix/stall-class-hardening
branch
from
August 6, 2026 06:04
4acd9a4 to
bef0cea
Compare
Restores the empty-index-dir sweep with a safety argument instead of a self-chosen number. Upstream context, read from lance's cleanup.rs: it unlinks a superseded index's files but never the directory, and contains no rmdir at all. That is structural, not an oversight -- lance targets object stores, where paths are flat keys and an empty directory does not exist. Only a local filesystem materialises them, where they accumulate as inodes (a soak run reached 13061 dirs, 98% empty) and slow every directory scan. Three independent guarantees, in order of strength: 1. rmdir cannot delete data. The kernel refuses it on a non-empty directory, so no file can be lost whatever the rest of the logic decides -- and because the check *is* the operation, there is no check-then-act window to race. 2. Live indexes are excluded by UUID, read from list_indices(). 3. Anything else must outlive UNVERIFIED_THRESHOLD_DAYS = 7, which is lance's own bound for deciding an unreferenced index UUID is dead rather than an index build in progress. Matching it means the sweep can never be more aggressive than lance itself; the previous 300s was our invention, and that is what made it indefensible. Each guarantee is pinned by the same test and mutation-verified: dropping the live-UUID check, zeroing the age gate, and swapping rmdir for a recursive delete each turn it red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The four cadences were already constructor arguments on CascadeWorker, but CascadeConfig did not carry them and none of the three production construction paths passed a config, so the module defaults were unreachable from outside the code. That is why no soak run shorter than half a day could exercise the 12h rebuild sweep: not a missing parameter, a config layer that dropped it. Adds CascadeSettings ([cascade] in default.toml) and CascadeConfig.from_settings, which the orchestrator now uses when no config is passed -- so the CLI, backfill and server paths all pick settings up at once. Deliberately not exposed: the read / write / prune / rebuild deadlines. Those are hang-catchers sized from measured durations, and both directions are worse -- too low manufactures failures on a healthy table, too high leaves a wedged one invisible for longer. Cadences depend on write volume and are a real tuning axis; deadlines are not. A test pins the exposed field set so a later change has to state its intent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
agent_case was the one business kind with no handler test, and the storage soak never writes it either -- four of the seven tables stay at zero rows there -- so its md -> row contract was unexercised from both directions. Covers what makes this kind different from its daily-log siblings: it lives on the agent track, and it embeds task_intent only while approach is BM25-indexed but deliberately never sent to the embedder. Plus the branches every handler shares: soft-dependency embedding (no provider -> vector=None, row still written for keyword-only deployments), optional KeyInsight, the content_sha256 short-circuit that stops the 30s scanner re-embedding untouched files, edit detection, and delete-by-path. Mutation-verified: routing approach into the embedder, and dropping section:TaskIntent from content_change_keys, each turn the relevant test red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gloryfromca
requested review from
Kendrick-Song and
cyfyifanchen
and removed request for
Kendrick-Song
August 6, 2026 09:39
Two constants now carry the arithmetic instead of leaving it in a chat log. _HUSK_MIN_AGE_SECONDS states what the 7-day gate costs: nothing reclaims an empty index dir before then, each is an inode plus a 4KB block, and at ceiling load that is ~890k dirs / ~3.6GB / 14% of a default 98GB ext4's inodes at the 7-day steady state. Also that this is the worst case and needs sustained saturation -- a single-user deployment sits four orders of magnitude below it -- and that only ext4 has a fixed inode budget (APFS and xfs allocate dynamically, Windows is out of scope). DEFAULT_OPTIMIZE_MIN_INTERVAL_SECONDS gets two things it never said. First, it is not a visibility delay: a row is searchable as soon as its upsert commits, because LanceDB flat-scans the unindexed tail -- verified to cover BM25, not just vector and scalar, which was the leg worth doubting given a missing FTS index hard-fails rather than degrading. Sparse writes do not wait at all, since the scheduler uses max(0, interval - elapsed). Second, it is the ceiling on index-directory growth: past roughly one write per table per interval the beats coalesce, so the accrual rate is capped by this interval rather than by write volume, and raising it lowers the cost proportionally. That makes it the knob to reach for if the empty dirs ever bite -- not the husk threshold. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The supervisor's restart budget was spent per process lifetime: three strikes ever, regardless of how long the loop ran healthily between them. A loop hitting one recoverable transient every few days — each cleared by a single restart — would still pool those strikes and SIGTERM a healthy server weeks in, on the 4th, which punishes exactly the case supervision exists to absorb. The budget now counts consecutive quick crashes: a body that ran at least 60s before raising starts a fresh incident with the full ladder. A deterministic crash-on-entry still exhausts the budget in ~65s. Same windowed counting as systemd StartLimitIntervalSec / Erlang max_restarts-per-max_seconds. Also corrects the husk accrual numbers in the optimize-cooldown docstring to the ~14-day effective reclaim horizon (see the sibling lancedb commit for why the age gate doubles). Mutation-verified: with the reset removed, the new test exits after run 3 instead of surviving to run 5. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
By the time the sweep runs, the cleanup commit — the thing prune exists for — has already succeeded, and the sweep is best-effort by contract. Letting its deadline escape prune() billed the failure to the wrong account: the optimize scheduler counted a prune failure (feeding the fallback-rebuild threshold) and the prune-staleness clock stopped advancing, so both alarms reported a cleanup stall that did not happen. Same defect shape as the alert counter the fallback rebuild used to zero: an auxiliary path corrupting the main signal's ledger. Reachable, not theoretical: sweep time is proportional to dir count (~35us/dir measured) and the ceiling-load steady state sits right at the 60s budget. The timeout is tolerable exactly because it is now swallowed — and the orphaned worker thread finishes the walk anyway, so the reclamation still happens. Also corrects the age-gate docstrings: the gate reads st_mtime, which POSIX bumps when lance's cleanup empties the husk, so the effective reclaim horizon is file wait + 7 days (~14 days total) and the ceiling-load steady state is ~1.8M dirs / ~7GB, twice the previously recorded figure. The "never more aggressive than lance" property is unaffected (it is strictly more conservative). Mutation-verified: with the try/except removed, the new test fails on the escaping VectorStoreBusyError. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
300s sat at the edge of what the docstring itself calls a legitimate hold (a large migration is minutes of O(rows) work), so the worst honest migration turned every waiting process's startup into a LockError crash. Now 1800s: the wait has been visible since the first poll (memory_root_lock_waiting), and against the one case the bound exists for — a holder alive but wedged — giving up at 5 minutes buys nothing over 30, because the timeout's job is diagnosis, not recovery. The timeout message now says which way to look: the kernel releases a dead holder's flock automatically, so reaching the timeout means the holder is alive — inspect that process instead of retrying this one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Supervisor bullet gains the per-incident budget, the sweep bullet gains the ~14-day effective horizon and the swallowed timeout, and the lock bullet records the 30min default with its sizing rationale. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Kendrick-Song
approved these changes
Aug 6, 2026
The sibling test covers one side -- a deadline miss must not bill prune's ledger. Nothing covered the other: widening the catch to `except Exception` passes every other test in the file, and would turn a genuine fault in _remove_empty_index_dirs (a TypeError after a signature change, a permission error on the index dir) into a silent removed = 0 with no signal anywhere. That is the failure shape this module keeps being audited for, so the narrowness of the catch needs its own guard. Found by mutating the catch rather than by reading it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two maintenance jobs park on each other -- whichever arrives second waits on the first -- so the two waits are one hazard seen from opposite ends. The rebuild side was bounded; this side was left open on the argument that rebuild_indexes carries its own 300s deadline. That deadline covers its critical section, not the task's dispatch and teardown around it, so the transitive bound was never real. While the runner waits, its per-kind task slot stays occupied, every _schedule_optimize call short-circuits on it, and that table silently stops being pruned -- the same shape as the stall this branch has been chasing. Bounded at 180s. On expiry the beat is skipped rather than run: compacting under a live rebuild is the interleaving the wait exists to prevent, and both commit on the same manifest. Writes keep the dirty flag set, so the next beat retries. Mutation-verified: replacing the timeout with a plain await hangs the new test until its own guard fires. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Kendrick-Song
added a commit
that referenced
this pull request
Aug 7, 2026
#392 landed on main with its entries under [Unreleased] and no version bump. Since 1.2.3 ships that code, leaving them there would have the release notes disclaim work the release contains. Merged section by section into [1.2.3] and dated it to the actual release day.
gloryfromca
pushed a commit
that referenced
this pull request
Aug 7, 2026
…#393) * feat(ome): exponential backoff + jitter between retry attempts Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(events): carry case body + vector on agent skill chain * feat(strategies): populate extended agent-skill chain events * feat(md): AgentSkillReader.list_by_cluster for md-first skill enum * fix(strategies): rescue extract_agent_skill from cascade-lag dead-letter Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(agentic): honor radius, use kind-shaped rerank, non-empty passage Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(api/ome): expose dispatched + runs, distinguish not_dispatched Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(release): v1.2.3 — agent-skill rescue + OME/agentic contracts * fix(agentic): drop inert radius plumbing; correct release docs * fix(md): sanitize LLM-generated skill names against path traversal AgentSkillFrontmatter.name comes straight from LLM output (memory.strategies.extract_agent_skill) and was concatenated unsanitized into the skills/skill_<name>/ directory segment on both the write path (agent_skill_writer._skill_dir) and the read path (agent_skill_reader._skill_dir). Given a sufficiently long ../ prefix, the write target could escape the memory root (CWE-22). A live run also produced a skill name containing spaces and CJK characters, proving the "keep snake_case" docstring convention on AgentSkillFrontmatter.name is not enforced at runtime. This is the same class of defect already fixed for knowledge-upload titles/categories. Promote that fix's sanitizer (knowledge_writer._sanitize_dirname) to a shared primitive, everos.core.persistence.markdown.sanitize_dirname, so there is one CWE-22 defense for md directory names instead of two independently maintained copies: - New core/persistence/markdown/path_safety.py holds sanitize_dirname (idempotent: sanitize(sanitize(x)) == sanitize(x)), exported through the markdown + persistence facades. - SkillPathMixin gains skill_dir_name(), the single sanitization point both AgentSkillWriter._skill_dir and AgentSkillReader._skill_dir now derive from, replacing their previous independent string concatenation. - KnowledgeWriter now imports the shared sanitize_dirname instead of keeping its own private copy. - AgentSkillFrontmatter.name gains a field_validator rejecting path separators / ".." as defence in depth, so a hand-edited SKILL.md is caught on parse rather than silently relocating the skill on the next write. Idempotency is what keeps the reader and writer in agreement even though they recover a skill_name from different sources: list_by_cluster derives it from the on-disk (already-sanitized) directory name, while extract_agent_skill._hydrate_algo_skills re-reads using the frontmatter's raw name field. A regression test (test_agent_skill_reader.py) seeds a skill whose frontmatter name contains CJK + a space and asserts both routes resolve to the same file. No data migration: agent-skill extraction has never once succeeded before this branch (the cascade-lag defect this branch fixes meant .skills/ was never created), so there is no legacy skill corpus whose directory names would change under the new sanitizer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(e2e): make the agent-skill chain assertion real tests/conftest.py's autouse fixture pins embedding + rerank capability to unavailable for hermeticity, and this test never opted back in. trigger_skill_clustering and extract_agent_skill both body-guard on get_embedding_capability().available and return early, so the module docstring's "real embedder" claim was false and the skill chain never ran: measured log counts were skill_cluster_updated=0, agent_skills_extracted=0, strategy_gated_off_embedding_unavailable=10. The three skill assertions were assert len(...) >= 0 — always true — with a comment blaming "LLM-dependent" flakiness for a count that was in fact deterministically zero. This is why a defect that made agent-skill extraction fail 4/4 in production reached a release: there was no working e2e coverage of the chain. - New _opt_in_real_embedding_and_rerank autouse fixture, scoped to this file only, resets everos.component.embedding.accessor._capability and everos.component.rerank.accessor._capability to None (the mechanism the global fixture's own docstring prescribes) so both capabilities rebuild from the real .env credentials tests/e2e/conftest.py already loads. Restores to None on teardown; every other test keeps its hermetic default. - Replaced the three vacuous per-agent assertions with one aggregate floor across all three agents (>= 1 total skill). A per-agent floor would be flaky: extract_agent_skill has no cluster-size gate, only everalgo's per-case skip_quality_threshold, so a single low-quality trajectory can legitimately yield 0 skills for one agent. - Added a sharper, defect-specific check: assert no dead-lettered extract_agent_skill run in OME's run_record (via OfflineEngine.list_runs), since a dead-letter (retries exhausted) is unambiguously a failure, unlike a quality-gated 0-skill outcome. - Corrected the module docstring's "real embedder" claim and the old "# 4.5" comment's reasoning: extract_agent_skill has no cluster-size gate, only everalgo's per-case quality threshold. Unexecuted: this test is slow + live_llm and this machine has no provider credentials (the verification .env was deleted), so make ci does not run it and it could not be run here. Verified by inspection instead: ran the test file with -m "" to override the marker deselection and confirmed it proceeds past the new fixture and through app lifespan startup without error, failing only at the expected point — LLMNotConfiguredError from the missing API key — which confirms the fixture and imports are wired correctly and the only blocker is the missing credentials, not a bug in this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(strategies): sanitize skill name before frontmatter construction Follow-up to afe1609 (path-traversal sanitization). That commit added a field_validator on AgentSkillFrontmatter.name rejecting path separators or "..", intended as a read-side defence for hand-edited SKILL.md files. But _persist_skill (memory/strategies/extract_agent_skill.py) constructs AgentSkillFrontmatter(name=skill.name) with skill.name straight from LLM output — the raw, unsanitized string — so the validator actually fires on the write path too. Sanitization already makes the on-disk path safe (SkillPathMixin.skill_dir_name), so an LLM emitting a traversal-shaped name (reachable via prompt injection, since the LLM's input is user conversation content) gained nothing from the validator except a new failure mode: ValidationError -> strategy raises -> OME retries with backoff -> dead-letter -> that case's skill is permanently lost. A DoS vector introduced by a security fix, and it also contradicted the validator's own docstring ("catches a hand-edited SKILL.md"). Also fixes a latent second bug in the validator itself, caught by the new tests below: it rejected any name containing the substring "..", but sanitize_dirname keeps "." as a safe character, so "../" * 8 + "tmp/pwned" sanitizes to "................tmppwned" — still containing ".." many times over. The validator would have rejected the sanitizer's own safe output. Narrowed the check to actual path separators or the name being exactly ".." (the only case where ".." functions as a real traversal component when there's no separator left to combine it with). Fix: - SkillPathMixin gains sanitize_skill_name(skill_name) — the bare sanitized name (no skill_ prefix), factored out of skill_dir_name so both share one sanitizer call. - _persist_skill now sanitizes skill.name via sanitize_skill_name once, up front, and uses that same sanitized string for AgentSkillFrontmatter.id, .name, and the writer.write_main() call. A traversal-shaped LLM name is now made filesystem-safe before it ever reaches the frontmatter constructor, instead of tripping the validator. - The validator's docstring now describes actual behaviour: the write path pre-sanitizes, so the validator only fires for a name that bypassed the writer (e.g. a hand-edited file, or any other direct AgentSkillFrontmatter construction that skips pre-sanitization). Bonus: with the write path pre-sanitizing, frontmatter.name becomes byte-identical to the directory-derived name for LLM-written skills — an identity, not merely an idempotency argument. This also closes the gap in the previous commit's reader/writer test, which proved idempotency generically but never drove an adversarial name through the actual production write path end-to-end. Tests: - test_agent_skill.py: constructing AgentSkillFrontmatter with a pre-sanitized adversarial name (mirroring _persist_skill's own call shape) succeeds and yields a separator-free name; the read-side rejection test for bypassed/hand-edited names is unchanged and still passes with the narrowed check. - test_agent_skill_writer.py: new parametrized identity test — for both an adversarial and a CJK/space raw name, sanitize once, write via that sanitized name, and assert frontmatter.name equals the directory-derived name exactly. - test_agent_skill_reader.py: docstring updated to clarify its existing round-trip test now covers the bypass case (a caller that writes via a raw, unsanitized name directly through the writer, skipping _persist_skill's pre-sanitization) rather than the normal production path, which is proven as an identity by the writer test above. - Existing test_extract_agent_skill.py strategy tests (snake_case fixture names) are unaffected — sanitize_dirname is the identity function for already-safe names. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(md): reject degenerate sanitizer results, read globbed paths Security review of 0c6820f found it did not close the DoS it was meant to close: sanitize_dirname("../") returns ".." verbatim, because "." is a safe character and is not stripped by the character-class filter — only the leading "/" is removed. sanitize_skill_name("../") -> sanitize_dirname("../") -> "..", and AgentSkillFrontmatter(name="..") still raises ValidationError (name == ".." is exactly the case the narrowed validator rejects). Same dead-letter DoS as 0c6820f, just a shorter payload; the previous commit's tests only exercised the long "../" * 8 + "tmp/pwned" payload, which happens to sanitize past the fixpoint. The same fixpoint is a real one-level directory escape on the knowledge path, which has no skill_ prefix to protect it: Path(root) / sanitize_dirname("../", "Others") / "doc_123" resolved to root/doc_123, skipping the category directory entirely. Fix (path_safety.py): sanitize_dirname now falls back on "", ".", or ".." instead of only "". This one change closes both the skill dead-letter DoS and the knowledge one-level escape, since both callers already route through this single primitive. Also NFC-normalizes the input before the character filter (unicodedata.normalize("NFC", raw)), so an NFD-decomposed accented character (base letter + combining mark, which is not \w) no longer silently loses its accent. Rewrote the docstring, which previously claimed ".. sequences are always stripped" (false — "." is explicitly a safe character) and "cannot escape the directory it is concatenated into" (false for an unprefixed caller before this fix); it now states what actually holds: no separator survives, so the result is always exactly one path component, and it is never "", ".", or "..". Also fixes (per review, cheap and worth doing alongside): - AgentSkillReader.list_by_cluster previously globbed skill_*/SKILL.md, stripped the prefix to recover a name, then called read_main(name), which re-derives (and re-sanitizes) the path from that name. Any on-disk directory whose suffix was not already a sanitizer fixpoint (e.g. "skill_My Skill", a raw space) re-derived to a path that doesn't exist and was silently dropped. Since list_by_cluster is the documented strong-consistency existence check, a dropped skill would make the LLM emit add() for a skill that already exists, duplicating it at the sanitized path and orphaning the original. Fixed by having list_by_cluster read each globbed path directly (new _read_path helper, shared with read_main) instead of round-tripping through a recovered name — the reader never derives a path at all on this route, which is a stronger guarantee than the idempotency argument the docstrings previously leaned on. - e2e test: made fixture ordering explicit — the embedding opt-in fixture now takes _reset_embedding_capability_singleton and _reset_rerank_capability_singleton as parameters so pytest's dependency graph guarantees correct ordering, rather than relying on collection order between conftest files. Added a positive "extract_agent_skill actually ran" assertion (any status) before the dead-letter check — without it, the dead-letter assertion alone is vacuously satisfied by a strategy that never executed at all; it was only meaningful before because the skill-count floor happened to run first. Dropped the rerank capability opt-in and the module docstring's "real reranker credentials" claim: nothing on the agent-skill write path touches rerank, so opting it in only widened the credential surface with no coverage benefit. - CHANGELOG: corrected the validator description (rejects a path separator or being exactly "..", not any string containing ".."), and added the previously-missing user-visible fact that AgentSkillFrontmatter.name and the agent_skill LanceDB primary key now hold the sanitized name, not the raw LLM output. Tests: parametrized the sanitize -> construct -> (write, for the writer-level test) tests over a boundary family instead of one long payload: "..", "../", "/../", ".", "./", "!!!" (empty), "a" * 200 (truncation), a CJK+space name, and the original "../" * 8 + "tmp/pwned". Each case asserts the sanitized name is a single component, is never "" / "." / "..", frontmatter construction succeeds, and (writer-level) frontmatter.name is byte-identical to the directory-derived suffix. New test_path_safety.py cases pin the degenerate-fixpoint fallback directly, the knowledge-style unprefixed one-level-escape repro, and NFC normalization. New test_list_by_cluster_finds_skill_whose_directory_suffix_has_a_space reproduces the exact list_by_cluster drop bug against a directory written outside the writer entirely. Explicitly not in scope (per review): the collision behaviour where "fix django" and "fix_django" now map to the same directory is a real product-decision question the reviewer is raising separately, not touched here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(md): record the skill-name collision trade-off No behaviour change. Documents a product decision the coordinator made explicit: sanitize_dirname is lossy, so distinct raw skill names can collapse onto the same directory ("fix django" and "fix_django" both become "fix_django"; "fix!django" and "fixdjango" both become "fixdjango"; names differing only past the 50-char cap also collide). Because AgentSkillWriter.write_main is a full-file replace and the LanceDB primary key is f"{agent_id}_{sanitized_name}", a collision means the later skill silently overwrites the earlier one, losing its accumulated source_case_ids, maturity_score, and body. This is accepted rather than mitigated: the LLM's add/update decision for a skill is keyed on the name it sees, so a collision usually reads as an intended update anyway; and adding a disambiguating suffix would break the frontmatter.name == directory-suffix identity the reader/writer seam (from 0c6820f) relies on. - SkillPathMixin.sanitize_skill_name docstring now states the collision consequence and the two reasons it is accepted, so a reader does not have to derive them. - sanitize_dirname's docstring gains one line: the function is lossy and not injective; callers that need distinct outputs for distinct inputs must disambiguate themselves. General primitive — the knowledge path calls it too. - CHANGELOG: added the collision consequence to the existing path-traversal entry, next to the already-documented fact that name / the LanceDB key hold the sanitized value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(md): return skill bodies from list_by_cluster; correct docs Security review of 1b8cf11 + add842d found the list_by_cluster fix was incomplete: it stopped its own enumeration from dropping a skill whose directory suffix wasn't a sanitizer fixpoint, but the caller (_hydrate_algo_skills) still re-read each selected skill by fm.name via read_main, which re-derives (and re-sanitizes) a path from that name and drops it there instead. Reproduced on a real filesystem: skill_My Skill/ enumerates fine, but read_main("My Skill") re-derives to skill_My_Skill/ and misses. The drop moved one layer downstream; existing_relevant_skills was empty before and after the prior fix. Fix: list_by_cluster now returns (frontmatter, body) pairs instead of frontmatter alone, so the caller never needs a second, name-based read. _select_existing_skills / _rank_skills_by_relevance updated to carry (fm, body) tuples through selection; _hydrate_algo_skills is deleted — the body is already in hand, so there's nothing left for it to do. This closes the drop for real, removes the second disk read (the 2n-read concern carried since Task 4), and makes "the reader never derives a path" true end-to-end rather than true only for list_by_cluster's own enumeration step. New end-to-end regression test (test_select_existing_skills... / test_existing_skills_reaches_llm_for_skill_whose_directory_has_a_space) seeds a skill_My Skill/ directory directly on disk (bypassing the writer) and runs the real extract_agent_skill strategy against it, asserting the skill reaches existing_relevant_skills with non-empty content — the property the previous commit's test docstring claimed but the code didn't yet deliver. The reader-level regression test gained the same body assertion. Also, per review: - path_safety.py: corrected the NFC docstring claim, which was wrong for the ~1,082 Unicode composition-exclusion codepoints (e.g. Devanagari क़/ख़, U+0958/U+0959) — NFC decomposes an already-precomposed exclusion character instead of preserving it, so the combining mark is stripped either way. Scoped the claim to "best-effort for the common case", not a guarantee for every script. New test pins this directly. - Dropped the e2e test's unused _reset_rerank_capability_singleton fixture parameter: the reviewer adjudicated the earlier instruction conflict the other way — ordering is only meaningful between fixtures that touch the same state, and this fixture never reads or writes the rerank capability at all. - Widened the skill-name collision documentation (SkillPathMixin.sanitize_skill_name, CHANGELOG) beyond dropped-punctuation / space-collapse / truncation to the larger case: every combining mark is non-\w and is stripped regardless of script, so e.g. Devanagari "किताब" and "कताब" both collapse to "कतब" (same for Thai tone marks, Hebrew niqqud, Arabic harakat). - Corrected the collision justification: because _persist_skill sanitizes before frontmatter construction, the LLM sees the already-sanitized name in existing_relevant_skills, so a colliding raw name is an *affirmative* decision that two skills are different, not a probable intended update. The decision to accept collisions still stands, but on its real grounds: a disambiguating suffix would break the fm.name == directory-suffix identity the reader/writer seam relies on, and detecting-and-raising would reintroduce the dead-letter DoS. - Merged two consecutive "# -- Internals --" banners in agent_skill_reader.py into one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(md): skip unparseable SKILL.md instead of failing the cluster `list_by_cluster` only handled a missing file, so a ValidationError from `_read_path` aborted the whole enumeration. That enumeration is what feeds `extract_agent_skill` its existing skills, so one bad SKILL.md starved every skill in the cluster and dead-lettered that cluster's extraction on every subsequent run -- the permanent-failure mode this md-first read path was introduced to eliminate. The write path already pre-sanitizes to avoid exactly this; the read path was left open. The trigger surface is the whole schema, not just the traversal validator this PR added: `_read_path` validates the full `AgentSkillFrontmatter`, so a field a later revision makes required would take out every existing file at once. Reproduced both ways. `read_main` still propagates -- a caller naming one specific skill needs an error, since `None` already means "not created yet" and reusing it for "exists but is corrupt" would let an upsert overwrite the damage. Also moves this route's `OfflineEngine` import under TYPE_CHECKING. It is used only to annotate `_summarize_runs`, and an eager import contradicted the deferred `_get_engine` import a few lines below. Note this saves nothing at startup today: `service.memorize` imports the engine eagerly to construct it, so any app process pays the ~750ms apscheduler cost regardless. * docs(md): correct two docstrings, add the case-collision dimension Three fixes to claims that did not match behavior: - `_rank_skills_by_relevance` claimed no skill is silently dropped from the prompt. The backfill loop is capped at MAX_SKILLS_IN_PROMPT, and the function only runs when the cluster already exceeds that budget, so skills beyond K are dropped by design. Reworded to what the backfill actually guarantees: a lagging index cannot under-fill the prompt. - `sanitize_skill_name` enumerated collision causes in detail but omitted case, the dimension an LLM varies most freely. "Fix Django" and "fix django" sanitize to two distinct names -- two LanceDB rows, but one directory on a case-insensitive filesystem (macOS APFS, Windows NTFS defaults), so the index advertises a name whose content was overwritten. - The same docstring justified accepting collisions partly on a disambiguating suffix breaking the `frontmatter.name` = directory-suffix identity. It would not: writing "fix_django_2" into both keeps that intact. Replaced with the real reason it is deferred rather than dismissed -- it needs a collision probe and a case-folding rule. Adds the knowledge-writer sanitization tests that were missing entirely: swapping in the shared primitive changed NFD input ("Résumé" no longer degrades to "Resume") and made a "." / ".." topic fall back. Knowledge upload predates this PR, so unlike skills it has a corpus whose directory names those first cases affect. Both tests verified red against the 1.2.2 sanitizer. * docs(changelog): scope the migration claim, record run_record growth Three corrections to the 1.2.3 entry: - "No data migration" was asserted for the whole sanitization change but only holds for agent skills, which have no corpus because extraction never succeeded. Knowledge upload predates this release and does have one: NFD topics and `.`/`..` topics resolve to a different directory now. Scoped the claim and spelled out both cases. - Added the case dimension to the collision list, and replaced the "disambiguating suffix breaks the name = directory identity" reason with the accurate one -- it does not break it, it just needs a probe and a case-folding rule, so it is deferred rather than rejected. - Recorded that `SkillClusterUpdated` now persists a 1024-dim vector in `run_record.event_payload`: ~0.8 KB to ~14 KB per record, ~14 MB per strategy at the default 1000-record ring buffer. Operators sizing ome.db need this number, and it was not stated anywhere. * fix(strategies): ship extract_foresight disabled by default The sender scan reads `m.role` off every memcell item, but only ChatMessage carries it: ToolCallRequest has `sender_id` and no `role`, ToolCallResult has neither. So any memcell holding a tool call raises AttributeError before the first sender resolves -- correct on plain user chat, guaranteed to fail on agent trajectories, where it burns its max_retries budget and dead-letters on output nothing consumes today. Flipped the decorator rather than `default_ome.toml`, because `everos init` skips an existing `~/.everos/ome.toml` (init_cmd.py:85), so a template edit would reach new installs only. The toml opt-in is left working on purpose -- a chat-only deployment does get correct foresights -- and documented in both the module docstring and the template comment. This is a stop-gap. The fix is per-episode extraction, like atomic_fact, which needs an everalgo entry point that does not exist yet. The one test that used foresight as its UserPipelineStarted subscriber now opts back in through that same toml key, so the opt-in path is covered rather than worked around. It has to wait for the override to reach the registry first: ConfigReloader.start() fires its initial load as a task, so engine.start() returns before ome.toml is applied, and an emit inside that window is judged against the coded defaults and dropped by the enabled gate with no redelivery. * fix(ome): hold engine_sem per attempt, not across the retry chain The backoff this PR added slept inside the semaphore block, so a run waiting to retry kept its concurrency slot. That turns a partial outage into a total stall: with max_concurrent_runs slots and a 1s/2s/4s backoff, enough simultaneously-failing runs park every slot in asyncio.sleep and starve strategies that would have succeeded. The cap exists to bound concurrent strategy work -- LLM calls, embeddings, storage IO -- and a sleeping coroutine consumes none of it. Backpressure on the failing work is intended; backpressure on everything else is not. Semantics change is deliberate and stated in the docstring: the cap still applies to execution, no longer to waiting. The guard uses a single-permit semaphore so locked() is unambiguous, and asserts a second waiter actually acquires -- locked() alone would pass on an implementation that freed the slot but left waiters unable to take it. Verified red against the previous structure. * fix(strategies): reap the directory a renamed skill leaves behind everalgo treats a name change as a first-class update: _apply_update preserves prior.id while swapping the name, so _persist_skill wrote the skill to a new skill_<new_name>/ and the old directory survived with the same cluster_id. That is not a cosmetic leak now. Since existing skills are read from markdown rather than LanceDB, the orphan returns in the next run's existing_relevant_skills as a duplicate of a skill the LLM already renamed -- feeding exactly the add-instead-of-update full-replace clobber this PR set out to close, once more per rename. Reconciliation keys off skill.id, the only field that survives a rename (a fresh add mints a uuid4 and can never match), and never deletes a name another emitted skill just claimed. Also in this pass: - AgentSkillWriter.delete_skill, the one destructive operation here. It fails closed: a directory it cannot resolve by the writer's own path rule is left alone rather than targeted by anything looser. - reference_name / script_filename now sanitized on both reader and writer. They are appended after the skill_<name> segment, so skill_dir_name never covered them. Zero callers in src/ today; closing it before progressive disclosure wires them up. - Agentic case rows with every passage field empty fall back to a placeholder instead of raising ValueError in everalgo's _format_docs and 500ing a whole search the row merely appears in. - The retire op is documented as unimplemented rather than left implied. aextract returns a flat list with no discriminator, so a retirement arrives as an ordinary low-confidence skill and is written back like any other. Honouring it means either giving an LLM confidence score authority to delete the source of truth, or a retired flag that the enumeration, cascade, and search all learn to filter on -- a design decision, deferred. - The embedding body-guard comment no longer claims to protect a local embed call; this strategy stopped embedding when the vector moved onto the event. * fix(strategies): stop extract_foresight crashing on tool-call memcells The sender scan read m.role off every memcell item, but only ChatMessage carries it: ToolCallRequest has sender_id without it, ToolCallResult has neither. The first tool call raised AttributeError before any sender was resolved, so the strategy was correct on plain user chat and dead-lettered every time on agent trajectories. everalgo contracts for exactly this input -- user_memory/_render .chat_messages says "the caller need not pre-filter; an AgentMemCell-shaped MemCell is acceptable input" -- and every other user-memory extractor gets that for free by delegating. This strategy was the one place the filter was hand-rolled, and it was hand-rolled wrong. It stays disabled by default, but for the correct reason: nothing in EverOS reads foresights yet, so running it spends one LLM call per sender per memcell on write-only data. The earlier justification (per-episode extraction needs an everalgo entry point that does not exist) confused extraction granularity with the crash; granularity is still open, the crash was one line. Fixing it is what makes the documented ome.toml opt-in actually usable. The guard pins both directions: a pure agent trajectory extracts nothing and never reaches the LLM, and a mixed memcell extracts for human senders only -- an implementation that stopped raising but scanned tool-call sender_id values would invent "agent" as a user. CHANGELOG also records that the stale-index clobber is fully closed only for clusters at or below MAX_SKILLS_IN_PROMPT; above it LanceDB orders the markdown candidates, and the skill a lagging index omits is the one written most recently. * docs(changelog): fold the merged cascade work into the 1.2.3 entry #392 landed on main with its entries under [Unreleased] and no version bump. Since 1.2.3 ships that code, leaving them there would have the release notes disclaim work the release contains. Merged section by section into [1.2.3] and dated it to the actual release day. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audit follow-up to #384 / #386. The version-cleanup stall those fixed had a shape worth generalising: an unbounded await in a path whose scheduler treats "not finished" as "skip" — so one hang stops that work forever, and because nothing failed, every health signal built from failure counters reads as healthy.
Seven instances were found. Five are fixed here; the reasoning for what is deferred is at the bottom.
What each fix addresses
processing;/healthgreentrycreate_taskon a closing loopoptimize_failure_streak >= 5unreachable (~1% observable)None is high-probability. What they share with the fixed bug is the shape, and one of them (reads) has a strictly larger blast radius than the original — it stops all projection, not one table's cleanup.
Notable reasoning
Reads were deliberately skipped last round on the grounds that a read takes no lock and so blocks no writer. That is true and was the wrong conclusion: the drain loop reads on every batch and advances one batch at a time, so a hung read parks the pipeline even though it parks no lock.
The husk sweep's docstring contract was unsatisfiable. It required the write lock, but the sweep runs via
asyncio.to_threadand a deadline cancels the future, not the thread — so an orphan sweep outlives any critical section, andPath.iterdiris a lazyos.scandirthat can yield a dir created after the scan began. Holding the lock harder does not fix it; an age filter (skip dirs younger than lance's own 7-day unverified threshold) holds regardless of lock ownership, so that is the guarantee now.The lock wait was made pollable rather than merely bounded. Wrapping a blocking
flockin a timeout is worse than no timeout: cancelling the coroutine leaves the worker thread to acquire the lock later with nobody left to release it. ShortLOCK_NBattempts on an interval give the same semantics while being bounded and cancellable.Loop supervision escalates to process exit (
SIGTERM, so the ASGI server shuts down gracefully) after 5s/15s/45s restarts. This assumes a restarting supervisor; without one the process stops, which still beats serving searches from a silently frozen index.Verification
ruffclean;import-linter3/3 contracts kept.Deferred, with reasons
cascade_lancedb_rebuild_skipped_optimize_unfinished) but not absent. The functional fix needs the runner to yield when a rebuild is pending, which changes the optimize↔rebuild mutual-exclusion contract — both commit on the same manifest version, so getting it wrong is worse than the bug. It also cannot be validated today: the cadence is 12h and no soak run has exceeded 2h, so the periodic sweep has only ever run at boot (where there is no write load). Making the cadence configurable is a prerequisite.awatchoutside itstry) — only the optional OME subsystem's hot reload dies; running config is unaffected and a restart clears it./health— five of the seven are invisible because the verdict is assembled from failure counters, which a hang leaves untouched.last_prune_atis the one exception and is the only reason the original bug was ever found. Giving each long-lived loop a last-success clock is the structural fix; the drain half is nearly free, sincemax_lsn/last_processed_lsnare already computed and then dropped when buildingCascadeHealth.Review follow-up (adversarial pass)
Four findings from the adversarial review, fixed on this branch (each with a mutation-verified test where applicable):
prune()— the cleanup commit has already succeeded at that point; letting the best-effort sweep's deadline escape counted a prune "failure" and stalled the prune-staleness clock for a stall that did not happen. Reachable at ceiling-load steady state, where sweep time sits right at the 60s budget.🤖 Generated with Claude Code