Skip to content

test: align PB memory leak check with RecordSet lifecycle#9

Merged
zanmato1984 merged 1 commit intomasterfrom
codex/pb-memory-contract-coverage-final
Apr 12, 2026
Merged

test: align PB memory leak check with RecordSet lifecycle#9
zanmato1984 merged 1 commit intomasterfrom
codex/pb-memory-contract-coverage-final

Conversation

@zanmato1984
Copy link
Copy Markdown
Owner

@zanmato1984 zanmato1984 commented Apr 3, 2026

Role: Coder-R251

Problem

The PB memory leak coverage checks post-read memory behavior around a result-set lifecycle, but the test was not closing the result set before making the final memory assertions. That leaves the coverage misaligned with the intended cleanup contract.

Root Cause

RecordSet.Close() is part of the expected lifecycle contract here. Reaching EOF is not the same thing as reaching lifecycle end: the lower-level response resource is already released at EOF, but the higher-level record-set / executor state is only cleared when Close() / Finish() runs. A local timing-window validation around that boundary substantially confirmed that this is the decisive boundary for the memory symptom exercised by the test.

Fix

Update TestPBMemoryLeak to assert a single result set, drain it, defer RecordSet.Close(), and only then run the post-read memory assertions.

Validation

  • env GOCACHE=/tmp/go-build-omar go test --tags=intest,deadlock ./pkg/executor/test/unstabletest -run ^TestPBMemoryLeak -count=1
  • env GOCACHE=/tmp/go-build-omar go test --tags=intest,deadlock ./pkg/executor/test/unstabletest -run ^TestPBMemoryLeak -count=5
  • A local timing-window validation around the EOF-before-Close boundary repeatedly showed the memory symptom before Close() and its collapse after Close().

Scope

This strongly validates Close() as the decisive boundary for the memory symptom covered by this test. It does not overclaim universal certainty for every historical fluctuation outside this validated path.

Signed-off-by: Rossi Sun <zanmato1984@gmail.com>
@zanmato1984 zanmato1984 force-pushed the codex/pb-memory-contract-coverage-final branch from 76a6b44 to 3807537 Compare April 12, 2026 01:16
@zanmato1984
Copy link
Copy Markdown
Owner Author

Role: Coder-R251

Rewrote the single PR commit with DCO signoff and force-with-lease pushed the branch to clear the DCO gate for fresh review.

Copy link
Copy Markdown
Owner Author

@zanmato1984 zanmato1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Role: Reviewer-R1

Verdict: LGTM

Checks performed:

  1. Lifecycle boundary correctness: confirmed the test now closes the RecordSet before post-read memory assertions, matching the cleanup contract.
  2. Result-set handling safety: verified a single-result expectation is asserted before indexing (require.Len(records, 1)).
  3. Resource-release guarantee: verified record.Close() is deferred in a bounded closure, covering normal EOF and fail-fast paths.
  4. Assertion validity after refactor: confirmed row-count validation still executes and memory assertions still verify allocation growth plus post-read in-use stabilization.
  5. Scope/regression check: change is isolated to one unstable test file and does not alter production code paths.
  6. PR hygiene and CI: verified DCO signoff on the commit and all required GitHub checks passing on this head SHA.

No material blocking issues found in the current PR state.

Note: GitHub does not allow approving your own PR from this account, so this is posted as a review comment rather than an approval event.

@zanmato1984 zanmato1984 merged commit 54392f5 into master Apr 12, 2026
7 checks passed
@zanmato1984 zanmato1984 deleted the codex/pb-memory-contract-coverage-final branch April 12, 2026 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant