feat: lock on email, not session ID

[joaquimds]

No description provided.

[Copilot]

Pull request overview

This PR updates the join/webhook concurrency strategy to lock per person (email) rather than per request/session, to reduce race conditions across /join and concurrent webhook deliveries for the same member.

Changes:

• Replaced session-token locking with a generalized JoinService::acquireLock($key) / releaseLock() keyed primarily by email (with sessionToken fallback).
• Added per-email locking to Stripe webhook handling to serialize CRM mutations for a single customer.
• Updated the session lock concurrency test to use an email-shaped lock key and safer shell escaping.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
packages/join-block/src/Services/JoinService.php	Introduces generalized per-key locking and updates /join to lock by email (fallback to sessionToken).
packages/join-block/src/Services/StripeService.php	Acquires/releases the per-email lock around webhook flows that mutate CRM state.
packages/join-block/src/Services/ActionNetworkService.php	Adds tag-existence checks by fetching a person’s tags prior to add/remove operations.
packages/join-block/tests/SessionLockTest.php	Updates the locking test to validate serialization by an email-shaped key and escapes shell args.
packages/join-block/tests/SessionLockTestProcess.php	Updates the child process to use acquireLock()/releaseLock() and key-based logging.

Comments suppressed due to low confidence (2)

packages/join-block/src/Services/ActionNetworkService.php:186

• Inside getPersonTagNames(), the code issues an additional HTTP request per tagging to resolve the tag resource (GET $tagHref). This is an N+1 request pattern and can significantly slow down webhook/join requests and increase the chance of hitting Action Network rate limits. Consider using data already present in the taggings response (if available), batching, caching tag href->name lookups, or restructuring to avoid fetching every tag when callers only need to test membership for a specific tag.

            foreach ($taggings as $tagging) {
                $tagHref = $tagging["_links"]["osdi:tag"]["href"] ?? null;
                if (!$tagHref) {
                    continue;
                }
                $tagResponse = $client->request("GET", $tagHref, ["headers" => $headers]);
                $tagBody = json_decode($tagResponse->getBody()->getContents(), true);
                if (!empty($tagBody["name"])) {
                    $tagNames[] = $tagBody["name"];
                }

packages/join-block/src/Services/ActionNetworkService.php:208

• addTag()/removeTag() now call getPersonTagNames() which fetches all tags for the person before adding/removing a single tag. This makes each tag mutation O(total tags) network calls and work. A more scalable approach is a personHasTag($email, $tag) helper that short-circuits as soon as the target tag is found (and avoids resolving unrelated tags).

        $tagNames = self::getPersonTagNames($email);
        if ($tagNames === null) {
            $joinBlockLog->warning("Skipping Action Network addTag('$tag') for $email: person does not exist");
            return;
        }
        if (in_array($tag, $tagNames, true)) {
            $joinBlockLog->info("Skipping Action Network addTag('$tag') for $email: tag already applied");
            return;
        }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (3)

packages/join-block/src/Services/ActionNetworkService.php:178

• In getPersonTagNames(), the initial self::getPerson($email) call happens outside the try/catch. If Action Network returns a non-2xx or there’s a network error, Guzzle will throw and we’ll never hit the intended fallback (TAG_ENUMERATION_ABORTED) — addTag/removeTag will fail instead of proceeding with the no-op POST. Consider wrapping getPerson() in the same try/catch (or catching RequestException) and returning TAG_ENUMERATION_ABORTED on failure.

        $person = self::getPerson($email);
        if ($person === null) {
            return null;
        }

packages/join-block/src/Services/ActionNetworkService.php:219

• getPersonTagNames() can issue a very large number of HTTP requests: up to MAX_TAGGING_PAGES taggings-page requests, plus an additional GET per tagging to fetch the tag resource just to read its name. This is potentially more expensive than the prior behavior (single POST no-op) and will also extend the time we hold the per-email JoinService lock. Consider avoiding per-tag fetches (e.g., use tag data already embedded in taggings if available, request an expanded/embedded tag representation, or skip enumeration and rely on Action Network’s no-op semantics).

                $response = $client->request("GET", $nextHref, $requestOptions);
                $body = json_decode($response->getBody()->getContents(), true);
                $taggings = $body["_embedded"]["osdi:taggings"] ?? [];
                foreach ($taggings as $tagging) {
                    $tagHref = $tagging["_links"]["osdi:tag"]["href"] ?? null;
                    if (!$tagHref) {
                        continue;
                    }
                    $tagResponse = $client->request("GET", $tagHref, $requestOptions);
                    $tagBody = json_decode($tagResponse->getBody()->getContents(), true);
                    if (!empty($tagBody["name"])) {
                        $tagNames[] = $tagBody["name"];
                    }
                }

packages/join-block/src/Services/ActionNetworkService.php:158

• getPerson() doesn’t set any timeout/connect_timeout options, even though HTTP_TIMEOUT_SECONDS is introduced and used elsewhere. A slow/hung Action Network request here can block the join/webhook flow (and any per-email lock). Consider applying the same timeout/connect_timeout options used in getPersonTagNames() to this request as well.

        $response = $client->request(
            "GET",
            "https://actionnetwork.org/api/v2/people/",
            [
                "headers" => [
                    "OSDI-API-Token" => Settings::get("ACTION_NETWORK_API_KEY")
                ],
                "query" => $query
            ]
        );