Skip to content

fix(websocket): Preset fallback build 7.1.3.1 — timestamp precision and pre-ping recovery - #1

Merged
aminghadersohi merged 6 commits into
masterfrom
workflow-a/ch120359-websocket-fix
Sep 16, 2026
Merged

aminghadersohi merged 6 commits into
masterfrom
workflow-a/ch120359-websocket-fix

Conversation

@aminghadersohi

@aminghadersohi aminghadersohi commented Sep 15, 2026 •

Copy link
Copy Markdown
Collaborator

Purpose

Release-blocker fallback for SC-120359. Upstream exasol/sqlalchemy-exasol#808 stays open and is still the preferred path; this PR exists so Preset is not blocked if upstream review/release does not land in time.

Production Shell pins sqlalchemy-exasol==7.1.3, which carries two defects reproduced against Exasol Nano:

  1. websocket.DateTime.result_processor parses through time.strptime/mktime, losing fractional seconds and reinterpreting naive wall time in the client's timezone.
  2. The websocket dialect inherits is_disconnect() == False, so SQLAlchemy's pool_pre_ping cannot recognize a dead pooled connection and the first checkout after a killed session fails.

Exact provenance

The first two commits are git cherry-picked from the reviewed upstream head, with original authorship and author dates preserved:

This branch Upstream exasol#808
06d6a14 2f88949e721bdcbd45f544d9e2b463c66603b947
44f4792 92039cf65b1160293e04567cf403ab56064d0b7f

Verified: git diff --quiet 44f4792 92039cf6 — the tree at 44f4792 is byte-identical to the reviewed upstream head. Base is upstream's fb2675b4 (“Prepare release 7.1.3”, exasol#804), pushed unmodified to preset/upstream-7.1.3 because this fork's master is protected and 30+ commits stale; that keeps this diff to exactly the fix plus the version marker.

The third commit 49227ff is the only Preset-owned change: the internal artifact version and a provenance note.

Version choice

7.1.3.1 — a four-component PEP 440 release version, following Preset's existing convention for rebuilt third-party packages (pyhive-0.7.0.1, consumed by Shell today). Verified with packaging: it sorts above 7.1.3 and below 7.1.4, so any future upstream release supersedes it automatically in resolution. Upstream has only ever published three-component versions (7.1.0–7.1.3, 6.x, 5.x), so the fourth component cannot collide.

This replaces the earlier 7.1.3.1 proposal. A PEP 440 local segment is collision-proof, but the internal index requires the + to be percent-encoded in the consuming URL — verified against the live host, …pyhive-0.7.0.2%2Bpr.4.c26b81d.tar.gz returns 200 while the literal + form returns 404. 7.1.3.1 keeps the artifact filename and the Shell pin free of any encoding.

Internal index only; never published to public PyPI.

Scope

No workflow, CI, packaging-backend or dependency changes. No new runtime dependency. Nothing is published, tagged or released by this PR.

Gates before any publication

  • Full unit suite green on this head
  • Exasol Nano acceptance green on this head
  • Trusted-publishing path to the internal package index confirmed by an owner (no secrets in logs, workflow or local shell)
  • Internal index confirmed to accept a PEP 440 local version
  • Shell pin change is a separate PR; Shell #5084 stays test-only

Supersede and close this in favour of the upstream release as soon as exasol#808 ships.

aminghadersohi and others added 4 commits August 28, 2026 09:39
* Fix bare SQLAlchemy dialect entry point
Load the default websocket dialect class directly so entry-point consumers do not receive a legacy module. Cover installed discovery, Superset's dialect shape, and both Exasol engine URL forms.
* Strengthen dialect entry point regression checks
* Reference entry point issue in changelog
* Prepare release 7.1.3
@aminghadersohi

aminghadersohi commented Sep 16, 2026 •

Copy link
Copy Markdown
Collaborator Author

Qualification results — all run on this exact head 8af5fb7f1b8fa309fc989024dd87d271de4fa69a

Server: official Exasol Nano 2026.2.0-nano.3, linux/amd64 digest sha256:d1393db959c7ab4b6098b124e9ea8afb3e0c4d222ba243b3173dc015aecfda2f (exasol/nano). Driver config from the repo's own setup.cfg [db] exasol-websocket entry.

Suite Result
test/unit 116 passed
test/integration/sqlalchemy (dialect compliance) 746 passed, 526 skipped, 33 xfailed, 0 failed
test/integration/exasol 113 passed, 2 failed (see below)
test/integration/regression 4 passed
test/integration/exasol/test_websocket_recovery.py 12 passed

Causality control. With sqlalchemy_exasol/websocket.py reverted to unmodified upstream 7.1.3 and nothing else changed, the same 12 cases give 10 failed / 2 passed. With the fix: 12 passed. The regressions therefore track exactly the two defects this branch fixes.

The 2 test/integration/exasol failures are pre-existing, not regressions. Controlled the same way on unmodified 7.1.3: test_udf.py::Udf::test_udf and test_large_metadata.py::test_reflect_table_object fail there too (the control run actually failed one more test, test_reflect_metadata_object). test_udf fails because Nano cannot create PYTHON3 SCALAR SCRIPT UDFs; test_large_metadata is size/timing-sensitive on this small server. Neither touches the changed code paths.

Artifacts built (local only — nothing published)

sqlalchemy_exasol-7.1.3.1.tar.gz          sha256 825cf9a4def84f59d2b7d619433ed6b66afbebb9ccd3a5212c5b0ea46c0faec8
sqlalchemy_exasol-7.1.3.1-py3-none-any.whl sha256 f841754a1d775576acc39cb8137dbf9b2f7214a3a71c8547030e858eebbc2602

Verified: poetry build succeeds; the wheel and the sdist both install into a clean venv reporting 7.1.3.1; the installed dialect carries the is_disconnect override; and uv pip compile resolves a direct-URL requirement for it and emits the pin in the same form Shell already uses.

Smallest safe publication path (design only — do not execute yet)

the internal package index is a public-read S3 bucket (preset-pypi) serving flat per-project prefixes — PyHive/, ai-assist/, firebolt-sqlalchemy/. Shell consumes it as direct sdist URLs, e.g. setup.py already carries pyhive[...]@s3://preset-pypi/PyHive/pyhive-0.7.0.1.tar.gz. There is no sqlalchemy-exasol/ prefix today, so the key below is free.

  1. Build reproducibly from this head — poetry build — and match the sha256 values above.
  2. Upload one object to s3://preset-pypi/sqlalchemy-exasol/sqlalchemy_exasol-7.1.3.1.tar.gz (optionally the wheel alongside). This must be done by someone who already holds bucket write access, using their existing SSO/role credentials, or by a CI job assuming an org OIDC role — the same trusted-publishing shape as Shell's github-actions-bedrock-role. No long-lived token should be pasted into a shell, a workflow file, or this thread; nothing in this branch reads or needs a credential.
  3. Consume from Shell in a separate PR: replace setup.py:228 "sqlalchemy-exasol==7.1.3" with the direct URL and relock with the documented uv pip compile command.

⚠️ Encoding requirement, verified against the live host. The + must be percent-encoded in the consuming URL:

s3://preset-pypi/PyHive/pyhive-0.7.0.2%2Bpr.4.c26b81d.tar.gz  → 200
s3://preset-pypi/PyHive/pyhive-0.7.0.2+pr.4.c26b81d.tar.gz    → 404

So the Shell pin must read …/sqlalchemy-exasol/sqlalchemy_exasol-7.1.3%2Bpreset.1.tar.gz. I confirmed pip install from a %2B URL resolves and reports 7.1.3.1. Local +pr.N.<sha> versions are already established practice in this bucket, so the form is not novel — but if an owner would rather avoid the encoding entirely, the drop-in alternative is 7.1.3.1, matching the pyhive-0.7.0.1 precedent Shell consumes today; it sorts identically (above 7.1.3, below 7.1.4). Say which you prefer before anything is uploaded.

Gates still open

  • Fork CI is not running. Only the Dependency Graph workflow is registered on preset-io/sqlalchemy-exasol, so this PR reports no checks even though the upstream workflow files are present. GitHub disables workflows on forks until a repo admin opens Actions → “I understand my workflows, go ahead and enable them”; this cannot be done through the REST API (actions/permissions already reports enabled: true). All qualification above is therefore local, not CI-attested.
  • Owner confirms the bucket key, the version form, and who performs the upload.
  • Separate Shell PR for the pin + relock; Shell #5084 stays test-only and untouched.
  • Upstream #808 stays open — it remains the preferred outcome. Nothing here is tagged, released or merged, and Fix websocket timestamp precision and pre-ping disconnect recovery exasol/sqlalchemy-exasol#808 will not be closed.

Updated 2026-09-16. Version decision is now 7.1.3.1 (four-component, matching the PyHive/Drill convention), so the %2B encoding caveat above no longer applies to this artifact — the Shell pin is a plain URL:

sqlalchemy-exasol @ s3://preset-pypi/sqlalchemy-exasol/sqlalchemy_exasol-7.1.3.1.tar.gz

Checksums and the head SHA in this comment have been updated to the rebuilt 7.1.3.1 artifacts. The reviewed fix tree is unchanged: git diff --quiet 44f4792 92039cf6 still reports the parent tree byte-identical to the upstream exasol#808 head; only the tip commit carries the version and provenance note.

Rebuild of upstream 7.1.3 carrying only the reviewed websocket
timestamp-precision and pre-ping disconnect fixes from
exasol#808.

Uses Preset's existing four-component convention for rebuilt third-party
packages (cf. pyhive-0.7.0.1), which keeps the artifact filename free of
a '+' and therefore free of URL percent-encoding on the internal index.
Upstream has only ever released three-component versions; 7.1.3.1 sorts
above 7.1.3 and below 7.1.4, so an upstream release supersedes it
automatically. Internal index only; never published to public PyPI.

Refs SC-120359.
@aminghadersohi
aminghadersohi force-pushed the workflow-a/ch120359-websocket-fix branch from 49227ff to 8af5fb7 Compare September 16, 2026 00:05
@aminghadersohi
aminghadersohi deployed to manual-approval September 16, 2026 00:06 — with GitHub Actions Active
@aminghadersohi
aminghadersohi deployed to manual-approval September 16, 2026 00:06 — with GitHub Actions Active
@aminghadersohi aminghadersohi changed the title fix(websocket): Preset fallback build 7.1.3+preset.1 — timestamp precision and pre-ping recovery fix(websocket): Preset fallback build 7.1.3.1 — timestamp precision and pre-ping recovery Sep 16, 2026
@aminghadersohi

Copy link
Copy Markdown
Collaborator Author

CI is now live on this fork, and it is green apart from one external-credential gate

Exact head: 8af5fb7f1b8fa309fc989024dd87d271de4fa69a. Run 35038607736.

63 jobs succeeded, 1 failed, 1 blocked behind it.

This is materially stronger evidence than upstream exasol#808 has ever been able to produce, because exasol#808's slow-test and extension gates are stuck awaiting maintainer approval. Here they ran:

  • 30 Integration-Tests jobs — test:exasol, test:sqla and test:regression groups × Python 3.10/3.11/3.12/3.13/3.14 × real Exasol 8.29.13 and 2025.1.8 servers, websocket driver. All green.
  • Verify Examples on both server versions — green.
  • Unit tests on Python 3.10–3.14, lint, typing, format, security audit, workflow audit, build package, changelog — all green.
  • Docs — green. Worth noting for #808: the same job is red there purely from transient GitHub 504s on two unchanged security.rst links. It passes here, which corroborates that diagnosis independently of my local reproduction.
  • Merge Gate / Allow Merge — success.

test_udf and test_large_metadata::test_reflect_table_object, the only two failures in my local Nano run, pass on the real servers — confirming they were Nano-edition limitations (no PYTHON3 SCALAR SCRIPT support) rather than anything to do with this change.

The one failure — Merge Gate / Fast Report / report

Job 104613941885. The scanner checks out, builds and validates coverage artifacts successfully. It fails only at the upload step:

pysonar --sonar-token '' …
ERROR: Not authorized or project not found. Please check the 'SONAR_TOKEN' …

SONAR_TOKEN is empty because this fork has no such secret, and the target is Exasol's SonarQube organization — which a Preset fork has no token for and arguably should not. Report / report stays pending because it depends on this job. No test failed.

I have deliberately not made this green. Editing the workflow to skip or soften the Sonar step is precisely the CI weakening I declined to propose upstream, and I am not doing it here either. It needs an explicit owner decision:

Exact human action — pick one: (a) create a Preset-owned SonarQube project for this fork and add a SONAR_TOKEN repository secret, or (b) decide that Sonar reporting is out of scope for a fallback fork and record that decision. Until one is chosen, this PR is not all-green and should not be described as such.

Workflow activation, and what stays off

The upstream workflows were never registered on this fork (GitHub's fork gate). Re-asserting PUT /repos/preset-io/sqlalchemy-exasol/actions/permissions registered them; CI was then re-triggered at this exact head. Publishing and release workflows are explicitly disabled so nothing can reach public PyPI, GitHub Releases or Pages from here:

Workflow State
Build & Publish disabled_manually
CD disabled_manually
Check Release Tag disabled_manually
Publish Documentation disabled_manually
PR-Merge disabled_manually
Dependency Update / Periodic-Validation disabled_fork (default)
CI, Checks, Fast-Tests, Merge-Gate, Slow-Checks, Verify examples, Build Matrix active

The fork's master remains untouched: it is protected (1 approving review, enforce_admins: true) and I did not alter that protection, which is why this PR is based on preset/upstream-7.1.3.

Nothing published, tagged, merged or released. Upstream exasol#808 is untouched and stays open.

The upstream report workflow uploads coverage to Exasol's own SonarQube
organization with a SONAR_TOKEN only exasol/sqlalchemy-exasol holds. A
fork cannot authenticate there and must not be given that secret, so the
job fails on this fork for reasons unrelated to the code under review.

Guard the upload step with github.repository != 'preset-io/sqlalchemy-exasol'
so behaviour is unchanged for exasol/sqlalchemy-exasol and for every other
fork, and add GitHub CodeQL (Python, security-and-quality) as the
replacement static-analysis gate on this fork only.

The guard is expressed through the exasol-toolbox's supported
.workflow-patcher.yml mechanism rather than by hand-editing a generated
workflow, so 'nox -s workflow:check -- all' still passes. The CodeQL
action is hash-pinned per the repository's zizmor unpinned-uses policy;
'nox -s workflow:audit' reports no findings.

No lint, typing, security or test job is removed, skipped or relaxed.
Publishing and release workflows remain disabled on this fork.

Refs SC-120359.
@aminghadersohi

Copy link
Copy Markdown
Collaborator Author

Preset-fork CI adaptation — CI is now terminal green at 2c693160616660b17e64369ef08181601a2e3255d

Decision: Exasol's SonarQube organization and its SONAR_TOKEN are not applicable to this fallback fork. I did not request, copy or recreate that secret, and I did not leave the check red. Instead, commit 2c69316 — its own commit, separate from the fix and the version marker — makes a narrowly scoped adaptation.

What changed

1. The Sonar upload step is skipped on this fork only.

- name: Upload to Sonar
  id: upload-to-sonar
  if: "github.repository != 'preset-io/sqlalchemy-exasol'"

The guard is keyed on github.repository, so behaviour is unchanged for exasol/sqlalchemy-exasol and for every other fork — only this repository skips it. Verified in the run: the Fast Report job's steps are Download Artifacts ✅, Copy Artifacts into Root Folder ✅, Validate Artifacts ✅, Upload to Sonar skipped. The reporting pipeline still runs and still validates coverage artifacts; only the unauthenticable third-party upload is bypassed.

2. It is expressed through the toolbox's supported patcher, not a hand-edit. report.yml is a generated file, and hand-editing it would break nox -s workflow:check -- all. The change is declared in a new .workflow-patcher.yml and the file regenerated, so Check Workflows still passes — confirmed green in this run.

3. GitHub CodeQL is the replacement static-analysis gate. New .github/workflows/codeql.yml runs CodeQL for Python with the security-and-quality query suite on pull requests and pushes, scoped with if: github.repository == 'preset-io/sqlalchemy-exasol'. The action is hash-pinned (b96794f015dfd88f77b49b1c93e0fa7110f94c63) to satisfy this repo's existing zizmor unpinned-uses policy rather than relaxing that policy — nox -s workflow:audit reports no findings, and Audit Workflows is green.

No lint, typing, security or test job was removed, skipped or relaxed. The only skipped step in the entire run is the Sonar upload.

Result — run 35039591501, conclusion success

67 checks pass, 0 fail, 0 pending. 65/65 jobs succeeded.

  • CodeQL Python — success (new gate), run 35039590907
  • Merge Gate / Fast Report / report — success (was the sole failure at the previous head)
  • Report / report — success
  • 30 Integration-Tests jobs — test:exasol, test:sqla, test:regression × Python 3.10–3.14 × real Exasol 8.29.13 and 2025.1.8, websocket — all green
  • Verify Examples on both server versions, Docs, Check Workflows, Audit Workflows, Audit Security 3.10–3.14, Lint Code/Typing 3.10–3.14, Check Format, Build Package, Changelog, Unit Tests 3.10–3.14 — all green
  • Merge Gate / Allow Merge — success

Unchanged

Version remains 7.1.3.1. The reviewed fix tree is still pristine: git diff --quiet 44f4792 92039cf6 confirms the tree two commits below HEAD is byte-identical to the upstream exasol#808 head. Publishing and release workflows remain disabled (Build & Publish, CD, Check Release Tag, Publish Documentation, PR-Merge → disabled_manually). Nothing published, tagged, merged or released. Upstream #808 and Shell #5084 are untouched.

Note that the artifact checksums recorded earlier were built at 8af5fb7; the packaged source is unchanged by this commit (it touches only CI configuration), but rebuild from this head before any upload so the published artifact matches the head that CI validated.

@fitzee

fitzee commented Sep 16, 2026

Copy link
Copy Markdown

Adversarial code review

Smaller, high-quality PR with strong tests. The findings center on the disconnect-detection contract:

1. is_disconnect too narrow — sqlalchemy_exasol/websocket.py:194

It only returns True when e.__cause__ is exactly an ExaCommunicationError. But the driver's translate_exception maps both ExaConnectionError and ExaCommunicationError to OperationalError (treating both as connection-level failures), and IMPORT/EXPORT paths surface transport failures as ExaExportError/ExaImportError → generic DatabaseError. In all those cases __cause__ isn't an ExaCommunicationError, so connection_invalidated stays False, pre-ping/pool never recycles the dead socket, and the app keeps erroring on a broken connection instead of recovering. Matching the OperationalError class (or the full set of connection-level exceptions) rather than the narrow __cause__ type would make this robust.

2. Fragile, unpinned dependency on single-level chaining — sqlalchemy_exasol/websocket.py:194

The same check assumes the driver keeps raise ... from ex one-level chaining forever. pyexasol is only pinned >=0.26.0,<3.0; if a future exasol.driver.websocket.dbapi2 refactor wraps one level deeper, stops chaining, or renames the type, disconnect detection silently turns off — no exception, no unit-test failure — and only the destructive integration tests (which require a disposable DB with KILL SESSION privileges) would catch it. Fixing #1 by matching on the SQLAlchemy exception class also resolves this.

3. Reflection relies on identifier folding — test/integration/exasol/test_websocket_recovery.py:282

The schema is created uppercase (EXA_REG_<hex>) but reflected via schema=schema.lower(), working only because Exasol folds the unquoted lowercase name back to uppercase. If identifier-quoting behavior for the schema name ever changes, reflection silently targets a nonexistent lowercase schema and the test fails/reflects nothing rather than exercising the intended round-trip.


#1 is the one that matters — it undercuts the PR's central goal of transparent reconnection.

🤖 Generated with Claude Code

@aminghadersohi
aminghadersohi changed the base branch from preset/upstream-7.1.3 to master September 16, 2026 03:59
@aminghadersohi
aminghadersohi merged commit 018427d into master Sep 16, 2026
67 checks passed

This branch was successfully deployed

1 active deployment
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