Skip queueing duplicate certificate generation event by hmacr · Pull Request #10942 · appwrite/appwrite

hmacr · 2025-12-12T08:24:22Z

Related SER-287

coderabbitai · 2025-12-12T08:24:41Z

📝 Walkthrough

Walkthrough

The guard condition in Update::action (Verification/Update) was changed to short-circuit not only when a rule is already verified but also when its status is 'verifying'. A comment explaining the added condition was added above the check to clarify intent and prevent re-queuing during ongoing certificate generation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Focused scope: Single-file, single-method change
Straightforward logic: Additional OR condition in an existing guard clause
Low heterogeneity: One consistent change

Areas requiring attention:

Confirm the 'verifying' status string matches status values used elsewhere
Check for potential race conditions with concurrent verification requests
Ensure legitimate verification retries are not inadvertently blocked

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Skip queueing duplicate certificate generation event' accurately describes the main change: preventing re-queueing of certificate generation when a rule is already being verified.
Description check	✅ Passed	The description provides a minimal but valid reference to the related issue (SER-287), which is on-topic even though it lacks detailed explanation.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch hmacr/skip-certificate-queueing

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a05010e and 68358ac.

📒 Files selected for processing (1)

src/Appwrite/Platform/Modules/Proxy/Http/Rules/Verification/Update.php (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

src/Appwrite/Platform/Modules/Proxy/Http/Rules/Verification/Update.php

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)

GitHub Check: E2E Service Test (VCS)
GitHub Check: E2E Service Test (Webhooks)
GitHub Check: E2E Service Test (Sites)
GitHub Check: E2E Service Test (Databases/TablesDB)
GitHub Check: E2E Service Test (Account)
GitHub Check: E2E Service Test (Site Screenshots)
GitHub Check: E2E General Test
GitHub Check: Benchmark
GitHub Check: scan

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions · 2025-12-12T08:27:01Z

Security Scan Results for PR

Docker Image Scan Results

Package	Version	Vulnerability	Severity
libpng	1.6.51-r0	CVE-2025-66293	HIGH
libpng-dev	1.6.51-r0	CVE-2025-66293	HIGH

Source Code Scan Results

🎉 No vulnerabilities found!

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/Appwrite/Platform/Modules/Proxy/Http/Rules/Verification/Update.php (1)

132-135: Extract rule status values to shared constants

The 'verifying' status is used as a raw string literal across multiple files in the proxy rules module (Update.php, Site/Create.php, Redirect/Create.php, Function/Create.php, API/Create.php). Without a centralized constant, there's risk of inconsistency if the status name changes or if new code introduces typos. Create a status constants class (e.g., RuleStatus with const VERIFYING = 'verifying') and wire it throughout the proxy rules module.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 278679a and a05010e.

📒 Files selected for processing (1)

src/Appwrite/Platform/Modules/Proxy/Http/Rules/Verification/Update.php (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

GitHub Check: Setup & Build Appwrite Image
GitHub Check: Setup & Build Appwrite Image
GitHub Check: scan

🔇 Additional comments (1)

src/Appwrite/Platform/Modules/Proxy/Http/Rules/Verification/Update.php (1)

132-135: The early-return guard is appropriate. The finally block at line 225 ensures saveCertificateDocument always executes and updates the rule status to either 'verified' or 'unverified', even if the certificate job fails. Status will never remain stuck in 'verifying' after the worker completes. The 'verification' attribute check appears unused (never set to true anywhere in the codebase). No additional stale-timeout or force-retry mechanism is needed.

github-actions · 2025-12-12T08:37:55Z

✨ Benchmark results

Requests per second: 1,129
Requests with 200 status code: 203,255
P99 latency: 0.173830237

⚡ Benchmark Comparison

Metric	This PR	Latest version
RPS	1,129	1,194
200	203,255	215,004
P99	0.173830237	0.172225564

hmacr · 2025-12-16T05:57:55Z

Superseded by #10962

Skip queueing duplicate certificate generation event

a05010e

coderabbitai bot reviewed Dec 12, 2025

View reviewed changes

incorrect verified check

68358ac

stnguyen90 approved these changes Dec 12, 2025

View reviewed changes

hmacr closed this Dec 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip queueing duplicate certificate generation event#10942

Skip queueing duplicate certificate generation event#10942
hmacr wants to merge 2 commits into1.8.xfrom
hmacr/skip-certificate-queueing

hmacr commented Dec 12, 2025

Uh oh!

coderabbitai bot commented Dec 12, 2025 •

edited

Loading

Walkthrough

Estimated code review effort

Uh oh!

github-actions bot commented Dec 12, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

github-actions bot commented Dec 12, 2025 •

edited

Loading

Uh oh!

hmacr commented Dec 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

hmacr commented Dec 12, 2025

Uh oh!

coderabbitai bot commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

github-actions bot commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security Scan Results for PR

Docker Image Scan Results

Source Code Scan Results

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✨ Benchmark results

⚡ Benchmark Comparison

Uh oh!

hmacr commented Dec 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai bot commented Dec 12, 2025 •

edited

Loading

github-actions bot commented Dec 12, 2025 •

edited

Loading

github-actions bot commented Dec 12, 2025 •

edited

Loading