fix: prevent hang when store-configured query returns zero rows

[james-willis]

Summary

• Fixes a critical bug where cursor.get_store_result() (and fetchall()) hangs indefinitely when a store-configured query returns zero rows
• The server correctly returns state=succeeded with result_uri=None for empty store results, but the driver misinterpreted this as a non-store query and fell into a code path that silently dropped the response — leaving cursor.__queue.get() blocking forever

Design Docs

• Architecture: docs/architecture/fix-empty-store-results-hang.md
• Technical Design: docs/design/fix-empty-store-results-hang.md

Changes

• Primary fix (connection.py, __listen()): Added if query.store is not None: check after the if result_uri: block in the SUCCEEDED + STATE_UPDATED path. When store is configured but result_uri is falsy (empty result set), immediately marks query as COMPLETED and delivers an empty ExecutionResult() instead of calling __request_results()
• Defensive fix (connection.py, __listen()): In the execution_result handler, when results is falsy or not a dict, now marks query as COMPLETED and delivers an empty ExecutionResult() instead of silently returning — ensures the cursor is always unblocked
• Tests (tests/test_empty_store_results.py): 5 tests covering empty store results, normal store results, non-store query fallback, null execution results, and empty dict execution results
• Version bump: 0.25.0 → 0.25.1

Testing

• pytest tests/ -v — all 19 tests pass (14 existing + 5 new)
• New tests use unittest.mock to mock the WebSocket and test __listen() behavior by injecting messages and verifying the handler receives the correct ExecutionResult

Deviations from Design

None

[james-willis]

[Arch] Architectural Review — ✅ APPROVED

I reviewed this PR against the architecture doc (docs/architecture/fix-empty-store-results-hang.md) and the implementation is a faithful, precise translation of the design.

Layer 1 (Primary Fix) ✅ — The if query.store is not None: check in __listen() is positioned exactly where the architecture specified: after the if result_uri: block and before the self.__request_results() fallthrough. It correctly short-circuits with ExecutionResult() for store-configured queries that return empty results, preventing the erroneous retrieve_results call.

Layer 2 (Defensive Fix) ✅ — The null/empty results handler in the execution_result path now sets COMPLETED state and calls query.handler(ExecutionResult()) instead of silently returning. This ensures the cursor is always unblocked regardless of the server response.

Option C (No server changes) ✅ — Correctly client-only fix as designed. The fix relies on local state (query.store) rather than protocol assumptions, making it robust against future server changes.

Component boundaries ✅ — No new abstractions, no new dependencies, no changes to the Query dataclass or ExecutionResult model. The fix works entirely within the existing __listen() state machine.

Future extensibility ✅ — The fix is additive (12 lines of production code) and doesn't constrain any future protocol changes. The defensive layer protects against future edge cases beyond the immediate bug.

Test coverage ✅ — 5 well-structured tests covering all three paths (empty store, normal store, non-store fallback) plus defensive cases (null results, empty dict results). Tests operate at the right level of abstraction by injecting messages into __listen().

No architectural concerns. This is ready to merge from an architecture perspective (pending pre-commit formatting fix).

[james-willis]

[Design] Spec Compliance Review

✅ Primary Fix — connection.py lines 195-203

The if query.store is not None: check matches Section 2.1, Change 1 of the design spec exactly:

• Placement after if result_uri: and before __request_results() fallback ✅
• Sets query.state = ExecutionState.COMPLETED ✅
• Calls query.handler(ExecutionResult()) with no args (all-None fields) ✅
• Log message matches spec ✅

✅ Defensive Fix — connection.py lines 213-214

The two added lines inside the existing if not results or not isinstance(results, dict): block match Section 2.1, Change 2 exactly:

• query.state = ExecutionState.COMPLETED ✅
• query.handler(ExecutionResult()) ✅
• Existing warning log preserved ✅

✅ Tests — tests/test_empty_store_results.py

All 5 test cases from Section 2.2 are implemented:

1. test_store_configured_empty_results_completes — store configured, result_uri=None → empty ExecutionResult, state COMPLETED ✅
2. test_store_configured_with_result_uri_produces_store_result — store configured, result_uri present → StoreResult ✅
3. test_no_store_configured_calls_request_results — no store, result_uri=None → __request_results() called ✅
4. test_null_results_in_execution_result_unblocks_cursor — execution_result with results=None → unblocked ✅
5. test_empty_dict_results_in_execution_result_unblocks_cursor — execution_result with results={} → unblocked ✅

Test assertions are meaningful — each test verifies isinstance, all three ExecutionResult fields, and the query state transition. Good.

✅ Edge Cases from Spec (Section 5)

• Non-store query with result_uri=None → falls through to __request_results() (verified by test 3) ✅
• result_uri empty string is falsy in Python → handled correctly by existing if result_uri: check ✅
• Empty dict results ({}) are falsy → falls into defensive handler (verified by test 5) ✅

✅ No Unexpected Dependencies

No new runtime or test dependencies added. Tests use unittest.mock from stdlib, as specified.

⚠️ One Issue: Pre-commit CI Failure (unused imports)

The test file has unused imports that were in the design spec template but are not actually used by the test code:

• threading (line 11)
• pytest (line 15)
• Cursor from wherobots.db.cursor (line 18)
• StoreResult from wherobots.db.models (line 19)
• EventKind from wherobots.db.types (line 20)

This is causing the pre-commit check to fail. Remove these unused imports to pass CI.

Version Bump

0.25.0 → 0.25.1 — appropriate for a patch-level bug fix.

Verdict

The implementation matches the design spec exactly. The only issue is the unused imports causing CI failure — a trivial fix. Once that's resolved, this is ready to merge from a design perspective.

[james-willis]

[Design] Spec compliance review complete. Implementation matches the design spec precisely. One CI fix needed: remove unused imports in tests/test_empty_store_results.py (see PR comment for details). Will approve once that's addressed.