fix(cascade): keep the rebuild clock across restarts

[gloryfromca]

What is wrong

_rebuild_loop sweeps every kind on worker start, unconditionally. So restart frequency is rebuild frequency: a daemon that restarts hourly does a full rebuild_indexes per kind per hour — O(rows × indexed columns) of index work the configured 12h cadence never asked for, competing with the requests the freshly started process exists to serve.

Why not just delete the startup sweep

It exists for the opposite failure, and that one is real: a daemon that keeps restarting never lives long enough to reach the periodic sweep, and that sweep is the only lever bounding active index UUID growth. Remove it and exactly the restart-heavy deployments it was written for get nothing.

Neither "always sweep" nor "never sweep" is right. The bug is that the clock restarts with the process.

The change

Persist the last successful rebuild per kind in <index_dir>/.index_rebuild_state; the startup sweep skips kinds that are not due.

• Wall clock, not time.monotonic() — the span crosses process lifetimes.
• Missing, unreadable or malformed state reads as due. A lost file costs one extra rebuild and can never skip one.
• The skipped age is credited to the periodic timer, so a skip defers the next sweep instead of extending the interval beyond its configured value.
• Only a successful rebuild advances the clock.
• memory_root is an optional keyword-only argument; a worker built without one behaves exactly as before.

What adversarial review found, and what changed

Two defects in the first commit, both fixed in 46b45b7:

1. A corrupt byte crashlooped the server. UnicodeDecodeError is a ValueError, not an OSError, and the read is the first statement of _rebuild_loop — which _supervise escalates to a process exit. The file reproduces on the next boot. Nothing read that file before this feature added it, so the crash vector was this change's own. _record_rebuild had the same hole while its docstring claimed it could not raise.
2. The skip restarted the periodic timer. last_sweep was set unconditionally after the sweep, so a restart at 0.95× the interval gave a 1.95× worst-case gap — the silent skip this PR claims is impossible, on the mechanism it calls the only bound on index growth.

And one overclaim, corrected: the original motivation said "drop + create" and "GiB-scale native allocation". Neither holds. rebuild_indexes has replaced in place since #392, and the only measured figure in this repo is a 3–6 MB resident steady state (LanceSettings.index_cache_size_bytes). LanceRepoBase.rebuild_indexes states outright that rebuild frequency is bounded by cost, not correctness. The docstring and this description are now written around the per-restart work, which is what the evidence supports.

Open, and worth saying plainly: no GiB-scale memory measurement has been reproduced here. If the real cost is closer to 3–6 MB than to GiB, this change is worth less than its first description implied, and the periodic sweep's existing 12h default may already be fine. The two defects above were worth finding either way.

Verification

Eleven tests. Four mutations, each applied alone and each caught:

Sabotage	Caught by
except OSError instead of except (OSError, ValueError)	test_corrupt_state_file_does_not_take_the_loop_down
last_sweep = time.monotonic() without the credit	test_a_skipped_kind_does_not_restart_the_periodic_timer
stamp time.monotonic() instead of wall clock	test_clock_round_trips_between_two_workers, test_recorded_stamp_is_wall_clock
stamp every kind instead of one	test_one_kind_does_not_stamp_the_others

The first three of those sabotages passed the original six tests, which is why they are here.

Correcting the earlier Verification claim in this description: it said the two load-bearing tests "were confirmed to fail against the unchanged code: 2 failed, 51 passed". That number came from a targeted revert — the skip branch and the _record_rebuild call, with the memory_root plumbing kept — not from origin/main's files, which fail five tests with TypeError on the new keyword argument. The conclusion held; the wording did not describe what was run.

272 passed across tests/unit/test_memory/test_cascade; make lint, lint-imports (4 contracts), make check-datetime, make check-commits all clean.

Docs

.index_rebuild_state is documented in docs/cascade_runbook.md beside .index_reconcile_version, including the escape hatch — deleting the file, or one kind= line, sweeps those kinds again. That matters because "restart the server" used to be how an operator forced a rebuild, and this change deliberately stops that.

Not in scope

optimize_rebuild_interval_seconds and the periodic sweep are untouched; the periodic rebuild stays on at its 12h default.

[gloryfromca]

Closing this. The benefit is smaller and less certain than the description claimed, and the change introduced more risk than it removed. Recording the measurements here so a future attempt starts from data rather than from this PR's prose.

Why the benefit is uncertain

The motivation was inherited from #491, which asserts "GiB-scale native allocations" for a startup rebuild three times and measures it zero times. Measured here instead, on a real index with the FTS and vector indexes rebuild_indexes() touches, with the build and the rebuild in separate processes so the peak belongs to the rebuild and not to the fixture:

rows	rebuild	RSS before	peak	delta
20,000	0.04 s	177 MB	270 MB	+93 MB
100,000	0.14 s	182 MB	389 MB	+207 MB
300,000	0.53 s	185 MB	496 MB	+312 MB

The cost is real and it does scale — roughly 77 MB + 0.78 MB per 1000 rows — so the direction of #491's claim was right. But "several GiB" needs on the order of 4 million rows, about 1000x the corpus a single-user store reaches. At 300k rows a restart costs 0.5 s and ~312 MB transient, which is not nothing and is also not worth the surface this PR added.

Worth noting in passing: rebuild is faster than the comment this PR deleted claimed. That comment said ~0.3 s per 50k rows, predicting 1.8 s at 300k; the measurement is 0.53 s.

Also correcting myself: the description cited the repo's "3–6 MB resident steady state" as the only measured figure. That number is the index cache working set, and using it to imply rebuild is cheap was wrong — rebuild's transient is an order of magnitude larger, as above.

Why the risk was not worth it

Adversarial review of the first commit found two defects, both introduced by this change:

1. A single corrupt byte in the new state file crashlooped the server. UnicodeDecodeError is a ValueError, not an OSError, and the read is the first statement of _rebuild_loop, which _supervise escalates to a process exit. The file reproduces on the next boot. Under a supervisor that is an endless restart loop; without one the server simply dies a minute after launch. Nothing read that file before this feature added it, so the whole failure mode was this change's own.
2. A skip restarted the periodic timer, making the worst-case gap between rebuilds 1.95x the configured interval — a silent deferral of the only mechanism bounding index UUID growth, which is exactly what this PR's own docstring promised could not happen.

Both were fixed in 46b45b7, and three of the original six tests turned out to pin nothing (a sabotage stamping time.monotonic() instead of wall clock disabled the feature entirely and kept all six green). That ratio — two self-inflicted defects and five added tests, against a 0.5 s / 312 MB saving at 300k rows — is the argument for closing rather than merging.

If someone picks this up again

The underlying observation still stands: restart frequency currently sets rebuild frequency, because the startup sweep is unconditional and nothing remembers when the last rebuild happened. A deployment that restarts often pays O(rows) index work every time.

What a new attempt should bring that this one did not:

• A repro with the data volume stated, on the machine shape that hurts — the numbers above suggest the interesting regime starts somewhere past a million rows.
• The persisted clock credited to the periodic timer, so a skip defers the next sweep rather than extending the interval.
• Every read of the persisted state failing closed toward "due", including decode errors.
• A test that round-trips write→read across two worker instances; without one, the wall-clock semantics are untested.
• The marker documented in docs/cascade_runbook.md beside .index_reconcile_version, with its escape hatch — "restart the server" is currently how an operator forces a sweep, and a clock silently removes that.

The branch stays at 6ad0dbd if any of it is useful.