Sprint 9 P2 cleanup: resolve 6 deferred Codex follow-ups in one bundle - #86
Merged
Merged
Conversation
Closes the deferred follow-up list across Sprint 9 PRs #78, #81, #82, #84, #85 so the closeout doc (9.8) ships against a clean slate. P2 fixes: 1. /forgot-password concurrent-request race (api/routers/password_reset.py). Add with_for_update() to the Person lookup so two parallel forgot-password calls serialize on the Person row. Without it both transactions could invalidate-then-insert, leaving 2 valid reset tokens (Scenario 6 in docs/features/password-reset.md says fresh request must invalidate older links). SQLite ignores FOR UPDATE (global lock already serializes writers); PostgreSQL enforces a row lock under READ COMMITTED. 2. Settings.EMAIL_ENABLED default mismatch (api/core/config.py). Flip default from True to False to match EmailService's effective env-read gate (defaults to false per #78). Without alignment, notification_service._should_queue_email() reads True from Settings and queues Celery email jobs that the worker silently no-ops. 3. Refresh-in-flight signOut race (mobile/lib/api/api_client.dart). Before persisting tokens from /auth/refresh response, re-read the refresh slot and confirm it still matches the token we sent. If signOut/clearAll ran during the round-trip, storage is empty or replaced — drop the write. Otherwise the dio interceptor silently resurrects the prior session after explicit logout. New regression test in refresh_test.dart. 4. invitation_repository absent-refresh handling (mobile/lib/auth/invitation_repository.dart). Mirror the #82 fix from login_repository / signup_repository: clear any prior refresh token when the server omits one, so accepting an invitation from a pre-9.4b backend can't inherit the previous user's refresh credential. 5. email_smoke.py shell-env leak (scripts/email_smoke.py + docs/saas/SMOKE_TESTING_EMAIL.md). load_dotenv(override=True) so a blank SENDGRID_API_KEY in .env wins over a stale exported value; added warning when both SendGrid and Mailtrap creds are populated; runbook now prefixes Path A with `unset SENDGRID_API_KEY` as a belt-and-braces guarantee. 6. Codex review base ref (CLAUDE.md). Replace --base main with --base origin/main on both review invocations + add `git fetch origin main` prelude. Reviewing against local main runs against a stale base when local hasn't fetched recently. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex caught three issues in the first iteration that meaningfully undermined the fixes' value. Each addressed: 1. password_reset row lock was released by log_audit_event's commit (api/routers/password_reset.py). Moved with_for_update() out of the initial Person lookup (which is followed by a committing audit log) and into a tight re-acquire right before the invalidate+insert critical section. Lock is now held until the token-rotation commit. 2. Refresh-in-flight signOut race fix was wiping fresh logins (mobile/lib/api/api_client.dart). The original change made _attemptRefresh return false on stale; the interceptor then ran clearAll(), clobbering a user who'd just signed back in mid-flight. Introduced _RefreshOutcome enum with success/failure/stale. The interceptor only clearAll()s on failure; stale leaves storage alone. New regression test in refresh_test.dart covers the fresh-login case explicitly. 3. email_smoke.py override=True overrode every env var, not just the SendGrid key. A shell-set EMAIL_ENABLED=false or TESTING=true safety toggle could be silently overridden by .env values. Replaced with a narrow override: read .env via dotenv_values(), and only force SENDGRID_API_KEY-blank when .env explicitly sets it that way. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex iter 2 caught: P0: the new with_for_update() Person re-fetch in /forgot-password queried by id only, violating the repo-wide tenancy convention that every Person query filters by org_id. Filter is now (Person.id == person.id AND Person.org_id == person.org_id). Same result (id is globally unique), but matches the convention reviewers grep against. P1: the value-compare stale-check in _attemptRefresh had a TOCTOU window — between readRefreshToken() and writeToken(), an interleaved signOut/login could pass the value check but invalidate the session state we were about to overwrite. Replaced value compare with a monotonic sessionGeneration counter on SecureTokenStorage that bumps on every write/clear. The interceptor snapshots the generation at request start; if anything mutates storage during the round-trip, the response is dropped without touching state. Existing regression tests in refresh_test.dart still cover the clearAll and fresh-login paths end-to-end via the generation counter. SecureTokenStorage abstract interface adds `int get sessionGeneration`; both _RealStorage and InMemoryTokenStorage maintain a private `_gen` that increments on each mutator (writeToken / clearToken / writeRefreshToken / clearRefreshToken / clearAll). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex iter 3 P1: the generation check before writeToken closes the "during round-trip" race but not the narrow "between the two awaited writes" race. If a signOut/login lands after writeToken but before writeRefreshToken, storage ends up with one session's access and another session's refresh. Fix: after the first write, recheck generation against (genAtStart+1). If it bumped higher, another mutator interleaved. Roll back the access write (clearToken — which itself bumps generation, but we're now deliberately discarding both halves of this rotation) and return stale. The remaining theoretical race (a mutator landing during the rollback's own await) is acceptable: in Dart's single-isolate cooperative model the window is platform-call latency, and on the next 401 the interceptor will pick up whichever session is actually in storage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… failure as stale Codex iter 4 caught two follow-on P2s: 1. The inter-write recheck (iter 4) called clearToken() on detected race — but by the time we detect it, the access slot may hold a fresh login's token, and clearing it logs that user out. Reverted the rollback. Single generation check before the writes remains; inter-write race documented as a known narrow window (microseconds, Dart cooperative async, no preemption). 2. On DioException (refresh failed), we previously returned `failure` unconditionally, which makes the caller run clearAll() — but if a fresh login landed during the round-trip, that wipes the new session. Now also gate the DioException path on the generation check: if storage changed mid-flight, return `stale` instead of `failure` so the new session is preserved. The remaining inter-write race (mutator landing between writeToken and writeRefreshToken) is platform-call latency. Closing it requires either package:synchronized or an atomic compareAndWriteTokens method on SecureTokenStorage — both meaningful design changes deferred to a focused follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex iter 5: Dart scoping error — `genAtStart` declared inside the try block, referenced from the DioException catch path. Wouldn't compile under flutter analyze. Repo's CI doesn't run flutter analyze so it slipped past. Moved the snapshot above the try. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 of 4 tasks
tomqwu
added a commit
that referenced
this pull request
May 13, 2026
…0 PR 10.3) (#93) Squash-merges PR #93 (Sprint 10.3 mobile concurrency + deep-link). Closes two deferred items from Sprint 9's closeout list. 1. Mobile refresh inter-write race (residual from #86). - SecureTokenStorage.compareAndWriteTokens(expectedGen, access, refresh) → bool: sync prelude (gen check + bump) runs to completion before any await, so the "another mutator passes the check before we increment" race is closed. - InMemoryTokenStorage: trivially atomic. - _RealStorage: still has narrow platform-call-latency window between the two awaited writes; documented inline. - _attemptRefresh uses the new method; old gen-check + two-writes shape collapses into one rejectable call. - refresh_test.dart: two new unit tests pin the contract. 2. Custom-scheme deep-link routing (Codex flag from #87/#88). - Switched email URL generation in api/services/email_service.py to triple-slash form (signupflow:///invitation?token=...) so cold-launch parses path correctly (host-form path is empty, dropped by go_router bootstrap). - Added defensive _hostRouteRemap in mobile/lib/routing/router.dart to remap warm-start host-form URLs (any old email in flight). - mobile/integration_test/deep_link_test.dart: 3 tests covering both URL forms + reset-password route. Isolated from real API via FakeInvitationRepo override. specs/024-sprint-9-completion/spec.md: both items struck with pointers to this PR. Codex iter sequence: - iter 1 → 2 findings: deep-link test widgets didn't match LoginScreen reality + integration test used wrong context to find GoRouter. Plus hosting-form cold-launch P1. - iter 2 → P1 (cold-start drops host before redirect), P2 (test context still wrong). Switched to triple-slash URL + LoginScreen context. - iter 3 → 2 P2 (test isn't isolated from real API; underscore helpers fail VGA lint). Added FakeInvitationRepo + renamed helpers. - iter 4 → no findings, merging. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the deferred follow-up list across Sprint 9 PRs #78, #81, #82, #84, #85 so the closeout doc (9.8) ships against a clean slate.
api/routers/password_reset.pywith_for_update()serializes concurrent forgot-password callsapi/core/config.pySettings.EMAIL_ENABLEDdefault flipped toFalseto match EmailService gatemobile/lib/api/api_client.dart+mobile/test/refresh_test.dartmobile/lib/auth/invitation_repository.dartscripts/email_smoke.py+docs/saas/SMOKE_TESTING_EMAIL.mdload_dotenv(override=True)+ warning when both backends configured + runbookunset SENDGRID_API_KEYpreludeCLAUDE.md--base origin/main+git fetchprelude (was--base mainagainst stale local ref)Test plan
*.pyfiles.refresh_test.dartregression test for P2 E2E: Baseline broad run (incl. admin_strict) + capture failures/flake rate #3 (refresh-in-flight signOut → tokens stay cleared).Lint, type-check, and test, including PG Alembic step which exercises theFOR UPDATEpath).scripts/email_smoke.pyperSMOKE_TESTING_EMAIL.mdPath A withunset SENDGRID_API_KEY— verify warning fires when both creds are set.🤖 Generated with Claude Code