[Config] Round 6.3: the record remembers how it was asked for, and is sealed while resolution runs - #38048
Merged
Conversation
ch-wan
requested review from
CatherineSue,
JustinTong0323,
ispobock,
merrymercy and
slin1237
as code owners
September 4, 2026 19:21
ch-wan
force-pushed
the
cheng/gc-r6-2-fields
branch
from
September 5, 2026 08:10
2607c62 to
3efd951
Compare
ch-wan
force-pushed
the
cheng/gc-r6-3-surfaces
branch
from
September 5, 2026 08:10
f39d589 to
7ae4820
Compare
ch-wan
force-pushed
the
cheng/gc-r6-2-fields
branch
from
September 6, 2026 03:52
3efd951 to
473d21c
Compare
ch-wan
force-pushed
the
cheng/gc-r6-3-surfaces
branch
from
September 6, 2026 03:52
7ae4820 to
9582c70
Compare
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
force-pushed
the
cheng/gc-r6-2-fields
branch
from
September 6, 2026 07:12
473d21c to
4f45882
Compare
ch-wan
force-pushed
the
cheng/gc-r6-3-surfaces
branch
from
September 6, 2026 07:12
9582c70 to
e4772a5
Compare
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049, #38113 and #38194, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
force-pushed
the
cheng/gc-r6-2-fields
branch
from
September 6, 2026 08:43
4f45882 to
c37098c
Compare
ch-wan
force-pushed
the
cheng/gc-r6-3-surfaces
branch
from
September 6, 2026 08:43
e4772a5 to
b35bd14
Compare
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
force-pushed
the
cheng/gc-r6-2-fields
branch
from
September 6, 2026 09:00
c37098c to
3bfa93e
Compare
ch-wan
force-pushed
the
cheng/gc-r6-3-surfaces
branch
from
September 6, 2026 09:00
b35bd14 to
9a17063
Compare
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
force-pushed
the
cheng/gc-r6-2-fields
branch
from
September 6, 2026 10:18
3bfa93e to
2f8f653
Compare
ch-wan
force-pushed
the
cheng/gc-r6-3-surfaces
branch
from
September 6, 2026 10:18
9a17063 to
1551d0d
Compare
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
added a commit
that referenced
this pull request
Sep 6, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
added a commit
that referenced
this pull request
Sep 7, 2026
Not for merging. The work is #38046, #38047, #38048, #38049 and #38113, each stacked on the one before it, and each reviewable on its own terms. GitHub shows a stacked PR only against its parent, so there is nowhere to read the whole thing at once -- this branch is that view, and this empty commit is what lets it be a separate pull request from the same content.
ch-wan
force-pushed
the
cheng/gc-r6-2-fields
branch
from
September 7, 2026 04:25
2f8f653 to
b51a1c9
Compare
ch-wan
force-pushed
the
cheng/gc-r6-3-surfaces
branch
from
September 7, 2026 04:25
1551d0d to
68bd486
Compare
ch-wan
force-pushed
the
cheng/gc-r6-2-fields
branch
from
September 7, 2026 04:36
b51a1c9 to
df385fc
Compare
ch-wan
force-pushed
the
cheng/gc-r6-3-surfaces
branch
from
September 7, 2026 04:36
68bd486 to
4a353b9
Compare
ch-wan
force-pushed
the
cheng/gc-r6-2-fields
branch
from
September 7, 2026 04:40
df385fc to
945d9ad
Compare
ch-wan
force-pushed
the
cheng/gc-r6-3-surfaces
branch
from
September 7, 2026 04:40
4a353b9 to
77275f0
Compare
`/server_info` reports `resolved_dict()` -- what resolution decided. There was no way to ask the other question: what did the operator actually type? The two are not derivable from each other, and the resolved side cannot stand in for the raw one, because a field nobody set reads the same as one set to the value resolution would have picked anyway. The launcher stores the arguments it parsed and the in-process `Engine` stores the call that built the record; `/server_info` and `Engine.get_server_info` report it beside the resolved values, so both surfaces are answerable from one request. It rides on the record rather than in a field: it describes how the configuration was asked for, so it is not part of the configuration -- no CLI flag, no namespace, not in the bags. Being on the record is what lets a subprocess copy answer the same question the launcher can, and `replace_resolved` carries it because a copy was launched by whatever launched its parent.
The read-only guard armed on `_resolution_finished`, so for the whole run of the pipeline nothing stopped a resolver from assigning a field. Nothing in `srt/` does -- 0 assignments statically, and 0 writes observed across the launch-shape matrix with a watching `__setattr__` -- but that was a convention, and the defect it permits is invisible: a value a resolver wrote onto the record is indistinguishable from a value the operator typed, which is the one distinction the record exists to preserve. Arm it when resolution starts instead. A resolver that assigns a field now fails at boot with a message naming `declare_resolution`, which is where the decision belongs: the stash carries a source and leaves the input intact. `declare_direct_writes` asks for the seal by name through `record_writable`. It hands the record to an out-of-tree platform plugin that sets fields on it; those implementations cannot be converted by editing a resolver here, so the write stays and the diff is captured into the stash afterwards. Naming the exception is the point -- an in-tree resolver reaching for it is doing something it should be declaring. Costs nothing: the 211 test-side assignments all happen before `resolve_once`, which a post-resolution write already refused.
ch-wan
force-pushed
the
cheng/gc-r6-3-surfaces
branch
from
September 7, 2026 04:41
77275f0 to
7b421f1
Compare
ch-wan
added a commit
that referenced
this pull request
Sep 7, 2026
Last of four; stacked on #38048. The record is the operator's input; the bags are what is in effect. A reader that takes the record and reads a field off it gets the input, which is the wrong one of the two whenever resolution decided something -- and the mistake is silent, because for most fields and most launches the two agree. Several of these files already read both ways, sometimes in the same expression: ```python get_tokenizer( get_serving().tokenizer_path, tokenizer_mode=server_args.tokenizer_mode, # the input, not the decision ... ) ``` Sixty-odd files convert. Record field reads in runtime code go from 199 to 11. Nine parameters that the conversion emptied are dropped along with the argument at every call site -- the dead-parameter ratchet is what names them. ### "Runs after its process publishes" is a per-entry-point claim Most converted reads sit in the serving and model-executor layers, which only exist after publication, or in the two subprocess entry points, which publish first thing. Three places are not like that, and they keep reading the record they were handed: - **`HttpServerEngineAdapter`** launches the server as a *child*. The parent resolves the record and never publishes, so the adapter's own reads -- the launch banner, the API key in its readiness loop, the TP width in `update_weights_from_tensor` -- are of `self.server_args`. A bag read here fails closed in a bare process, or answers for an unrelated engine in one that happens to have published. - **`serve_grpc`** reads its sidecar port before the integrated servicer builds the `Engine` that publishes. The comment above that line already said so and already bound `cfg = resolving_view(server_args)` for it; the sidecar port and the port it derives from read `cfg`. - **`initialize_dp_attention`** runs from callers whose publish is not guaranteed, so its one predicate stays on the resolution view. `ROLE_NAMESPACE_SETS["dp_controller"]` gains `observability` and `serving`, because the controller's metrics gate, tracing setup and worker-port broadcast now read those namespaces. Under `SGLANG_ROLE_NAMESPACES=enforce` that set is what the process may read, so a conversion that reaches a new namespace has to widen it in the same change. ## Three things worth a reviewer's attention **Eleven reads were `getattr(record, "field", default)`.** An AST scan for attribute access does not see those, so the census that said "43 readers" was counting the shape it could match rather than the thing it was after. `incremental_streaming_output` was read that way twice, and the transcription tests were the only reason it surfaced. **Not every record read is a bag read waiting to happen.** A multimodal processor's `base_gpu_id` is the instance's, not the process's: two engines in one process keep different ones, and `test_publishing_another_config_does_not_move_the_device` exists to say so. It stays on the record while `rl_on_policy_target` beside it moves. `RequestMetricsExporter` is the same shape -- it is handed the directory it writes to, and a test builds several with different ones. `configure_logger` is a third: 17 call sites, one of which passes an `argparse.Namespace`, so it is not a global-context reader at all. Those eleven remaining reads are the ones with a reason. **The fixtures move with the code.** Tests that hung config off a mock manager now publish a record, which is what the serving layer reads; where a test states a value it says so with `override_server_args` instead of assigning through the mock. `test_hisparse_unit` is the last of them: it stubbed a `server_args` onto a fake scheduler to say the decode radix cache was off, and the value it was standing in for is the published default, so the stub goes and the class publishes. ## Two things CI caught that a local sweep could not **`unittest.TestCase.enterContext` is Python 3.11+.** The converted fixtures used it at 18 sites; `requires-python` is `>=3.10` and CI runs 3.10, so every one of them raised `AttributeError` there while passing on a newer local interpreter. They call `enter_override(self, ...)` now -- a four-line helper in `sglang/test/test_utils.py` over the override's own `install()` / `restore()`. **A batched sweep cannot see a missing publish.** Three fixtures needed a published config and did not have one; each *passed* inside a shard where some other file had published, and failed when run alone. The affected cases are `test_serving_completions` (which set `incremental_streaming_output` on the mock manager's record, where nothing reads it now), `test_qwen3_vl_feature_materialization` (same shape for `mm_enable_dp_encoder`), and the two Qwen Rust tests -- whose fixture already carried the comment `# Non-auto: get_resolved_model_impl would choke on a SimpleNamespace` next to the `model_impl` it sets, which is exactly what happened once `get_mm_processor_cls` started reading that value from the bag. Its `publish` mirrors `model_impl` now, like the four fields it already mirrored. ## Verification A full registered-unit sweep (648 files) against this stack's merge-base: 19 failures on both sides, the same 19, none of them config. That sweep is what caught 23 failures the file-scoped runs missed -- and, later, that the narrower 139-file list did not even contain the files this change reaches. It is also what caught the `test_hisparse_unit` fixture above: the file passes inside a shard where something else published, and fails when it is run on its own, which is why every failing file is re-run alone before it is counted.
5 tasks
avifenesh
added a commit
to avifenesh/sglang-upstream
that referenced
this pull request
Sep 14, 2026
…ommand `ServerArgs.resolved_dict()` projects every field verbatim, and it is the string the whole-record readbacks publish: the `server_args=` line `Engine.__init__` and `_launch_subprocesses` log at INFO, `/server_info` and its gRPC and in-process twins, the tokenizer crash-dump snapshot and the runtime-context overlay. `api_key`, `admin_api_key` and `ssl_keyfile_password` are on that record, so every boot writes the configured keys into whatever aggregates stdout, where they outlive the process (reproduced 2026-09-14 on a production-shaped launch: the raw key in the docker log). `/server_info` is `AuthLevel.NORMAL`, so a caller holding only `api_key` could also read `admin_api_key` from the body and replay it against the `ADMIN_FORCE` endpoints. `launch_command` (sgl-project#38048) is a second copy of the same secrets: the launcher stores `" ".join(argv)`, so `--api-key VALUE` travels verbatim, and the in-process `Engine` stores `Engine(api_key='VALUE', ...)`. The same three readbacks publish it. - `arg_utils`: `Arg(secret=True)`, `REDACTED`, `secret_fields(cls)`, `secret_cli_flags(cls)`, and two builders, `redacted_argv` (both argparse spellings, `--flag VALUE` and `--flag=VALUE`) and `redacted_call`. - `fields/serving.py`: the three credentials are marked `secret=True`; help text unchanged. - `ServerArgs.resolved_dict`: a marked field reads as `REDACTED` when set, `None` when unset (whether a key is configured is visible from outside anyway). One chokepoint covers all six publication sites. - `prepare_server_args` and `Engine.__init__` redact the launch command at capture, so no reader has to know. - `RuntimeContext.resolved_server_args_dict` re-applies the redaction after the overlay, which writes raw values. The record keeps the real values: the auth middleware and the SSL loader read the fields, not the projection. Test: test/registered/unit/server_args/test_credential_redaction.py (15, CPU, `base-a-test-cpu`): projection, both launch-command forms, the overlay, the record, the escalation, and a pattern guard that fails when a new credential-shaped field arrives unclassified. `test_server_info.py` gains the endpoint-level case. The 7 behavioral cases fail with only the three markers removed. Design and most of the projection test follow sgl-project#37499 (koladefaj), rebased onto the msgspec record and extended to `launch_command`, which landed after that PR was opened. Refs sgl-project#37457. Co-authored-by: Kolade Fajimi <107228310+koladefaj@users.noreply.github.com>
5 tasks
avifenesh
added a commit
to avifenesh/sglang-upstream
that referenced
this pull request
Sep 14, 2026
…ommand `ServerArgs.resolved_dict()` projects every field verbatim, and it is the string the whole-record readbacks publish: the `server_args=` line `Engine.__init__` and `_launch_subprocesses` log at INFO, `/server_info` and its gRPC and in-process twins, the tokenizer crash-dump snapshot and the runtime-context overlay. `api_key`, `admin_api_key` and `ssl_keyfile_password` are on that record, so every boot writes the configured keys into whatever aggregates stdout, where they outlive the process (reproduced 2026-09-14 on a production-shaped launch: the raw key in the docker log). `/server_info` is `AuthLevel.NORMAL`, so a caller holding only `api_key` could also read `admin_api_key` from the body and replay it against the `ADMIN_FORCE` endpoints. `launch_command` (sgl-project#38048) is a second copy of the same secrets: the launcher stores `" ".join(argv)`, so `--api-key VALUE` travels verbatim, and the in-process `Engine` stores `Engine(api_key='VALUE', ...)`. The same three readbacks publish it. - `arg_utils`: `Arg(secret=True)`, `REDACTED`, `secret_fields(cls)`, `secret_cli_flags(cls)`, and two builders, `redacted_argv` (both argparse spellings, `--flag VALUE` and `--flag=VALUE`) and `redacted_call`. - `fields/serving.py`: the three credentials are marked `secret=True`; help text unchanged. - `ServerArgs.resolved_dict`: a marked field reads as `REDACTED` when set, `None` when unset (whether a key is configured is visible from outside anyway). One chokepoint covers all six publication sites. - `prepare_server_args` and `Engine.__init__` redact the launch command at capture, so no reader has to know. - `RuntimeContext.resolved_server_args_dict` re-applies the redaction after the overlay, which writes raw values. The record keeps the real values: the auth middleware and the SSL loader read the fields, not the projection. Test: test/registered/unit/server_args/test_credential_redaction.py (15, CPU, `base-a-test-cpu`): projection, both launch-command forms, the overlay, the record, the escalation, and a pattern guard that fails when a new credential-shaped field arrives unclassified. `test_server_info.py` gains the endpoint-level case. The 7 behavioral cases fail with only the three markers removed. Design and most of the projection test follow sgl-project#37499 (koladefaj), rebased onto the msgspec record and extended to `launch_command`, which landed after that PR was opened. Refs sgl-project#37457. Co-authored-by: Kolade Fajimi <107228310+koladefaj@users.noreply.github.com>
avifenesh
added a commit
to avifenesh/sglang-upstream
that referenced
this pull request
Sep 14, 2026
…ommand `ServerArgs.resolved_dict()` projects every field verbatim, and it is the string the whole-record readbacks publish: the `server_args=` line `Engine.__init__` and `_launch_subprocesses` log at INFO, `/server_info` and its gRPC and in-process twins, the tokenizer crash-dump snapshot and the runtime-context overlay. `api_key`, `admin_api_key` and `ssl_keyfile_password` are on that record, so every boot writes the configured keys into whatever aggregates stdout, where they outlive the process (reproduced 2026-09-14 on a production-shaped launch: the raw key in the docker log). `/server_info` is `AuthLevel.NORMAL`, so a caller holding only `api_key` could also read `admin_api_key` from the body and replay it against the `ADMIN_FORCE` endpoints. `launch_command` (sgl-project#38048) is a second copy of the same secrets: the launcher stores `" ".join(argv)`, so `--api-key VALUE` travels verbatim, and the in-process `Engine` stores `Engine(api_key='VALUE', ...)`. The same three readbacks publish it. - `arg_utils`: `Arg(secret=True)`, `REDACTED`, `secret_fields(cls)`, `cli_flags(cls)`, `secret_cli_flags(cls)`, and two builders, `redacted_argv` and `redacted_call`. `redacted_argv` reads the flags the way argparse did: exact spelling, alias, or a unique `--` prefix (the launcher parser keeps `allow_abbrev`, so `--api-k SECRET` and `--admin-api-k=SECRET` parse into the fields), in both the `--flag VALUE` and `--flag=VALUE` forms. Handed the parsed record, it also hides the parsed values themselves wherever a whole token or the `=` half of a token equals one, which closes any spelling the metadata cannot know. A credential reused verbatim as another argument's value is hidden there too; that over-hides a readback, the alternative under-hides a key. - `fields/serving.py`: the three credentials are marked `secret=True`; help text unchanged. - `ServerArgs.resolved_dict`: a marked field reads as `REDACTED` when set, `None` when unset (whether a key is configured is visible from outside anyway). One chokepoint covers all six publication sites. - `prepare_server_args` and `Engine.__init__` redact the launch command at capture, so no reader has to know. - `RuntimeContext.resolved_server_args_dict` re-applies the redaction after the overlay, which writes raw values. The record keeps the real values: the auth middleware and the SSL loader read the fields, not the projection. Test: test/registered/unit/server_args/test_credential_redaction.py (22, CPU, `base-a-test-cpu`): projection, the launch command under the full, `=`, abbreviated and metadata-unknown spellings, the prefix-collision flag (`--ssl-keyfile`) and the ambiguous prefix left alone, a lookalike value kept and a reused value hidden, the overlay, the record, the escalation, and a pattern guard that fails when a new credential-shaped field arrives unclassified. `test_server_info.py` gains the endpoint-level case. The 7 behavioral cases fail with only the three markers removed. Design and most of the projection test follow sgl-project#37499 (koladefaj), rebased onto the msgspec record and extended to `launch_command`, which landed after that PR was opened. Refs sgl-project#37457. Co-authored-by: Kolade Fajimi <107228310+koladefaj@users.noreply.github.com>
avifenesh
added a commit
to avifenesh/sglang-upstream
that referenced
this pull request
Sep 14, 2026
…ommand `ServerArgs.resolved_dict()` projects every field verbatim, and it is the string the whole-record readbacks publish: the `server_args=` line `Engine.__init__` and `_launch_subprocesses` log at INFO, `/server_info` and its gRPC and in-process twins, the tokenizer crash-dump snapshot and the runtime-context overlay. `api_key`, `admin_api_key` and `ssl_keyfile_password` are on that record, so every boot writes the configured keys into whatever aggregates stdout, where they outlive the process (reproduced 2026-09-14 on a production-shaped launch: the raw key in the docker log). `/server_info` is `AuthLevel.NORMAL`, so a caller holding only `api_key` could also read `admin_api_key` from the body and replay it against the `ADMIN_FORCE` endpoints. `launch_command` (sgl-project#38048) is a second copy of the same secrets: the launcher stores `" ".join(argv)`, so `--api-key VALUE` travels verbatim, and the in-process `Engine` stores `Engine(api_key='VALUE', ...)`. The same three readbacks publish it. - `arg_utils`: `Arg(secret=True)`, `REDACTED`, `secret_fields(cls)`, `cli_flags(cls)`, `secret_cli_flags(cls)`, and two builders, `redacted_argv` and `redacted_call`. `redacted_argv` reads the flags the way argparse did: exact spelling, alias, or a unique `--` prefix (the launcher parser keeps `allow_abbrev`, so `--api-k SECRET` and `--admin-api-k=SECRET` parse into the fields), in both the `--flag VALUE` and `--flag=VALUE` forms. Handed the parsed record, it also hides the parsed values themselves wherever a whole token or the `=` half of a token equals one, which closes any spelling the metadata cannot know. That pass leaves one position alone: the value of a flag that resolves (exact, alias, unique abbreviation) to a non-credential field of the record, as the next token or the `=` half, because argparse bound it to a field the metadata knows is public (`--api-key 1 --tp 1` keeps `--tp 1`). A matching token anywhere else, after a flag the metadata does not know or standalone, is hidden; there the alternative under-hides a key. No length threshold. - `fields/serving.py`: the three credentials are marked `secret=True`; help text unchanged. - `ServerArgs.resolved_dict`: a marked field reads as `REDACTED` when set, `None` when unset (whether a key is configured is visible from outside anyway). One chokepoint covers all six publication sites. - `prepare_server_args` and `Engine.__init__` redact the launch command at capture, so no reader has to know. - `RuntimeContext.resolved_server_args_dict` re-applies the redaction after the overlay, which writes raw values. The record keeps the real values: the auth middleware and the SSL loader read the fields, not the projection. - `tokenizer_manager.dump_requests_before_crash`: the crash dump writes the record's redacted `launch_command` instead of `" ".join(sys.argv)`. The pickled record itself keeps the values, so the dump stays replayable. Test: test/registered/unit/server_args/test_credential_redaction.py (24, CPU, `base-a-test-cpu`): projection, the launch command under the full, `=`, abbreviated and metadata-unknown spellings, the prefix-collision flag (`--ssl-keyfile`) and the ambiguous prefix left alone, a lookalike value kept, a reused value kept after a known public flag (`--tp 1`, `--model-path m`, `--log-level info`, `--served-model-name`, `=` form) and hidden after an unknown flag or standalone, the crash dump's `launch_command`, the overlay, the record, the escalation, and a pattern guard that fails when a new credential-shaped field arrives unclassified. `test_server_info.py` gains the endpoint-level case. With the three markers removed, 17 of the 24 fail; the 7 that pass do not depend on them. Design and most of the projection test follow sgl-project#37499 (koladefaj), rebased onto the msgspec record and extended to `launch_command`, which landed after that PR was opened. Refs sgl-project#37457. Co-authored-by: Kolade Fajimi <107228310+koladefaj@users.noreply.github.com>
mqhc2020
pushed a commit
to mqhc2020/sglang
that referenced
this pull request
Sep 15, 2026
… sealed while resolution runs (sgl-project#38048) Third of four; stacked on sgl-project#38047. Two small changes, both about the same thing: the record holds the operator's input, and nothing else should be true of it. ## `/server_info` can answer what was actually typed It reports `resolved_dict()` -- what resolution decided. There was no way to ask the other question, and the two are not derivable from each other: a field nobody set reads the same as one set to the value resolution would have picked anyway. The launcher stores the arguments it parsed and the in-process `Engine` stores the call that built the record. All three readbacks report it beside the resolved values, so both surfaces come back in one request: HTTP `/server_info`, `Engine.get_server_info`, and the gRPC bridge's -- the last one builds from `resolved_dict()` and would otherwise have been the one surface of the three that answers only "what resolution decided". It rides on the record rather than in a field -- it describes how the configuration was asked for, so it is not part of the configuration: no CLI flag, no namespace, not in the bags. Being on the record is what lets a subprocess copy answer the same question the launcher can, and `replace_resolved` carries it because a copy was launched by whatever launched its parent. The crash dump already collected all four surfaces (`server_args`, `config_updates`, `resolved_config`, `launch_command`); this is the one that `/server_info` was missing. ## The record is sealed for the length of resolution The read-only guard armed on `_resolution_finished`, so for the whole run of the pipeline nothing stopped a resolver from assigning a field. Nothing in `srt/` does -- 0 assignments statically, and 0 writes observed across the launch-shape matrix with a watching `__setattr__` -- but that was a convention, and the defect it permits is invisible: a value a resolver wrote onto the record is indistinguishable from a value the operator typed, which is the one distinction the record exists to preserve. It now arms when resolution starts. A resolver that assigns a field fails at boot with a message naming `declare_resolution`, which is where the decision belongs: the stash carries a source and leaves the input intact. `declare_direct_writes` asks for the seal by name through `record_writable`. It hands the record to an out-of-tree platform plugin that sets fields on it; those implementations cannot be converted by editing a resolver here, so the write stays and the diff is captured into the stash afterwards. Naming the exception is the point -- an in-tree resolver reaching for it is doing something it should be declaring. ## Verification Costs nothing: the 211 test-side assignments all happen before `resolve_once`, which a post-resolution write already refused. A full registered-unit sweep (648 files) against the stack's merge-base: 19 failures on both sides, the same 19, none of them config. Driving a deliberate write into a real handler produces the new error, so the seal is tested by more than its own unit test.
mqhc2020
pushed a commit
to mqhc2020/sglang
that referenced
this pull request
Sep 15, 2026
…roject#38049) Last of four; stacked on sgl-project#38048. The record is the operator's input; the bags are what is in effect. A reader that takes the record and reads a field off it gets the input, which is the wrong one of the two whenever resolution decided something -- and the mistake is silent, because for most fields and most launches the two agree. Several of these files already read both ways, sometimes in the same expression: ```python get_tokenizer( get_serving().tokenizer_path, tokenizer_mode=server_args.tokenizer_mode, # the input, not the decision ... ) ``` Sixty-odd files convert. Record field reads in runtime code go from 199 to 11. Nine parameters that the conversion emptied are dropped along with the argument at every call site -- the dead-parameter ratchet is what names them. ### "Runs after its process publishes" is a per-entry-point claim Most converted reads sit in the serving and model-executor layers, which only exist after publication, or in the two subprocess entry points, which publish first thing. Three places are not like that, and they keep reading the record they were handed: - **`HttpServerEngineAdapter`** launches the server as a *child*. The parent resolves the record and never publishes, so the adapter's own reads -- the launch banner, the API key in its readiness loop, the TP width in `update_weights_from_tensor` -- are of `self.server_args`. A bag read here fails closed in a bare process, or answers for an unrelated engine in one that happens to have published. - **`serve_grpc`** reads its sidecar port before the integrated servicer builds the `Engine` that publishes. The comment above that line already said so and already bound `cfg = resolving_view(server_args)` for it; the sidecar port and the port it derives from read `cfg`. - **`initialize_dp_attention`** runs from callers whose publish is not guaranteed, so its one predicate stays on the resolution view. `ROLE_NAMESPACE_SETS["dp_controller"]` gains `observability` and `serving`, because the controller's metrics gate, tracing setup and worker-port broadcast now read those namespaces. Under `SGLANG_ROLE_NAMESPACES=enforce` that set is what the process may read, so a conversion that reaches a new namespace has to widen it in the same change. ## Three things worth a reviewer's attention **Eleven reads were `getattr(record, "field", default)`.** An AST scan for attribute access does not see those, so the census that said "43 readers" was counting the shape it could match rather than the thing it was after. `incremental_streaming_output` was read that way twice, and the transcription tests were the only reason it surfaced. **Not every record read is a bag read waiting to happen.** A multimodal processor's `base_gpu_id` is the instance's, not the process's: two engines in one process keep different ones, and `test_publishing_another_config_does_not_move_the_device` exists to say so. It stays on the record while `rl_on_policy_target` beside it moves. `RequestMetricsExporter` is the same shape -- it is handed the directory it writes to, and a test builds several with different ones. `configure_logger` is a third: 17 call sites, one of which passes an `argparse.Namespace`, so it is not a global-context reader at all. Those eleven remaining reads are the ones with a reason. **The fixtures move with the code.** Tests that hung config off a mock manager now publish a record, which is what the serving layer reads; where a test states a value it says so with `override_server_args` instead of assigning through the mock. `test_hisparse_unit` is the last of them: it stubbed a `server_args` onto a fake scheduler to say the decode radix cache was off, and the value it was standing in for is the published default, so the stub goes and the class publishes. ## Two things CI caught that a local sweep could not **`unittest.TestCase.enterContext` is Python 3.11+.** The converted fixtures used it at 18 sites; `requires-python` is `>=3.10` and CI runs 3.10, so every one of them raised `AttributeError` there while passing on a newer local interpreter. They call `enter_override(self, ...)` now -- a four-line helper in `sglang/test/test_utils.py` over the override's own `install()` / `restore()`. **A batched sweep cannot see a missing publish.** Three fixtures needed a published config and did not have one; each *passed* inside a shard where some other file had published, and failed when run alone. The affected cases are `test_serving_completions` (which set `incremental_streaming_output` on the mock manager's record, where nothing reads it now), `test_qwen3_vl_feature_materialization` (same shape for `mm_enable_dp_encoder`), and the two Qwen Rust tests -- whose fixture already carried the comment `# Non-auto: get_resolved_model_impl would choke on a SimpleNamespace` next to the `model_impl` it sets, which is exactly what happened once `get_mm_processor_cls` started reading that value from the bag. Its `publish` mirrors `model_impl` now, like the four fields it already mirrored. ## Verification A full registered-unit sweep (648 files) against this stack's merge-base: 19 failures on both sides, the same 19, none of them config. That sweep is what caught 23 failures the file-scoped runs missed -- and, later, that the narrower 139-file list did not even contain the files this change reaches. It is also what caught the `test_hisparse_unit` fixture above: the file passes inside a shard where something else published, and fails when it is run on its own, which is why every failing file is re-run alone before it is counted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Third of four; stacked on #38047. Two small changes, both about the same thing:
the record holds the operator's input, and nothing else should be true of it.
/server_infocan answer what was actually typedIt reports
resolved_dict()-- what resolution decided. There was no way to askthe other question, and the two are not derivable from each other: a field
nobody set reads the same as one set to the value resolution would have picked
anyway.
The launcher stores the arguments it parsed and the in-process
Enginestoresthe call that built the record. All three readbacks report it beside the
resolved values, so both surfaces come back in one request: HTTP
/server_info,Engine.get_server_info, and the gRPC bridge's -- the last one builds fromresolved_dict()and would otherwise have been the one surface of the threethat answers only "what resolution decided".
It rides on the record rather than in a field -- it describes how the
configuration was asked for, so it is not part of the configuration: no CLI
flag, no namespace, not in the bags. Being on the record is what lets a
subprocess copy answer the same question the launcher can, and
replace_resolvedcarries it because a copy was launched by whatever launchedits parent.
The crash dump already collected all four surfaces (
server_args,config_updates,resolved_config,launch_command); this is the one that/server_infowas missing.The record is sealed for the length of resolution
The read-only guard armed on
_resolution_finished, so for the whole run of thepipeline nothing stopped a resolver from assigning a field. Nothing in
srt/does -- 0 assignments statically, and 0 writes observed across the launch-shape
matrix with a watching
__setattr__-- but that was a convention, and thedefect it permits is invisible: a value a resolver wrote onto the record is
indistinguishable from a value the operator typed, which is the one distinction
the record exists to preserve.
It now arms when resolution starts. A resolver that assigns a field fails at
boot with a message naming
declare_resolution, which is where the decisionbelongs: the stash carries a source and leaves the input intact.
declare_direct_writesasks for the seal by name throughrecord_writable. Ithands the record to an out-of-tree platform plugin that sets fields on it; those
implementations cannot be converted by editing a resolver here, so the write
stays and the diff is captured into the stash afterwards. Naming the exception
is the point -- an in-tree resolver reaching for it is doing something it should
be declaring.
Verification
Costs nothing: the 211 test-side assignments all happen before
resolve_once,which a post-resolution write already refused. A full registered-unit sweep
(648 files) against the stack's merge-base: 19 failures on both sides, the same
19, none of them config. Driving a
deliberate write into a real handler produces the new error, so the seal is
tested by more than its own unit test.
CI States
Latest PR Test (Base): ❌ Run #34022668167
Latest PR Test (Extra): ❌ Run #34022668046
Latest PR Test (AMD ROCm 7.2): ❌ Run #34022668188
CI States
Latest PR Test (Base): 🚫 Run #34084017855
Latest PR Test (Extra): 🚫 Run #34084017699
Latest PR Test (AMD ROCm 7.2): ❌ Run #34084017879