Repository navigation
wsl: terminate over COM, and stop demoting on service errors - #408
Merged
Merged
Conversation
Adds GetDistributionId and TerminateDistribution to the COM fast path, and records why Exec is not joining them. ## Exec is not tractable, and that is now written down The issue and the existing code both suspected CreateLxProcess was the hard part. Reading the real signature in wslservice.idl settles it: 24 parameters, four RETURNED sockets (stdin, stdout, stderr, CommunicationChannel), a separate InteropSocket, a process handle and a server handle. Driving it means reimplementing the relay and channel protocol wsl.exe already implements, against an interface whose stability Microsoft disclaims, for calls that are not on a hot loop. The spawn is the better trade. Said so in the Fast doc comment so nobody re-opens it hopefully. ## What the numbers actually are GetDistributionId 0.54 ms (List, for scale: 0.66-0.81 ms) Terminate over COM 10-16 ms Terminate via wsl.exe ~99 ms So ~89 ms saved, all of it process spawn -- the remaining ~15 ms is the service genuinely stopping the distro, which the CLI pays too. That is why this is 9x and List is 85x: List is a pure query where spawn IS the cost. Marginal as latency: terminate runs on `skrog stop` and restarts, not in a loop. The real returns are error quality, and GetDistributionId itself -- every GUID-taking method (#381's ExportDistributionPipe and RegisterDistributionPipe, #382's SetSparse and ResizeDistribution, #383's AttachDisk) needs exactly this lookup, and now has it at half a millisecond. ## A bug the live test caught, that offline tests structurally could not Terminating a distro that does not exist is the service ANSWERING, not the interface having moved. The first version treated it as the latter: it demoted COM for the rest of the process and re-ran the doomed operation through wsl.exe to produce a second, localised error. Visible only against a real service. ServiceError now separates "the service replied with an error" from "the plumbing failed", and only the second demotes. The IID gate already covers slot correctness, so a call that came back at all is evidence the surface is intact, whatever it came back with. Error text went from matching English prose to `GetDistributionId: HRESULT 0x80040302`. Slots are anchored on the one already proven -- EnumerateDistributions is declaration 13, three IUnknown methods ahead, so slot 15, which is the existing verified constant. Counting from that anchor gives 6 and 7. Live tests assert the distro actually STOPPED rather than that the call returned S_OK: a wrong slot would call a different method on a live object, and only a state assertion catches that.
This was referenced Sep 18, 2026
zcsizmadia
added a commit
that referenced
this pull request
Sep 18, 2026
* fix: three defects the independent release review found, all shipped today An independent 4-agent review of main (correctness / security / concurrency / release readiness) ahead of dropping the pre-release flag. These three were confirmed against the code and are mine from the last two days. 1. deny-unattributable-builds (#376) was a COMPLETE no-op in the product. Watcher.DenyBuild returned a hardcoded ("", false). Watcher -- not Rules -- is what the bridge installs as its gate (proxy.go, serve.go, supervise.go and wslcbackend.go all pass it), so the rule shipped, gained 176 lines of documentation, was reported active by `policy show`, and did nothing. The comment explaining the hardcode outlived its reason: it predated #376 giving Rules.DenyBuild something to say. Every test in build_test.go called Rules.DenyBuild(). Nothing called the method the product reaches. The `var _ pipeproxy.ImageGate` guard -- which exists precisely to stop this gate going quiet -- only proves the method exists, not that it consults anything. combinedGate.DenyBuild had the same gap on wslc: DenyPull and DenyPush consult both layers, DenyBuild consulted only WSL's. 2. The machine-wide policy layer (#386) failed OPEN on an unreadable file. The user layer sets `unknown` and refuses when nothing has ever parsed (#254). refreshMachineLocked had no equivalent, so machineRules stayed the zero value and requests were judged as though no fleet policy existed. Parse uses KnownFields(true), so a misspelled rule is a hard error rather than an ignored key: one typo in an Intune deployment meant every machine that received it ran unenforced, with one log line -- while `skrog policy show` reported the file as broken. The two disagreeing in that direction is the worst available pair of answers. machineLastErr is now separate from lastErr; sharing one field let two layers erroring alternately defeat the once-only OnError dedupe. 3. internal/wsl stopped building for !windows in #408. `terminate` was added to com_windows.go and not to the shim. CI builds only windows-latest and windows-11-arm, so nothing caught it, and I had run GOOS=linux against a different package and moved on. com_other.go now carries a note that a method added on one side must be added on the other, with the command that checks it. Each fix has a test that fails against the old code -- verified by reverting the fix and watching the test catch it, rather than assuming it would. * docs: the command reference described the ACL vulnerability #79 fixed (S1) `skrog proxy --sddl` help said the default "restricts to SYSTEM, admins and interactive users". It has not since v0.3: defaultSDDL returns D:P(A;;GA;;;SY)(A;;GA;;;BA)(A;;GA;;;<owning user SID>) with no IU ACE, and dacl_windows_test.go asserts ";IU)" never appears. That string is published verbatim into docs/reference.md and onto the docs site, so the command reference told every reader -- including any security reviewer evaluating this for an enterprise -- that the pipe is open to every interactive account on the machine. docs/security.md describes exactly that as a v0.2.0 behaviour that was tightened in v0.3, so the two documents contradicted each other and the generated one was wrong. Worth noting what did and did not work here. The CI drift-check did its job perfectly: it faithfully republished the help text on every change. A generated doc is only as true as its source string, and nothing checks a source string against the behaviour it describes. One line, plus a comment at the flag saying why the wording matters, so the next person to reword it knows it is load-bearing.
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.
Part of #356. Adds
GetDistributionIdandTerminateDistributionto the COM fast path — and records whyExecis not joining them, so the issue can be scoped honestly rather than left open on a hope.Exec is not tractable
The issue and the existing code both suspected
CreateLxProcesswas the hard part. Reading the real signature inwslservice.idlsettles it:Driving that means reimplementing the relay and channel protocol
wsl.exealready implements, against an interface whose stability Microsoft disclaims, for calls that are not on a hot loop. The spawn is the better trade. It is now stated in theFastdoc comment so nobody re-opens it hopefully.This corrects a claim I made when proposing the sprint. I argued #356 would attack the remaining #398 start latency, because every
Execis 165 ms warm / 2,963 ms cold. That is not deliverable. The enabler argument still holds, and is stronger than I thought — see below.The numbers, measured on this host
~89 ms saved, all of it process spawn. The remaining ~15 ms is the service genuinely stopping the distro, which the CLI pays too. That is why this is 9× where
Listis 85×:Listis a pure query, so spawn is the entire cost.As latency this is marginal —
Terminateruns onskrog stopand restarts, not in a loop, and 89 ms is under 2% of the 4.5–5.5 s start path. The real returns are error quality andGetDistributionIditself: every GUID-taking method needs that lookup, and it now exists at half a millisecond. #381 (ExportDistributionPipe,RegisterDistributionPipe), #382 (SetSparse,ResizeDistribution) and #383 (AttachDisk,MountDisk) all sit directly on it.A bug the live test caught, that offline tests structurally could not
Terminating a distro that does not exist is the service answering, not the interface having moved. My first version treated it as the latter: it demoted COM for the rest of the process and re-ran the doomed operation through
wsl.exeto produce a second, localised error.ServiceErrornow separates "the service replied with an error" from "the plumbing failed", and only the second demotes. The IID gate already covers slot correctness, so a call that came back at all is evidence the surface is intact, whatever it came back with.Offline tests could not have found this: they exercise the fallback against a fake, and the fake has no opinion about what kind of error it returns.
Slot safety
Anchored on the one already proven.
EnumerateDistributionsis declaration 13 with three IUnknown methods ahead of it, so slot 15 — the existing constant, verified working against the live service. Counting from that anchor givesGetDistributionId= 6 andTerminateDistribution= 7. The IID gate covers these exactly as it covers slot 15.The live tests assert the distro actually stopped, not that the call returned
S_OK. A wrong slot would invoke a different method on a live object, and only a state assertion catches that.Verified
go test -tags wslcom ./internal/wsl/against the realwslservice: terminate works and the state changes; an unknown distro produces an HRESULT with no fallback and no demotion; latency and the lookup/work split are logged.go test ./...andlint.ps1green. Engine restored and running.Leaving #356 open
ExecandStartwere the bulk of the issue's ambition and are now explicitly out of scope with a reason. What remains worth doing under it is the rest of the GUID-taking surface, which #381/#382/#383 will pull in as they need it. Worth deciding whether to narrow the title or close it in favour of those three.