refactor(pool): inline item close and must-delete session logic

[kprokopenko]

Summary

• Remove closeItemFunc and mustDeleteItemFunc hooks from internal/pool configuration.
• Call item.Close directly when closing pool items and use xerrors.MustDeleteTableOrQuerySession in checkItemAndError.
• Drop duplicated WithMustDeleteItemFunc setup from table/query clients and update pool tests to use BAD_SESSION errors instead of custom predicates.

Test plan

• go test ./internal/pool/ -count=1
• go test ./internal/table/ -count=1
• go test ./internal/query/ -count=1
• golangci-lint run ./internal/pool/ ./internal/table/ ./internal/query/

Made with Cursor

[github-actions]

summary

Inferred base version: v3.143.0
Suggested version: v3.143.1

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.32%. Comparing base (9efa606) to head (8ef3bc3).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2232      +/-   ##
==========================================
- Coverage   75.40%   75.32%   -0.09%     
==========================================
  Files         445      445              
  Lines       45996    45981      -15     
==========================================
- Hits        34684    34635      -49     
- Misses       9352     9385      +33     
- Partials     1960     1961       +1     

Flag	Coverage Δ
experiment	75.23% <100.00%> (-0.04%)	⬇️
go-1.21.x	72.20% <100.00%> (-0.09%)	⬇️
go-1.26.x	75.32% <100.00%> (-0.04%)	⬇️
integration	56.89% <100.00%> (-0.10%)	⬇️
macOS	45.21% <100.00%> (+<0.01%)	⬆️
ubuntu	75.32% <100.00%> (-0.09%)	⬇️
unit	45.21% <100.00%> (-0.01%)	⬇️
windows	45.20% <100.00%> (-0.01%)	⬇️
ydb-24.4	56.49% <100.00%> (-0.02%)	⬇️
ydb-edge	56.69% <100.00%> (-0.11%)	⬇️
ydb-latest	56.61% <100.00%> (-0.09%)	⬇️
ydb-nightly	75.23% <100.00%> (-0.04%)	⬇️

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

☔ 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.

[robot-vibe-db]

AI Review Summary

Verdict: ✅ No critical issues found

Critical issues

No critical issues found.

Other findings

• Nit | Low: The comment block in pool_test.go:571-573 ("Count closes to detect context-cancelled failures…") was moved from the old closeItemFunc override into the onClose callback definition. In the new location the comment describes why closes are counted, which is fine, but the remark about gRPC context-cancelled behavior is less relevant to the onClose factory and slightly misleading — testItem.Close() does not actually use the context. Consider shortening to just the counting rationale. — internal/pool/pool_test.go:571

Detailed analysis:

The refactoring is behaviorally equivalent for all production callers:

1. mustDeleteItemFunc removal: The old callers (table/client, query/client) all passed the same predicate: !s.IsAlive() || (err != nil && MustDeleteTableOrQuerySession(err)). The IsAlive() part was redundant because checkItemAndError already checks !item.IsAlive() as its first operation. The err != nil guard was redundant because checkItemAndError returns early when err == nil. So the inlined xerrors.MustDeleteTableOrQuerySession(err) call is exactly equivalent.

2. closeItemFunc removal: The default implementation was _ = item.Close(ctx), and no caller ever overrode it. The inline _ = item.Close(closeCtx) is identical.

3. Test updates: Tests now use xerrors.Operation(xerrors.WithStatusCode(Ydb.StatusIds_BAD_SESSION)) instead of custom error+predicate combinations. BAD_SESSION is both naturally retryable (via operationError.Type() → TypeRetryable) and detected by MustDeleteTableOrQuerySession, so the test semantics are preserved.

4. //nolint:funlen removal: Both pool.New() and query.newWithQueryServiceClient() are now under the default funlen threshold (~56–57 lines), making the nolint directives unnecessary.

No remaining references to the removed API (WithMustDeleteItemFunc, closeItemFunc, mustDeleteItemFunc) exist in the codebase.

---

This review was generated automatically. Critical issues require attention; other findings are advisory.
If this comment was useful, please give it a 👍 — it helps us improve the review bot.

[robot-vibe-db]

Full analysis log

Analysis performed by claude, claude-opus-4-6.