[kdcokenny]

related to #141.

get() owns its connection attempt and returns errors when retries are disabled or error_is_fatal() is true. temporary failures still retry. cancelling get() drops the attempt and releases capacity.

the no-retry test now expects User(Error) instead of TimedOut. requires tokio 1.26 for the Notify fix.

i manually verified red/green e2e runs for checkout and postgres, both mutexes, rust 1.75 and minimum tokio.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.08%. Comparing base (facae9e) to head (86f8e5b).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #321      +/-   ##
==========================================
+ Coverage   77.99%   83.08%   +5.08%     
==========================================
  Files           6        6              
  Lines         659      739      +80     
==========================================
+ Hits          514      614     +100     
+ Misses        145      125      -20     

☔ View full report in Codecov by Harness.
📢 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.

[djc]

You can pick one of per-commit or per-PR review. This seems to have some small commits that are independent but also changes that are spread over multiple commits, which make them harder to review.

[kdcokenny]

You can pick one of per-commit or per-PR review. This seems to have some small commits that are independent but also changes that are spread over multiple commits, which make them harder to review.

Ah yes I intended per PR review. If you'd like me to squash these commits lmk!

[djc]

In its current shape I don't think I'll be able to review this change -- it has too much going on without it being obvious why things are going on. It might work better if you can break it down into a series of smaller changes, each of which should have clear justification.

[kdcokenny]

In its current shape I don't think I'll be able to review this change -- it has too much going on without it being obvious why things are going on. It might work better if you can break it down into a series of smaller changes, each of which should have clear justification.

got it. i split this into 4 commits so you can go through them in order:

1. fix a background connection failure not waking a caller waiting for capacity. confirmed this fails on upstream too
2. extract the retry loop without changing behavior
3. add the fatal-error hook and its tests
4. return connection errors to get(), keeping the cancellation/wakeup handling and regression tests together

is this more of what you were looking for? if there's anything else i can do to make it easier to review lmk.

[djc]

The first 3 commits seem fine. I'm still not sure why the 4th commit is useful/related.

[kdcokenny]

the first 3 commits stop retries for an attempt, but get() still waits on Notify while the error goes to the sink, so persistent failures can still become TimedOut. commit 4 lets the checkout own the attempt and return its actual error. the cancellation/capacity handling preserves pool behavior around that change.
other than that, i've reordered the tests locally. the semicolon nit works on commit two, but commit four adds the statistics guard, so rustfmt keeps the block and restores the semicolon. i've left that as-is

[djc]

commit 4 lets the checkout own the attempt and return its actual error.

Still not sure what you mean by this.

[kdcokenny]

commit 4 lets the checkout own the attempt and return its actual error.

Still not sure what you mean by this.

by checkout i mean the pool.get() call. currently, creating a connection happens in a separate task, and get() only waits for a notification, so it never receives that task's connection error.
commit 4 keeps the connect_with_retry() future inside get() instead, polling it alongside notifications. that lets get() return RunError::User(e) when retries are disabled or the error is fatal. also when i say "owns the attempt" i mean that dropping get() also drops the connection future

[djc]

Well, I don't understand commit 4, so I won't be merging it in its current state. Perhaps there's a way to restructure it to make it more clear, or break it down into smaller pieces that make it easier to understand.