runtime_context: per-role namespace enforcement behind SGLANG_ROLE_NAMESPACES - #33172
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a988aa03be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| global _RECORD_DUMP_REGISTERED | ||
| if (role, name) in _RECORDED_NS_READS: | ||
| return | ||
| _RECORDED_NS_READS.add((role, name)) |
There was a problem hiding this comment.
When SGLANG_ROLE_NAMESPACES=record is combined with --enable-torch-compile, namespace access during model tracing reaches this mutation of a module-global set, followed on the first read by optional file I/O and atexit registration. Those Python side effects are incompatible with Dynamo full-graph capture; the piecewise compilation path passes fullgraph=True by default in tc_piecewise_cuda_graph_backend.py:133-146, so audit mode can prevent the model from compiling instead of recording its reads. Move recording outside the traced accessor or otherwise make the record path graph-safe.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — recording is now pruned under tracing via torch.compiler.is_compiling() (lazy sys.modules lookup keeps the module import-light), and a fullgraph regression test compiles a bag read in record mode.
| allowed = ROLE_NAMESPACE_SETS.get(role) | ||
| if allowed is not None and name not in allowed: |
There was a problem hiding this comment.
Under enforce, publish(..., role="new_or_misspelled_role") makes dict.get() return None, which is indistinguishable here from an explicitly declared unrestricted role. Consequently, adding or mistyping a process role silently grants access to every namespace, contrary to the advertised fail-closed behavior; use a distinct missing sentinel or validate the role at publication.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — under enforce, a role missing from ROLE_NAMESPACE_SETS is rejected at publish (and defensively at read time) instead of silently inheriting the full tree; covered by a test.
| "weight_cache_daemon": None, | ||
| } | ||
|
|
||
| _ROLE_NS_MODE = os.environ.get("SGLANG_ROLE_NAMESPACES", "off") |
There was a problem hiding this comment.
Any value other than the exact strings off, record, or enforce is accepted here. For example, SGLANG_ROLE_NAMESPACES=Enforce or a value with trailing whitespace enters _check_role_namespace() but matches neither branch, silently disabling enforcement instead of reporting the misconfiguration. Validate or normalize the environment value when it is loaded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — the env value is normalized (strip + lower) and anything outside off/record/enforce raises at import; covered by a test.
efa70fb to
7d5b932
Compare
a988aa0 to
782c2b0
Compare
ee1dfd2 to
352ff4e
Compare
d37d8a4 to
3879e20
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3879e2018e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # typo into "off"). | ||
| from sglang.srt.environ import envs | ||
|
|
||
| return _validated_role_ns_mode(envs.SGLANG_ROLE_NAMESPACES.get() or "off") |
There was a problem hiding this comment.
When an operator explicitly sets SGLANG_ROLE_NAMESPACES= (for example, through an unset deployment-template value), the or "off" fallback converts that invalid value into off and silently disables enforcement. Fresh evidence beyond the earlier validation thread is that the new _validated_role_ns_mode() check is still bypassed specifically for the empty string; the field already defaults to off when the variable is absent, so the fallback should not replace an explicitly supplied value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — the or \"off\" fallback is gone: an explicitly empty SGLANG_ROLE_NAMESPACES= now reaches the validator and raises instead of silently disabling enforcement (absence still defaults to off via the EnvStr default).
Review dispositionAll review threads resolved (env registration, OUT-file stderr on failure, tokenizer comment). No remaining open findings from this review pass. |
edd6acc to
efbcf96
Compare
3879e20 to
0b5a5eb
Compare
Review disposition (r3)Clean — no open findings. Prior threads remain resolved. Also verified latest head fixes:
No PENDING review this round (zero issues). |
efbcf96 to
3b6ec54
Compare
0b5a5eb to
eac3495
Compare
3b6ec54 to
6f94dae
Compare
eac3495 to
6a68c5e
Compare
6f94dae to
dbb9edc
Compare
6a68c5e to
9f6e1cb
Compare
dbb9edc to
5dcfb35
Compare
9f6e1cb to
437fefa
Compare
5dcfb35 to
1b9e720
Compare
437fefa to
27f4422
Compare
1b9e720 to
78318da
Compare
27f4422 to
d9aa7f8
Compare
…MESPACES publish(role=...) has recorded provenance since the namespace split; this wires the enforcement the role was reserved for. SGLANG_ROLE_NAMESPACES selects the mode: - off (default): no bookkeeping; the mode gate in config_bag stays a dead-branch-prunable check under dynamo (bag reads run inside compiled forwards — pinned by a fullgraph test). - record: audit mode — collect (role, namespace) pairs per process and persist each new pair immediately to SGLANG_ROLE_NAMESPACES_OUT (worker teardown skips atexit), plus a per-process stderr summary at exit. - enforce: a bag read outside the role's ROLE_NAMESPACE_SETS entry fails closed with an actionable error; None entries mean full tree. Sets are filled only where audits back them: dp_controller reads only exec (record-mode plain + DP-attention smokes agree with the module's static read set — the elastic-EP gate). tokenizer observed zero bag reads (per-instance managers read self.server_args by design) but keeps the full tree until the multi-tokenizer disagg shape (TokenizerWorker's get_disagg read) is audited; encoder / expert_backup / weight_cache_daemon likewise await their deployment shapes. Verified end-to-end: DP-attention smoke under enforce boots and serves with zero violations.
d9aa7f8 to
96a560f
Compare
…MESPACES (sgl-project#33172) publish(role=...) has recorded provenance since the namespace split; this wires the enforcement the role was reserved for. SGLANG_ROLE_NAMESPACES selects the mode: - off (default): no bookkeeping; the mode gate in config_bag stays a dead-branch-prunable check under dynamo (bag reads run inside compiled forwards — pinned by a fullgraph test). - record: audit mode — collect (role, namespace) pairs per process and persist each new pair immediately to SGLANG_ROLE_NAMESPACES_OUT (worker teardown skips atexit), plus a per-process stderr summary at exit. - enforce: a bag read outside the role's ROLE_NAMESPACE_SETS entry fails closed with an actionable error; None entries mean full tree. Sets are filled only where audits back them: dp_controller reads only exec (record-mode plain + DP-attention smokes agree with the module's static read set — the elastic-EP gate). tokenizer observed zero bag reads (per-instance managers read self.server_args by design) but keeps the full tree until the multi-tokenizer disagg shape (TokenizerWorker's get_disagg read) is audited; encoder / expert_backup / weight_cache_daemon likewise await their deployment shapes. Verified end-to-end: DP-attention smoke under enforce boots and serves with zero violations.
…MESPACES (sgl-project#33172) publish(role=...) has recorded provenance since the namespace split; this wires the enforcement the role was reserved for. SGLANG_ROLE_NAMESPACES selects the mode: - off (default): no bookkeeping; the mode gate in config_bag stays a dead-branch-prunable check under dynamo (bag reads run inside compiled forwards — pinned by a fullgraph test). - record: audit mode — collect (role, namespace) pairs per process and persist each new pair immediately to SGLANG_ROLE_NAMESPACES_OUT (worker teardown skips atexit), plus a per-process stderr summary at exit. - enforce: a bag read outside the role's ROLE_NAMESPACE_SETS entry fails closed with an actionable error; None entries mean full tree. Sets are filled only where audits back them: dp_controller reads only exec (record-mode plain + DP-attention smokes agree with the module's static read set — the elastic-EP gate). tokenizer observed zero bag reads (per-instance managers read self.server_args by design) but keeps the full tree until the multi-tokenizer disagg shape (TokenizerWorker's get_disagg read) is audited; encoder / expert_backup / weight_cache_daemon likewise await their deployment shapes. Verified end-to-end: DP-attention smoke under enforce boots and serves with zero violations.
…MESPACES (sgl-project#33172) publish(role=...) has recorded provenance since the namespace split; this wires the enforcement the role was reserved for. SGLANG_ROLE_NAMESPACES selects the mode: - off (default): no bookkeeping; the mode gate in config_bag stays a dead-branch-prunable check under dynamo (bag reads run inside compiled forwards — pinned by a fullgraph test). - record: audit mode — collect (role, namespace) pairs per process and persist each new pair immediately to SGLANG_ROLE_NAMESPACES_OUT (worker teardown skips atexit), plus a per-process stderr summary at exit. - enforce: a bag read outside the role's ROLE_NAMESPACE_SETS entry fails closed with an actionable error; None entries mean full tree. Sets are filled only where audits back them: dp_controller reads only exec (record-mode plain + DP-attention smokes agree with the module's static read set — the elastic-EP gate). tokenizer observed zero bag reads (per-instance managers read self.server_args by design) but keeps the full tree until the multi-tokenizer disagg shape (TokenizerWorker's get_disagg read) is audited; encoder / expert_backup / weight_cache_daemon likewise await their deployment shapes. Verified end-to-end: DP-attention smoke under enforce boots and serves with zero violations.
1 participant
Part 3/4 of the config-namespace follow-up stack (RFC: #30696). Base: #33171.
Motivation
publish(role=...)has recorded which process type installed the config since the namespace split; this wires the enforcement that the role was reserved for, so a config read outside a process type's declared surface fails loud instead of silently coupling process types.Modifications
SGLANG_ROLE_NAMESPACESselects the mode:config_bagis a single dead-branch-prunable check under dynamo (bag reads run inside compiled forwards; pinned by a fullgraph test).SGLANG_ROLE_NAMESPACES_OUT(worker teardown skips atexit), plus a per-process stderr summary at exit.ROLE_NAMESPACE_SETSentry fails closed with an actionable error;Noneentries mean full tree.Sets are filled only where audits back them:
dp_controllerreads onlyexec(record-mode plain + DP-attention smokes agree with the module's static read set — the elastic-EP gate).tokenizerobserved zero bag reads (per-instance managers readself.server_argsby design) but keeps the full tree until the multi-tokenizer disagg shape is audited;encoder/expert_backup/weight_cache_daemonlikewise await their deployment shapes.Verification
Unit tests for all three modes + the fullgraph compile probe; DP-attention smoke under
enforceboots and serves with zero violations.🤖 Generated with Claude Code
CI States
Latest PR Test (Base): 🚫 Run #30707146492
Latest PR Test (Extra): 🚫 Run #30707146389