Skip to content

Apply the memory-plan SessionBank ceiling at idle safe points - #357

Open
PhilipJohnBasile wants to merge 6 commits into
youssofal:mainfrom
PhilipJohnBasile:feat/sessionbank-memory-governor
Open

PhilipJohnBasile wants to merge 6 commits into
youssofal:mainfrom
PhilipJohnBasile:feat/sessionbank-memory-governor

Conversation

@PhilipJohnBasile

@PhilipJohnBasile PhilipJohnBasile commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

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 set active_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:

  • 581 tests passed across safe points, memory pressure/plan, all scheduler lanes, admission, SessionBank, SSD spill, no-MLX imports and server behavior.
  • The contribution-guide CLI/KPI/no-MLX selection passed all 331 tests; this overlaps the larger selection.
  • Source/wheel build and scripts/fresh_venv_smoke.sh passed.
  • Compile checks, hygiene and attribution checks passed. Ruff passes for changed files with the existing server F841 exclusion; new scheduler/test code also passes without that exclusion.
  • CI now includes the focused safe-point and existing pressure/admission regressions. Its first run exposed an old assumption that all hosts have more than 8 GiB RAM; the test now checks detected RAM against the OS total. All 41 memory-plan tests pass locally after that correction. A subsequent run exposed three existing keepalive tests that assumed sub-second scheduler deadlines; those tests now advance a controlled clock and await completion. The full focused CI selection passes all 162 tests locally after both fixture corrections.

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.

@PhilipJohnBasile
PhilipJohnBasile force-pushed the feat/sessionbank-memory-governor branch from bc6f9bc to 4adb2fa Compare September 1, 2026 13:09
@youssofal

youssofal commented Sep 6, 2026

Copy link
Copy Markdown
Owner

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.

Copy link
Copy Markdown
Contributor Author

@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.

@PhilipJohnBasile PhilipJohnBasile changed the title feat(memory): add safe-point SessionBank budget governor Apply the memory-plan SessionBank ceiling at idle safe points Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants