Skip to content

fix(csharp): surface closed/expired sessions as a clear, typed error - #510

Open
jadewang-db wants to merge 1 commit into
mainfrom
fix/csharp-session-expired-typed-error
Open

fix(csharp): surface closed/expired sessions as a clear, typed error#510
jadewang-db wants to merge 1 commit into
mainfrom
fix/csharp-session-expired-typed-error

Conversation

@jadewang-db

Copy link
Copy Markdown
Collaborator

When a server-side session is closed or times out due to inactivity, the next Thrift call returns HTTP 400 "Invalid SessionHandle". The driver previously surfaced this as a misleading "An unexpected error occurred while fetching results / Couldn't connect to server" wrapper, and kept reusing the now-stale handle so every subsequent call failed the same opaque way until the caller disposed and re-created the connection.

Tier 1 (clear, typed error): detect the closed/expired-session signature and surface it as a typed DatabricksSessionExpiredException (AdbcStatusCode.InvalidState) with a clear message, preserving the original error as InnerException for diagnostics.

Tier 2 (fail fast): mark the connection's session invalid on first detection so subsequent statements fail fast with the same typed error instead of reusing the dead handle.

Detection walks the inner/aggregate exception chain for the stable server phrase "Invalid SessionHandle" (covers both the inactivity-timeout "Session [..] is closed" and explicit-close variants) and is careful not to misclassify connectivity errors or invalid operation handles.

Tests:

  • Unit tests for the classifier (no workspace required, runs in CI).
  • E2E test that reproduces the condition on demand by closing the session out-of-band, asserting both the typed Tier 1 error and Tier 2 fast-fail.

When a server-side session is closed or times out due to inactivity, the
next Thrift call returns HTTP 400 "Invalid SessionHandle". The driver
previously surfaced this as a misleading "An unexpected error occurred
while fetching results / Couldn't connect to server" wrapper, and kept
reusing the now-stale handle so every subsequent call failed the same
opaque way until the caller disposed and re-created the connection.

Tier 1 (clear, typed error): detect the closed/expired-session signature
and surface it as a typed DatabricksSessionExpiredException
(AdbcStatusCode.InvalidState) with a clear message, preserving the
original error as InnerException for diagnostics.

Tier 2 (fail fast): mark the connection's session invalid on first
detection so subsequent statements fail fast with the same typed error
instead of reusing the dead handle.

Detection walks the inner/aggregate exception chain for the stable server
phrase "Invalid SessionHandle" (covers both the inactivity-timeout
"Session [..] is closed" and explicit-close variants) and is careful not
to misclassify connectivity errors or invalid *operation* handles.

Tests:
- Unit tests for the classifier (no workspace required, runs in CI).
- E2E test that reproduces the condition on demand by closing the session
  out-of-band, asserting both the typed Tier 1 error and Tier 2 fast-fail.

Co-authored-by: Isaac
/// and the explicitly-closed variant ("Invalid SessionHandle: SessionHandle [..]")
/// contain this phrase.
/// </summary>
internal const string ServerErrorSignature = "Invalid SessionHandle";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is there no better way to identify this?


public override QueryResult ExecuteQuery()
/// <summary>
/// Tier 2 fast-fail: if the connection's server-side session is already known to be

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Tier 2 fast-fails emit no telemetry

@msrathore-db msrathore-db left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We need to do the same for metadata and fetch paths also right?

@eric-wang-1990

Copy link
Copy Markdown
Collaborator

I think this logic should happen in the HiveServer2Connection class: https://github.com/adbc-drivers/hiveserver2/blob/main/csharp/src/AdbcDrivers.HiveServer2/Hive2/HiveServer2Statement.cs#L360C85-L360C105.

We can change the SessionHandle to something like this:

internal TSessionHandle SessionHandle
{
    get
    {
        if (_isSessionInvalidated)
        {
            throw new AdbcException(
                "The session was closed by the server.");
        }

        return _sessionHandle
            ?? throw new AdbcException(
                "Connection is not open.");
    }

    private set => _sessionHandle = value;
}

Then this will guard for every Thrift operation and we do not need to manually edit the callsite one by one.
WDYT?

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.

3 participants