Repository navigation
fix(chatgpt): remove the unguarded markOpenAiChatGptError write - #7847
Conversation
markOpenAiChatGptError read the connection without a row lock and wrote the terminal error unconditionally, so a reconnect landing between the read and the write could be disabled and emptied by a stale failure. It has no production callers left (the refresh path marks errors inside its row-locked transaction, and refusals go through the locked, token-checked markOpenAiChatGptNotEligible), so the export is deleted instead of guarded. Tests that exercised only this helper are dropped; the cleared-state test now stores an errored payload directly.
iscekic
added
drain
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe incremental commit adds only a real-database regression suite; the row-lock assertion is deterministic (the refresh transaction holds the lock until the test releases it), and the deleted Files Reviewed (3 files)
Note: Previous Review Summary (commit 169b0f6)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 169b0f6)Status: No Issues Found | Recommendation: Merge Executive SummaryThe PR removes the unguarded Overview
Files Reviewed (2 files)
Note: a repo-wide search (including ignored files) confirms no remaining reference to Reviewed by deepseek-v4.1-flash · Input: 54.9K · Output: 11.2K · Cached: 740.2K Review guidance: REVIEW.md from base branch |
The deleted markOpenAiChatGptError was unguarded; the guarded terminal write lives in the refresh transaction. Add a real-database suite that holds the failing refresh in its token exchange and proves a reconnect issued in that window does not land until the transaction commits, so the terminal write cannot erase it. Removing the row lock from resolveInsideLock makes the reconnect land and the test fail.
Change
markOpenAiChatGptErrorinpackages/web-shared/src/lib/ai-gateway/openai-chatgpt/store.tsread the stored connection without a row lock and then wrote the terminal error unconditionally: it disabled the row and cleared the stored token set. Its guarded siblings never had this hazard — the refresh path writesmarkOpenAiChatGptConnectionErroredinside the transaction that holds theSELECT ... FOR UPDATErow lock, andmarkOpenAiChatGptNotEligiblere-reads the row under lock and writes only when the stored access token is still the refused one. A reconnect that landed between the unguarded read and write could be disabled and emptied by the stale failure.The helper no longer has any production caller: the refresh path moved inside the row-locked transaction, and refusal handling goes through
markOpenAiChatGptNotEligible. Rather than guard a dead export, this change deletes it, so no future caller can reuse the unguarded write. Tests that exercised only the deleted helper are dropped; the cleared-state test instore.test.tsnow stores an errored payload directly.Verification
markOpenAiChatGptErroranywhere (source or tests).refresh-terminal-race.test.tspins the guarded path: it parks a failing refresh inside its token exchange, issues a reconnect while the row lock is held, and proves the reconnect does not land until the transaction commits — so the terminal write cannot erase it, and the reconnected connection ends enabled with its own tokens. Removing the row lock fromresolveInsideLockmakes the reconnect land and the test fail, so the test discriminates.pnpm test -- openai-chatgptinapps/web: 10 suites, 139 tests, all passing.pnpm --filter @kilocode/web-shared run typecheck: clean (both tsconfigs).pnpm --filter web run typecheck: clean (pre-existing warnings only).pnpm format+git diff --check: clean.Proxy answers
The owner's proxy gave these answers in the owner's place.
Answer: Delete the dead export
Confidence: 0.50 · Precedent: Speed and least work: 'Replace old paths instead of keeping them' (session 2190fcfd0f42: 'I am leaning heavily toward just ripping things out'); Product scope: 'Trust the code over documents' (session 269375d6e466). Deletion removes the unguarded write, so it fixes the class. Confirm in end-to-end proof that the refresh path's locked transaction already guards against the race.
Answer: Access token (recommended)
Confidence: 0.60 · Precedent: Speed and least work: 'Do the least work possible with no drop in quality' (session 7f3470e0ba6f). This applies only if the guard is kept; the access token mirrors markOpenAiChatGptNotEligible and needs no new stored id.
Answer: New real-DB suite file (recommended)
Confidence: 0.55 · Precedent: Proof and verification: 'Require valid end-to-end proof on every PR' (session 55c9c80332a7). Only a real database can prove the lock behavior. If the export is deleted, add the race regression tests against the refresh path in the real-DB suite.
Both conditions attached to answer 1 are met: the refresh path's locked transaction is proven against the race in the new real-database suite, and the race regression tests live in that suite.