fix: resolve stale credentials causing astrolabe test failures

[cbcoutinho]

Summary

• Fix revoke test: Use complete_astrolabe_authorization() for full two-step flow (OAuth + app password) instead of only completing Step 2. In hybrid mode, Astrolabe requires both steps for $isFullyConfigured=true, which gates the "Revoke Access" button.
• Add stale state cleanup: Clear app passwords, bruteforce entries, and Astrolabe preferences at the start of both enablement and revoke tests to prevent interference from previous runs.
• Harden background scanner: Pre-validate credentials before entering scan loop, handle 401/403/429 with proper circuit breaking, and clean up stale app passwords at startup in BasicAuth mode.

Test plan

• test_revoke_background_sync_access passes (was failing)
• test_multi_user_astrolabe_background_sync_enablement passes (no regression)
• Ruff check/format clean
• ty check clean

---

This PR was generated with the help of AI, and reviewed by a Human

[claude]

PR Review: fix: resolve stale credentials causing astrolabe test failures

Overall this is a solid fix — the root cause (stale app passwords blocking re-authorization) is well-diagnosed and the circuit breaker / pre-validation additions make the scanner meaningfully more robust. A few things worth addressing:

---

Issues

1. Inline import httpx in storage.py

cleanup_invalid_app_passwords places import httpx inside the function body (line 1430). httpx is a core dependency used throughout the codebase and should be imported at the module level alongside the other imports. Inline imports are typically only justified for circular-dependency avoidance, which is not the case here.

2. SQL injection pattern in conftest.py

The cleanup in conftest.py uses string interpolation in a SQL statement:

f"DELETE FROM app_passwords WHERE user_id = '{username}';"

This passes username directly into SQL via subprocess to sqlite3. Even though test usernames are controlled data, this is a bad pattern to introduce. If a future username contains a quote or semicolon (e.g. a user created to test edge cases), this would silently break or behave unexpectedly. The existing scripts/sqlitequery.py helper exists precisely to handle this cleanly.

3. Duplicate stale-state cleanup blocks

The cleanup logic in test_multi_user_astrolabe_background_sync_enablement and test_revoke_background_sync_access is nearly identical (delete app_passwords, delete bruteforce attempts). Extracting a clear_stale_test_state(clear_preferences=False) helper would reduce future drift and make intent clearer.

4. Missing Retry-After header for 429 backoff

The 429 handler uses a hardcoded 60-second backoff. HTTP 429 responses frequently include a Retry-After header specifying the required delay. Consider:

retry_after = int(e.response.headers.get("Retry-After", "60"))
backoff = min(retry_after, 300)  # cap at 5 minutes
with anyio.move_on_after(backoff):
    await shutdown_event.wait()

5. Sequential validation in cleanup_invalid_app_passwords

The method validates each stored app password one at a time. For deployments with many users, this serialized network I/O at startup could be noticeable. Per the project's own conventions, anyio.create_task_group() would be the natural fit for concurrent validation. Low priority for this fix, but worth a TODO comment if left sequential.

6. subprocess.run return codes are silently ignored

The cleanup subprocess.run() calls use capture_output=True but never check returncode. A failed docker exec (e.g. container not running) would silently proceed, leaving stale state intact while the test expects a clean slate. At minimum, log a warning when returncode != 0.

---

Minor Notes

• Pre-validation exception handling: If get_user_client() in the pre-validation block raises something other than NotProvisionedError (e.g. a transient network error at startup), that exception propagates to the task group after task_status.started() has already been called. Wrapping the full pre-validation in a broader except Exception with a warning log and return would make startup failures softer.

• Cancellation comment: The try/except anyio.get_cancelled_exc_class(): break inside the 429 handler is correct but subtle. A brief comment explaining why cancellation needs explicit handling there (to propagate shutdown cleanly through the backoff wait) would help future readers.

---

What's Good

• The circuit breaker for consecutive errors (max_consecutive_errors = 5) is a clean improvement over unbounded error loops.
• Using complete_astrolabe_authorization() (full two-step flow) in test_revoke_background_sync_access is the correct fix — the revoke button only appears when $isFullyConfigured=true, which requires both steps.
• Startup cleanup of invalid app passwords is a practical safeguard for BasicAuth mode restarts.
• Error categorization (401/403 → stop, 429 → backoff, other → count toward circuit breaker) is well-structured.