Skip to content

Bound user cancellation by Cancellation Timeout while prepended responses are outstanding - #6663

Open
lahma wants to merge 2 commits into
npgsql:mainfrom
lahma:fix/cancel-with-prepended-unresponsive
Open

lahma wants to merge 2 commits into
npgsql:mainfrom
lahma:fix/cancel-with-prepended-unresponsive

Conversation

@lahma

@lahma lahma commented Oct 2, 2026 •

Copy link
Copy Markdown

Fixes #6662

When the user's token fires while a prepended query (DISCARD ALL on a pooled connection) is still unanswered, PerformImmediateUserCancellation defers the cancellation and returns without arming a deadline. If the server never answers, the token has no effect until Command Timeout.

What this changes:

Limitation: a synchronous receive that is already blocked isn't interrupted, because a ReceiveTimeout change doesn't affect it. That read still ends at Command Timeout. It then breaks without sending a cancel request, and the sync reads after it are bounded by Cancellation Timeout. Main has the same limit for sync hard cancellation of non-prepended commands. Interrupting the blocked receive is #5070.

How I tested it:

  • New CommandTests.Cancel_async_with_prepended_query_server_unresponsive (PgPostmasterMock, Command Timeout=30, Cancellation Timeout=1000). It fails on main after 31 s and passes with this change in about 1 s. It also checks that no cancel request reaches the server.
  • New CommandTests.Cancel_sync_with_prepended_query_server_unresponsive(1000, -1): sync Cancel(). The server sends part of the DISCARD ALL response and then stalls. It takes 30–31 s on main and about 1 s with this change. It also checks that no cancel request is sent.
  • These pass locally against PostgreSQL 17: CommandTests, PoolTests and the tests matching Cancel (117 passed, 3 skipped), and the tests matching Timeout/Prepend (12 passed, 1 skipped).
  • Batched_small_then_big_statements_do_not_deadlock_in_sync_io hangs in my local setup (Windows + Docker), and it hangs on unmodified main too, so I excluded it.
  • Freezing-proxy repro from the issue: with defaults and the token canceled at 3 s, the command now returns at 5.0 s instead of 60 s. The no-token Command Timeout path is unchanged.

@lahma
lahma requested review from roji and vonzshik as code owners October 2, 2026 13:18
Copilot AI balanced review requested due to automatic review settings October 2, 2026 13:18

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Immediate cancellation does not interrupt a synchronous read blocked on a prepended response.

Review effort: Balanced
Findings: 1 High severity

Open (1)
What changed in this PR

Bounds deferred cancellation while prepended responses remain unread.

Changes:

  • Arms Cancellation Timeout for deferred cancellation.
  • Prevents PostgreSQL cancellation of prepended queries.
  • Adds an asynchronous unresponsive-server regression test.
File Description
NpgsqlConnector.cs Implements deferred cancellation deadlines.
NpgsqlReadBuffer.cs Suppresses unsafe PostgreSQL cancellation.
CommandTests.cs Tests asynchronous deferred cancellation.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/Npgsql/Internal/NpgsqlConnector.cs Outdated
…ion Timeout -1

A zero Socket.ReceiveTimeout disables the timeout, so synchronous reads after the deferred cancellation were unbounded. Add a synchronous prepended-query cancellation test.
Copilot AI balanced review requested due to automatic review settings October 2, 2026 13:50

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The focused implementation addresses the reported cancellation delay and includes coverage for synchronous and asynchronous paths.

Review effort: Balanced
Findings: None

Resolved since last review (1)

This branch has not been deployed

No deployments
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.

CancellationToken has no effect until Command Timeout while the server doesn't answer the prepended DISCARD ALL

2 participants