fix(dispatch): close result-pull gaps left by the first pass - #1900
Merged
Conversation
Four loose ends in the result-return path shipped in GCWing#1893, found by re-reading that change rather than by a failure. **An older target failed with clap's own error.** `workspace_result_bundle` is advertised and deliberately kept out of `REQUIRED_DISPATCH_CAPABILITIES` so a CLI that predates it stays fully usable for running jobs — but nothing checked it, so pulling from such a target surfaced `unrecognized subcommand '__workspace_result'`, which says nothing about what to do. Probe for the capability first and fail with the actual remedy. Checked against the live protocol rather than anything cached, so it cannot go stale. **The staging directory did not follow its own neighbour's rule.** Result bundles land beside `.workspace-uploads`, which hardens itself to 0700 even though the outbound root is already owner-only. `.results` was created with a bare `create_dir_all` and its bundle and summary written under the process umask. The parent's 0700 does block traversal today, so this was defence in depth rather than an open door — but the bundle carries the user's source, including the ignored files the snapshot deliberately shipped, and the file beside it records which paths changed. Harden the directory and create both files 0600 before writing, so their contents are never briefly umask-governed. **Pulled bundles were never collected.** `remove_workspace_snapshot` clears only `.workspace-uploads`, so terminal jobs expired after 30 days while their bundles accumulated forever. Add `remove_result_bundle` and call it from the same retention sweep. Kept separate from the snapshot removal on purpose: that one runs as soon as the target durably owns the job, long before the user has had a chance to look at the results. **Account devices could receive a snapshot but never return one.** Device dispatch has a full chunked upload path, yet `pull_result` bailed with "requires an SSH target", leaving device-dispatched snapshot jobs with no way home. SSH pulls the bundle over SFTP; a device transport carries JSON only, so add a bounded read-side verb and stream the same bytes back in base64 chunks — the mirror of the upload. The verb never rebuilds the bundle, so the digest the target reported stays the digest the controller verifies, and the reassembled bytes are checked against it before anything is staged. Both transports record the same durable summary, so the apply step stays transport-blind. Verified through the real CLI: the chunked read reassembles a bundle across five 64-byte chunks with a matching digest, and rejects a zero length, an oversized length, and an offset past the end. Device streaming and its digest-mismatch refusal are covered by tests over a stub RPC. Note: `cargo clippy --all-targets` on services-integrations reports three pre-existing `octal_escapes` errors in manager.rs:6037, unrelated to this change and present on main; only `--all-targets` surfaces them.
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.
Four loose ends in the result-return path shipped in #1893. None were reported as failures — they came out of re-reading that change.
An older target failed with clap's own error
workspace_result_bundleis advertised and deliberately kept out ofREQUIRED_DISPATCH_CAPABILITIES, so a CLI predating it stays fully usable for running jobs. But nothing ever checked it, so pulling from such a target surfaced:which tells the user nothing about what to do. The pull now probes for the capability first and fails with the actual remedy. Checked against the live protocol rather than anything cached, so it cannot go stale.
The staging directory did not follow its own neighbour's rule
Result bundles land beside
.workspace-uploads, which hardens itself to 0700 even though the outbound root is already owner-only..resultswas created with a barecreate_dir_all, and its bundle and summary written under the process umask.To be precise about severity: the parent's 0700 does block traversal today, so this was defence in depth rather than an open door. But the bundle carries the user's source — including the ignored files the snapshot deliberately shipped — and the summary beside it records which paths changed. Both are now created 0600 before writing, so their contents are never briefly umask-governed, and the directory is hardened like its neighbour.
Pulled bundles were never collected
remove_workspace_snapshotclears only.workspace-uploads. Terminal jobs expired after 30 days while their bundles accumulated indefinitely.remove_result_bundleis called from the same retention sweep. Kept separate from the snapshot removal on purpose: that one runs as soon as the target durably owns the job, which is long before the user has had a chance to look at the results.Account devices could receive a snapshot but never return one
Device dispatch has a full chunked upload path, yet
pull_resultbailed withrequires an SSH target— so a device-dispatched snapshot job had no way home.SSH pulls the bundle over SFTP; a device transport carries JSON only. So there is now a bounded read-side verb that streams the same bytes back in base64 chunks, mirroring the upload. It never rebuilds the bundle, so the digest the target reported stays the digest the controller verifies, and the reassembled bytes are checked against it before anything is staged. Both transports record the same durable summary, so the apply step stays transport-blind.
Verification
Through the real CLI:
offset == sizeoffset > sizelength = 0lengthbeyond the capdispatch --helpDevice streaming and its digest-mismatch refusal are covered by tests over a stub RPC, including a multi-chunk bundle to exercise the loop rather than a single-shot read.
Automated: 122
services-integrations, 100services-core, 15 dispatch controller/store, 54 CLI.Out of scope, flagged not fixed
cargo clippy --all-targetsonservices-integrationsreports three deny-leveloctal_escapeserrors atmanager.rs:6037. They are pre-existing onmain, unrelated to this change, and only--all-targetssurfaces them (the literal is in a#[cfg(test)]module), which is why the Rust Build Check stays green. Left alone rather than mixed into this PR.