Skip to content

Fix snapshot bootstrap for short generations - #95

Draft
Clarit-AI wants to merge 2 commits into
mainfrom
ops/fix-short-generation-snapshot-bootstrap
Draft

Fix snapshot bootstrap for short generations#95
Clarit-AI wants to merge 2 commits into
mainfrom
ops/fix-short-generation-snapshot-bootstrap

Conversation

@Clarit-AI

@Clarit-AI Clarit-AI commented May 25, 2026

Copy link
Copy Markdown
Owner

Summary

  • snapshot requests that finish during prefill before releasing request/KV pool slots
  • reuse the finished-request snapshot path for decode completions
  • skip generic post-forward snapshot hooks once request pool state has already been released

Verification

  • PYTHONPATH=/workspace/engram-remote-elastic-30b/python python -m py_compile /workspace/engram-remote-elastic-30b/python/sglang/srt/managers/scheduler_components/batch_result_processor.py /workspace/engram-remote-elastic-30b/python/sglang/srt/managers/scheduler.py /workspace/engram-remote-elastic-30b/python/sglang/srt/utils/auth.py
  • PYTHONPATH=/workspace/engram-remote-elastic-30b/python python -m pytest /workspace/engram-remote-elastic-30b/test/registered/unit/utils/test_auth.py -q
  • live short-generation probe: max_tokens: 1 returned Thor; /save_snapshot returned success:true; /list_snapshots showed turn 1 under short-bootstrap-040001

Note

Temporary auth loosening for the demo instance is intentionally not included in this PR.


CI States

Latest PR Test (Base): ❌ Run #26386031478
Latest PR Test (Extra): ❌ Run #26386030063

@Clarit-AI

Copy link
Copy Markdown
Owner Author

HOLD — do not merge. This PR (snapshot bootstrap fix for short generations) is merge-gated pending reconciliation with the KHA-398 cold-tier restore work, which lives in the same scheduler snapshot/restore/prefix path. Merging blind risks conflicting with or silently breaking that work. Convert to draft until the cross-compare is done. The temporary tokenize admin-key auth loosening is NOT in this PR and must never be merged.

@Clarit-AI
Clarit-AI marked this pull request as draft May 25, 2026 04:12
@kilo-code-bot

kilo-code-bot Bot commented May 25, 2026

Copy link
Copy Markdown

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 0

Review Notes

The PR correctly fixes snapshot bootstrap for short generations by ensuring Mamba state is persisted before request pool slots are released:

  • batch_result_processor.py: New _snapshot_finished_req_before_release() method extracts the snapshot logic and adds a req_pool_idx is None guard to skip if already released. Called at all three finish paths: prefill (line 271), embedding (line 356), and decode (line 826 via the refactored call).

  • scheduler.py: The _trigger_snapshot_hooks() guard at line 3190 is made consistent with the new method's check.

Fork-specific concerns (Engram):

  • The --- BEGIN/END ENGRAM block at lines 821-827 is correctly balanced and marks the refactored fork edit.
  • This change touches the snapshot/restore surface but fixes the bug where restored state was being silently ignored for short generations — no regression risk, only improvement.
Files Reviewed (2 files)
  • python/sglang/srt/managers/scheduler.py - 1 issue
  • python/sglang/srt/managers/scheduler_components/batch_result_processor.py - 1 issue

Reviewed by minimax-m2.7 · 243,189 tokens

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.

1 participant