Repository navigation
outpost: pull Polar's changes instead of holding a WebSocket - #16332
maximevast wants to merge 12 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
maximevast
changed the title
maximevast
changed the title
maximevast
changed the title
This comment has been minimized.
This comment has been minimized.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 30 files
This PR changes concurrency-sensitive code such as locks, queues, or retries. Ultrareviews find 2.4x more serious bugs than standard reviews. Comment @cubic-dev-ai ultrareview to run one.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Replaces the Outpost WebSocket with Redis-coordinated HTTP pulls in both SDKs, adds a metering flush(), and removes ws/httpx-ws dependencies. This is a protocol and coordination redesign that needs human sign-off.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Replaces Outpost's WebSocket with Redis-coordinated HTTP pulls in both SDKs, removing ws/httpx-ws deps and connect_websocket while adding flush(). Needs sign-off because the server-side contract deployment and public API change are unverified.
View guided diff | Turn on auto-fix | Re-trigger cubic
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
3 issues found across 8 files (changes from recent commits).
Confidence score: 3/5
- In
sdk/generator/python/template/polar/outpost/client.py, about 1,025 consecutive transient failures can make the uncapped backoff overflow, escaping the handler. Cap the exponent so retries can continue. - In
sdk/generator/typescript/template/src/outpost/client.tsandsdk/generator/python/template/polar/outpost/client.py, a long snapshot confirmation can outlastPULL_LOCK_TTL, allowing another instance to pull concurrently. Renew the lock during confirmation.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="sdk/generator/typescript/template/src/outpost/client.ts">
<violation number="1" location="sdk/generator/typescript/template/src/outpost/client.ts:895">
P2: This extends the pull lock only after the entire snapshot confirmation finishes. A snapshot with enough sealed buckets or confirm batches can exceed `PULL_LOCK_TTL`, letting another instance pull concurrently before this call runs; renew the lock during confirmation or otherwise bound the snapshot work below the TTL.</violation>
</file>
<file name="sdk/generator/python/template/polar/outpost/client.py">
<violation number="1" location="sdk/generator/python/template/polar/outpost/client.py:1221">
P2: The backoff exponent is unbounded. After about 1025 consecutive transient failures, `PULL_RETRY_DELAY * 2 ** (self._pull_failures - 1)` overflows the float conversion. The `OverflowError` escapes the except handler and takes down the task group. Cap the exponent so the delay stays at `MAX_PULL_RETRY_DELAY`.</violation>
<violation number="2" location="sdk/generator/python/template/polar/outpost/client.py:1264">
P2: This extends the pull lock only after the entire snapshot confirmation finishes. A snapshot with enough sealed buckets or confirm batches can exceed `PULL_LOCK_TTL`, letting another instance pull concurrently before this call runs; renew the lock during confirmation or otherwise bound the snapshot work below the TTL.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
View guided diff | Turn on auto-fix | Re-trigger cubic
302a52e to
c19e389
Compare
There was a problem hiding this comment.
All reported issues were addressed across 13 reviewed files. 10 files intentionally excluded from review.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
c19e389 to
49f06ac
Compare
There was a problem hiding this comment.
0 issues found across 12 reviewed files. 5 files intentionally excluded from review.
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 4 unresolved issues from previous reviews.
View guided diff | Turn on auto-fix | Re-trigger cubic
49f06ac to
1a603ca
Compare
There was a problem hiding this comment.
2 issues found across 8 reviewed files. 5 files intentionally excluded from review.
Confidence score: 3/5
FAIL_PULL_SCRIPTcan let an expired owner overwrite a newer successful pull’s state and back off the fleet. Check lock ownership before recording a failure.client.pycan keep staleretry_atafter a Pub/Sub disconnect and pull during the shared backoff. Re-read the pull metadata before relying on the local timer state.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="sdk/generator/python/template/polar/outpost/client.py">
<violation number="1" location="sdk/generator/python/template/polar/outpost/client.py:537">
P2: This loads `retry_at` only at startup, so a Pub/Sub disconnect can lose a `failed` notification while the timer keeps using stale local state and pulls during the shared backoff. Re-read the pull metadata when the Redis listener reconnects, or make pull-lock acquisition enforce `retry_at` atomically.</violation>
</file>
<file name="sdk/generator/outpost/fail_pull.lua">
<violation number="1" location="sdk/generator/outpost/fail_pull.lua:3">
P2: `FAIL_PULL_SCRIPT` records failures without checking lock ownership. A frozen owner can resume after its TTL, overwrite a newer successful pull with `retry_at`, and unnecessarily back off the fleet; make failure recording lock-token-aware or perform it before releasing the owned lock.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| self._pull_interval = ( | ||
| float(next_poll_in) / 1000 if next_poll_in else PULL_INTERVAL | ||
| ) | ||
| self._pull_retry_at = float(retry_at) / 1000 if retry_at else 0.0 |
There was a problem hiding this comment.
P2: This loads retry_at only at startup, so a Pub/Sub disconnect can lose a failed notification while the timer keeps using stale local state and pulls during the shared backoff. Re-read the pull metadata when the Redis listener reconnects, or make pull-lock acquisition enforce retry_at atomically.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At sdk/generator/python/template/polar/outpost/client.py, line 537:
<comment>This loads `retry_at` only at startup, so a Pub/Sub disconnect can lose a `failed` notification while the timer keeps using stale local state and pulls during the shared backoff. Re-read the pull metadata when the Redis listener reconnects, or make pull-lock acquisition enforce `retry_at` atomically.</comment>
<file context>
@@ -520,13 +527,14 @@ class Outpost:
self._pull_interval = (
float(next_poll_in) / 1000 if next_poll_in else PULL_INTERVAL
)
+ self._pull_retry_at = float(retry_at) / 1000 if retry_at else 0.0
self._tasks.start_soon(self._pull_on_time)
except BaseException:
</file context>
| @@ -0,0 +1,8 @@ | |||
| -- A pull failed: the whole fleet waits before the next one, twice as long after each failure in | |||
| -- a row, up to ARGV[3] milliseconds. | |||
| local failures = redis.call('HINCRBY', KEYS[1], 'failures', 1) | |||
There was a problem hiding this comment.
P2: FAIL_PULL_SCRIPT records failures without checking lock ownership. A frozen owner can resume after its TTL, overwrite a newer successful pull with retry_at, and unnecessarily back off the fleet; make failure recording lock-token-aware or perform it before releasing the owned lock.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At sdk/generator/outpost/fail_pull.lua, line 3:
<comment>`FAIL_PULL_SCRIPT` records failures without checking lock ownership. A frozen owner can resume after its TTL, overwrite a newer successful pull with `retry_at`, and unnecessarily back off the fleet; make failure recording lock-token-aware or perform it before releasing the owned lock.</comment>
<file context>
@@ -0,0 +1,8 @@
+-- A pull failed: the whole fleet waits before the next one, twice as long after each failure in
+-- a row, up to ARGV[3] milliseconds.
+local failures = redis.call('HINCRBY', KEYS[1], 'failures', 1)
+local delay = math.min(tonumber(ARGV[3]), tonumber(ARGV[2]) * 2 ^ math.min(failures - 1, 16))
+local retry_at = tonumber(ARGV[1]) + delay
</file context>
5eafb7b to
fcb8612
Compare
…olding a WebSocket (RED)
…ypeScript SDK (GREEN)
…ng a WebSocket (RED)
…ython SDK (GREEN)
fcb8612 to
b6960f7
Compare
There was a problem hiding this comment.
2 issues found across 8 files (changes from recent commits).
Confidence score: 3/5
- In
sdk/generator/typescript/template/src/outpost/client.tsandsdk/generator/python/template/polar/outpost/client.py, removing periodic resync can leave cached states stale indefinitely if pub/sub messages are lost without a disconnect event. Keep the periodic resync or add a pub/sub health check.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="sdk/generator/python/template/polar/outpost/client.py">
<violation number="1" location="sdk/generator/python/template/polar/outpost/client.py:1178">
P2: Removing the periodic `_refresh_states()` leaves no recovery path for pub/sub messages that are lost without a disconnect. Keep the periodic resync, or add a `health_check_interval` to the pubsub connection so a silently dead subscription raises and reconnects.</violation>
</file>
<file name="sdk/generator/typescript/template/src/outpost/client.ts">
<violation number="1" location="sdk/generator/typescript/template/src/outpost/client.ts:531">
P2: Skipping the periodic `synchronize()` whenever `isSynced` is true removes the only self-healing for missed pub/sub messages. If the subscriber connection dies without a `close` event, cached states never reload until a reconnect, and reads keep returning them. Keep the periodic reload, or confirm that the subscriber's liveness is detected some other way. Also update the `sync_interval` comment in `sdk/generator/outpost/constants.toml` to match.</violation>
</file>
Requires human review: Auto-approval skipped because cubic reviewed only this push, not the earlier force-push. Comment @cubic review to review the whole pull request.
View guided diff | Turn on auto-fix | Re-trigger cubic
| assert self._tasks is not None | ||
| self._tasks.start_soon(self._report_redis_size) | ||
| await self._refresh_states() | ||
| # Pub/sub delivers every update while connected: only a |
There was a problem hiding this comment.
P2: Removing the periodic _refresh_states() leaves no recovery path for pub/sub messages that are lost without a disconnect. Keep the periodic resync, or add a health_check_interval to the pubsub connection so a silently dead subscription raises and reconnects.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At sdk/generator/python/template/polar/outpost/client.py, line 1178:
<comment>Removing the periodic `_refresh_states()` leaves no recovery path for pub/sub messages that are lost without a disconnect. Keep the periodic resync, or add a `health_check_interval` to the pubsub connection so a silently dead subscription raises and reconnects.</comment>
<file context>
@@ -1175,7 +1175,8 @@ async def listen() -> None:
assert self._tasks is not None
self._tasks.start_soon(self._report_redis_size)
- await self._refresh_states()
+ # Pub/sub delivers every update while connected: only a
+ # reconnect needs a resync.
next_refresh = anyio.current_time() + SYNC_INTERVAL
</file context>
| void this.reportRedisSize(); | ||
| if (this.synchronizing) return; | ||
| // Pub/sub delivers every update while connected: only a reconnect needs a resync. | ||
| if (this.synchronizing || this.isSynced) return; |
There was a problem hiding this comment.
P2: Skipping the periodic synchronize() whenever isSynced is true removes the only self-healing for missed pub/sub messages. If the subscriber connection dies without a close event, cached states never reload until a reconnect, and reads keep returning them. Keep the periodic reload, or confirm that the subscriber's liveness is detected some other way. Also update the sync_interval comment in sdk/generator/outpost/constants.toml to match.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At sdk/generator/typescript/template/src/outpost/client.ts, line 531:
<comment>Skipping the periodic `synchronize()` whenever `isSynced` is true removes the only self-healing for missed pub/sub messages. If the subscriber connection dies without a `close` event, cached states never reload until a reconnect, and reads keep returning them. Keep the periodic reload, or confirm that the subscriber's liveness is detected some other way. Also update the `sync_interval` comment in `sdk/generator/outpost/constants.toml` to match.</comment>
<file context>
@@ -527,7 +527,8 @@ export class Outpost {
void this.reportRedisSize();
- if (this.synchronizing) return;
+ // Pub/sub delivers every update while connected: only a reconnect needs a resync.
+ if (this.synchronizing || this.isSynced) return;
void this.synchronize();
}, SYNC_INTERVAL);
</file context>
| if (this.synchronizing || this.isSynced) return; | |
| if (this.synchronizing) return; |
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Confidence score: 2/5
- In
sdk/generator/typescript/template/src/outpost/client.ts, reconnect recovery restores onlythis.channel, so cached customers stop receiving updates after a disconnect. Restore their subscriptions insynchronize()or enable automatic resubscription.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="sdk/generator/typescript/template/src/outpost/client.ts">
<violation number="1" location="sdk/generator/typescript/template/src/outpost/client.ts:984">
P1: Per-customer channels are not restored after a subscriber reconnect. The subscriber sets `autoResubscribe: false`, and `synchronize()` re-subscribes only `this.channel`. Cached customers then stop receiving updates and keep stale state until eviction. Re-subscribe the cached identities' update channels in `synchronize()`. The `.catch(() => {})` also hides a failed subscribe, so the entry stays unsubscribed with no retry.</violation>
</file>
Requires human review: Auto-approval skipped because cubic reviewed only this push, not the earlier force-push. Comment @cubic review to review the whole pull request.
View guided diff | Turn on auto-fix | Re-trigger cubic
| local = { | ||
| pending: Promise.resolve(), | ||
| following: ( | ||
| this.subscriber?.subscribe(this.updatesChannel(identity)) ?? Promise.resolve() |
There was a problem hiding this comment.
P1: Per-customer channels are not restored after a subscriber reconnect. The subscriber sets autoResubscribe: false, and synchronize() re-subscribes only this.channel. Cached customers then stop receiving updates and keep stale state until eviction. Re-subscribe the cached identities' update channels in synchronize(). The .catch(() => {}) also hides a failed subscribe, so the entry stays unsubscribed with no retry.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At sdk/generator/typescript/template/src/outpost/client.ts, line 984:
<comment>Per-customer channels are not restored after a subscriber reconnect. The subscriber sets `autoResubscribe: false`, and `synchronize()` re-subscribes only `this.channel`. Cached customers then stop receiving updates and keep stale state until eviction. Re-subscribe the cached identities' update channels in `synchronize()`. The `.catch(() => {})` also hides a failed subscribe, so the entry stays unsubscribed with no retry.</comment>
<file context>
@@ -971,9 +977,21 @@ export class Outpost {
+ local = {
+ pending: Promise.resolve(),
+ following: (
+ this.subscriber?.subscribe(this.updatesChannel(identity)) ?? Promise.resolve()
+ ).catch(() => {}),
+ missingUntil: 0,
</file context>
There was a problem hiding this comment.
0 issues found across 4 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 5 unresolved issues from previous reviews.
View guided diff | Turn on auto-fix | Re-trigger cubic
Summary
Outpost no longer holds a WebSocket (PLR-423), in both SDKs.
One pull per merchant Redis. The instance that takes a lock in the merchant's Redis pulls
/v1/outpost/changes. It applies each change, then moves the cursor stored in that Redis. A frozen instance only holds the lock until it expires, and a failed pull leaves the cursor where it was.Cold means absent from the shared Redis. One instance of the fleet fetches a cold customer; the others get the state through the existing Redis pub/sub.
When reads pull. Time counts from the last successful pull, in multiples of
next_poll_in(30 s by default):Long-running processes also pull on a timer.
metering.flush()waits for a pull in flight.Confirms are plain requests with their answer, so superseded replies (PLR-410) cannot happen.
ws,httpx-wsandwsprotoare gone.Depends on the server stack (PLR-420 to 422) being deployed.
Local run against the server stack, with N instances on one Redis for 75 s:
Checklist
uv run task lint && uv run task lint_types)