Skip to content

stop_statement(): string-name path skips the terminal-phase short-circuit that the Statement-object path gets #203

Description

Connection.stop_statement() (src/confluent_sql/connection.py:1404) accepts either a statement
name or a Statement object, and its docstring frames these as interchangeable ("The name of the
statement to stop, or a Statement object"). They aren't: passed a Statement whose cached phase
is already terminal (STOPPED/COMPLETED/FAILED/DELETED), it returns immediately with no server call
(line 1443-1449); passed a plain string, it always issues the PATCH regardless of the statement's
actual current state (line 1457 falls straight through to _request).

This showed up in review of #202
(jbreeden's comment):
examples/oauth_data_plane_token_refresh_example.py's cleanup called stop_statement() with a
bare name captured once, so a statement that reached FAILED on its own (ending the polling loop)
would blow up mid-cleanup instead of short-circuiting. The example now tracks a Statement object
refreshed after every poll instead (avoiding the bug there), but that's a workaround at the call
site, not a fix to the asymmetry itself -- any other caller that reaches for a name instead of
holding onto the Statement object hits the same trap.

Root cause (confirmed live against a real environment)

The original hypothesis here was that the server rejects the stop PATCH when the target is already
terminal, and that the string path had no defense against that rejection. Confirmed live that
this isn't what happens
: the stop PATCH is fully idempotent server-side and always returns
200 OK, even against a statement that's already in a terminal phase (including FAILED) -- it just
echoes back the statement's current phase, with spec.stopped flipped to true regardless.
There's no "already terminal" rejection to special-case.

The actual bug is in Connection._wait_for_statement_stopped(): its FAILED check
(raise_if_failed) ran before its terminal-phase check on the very first look at the PATCH
response. So a statement that had already reached FAILED on its own -- before stop_statement()
was ever called -- got misdiagnosed as "transitioned to FAILED while stopping" and raised
OperationalError, instead of being treated as the same "already terminal, nothing to stop"
success the Statement-object short-circuit already returns for a cached terminal statement. This
only bites wait_for_stopped=True (the default); wait_for_stopped=False returns the PATCH
response directly without ever reaching this code, so it was never affected.

Fix

A one-line reorder in _wait_for_statement_stopped: check phase.is_terminal before checking
is_failed on the statement already held (the PATCH response). A genuine transition into FAILED
partway through the wait (observed by a poll issued after that initial check has passed) still
raises, unchanged -- only the very first look was affected.

Testing

  • Unit: test_blocking_returns_without_polling_when_patch_already_failed in
    tests/unit/test_connection_unit.py -- a PATCH response already in phase FAILED returns cleanly
    with no follow-up poll, instead of raising.
  • Integration: test_stopping_statement_that_already_failed_returns_without_raising in
    tests/integration/test_connection.py -- submits SELECT 1/0 (a fast, deterministic way to
    force a statement to FAILED on its own), then calls stop_statement(name) on it and asserts it
    returns rather than raising.
  • Both verified red (reproducing the original bug) before the fix and green after, against a live
    Confluent Cloud environment for the integration case.
  • Existing tests for the non-buggy paths (mid-wait transition to FAILED, blocking poll to STOPPED,
    COMPLETED racing a bounded query's stop, the Statement-object short-circuit, re-stopping an
    already-STOPPED statement by name) all remain green, confirming those behaviors were correct
    all along and unaffected by this fix.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions