feat(index): add worker resource watchdogs

[liuchong]

Stacked on #1723. A fork PR must target a branch in this repository, so the diff below also contains #1723's commit. The new work here is the last commit, ece90423. Once #1723 merges, this diff shrinks to that one commit on its own.

Related to #1347.

Problem

Discovery limits bound what indexing accepts, not what it then costs. A repository well inside those bounds can still exhaust the host through parser memory, or simply never finish. A supervised worker that hangs leaves the parent waiting with nothing to report and no way to attribute the stall.

What this changes

Two opt-in limits, enforced by the parent against the worker process tree:

Key	Meaning
index_max_rss_mb	Maximum resident memory of the worker and its descendants
index_max_duration_seconds	Maximum wall-clock duration of the attempt

Both default to off.

Measuring the tree rather than the worker process alone matters: indexing spawns children, and a runaway child hiding behind a small parent is exactly the case that goes unattributed today. Resident memory is sampled through the platform interface on macOS, Linux and Windows.

Crossing a limit terminates the process tree and produces one trusted, structured terminal result attributing the failure to the resource that caused it, so the caller learns which limit ended the attempt rather than seeing a generic worker failure.

A measurement that cannot be taken fails the attempt instead of passing it. A watchdog that quietly stops watching is worse than no watchdog, so the probe is fail-closed and says so in the result.

Testing

make -f Makefile.cbm test and make -f Makefile.cbm lint-ci on macOS. New coverage: process-tree RSS measurement, watchdog termination on each limit, probe-failure fail-closed behaviour, terminal-result attribution, and policy validation for the two new keys.

Stack

1. feat(index): add opt-in discovery resource limits #1723 — discovery limits
2. this PR — worker watchdogs
3. feat(index): enforce opt-in storage resource limits #1725 — storage limits
4. feat(index): add opt-in index resource profiles #1726 — resource profiles
5. feat(index): record and report the latest index attempt #1727 — attempt record and freshness
6. feat(mcp): warn on answers served from a failed rebuild #1728 — stale-index warning on answers

[github-actions]

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

• It will not be closed for inactivity. No stale bot touches pull requests here.
• It may still sit a while before a human reads it. That is on us, not on you.
• Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

• Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
• Get CI green, or say which failures you believe are pre-existing.
• Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
• Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

[DeusData]

Reviewed slice 2. The direction is already accepted (see #1723); this is the merit read, and the design holds up well.

The three-valued status is the right call and it is the thing most people get wrong here.

CBM_PROC_TREE_RSS_ERROR = -1,
CBM_PROC_TREE_RSS_EMPTY = 0,
CBM_PROC_TREE_RSS_OK    = 1,

"No trustworthy measurement" and "the tree has no observable members" are different facts, and collapsing them into 0 is exactly how a watchdog silently stops watching. Keeping them apart is what makes the fail-closed behaviour possible rather than aspirational — and I checked it is real: resource_probe_failed gates the success predicate and is logged through index.supervisor.resource_probe_failed, so a probe that cannot measure fails the attempt instead of waving it through.

"A watchdog that quietly stops watching is worse than no watchdog" is the right principle and you built to it.

Using a Windows Job Object is the detail I want to single out. QueryInformationJobObject with JobObjectBasicProcessIdList gives a kernel-maintained, authoritative membership list for the contained tree. The obvious alternative — enumerating descendants by walking parent PIDs — is racy by construction, and we have a live defect in this repo where exactly that style of Windows descendant probe fails closed under a tight timeout. You avoided that entire class by picking the right primitive rather than by handling its symptoms.

The error paths around it are thorough too: allocation failure, size overflow past UINT32_MAX, an unexpected GetLastError(), and a capacity guard so the ERROR_MORE_DATA retry cannot grow without bound. Each returns ERROR rather than a plausible number.

Measuring the tree rather than the root process is the correct scope, and your reason for it — "a runaway child hiding behind a small parent is exactly the case that goes unattributed today" — is the sentence that justifies the extra complexity. Ignoring processes that exit mid-enumeration is the right way to handle the inherent race.

Exposing cbm_subprocess_rss_sum_for_testing so the overflow-safe summation can be tested directly, rather than only through a live process tree, is a good seam.

Process notes

Clearance: this shows REVIEW(3) — Makefile.cbm and scripts/test-runtime.sh (both inherited from #1723's stacked commit) plus a test that spawns processes, which is unavoidable for a watchdog test. Nothing here is an exemption; it is the same shape already cleared elsewhere. I will get the marker written before merge.

Rebase when #1723 lands. This diff currently carries #1723's commit; once that merges it collapses to ece90423 alone, which will make the final read much easier. main also moved three times today, so rebase onto current main rather than anything older.

Slice 3 onward is where the configuration surface really grows, so expect those to get a closer look than this one — but this slice is in good shape.

[liuchong]

@DeusData rebased #1724 onto current main (92abefa3, which includes the #1723 merge) and stopped there. #1725–#1728 are untouched.

One conflict worth naming: main now has cbm_index_worker_job_memory_limit (the Windows job-object commit cap). This slice's RSS/duration watchdog is a separate check, so both stayed — the OS cap is still applied at spawn, and the parent still samples the process tree against index_max_rss_mb / index_max_duration_seconds. Local index_supervisor suite: 15 passed, including the job-memory floor test from main and the watchdog cases from this slice.

CI is restarting. I will wait for you to say when the next slice should move.

[liuchong]

@DeusData rebased #1724 onto current main again (def38f3e, including #2257) and stopped there. #1725–#1728 are still untouched.

The previous matrix's reds were:

• lint / lint — ours. The memory-core ratchet caught new raw malloc/realloc/free in the process-tree RSS probe and four extra free sites in the worker resource response. The probe now allocates through mem_core; the worker JSON responses share one release helper so the file does not grow past baseline.
• ci-ok / test — follow-on. test is needs: lint, so it skipped; ci-ok then failed because lint (and therefore test) did not succeed.
• memwaste event-windows / event-macos / access-linux — expected skips. Those jobs only run in mode: full; PR validation uses mode: pr (Linux event + scaling only). They are report-only and not in ci-ok.

Local index_supervisor + subprocess: 49 passed. CI is restarting.

[DeusData]

Thank you — for the two rebases, for stopping exactly where you said you would, and for two fixes I was about to ask for and found already done.

I read head 42a3d710 in full this morning and wrote three blocking findings. By the time I could post, you had fixed two of them yourself:

• The memory-core ratchet — the process-tree probe now allocates through mem_core and the worker resource responses share one release helper. lint / lint is green on 3743fe95.
• The Step 5f collision — this one was ours, not yours, and you fixed it anyway. tests/test_worker_session_scope.sh on main hard-coded a two-key policy literal while the worker demands the exact key set, so any slice that adds a policy key turns main red after a textually clean merge. That is precisely what #1723 did to us at the weekend, and this slice (2 → 4 keys) would have done it again. Your last commit routes the fixture through cbm_test_index_worker_policy_json, which is the durable fix: every later slice only has to touch the helper. For the record, I think the worker's exact-key-count check should stay as strict as it is — a supervisor that silently drops a key would silently disable a limit.

That leaves one blocker, and it is a one-word change with a long story behind it.

Blocking: macOS samples ri_resident_size; it must be ri_phys_footprint

src/foundation/subprocess.c:577 sums usage.ri_resident_size over the process group. On macOS, resident size keeps counting every page mimalloc has already handed back (MADV_FREE_REUSABLE) until the kernel gets round to reclaiming it. We measured this the hard way — it is written down in src/foundation/mem.h on main: on the Linux-kernel corpus the worker sat at 17.4 GB resident with 5.5 GB actually charged. With this slice as it stands, a user who sets index_max_rss_mb to 8 GB on a Mac can get a healthy run killed while the OS is charging it a fraction of that, with an error message naming a number the OS itself does not hold against them.

rusage_info_v2 already carries ri_phys_footprint, so the probe needs no new call — just the other field. That is also the number cbm_mem_charged() enforces the cooperative budget against, so the two limits would finally agree on what "memory" means. Linux and Windows are fine as they are. If you can, make the fake-RSS seam's test name say which quantity it represents, so nobody "fixes" it back.

Not blocking — but please read

1. The daemon's default start path has no test. application_worker_start_default (src/daemon/application.c, the policy parse plus cbm_index_worker_start_with_policy) is only reachable through the real .start; every case in tests/test_daemon_application.c installs app_fake_worker_start. So "a malformed internal policy makes the daemon refuse the job" is currently asserted by nothing. One case that feeds a bad _cbm_index_policy through the default start and expects -1 plus daemon.index.policy would close it.
2. index_max_duration_seconds is per attempt, not per request. started_ms is set when a worker handle starts, and recovery starts a new handle per attempt — so a request that crashes and recovers can run for several times the configured duration. Either is defensible; what is not is the ambiguity. Please pick one, name it in the option's help text, and pin it with a fake-clock test across a recovery. My preference is per request: a user who writes "30 minutes" means the thing they are waiting for.
3. A duration limit under 15 minutes pre-empts hang quarantine. DEFAULT_QUIET_TIMEOUT_MS is 900 000; a shorter duration limit kills a hung worker first, and because a limit kill (correctly) never enters the quarantine loop, as far as I can see the file that hangs is never identified, so the next run would hang on it again. Worth one sentence in the docs at minimum; better, log the in-flight file marker on a duration kill.
4. Windows probe, small. OpenProcess(PROCESS_QUERY_INFORMATION | PROCESS_VM_READ, …) matches the documented requirement for GetProcessMemoryInfo, so this is only a suggestion: PROCESS_QUERY_LIMITED_INFORMATION is the documented lesser alternative to PROCESS_QUERY_INFORMATION and succeeds on more kinds of process, which matters because a member you cannot open counts as a probe failure when it is the root. The Job Object membership list already closes most of the pid-reuse window between listing and opening; a comment saying so would help the next reader.

What I checked and found right

The limit kill reuses the existing graceful-then-forced tree shutdown rather than inventing one. A limit kill never enters quarantine. The daemon path and the direct path behave the same, and the client gets a structured error naming the limit, the observed value and the configured value. Sampling piggy-backs on the existing poll loops — no new sleep anywhere — and graph content is never affected by a limit, which is the line we care most about. The tests use clock and RSS seams instead of real time. The coexistence with cbm_index_worker_job_memory_limit is exactly as you describe it: OS cap at spawn, parent-side sampling on top.

One thing I have not done yet: build and run this head locally. External changes that spawn processes or edit executed scripts go through a per-head clearance step on our side first, and this head has not been through it. That is our queue, not a request to you.

When should the next slice move?

After this one merges — not before. Concretely: the ri_phys_footprint change, then a decision on point 2 (it changes an option's meaning, so it is cheaper to settle here than in #1725). Points 1, 3 and 4 can ride in this PR or a follow-up, your choice. Please keep #1725–#1728 untouched until then; each of them will need exactly one helper line for its new policy key now, instead of a main breakage — which is thanks to you.

A design question that is ours to decide, not yours to solve: whether index_max_rss_mb should also clamp the worker's cooperative memory budget, so a run stays under the cap instead of being killed at it. I will settle it and report back here.

Thank you for the care in this series. It shows in every error path.

[liuchong]

@DeusData addressed on 5033f97d (still contains current main d615088a; #1725–#1728 untouched).

Blocker — macOS ri_phys_footprint. The process-tree probe now sums usage.ri_phys_footprint instead of ri_resident_size, matching cbm_mem_charged(). The fake seam and the equality test are named charged_rss so that quantity is hard to “fix” back to resident size.

Point 2 — duration is per request. index_max_duration_seconds covers the whole index request: the clock is stamped once at the first spawn and recovery respawns reuse that origin. Help text, CONFIGURATION.md, and INDEX_RESOURCE_LIMITS.md say so. Pinned by index_supervisor_duration_spans_recovery_attempts (fake clock, origin 100, second attempt at 1101 observes 1001 against a 1000 ms limit).

Point 3. Docs now state that a duration shorter than the 15-minute quiet timeout kills a hung worker before hang quarantine can name the in-flight file.

Point 4. Comment on the Windows OpenProcess flags and that the Job Object membership list already closes most of the pid-reuse window. Flags unchanged.

Point 1 (daemon default-start malformed-policy test) left for a follow-up.

Local index_supervisor + subprocess: 50 passed, 2 Windows-only skips. CI is restarting on this head.

[DeusData]

That was fast, and it is right. I read 5033f97d against the review rather than against your summary:

• Blocker — resolved. subprocess.c sums usage.ri_phys_footprint, and the comment records why (the 17 GB resident / 5.5 GB charged case), which is what keeps it from being "fixed" back. Naming the seam and the equality test charged_rss does the same job on the test side. Thank you for taking the naming suggestion seriously — it is the cheap kind of protection that actually works.
• Point 2 — resolved, and done the thorough way. The origin is stamped once per request and threaded through both paths: index_run_supervised passes request_started_ms to the first spawn and to both recovery spawns, and the daemon keeps it on the job (request_started_ms, set once) and hands it to worker_ops.start. I was half expecting only the direct path to be covered; the daemon path is the one that usually gets forgotten. index_supervisor_duration_spans_recovery_attempts pins it with the fake clock, and the help text, CONFIGURATION.md and INDEX_RESOURCE_LIMITS.md now say "per request". One tiny thing for whenever you next touch it, not for this PR: duration_origin_ms == 0 means "now", so a fake clock that legitimately returns 0 cannot be used as an origin — worth a word in the header comment.
• Points 3 and 4 — resolved as documentation and a code comment, which is all I asked for.
• Point 1 (the daemon default-start malformed-policy test) as a follow-up is fine. Please keep it on your list; it is the only path in this slice that no test executes.

The design question I said I would settle: yes — index_max_rss_mb should also clamp the worker's cooperative memory budget, so that a run spills and stays under the cap and the watchdog becomes the backstop rather than the normal ending. But not in this PR: #1724 stays exactly the slice it is. It becomes a small follow-up (budget = min(configured budget, RSS limit minus headroom), applied where the worker budget is computed). If you would like to take it, it fits naturally after this merges and before or alongside #1725; if not, we will do it and credit the idea to this thread.

What is left is on our side: the full matrix on 5033f97d (running now), then our local build of the merge result — external changes that spawn processes go through a per-head clearance step here before we build them — then the merge decision. Nothing further is needed from you on this slice. Please keep #1725–#1728 where they are until it lands.

Thank you, again, for how you work on this series.

[liuchong]

@DeusData current head 879aa5f9 is sitting in the pr-1724 concurrency group behind superseded PR run 2592 (5033f97d). That run has one job still queued — test / shard-completeness — so the new workflow reports Waiting for PR #2592 to complete. Cancelling 2592 is a 403 from this fork identity. If you can cancel that run, this head's dco / ci-ok can start. #1725–#1728 remain untouched.

[DeusData]

Thanks for the precise diagnosis — that is exactly what is happening. I am not going to cancel run 2592, though: cancelling workflow runs is one of the few actions I keep off the table from this side, even for a superseded run, so nothing here is ever "cancelled by accident". Its one remaining job, shard-completeness, is a small aggregation step that is only waiting for a runner slot; the pool is unusually busy tonight, and it will complete on its own, after which the run for 879aa5f9 starts automatically. Nothing is needed from you, and there is no time pressure on this end: the step that gates the merge is our local build of the merge result, which happens after that fresh run in any case. Thank you for keeping #1725–#1728 parked.

[DeusData]

Same request as I have just made on another PR, and for the same reason: please do not rebase again until you hear from me. Each rebase onto the newest main cancels the run in progress and restarts the whole matrix; that is exactly the "waiting for the superseded run" you diagnosed, but self-inflicted — the superseded run was itself the previous rebase. main is moving quickly tonight because several long-parked PRs are being merged, and none of them touches what this slice needs; a two-hour-old main is fine for the CI verdict, and the merge result is rebuilt locally on the real main before the merge in any case.

The content of 89030aac is unchanged from 5033f97d, which I have already verified in code. What remains is on our side and does not depend on your head moving.

[liuchong]

Rebased onto current main (e783f73d) because CI failed: test / test-msan was red on b47ba846, so ci-ok could not pass. dco was already green. New head is 3889b475. #1725–#1728 stay parked.

[liuchong]

Follow-up on 08d5e206: the previous test / test-msan failure was daemon_bootstrap_fails_fast_when_daemon_dies_at_publication (ASSERT(daemon_named_cause)), not an uninitialized-value report. index_supervisor on that run was green. The client startup budget for that test is now 180s under CBM_SANITIZED, same pattern as the frontend announce backstop. #1725–#1728 stay parked.

[liuchong]

@DeusData dco and ci-ok are green on 08d5e206, which is on current main (e783f73d). The review content you already verified on 5033f97d is unchanged (phys_footprint, per-request duration, docs). The only extra commit is the MSan-only startup budget for daemon_bootstrap_fails_fast_when_daemon_dies_at_publication, which is what made test-msan fail last time.

If you have a moment for the local clearance build, that would keep this slice from picking up more main drift. I will not rebase unless you ask. #1725–#1728 stay parked.