Visitar URL original
fix: linearize scheduled execution cancel with publish by HarshMN2345 · Pull Request #14225 · appwrite/appwrite · GitHub
Skip to content

fix: linearize scheduled execution cancel with publish - #14225

Draft
HarshMN2345 wants to merge 1 commit into
mainfrom
cursor/fix-scheduled-execution-cancel-cbf3
Draft

HarshMN2345 wants to merge 1 commit into
mainfrom
cursor/fix-scheduled-execution-cancel-cbf3

Conversation

@HarshMN2345

@HarshMN2345 HarshMN2345 commented Oct 7, 2026 •

Copy link
Copy Markdown
Member

What does this PR do?

Scheduled execution cancellation raced with the functions worker's claim, queue publication, and cleanup.

  1. The worker claimed a schedule (active=false). A concurrent cancel saw that claim and was rejected. If publication then failed, the worker restored active=true, so the execution could still run.
  2. If publication succeeded but schedule cleanup failed, cancelling the inactive schedule could still report success even though the function job was already queued.

Publication and cancellation now share one coordinator, scheduleForExecutions, registered as an object dependency. It holds lock:platform:schedules:{scheduleId} across claim, publish, cleanup, failed-publish restore, and cancel.

  • 204 only when the schedule is still active under that lock, so publication was prevented.
  • A cancel that loses to a successful publish is rejected as execution_in_progress (400), including when cleanup left the row inactive.
  • A failed publish restores active=true before it releases the lock, so a cancel that was waiting deletes the schedule instead of leaving it runnable.
  • If the lock cannot be acquired within 10 seconds, cancel throws general_resource_locked (409). The schedule is left unchanged and the client can retry. A timeout is not treated as losing to publication, so a later failed publish cannot revive a schedule after a rejected cancel.

Fixes #13249

Test Plan

  • php vendor/bin/phpunit tests/unit/Schedule/ExecutionTest.php tests/unit/Platform/Workers/FunctionsTest.php --no-configuration --bootstrap app/init.php (10 tests, 79 assertions)
  • php vendor/bin/phpstan analyse on the changed files
  • php vendor/bin/pint --test on the changed files

Library tests in tests/unit/Schedule/ExecutionTest.php wait on the lock with fibers and assert both the cancellation result and persistence:

  • Cancel before publish, so the worker does not enqueue and cancel() returns true.
  • Cancel that waits through a failed publish: cancel() returns true and the restored row is deleted.
  • Cancel that waits through a successful publish whose cleanup fails: cancel() returns false and the inactive row stays.
  • Cancel of an already claimed schedule does not delete it.
  • Lock acquisition timeout throws general_resource_locked and leaves the active schedule in place.

The existing e2e testDeleteScheduledExecution remains the HTTP contract for a cancel that wins outright.

Related PRs and Issues

Checklist

  • Have you read the Contributing Guidelines on issues?
  • If the PR includes a change to an API's metadata (desc, label, params, etc.), does it also include updated API specs and example docs?
Open in Web Open in Cursor 

Copy link
Copy Markdown
Member Author

@hansi-codes review

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown

Security rules

No new WARNING or ERROR findings from security rules.

32 existing findings tracked in .semgrep/baseline.json
  • php.appwrite.guest-write-without-abuse-limit (17)
  • php.appwrite.permissive-write-permission (8)
  • php.appwrite.secret-compare-timing (5)
  • php.appwrite.weak-secret-env-default (2)

Posted by Checks / Rules. Re-runs update this comment in place. Rule details and baseline: .semgrep/README.md.

@hansi-codes

hansi-codes Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

🟢 Tier S · Ready to merge

The previously reported coordination, dependency-injection, and waiting-test issues are addressed, with no additional defects identified in the follow-up review.

Scheduled execution publication and cancellation now use a shared coordinator registered as an object dependency. The coordinator spans claim, queue publication, cleanup, and failed-publication restoration with a per-schedule lock, and maps cancellation lock contention to a retryable resource-locked error. New library tests exercise waiting cancellation and verify both its result and persisted schedule state.

Verdict New comments Fixed Still open
✅ Approved 0 3 0
📂 Walkthrough · 6
File Change
app/init/resources.php Registers the scheduled-execution coordinator as an object dependency.
src/Appwrite/Platform/Modules/Functions/Http/Executions/Delete.php Includes inactive schedules in lookup and delegates cancellation to the coordinator.
src/Appwrite/Platform/Workers/Functions.php Delegates scheduled publication to the shared coordinator.
src/Appwrite/Schedule/Execution.php Coordinates publication and cancellation and translates cancellation lock timeouts.
tests/unit/Platform/Workers/FunctionsTest.php Updates the existing test harness for the coordinator dependency.
tests/unit/Schedule/ExecutionTest.php Adds library coverage for cancellation ordering, publication failures, and lock contention.
✅ Fixed since the last review · 3
  • Distinguish lock timeout from a committed publication · src/Appwrite/Schedule/Execution.php:68
  • The lock fake returns before cancellation actually happens · tests/unit/Platform/Workers/FunctionsTest.php:368
  • Inject an object dependency for schedule coordination · src/Appwrite/Platform/Modules/Functions/Http/Executions/Delete.php:76

Reviewed 07ebb27 · Details · Comment @hansi-codes review to re-run, or mention @hansi-codes with a question.

@hansi-codes hansi-codes Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Tier A · Looks good to merge. Summary

Comment thread src/Appwrite/Schedule/Execution.php Outdated
Comment thread tests/unit/Platform/Workers/FunctionsTest.php Outdated
Comment thread src/Appwrite/Platform/Modules/Functions/Http/Executions/Delete.php Outdated
@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

✨ Benchmark results

Comparing main (before) → cursor/fix-scheduled-execution-cancel-cbf3 (after).

Metric Before After Change
🚀 Requests/sec 176.08 138.57 🔴 -21.3%
⏱️ Latency P50 97.69 ms 125.36 ms 🔴 +28.3%
⏱️ Latency P95 227.6 ms 294.7 ms 🔴 +29.5%
Per-scenario breakdown & investigation details

Metrics below reflect the current branch (after). Δ P95 compares against the base.

Scenario P50 (ms) P95 (ms) Requests RPS Δ P95 (ms)
API total 125.36 294.7 9,120 138.57 +67.1
Account 246.1 433.35 480 8.09 +70.05
TablesDB 121.18 228.29 4,960 78.87 +45.4
Storage 115.76 249.03 2,400 39.24 +52.83
Functions 181.57 358.76 1,280 21.69 +80.98

Top API waits (after)

API request Max wait (ms)
account.name.update 628.77
storage.files.create 564.31
account.prefs.update 559.16
functions.create 536.2
functions.variables.update 535.96

Hold one per-schedule lock across the functions worker's claim, queue
publication, cleanup, and failed-publish restore, and across cancellation.
A cancel succeeds only when publication was prevented. A cancel that loses
to a successful publish is rejected as in progress, and a failed publish
cannot revive a schedule after cancellation has won.

A lock acquisition timeout is a retryable resource_locked coordination
failure. It is not reported as execution_in_progress, so a later failed
publish cannot look like cancellation lost. Publication and cancellation
share the scheduleForExecutions coordinator.

Fixes #13249
@cursor
cursor Bot force-pushed the cursor/fix-scheduled-execution-cancel-cbf3 branch from adfbeee to 07ebb27 Compare October 7, 2026 13:43

Copy link
Copy Markdown
Member Author

@hansi-codes review

@hansi-codes hansi-codes Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Tier S · Looks good to merge. Summary

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Scheduled execution cancellation can race with queue publication

1 participant