Skip to content

fix: eliminate CI test lifecycle leaks and metric flakiness - #27480

Merged
XuPeng-SH merged 4 commits into
mainfrom
codex/issues-27452-27412-root-fixes
Aug 22, 2026
Merged

fix: eliminate CI test lifecycle leaks and metric flakiness#27480
XuPeng-SH merged 4 commits into
mainfrom
codex/issues-27452-27412-root-fixes

Conversation

@XuPeng-SH

@XuPeng-SH XuPeng-SH commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

Fixes #27452
Fixes #27412
Fixes #27408

What this PR does / why we need it:

This fixes the underlying lifecycle and measurement contracts behind the two CI failures instead of reducing coverage scope, weakening assertions, or rerunning deterministic failures.

Coverage OOM root cause

Heap profiling of the exact 462-package coverage shape showed that repeated MockCompilerContext.GetProcess calls created fresh Process instances. Each Process then replaced the service runtime's AutoIncrementService. The displaced services retained background workers and caches, so the plan test process grew to about 12 GiB and was killed by the 14 GiB runner cgroup.

  • Reuse one Process for each mock compiler context, matching the compiler-session contract.
  • Preserve that identity when constructor-created mock contexts are copied by sharing a holder instead of copying synchronization state.
  • Make testutil.SetupAutoIncrService concurrency-safe and idempotent per service runtime.
  • Add concurrent regression tests for constructor-created, literal, and copied mock contexts and for auto-increment service identity.

The same full pkg/sql/plan suite with the CI 462-package coverpkg list passes with peak RSS reduced from 12,537,728 KiB to 269,880 KiB (97.85%). Coverage scope and test parallelism are unchanged.

Hardware CPU test root cause

The old assertion compared a cumulative busy-seconds delta with cpu.Percent sampled over a different time window. Those values are not mathematically comparable under changing runner load.

The test now validates each metric's actual contract independently: percentage is bounded to [0, 100], the cumulative counter is non-negative, and the counter is monotonic. The unnecessary one-second sleep is removed.

Scope decision

Native/CGo dependency-closure hardening is independent of these lifecycle and metric fixes. It has been removed from this PR so the change remains reviewable and continues to use the current main CI link contract. Any native-link hardening should be reviewed separately with its matching MatrixOne and CI changes.

Validation

  • Exact 462-package coverpkg full pkg/sql/plan suite.
  • Each new/changed regression test with -race -count=100.
  • Full pkg/sql/plan, pkg/testutil, and pkg/util/metric suites with -race.
  • Full normal tests for all three owning packages.
  • go build and go vet for all modified Go packages.
  • git diff --check and explicit verification that the four native/CI files match the PR base.

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@XuPeng-SH

Copy link
Copy Markdown
Contributor Author

Follow-up on the failed coverage check:

  • This is not a recurrence of the coverage OOM. The current job compiled successfully, then every generated test binary failed at startup because libmo.so was not in its ELF RUNPATH.
  • The missing flag lives in the reusable matrixorigin/CI workflow, outside this repository. Companion fix: ci: fix coverage cgo runtime loading CI#429.
  • The fix also removes another hard-coded native dependency list by querying MatrixOne cgo/lib.go, matching this PRs run_ut.sh and ci-builder contract.

Linux/arm64 reproduction with LD_LIBRARY_PATH unset: old flags linked but exited 127 (libmo.so missing); fixed flags embedded /workspace/cgo and exited 0. The MatrixOne product build rpath remains unchanged.

@XuPeng-SH

Copy link
Copy Markdown
Contributor Author

Scope correction completed in f12d818: the four native/CGo link-contract files were restored to the PR base. #27480 now contains only the lifecycle leak and hardware-metric test fixes, so matrixorigin/CI#429 is no longer required for this PR. The earlier loader failure was caused by the bundled native-link change, not by the lifecycle fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working kind/enhancement kind/refactor Code refactor kind/test-ci size/M Denotes a PR that changes [100,499] lines

Projects

None yet

2 participants