Repository navigation
test: retire synthetic runtime constructor probes - #29309
Conversation
…audit-runtime-construction
…ntime-construction
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title clearly identifies the primary change: retiring synthetic runtime constructor probes. It is concise and specific. Description check The description is complete and detailed. It covers the change, rationale, testing, retained behavior, visual proof, review notes, and checklist items. The maintainer-requested issue context and linke… Full details: Description check
-
Explanation
The description is complete and detailed. It covers the change, rationale, testing, retained behavior, visual proof, review notes, and checklist items. The maintainer-requested issue context and linked CI evidence provide sufficient context for this maintenance change.
-
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
ELI5
Some runtime tests made the constructor call methods before initialization was finished, using getters that existed only for tests. Remove that artificial execution path and test cache behavior through its real owner.
What Changed
Remove two constructor reads, two runtime forwarding getters, and two unused cache snapshot getters. Retire 76 cases that override those getters to probe partially initialized runtime objects. Retain actual terminal creation, rename, close, storage rollback, SSH, publication, and liveness tests.
Replace private-map cache inspection with executable checks: after 513 distinct keys, the oldest key refetches while the next key remains cached; two independent owners fetch independently and then reuse their own successful results. Preserve public
runtime_unavailableprefetch rejection through a fully constructed runtime.Why
The removed constructor callbacks have no production caller. Keeping them makes production code support an invented lifecycle and makes implementation refactors break tests without changing user behavior. Real owner tests preserve the useful bounded-cache and instance-isolation contracts without exposing mutable maps. Production cache algorithms are unchanged. Production removes 18 lines; test/support removes 983 lines.
Linked Issue
Maintainer-requested CI test audit. Stacked after #29304; retarget main and rerun full checks after its base lands.
Visual Proof
N/A — test maintenance and removal of test-only getters; no rendered behavior changes.
Testing
Linux, Node 26.8.1, two workers, same 12 owner/sibling suites: baseline 1,610 passed / one skipped; candidate 1,537 passed / one skipped. All six alternating comparison runs pass. Median wall time 24.622 → 23.665s; summed module time 45.772 → 44.393s. This is a modest local reduction; hosted measurements are recorded below.
Hosted before/after: parent #29304 run 38828680027 versus this PR run 38829650565, using all five passing timing artifacts from each. The same 12 selected modules totaled 80.352 → 76.659s (4.6% lower), with Node 24.21.0/Bun 1.4.3, five ARM shards, and four workers per shard. Every measured file passed; all artifacts report zero unhandled errors. Actual merge-checkout trees match their respective PR heads, and routing/workflow sources are unchanged. This is historical, unpaired summed module worker time, not whole-workflow elapsed time. The unchanged runtime owner accounts for 2.110s of the 3.693s difference, so ordinary runner variation may explain part of this modest result; no stable speedup or full-delta attribution is claimed. Main-based required CI remains pending.
Deliberate mutation controls: raising cache cap from 512 to 1024 fails both eviction checks; sharing successful freshness between owners fails isolation. Production source restored byte for byte afterward. Independent review preserves rename notification ordering and adjacent postconstruction contracts. Removing the public prefetch no-store guard also fails its keeper. An additional five keeper suites passed (1,407 tests / one skipped). Node typecheck, changed-code-quality, formatting, and whitespace gates pass.
Command:
ORCA_BACKGROUND_LAUNCH=1 node node_modules/vitest/vitest.mjs run --config config/vitest.config.ts --maxWorkers=2for the seven edited standalone suites, fetch cache, reference maintenance, nested session inventory, canonical-repo-key, andorca-runtime.test.ts, with default and timing reporters.Review
Initial owner run caught six additional constructor probes in imported specs; reviewing those fragments found a seventh assertion that otherwise passed without running its getter. All seven were independently reviewed before removal. No whole suite retired. The remaining genuine registration, stop-receipt, and liveness contracts retain owner tests.
Agent skill upstream boundary
Checklist
Fresh main-based head c866091 includes landed #29304 and main 46779ca. The exact 13-path delta is byte-identical to the independently approved refresh. Three fresh alternating Linux Node 24.21.0 / Bun 1.4.3 one-worker comparisons all pass: baseline 1,630 cases / one skip, candidate 1,557 / one skip. Median wall 42.605 → 41.935 seconds; summed module 40.063 → 39.440 seconds. These small local observations do not establish a stable speedup. Dynamic case inventory records all changed names. Cache proof is now three behavioral cases replacing one private-map inspection; the public no-store guard is retained outside partial construction. Full changed-code gate against the actual main base passes (two files have added lines; eleven are deletion-only); normal lint and formatting pass across all 13 changed files. Fresh full workflow 38871935998 is pending. Historical hosted measurements above remain historical.
Final exact-head qualification:
.github/workflows/pr.ymlrun38871935998 at c866091 passed aggregate verify / PR checks passed116880839681 and all current checks. Five clean reports cover12,323 files once, with zero unhandled errors. All12 owner modules passed1557 cases/one skip; actual tested source matches all48 independently checked source/config/runner paths. Current hosted comparison to parent run38870300944 gives selected module sum47,741 →44,799ms (6.16% lower), both Node24.19/Bun1.4.3 BlacksmithX64/five shards/four workers. This unpaired comparison includes main/workload/runner variation and does not establish causal or whole-workflow speedup. Fresh local cli/node typechecks and all13-file lint/format passed. This supersedes pending status above.