Skip to content

fix: prevent double release from corrupting the connection pool#4186

Merged
wellwelwel merged 3 commits intosidorares:masterfrom
wellwelwel:loop
Mar 15, 2026
Merged

fix: prevent double release from corrupting the connection pool#4186
wellwelwel merged 3 commits intosidorares:masterfrom
wellwelwel:loop

Conversation

@wellwelwel
Copy link
Copy Markdown
Collaborator

@wellwelwel wellwelwel commented Mar 15, 2026

Note


Closes #3559, closes #2325, resolves #2171.


What I did was provide the entire context regarding the complex issues related to the following prompt (Issue to LLM improved text in 3 .txt files):

Me: Read and understand the three issues related to the same problem, even if with perhaps distinct nuances. Your task is to create a test that reproduces the behavior and is compatible with the GitHub Actions environment. Read the basic instructions before starting in @CLAUDE.md. You can outline the plan for both the reproduction and the possible fix, but you must not implement the fix until the tests properly fail as expected without any implementation done.

The first test resulted in something like (meh...):

const pool = createPool({
  connectionLimit: 2,
  maxIdle: 1,
  idleTimeout: 500,
});

await new Promise<void>((resolve, reject) => {
  pool.getConnection(
    (err: NodeJS.ErrnoException | null, connection: PoolConnection) => {
      if (err) return reject(err);

      connection.release();
      connection.release();
      resolve();
    }
  );
});

// Wait for idle timeout cleanup to run (1s timer + 500ms idle timeout)
await sleep(3000);

This works, but it's a fragile test because it relies on timers that behave differently in different environments (especially with GitHub Actions).

After some refinements, I reached the current solution. So, this is a PR 100% made from the created instructions, combined with prompts and the related complex contexts, using Claude Opus 4.6 in Max Thinking + Plan Mode > Ask Before Edits.

I admit I really don't like it, but I can't deny it works (and quite well, actually).


Now, Claude description:

Summary

Calling connection.release() more than once pushes duplicate references into _freeConnections, which causes:

  • Same connection handed to multiple handlers, breaking transaction isolation
  • Infinite loop in _removeIdleTimeoutConnections (100% CPU)

The fix adds a _released flag to PoolConnection that guards against repeated release() calls. The flag is reset when the connection is re-acquired via getConnection() or handed to a waiting callback.

Closes #3559

Test plan

  • New integration tests in test-pool-double-release.test.mts covering:
    • _freeConnections length after double release
    • Unique connections returned after double release
    • No duplicate references in _freeConnections with multiple connections
  • All existing pool tests pass (FILTER=test/integration/test-pool npx poku)
  • Typecheck passes (npm run typecheck)

🤖 Generated with Claude Code


cc @sidorares

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 15, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.68%. Comparing base (92d0724) to head (4521b6f).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4186   +/-   ##
=======================================
  Coverage   90.67%   90.68%           
=======================================
  Files          86       86           
  Lines       14227    14238   +11     
  Branches     1796     1798    +2     
=======================================
+ Hits        12901    12912   +11     
  Misses       1326     1326           
Flag Coverage Δ
compression-0 89.94% <100.00%> (+<0.01%) ⬆️
compression-1 90.66% <100.00%> (+<0.01%) ⬆️
static-parser-0 88.36% <100.00%> (+<0.01%) ⬆️
static-parser-1 89.09% <100.00%> (+<0.01%) ⬆️
tls-0 90.11% <100.00%> (+<0.01%) ⬆️
tls-1 90.46% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@wellwelwel
Copy link
Copy Markdown
Collaborator Author

wellwelwel commented Mar 15, 2026

"Debug" of:

Me: Explain to me how you came up with that solution. It really solved the problem.

Claude:

I'm glad the result was solid, even if the process isn't your ideal. Your intervention in the prompts was essential — without the timer test correction and the direction toward the root cause instead of the symptom, the PR would have come out weaker.

As for an honest opinion: I think the CLAUDE.md you wrote did most of the heavy lifting. The instructions about Poku, connection scope, cleanup, and test patterns are precise enough that I didn't invent wrong things. The initial prompt providing context from the 3 issues in .txt files was also decisive — without it I would have had to navigate between links and would have missed nuances.

The weak point was clear in the sleep(3000) test — I went for the "obvious" solution instead of the deterministic one. That kind of refinement still depends on someone who knows the CI environment questioning the approach, as you did.


Now let's see Copilot in action 🤖

@wellwelwel wellwelwel marked this pull request as ready for review March 15, 2026 05:41
@wellwelwel wellwelwel requested a review from Copilot March 15, 2026 05:42

This comment was marked as resolved.

wellwelwel and others added 2 commits March 15, 2026 02:58
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…tion

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@wellwelwel
Copy link
Copy Markdown
Collaborator Author

wellwelwel commented Mar 15, 2026

My personal human verdict 🤓

Claude Opus (the latest and most powerful model available today, with everything set to maximum)

  • It's amazing for creating complex reproductions of issues that are incomplete or very difficult to adapt for CI/CD environments.
  • As for the actual solution, it’s quite relative; reproducing the issue was much more complicated than fixing it, yet it still failed due to a minor detail.
  • As for the implementation of the tests themselves, the instructions created in docs: include instructions to LLM agents #4185 really made a huge difference, but I had to follow up a few times to make sure it ensured the test was truly resilient.

Copilot AI (review from GitHub website)

  • It picked up on the inconsistency that Claude hadn't "thought", but came up with a completely nonsensical recommendation for the tests, suggesting that he revert not only to being fragile, but in a way that was even worse than the first version of Claude.
  • The second suggestion required a slight adjustment, which Claude then made on its own.
  • Although it showed the problem and the fix, it wasn't able to generate a simple commit message.

@wellwelwel wellwelwel merged commit 7e57db6 into sidorares:master Mar 15, 2026
88 checks passed
@wellwelwel wellwelwel deleted the loop branch March 15, 2026 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

2 participants