Skip to content

refactor: make the pending-subscription sentinel explicit - #56

Merged
cigamit merged 1 commit into
ctrliq:mainfrom
blaipr:refactor/pending-job-sentinel
Sep 13, 2026
Merged

refactor: make the pending-subscription sentinel explicit#56
cigamit merged 1 commit into
ctrliq:mainfrom
blaipr:refactor/pending-job-sentinel

Conversation

@blaipr

@blaipr blaipr commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

WSClient._should_subscribe_to_pending_job holds False until a pending subscription is queued and a dict afterwards, and three places subscript it. The subscripts are safe, because the only path that reaches them tests the flag first, but the test was buried:

if all([message.get('group_name') == 'jobs', message.get('status') == 'pending', message.get('unified_job_id'), self._should_subscribe_to_pending_job]):
    if bool(message.get('project_id')) == (self._should_subscribe_to_pending_job['events'] == 'project_update_events'):
        self._update_subscription(message['unified_job_id'])

Four unrelated conditions in an all([...]), one of them the guard for the line below it, and _update_subscription then reaching back for the attribute a second time rather than being handed it.

Three changes, none of which alters behaviour:

  • The sentinel is None rather than False, annotated dict | None. Both are falsy and nothing compares it by identity or to False, so every existing check behaves the same. None is what "not set yet" means.
  • The flag is bound to a local and tested first, with and instead of all([...]). Short-circuiting rather than eager evaluation, which is fine here since every element is a pure .get().
  • _update_subscription takes the dict as an argument instead of re-reading the attribute, so it cannot be called in a state where that attribute is unset.

Exercised the whole path directly, since the unit suite covers the callbacks but not this branch: queueing with subscribe_to_pending_events('job_events'), then feeding a pending-job message through _on_message, resubscribes with {'jobs': ['status_changed'], 'job_events': [7]} and clears the sentinel back to None.

Four diagnostics retired. Diffed the full list before and after: strict subset, nothing introduced.

Note for whoever merges: #27 also edits this file, so whichever lands second needs a rebase.

Verified with black --check, flake8 and the unit suite, 355 passing.

@ciq-it-service-account

ciq-it-service-account commented Sep 12, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

`WSClient._should_subscribe_to_pending_job` holds `False` until a pending subscription is queued and a dict afterwards, and three places subscript it. The subscripts are safe, because the only path that reaches them tests the flag first, but the test was buried:

```python
if all([message.get('group_name') == 'jobs', message.get('status') == 'pending', message.get('unified_job_id'), self._should_subscribe_to_pending_job]):
    if bool(message.get('project_id')) == (self._should_subscribe_to_pending_job['events'] == 'project_update_events'):
        self._update_subscription(message['unified_job_id'])
```

Four unrelated conditions in an `all([...])`, one of them the guard for the line below it, and `_update_subscription` then reaching back for the attribute a second time rather than being handed it.

Three changes, none of which alters behaviour:

- The sentinel is `None` rather than `False`, annotated `dict | None`. Both are falsy and nothing compares it by identity or to `False`, so every existing check behaves the same. `None` is what "not set yet" means.
- The flag is bound to a local and tested first, with `and` instead of `all([...])`. Short-circuiting rather than eager evaluation, which is fine here since every element is a pure `.get()`.
- `_update_subscription` takes the dict as an argument instead of re-reading the attribute, so it cannot be called in a state where that attribute is unset.

Exercised the whole path directly, since the unit suite covers the callbacks but not this branch: queueing with `subscribe_to_pending_events('job_events')`, then feeding a pending-job message through `_on_message`, resubscribes with `{'jobs': ['status_changed'], 'job_events': [7]}` and clears the sentinel back to `None`.

Four diagnostics retired. Diffed the full list before and after: strict subset, nothing introduced.

Note for whoever merges: ctrliq#27 also edits this file, so whichever lands second needs a rebase.

Verified with `black --check`, `flake8` and the unit suite, 355 passing.
@blaipr
blaipr force-pushed the refactor/pending-job-sentinel branch from f73a33e to a4edd0f Compare September 13, 2026 09:03
@cigamit cigamit self-assigned this Sep 13, 2026
@cigamit cigamit added the enhancement New feature or request label Sep 13, 2026
@cigamit
cigamit merged commit e96c545 into ctrliq:main Sep 13, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Development

Successfully merging this pull request may close these issues.

3 participants