Skip to content

fix(run): bound catalog watches to declaration space - #333

Merged
schickling-assistant merged 3 commits into
mainfrom
schickling-assistant/issue314-1-supervisor-watch
Aug 25, 2026
Merged

fix(run): bound catalog watches to declaration space#333
schickling-assistant merged 3 commits into
mainfrom
schickling-assistant/issue314-1-supervisor-watch

Conversation

@schickling-assistant

@schickling-assistant schickling-assistant commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Problem

The supervisor registers one recursive notify watch over the entire catalog root before its first reconcile. On Linux, notify eagerly walks the full tree and allocates one inotify watch per directory before any callback filtering runs, and follows symlinks by default. Catalog Resource payload trees (a production deployment measured 37 GB / 1.74 M entries / 151 k directories) therefore dominated startup: ~13 minutes of traversal before the first reconcile, a multi-GiB memory peak, and installation failure once kernel limits were hit.

When installation failed, the dropped channel sender made RecvTimeoutError::Disconnected look like an immediate wake in the wait loop (run.rs), turning the nominal 30 s timer into a tight full-catalog reconcile loop: ~48 passes/min at ~70 % of one core, with per-pass git subprocess churn. The failure itself was discarded behind an Option, so timer-only fallback was silent.

Goal

Watcher installation scales with declaration space, not Resource payloads; declaration mutations still wake reconciliation immediately; failures are visible; the timer fallback honors its interval; stop stays responsive.

Decisions

  • Shallow per-directory watches over the declaration namespace instead of one recursive registration: recursion is gated by agent_spec::is_catalog_path, so resources//archive/inbox/.git/.st2 prune discovery, and RecursiveMode::NonRecursive never traverses payloads. A refresh() after each pass picks up newly created directories without backend traversal.
  • Directory topology mutations wake even though no agent.kdl exists yet — the created dir may receive one next, and refresh needs to run.
  • Deadline-based wait: disconnection sleeps the remaining slice instead of waking; change/timer/stop are distinct outcomes.
  • Injectable watcher factory on the supervisor loop so tests can drive the real loop under installation failure (prior art fix(run): bound catalog watches to declaration space #316 tested only the helper).
  • Ported the sound core of the closed fix(run): bound catalog watches to declaration space #316 branch; this PR adds what it lacked (see Verification).

Verification

  • New e2e tests drive up_loop_until: injected watcher failure stays on timer cadence (~4 passes in 350 ms at a 100 ms interval; the old code produced hundreds), a live watcher wakes on declaration mutation long before the timer, and a disconnected channel waits out the interval.
  • Watch-boundary unit test: symlinked external worktree is not entered; payload depth adds zero watches.
  • Live reproduction against a synthetic oversized catalog (1539 payload directories plus a symlinked external tree), fixed binary:
    • inotify watches installed: 11 (declaration dirs only; recursive registration would allocate 1500+)
    • first reconcile: immediate
    • idle CPU over a 4 s window: 3 ticks of 400 (~0.75 %)
    • SIGINT stops cleanly mid-wait

Complexity

CatalogDeclarationWatcher is a small BTreeSet-diff over declaration dirs; it replaces rather than adds machinery. No new dependencies.

Concerns

  • Watch count now scales with agent count (one watch per declaration-space directory); bounded by fleet size, not payload size.
  • refresh() walks declaration space each pass; pruned at payload boundaries, so cost tracks agents, not data.

Follow-ups

References

Closes #314. Supersedes the unmerged #316 approach with the missing supervisor-level proof. Related: #328.

Posted on behalf of @schickling
field value
agent_identity unknown
agent_persona generalist
agent_supervisor unavailable
agent_tool OMP
agent_tool_version 18.0.3
agent_runtime OMP 18.0.3
tooling_profile dotfiles@f33cd9c-dirty

The supervisor registered one recursive inotify watch over the whole
catalog root before its first reconcile. notify eagerly walks the entire
tree and allocates one watch per directory BEFORE any callback filtering,
and follows symlinks by default, so Resource payload trees (37GB /
151k directories on dev3) dominated startup and could exhaust kernel
limits. When installation then failed, the dropped channel sender made
RecvTimeoutError::Disconnected look like an immediate wake, turning the
nominal 30s timer into a tight full-reconcile loop (~48 passes/min,
~70% of a core, measured live).

- Replace the recursive registration with CatalogDeclarationWatcher: one
  non-recursive watch per declaration-space directory, discovered without
  descending into Resource payloads (gated by is_catalog_path), refreshed
  after each pass so new directories become watched.
- Treat directory topology mutations as wakes so create/rename/remove
  still reconcile immediately; file mutations filter through
  is_declaration_path as before.
- Diagnose watcher installation failure once instead of discarding the
  error behind Option.
- Wait on an absolute deadline where Disconnection sleeps the slice
  instead of waking, so the timer fallback actually honors the interval
  while stop stays responsive in bounded slices.
- Drive the real supervisor loop in tests with an injectable watcher
  factory: installation failure must stay on timer cadence (the spin was
  only covered at helper level before), and a live watcher must wake on
  declaration mutation long before the timer.

Closes #314, closes #328.

Verified against a synthetic oversized catalog (2051 payload dirs plus a
symlinked external tree): 7 inotify watches installed, first reconcile
immediate, zero CPU ticks across a 3s idle window, SIGINT responsive.

Co-authored-by: schickling-assistant <schickling-assistant@users.noreply.github.com>
agent-identity: unknown
agent-persona: generalist
agent-supervisor: unavailable
agent-tool: OMP
agent-tool-version: 18.0.3
agent-runtime: OMP 18.0.3
tooling-profile: dotfiles@f33cd9c-dirty
@schickling-assistant

Copy link
Copy Markdown
Contributor Author

@codex Please review this PR.

Posted on behalf of @schickling
field value
agent_identity unknown
agent_persona generalist
agent_supervisor unavailable
agent_tool OMP
agent_tool_version 18.0.3
agent_runtime OMP 18.0.3
tooling_profile dotfiles@f33cd9c-dirty

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dd0fcc7bee

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/watch.rs Outdated
Comment thread src/watch.rs Outdated
schickling-assistant and others added 2 commits August 25, 2026 01:01
Review follow-ups on the declaration-space watcher:

- Topology wakes matched every create/remove/rename in declaration space,
  so a scratch, log, or editor-swap FILE could wake a full-catalog
  reconcile — the churn this watcher exists to prevent. Classify by entry
  type where the backend provides it (Linux does), check the live tree
  for untyped rename arrivals, and leave rename-away silent (an
  in-catalog rename emits the arrived side; a full removal is bounded by
  the timer plus refresh).
- An inotify watch dies with its inode, but `watched` tracked names only:
  a directory deleted and recreated at the same pathname was never
  resubscribed until the timer. Track (dev, ino) identity per watched
  directory and force re-registration when a replacement differs.

Co-authored-by: schickling-assistant <schickling-assistant@users.noreply.github.com>
agent-identity: unknown
agent-persona: generalist
agent-supervisor: unavailable
agent-tool: OMP
agent-tool-version: 18.0.3
agent-runtime: OMP 18.0.3
tooling-profile: dotfiles@f33cd9c-dirty
Identity comparison alone cannot catch every replacement: deleting and
recreating a directory at the same pathname can reuse the old (dev, ino),
so refresh would see no change while the backend watch had died with the
original inode. Share the tracked-directory map with the notify callback
and drop entries the moment the backend reports their directory removed
or renamed away (including descendants), making resubscription at the
next refresh deterministic instead of trusting a stat race.

agent-identity: unknown
agent-persona: generalist
agent-supervisor: unavailable
agent-tool: OMP
agent-tool-version: 18.0.3
agent-runtime: OMP 18.0.3
tooling-profile: dotfiles@f33cd9c-dirty
@schickling-assistant
schickling-assistant merged commit b9e9d20 into main Aug 25, 2026
1 check passed
@schickling-assistant
schickling-assistant deleted the schickling-assistant/issue314-1-supervisor-watch branch August 25, 2026 07:33
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.

Recursive catalog watcher traverses Resource payloads before first reconcile

1 participant