runtime: bound the status-watch set with a capped, expiring policy#438
Conversation
lgahdl
left a comment
lgahdl
left a comment
There was a problem hiding this comment.
Solid overall — verified the concurrency and ordering claims hold up: watch() prunes expired entries before the cap check in one critical section (no reject-while-stale-entries-sit-unevicted scenario), the cap check is strict < (no off-by-one), all mutation sites hold short non-overlapping locks never spanning an .await, and zero/unset config correctly clamps up to a sane default rather than capping at literally 0.
One real edge case worth a look, flagged here since it's on an unchanged line just outside this diff's hunks: crates/nexum-runtime/src/host/venue_registry.rs:649 (record_polled_status, the return None; inside the encode-failure branch). On a status-body encode failure, the function returns early before reaching the expires_at refresh added at line 659 — so a venue that's actively responding but whose status chronically fails to encode never gets its eviction deadline pushed out, and will eventually be silently evicted by prune_expired as if it "went silent," even though it's live. The doc comment above (line 623, "the entry is left untouched for the next cadence") reads as benign but the actual effect is the expiry clock keeps ticking down unrefreshed. Consider refreshing expires_at regardless of encode outcome, or note this caveat explicitly in the doc comment.
d6f4a3f to
72601f2
Compare
The registry's watch list grew without bound and was polled O(N) per cadence. Each watch now carries an eviction deadline and the set carries a cap, both operator-configurable via [limits.watch]; expired entries evict unpolled, at the cap a new watch is refused and logged rather than a live watch dropped, and every successful non-terminal poll pushes the deadline a full window out, so only a venue silent for the whole window expires.
399f363 to
5f235f2
Compare
|
Confirmed still valid at HEAD, not superseded. The code moved from |
lgahdl
left a comment
lgahdl
left a comment
There was a problem hiding this comment.
Re-posting as an inline comment (my earlier review body-only comment didn't render in the diff view).
| watched.remove(pos); | ||
| } else { | ||
| watched[pos].last = Some(status); | ||
| watched[pos].expires_at = Instant::now().checked_add(self.inner.watch_limit.expiry); |
There was a problem hiding this comment.
Solid overall — verified the concurrency and ordering claims hold up: watch() prunes expired entries before the cap check in one critical section, the cap check is strict < (no off-by-one), all mutation sites hold short non-overlapping locks never spanning an .await, and zero/unset config correctly clamps up to a sane default rather than capping at literally 0.
One real edge case on this refresh: a few lines up, record_polled_status's encode-failure branch (status_body(status).encode() returning Err) does return None; before reaching this expires_at refresh. So a venue that's actively responding but whose status body chronically fails to encode never gets its eviction deadline pushed out here, and will eventually be silently evicted by prune_expired as if it "went silent," even though it's live. The doc comment on this function ("the entry is left untouched for the next cadence") reads as benign but the actual effect is the expiry clock keeps ticking down unrefreshed. Consider refreshing expires_at regardless of encode outcome, or note this caveat explicitly in the doc comment.
A status watch refreshes its give-up deadline only when the venue is reachable: a status poll returned Ok, even if the body then failed to encode. A resolve failure or an errored poll now rides out against a grace window rather than burning the deadline, so a venue outage no longer silently evicts a pending intent's watch mid-recovery. grace = min(2 * expiry, 24h), overridable via [limits.watch] grace_secs. Closes #511 Closes #438
A status watch refreshes its give-up deadline only when the venue is reachable: a status poll returned Ok, even if the body then failed to encode. A resolve failure or an errored poll now rides out against a grace window rather than burning the deadline, so a venue outage no longer silently evicts a pending intent's watch mid-recovery. grace = min(2 * expiry, 24h), overridable via [limits.watch] grace_secs. Closes #511 Closes #438
|
Landed via #514 (squash-merged into dev/m1). Closing manually since the merge target is not the default branch. |
2 participants
Closes #321
What
Bounds
VenueRegistry's status-watch set with a configurable cap and per-entry expiry (WatchLimit), plumbed throughModuleLimits/[limits.watch]inengine_config.rsand wired into the registry builder from the supervisor.Why
The watch set grew unboundedly and was polled O(N) per cadence with no eviction, invisible at echo scale but unbounded on a non-instant-settling venue. A configurable cap plus expiry closes this before the CoW adapter lands, without dropping live pending watches: at the cap a new watch is refused and logged; expired entries evict first and a successful poll pushes the deadline out a full window.
Closes #321
Testing
cargo test -p nexum-runtime(venue_registry + engine_config unit tests), including cap-refusal, expiry-eviction, deadline-refresh-on-poll, and zero-config saturation cases.AI Assistance
Implemented with Claude Code.