mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-08 20:42:43 +00:00
fix(sharing): re-read a Guardian's grant under the record lock
Renouncing a Guardian grant read the grant before taking the managed record's lock. When the Guardian's own account deletion held the lock first and ended the grant, the count under the lock then refused with "add another Guardian" for a grant that was no longer active. The grant is now re-read under the lock, so the renunciation answers that it has already ended (or is gone) instead. The race test accepts the already-ended answer beside 200 and 404, since which side wins depends only on timing.
This commit is contained in:
1 parent
c73c6a7b1e
commit
44f3181998
2 files changed
+38
-3
No files matched your search
@@ -983,7 +983,20 @@ async function endGrant(
|
||||
(current.expiresAt === null || current.expiresAt > new Date());
|
||||
|
||||
if (!actorOwnsGrant || !isActiveManagedGuardian) return end();
|
||||
return reduceManagedProfileGuardian(db, current.grantorId, end);
|
||||
// `current` was read before the record lock. A concurrent change that holds
|
||||
// the lock first (the Guardian's account deletion ends this grant) commits
|
||||
// in between, and counting under the lock would then refuse with "add
|
||||
// another Guardian" for a grant that is no longer active, instead of saying
|
||||
// it has already ended. So the grant is re-read under the lock; only a grant
|
||||
// still live there is a Guardian whose removal the count has to allow.
|
||||
return withManagedProfileLock(db, current.grantorId, async () => {
|
||||
const live = await db.accountGrant.findFirst({
|
||||
where: { id: grantId, revokedAt: null },
|
||||
select: { id: true },
|
||||
});
|
||||
if (!live) return end();
|
||||
return reduceManagedProfileGuardian(db, current.grantorId, end);
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -771,7 +771,18 @@ describe("managed profile lifecycle (real Postgres)", () => {
|
||||
),
|
||||
]);
|
||||
|
||||
expect([200, 404]).toContain(renounced.status);
|
||||
// Either order is a correct serialization: the renunciation lands first
|
||||
// (200), or the deletion ends the grant first and the renunciation finds
|
||||
// it gone (404) or already ended (409 `sharing.renounce.already_ended`).
|
||||
// Which one wins depends only on timing, and the write ceiling charged
|
||||
// ahead of the renunciation shifts that timing.
|
||||
expect([200, 404, 409]).toContain(renounced.status);
|
||||
if (renounced.status === 409) {
|
||||
const body = (await renounced.clone().json()) as {
|
||||
meta?: { errorCode?: string };
|
||||
};
|
||||
expect(body.meta?.errorCode).toBe("sharing.renounce.already_ended");
|
||||
}
|
||||
expect(deleted.status).toBe(200);
|
||||
await expect(
|
||||
Promise.all([
|
||||
@@ -818,7 +829,18 @@ describe("managed profile lifecycle (real Postgres)", () => {
|
||||
),
|
||||
]);
|
||||
|
||||
expect([200, 404]).toContain(renounced.status);
|
||||
// Either order is a correct serialization: the renunciation lands first
|
||||
// (200), or the deletion ends the grant first and the renunciation finds
|
||||
// it gone (404) or already ended (409 `sharing.renounce.already_ended`).
|
||||
// Which one wins depends only on timing, and the write ceiling charged
|
||||
// ahead of the renunciation shifts that timing.
|
||||
expect([200, 404, 409]).toContain(renounced.status);
|
||||
if (renounced.status === 409) {
|
||||
const body = (await renounced.clone().json()) as {
|
||||
meta?: { errorCode?: string };
|
||||
};
|
||||
expect(body.meta?.errorCode).toBe("sharing.renounce.already_ended");
|
||||
}
|
||||
expect(deleted.status).toBe(200);
|
||||
await expect(
|
||||
Promise.all([
|
||||
|
||||
Reference in new issue
Block a user