[Router] fix(looper): lifecycle-own workflow state and make resume semantics real #2522
[Router] fix(looper): lifecycle-own workflow state and make resume semantics real #2522pranavthakur0-0 wants to merge 3 commits into
Conversation
✅ Deploy Preview for vllm-semantic-router ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
Bot
assigned
AayushSaini101,
drivebyer,
FAUST-BENCHOU,
ramkrishs,
shraderdm and
WUKUNTAI-0211
👥 vLLM Semantic Team NotificationThe following members have been identified for the changed files in this PR and have been automatically assigned when their GitHub accounts are assignable in this repository: 📁
|
pranavthakur0-0
changed the title
✅ Supply Chain Security Report — All Clear
Scanned at |
…nd background cleanup for workflow state stores Signed-off-by: pranavthakur0-0 <pranavthakurdew@gmail.com> Signed-off-by: pranavthakur0-0 <pranavthakurdew@gmail.com>
b39c6cd to
41c62eb
Compare
Xunzhuo
left a comment
Xunzhuo
left a comment
There was a problem hiding this comment.
Thanks for moving workflow pause/resume state behind a shared store. The core direction is useful, but the current memory bound and router/store lifecycle are not safe enough to meet #2471. Please address the inline blockers. Two additional ownership points should be handled in the same pass: buildRouterComponents currently starts a file-store sweeper even when no workflow path is enabled, and the public/fallback NewWorkflowsLooper path creates a per-instance sweeper without an owner-visible Close. CI is green and git diff --check is clean; the local race test could not link without the native Candle/ML libraries.
| defer s.mu.Unlock() | ||
| s.cleanupLocked(time.Now().UTC()) | ||
| normalizeWorkflowToolStateForStore(state) | ||
| if len(s.states) >= maxMemoryStateEntries { |
There was a problem hiding this comment.
[P1] Enforce a store-wide byte budget, not only a per-entry/cardinality cap. At 10,000 entries of up to 512 KiB each, this store admits about 4.88 GiB of serialized state before Go object overhead, so a client can still OOM the router while staying within both limits. Please account aggregate serialized bytes on insert/replacement and decrement them on Take, expiry, and Clear, with boundary/concurrency tests.
| if r.lookupTableCancel != nil { | ||
| r.lookupTableCancel() | ||
| } | ||
| if r.WorkflowStateService != nil { |
There was a problem hiding this comment.
[P1] Closing the store here is not sufficient without draining users of this router generation. RouterService.Process loads the old pointer without a lease, while reload swaps and immediately closes it: an in-flight Redis-backed stream can then use a closed client, and a memory/file pause can publish an ID into a retired store that the next request cannot resume from. Please add generation leasing/drain (or keep compatible state-service ownership stable across reloads), close the current router during Server.Stop, and test in-flight pause/resume across reload and shutdown under -race.
| }, | ||
| }, | ||
| // Redis is excluded from unit tests because it requires an | ||
| // external server. The Redis backend shares the same interface |
There was a problem hiding this comment.
[P1] This E2E coverage claim is currently false: there is no registered workflow-state Redis testcase/profile, and these unit tests exercise one store instance directly rather than two independent router requests. Please add the Redis integration plus router-level pause/resume coverage required by #2471, including Redis TTL/pool behavior and reload/shutdown ownership, before treating the backend contract as covered.
8 participants
Closes #2471
Purpose
Moves the workflow state store to the router lifecycle so it's shared across requests instead of being created per-request. Also adds a 512KB payload limit, a 10K item memory limit, and background cleanup goroutines for the state stores.
Redis clients were leaking because they were created per-request without being closed. Also, memory states couldn't actually resume across different HTTP turns before this. The caps prevent memory blowouts.
Router/CLI/Dashboard/Operator/Fleet-Sim/Bindings/Training/E2E/Docs/CI/BuildRouter
Test Plan
make agent-ci-gate CHANGED_FILES="..."(runs the new pause/resume and concurrent take tests in workflows_state_store_test.go).The tests spin up 20 goroutines to prove we don't double-consume state, and verify that oversized payloads are actually rejected.
Test Result
Tests passed. The CI gate ran cleanly and verified the exact-once extraction.
We capped the payload size, but we haven't capped the actual
breadth_scheduleormodelRefscount (mentioned in security: cap Looper fan-out and total upstream-call amplification #1456). That needs a separate config fix later.Semantic Router PR Checklist
[Router],[CLI],[Dashboard],[Operator],[Fleet-Sim],[Bindings],[Training],[E2E],[Docs], or[CI/Build]git commit -s