[Router] vectorstore: add lifecycle root context and bounded Stop(ctx)#2517
[Router] vectorstore: add lifecycle root context and bounded Stop(ctx)#2517Peterren wants to merge 4 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: 📁
|
✅ Supply Chain Security Report — All Clear
Scanned at |
|
Good catch — you're right, 30s was a made-up constant. I've made it a config param. To be precise about what it controls: it's not a per-file ingest budget (a single job can take much longer or shorter). It's the graceful-shutdown drain window — on shutdown the pipeline stops accepting new jobs, waits up to this long for currently-running jobs to finish, then cancels the rest so a wedged embedder/backend can't block process exit. That means the right value is deployment-specific, bounded on both sides:
Now exposed as |
Xunzhuo
left a comment
Xunzhuo
left a comment
There was a problem hiding this comment.
A lifecycle root and deadline-aware Stop(ctx) are the right foundation, but the current implementation still has work outside the deadline and leaves ownership races after a timeout. Please address the four inline lifecycle blockers. Also keep the new config contract in sync with the dashboard/config surface, and either set the default below the Kubernetes termination grace period or configure a larger pod grace period; both are currently 30s, leaving no cleanup margin. CI is otherwise green, but check-linked-issue is failing and the branch is behind main. make agent-report and git diff --check passed; local race tests were blocked by unavailable native Candle dylibs.
|
Thanks for the thorough review — all four lifecycle blockers are addressed in d7b28da, replies inline. Summary:
Also: synced the dashboard config surface with New regression tests cover timeout-then-restart, second-Stop honesty, and attach racing a restart; |
d7b28da to
bbd5823
Compare
|
@Xunzhuo this is ready for another look. Since your review:
Rebased onto latest |
|
Pushed 6202207 to fix a small classification gap this PR introduces.
The commit extracts a Test: a new |
The ingestion pipeline processed each job with a hardcoded context.Background() and shut down via an unbounded wg.Wait(). If an embedder or backend wedged inside a stage, Stop() blocked forever and the process could not be shut down. Introduce a pipeline lifecycle root context created in Start() and cancelled on shutdown. processJob now derives its work from that context and checks cancellation between stages (read, chunk, per-chunk embed, insert), so a cancelled lifecycle aborts in-flight jobs at the next checkpoint instead of running to completion. Stop() becomes Stop(ctx) error: it stops accepting jobs, fails queued jobs, and drains in-flight work bounded by ctx. If ctx elapses before the drain finishes, the root context is cancelled and Stop returns ctx.Err() without blocking further. A nil ctx uses a default 30s bound. The runtime Shutdown path now bounds pipeline drain with a 30s deadline so a wedged backend cannot hang process shutdown. Add lifecycle and integration tests covering: bounded Stop returning within its deadline when a job is wedged, Stop idempotency, graceful drain with no leaked worker goroutines, and post-shutdown searchability. Part of vllm-project#2474 Signed-off-by: Yincheng Ren <27608815+Peterren@users.noreply.github.com>
Address review: the shutdown drain bound was a hardcoded 30s. It is not a per-file ingest budget but a graceful-shutdown grace window (stop accepting jobs, wait for in-flight jobs to drain, then cancel), so it must be sized per deployment — above typical single-job latency but below the platform's shutdown grace period (e.g. Kubernetes terminationGracePeriodSeconds). Bubble it up as vector_store.ingestion_drain_timeout_seconds (default 30), sourced by VectorStoreRuntime; keep a bounded default fallback so Stop is never unbounded when constructed without config. Add the key to the reference config, Helm values, and the RAG e2e profile. Also check the error return of Stop(ctx) at all test call sites (errcheck), which the pre-commit go-lint hook flagged after Stop's signature changed. Part of vllm-project#2474 Signed-off-by: Yincheng Ren <27608815+Peterren@users.noreply.github.com>
processJob exceeded the agent lint funlen limit (41 > 40 statements). Extract the per-chunk embedding loop (Step 5) into embedChunks, which preserves the same per-chunk ctx cancellation checks and error handling and returns ok=false to signal the caller to stop. No behavior change. Signed-off-by: Yincheng Ren <27608815+Peterren@users.noreply.github.com>
…start Address review feedback on the bounded Stop(ctx) work. The prior version still had unbounded I/O outside the deadline and left ownership races after a timed-out stop. - Per-generation resources: each Start creates a generation with its own jobQueue, stopCh, WaitGroup, and root context. A timed-out Stop can never race a subsequent Start into reusing a live generation's WaitGroup/queue, and an old worker can no longer steal a new-generation job. - Tri-state lifecycle (stopped/running/stopping): after a timed-out drain the generation stays stopping; a second Stop waits on the same generation instead of falsely returning nil, so callers can retry the join. - failQueuedJobs is bounded by the Stop ctx and no longer runs unbounded persistence before the drain wait; it always sets the in-memory failed status so no queued job is left stuck in_progress. - AttachFile no longer holds the lifecycle lock across registry I/O: the count is reserved with a bounded context before the brief enqueue critical section, with a compensation path when the pipeline is not running. - Start no longer holds the lifecycle lock while waiting for a stopping generation to join, so a concurrent attach/stop is never blocked behind a restart parked on a wedged generation. - processJob commits the completed status and count coherently with a bounded, cancellation-detached context and surfaces persistence failures. - Shutdown only closes the registry and backend once workers are quiescent (Stop returned nil); on timeout it leaves them open to avoid use-after-close and returns the error. - Lower the default ingestion drain timeout from 30s to 25s so it sits below the 30s Kubernetes default grace period, leaving cleanup margin. Sync the dashboard config surface and helm/config defaults. Adds regression tests for timeout-then-restart, second-Stop honesty, and attach racing a restart. go test -race is clean for pkg/vectorstore, pkg/routerruntime, and pkg/config. Signed-off-by: Yincheng Ren <27608815+Peterren@users.noreply.github.com>
6202207 to
f84a01f
Compare
|
Quick status: this now has approvals from @drivebyer and @FAUST-BENCHOU, CI is green ( @Xunzhuo whenever you get a chance — since all five of your lifecycle P1s are fixed and answered inline, could you re-review here so your earlier |
8 participants
Purpose
Stop()with a boundedStop(ctx) error.processJobruns with a hardcodedcontext.Background()andStop()waits on an unboundedwg.Wait(). If an embedder or backend wedges inside a stage,Stop()blocks forever and the process cannot shut down. This is the first, foundational slice of the larger vector-store hardening effort (vectorstore: make ingestion cancellable, bounded, and transactional #2474): make ingestion cancellable and shutdown bounded.Router(plusE2E-style integration tests within thevectorstorepackage).What changed
IngestionPipelinegains a lifecycle root context created inStart()and cancelled on shutdown. Workers andprocessJobderive their work from it.processJobchecksctx.Err()between stages (read → chunk → per-chunk embed → insert), so a cancelled lifecycle aborts in-flight jobs at the next checkpoint instead of running to completion. Cancelled jobs are marked failed with acancellederror code; count/metadata updates usecontext.WithoutCancelso status stays consistent even under cancellation.Stop()→Stop(ctx context.Context) error: stops accepting jobs, fails queued jobs, then drains in-flight work bounded byctx. Ifctxelapses first, the root context is cancelled andStopreturnsctx.Err()without blocking further. Anilctx uses a default 30s bound.Stopremains idempotent.VectorStoreRuntime.Shutdown()now bounds pipeline drain with a 30s deadline and logs if the drain doesn't complete, so a wedged backend cannot hang process shutdown.This is intentionally scoped as a standalone, foundational slice. It changes the
Stop()signature toStop(ctx context.Context) error(a source-level change for direct callers, all of which are updated here). Making individual stages (embedder/backend) natively ctx-aware is a deliberate follow-up;Stop's own contract (return withinctx) holds regardless.Test Plan
New tests:
pipeline_lifecycle_test.go): with a wedged embedder,Stop(ctx)returnscontext.DeadlineExceededpromptly (asserted< 3s) rather than hanging;Stopis idempotent and a second call returnsnil.integration_test.go): a full ingest-to-search run, thenStop(context.Background())returnsnil, worker goroutines are reclaimed (goroutine count returns to baseline), and completed vectors remain searchable after shutdown.integration_test.go):Stop(ctx)honors the deadline while a worker is parked insideEmbed, and after the embedder is released no goroutines remain.This validates the two behaviors the change targets — cancellability and bounded shutdown — under both the graceful and the wedged path, with
-raceand goroutine-leak assertions.Test Result
go test -race ./pkg/vectorstore ./pkg/routerruntimepasses; race detector clean.-count=1x3) with no flakiness.[Router]git commit -sCloses #2602
Part of the umbrella #2474.