review-topic-workflow-state-management

# Review topic: Workflow / block state management ## When this applies Load this skill when a diff in `inference/core/workflows/core_steps/**` (or `inference/core/workflows/prototypes/block.py`) does any of: - Seeds instance state in a `WorkflowBlock.__init__` that persists across `run()` calls (`self._trackers`, `self._per_video_cache`, `self._sessions`, `self._previous_positions`, `self.cache`, any `Dict`/`deque`/`set`/`list`). - Keys state by `image.video_metadata.video_identifier`, session id, or API key; or tracks objects across frames (bytetrack, botsort, sort, ocsort under `core_steps/trackers/`). - Touches temporal-analytics blocks: `analytics/time_in_zone`, `analytics/line_counter`, `analytics/velocity`, `analytics/path_deviation`, `transformations/stabilize_detections`, `flow_control/delta_filter`. - Manages TTL / eviction / cooldown / reattach windows, or reads/writes `inference/core/cache` (`BaseCache`, `MemoryCache`) with `expire=`, or `deque(maxlen=...)` / FIFO / LRU buffers in a block or sink. - Adds or edits `get_restrictions()`, `STATEFUL_VIDEO_HTTP_SOFT_RESTRICTION`, `COOLDOWN_HTTP_SOFT_RESTRICTION`, or a `StepExecutionMode` / `NotImplementedError` remote guard. Paths are hints; the trigger is the *behaviour* — a block holding state that must outlive a single `run()` yet must not silently outlive its scope or cross a request/shard boundary. ## Review checklist ### BLOCK — must be fixed before merge - [ ] **Remote-execution contract.** A block holding cross-frame or cross-request state MUST either return a SOFT restriction from `get_restrictions()` (e.g. `STATEFUL_VIDEO_HTTP_SOFT_RESTRICTION` / `COOLDOWN_HTTP_SOFT_RESTRICTION`) OR hard-fail: `run()` raises `NotImplementedError` when `step_execution_mode is not StepExecutionMode.LOCAL`. A stateful block with **neither** produces silently-wrong tracks/counts/cooldowns on hosted-serverless (frames land on different workers). - [ ] **Keying / isolation.** Per-video/per-session state must be keyed by a real discriminator (`video_identifier`, session id, API-key hash), guarded with `setdefault(video_id, ...)` / `if video_id not in self._x`. A shared or empty-default namespace cross-contaminates tracks/counts/dwell-times and can leak one tenant's data into another's output (cross-session bleed regressed in #632). ### FLAG — reviewer should raise - [ ] **Bounded state.** Every mutable `__init__` attribute that grows per-video or per-track needs a bound: `deque(maxlen=...)`, an LRU / `WithFixedSizeCache`, capacity + FIFO/LRU eviction, or `cache.set(..., expire=...)`. A bare `Dict`/`set`/`list` keyed by `tracker_id` (unbounded — new ids arrive forever) is a memory leak on a long-lived server. Per-`video_identifier` dicts are bounded by distinct videos (softer, but still flag on a hosted process). - [ ] **Eviction correctness.** A bounded cache must evict from *all* backing structures. `InstanceCache` pops `_cache_inserts_track` (the deque) AND `.remove()`s from `_cache` (the set) — dropping one leaks the other (the model-manager analogue regressed in #1212 / #1526). - [ ] **State cleanup on invalidation.** When config the state depends on changes (e.g. model switch), stale state must be cleared, not reused against a mismatched config — e.g. `self._sessions.clear()` in the SAM2-video block. - [ ] **Cache-key TTL & namespacing.** A cache key holding a transient (last-report-time, cooldown, workspace lookup) must set `expire=` OR be namespaced by workflow/step/aggregator id so it can't grow unbounded or collide across workflows/tenants. A durable-looking `cache.set(key, value)` with no `expire=` on transient data is an unbounded key. - [ ] **New stateful transformation blocks.** Treat brand-new stateful transformation blocks with extra scrutiny — the tracklet-recognition transformation was reverted (#2527 reverting #2497). ### NIT — optional - [ ] **Window consistency.** TTL / reattach / lost-track-buffer / cooldown windows should be internally consistent — a reattach window shorter than the eviction interval silently drops tracks that should survive. - [ ] **Metadata fallbacks.** Missing `fps` / `video_identifier` should degrade gracefully (default + warn), never crash the pipeline nor collapse all streams under one empty identifier. - [ ] **Required stateful manifest fields.** Fields that seed state (zones, lines) should not carry `default=None` — a silent default yields empty state instead of a clear error (#658 removed `default=None` from line-counter / time-in-zone manifests). ### Not blocking - A block keyed only by `video_identifier` with a plain `Dict` (e.g. `delta_filter`'s `self.cache`) is acceptable for a minimal per-stream skeleton — do NOT demand a `deque`/LRU when the key space is distinct videos, not per-track ids. - Do NOT demand a hard `NotImplementedError` guard when the block already declares a SOFT restriction via `get_restrictions()` — SOFT (degraded output) and HARD (fail-closed) are alternatives, not both. - Do NOT flag missing eviction on state that is flushed each `run()` (e.g. the model-monitoring `PredictionsAggregator` resets `_raw_predictions` on `get_and_flush`). - Do NOT require Redis-offloaded state for a block explicitly scoped to `InferencePipeline` / local execution and honestly declaring its remote restriction. ## Standards - **State must declare its remote/serverless contract.** In-memory state cannot cross a request or shard boundary; on stateless / multi-replica HTTP runtimes successive frames may hit different workers, so in-memory tracking/counting/cooldown is meaningless. The block declares this via `get_restrictions()` (SOFT: degraded output, surfaces at compile) or fails closed by raising `NotImplementedError` when `step_execution_mode is not StepExecutionMode.LOCAL` (HARD: surfaces at first frame). The `RuntimeRestriction` / `Severity` / `Runtime` framework in `inference/core/workflows/prototypes/block.py` exists to make this declarable — new stateful blocks must opt in. - **State must be scoped and isolated.** Per-video/per-session state is keyed by a real discriminator and guarded before first use; nothing is shared under an anonymous/default namespace. - **State must be bounded and evicted.** A block instance can live for the whole process. Anything keyed by `tracker_id` is effectively unbounded over a long stream and needs a cap + eviction that touches every backing structure; `maxlen`/size clamped `>= 1`. - **Transient cache keys carry a TTL and a namespace.** `expire=` on transient values, namespaced keys so two workflows/tenants can't collide. - **State is cleared when its config is invalidated**, and window parameters (TTL / reattach / cooldown) are internally consistent. ## Key files (canonical patterns) - `inference/core/workflows/core_steps/trackers/_base.py` — canonical bounded per-video state. `TrackerBlockBase` keys `self._trackers` / `self._per_video_cache` by `video_id`; `InstanceCache` is a FIFO `deque(maxlen)` (`_cache_inserts_track`) + `set` (`_cache`) with correct dual-structure eviction in `_cache_new_tracker_id` and a `size = max(1, size)` clamp. New trackers subclass this instead of re-rolling state (#642, later #705). - `inference/core/workflows/core_steps/transformations/byte_tracker/v3.py` — shipped per-video tracker: `self._trackers` + `InstanceCache`, `get_restrictions()` returning `STATEFUL_VIDEO_HTTP_SOFT_RESTRICTION` + `STILL_IMAGE_INPUT_SOFT_RESTRICTION`. - `inference/core/workflows/core_steps/models/foundation/segment_anything2_video/v1.py` — the **fail-closed** reference: `run()` raises `NotImplementedError` when `self._step_execution_mode is not StepExecutionMode.LOCAL`, keys `self._sessions` by `video_id` via `setdefault`, and `self._sessions.clear()`s on model switch. Copy this for any block whose state genuinely cannot survive remote execution. - `inference/core/workflows/core_steps/analytics/time_in_zone/v3.py` (and `v2.py`) — per-video zone state keyed by `video_identifier`, requires `tracker_id` (raises if absent). The tracked-ids map is **unbounded** — cite as the shape to bound in new work, not as a bound reference. - `inference/core/workflows/core_steps/analytics/velocity/v1.py` — `self._previous_positions` / `self._smoothed_velocities` are `setdefault(video_id, {})` dicts that are **unbounded per track**; the anti-pattern new analytics code copies. Cite when reviewing a new analytics block with no bound. - `inference/core/workflows/core_steps/flow_control/delta_filter/v1.py` — smallest correct stateful skeleton: plain `self.cache` keyed by `video_identifier` + `STATEFUL_VIDEO_HTTP_SOFT_RESTRICTION`. - `inference/core/workflows/prototypes/block.py` — the contract source: `Severity`, `Runtime`, `StepExecutionMode`, `RuntimeRestriction`, and the `STATEFUL_VIDEO_HTTP_SOFT_RESTRICTION` / `COOLDOWN_HTTP_SOFT_RESTRICTION` / `STILL_IMAGE_INPUT_SOFT_RESTRICTION` presets. (`common/entities.py` re-exports `StepExecutionMode` for back-compat.) - `inference/core/workflows/core_steps/sinks/roboflow/model_monitoring_inference_aggregator/v1.py` — **mixed** example, read carefully. Good: `get_restrictions()` declares a SOFT restriction; the workspace-lookup key in `get_workspace_name` uses `cache.set(..., expire=900)` (15-min TTL) — the correct pattern for a durable/TTL'd cross-frame value. Anti-pattern: `_last_report_time_cache_key` (`workflows:steps_cache:...:{unique_aggregator_key}:last_report_time`) is written with `cache.set(key, value)` and **no `expire=`** — a per-aggregator-key cache entry with no eviction. Do NOT cite `last_report_time` as the TTL example; it is the "transient key missing `expire=`" anti-pattern this rule catches. ## Reference PRs - #642 / #705 — bounded `InstanceCache` for tracker seen-ids sets. - #658 — removed `default=None` from line-counter / time-in-zone manifests. - #632 — kept distinct exec sessions for InferencePipeline usage tracking (cross-session bleed). - #1212 / #1526 — fixed-size-cache eviction bugs in the model manager (same class as block-level FIFO). - #2527 (reverting #2497) — reverted the tracklet-recognition transformation block. - #2387 — graceful fallback on ephemeral cache failure (state must not assume the cache is durable on serverless).