[cnathe]

Rationale

This PR makes the server reliably stop a read-only request's database work when the request times out, the client disconnects, or the server shuts down. Previously an interrupt stopped a query only on the AsyncQueryRequest path. Synchronous queries such as selectDistinct and the Summary Statistics dialog kept running in Postgres until they finished, and an interrupted async request could leave its Tomcat thread holding an aborted connection that failed the thread's next request with a 25P02 error.

Tested with the new AsyncQueryRequest and RequestCancellation integration tests and manually against slow test queries: timeouts on async grids and on selectDistinct, a repeated-interrupt soak with no leaked connections, server shutdown with a running query, a long export under a short timeout, and both summary statistics paths.

Related Pull Requests

• None

Changes

• An interrupted async query now waits for its worker to release the shared connection, so the request thread's next query no longer fails.
• Timed-out, disconnected, and shutdown requests cancel their running statements in the database, including those run by async workers, without needing a free pool connection.
• A cancel can no longer reach the next request that reuses the same thread.
• Each cancel is logged and counted in usage metrics by source (timeout, client disconnect, shutdown).
• Exports are exempt from the read-only request timeout.
• Cancelled queries no longer log as server errors, and a timed-out request that hasn't started its response returns a 503 explaining it was cancelled.

[labkey-jeckels]

I wonder if we should also try an unpooled connection if we fail to get one because the connection pool is maxed out. That's a scenario when we're probably most in need of being able to kill it.

[cnathe]

Per claude: changed to use scope.getUnpooledConnection() instead of getPooledConnection(). An exhausted pool no longer makes the cancel wait up to maxWaitMillis (120 s). Pipeline job cancels go through this code too, so they also stop depending on the pool.

[cnathe]

@labkey-jeckels I pushed changes per your CR feedback and some logging consolidation. I reran some local manual test cases with my local HTTP/2 poxy in place.

@labkey-tchad I'm putting together a manual test plan now and will let you know when it is ready.

[cnathe]

@labkey-tchad here is the test plan for this story, including setup steps (will callouts to places that claude can help): https://claude.ai/artifact/7Bo6d5SKVKFaypKybAJE6M?sk=NxhsaaAw_EvO_8v0hYztXw. Let me know if you have any questions on the setup or test cases.

[labkey-jeckels]

Overall looking very good. A few small nits and a couple of concerns related to how connections get handled post-cancellation.

One new idea to consider: if we cancel a query on a connection, via any of the cancellation approaches, yank it from the pool. That eliminates potential race conditions from reusing it, which was tricky in my previous work (which was much more limited in scope) in this area

[labkey-jeckels]

This method name keeps tripping me up. I understand what it's trying to convey, but it sounds like it's cancelling queries that aren't using a connection, like they were leaked.

I don't love it, but maybe something like cancelQueriesViaOwningConnection()

[cnathe]

done

[labkey-jeckels]

I think this is a Postgres-specific setting. I doubt it would do harm on other DBs, but consider switching to a dialect method.

[cnathe]

done

[labkey-jeckels]

This is running via shutdownPre(), which will fire before all other listeners have had a chance to start their shutdown sequence and might be mid-query. This connection killing probably belongs in shutdownStarted() instead

[cnathe]

From Claude:
Moved the cancel into shutdownStarted(), just before the existing forced close of the same connections, so every listener's shutdownPre() runs first. It still runs early in the shutdownStarted() pass, since this listener registers when ConnectionWrapper loads; that's the same point where develop already force-closes connections more than 2 s old.

[labkey-jeckels]

Claude review is concerned that this could match against HY000 which isn't specifically a cancel. It's probably a remnant of when we used jTDS and can be removed from the SQLServer dialect's check now.

[cnathe]

Done, dropped HY000. From Claude: HY000 is SQL Server's generic error state, not a cancel; it was a jTDS leftover.

[labkey-jeckels]

Claude suggests detaching the thread from the connection here, with a call like:

DbScope.detachConnectionHolders(Thread.currentThread());

where the new method in DbScope looks like:

/** Next connection request from the thread gets a new holder; wrappers already handed out still release through their own */                                                                                                            
  static void detachConnectionHolders(Thread thread)                                                                                                                                                                                       
  {
      for (DbScope scope : getInitializedDbScopes())                                                                                                                                                                                       
      {
          synchronized (scope._threadConnections)
          {
              scope._threadConnections.remove(thread);
          }
      }
  }

[cnathe]

Done

[labkey-jeckels]

The pg_cancel_backend fallback can't reach the request thread's own connections. closeConnectionsForCurrentThreadWithoutReleasingLocks() closes them inside the lock, so by the time cancelQueriesWithNewConnection runs they're no longer allocated and it skips them. If the driver cancel fails (e.g. the cancel socket times out, or accessToUnderlyingConnectionAllowed=false so getPhysicalConnection() isn't a BaseConnection), a timed-out synchronous query like selectDistinct keeps running in Postgres and nothing is logged. That's the case this PR is meant to fix.

Suggest running the fallback inside the lock, before the close:

Map<DbScope, Set<ConnectionWrapper>> unsignalled = RequestCancellation.cancelQueriesWithoutConnection(ConnectionWrapper.getConnectionsByScopeForEffectiveThread(thread), source, cancelled, first);

// Before the close below: once a wrapper returns to the pool its SPID may belong to another request, so the fallback skips it
RequestCancellation.cancelQueriesWithNewConnection(unsignalled, source, cancelled, first);

try (DbScope.ConnectionSharingCloseable ignored = DbScope.shareConnections(thread, Thread.currentThread()))
{
    DbScope.closeConnectionsForCurrentThreadWithoutReleasingLocks();
    ...

When the driver cancel works, unsignalled is empty, so normal requests aren't affected. When it doesn't, the request can't exit until its query is cancelled anyway, so holding the lock while the unpooled connection opens doesn't delay anything. Recording SPIDs and cancelling after the close would hit the reuse race that the isAllocated() check is there to prevent. Is there a connect/login timeout on getUnpooledConnection()? Without one, an unreachable database would block the sweep inside the lock.

A test with the driver cancel unavailable (empty set from cancelQueriesWithoutConnection, or a scope with accessToUnderlyingConnectionAllowed=false) that runs a synchronous pg_sleep and checks it fails with 57014 would cover this path.

[cnathe]

From Claude:

Moved the pg_cancel_backend fallback inside the lock, before anything can release the thread's connections, and added TransactionFilter.TestCase.testCancelFallback, which disables the driver cancel and checks that a synchronous pg_sleep fails with 57014. There was no connect timeout (pgjdbc's loginTimeout defaults to waiting forever), so the fallback connection now sets loginTimeout and socketTimeout to 5 s. This covers a driver cancel that can't be sent at all; pgjdbc swallows an IOException from the cancel socket, so a cancel that times out still counts as signalled and skips the fallback.

The new test also exposed a bigger problem: closeConnectionsForCurrentThreadWithoutReleasingLocks() returned the request thread's connection to the pool while the thread was still mid-call on it, and the thread's own release then threw ConnectionAlreadyReleasedException, hiding the 57014. I removed that close: the cancel makes the thread unwind and release its connections itself, and a thread holding no connections is still banned from getting new ones, as before.

[cnathe]

Overall looking very good. A few small nits and a couple of concerns related to how connections get handled post-cancellation.

One new idea to consider: if we cancel a query on a connection, via any of the cancellation approaches, yank it from the pool. That eliminates potential race conditions from reusing it, which was tricky in my previous work (which was much more limited in scope) in this area

Took your suggestion: a cancelled connection is now discarded when it's released instead of going back to the pool, so a late cancel can't hit whoever reuses that backend. Along the way claude removed the timeout sweep's forced close of the request thread's connections, which was returning connections to the pool while they were still in use.

I'm doing a round of manual testing with the HTTP/2 proxy in place before I push these changes. I'll ping you once that is done.

[labkey-tchad]

Manual test results:

• ❌ T2: Async grid, timeout
  • Grid just spins, no error message. query-getQuery.api response is truncated: {"schemaName" : [ "core" ], "queryName" : "SlowCount"
• ❌ T5: Saved summary statistics in the grid footer, timeout
  • Grid is messed up: An unexpected error occurred in the data region status panel. Server stacktrace in grid footer.

Other findings:

• SDMS Data region HTML contains extra whitespace for long running queries
• App rowcount query (query-getQuery with maxRows=0, includeTotalCount=true) can be significantly slower than loading all data via query-executeQuery: (26 minutes vs 16 seconds).

[cnathe]

I spoke to Trey about the manual testing results. The notes on T2 and T5 were as expected for this story 1 dev changes. The test plan was misleading on what the expected result should be. The two other findings are unrelated to this PR. We should get a separate issue open for the "extra whitespace" part and look into addressing that separately.

@labkey-jeckels you should be all clear for your next code review pass.

[labkey-jeckels]

A few more ideas. I don't think any are strictly required.

[labkey-jeckels]

This will apply to Redshift too, so it might belong in PostgreSql92Dialect. Not sure about the other properties above.

[cnathe]

moved the cancelSignalTimeout default to PostgreSql92Dialect

[labkey-jeckels]

This may only be used for non-Postgres at this point. If so, should we eliminate it?

[cnathe]

keeping it. it's the only way to stop a request blocked on an external SQL Server or Snowflake scope.

[labkey-jeckels]

This should probably route through RequestCancellation.cancelQueries() to benefit from the discard pathway, etc.

[cnathe]

Done. cancelForJob goes through RequestCancellation.cancelQueries with a new PIPELINE source. tested locally

[labkey-jeckels]

Cancellation can potentially take a while if we hit all the timeouts (2s for the initial cancel, 5s each for connecting and reading on the new connection). We could potentially move some of that out of the synchronized block.

[cnathe]

Done. The fallback now opens its unpooled connection outside it and only takes the lock to re-check the tracker and run pg_cancel_backend.

[labkey-jeckels]

I had a brainstorm after approving - wouldn't things be simpler if we flipped responsibilities for the async query execution. Have the main HTTP thread do the DB queries, and have the ancillary/async thread do the pinging. This could even be done by a virtual thread. This could also be handled as a separate PR, of course.

Rough plan compliments of Claude:

Most of the hardening in this PR exists because a worker thread shares the request thread's connections and can outlive the request. If we swapped the roles, so waitForResult runs the callable on the request thread and a helper thread writes the keep-alive spaces and cancels the statement when the client disconnects, the sharing would go away. A lot of that code would go with it:

• awaitWorkerAfterInterrupt, the 10 s stuck-worker path, and DbScope.detachConnectionHolders. That also removes the case where the detach drops the request thread's own outer connection (for example, one holding a temp table).
• The ConnectionHolder stored on each ConnectionWrapper, and the special holder lookup in DbScope's thread-connection close path.
• DbScope.shareConnections and the effective-thread connection lookups. AsyncQueryRequest is their only production caller.
• Copying query and logging state to the worker: QueryService environment cloning, the MemTracker merge, the log context, and handing off the Datadog span.

The only callables are TableSelector.getResults and getAggregates, and neither writes to the response, so the swap looks contained. Things to handle:

1. Stop the helper before the request writes its response. Each ping's write and flush would run under a lock with a stopped flag checked inside it, and the request thread sets that flag under the same lock in a finally before returning. Without this, a late ping could corrupt the response or write into a recycled Tomcat response that now belongs to another request.
2. The request thread is no longer protected from a dead socket. Today the interrupt wakes it from wait() even if the database cancel never arrives. After the swap, it would hang in the socket read the same way the synchronous paths (selectDistinct, etc.) already can. That makes it necessary to close the physical connection when cancels keep failing, for example once request-cancel-stuck fires. The synchronous paths need that anyway.
3. Clear the interrupt flag after a cancel, since wait() will no longer absorb it.
4. Use a virtual thread for each helper rather than one shared scheduler. With a shared scheduler, one client whose connection has stopped accepting data blocks pings for everyone else.
5. Keep the AsyncRequest span as a child span on the request thread, so existing dashboards and monitors keep working.

[cnathe]

FYI, I'll fold this comment about the larger role-swap comment into the implementation plan and we can come back to it after dev story 2.