Apply the memory-plan SessionBank ceiling at idle safe points - #357
PhilipJohnBasile wants to merge 6 commits into
Conversation
2382dfd to
8bc4d88
Compare
bc6f9bc to
4adb2fa
Compare
|
Thanks. The safe-point rule is the right property and I want it: touch the bank only after taking the model lock without blocking, and only when the foreground, scheduler, restore, commit, MTP and postcommit stages are all idle. The problem is that there is already a governor for the bank budget. It comes from the memory plan (plan-driven defaults, a dynamic ceiling with early pressure, a structured 507 when an out-of-memory is projected, SSD spill), plus the #447 and #450 admission work now in 2.11.2. Two controllers would fight over the same budget. So the plan is to fold your safe-point apply into that governor, with your name on it, rather than merge a second controller. On hold until then. Your synthetic measurement is useful either way as the ceiling on the overhead. |
|
@youssofal Agreed: the memory-plan governor should remain the single owner of the bank budget. Please fold the safe-point apply mechanism into that controller rather than introducing this as a competing governor. The invariant to preserve is nonblocking acquisition of the model lock, followed by rechecking that foreground, scheduler, restore, commit, MTP, and postcommit work are idle before any bank mutation. Sampling remains outside that lock. The existing synthetic receipt measures controller overhead only, not live pressure/OOM protection. I am leaving the branch held for your governor integration, with no second-controller changes. |
Routine SessionBank ceiling enforcement now waits for a proven idle safe point inside the existing memory-plan governor. This follows the requested integration in #357 (comment) and replaces the standalone controller in this PR.
The existing
effective_max_bytes()policy still determines the ceiling and the 256 MiB slack remains unchanged. Sampling happens before locking. Application requires nonblocking acquisition of the model lock and scheduler idle gate, followed by a fresh foreground/in-flight check; failed probes or a changed bank defer the tick. The scheduler gate excludes queued/running foreground, postcommit, persistence and keepalive work. A dequeue claim closes the interval where an item was removed from its queue but had not yet setactive_kind.The final diff has no extra controller, budget knobs, growth policy or allocator-cache clearing. Existing configured bank and per-session caps are unchanged. Deferred attempts use the existing bounded guard-event history. The old proposal and synthetic receipt remain recoverable at commit 01bd8f0; those overhead numbers do not qualify this implementation. Branch history is preserved.
There is a deliberate behavior change: routine ceiling eviction now defers through a long prefill. Admission projection, structured 507 refusal, SSD spill, allocation-failure recovery and sustained-critical abort remain in place. The separate emergency pressure responder keeps its existing semantics: WARNING has its bounded defer interval and CRITICAL still sheds immediately during foreground work. This PR does not claim that emergency shedding is idle safe-point maintenance.
Validation on the implementation tree, with the final CI-fixture corrections checked separately:
scripts/fresh_venv_smoke.shpassed.No real model, physical pressure/OOM event or throughput benchmark was run. Maintainer workflow approval and upstream review remain separate gates. Details and the long-prefill tradeoff are documented in
docs/memory-plan-safe-points.md.Final hosted qualification: all four required checks pass at a46ac9a. The CI job passes both the standard smoke selection and the added memory-plan/scheduler/admission selection: https://github.com/youssofal/MTPLX/actions/runs/34380444586. Wheel build, repository hygiene and attribution checks also pass. The PR is non-draft; maintainer scope/review approval is still required.