fix(placement): single-flight the placement requester registration#420
Conversation
) "Any available node" spawn failed with `RelayError: Invalid agent token` on the first try after a fresh Pear launch. Relaycast stores exactly one `token_hash` per agent row (relaycast packages/engine/src/db/schema.ts:63) and authenticates by looking an agent up on that column (packages/engine/src/auth/index.ts:51). `registerOrRotate` is register -> 409 -> rotate, so a second registration of a name OVERWRITES the first caller's token rather than adding one. There is no expiry check anywhere in that path: a token dies only when something re-registers its name. `getPlacementRelay` cached the RESOLVED client after two awaits, keyed on the deterministic identity `pear-requester-<projectId>`. Concurrent callers (the spawn dialog's listNodes effect racing placeAgent) all missed the cache and each registered that one identity; the last write won and the earlier callers were left holding a freshly-minted, already-superseded token -> 401 on the first placement. Cache the in-flight promise instead, so N concurrent callers await a single registration and exactly one token is minted per session. Evict on failure so a transient registration error doesn't poison placement for the session's lifetime, and compare identity before evicting so a late rejection can't clear a newer healthy entry. Reproduced RED-first: three concurrent listNodes calls produced three registrations against one identity. Needs no StrictMode double-invoke — plain concurrency is sufficient, so the defect is real in packaged builds too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request resolves a race condition where concurrent placement callers would register the same deterministic identity and rotate each other's tokens, leading to 401 errors. It addresses this by caching the in-flight Promise<AgentRelay> instead of the resolved client, ensuring a single-flight registration, and evicting the cache on failure. Unit tests have been added to verify this behavior. The review feedback suggests avoiding a potential linter warning by not referencing a variable within its own initializer, and using setImmediate instead of setTimeout in the test mock to yield to the event loop more robustly.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const pending = this.buildPlacementRelay(session).catch((error: unknown) => { | ||
| if (session.placementRelay === pending) session.placementRelay = undefined | ||
| throw error | ||
| }) | ||
| session.placementRelay = pending | ||
| return pending |
There was a problem hiding this comment.
Referencing pending inside its own initializer/declaration can trigger static analysis or linter warnings (such as no-use-before-define). We can avoid this by assigning session.placementRelay first, and then attaching a .catch handler to the promise for the side effect of clearing the cache on failure. Since .catch returns a new promise but does not mutate the original, the returned pending promise will still reject and propagate the error to the callers as expected.
const pending = this.buildPlacementRelay(session)
session.placementRelay = pending
pending.catch(() => {
if (session.placementRelay === pending) session.placementRelay = undefined
})
return pending| placementSdkMock.registerCalls.push(input.name) | ||
| // Await a macrotask so concurrent callers overlap here — this is the | ||
| // window between getPlacementRelay's cache check and its cache write. | ||
| await new Promise((resolve) => setTimeout(resolve, 0)) |
There was a problem hiding this comment.
Using setTimeout with a delay of 0 to yield to the event loop can be fragile in test environments if fake timers are enabled (either globally or in other tests within the suite). Using setImmediate is more robust and standard in Node.js test environments to schedule a macrotask without relying on timer mocks.
| await new Promise((resolve) => setTimeout(resolve, 0)) | |
| await new Promise((resolve) => setImmediate(resolve)) |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
1 participant
Fixes the operator-reported
RelayError: Invalid agent tokenon "Any available node" spawn, first try after a fresh Pear launch.Root cause — confirmed from source, not inferred
Relaycast stores exactly one
token_hashper agent row and authenticates by looking the agent up on that column:relaycast/packages/engine/src/db/schema.ts:63—tokenHash: text('token_hash').notNull().unique()relaycast/packages/engine/src/auth/index.ts:51-53— no matching row →unauthorized('Invalid agent token', 'agent_token_invalid')Only two writers exist:
createAgent(plain INSERT, 409s on an existing name) androtateAgentToken(the sole overwriter, request-scoped). SoregisterOrRotate= register → 409 → rotate, and a second registration of a name overwrites the first caller's token rather than adding one. There is no expiry check anywhere in that path — a token dies only when something re-registers its name.getPlacementRelaycached the resolved client atbroker.ts:2595, two awaits after the cache check at:2571, keyed on the deterministic identitypear-requester-<projectId>. Concurrent callers — the spawn dialog'slistNodeseffect racingplaceAgent— all missed the cache and each registered that one identity. Last write wins; earlier callers hold a freshly-minted, already-superseded token → 401 on the first placement.This corrects two premises in the original diagnosis: the failure is supersession, not expiry (so re-mint-on-401 would not have helped), and the 401 originates in the
relaycastrepo —relay/crates/broker/src/relaycast/auth.rs:953, previously cited as the source, is a string insidemod tests.Fix
Cache the in-flight promise rather than the resolved client, so N concurrent callers await one registration and exactly one token is minted per session. Evict on failure so a transient registration error can't poison placement for the session's lifetime, comparing identity before evicting so a late rejection can't clear a newer healthy entry.
Verification
Reproduced RED-first before the fix — three concurrent
listNodescalls produced three registrations against one identity:Both tests are wired into the suite as regressions:
broker.test.ts109/109 pass, full suite withplacement-helpers.test.ts120/120,typecheck:nodeclean.Note: this reproduces with plain concurrency and no StrictMode double-invoke, so the defect is real in packaged builds — the dev-vs-packaged question affects only how easily it fires, not whether.
Scope
getPlacementRelay+ its tests. No behavior change beyond the dedupe.dropSessionneeds no change (already setsundefined, still correct for a promise).🤖 Generated with Claude Code