perf: start commands without waiting for telemetry uploads - #1931
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed September 6, 2026, 6:45 PM ET / 22:45 UTC. ClawSweeper reviewWhat this changesThe PR moves initial telemetry uploads into the background sampler, joins cancelled sampling before completion or lease replacement, documents that lifecycle, and stabilizes an OpenSandbox cancellation test. Merge readiness✅ Ready for maintainer review This remains a useful, focused improvement: captured main and v0.50.0 still block on initial telemetry publication. No actionable correctness or security defect remains, and the landing disposition resolves the earlier changelog concern. Priority: P2 Review scores
Verification
How this fits togetherCrabbox’s run recorder collects resource samples from remote Linux runners and sends them to the coordinator. It preserves those samples in run summaries alongside command results and verified terminal receipts. flowchart LR
A[Remote Linux runner] --> B[Capture baseline]
B --> C[Admit command]
B --> D[Background sampler]
D --> E[Coordinator telemetry]
C --> F[Finish or replace lease]
F --> G[Cancel and join sampler]
G --> H[Finalize summary and receipt]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep best-effort telemetry off command admission while retaining one cancellable sampler owner, baseline summaries, and existing receipt verification. Do we have a high-confidence way to reproduce the issue? Yes: current-main baseline capture synchronously awaits the telemetry POST before command admission, so a slow endpoint delays execution; the supplied regression targets that exact dependency. This review did not execute tests. Is this the best way to solve the issue? Yes: using the existing sampler is a narrow repair that preserves baseline capture, cancellation ownership, API shape, configuration, and stored summaries. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 69e79538dae2. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
History |
|
Maintainer landing disposition: this PR and its Unreleased note are maintainer-authored landing work. The contributor-only changelog restriction does not apply; retaining the entry fulfills the repository's requirement to maintain release notes as work lands. The entry now includes the full PR link. The latest commit only finalizes that changelog reference. Production and test bytes remain those already reviewed and live-tested. The runtime review has no remaining correctness findings; the final exact-head Linux CI gate is running. |
|
Landed as 8dcdbd1 after exact-head Linux CI passed all 11 jobs, including the full Go race test step. The fresh final review has no remaining findings. The real AWS pair verified command completion, retained Linux telemetry baselines, signed receipts, and canonical provider cleanup. The controlled blocked-upload and lifecycle tests prove admission independence and cancellation/join behavior; the one timing pair remains a limited observation rather than a general performance benchmark. The landing checkout is clean on main, and its production recorder and regression-test bytes match the reviewed source. |
1 participant
Summary
Commands synchronously waited for the first best-effort telemetry upload after capturing their resource baseline. A slow telemetry endpoint could therefore delay workload admission even though an upload failure did not prevent execution.
Keep baseline capture before work, but publish it through the existing sampler. Start the 15-second ticker before publication, derive each upload's timeout from the sampler context, and cancel and fully join the sampler before finish, failure, or lease replacement. This also removes the one-second wait that could let an upload outlive its owner. Terminal event handling, baseline retention, and signed receipt verification remain with their existing owners.
Validation
go vet ./..., baseline/candidate builds with Go 1.26.5, and independent P0–P2 review pass.c7a.8xlargelease ineu-west-1, the same working directory/configuration and identical harmless no-sync commands. Both normal commands completed, both signed receipts verified, and both stored run summaries retained Linux baseline samples (32 CPUs; two samples each). Canonical Stop confirmed released/cleanup-complete; all command processes joined, SSH control sockets were absent, and operator configuration was unchanged.Live source provenance: baseline
4e3df6dfc42a49bd665a92b2e2756b909122c8c3, candidate with telemetry patch SHA-2567df3fbc3976486164ab4f4e1f038626519322b023ab8d00ef077dddabc0c2633. Both were task-built development binaries, not signed releases. The later fixture-only commit does not alter production bytes.CI follow-through
The local full race run first used Go's default ten-minute package budget; the rerun used the repository's prescribed
go test -race -timeout=20m ./.... Both exceeded the aggregate CLI-package budget while progressing through other fixtures. Those local failures are retained. Linux CI subsequently passed ond568627e629edd2f971f62394efc95ddd5ee6114, including the full Go test step. Final headd7396168246ccc3c9ad712fae3beaca0d2704c8aadds only the changelog PR reference. Its exact-head CI also passed without retries: all 11 jobs succeeded, including the full Go race step in 19m15s. The final independent bot review reports no remaining findings.The full run also exposed an unchanged OpenSandbox test race, reproduced on the clean baseline: its 20ms deadline could expire before sandbox creation, making its expected rollback deletion invalid. The fixture now cancels only after successful fake creation. It retains the real held lock, exact one-delete assertion, and no-published-claim assertion; existing coverage still tests timed lock cancellation. Its exact regression, full OpenSandbox race package, and independent P2 review pass. Provider production code is unchanged.
Production delta: 9 added / 9 removed (net zero). Docs and the Unreleased changelog describe the behavior. The existing telemetry API and stored representation are unchanged; no migration or configuration option is introduced.