Repository navigation
feat: serve events/subscribe and events/unsubscribe - #135
Conversation
🔵 Tier A · Mergeable after minor fixes
Adds HTTP-only MCP Events subscribe and unsubscribe methods backed by managed Appwrite webhooks, with encrypted subscription envelopes, callback verification, authorization, TTL handling, cleanup, telemetry, and HTTP ingress wiring. The repository also gains subscription lifecycle documentation and end-to-end coverage for successful flows, failures, cleanup, and key rotation. Latest changes: The newest commit clarifies that a webhook with a retired-key-derived signing secret is acknowledged as expired when its lease has elapsed, because ingress checks expiry before signature verification.
Note Not approving while a bug finding is open: A concurrent refresh can still be deleted after the re-read Fix with agent prompt### Issue 1
src/mcp_server_appwrite/events/subscriptions.py:459-460
**A concurrent refresh can still be deleted after the re-read**
If another subscribe is cleaning an expired webhook while its owner refreshes it, a refresh that lands after this `GET` but before `DELETE` is still deleted. The refresh can return success even though its webhook is gone, so events are dropped until a later scheduled refresh.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.📂 Walkthrough · 23
⏳ Still open from earlier reviews · 1
Reviewed the commits since |
![hansi-codes[bot]](https://avatars.githubusercontent.com/in/5046327?s=60&v=4){kind=small}
5dc7a55 to
f176565
Compare
7c03f56 to
326d338
Compare
f176565 to
0923baf
Compare
2a428f7 to
6d46b33
Compare
9169169 to
8be3201
Compare
| try: | ||
| webhooks.delete(webhook.id) |
There was a problem hiding this comment.
A concurrent refresh can still be deleted after the re-read
If another subscribe is cleaning an expired webhook while its owner refreshes it, a refresh that lands after this GET but before DELETE is still deleted. The refresh can return success even though its webhook is gone, so events are dropped until a later scheduled refresh.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/mcp_server_appwrite/events/subscriptions.py
Line: 459-460
Comment:
**A concurrent refresh can still be deleted after the re-read**
If another subscribe is cleaning an expired webhook while its owner refreshes it, a refresh that lands after this `GET` but before `DELETE` is still deleted. The refresh can return success even though its webhook is gone, so events are dropped until a later scheduled refresh.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟡 Minor · concurrency · Reply if this doesn't apply.
There was a problem hiding this comment.
This is the residual window already documented in _clean and under "Expired cleanup" in Decisions, and it cannot be closed from this side: Appwrite has no conditional delete (no If-Match / $updatedAt precondition on DELETE /webhooks/{id}), so any read-then-delete leaves a gap between the read and the delete. The mcp-event-subscriptions Appwrite profile lists the same constraint ("use conditional deletion or equivalent coordination").
What bounds it today: only webhooks expired for more than 10 minutes are candidates, so the subscription's own client already let it lapse well past refreshBefore; each candidate is re-read right before the delete and kept if its name or $updatedAt changed; and the client's next refresh recreates the webhook (create path), so the effect is a gap in deliveries for a subscription that was already lapsed, never a permanent loss. Closing the window fully needs a conditional delete in Appwrite; I would rather track that upstream than add a second best-effort read that only narrows it.
There was a problem hiding this comment.
The residual race is still possible: cleanup can read the old webhook, the owner’s refresh can update and successfully return, and cleanup can then delete that same ID. The re-read and subsequent unconditional delete are separate calls (cleanup); a refresh writes the webhook before returning its new refreshBefore (refresh flow, write). The grace period means the old lease had expired, but it does not prevent an in-flight refresh. That refresh can be lost until the next scheduled refresh, potentially nearly 24 hours later (TTL cap).
The lack of a conditional-delete API explains why this is hard to close here, but it doesn’t rule out the failure. Keeping the finding open; it may require upstream support for an atomic delete or equivalent coordination.
8be3201 to
41c6731
Compare
Each subscription is one managed Appwrite webhook in the subscriber's project, written with their own OAuth token, so the hosted server stays stateless. Subscribe validates, authorizes with a resource read, cleans up the caller's expired webhooks, verifies the callback and upserts the webhook; unsubscribe deletes it by the principal-bound id.
An Appwrite REST emulator joins the e2e harness, and the webhooks worker now fires the webhooks the server actually stored, closing the loop from subscribe to signed delivery to unsubscribe. The unit tests that only existed because nothing called the catalog, whsec_ validation, the handshake or subscription ids over HTTP are gone.
Covers the TTL policy, the webhook label used for cleanup, Free-plan limits, required OAuth scopes and secret rotation, and keeps the events flag framed as a testing flag.
events/unsubscribe for a project that does not exist reached the webhook
error fallback as -32603. A 404 project_not_found now maps to -32012 like
subscribe, so project ids cannot be probed; a missing webhook stays an
idempotent {}.
Expired-webhook cleanup decided from the earlier listing and deleted
unconditionally, so a concurrent refresh of that subscription could have its
now-live webhook deleted. Appwrite has no conditional delete, so cleanup now
only considers webhooks expired for over 10 minutes and re-reads each one
right before deleting it, skipping it when its name or $updatedAt changed.
The e2e flows add the Appwrite emulator's interleaving hook and use it for a
refresh landing mid-cleanup and for two replicas refreshing one subscription
across a key rotation, and check that the server refuses exactly what the
published input schemas reject.
A token that could list webhooks but not write them passed the listing and got the callback a signed verification POST before the write failed. The token's scope claim is now checked first: without project:webhooks.write, project:all or all it is -32012 before Appwrite or the callback is reached. A refresh whose secret update failed deleted the webhook. The previous signing key was derived from a key still in the ring, and the ingress accepts every ring key, so the webhook keeps delivering; the error is still reported so the client retries. Rotation is documented as three deploys (add the key behind the active one, then move it first, then drop the old one), so no replica ever receives a delivery sealed with a key it does not hold.
events/subscribe seals one delivery secret and the granted lease (expiresAt, refreshBefore) into the record, bound to this server through Ingress.server, and answers -32602 for a delivery the record refuses. The subscribe flow now opens the stored envelope and checks its key id, callback, secret and lease.
e389a85 to
9339d89
Compare
41c6731 to
281373e
Compare
A failed second write left a new envelope with a signing key derived from the previous sealing key, which could be retired while that envelope was still live. Writing the signing key first bounds a stale pairing by the previous envelope's own expiry.
…y replica Concurrent refreshes during step 2 can pair a new-key envelope with an old-key signing secret. Only step-1 replicas write that secret, so counting the TTL from the end of step 2 guarantees such envelopes expire before the old key leaves the ring. The e2e now produces that pairing and delivers through both replicas.
1 participant
Stack
feat/events← #131 ← #132 ← #133 ← #134 ← this PRMerges into
feat/events;feat/events→mainlands as one feature after appwrite/appwrite#14293. Each PR's diff shows only its own layer, and every layer passes the full checklist on its own.Summary
The top layer of MCP Events (#127):
events/subscribeandevents/unsubscribe, registered byprotocol.register(server, …)only with theMCP_EVENTSflag on and the HTTP transport. The server stays stateless: each subscription is one managed Appwrite webhook in the subscriber's project, written with the subscriber's own OAuth token, and the sealed envelope lives only in itsauthPassword.events/subscriptions.py:Subscriptions.subscribe/unsubscribe, the TTLGrant, and the mapping from Appwrite errors to events codes.events/webhooks.py: what a managed webhook looks like (Labelname, fixedauthUsername, derived secret), how it is recognized, and the SDK calls (appwrite_consoleWebhooksservice).events/catalog.py: each event declares its authorization read (Access: path, scope, how errors name the resource).events/protocol.py: registers both methods, withtools/call-style telemetry and Sentry reporting.events/ingress.py: exposes its keyring, egress, dispatcher andserver(the context envelopes are bound to) so subscribe shares them.server.py/http_app.py:build_mcp_server(..., ingress=), so subscribe writes webhooks pointing at the same ingress the app mounts.telemetry.py:mcp.events.subscriptions(operation, outcome, event, reason).Subscribe flow
catalog.lookup(name)(-32011,kind: "event"),event.validate(arguments)(-32602),delivery.modemust bewebhook(-32014, absent means webhook), callback must behttpswithout credentials and passEgress.check(private, loopback, metadata →-32602),delivery.decode_secret(-32602),ttlMs/maxAgeMsshape (-32602).Principal(iss, sub, client_id).digest. A token withoutsubis-32012; no token never reaches JSON-RPC (HTTP 401). A token whosescopeclaim has none ofproject:webhooks.write,project:allorallis-32012here, before Appwrite or the callback is contacted. A token with no scope claim is left for Appwrite to refuse.delivery.secret,lease.expiresAtandlease.refreshBeforefrom the grant) sealed with feat: seal MCP event subscriptions into an envelope #132'smcp-subscription+jweadapter for this server (Ingress.server, i.e.MCP_PUBLIC_URL), the project and the subscription id.EnvelopeTooLarge→-32602;InvalidSubscription(a delivery the record refuses: a URL with a fragment, a secret whose base64 is not canonical) →-32602. Sealing happens before anything leaves the server.resolve_client(project_id):GET /functions/{id},/sites/{id},/tablesdb/{db}/tables/{table},/storage/buckets/{id}, orGET /userswithlimit(1)(least privilege that provesusers.read).project:webhooks.*scope fails before the callback gets a POST.$id= subscription id,url=Ingress.url(id),tls: true,enabled: true,events=event.patterns(arguments),authUsername=mcp-events,authPassword= envelope,secret=keyring.signing_key(id)), or on refreshPUTthenPATCH /secret. A409race falls through to the refresh path.Result:
{id, refreshBefore, cursor: null, truncated: false}plus the SDK'sresultType: "complete".events/unsubscribevalidates name and arguments, recomputes the id from the principal anddelivery.url, and deletes the webhook only if it is managed. A missing webhook still returns{}. A missing project is-32012, as on subscribe. The id includes the principal, so a caller can only reach their own subscriptions (tested with a second user).Decisions
Verification is not cached; the handshake runs on every subscribe and refresh. There is no server-side store, and
authPasswordis write-only, so the server cannot tell whether a refresh carries the same callback and secret as the live webhook. Keeping a "verified" fact in the new envelope would not help either: it would have to be trusted from the request being verified. One signed POST per refresh (hourly by default, at most every 5 minutes) is cheap, keeps the spec's MUST trivially true, and re-proves the callback wants deliveries under the secret it is about to get. The spec's per-(principal, url)cache is a SHOULD-level optimization for servers with memory.Expired cleanup reads expiry from the webhook name. The name is a label:
MCP events | <event> | until <UTC expiry> | owner <first 8 chars of the principal digest>. It is the only place the expiry can be read back without opening the envelope.$updatedAt+ max TTL was the alternative, but it would keep an expired 1 h subscription holding a Free-plan slot for up to 24 h. Cleanup deletes only webhooks whose id is a subscription id and whose name parses as a label, whose owner tag is the caller's, and whose expiry passed more than 10 minutes ago.Appwrite has no conditional delete, so a late refresh of the same subscription could race the cleanup. Two guards prevent that. The grace period keeps any subscription whose client is still refreshing out of the candidates. Each candidate is also re-read right before its delete and kept if its name or
$updatedAtchanged since the listing. The remaining window is a refresh landing between that re-read and the delete, for a subscription left expired for over 10 minutes; its next refresh recreates the webhook. A tag collision between users could at worst delete an expired webhook whose deliveries the ingress already drops. The separator is ASCII on purpose: Appwrite echoes the name in theX-Appwrite-Webhook-Nameheader of every delivery (the first draft used·, which the e2e caught as an invalid header).NotFound vs Forbidden. Appwrite authenticates the caller against the project before it looks the resource up, so a
404for the resource itself (function_not_found,table_not_found,database_not_found, …) means the caller is in the project and the resource is missing: NotFound withdata.kind = "resource".401/403(not a member, missing scope) is Forbidden, naming the scope to grant when Appwrite saysgeneral_unauthorized_scope.404 project_not_foundis also Forbidden, so project ids cannot be probed.Secret rotation replaces; it cannot dual-sign. The previous secret exists only in the old envelope, which Appwrite never returns, and keeping it anywhere else would put a secret outside the envelope, which #127 rules out. Deliveries already in flight were opened from the old envelope and keep signing with the old secret through their retries, which covers the spec's "in-flight deliveries verify under either" intent. New deliveries sign with the new secret only. Documented in
docs/events.md.TTL. Default 1 h, floor 5 min, cap 24 h; absent or
null→ 1 h (neverrefreshBefore: null). The envelope'slease.expiresAtis the end of the grant, andrefreshBefore(returned to the client and sealed aslease.refreshBefore) is a tenth of the grant earlier (6 min for 1 h, 30 s at the floor), sorefreshBefore≤ granted expiry ≤ suggestion (except at the floor, which the spec allows).Concurrent refreshes across a key rotation. Two replicas with different active keys can interleave a refresh: old
PUT, newPUT, newPATCH /secret, oldPATCH /secret. That leaves the new key's envelope with the old key's signing secret. The ingress (#134) verifies the signature against every key in the ring, so such a webhook keeps delivering. A separate non-rotating root for the secret would also work, but it adds a second secret to configure that could never itself be rotated.No lost subscriptions on a partial refresh. Create is one call. On refresh, if the
PATCH /secretfails after thePUTsucceeded, the webhook keeps its new envelope and previous signing key, and the error (-32603) makes the client retry. That key was derived from a key still in the ring, and the ingress accepts every ring key (#134), so deliveries continue. The secret is always re-set rather than compared, because Appwrite's currentGETandPUTno longer return it.Key rotation is staged. Sealing with a key that some replica does not hold yet would make that replica drop deliveries as
retired_key. Rotation is therefore three deploys: add the key behind the active one (k1,k2), move it first (k2,k1), then drop the old one after the 24 h cap. This is documented indocs/events.md.Plan limit
max. Cloud refuses with403 additional_resource_not_allowedwhentotal >= planMax, sodata.maxis the number of webhooks in the project after cleanup.Error mapping
dataproject_id), non-https / credentialed / fragment / private / unresolvable callback, bad or non-canonicalwhsec_, oversized envelope, badttlMs/maxAgeMs-32602-32011{"kind": "event"}-32011{"kind": "resource"}project:webhooks.read/.write; nonexistent project; subscription id taken by a renamed webhook-32012-32013{"limit": "webhooks", "max": <n>}delivery.modenotwebhook-32014-32015{"reason": challenge_failed | timeout | connection_refused | tls_error | http_4xx | http_5xx}5xxor anything unexpected-32603Only
-32603goes to Sentry (tags:mcp.method,transport, client tags,event.name,appwrite.project_id). Every outcome is counted inmcp.messages.received,mcp.jsonrpc.errorsandmcp.events.subscriptions.Tests
Harness (
tests/e2e/support.py).Cloudis a small Appwrite REST emulator the server reaches throughAPPWRITE_ENDPOINT: the console region lookupresolve_clientmakes, the five authorization reads, and/v1/webhooks(list with limit/offset, create, get, update, delete,PATCH /secret). It checks projects (404 project_not_found), membership (401 user_unauthorized) and scopes (401 general_unauthorized_scope) like Appwrite, keepsauthPasswordwrite-only and returns the secret only from create and the secret update (current Appwrite), enforces the Free plan limit (403 additional_resource_not_allowed),CustomId, the 128-char name and a 2048-charauthPassword(what #14293 must allow), and takes injected failures.Appwrite.fire(cloud, project, event, body)delivers from the webhooks the server stored, with their real URL, Basic auth (authUsername+ envelope) and HMAC secret, matching events the way Appwrite's worker does. The token stub now returns the claims the real verifier produces (iss,sub,client_id,project_id) for a few users.Cloud.before(method, path, action)runs another client's work once, right before a matching request is handled. Flows use it to interleave a concurrent replica or refresh between two of the server's own calls.Flows (
tests/e2e/test_events_subscribe.py, 9 tests, ~13 s):authPasswordis a JWE withkidk1that opens for this server, project and id, and carries the callback, the secret and alease.refreshBeforeequal to the returnedrefreshBefore) →Appwrite.fire→ ingress →standardwebhooks-verified delivery. Refresh three times: same id, one webhook,refreshBeforefor the floor (5 min), the cap (24 h) andnull(1 h), with arguments reordered on one refresh; every refresh handshakes with a fresh challenge. Rotation: handshake and next delivery verify with the new secret only. Another user's unsubscribe for the same URL touches nothing. Unsubscribe → webhook gone → Appwrite fires nothing → no delivery; unsubscribing again is{}. Subscription counters: created +1, refreshed +4, removed +1, absent +2.k1,k2) refreshes a subscription, and just before itsPATCH /secreta new replica (ringk2,k1) refreshes the same subscription in full. The stored webhook ends with ak2envelope and ak1signing key. An event fired through either replica still opens, verifies and arrives.inputSchemaaccepts each argument set the server accepts, reads its resource with the caller's token, stores the event's patterns, and delivers a fired event end to end; ids are distinct. Missing function, site, table, database or bucket →-32011 kind: resource. Non-member, missingfunctions.read(message namesproject:functions.read), nonexistent project, and a token withoutsub→-32012. Unsubscribe from a nonexistent project →-32012. No bearer → HTTP 401. None of these POST to the callback or store a webhook.-32602): every ID argument of every event with*,a*,a.b,fn1.executions, empty, leading-/_, a space, 37 chars,nulland7; deploymentstatusofbuilding,READY,*,nulland7; unknown, missing or non-object arguments. For each of these argument cases, theinputSchemafromevents/listrejects it too, soEvent.validateand the published schema agree. http, ftp, file, host-less, unparseable, credentialed, fragment, private and metadata callbacks; missing, non-string, empty, unprefixed, uppercase-prefixed, spaced, non-base64, badly padded, non-canonical, 23-byte and 65-byte secrets; an oversized envelope (a 2000-char padded callback, over the 2048 budget with the JWE sizes); badttlMs/maxAgeMs. Unknown event →-32011 kind: event;poll/push→-32014. None of them reaches Appwrite or the callback.-32015):challenge_failed(endpoint cannot verify; and a 307 that is not followed),http_4xx,http_5xx,timeout,tls_error(untrusted certificate),connection_refused. No webhook stored, no raw response in the message.-32012naming both scopes, before any handshake (unsubscribe too).webhooks.write→-32012, with no Appwrite request and no POST to the callback.503on the authorization read, the listing or the create →-32603with nothing stored.500onPATCH /secretduring a refresh →-32603, but the webhook stays and a fired event is still delivered.mcp.jsonrpc.errors{-32603}+4, subscribe-32012+2.-32013{limit: "webhooks", max: 2}with an upgrade-to-Pro / unsubscribe message; unsubscribing frees the slot.sub_zzzz…), asub_webhook renamed by a person, another user's expired subscription, our own live one, and our own that expired within the 10-minute grace period. A long-expired subscription of ours that its client refreshes after cleanup listed the project, right before cleanup re-reads it, survives with its new expiry. Each of these fails without the fix. a renamed webhook holding the subscription's own id makes subscribe fail with-32012and unsubscribe leave it alone.-32602, with no Appwrite call and no POST.tests/e2e/test_events_protocol.py: subscribe/unsubscribe are no longer expected to be-32601with the flag on; with it off, all three methods are.tests/e2e/test_events_ingress.py: docstring now explains why those flows still hand-build webhooks (expired, retired-key, foreign-event, tampered and foreign-server states subscribe never produces).Unit tests deleted because these flows now cover them:
tests/unit/test_events_catalog.py(whole file): argument validation for every ID argument, and NotFound for unknown events.tests/unit/test_events_delivery.py:SecretTest(allwhsec_cases) andVerificationTest(echo, fresh challenges, mismatch, 4xx/5xx, redirect not followed, timeout, connection refused, forbidden destination).tests/unit/test_events_envelope.py:SubscriptionIdTests(determinism, argument order, principal binding, Appwrite custom-id charset, signing key fitsText(256, 8)).tests/unit/test_events_egress.py:ValidateUrlTest(callback URL shapes).No remaining unit test covers the
EventsErrorconstructors: every code anddatashape is asserted on the wire instead.Unit tests kept, and why e2e cannot reach them:
test_events_envelope.py: the publishedmcp-subscription+jweconformance vector opened through our adapter (from feat: seal MCP event subscriptions into an envelope #132), the PHP-computed Appwrite signature vector (external truth; the harness signs with the same Python formula) and the envelope size budget (deliberately extreme inputs, the contract for #14293).test_events_egress.py: the address blocklist (most ranges are unroutable from a test machine), mixed DNS answers and rebinding (need a resolver that changes its answer), the connect-target spy, and the 16 KiB response cap (invisible on the wire).test_events_delivery.py: the production retry schedule (30 s / 2 min / 8 min, too long to wait for).test_events_protocol.py: stdio never serves events (stdio needs a live project to boot).Review changes
0923baf:
404 project_not_foundon the webhook calls now maps to-32012instead of-32603. A missing webhook stays an idempotent{}.Event.validateand published-schema parity are asserted end to end in the invalid-request and every-event flows.9169169 (Hansi re-review):
PATCH /secret: the webhook is no longer deleted.75b7f73, 281373e (rebase onto #132's JWE envelope):
events/subscribeseals onedelivery.secretand the granted lease (expiresAt,refreshBefore) into the record, bound to this server throughIngress.server.subscriptions.timestampmoved toenvelope.timestamp, which formats both the result'srefreshBeforeand the sealed lease.InvalidSubscription) is-32602; the invalid-request flow gains a fragment URL and a non-canonical secret, both refused before Appwrite or the callback are contacted. The oversized-envelope case still gets-32602 EnvelopeTooLargewith the larger JWE sizes.v1.k1.prefix; the interleaved-rotation flow checks the storedkidisk2.Callbackits one secret (feat: sign and deliver MCP events safely #133's 656ac20). The delivery unit file keepsCallbackTest(secret not inrepr) next to the retry schedule.Hansi on 41c6731 (cleanup can still delete a refresh that lands between the re-read and the delete): this is the documented residual window; Appwrite has no conditional delete, so it cannot be closed from this side. Answered inline.
Verification
Run locally on Python 3.12.8 with uv 0.11.22 lockfile (the CI versions):
uv run --group dev ruff check src tests: passuv run --group dev black --check src tests: passuv run --group dev pyright: 0 errorsuv run python -m unittest discover -s tests/unit: 286 tests OKuv run --group e2e python -m unittest discover -s tests/e2e: 25 tests OK (about 36 s; the dual-secret ingress flow went away with feat: receive Appwrite webhooks and forward MCP events #134's rebase)docker build -t appwrite-mcp:jwe .: buildsNot yet
A live end-to-end run against Cloud staging (real OAuth tokens, real Appwrite webhooks and plan limits) follows appwrite/appwrite#14293, which must let
authPasswordhold up to 2048 characters (column andText(256)validator). Until then the emulator above plays Appwrite.Related: #127