Skip to content

fix: serialize USSD sends safely - #129

Draft
Justinabox wants to merge 1 commit into
mainfrom
agent/ussd-concurrency-20260627
Draft

fix: serialize USSD sends safely#129
Justinabox wants to merge 1 commit into
mainfrom
agent/ussd-concurrency-20260627

Conversation

@Justinabox

Copy link
Copy Markdown
Owner

Summary

  • Fixes Serialize or reject concurrent USSD sends to avoid response cross-correlation #126 by serializing USSD sends with a shared per-EventBus lock so concurrent service instances cannot cross-correlate responses.
  • Fails closed after command timeout, response timeout, or cancellation before any later AT+CUSD write; cancel() sends AT+CUSD=2 but does not clear the unsafe session state because late +CUSD responses cannot be correlated safely.
  • Redacts USSD commands/responses in executor TX/RX/timeout logs and URC debug/warning/error logs.

Tests

  • PYTHONPATH=. uv run --no-project --with pytest --with pytest-asyncio --with pytest-aiohttp --with pyserial-asyncio --with aiosqlite pytest tests/test_ussd.py tests/test_http_api_validation.py tests/test_api_auth.py tests/test_executor.py -q → 108 passed
  • git diff --check → passed
  • PYTHONPATH=. uv run --no-project --with pytest --with pytest-asyncio --with pytest-aiohttp --with pyserial-asyncio --with aiosqlite pytest tests/ -q → 595 passed

Risks

  • After an incomplete USSD session, the service intentionally requires a modem/session reset path rather than attempting to guess which later +CUSD belongs to which request.
  • No real modem/hardware was touched; coverage uses mock executors/transports and deterministic EventBus tests.

@Justinabox

Copy link
Copy Markdown
Owner Author

Review/merge-manager classification: needs-fix before autonomous landing.

The concurrency/USSD privacy direction is good and local read-only review found the new serialization/redaction tests useful, but there is an API/regression blocker around cancellation/reset semantics:

  • USSDService.cancel() now unconditionally marks the shared EventBus session as requiring reset before sending AT+CUSD=2.
  • There is no recovery/clear path, so a successful explicit await modem.ussd.cancel() can permanently cause future send() calls on the same service/EventBus to fail with Previous USSD request did not complete....
  • The shared EventBus flag also appears to survive modem close/reopen paths that reuse the same bus/service, so reconnect may not recover the session.

Requested fix: keep the fail-closed behavior for uncertain timeout/cancellation paths, but either (a) do not poison the session after a confirmed successful AT+CUSD=2, or (b) add an explicit reset/reconnect recovery path that clears the flag, with RED tests proving future sends recover only after a safe reset. Please rerun:

  • git diff --check origin/main...HEAD
  • PYTHONPATH=. uv run --no-project --with pytest --with pytest-asyncio --with pytest-aiohttp --with pyserial-asyncio --with aiosqlite pytest tests/ -q

Holding this PR as needs-fix; not marking ready/merging in this pass.

@Justinabox

Copy link
Copy Markdown
Owner Author

Review/merge-manager classification: needs-fix.

I am not marking this ready or merging yet. The branch currently conflicts with main, and USSD concurrency/redaction is a sensitive shared-modem surface.

Requested changes:

  • Refresh over current main and resolve conflicts.
  • Keep outbound USSD validation fail-closed before modem writes for quote/CR/LF breakout and invalid DCS values.
  • Prove concurrent sends are serialized or rejected safely across shared EventBus/modem state, including timeout/cancel paths.
  • Ensure USSD codes/responses are redacted from executor TX/RX/timeout logs and URC logs.
  • Re-run USSD/executor focused tests, git diff --check, and the full required pytest gate.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release review: needs-human.

This draft is held from autonomous merge because it changes a security-sensitive, credential/SIM-recovery, production-deployment, or public product/API policy surface. Please provide an explicit product/security decision and a refreshed review-ready head (this PR is also stale/dirty where applicable) before it can re-enter the landing queue. No behavior was approved or merged in this pass.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release-captain classification: needs-fix. This PR is now DIRTY against the current default branch after later safe merges. Please rebase/repackage it from current origin/main, resolve only the genuine overlap, then rerun git diff --check and the required no-hardware full pytest gate on the new head. Do not attempt to merge the stale head directly.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release lane update: needs-human.

This change affects a sensitive security/privacy, production-deployment, modem/SIM, audio/hardware, or public realtime product boundary. It will not be autonomously merged under this lane. Please obtain an explicit Korgo decision on the intended policy/scope and, where applicable, rebase on current main with current gate evidence before requesting a new release review.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release review: needs-human.

This PR changes a sensitive credential, security, or modem-safety policy. Per the autonomous release-lane policy it is intentionally held for an explicit Korgo decision and will not be merged on local-test evidence alone. After a decision, refresh it over current main and rerun the required gates before a new review.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release review: needs-human. This stale PR changes a sensitive modem/security-control surface that is not eligible for autonomous landing. A current-base rework may proceed only after Korgo records the intended policy and safety/compatibility decisions; do not merge or attempt to salvage the stale head autonomously.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release review classification: needs-human. This changes security/credential or externally reachable policy behavior and requires Korgo’s explicit product/security decision before release. It will not be auto-merged in this lane. Please retain the branch and provide the requested policy/operational rationale plus updated gates for a later human-approved review.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release-lane classification: needs-human.

This is an old conflicted USSD concurrency/security change. It needs an explicit product/security decision on serialization and timeout/cancellation behavior before a focused rebase and review. No merge action taken in this lane.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release-lane classification: needs human decision.

This changes a security-sensitive or externally exposed policy surface. It requires Korgo's explicit product/security decision and a focused security review before an autonomous merge; local test success alone is not authorization to land it.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release classification: needs-fix for this release pass.

The draft has not received a current exact-head review and prescribed local integration gate over the advancing main baseline. Rebase/repackage as needed against current main, resolve any conflicts, then provide a narrow behavioral review plus passing git diff --check and the prescribed full test suite before it can be reconsidered for landing.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release review: needs-fix. GitHub reports this USSD concurrency PR DIRTY. Rebase/repackage and rerun concurrent/cancelled send tests, including no-follow-up-write fail-closed behavior, before re-review.

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.

Serialize or reject concurrent USSD sends to avoid response cross-correlation

1 participant