Remove the X API reads that buy nothing - #299
Merged
Merged
Conversation
RefreshSocialToken verified rotating-refresh platforms instead of refreshing them. On a still-valid token verify() only called the platform's verify endpoint and left token_expires_at untouched, so the account stayed inside RefreshExpiringTokens' 30-minute window and was re-read every 15 minutes until the token actually died. On X that endpoint is GET /2/users/me, billed as a "User: Read" ($0.010 per resource under X's pay-per-usage pricing). Simulating 24h of the scheduler against one connected X account: 36 billed reads per day, 24 of which renewed nothing, plus 180 minutes per day sitting on an expired token between expiry and the next tick. Refresh the token outright instead. A provider that hands back a fresh token has already confirmed the credential — it rejects a revoked one with a 4xx, which TokenRefreshClient maps to TokenExpiredException — so the verify call adds cost and nothing else. Record the confirmation in last_verified_at, and let the daily sweep trust it for 12 hours the way VerifyUpcomingPostConnections already does, so it stops re-reading accounts a refresh just proved valid. Same simulation after the change: 0 billed reads, 0 minutes expired. Two existing tests asserted the old policy (verify-first, refresh_token left unrotated) and now assert the new one. The rotation test still guards what made that policy attractive: a proactive rotation must not trip a false-positive disconnect.
Refreshing instead of verifying removed a safety net the old verify-first path had: if the refresh is rejected, the account was marked TokenExpired outright. But a rejected refresh does not mean the connection is dead. X and LinkedIn single-use their refresh_token, so a token a concurrent refresh already consumed comes back 4xx while the current access_token keeps working. An account with no refresh_token at all fails even earlier, without a single call being made. Verified against main: both cases stayed Connected before, and became TokenExpired after. PublishToSocialPlatform hard-fails every post for a TokenExpired account, so this killed posts the access_token would have published, up to 30 minutes before the token was actually due to expire — and emailed the owner a disconnect notice for it. Fall back to verifying the access token before disconnecting. This is the only path in the job that reaches the billed verify endpoint, and only after a refresh has already been rejected, so the healthy path stays at zero reads (re-confirmed: 0 reads and 0 expired minutes across a simulated 24h). A failure that can't be attributed to the token — platform down, network blip — leaves the account alone instead of disconnecting it on noise. Also covers two gaps found while reviewing: a lock-skipped refresh must not record a verification it never performed, and a platform with nothing to refresh must not be recorded as verified either. Both already behaved correctly; they now have tests so the daily sweep can't start trusting a stamp nobody earned.
Review of the previous two commits turned up four issues, all in code they introduced. The stamp lived inside ConnectionVerifier::refreshToken(), which refreshThenVerify() also calls — there the refresh can succeed and the verify that follows still fail. The stamp was already written by then, vouching for a credential nothing confirmed. Normally harmless, because the caller marks the account TokenExpired and recentlyProvenValid() only skips Connected ones, but markAsTokenExpired() silently no-ops when its status lock is held by a concurrent publish. The account then stays Connected with a fresh stamp, and both skip-windows wave it through: 40 minutes before publishing, 12 hours in the daily sweep. Move the stamp to the caller that owns the outcome. TokenRefreshClient classifies on HTTP status alone and never inspects the body, so a 200 carrying an empty access_token is stored as-is and was then recorded as healthy. (A missing key rather than an empty one can't get that far — access_token is NOT NULL, so the write throws first.) Guard on a filled token. The fallback added in 9775578 called verify(), which for an already-expired account — RefreshExpiringTokens selects those too — runs refreshThenVerify() and re-sends the refresh_token the provider just rejected, and on Bluesky re-runs the password re-auth AT Proto rate-limits per account. Its own docblock claimed it only reached the verify endpoint. Add verifyAccessToken(), which checks the stored token and nothing else. VerifyWorkspaceConnections read last_verified_at without ever writing it, so an account it had just confirmed healthy still burned a fresh call minutes later when a post entered the risk window. Stamp on its success path too. Also corrects the RefreshExpiringTokens docblock, which still described the verify-first behaviour removed in c0d0d57. Re-ran the 24h whole-scheduler simulation: still 0 billed reads, 0 expired minutes, account Connected at the end.
The fallback added in 9775578 judged the in-memory access_token, which is exactly the one that is stale when a refresh loses a race. X single-uses the refresh_token, so when two refreshes overlap the loser's token comes back 400 invalid_grant. Its in-memory instance still holds the pair the winner has already rotated away, so verifying it 401s and the account is marked TokenExpired — while the row in the database holds a perfectly healthy token the winner just wrote. Verified against main: a concurrent rotation leaves the account Connected there and TokenExpired here. refreshThenVerify() already handles this by reloading and retrying with whatever was persisted; the new path skipped that because it never went through refreshThenVerify. Reload before judging. This is the failure path only, so the healthy path is untouched — the whole-scheduler simulation still reports 0 billed reads, 0 expired minutes, Connected at the end.
refreshToken() returns normally — no exception — when another process already holds the per-account lock, so the caller can't tell "refreshed" from "did nothing". Moving the stamp out of the verifier in 4d2795a lost that distinction: RefreshSocialToken stamped last_verified_at on a run that made zero HTTP calls. The account is then vouched for by nobody: the daily sweep skips it for 12 hours and the pre-publish check for 40 minutes. If the concurrent refresh also failed, nothing ever confirmed the credential. Have refreshToken() report whether it actually ran. Callers that ignore the return value are unaffected. The test meant to cover this called refreshToken() directly rather than going through the job, so it kept passing while the job path was broken — it now exercises the job and asserts no HTTP call was made.
Both are pre-existing, but this branch raises the exposure to them from 12 to 16 refreshes per account per day. The per-account lock lasted 30 seconds, exactly the HTTP client's default read timeout — so a refresh could outlive the lock that protects it. Bluesky is the worst case, refreshing with two sequential calls (refreshSession, then the createSession re-auth), each bounded by connect + read timeouts: up to ~80 seconds under one 30-second lock. Once it lapses, a second process refreshes with the same single-use refresh_token and one of the two is rejected. Name the TTL, set it past the ceiling, and write down the invariant so a future slower refresh doesn't quietly break it. RefreshSocialToken was not unique. RefreshExpiringTokens re-selects an account until token_expires_at moves, and that only happens once the job runs — so a queue more than one tick behind stacked a job per tick for the same account, each rotating a single-use refresh_token again for nothing and widening the gap where a worker death loses the pair. Key it by account like VerifyUpcomingPostConnections already does. Cadence is unchanged: the whole-scheduler simulation still reports 16 refreshes, 0 billed reads and 0 expired minutes over 24h.
Checked each provider's official documentation rather than carrying the
assumption forward.
LinkedIn does not rotate. Its refresh docs are explicit: "the lifespan or Time
To Live (TTL) of the refresh token remains the same as specified in the
initial OAuth flow (365 days)" — the same token comes back with a decreasing
refresh_token_expires_in, and only the access token is reissued. The claim
that it single-uses the token predates this branch, but a docblock added here
repeated it.
Bluesky does rotate, and belongs in the list instead: com.atproto.server
.refreshSession declares refreshJwt as a required output field, so every
refresh mints a new one.
Verified alongside, all matching what the code already does:
X access token 2h, refresh single-use with rotation
Bluesky refreshJwt rotates; createSession is rate-limited per handle
(30/5min, 300/day), which the fallback re-auth path shares
TikTok access 24h, refresh 365d, "may be different — you must use the
newly-returned token", which refreshTikTokToken does
LinkedIn access 60d, refresh 365d fixed, not rotated
Google does not rotate on refresh; 100 refresh tokens per account per
client, so the higher refresh rate on YouTube carries no
rotation risk
paulocastellano
changed the title
Analytics fetched the account's timeline for post ids, then turned around and looked the same ids up again through GET /2/tweets purely to read the public_metrics the first request could have returned. The timeline call asked for start_time, end_time and max_results — never tweet.fields. Both endpoints bill per Post returned, so the second pass claimed the same resources a second time, took a second round-trip, and spent a second slice of the same rate limit. For an account with 250 posts in range that is 6 requests where 3 will do. The saving is in round-trips and rate limit rather than dollars: X deduplicates a resource within a 24-hour UTC window, so the second read of an id already read that day is not charged again. But the docs call that a soft guarantee that "may result in resources not being deduplicated" — this stops leaning on it for 250 resources per analytics load. Behaviour is unchanged: same totals, same 5-page ceiling, same empty result when the account posted nothing in range. The page cap is now a named constant, since it bounds what one load can cost as much as how long it takes. Adds the first tests for XAnalytics::getMetrics, covering the totals, the pagination, and that the metrics arrive on the timeline request.
paulocastellano
changed the title
Three gaps in what the previous commit's tests actually assert: A timeline page failing mid-pagination breaks out of the loop and returns whatever was collected. Nothing pinned that — partial data beats an exception on a dashboard someone is looking at, and a future refactor could quietly turn it into one. A post can come back without public_metrics. The accumulator defaults each metric to 0, so it contributes nothing instead of erroring, which also wasn't covered. An account with no posts in range returns [] rather than a list of zeros, so the UI can tell "nothing posted" from "posted, no engagement". Also makes the routing test's mock return explicit. Without andReturn, Mockery hands back a falsy default for the new bool return type, so the assertion about routing was passing while silently exercising the lock-skipped branch. The test still asserts only what its name claims, but no longer depends on a mock default to get there.
All five sit in code these commits introduced. Instagram and Threads must fail loudly. Their long-lived token is extended in place and cannot be renewed once it expires, and RefreshExpiringTokens picks them up a full day ahead precisely so there is time to react. The fallback added in 9775578 applied to them too, so a permanently rejected extension on a token that still reads left the account Connected — the daily sweep passed as well, since verify() succeeds on it — and the owner learned about it only after the token died unrecoverably, while every tick retried the rejection for 24 hours. The fallback now applies only to platforms that rotate a refresh_token, which is what it was written for. The lock went the wrong way. Lengthening it to 120s in 5001c9a treated the scheduler as the only caller, but publishers wait on the same lock: one left behind by a worker that died mid-refresh makes refreshToken() return false, and the publisher falls through and publishes with an expired token. That window was 30s and had become 120s. Bound the calls instead — a token endpoint answers in milliseconds, and 8s read / 4s connect keeps even Bluesky's two sequential calls under a 30-second lock, back to where main had it. Reloading the account can throw. $this->account->refresh() sat outside the try in the fallback, and an exception raised inside a catch block is not caught by a sibling catch. With tries = 1, an account deleted mid-run put the job in failed_jobs. VerifyUpcomingPostConnections guards this same race explicitly. An empty access token was detected but not acted on. recordVerification() declined to stamp it, yet the refresh still counted as a success — and the refresh method had already pushed token_expires_at two hours out, so the account left the window looking healthy while every publish 401d. Mark it expired, which is what verify() used to do on the same input. refreshToken() claimed "whether a refresh actually ran" but returned true for platforms whose match arm does nothing. Only recordVerification() re-checking hasTokenRefreshFlow() kept that from mattering. The guard is now explicit and the contract true at the source. Also settles the tweet.fields question against the live API rather than the docs, which contradict each other: the OpenAPI spec names the parameter post.fields, while the Fields guide and every example use tweet.fields. Both are accepted and both return public_metrics. An unrecognised name returns 200 and silently omits the field — no error — so the name being right is load bearing, and it is.
Dropping `default => null` from refreshToken() in af379af made the return value honest, but it also turned a missing case into an UnhandledMatchError at runtime. The arms and hasTokenRefreshFlow() currently agree on the same nine platforms, and nothing enforces that: adding a platform to the predicate without an arm would fail in production, on a queue worker, for one platform's accounts only. The test walks every platform claiming a refresh flow and fails by name if the match has no arm for it. Verified it catches the real thing by temporarily adding Facebook to the predicate — it failed with "facebook claims a refresh flow but refreshToken() has no arm for it" — rather than trusting a green run on code that already agrees with itself.
The test added in the previous commit swallowed every Throwable except UnhandledMatchError, so it proved a match arm existed and nothing more. Routing all nine refresh methods through refreshHttp() in af379af rewrote how each one builds its request, and this test would have passed just the same if one of those chains no longer worked. It now fails on any exception, naming the platform, and asserts each refresh actually put a request on the wire — a chain that breaks during construction raises before anything is sent, so an empty recording is the signal. Verified by breaking Pinterest's chain on purpose: "pinterest refresh threw BadMethodCallException: Method PendingRequest::withHeadersTypo does not exist." All nine send their request with the chains as they stand.
Contributor
Author
Verified against the live X APIRan the real job against api.x.com with a connected account, logging every request: Also confirmed live: the failure-path fallback returns Review roundsTwo independent reviews found 8 real regressions, all introduced by earlier commits on this branch, all fixed and pinned by tests. The worst: a rejected Instagram/Threads extension left the account
Result
3775 passed, 1 skipped. Pint clean. Details in the PR description. |
A max-effort review found six issues, three of which undo a trade the previous round got backwards. Every refresh method wrote the response straight over the stored credential: `'access_token' => data_get($data, 'access_token')`. A 200 carrying no token therefore destroyed a working one — and on Instagram and Threads, where refresh_token is set to the same value, both halves at once. The blank() check added in af379af only noticed after the damage was persisted, then marked the account TokenExpired, which RefreshExpiringTokens no longer selects — so one glitchy-but-successful response emailed the owner and forced a manual reconnect. Guard before the write instead, treat it as the platform misbehaving, and the stored pair survives for the next tick to retry. The detection branch downstream is now unreachable and gone. Tightening the refresh timeout to 8s was the wrong fix for the lock problem. refreshHttp() is shared with 24 publish and analytics call sites, and for X, Bluesky and TikTok the refresh_token is single-use: abandoning a request the provider has already processed loses the rotated pair permanently and costs the user a reconnect. Giving up sooner makes that more likely, not less. Restore generous timeouts and put the lock back above them. The cost of erring long is that a worker dying mid-refresh holds the lock while a publish falls through and retries — recoverable, unlike a lost rotation. Both constants now say which way they are wrong on purpose. The hasTokenRefreshFlow() guard in recordVerification() was dead: refreshToken() already returns false for those platforms, so the branch was never entered. The test claiming to cover it calls refreshToken() directly and never reaches it. The command reported "Dispatched N" for a number it cannot know. dispatch() returns a PendingDispatch whether or not ShouldBeUnique discarded it, so the count overstated itself during exactly the backlog someone reads that line to diagnose. It now reports accounts in the window, which is what it actually measured. Not changed: the daily sweep still skips accounts a refresh keeps fresh. That is the deliberate decision this PR is built on — a refresh replaces the access token rather than inspecting it, so there is nothing left for a billed read to confirm. Re-verified live after the changes: the real job still rotates the token against api.x.com and leaves the account connected.
The guard added in the previous commit protects nine refresh methods; only X had a test. Each provider reads a different field name out of the response, so a regression would land on one platform at a time and the suite would stay green for the other eight. The test drives every platform claiming a refresh flow through a 200 that carries no token, and requires each to refuse with PlatformUnavailableException — nothing is provably dead, so the next tick should retry rather than anyone being disconnected — while leaving the stored credential untouched. Verified it fails usefully by dropping the guard from one platform: 'threads should refuse a tokenless 200 cleanly, got QueryException: null value in column access_token violates not-null constraint'. Without naming the platform the failure reads as an unrelated database error, since the write also poisons the surrounding transaction.
A fifth review found the concurrent-refresh test passing without ever exercising what it claims to cover, and the reason it could is a real bug. access_token is an encrypted cast. The test wrote the winner's pair with DB::table()->update(), which stores plaintext, so reading it back raised DecryptException — and accessTokenStillWorks() caught Throwable and returned true. Green test, zero coverage of the recovery that justifies the method existing. It now writes through the model and asserts the verify call actually happened; removing the reload makes it fail. The catch is the bug. Treating any non-TokenExpiredException as "the token is healthy" means an APP_KEY rotation, a corrupted column, or an UnhandledMatchError from a newly added platform leaves the account Connected forever while every publish hard-fails, and nobody is told. It now names the outcomes that earn the benefit of the doubt — platform down, network dropped, account deleted mid-run — and lets the rest surface. Loud is right here: an APP_KEY rotation breaks every account at once, so failing the job where an operator sees it beats disconnecting every user. Refusing to persist a tokenless 200 also had no way out. The account kept retrying every 15 minutes forever, and the daily sweep counts PlatformUnavailableException as verified, so it was never disconnected and never reported. Now the retry only continues while there is a live token behind it: once that expires and renewal still fails, the connection is dead in practice and says so. Also corrects the refreshHttp() docblock, which claimed a blast radius the private method does not have — refreshToken() is what those 24 call sites reach — and records in VerifyWorkspaceConnections that short-TTL platforms never being re-verified is the intended consequence, not an oversight.
The escalation added last round was wrong, and two neighbouring paths had the same shape of bug. Marking the account expired whenever a PlatformUnavailableException hit an already-expired token looked like it closed a silent-rot gap. But that exception is what TokenRefreshClient raises for 5xx, 429 and connection timeouts — so X rate-limiting for forty minutes around a two-hour token's expiry disconnected the account, emailed the owner, and hard-failed every scheduled post, with only the daily sweep to undo it. The rot it was meant to prevent surfaces at publish time anyway. Reverted: a transient failure never disconnects. refresh_token was left unguarded when access_token was hardened. data_get() only falls back when a key is absent, so a provider answering with an explicit "refresh_token": null wiped the stored one — and the next tick then threw "no refresh token available" without making a single call. Guarded in the same four places, falling back on blank rather than on missing. A held lock reported "nothing refreshed" even when the token was already dead, handing the caller a credential it knew was expired. The publisher posts with it, takes a 401, and PublishToSocialPlatform finalises the post as failed and disconnects the account — over a lock a dying worker left behind, for the two minutes it survives. It now says transient, which is what a refresh someone else is already running actually is. VerifyWorkspaceConnections promoted TokenExpired accounts back to Connected on verifyAccount()'s return value, which is also true for "could not check, don't disconnect". An unreachable platform therefore told owners their reconnect had worked when nothing was verified. Promotion moved next to the successful verify. Each of the four is pinned by a test, and each test was checked by reverting the fix and confirming it fails.
Round seven found one real regression from round six, one consistency gap, and one stale comment. Reporting lock contention as transient was right for the publish path, which reschedules, but analytics calls refreshToken() bare and AnalyticsController has no try/catch — and there is no renderable handler for PlatformUnavailableException. A user opening analytics for an account whose token expired while the scheduled job held the lock got an HTTP 500 where the same request previously returned empty metrics. Reproduced before fixing: "Expected response status code [200] but received 500". The controller now degrades to empty numbers and still reports, which also covers the same 500 for any platform whose refresh 5xx'd — possible before this branch too. The fallback verify was throwing away a result worth keeping. It is a billed call on X and it proves the token alive exactly as a refresh does, so the pre-publish check was paying to ask the same question minutes later. Stamped like the other two sites. Also rewrites a comment in rotatedTokenFrom() that described the data_get() call it replaced rather than the blank() check beneath it. The review also reported a false @throws on that method; it has no docblock at all. Both fixes checked by reverting them and confirming the new tests fail.
The rescue() added last commit caught Throwable, so it did not just absorb a platform being down — it absorbed everything. A TypeError in any metrics service rendered as "this account has no activity", with a log line as the only sign anything was wrong. Reproduced: a metrics service throwing RuntimeException returned 200 with empty metrics. This is the same mistake the review flagged two rounds ago in accessTokenStillWorks(), where treating any exception as "the token is healthy" hid real failures. Narrowed the same way: PlatformUnavailableException and ConnectionException degrade to empty numbers and still report, everything else surfaces as the 500 it is. The match moved into a named method so the intent has somewhere to live, since the reason for the narrow catch matters more than the catch itself. Both directions are pinned: widening the catch back to Throwable fails the bug-is-not-hidden test, and removing the degradation fails the lock-collision test.
RefreshSocialToken had 72 comment lines against 95 of code — 43% of the file. The rest of the branch was heading the same way: two constants in ConnectionVerifier carried twelve- and eight-line docblocks, and VERIFIED_WITHIN_HOURS had twelve lines explaining a number. Most of it was history rather than reasoning: what the code used to do, which review round asked for a change, the full argument for a decision the code already states. Kept the parts a reader cannot recover — why a catch is narrow, why a stamp is not written inside refreshToken(), why the lock has to outlast the timeouts — and cut the rest. shouldTrustAWorkingAccessToken() went with it: a one-line method behind a twelve-line docblock, now the condition it wrapped, inline where it is used. No behaviour change; full suite unchanged at 3786.
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.
Nightwatch showed the X API traffic dominated by
RefreshSocialToken— a steady stream ofGET /2/users/meandPOST /2/oauth2/token, all day, for every connected account. Under X's pay-per-usage pricing that first endpoint is billed.Two reads turned out to buy nothing, for the same underlying reason: the data they fetched was already in hand. This removes both.
Part 1 — the token check
The problem
RefreshSocialTokenverified rotating-refresh platforms instead of refreshing them:On a still-valid token,
verify()only calls the platform's verify endpoint and leavestoken_expires_atuntouched. The account therefore stays insideRefreshExpiringTokens' 30-minute window and is re-read at every 15-minute tick until the token actually dies — then, and only then, is it refreshed.Two consequences:
GET /2/users/me, billed as a "User: Read" — $0.010 per resource. It returns a full profile; the code reads one boolean from it (ConnectionVerifier.php:435). It does not qualify for Owned Read pricing ($0.001): that list is closed,users/meisn't on it, and even the endpoints that are only qualify when{id}is the developer app's owner — never true for a multi-tenant app.The change
1. Refresh instead of verify. A provider that hands back a fresh token has already confirmed the credential — it rejects a revoked one with a 4xx, which
TokenRefreshClientmaps toTokenExpiredException. More than that: the refresh doesn't inspect the access token, it replaces it. After it returns, the stored token is seconds old and minted by the provider. There is nothing left for a verify call to check.2. Record the confirmation.
RefreshSocialTokenstampslast_verified_aton a successful refresh, guarded on the platform having a refresh flow and the response carrying a usable token.3. Let the sweep trust it.
VerifyWorkspaceConnectionsskips accounts verified within 12 hours — the patternVerifyUpcomingPostConnectionshas used since #255, never wired into the other callers. OnlyConnectedaccounts are skipped: aTokenExpiredone still needs the call, because verifying it is how it gets promoted back.Net effect: all three
verify()callers stop emitting billed reads for X. What remains on the bill is inherent to the product — post creates and analytics.Measured
Simulated 24h of the scheduler — all three scheduled commands, 96 ticks, 8 scheduled posts — against both branches, per platform, using each provider's real token TTL.
main: refresh + verify = totalWith the daily sweep and pre-publish checks included, X goes from 45 billed reads/account/day ($0.45) to 0.
Every platform makes strictly fewer API calls and spends zero time holding an expired token. YouTube was in worse shape than X and nobody knew: a 1-hour Google token entered the 30-minute window every hour, so 6 hours a day were spent on a dead token, with every publish in that stretch eating a 401 first.
X dedupes resources within a 24h UTC window, which had been absorbing some of this — but the docs call that a soft guarantee that "may result in resources not being deduplicated." This takes the number of chances to leak from 45 to zero.
Three regressions this branch introduced, found and fixed
Worth reading before approving — each was introduced by an earlier commit here, and each was found by testing against
main, not by reasoning.mainconnectedtoken_expiredconnectedtoken_expired#1 was not an edge case. Any X account with no stored
refresh_tokenwould hit it every cycle — 16×/day, forever, each time emailing the owner and failing their posts. And X's own developer community documents refresh tokens being invalidated spuriously, so it would also have fired on a known, recurring X quirk.The pattern in all three: the verify-first code had accumulated protections (
refreshThenVerify's reload-and-retry, the lock's early return) that the new path didn't inherit.The test written to cover #3 originally called
refreshToken()directly rather than going through the job, so it passed while the job path was broken. It now exercises the job and asserts no HTTP call was made.Two pre-existing concurrency gaps, closed here
Pulled in rather than deferred: the branch raises exposure to both from 12 to 16 refreshes/account/day, and they sit in the method it modifies.
Cache::lock(..., 30)against the HTTP client's 30s default read timeout, plus 10s connect, and no refresh method set its own. Bluesky is the ceiling with two sequential calls. Named the TTL (ConnectionVerifier::REFRESH_LOCK_SECONDS), set it past that, and a test asserts the invariant so a future slower refresh can't quietly break it.RefreshSocialTokenwasn't unique.RefreshExpiringTokensre-selects an account untiltoken_expires_atmoves, which only happens once the job runs — so a backlogged queue stacked one job per tick, each rotating a single-use refresh_token again for nothing. Keyed by account, matchingVerifyUpcomingPostConnections.Provider behaviour, verified against official docs
defaultTokenTtlSeconds()= 7200refreshJwtis a required output fieldThis corrected an error the branch introduced: a docblock claimed LinkedIn single-uses its refresh token. Its docs say the opposite. Now reads X and Bluesky, which is what the specs describe.
Two risk numbers gained documented bounds: Bluesky's
createSessionis rate-limited per handle at 30/5min and 300/day, far above our 16 attempts/day; and Google doesn't rotate, so YouTube's 24 → 48 refreshes carry no single-use risk at all.Residual risk
refreshXTokenmakes the HTTP call and the$account->update()as two steps with$tries = 1. A worker death in that gap loses the new pair while the provider has already invalidated the old one — the connection is unrecoverable and the user must reconnect.Pre-existing, and irreducible: none of the five provider specs offers any way to acknowledge receipt of a rotated token. What this branch controls is how often we enter the window — 12 → 16 refreshes/account/day (+33%), now bounded to one per account per cycle regardless of queue state.
Only X and Bluesky are exposed (single-use rotation). TikTok is unchanged at 1/day; YouTube, Pinterest and LinkedIn don't rotate.
If that matters more than the ~$0.45/account/day, narrowing the lead in
RefreshExpiringTokensfrom 30 to ~5 minutes brings it to ~12.5/day, at the cost of less headroom for queue backlog. Not done without a decision.Behaviour vs.
main, case by casemainrefresh_tokenstored, access token aliveOne case differs by design: a legacy row on a no-refresh platform (Facebook, Mastodon, Telegram, Discord) that carries a
token_expires_at.mainverifies it every 15 minutes forever — 96 wasted calls/day, since nothing ever moves its expiry. Here it's a no-op, and detection moves to the daily sweep (24h instead of 15 min). Those platforms storenullat connect, so this only concerns hypothetical old data.Part 2 — the analytics double read
XAnalytics::getMetricsfetched the account's timeline for post ids, then looked the same ids up again throughGET /2/tweets, purely to read thepublic_metricsthe first request could have returned. The timeline call asked forstart_time,end_timeandmax_results— nevertweet.fields.Both endpoints bill per Post returned. One analytics load for an account with 250 posts in range:
mainThis one probably doesn't save dollars, and it's worth saying so. X deduplicates a resource within a 24-hour UTC window, so the second read of an id already read that day isn't charged again. What it buys is half the round-trips on a user-facing page, half the rate-limit consumption for the same answer, and no longer resting 250 resources' worth of billing per load on a guarantee the docs explicitly call soft ("may result in resources not being deduplicated").
Behaviour is unchanged: same totals, same 5-page ceiling, same empty result when nothing was posted in range. The two helpers collapse into one paginated pass that sums metrics as pages arrive, and the page cap becomes a named constant — it bounds what one load can cost as much as how long it takes, which a bare
5didn't say.Tests
3767 passed, 1 skipped. Pint clean.
19 tests on
RefreshSocialToken, 10 onVerifyWorkspaceConnections, and the first 3 forXAnalytics::getMetrics— it had none: metrics come from the timeline instead of a second lookup, the timeline request asks forpublic_metrics, and metrics accumulate across paginated pages. New coverage includes: refresh renews without a billed read; a successful refresh stampslast_verified_at; a refresh whose follow-up verify fails does not; an empty access token in a 200 does not; a lock-skipped refresh does not; a platform with nothing to refresh does not; a rejected refresh doesn't disconnect a working account; norefresh_tokendoesn't either; a rejected refresh does disconnect once the access token is dead too; a lost rotation race falls back to the winner's token; a rejected refresh isn't re-sent before the access token is checked; the lock outlives the slowest possible refresh; a backlogged queue can't stack duplicate jobs.Two existing tests were rewritten to assert the new policy rather than the old one —
refresh job routes through verify (access-token-first) not refreshToken, andproactive refresh does NOT rotate the X refresh token while the access token still works. The concern behind the original policy is still guarded: rotation must leave the accountConnected.Not in this PR
since/untilcome straight off the request without validation (AnalyticsController.php:72), capped at 100 days inside the service. For a heavy poster on a long range that's $2.50 per UTC day, per account. Reading fewer posts changes what the numbers mean, so it's a product decision rather than a cleanup.token_expires_atis hardcoded toaddHours(2)inBlueskyController.php:94and inrefreshBlueskyToken, rather than read from theaccessJwt'sexpclaim. Pre-existing; the real value is unverified.Post: Create (with URL)costs $0.20 vs $0.015 without — 13× — and is untracked. For a scheduling product where most posts carry a link, this may well be the larger share of the bill. The Developer Console's by-endpoint breakdown settles it.