Repository navigation
feat(plugins): ✨ renewable update-check lease, fenced writes and update alerts across AdminCP - #882
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
React Doctor found 1 new issue in 1 file · 1 warning · score 91 / 100 (Great) · 1 fixed · vs Reviewed by React Doctor for commit |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67e053e049
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * them, including the first insert when no row exists yet; the row lock also | ||
| * holds off a writer that does not take the advisory lock, such as an instance | ||
| * still running the previous release during a rolling deploy. Both are held |
There was a problem hiding this comment.
During a rolling deployment, an instance running the previous implementation can already hold the lease and continue writing core_update_sources and core_update_results directly after its five-minute lease expires. A new instance can then take over and commit newer results, after which the old instance can overwrite them because those writes never touch this row or advisory lock. The row lock only delays an old instance's lease statement; it does not fence a check that acquired its lease before the upgrade, so the stated rolling-deploy protection does not hold when such a check outlives its lease.
Useful? React with 👍 / 👎.
| const confirmedAt = Date.now(); | ||
| const lease = await acquireUpdateCheckLease(options.db, { manual }); |
There was a problem hiding this comment.
If acquisition waits on the advisory lock, a busy connection pool, or a slow transaction, confirmedAt includes that wait even though PostgreSQL creates a fresh five-minute lease only when acquisition succeeds. A wait approaching UPDATE_CHECK_LEASE_MS therefore makes the heartbeat abort immediately after successfully taking the lease, returning lease_lost without performing the check. Base the initial deadline on the successful acquisition (ideally using an expiry or timing bound returned by it), not on a timestamp captured before the blocking call.
Useful? React with 👍 / 👎.
…te alerts across AdminCP - Renew the update-check lease about once a minute from a non-overlapping heartbeat; validity uses the database clock. Lease loss, renewal errors or an unconfirmed deadline abort the check through an AbortSignal that reaches source fetching; cancellation is never stored as a publisher failure. - Fence every checker write with withUpdateCheckLease: a short transaction that takes the lease's advisory lock and row lock and re-checks ownership before writing. Acquire, renew and release share the same locking model. - Add GET /admin/plugins/updates/summary (plugins.can_view), read from stored results only, and one shared TanStack query for the AdminCP. - Show the available update count next to System → Plugins, a security banner in the AdminCP shell and a "System updates" dashboard widget. - Document alerts, freshness labels and the lease/fencing model. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F5u4nFU9nJ6kuXBV7e3VBj
…update alerts - Heartbeat: renewal past the first lease, no overlapping renewals, loss, renewal errors, the deadline for a renewal that never answers, timer cleanup. - Checker: lease loss aborts in-flight and queued sources, settles every worker, stores nothing and never records cancellation as a failure. - Write fence: a lost or expired owner cannot persist sources, results, failures or clean-up deletions. - Real-Postgres integration suite (VITNODE_TEST_POSTGRES_URL) proving a takeover waits for the old owner's fenced transaction. - Summary counts, the summary route's permission, and the nav count, security banner, dashboard widget and shared-query refresh. fix(plugins): 🐛 stop waiting for a renewal the heartbeat already gave up on, so a database call that never answers cannot hang the check's caller. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F5u4nFU9nJ6kuXBV7e3VBj
67e053e to
2a7bf4e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a7bf4e915
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await heartbeat.stop(); | ||
| // A failure here must not hide the check's own outcome; an unreleased lease | ||
| // simply runs out within UPDATE_CHECK_LEASE_MS. | ||
| await releaseUpdateCheckLease(options.db, lease.owner).catch(() => false); |
There was a problem hiding this comment.
When a renewal never answers after acquiring the transaction advisory lock, heartbeat.stop() now returns at its deadline, but this subsequent release immediately waits for the same advisory lock with no deadline. Consequently the manual request or cron invocation can still hang indefinitely in exactly the stuck-renewal scenario the heartbeat timeout is intended to handle; the release must also be bounded or the outstanding renewal must be cancelled.
Useful? React with 👍 / 👎.
…teSecurityPill` for improved security update notifications
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| sourceUrl: component.sourceUrl, | ||
| versions: component.versions, | ||
| }; | ||
| await tx |
There was a problem hiding this comment.
React Doctor · react-doctor/async-await-in-loop (warning)
This for…of loop waits before starting the next iteration. If iterations perform independent asynchronous work, consider bounded concurrency; await syntax alone does not establish a speedup.
Fix → Consider concurrent calls only for independent asynchronous work. Shared queues or synchronous work may not benefit. Preserve resource limits, transaction ordering, and failure/cancellation semantics; observe all callback promises.
There was a problem hiding this comment.
💡 Codex Review
Refresh summaries after background checks
When the daily cron or a check in another browser changes the stored results while this AdminCP shell is mounted, its cached running value is normally false, so this callback disables polling. Allowing staleTime to elapse does not itself refetch an active TanStack query; with only focus refetching, the navigation badge and security pill can continue showing old or missing security updates indefinitely in an active tab. Keep a low-frequency interval while idle or otherwise push/invalidate background changes.
AGENTS.md reference: AGENTS.md:L55-L55
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
2 participants
heartbeat; validity uses the database clock. Lease loss, renewal errors or
an unconfirmed deadline abort the check through an AbortSignal that reaches
source fetching; cancellation is never stored as a publisher failure.
that takes the lease's advisory lock and row lock and re-checks ownership
before writing. Acquire, renew and release share the same locking model.
results only, and one shared TanStack query for the AdminCP.
banner in the AdminCP shell and a "System updates" dashboard widget.
Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01F5u4nFU9nJ6kuXBV7e3VBj