[PR #6081] [MERGED] fix(ci): use one heap snapshot for JavaScript admission #7140

New issue

Closed

opened 2026-11-01 21:45:42 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:45:42 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/6081
Author: @gensokyo-dev
Created: 10/21/2026
Status: ✅ Merged
Merged: 10/21/2026
Merged by: @gensokyo-dev

Base: main ← Head: fix/javascript-parent-heap-snapshot-20261021

📝 Commits (1)

5e2a6af fix(ci): use one heap snapshot for JavaScript admission

📊 Changes

2 files changed (+31 additions, -20 deletions)

View changed files

📝 src/javascript/analysis/JavaScriptAnalysisWorker.ts (+9 -10)
📝 tests/boundary/javascript/javascriptParentResultAdmission.test.ts (+22 -10)

📄 Description

Summary

Main CI failed because a controlled 64 MiB parent-result fixture reported 67,373,948 bytes, above 67,108,864. Its mocked heap limit followed one live heap reading while admission subtracted another, so allocation/collection could move the supposed fixed envelope. No historical GC trace was captured.

Use the limit and used heap from one V8 observation in all three parent-admission paths. One private owner now supplies initial result budgeting, checkpoint decode and remaining-capacity checks. Node's implementation confirms process.memoryUsage().heapUsed already uses V8's used_heap_size; this keeps the metric and removes the second heap/RSS observation. Reserves, ceilings, worker heap, 30s deadline, transfer checks and partial-fact retention stay unchanged.

Make the fixture's limit/usage pair coherent, and cover normal observations plus a modeled 1 MiB collection between independent reads. The worker, transfer files and descriptors remain real; 600 callable facts must survive while only the oversized graph is declined. This is a CI-fixture correctness fix and related observation-owner refactor; it does not demonstrate a real-runtime GC over-admission bug.

Evidence

Head 5e2a6afcf4fa5c1368492d37a243084f143757f5, base f86206535 (latest fetched main); Node 24.18.0/npm 11.16.0. Four-core affinity, one test worker, two compiler checkers and serial Turbo.

Red before the implementation fix: 2 failures, observed budgets 67,099,544 and 67,623,832 versus the required 67,108,864. Green:both cases pass with an exact 64 MiB assertion. No widened threshold or retry.
Relevant worker, transfer-budget and cleanup/exit-ordering set: 53 passed, 0 skipped across 4 files. npm run check passed after the final test refactor; diff and formatting checks pass.
Warm filesystem/fresh test-process measurements:the two completed real admission tasks took 8.6s/12.4 CPU seconds/674MiB sampled family RSS before, and 9.1s/12.9 CPU seconds/677MiB after. The wider 53-test set took 21.9s/28.0 CPU seconds/653MiB; final static checks took 29.1s/51.5 CPU seconds/1660MiB. RSS may double-count shared pages/miss short children. No speedup, whole-CI duration or agent-token claim.
Complete exact-head qualification passed on its first attempt: 24,273 portable tests passed, 200 named skips; 1901 files passed, 24 skipped; all selected real-platform lanes and the required aggregate passed. The canonical PR run also passed after restoring its initially canceled feedback attempt; no test failure was retried. Codex reviewed this exact head without findings; Greptile 5/5, no unresolved threads.
Main advanced to d8a9673fc during CI. Its clean integration with this PR has tree ead505065ef7fd4d07101187ab522ea4c0f8b121:complete static checks passed, plus 295 tests passed and 7 Darwin-conditional skips across 13 files. This includes the four worker suites and all newly changed tests. Actual merge commit 01ee6551b4a39088cb93546f9f1cf4fe62956519 has the same tested integration tree. Its complete main qualification passed on the first attempt: 40 jobs passed, 1 intentional Source-test skip; 24,278 portable tests passed, 200 named skips (1901 files passed, 24 skipped). Both strengthened heap cases ran and passed. The normal main push CI also passed. Runtime evidence here is Node; Bun and unrelated deep providers were not exercised locally.

Author readiness

Human owner: @gensokyo-dev

Declarations entered under the user's explicit delegation; independent human review was not observed in this session.

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
I declare this PR ready for initial maintainer review.

Before merge

This PR addresses a concrete problem or an agreed enhancement.
Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/6081 **Author:** [@gensokyo-dev](https://github.com/gensokyo-dev) **Created:** 10/21/2026 **Status:** ✅ Merged **Merged:** 10/21/2026 **Merged by:** [@gensokyo-dev](https://github.com/gensokyo-dev) **Base:** `main` ← **Head:** `fix/javascript-parent-heap-snapshot-20261021` --- ### 📝 Commits (1) - [`5e2a6af`](https://github.com/morluto/rea/commit/5e2a6afcf4fa5c1368492d37a243084f143757f5) fix(ci): use one heap snapshot for JavaScript admission ### 📊 Changes **2 files changed** (+31 additions, -20 deletions) <details> <summary>View changed files</summary> 📝 `src/javascript/analysis/JavaScriptAnalysisWorker.ts` (+9 -10) 📝 `tests/boundary/javascript/javascriptParentResultAdmission.test.ts` (+22 -10) </details> ### 📄 Description ## Summary [Main CI](https://github.com/morluto/rea/actions/runs/39622715222/job/119305674391) failed because a controlled 64 MiB parent-result fixture reported 67,373,948 bytes, above 67,108,864. Its mocked heap limit followed one live heap reading while admission subtracted another, so allocation/collection could move the supposed fixed envelope. No historical GC trace was captured. Use the limit and used heap from one V8 observation in all three parent-admission paths. One private owner now supplies initial result budgeting, checkpoint decode and remaining-capacity checks. Node's [implementation](https://github.com/nodejs/node/blob/v24.18.0/src/node_process_methods.cc#L203-L224) confirms process.memoryUsage().heapUsed already uses V8's used_heap_size; this keeps the metric and removes the second heap/RSS observation. Reserves, ceilings, worker heap, 30s deadline, transfer checks and partial-fact retention stay unchanged. Make the fixture's limit/usage pair coherent, and cover normal observations plus a modeled 1 MiB collection between independent reads. The worker, transfer files and descriptors remain real; 600 callable facts must survive while only the oversized graph is declined. This is a CI-fixture correctness fix and related observation-owner refactor; it does not demonstrate a real-runtime GC over-admission bug. ## Evidence Head `5e2a6afcf4fa5c1368492d37a243084f143757f5`, base `f86206535` (latest fetched main); Node 24.18.0/npm 11.16.0. Four-core affinity, one test worker, two compiler checkers and serial Turbo. - Red before the implementation fix: 2 failures, observed budgets 67,099,544 and 67,623,832 versus the required 67,108,864. Green:both cases pass with an exact 64 MiB assertion. No widened threshold or retry. - Relevant worker, transfer-budget and cleanup/exit-ordering set: 53 passed, 0 skipped across 4 files. `npm run check` passed after the final test refactor; diff and formatting checks pass. - Warm filesystem/fresh test-process measurements:the two completed real admission tasks took 8.6s/12.4 CPU seconds/674MiB sampled family RSS before, and 9.1s/12.9 CPU seconds/677MiB after. The wider 53-test set took 21.9s/28.0 CPU seconds/653MiB; final static checks took 29.1s/51.5 CPU seconds/1660MiB. RSS may double-count shared pages/miss short children. No speedup, whole-CI duration or agent-token claim. - [Complete exact-head qualification](https://github.com/morluto/rea/actions/runs/39628915438) passed on its first attempt: 24,273 portable tests passed, 200 named skips; 1901 files passed, 24 skipped; all selected real-platform lanes and the required aggregate passed. The [canonical PR run](https://github.com/morluto/rea/actions/runs/39628730632) also passed after restoring its initially canceled feedback attempt; no test failure was retried. Codex reviewed this exact head without findings; Greptile 5/5, no unresolved threads. - Main advanced to `d8a9673fc` during CI. Its clean integration with this PR has tree `ead505065ef7fd4d07101187ab522ea4c0f8b121`:complete static checks passed, plus 295 tests passed and 7 Darwin-conditional skips across 13 files. This includes the four worker suites and all newly changed tests. Actual merge commit `01ee6551b4a39088cb93546f9f1cf4fe62956519` has the same tested integration tree. Its [complete main qualification](https://github.com/morluto/rea/actions/runs/39633363052) passed on the first attempt: 40 jobs passed, 1 intentional Source-test skip; 24,278 portable tests passed, 200 named skips (1901 files passed, 24 skipped). Both strengthened heap cases ran and passed. The [normal main push CI](https://github.com/morluto/rea/actions/runs/39633138703) also passed. Runtime evidence here is Node; Bun and unrelated deep providers were not exercised locally. ## Author readiness Human owner: @gensokyo-dev Declarations entered under the user's explicit delegation; independent human review was not observed in this session. - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-11-01 21:45:42 +00:00