diff --git a/docs/superpowers/plans/2026-05-30-completed-game-portfolio.md b/docs/superpowers/plans/2026-05-30-completed-game-portfolio.md index ec14b69..e0cb76c 100644 --- a/docs/superpowers/plans/2026-05-30-completed-game-portfolio.md +++ b/docs/superpowers/plans/2026-05-30-completed-game-portfolio.md @@ -78,7 +78,8 @@ - `da0a306` `fix(data): enforce completed portfolio sessions` - `a28b75d` `fix(data): align portfolio mutation lock order` - `d762ecc` `fix(data): serialize portfolio future reschedules` -- Current fix cycle: `fix(data): lock racing portfolio publications` +- `1d62f69` `fix(data): lock racing portfolio publications` +- Current fix cycle: `fix(data): serialize new-link publication races` **Files:** - Create: `tests/GmRelay.Bot.Tests/Web/PortfolioMigrationTests.cs` @@ -267,6 +268,9 @@ public async Task PublishingDraftCardWithAnyFutureLinkedSession_ShouldFailCommit [Fact] public async Task ConcurrentPublishAndFutureReschedule_ShouldNotDeadlockOrCommitInvalidPublicCard() +[Fact] +public async Task ConcurrentNewLinkPublishAndFutureReschedule_ShouldNotCommitInvalidPublicCard() + [Theory] [InlineData("portfolio_game_sessions")] [InlineData("portfolio_game_masters")] @@ -281,7 +285,7 @@ public async Task RequiredParentCascadeDelete_ShouldFailCommitForPublishedCard(s public async Task ParentCardAndGroupCascadeDeletes_ShouldCommit() ``` -The direct-delete, moved-link, invalid publication, and direct parent-cascade scenarios must expect PostgreSQL `23514` at commit. Every selected linked session must be completed with `scheduled_at < now()`: one future link among multiple selected sessions rejects publication. A final future reschedule must atomically unpublish linked public cards while preserving their first `published_at`; `past -> future -> past` in one transaction must leave the card public. Opposing-order batch reschedules must use an advisory test gate plus `pg_blocking_pids` observation with bounded timeouts, complete without card deadlock, and leave both cards private; do not rely on `pg_sleep` timing. The `READ COMMITTED` concurrency scenarios must launch bounded tasks together, cover both publish/delete lock orders, and prove there is no deadlock, write-skew, or invalid public commit. A session-delete versus future-reschedule race must use the common `sessions` then `portfolio_games` lock order, cover both first-session-lock orders through real blocking transactions, and finish with the card private and session deleted. The publish/reschedule race must finish with the future session committed and the card private. The `REPEATABLE READ` scenarios must reject triggered portfolio writes with `0A000`, including both draft-link deletion versus publication commit orders, because a stale snapshot after lock acquisition cannot safely validate the invariant. The parent-card and owning-group cascade scenarios must commit successfully. +The direct-delete, moved-link, invalid publication, and direct parent-cascade scenarios must expect PostgreSQL `23514` at commit. Every selected linked session must be completed with `scheduled_at < now()`: one future link among multiple selected sessions rejects publication. A final future reschedule must atomically unpublish linked public cards while preserving their first `published_at`; `past -> future -> past` in one transaction must leave the card public. Opposing-order batch reschedules must use an advisory test gate plus `pg_blocking_pids` observation with bounded timeouts, complete without card deadlock, and leave both cards private; do not rely on `pg_sleep` timing. The `READ COMMITTED` concurrency scenarios must launch bounded tasks together, cover both publish/delete lock orders, and prove there is no deadlock, write-skew, or invalid public commit. A session-delete versus future-reschedule race must use the common `sessions` then `portfolio_games` lock order, cover both first-session-lock orders through real blocking transactions, and finish with the card private and session deleted. The publish/reschedule races must finish with the future session committed and the card private, including a new-link draft publication forced behind the post-row-lock advisory gate. The `REPEATABLE READ` scenarios must reject triggered portfolio writes with `0A000`, including both draft-link deletion versus publication commit orders, because a stale snapshot after lock acquisition cannot safely validate the invariant. The parent-card and owning-group cascade scenarios must commit successfully. - [ ] **Step 3: Run the Task 1 tests to verify RED** @@ -435,6 +439,8 @@ BEGIN ORDER BY pg.id FOR UPDATE OF pg; + PERFORM pg_advisory_xact_lock(20260530, 108); + UPDATE portfolio_games pg SET is_public = false, updated_at = now() @@ -508,7 +514,7 @@ CREATE INDEX ix_portfolio_game_reviews_pending WHERE moderation_status = 'Pending'; ``` -The deferred constraint triggers retain the link-table `ON DELETE CASCADE` behavior. At transaction commit validators acquire the same transaction-level advisory lock and reject a surviving published card when either required link set is empty or any linked session has `scheduled_at >= now()`. The intentionally global lock is appropriate for low-volume portfolio publication writes: under the application default `READ COMMITTED` isolation level it serializes validation, prevents write-skew across distinct child links, and gives multi-card transactions one lock order. PostgreSQL retains stale snapshots under `REPEATABLE READ` and `SERIALIZABLE`, so the guard rejects every triggered portfolio write at those isolation levels with `0A000`. The deferred future-reschedule trigger re-reads the final session row, skips intermediate future values that end in the past, and for a final future value locks all cards linked to any final-future session in `portfolio_games.id` order before one guarded public-card unpublish update. The lock phase deliberately includes committed drafts so a concurrent draft-to-public publication cannot pass validation against the pre-reschedule session snapshot and commit afterward. This separate global row-lock pass avoids opposing batch order without adding the validator advisory lock before card locks. At `READ COMMITTED`, draft edits, explicit unpublishing, future reschedules, and card or club cascade deletion remain valid. Normal session-deletion handlers use the same `sessions` then `portfolio_games` lock order: explicitly lock the target session row, unpublish linked cards, then delete the session. +The deferred constraint triggers retain the link-table `ON DELETE CASCADE` behavior. At transaction commit validators acquire the same transaction-level advisory lock and reject a surviving published card when either required link set is empty or any linked session has `scheduled_at >= now()`. The intentionally global lock is appropriate for low-volume portfolio publication writes: under the application default `READ COMMITTED` isolation level it serializes validation, prevents write-skew across distinct child links, and gives multi-card transactions one lock order. PostgreSQL retains stale snapshots under `REPEATABLE READ` and `SERIALIZABLE`, so the guard rejects every triggered portfolio write at those isolation levels with `0A000`. The deferred future-reschedule trigger re-reads the final session row, skips intermediate future values that end in the past, and for a final future value locks all cards linked to any final-future session in `portfolio_games.id` order. It then acquires the publication advisory lock and runs one guarded public-card unpublish update with a fresh `READ COMMITTED` statement snapshot. The row-lock phase deliberately includes committed drafts so a concurrent draft-to-public publication cannot pass validation against the pre-reschedule session snapshot and commit afterward; the post-row-lock advisory phase also serializes a previously invisible concurrent link-add publication without moving the advisory lock above card locks. At `READ COMMITTED`, draft edits, explicit unpublishing, future reschedules, and card or club cascade deletion remain valid. Normal session-deletion handlers use the same `sessions` then `portfolio_games` lock order: explicitly lock the target session row, unpublish linked cards, then delete the session. - [ ] **Step 5: Lock sessions before explicitly unpublishing linked cards in both session-deletion handlers** @@ -554,7 +560,7 @@ Run: dotnet test tests/GmRelay.Bot.Tests/GmRelay.Bot.Tests.csproj --filter "FullyQualifiedName~PortfolioMigrationTests|FullyQualifiedName~PortfolioMigrationPostgresTests|FullyQualifiedName~PortfolioSessionDeletionSourceTests|FullyQualifiedName~PortfolioSchemaGateSourceTests" ``` -Expected: PASS, including PostgreSQL 17 migration application, rejected direct required-link deletes, rejected moved links and session/player cascades, rejected publication with any future linked session, automatic unpublish with preserved `published_at` after final future reschedule, preserved public state after `past -> future -> past`, opposing-order batch reschedules without card deadlock, bounded `READ COMMITTED` publish/delete in both commit orders, publish/reschedule races, session-delete/reschedule serialization in both first-lock orders, and distinct-link deletion without deadlock, write-skew, or invalid public commit, rejected `REPEATABLE READ` triggered writes including both draft-delete versus publish commit orders, successful parent-card and owning-group cascades, Discord identity scoping, and Compose/Aspire HTTP health gating with a non-proxied bot endpoint and matching `gmrelaydb` resource name. +Expected: PASS, including PostgreSQL 17 migration application, rejected direct required-link deletes, rejected moved links and session/player cascades, rejected publication with any future linked session, automatic unpublish with preserved `published_at` after final future reschedule, preserved public state after `past -> future -> past`, opposing-order batch reschedules without card deadlock, bounded `READ COMMITTED` publish/delete in both commit orders, existing-link and new-link publish/reschedule races, session-delete/reschedule serialization in both first-lock orders, and distinct-link deletion without deadlock, write-skew, or invalid public commit, rejected `REPEATABLE READ` triggered writes including both draft-delete versus publish commit orders, successful parent-card and owning-group cascades, Discord identity scoping, and Compose/Aspire HTTP health gating with a non-proxied bot endpoint and matching `gmrelaydb` resource name. - [ ] **Step 7: Commit** diff --git a/docs/superpowers/specs/2026-05-30-completed-game-portfolio-design.md b/docs/superpowers/specs/2026-05-30-completed-game-portfolio-design.md index 15372a9..f964ff8 100644 --- a/docs/superpowers/specs/2026-05-30-completed-game-portfolio-design.md +++ b/docs/superpowers/specs/2026-05-30-completed-game-portfolio-design.md @@ -81,7 +81,7 @@ Application validation additionally requires at least one linked session, every Deferred database constraint triggers validate the same invariant at transaction commit after a card transitions to public, a session link is inserted, deleted, moved, or repointed, or a required master link is deleted or moved. They raise a check-violation error if a published card would commit without both required link sets or with any linked session where `scheduled_at >= now()`. Before checking state, each validator acquires the same transaction-level PostgreSQL advisory lock, `pg_advisory_xact_lock(20260530, 108)`. Portfolio publication writes are low volume, so this intentionally global lock serializes invariant validation with one lock order, prevents write-skew under the application default `READ COMMITTED` isolation level, and avoids multi-card deadlocks. PostgreSQL keeps a stale snapshot after waiting under `REPEATABLE READ` or `SERIALIZABLE`, so the guard rejects every triggered portfolio write at those levels; callers must use `READ COMMITTED` for portfolio mutations. -A deferred `sessions.scheduled_at` trigger atomically unpublishes linked public cards when a completed session is finally rescheduled into the future, preserving the first `published_at`. Because deferred row triggers retain their event-time `NEW`, the trigger re-reads the final `sessions.scheduled_at` before acting. For a final future value it takes row locks for all cards linked to any final-future session in `portfolio_games.id` order, including committed drafts, then unpublishes the matching public cards in one guarded update. Including drafts prevents a concurrent draft-to-public publication from validating against the pre-reschedule session snapshot and committing afterward. The low-volume global pass gives batch reschedules one card-lock order without taking the publication validator advisory lock before card locks. Session mutation paths use `sessions` before linked `portfolio_games`; normal session-deletion handlers explicitly lock the target session row, unpublish linked cards in the same transaction, and only then delete the session. The link foreign keys retain `ON DELETE CASCADE`; when the card itself or its owning club is deleted at `READ COMMITTED`, deferred validation sees no surviving published card and remains harmless. +A deferred `sessions.scheduled_at` trigger atomically unpublishes linked public cards when a completed session is finally rescheduled into the future, preserving the first `published_at`. Because deferred row triggers retain their event-time `NEW`, the trigger re-reads the final `sessions.scheduled_at` before acting. For a final future value it takes row locks for all cards linked to any final-future session in `portfolio_games.id` order, including committed drafts. It then acquires the publication advisory lock and unpublishes matching public cards in a guarded update with a fresh `READ COMMITTED` statement snapshot. Including drafts prevents a concurrent draft-to-public publication from validating against the pre-reschedule session snapshot and committing afterward. Taking the shared advisory lock after card rows, but before the guarded update, also serializes a previously invisible concurrent link-add publication without reintroducing the card/advisory lock inversion. Session mutation paths use `sessions` before linked `portfolio_games`; normal session-deletion handlers explicitly lock the target session row, unpublish linked cards in the same transaction, and only then delete the session. The link foreign keys retain `ON DELETE CASCADE`; when the card itself or its owning club is deleted at `READ COMMITTED`, deferred validation sees no surviving published card and remains harmless. ### `portfolio_game_sessions` @@ -356,7 +356,7 @@ Follow TDD for production changes. ### Schema And Contracts - Migration source-contract tests assert the four new tables, format constraint, publication guard, case-insensitive slug uniqueness, group and GM-profile indexes, card-oriented pending-review index, completed-session validator, deferred future-reschedule unpublish trigger, session-first deletion locks, and the AppHost HTTP health gate. -- PostgreSQL integration tests apply migrations V001 through V029 to `postgres:17-alpine` and cover direct invalid link removal, moved links, direct session/player cascades, explicit session-lock then unpublish then session deletion, delete/reschedule lock ordering in both first-lock orders, rejection of publication when any linked session is future, automatic unpublish with preserved `published_at` after future reschedule, `past -> future -> past` final-state handling, opposing-order batch future reschedules without card deadlock using an observed advisory test gate instead of timing sleeps, publish/reschedule races, both bounded publish/delete commit orders, concurrent removal of distinct required links without write-skew or deadlock under `READ COMMITTED`, rejection of equivalent `REPEATABLE READ` writes including both draft-delete versus publish commit orders, and parent/card cascade deletion. +- PostgreSQL integration tests apply migrations V001 through V029 to `postgres:17-alpine` and cover direct invalid link removal, moved links, direct session/player cascades, explicit session-lock then unpublish then session deletion, delete/reschedule lock ordering in both first-lock orders, rejection of publication when any linked session is future, automatic unpublish with preserved `published_at` after future reschedule, `past -> future -> past` final-state handling, opposing-order batch future reschedules without card deadlock using an observed advisory test gate instead of timing sleeps, existing-link and new-link draft publication/reschedule races, both bounded publish/delete commit orders, concurrent removal of distinct required links without write-skew or deadlock under `READ COMMITTED`, rejection of equivalent `REPEATABLE READ` writes including both draft-delete versus publish commit orders, and parent/card cascade deletion. - Public DTO reflection/source tests assert that private identifiers and physical storage paths are absent. - Existing showcase tests continue to assert the future-session catalog boundary. diff --git a/src/GmRelay.Bot/Migrations/V029__add_completed_game_portfolios_and_reviews.sql b/src/GmRelay.Bot/Migrations/V029__add_completed_game_portfolios_and_reviews.sql index 757c1a6..4eab31f 100644 --- a/src/GmRelay.Bot/Migrations/V029__add_completed_game_portfolios_and_reviews.sql +++ b/src/GmRelay.Bot/Migrations/V029__add_completed_game_portfolios_and_reviews.sql @@ -140,6 +140,8 @@ BEGIN ORDER BY pg.id FOR UPDATE OF pg; + PERFORM pg_advisory_xact_lock(20260530, 108); + UPDATE portfolio_games pg SET is_public = false, updated_at = now() diff --git a/tests/GmRelay.Bot.Tests/Web/PortfolioMigrationPostgresTests.cs b/tests/GmRelay.Bot.Tests/Web/PortfolioMigrationPostgresTests.cs index 37b442b..6286fab 100644 --- a/tests/GmRelay.Bot.Tests/Web/PortfolioMigrationPostgresTests.cs +++ b/tests/GmRelay.Bot.Tests/Web/PortfolioMigrationPostgresTests.cs @@ -542,6 +542,95 @@ public sealed class PortfolioMigrationPostgresTests(PortfolioMigrationPostgresFi parameters: new NpgsqlParameter("sessionId", seed.SessionIds[0]))); } + [Fact] + public async Task ConcurrentNewLinkPublishAndFutureReschedule_ShouldNotCommitInvalidPublicCard() + { + var database = await fixture.CreateMigratedDatabaseAsync(); + await using var seedConnection = await database.OpenConnectionAsync(); + var seed = await SeedCardAsync(seedConnection, isPublic: false); + var rescheduledSessionId = Guid.NewGuid(); + await ExecuteNonQueryAsync( + seedConnection, + """ + INSERT INTO sessions (id, group_id, title, join_link, scheduled_at) + VALUES (@sessionId, @groupId, 'Completed Session', 'https://example.test/session', now() - interval '1 day'); + """, + parameters: + [ + new NpgsqlParameter("sessionId", rescheduledSessionId), + new NpgsqlParameter("groupId", seed.GroupId) + ]); + + await using var rescheduleConnection = await database.OpenConnectionAsync(); + await using var publishConnection = await database.OpenConnectionAsync(); + await using var gateConnection = await database.OpenConnectionAsync(); + await using var observerConnection = await database.OpenConnectionAsync(); + await using var rescheduleTransaction = await rescheduleConnection.BeginTransactionAsync(); + await using var publishTransaction = await publishConnection.BeginTransactionAsync(); + await using var gateTransaction = await gateConnection.BeginTransactionAsync(); + var reschedulePid = await GetBackendPidAsync(rescheduleConnection, rescheduleTransaction); + var publishPid = await GetBackendPidAsync(publishConnection, publishTransaction); + var gatePid = await GetBackendPidAsync(gateConnection, gateTransaction); + await AcquirePortfolioValidationLockAsync(gateConnection, gateTransaction); + + Assert.Equal(1, await RescheduleSessionAsync( + rescheduleConnection, + rescheduleTransaction, + rescheduledSessionId)); + var forceRescheduleTriggerTask = ExecuteNonQueryAsync( + rescheduleConnection, + "SET CONSTRAINTS trg_sessions_unpublish_public_portfolio_games_for_future_reschedule IMMEDIATE", + rescheduleTransaction); + await WaitUntilBlockedByAsync(observerConnection, reschedulePid, gatePid); + + await ExecuteNonQueryAsync( + publishConnection, + """ + INSERT INTO portfolio_game_sessions (portfolio_game_id, session_id) + VALUES (@portfolioGameId, @sessionId); + + UPDATE portfolio_games + SET is_public = true, + published_at = COALESCE(published_at, now()), + updated_at = now() + WHERE id = @portfolioGameId; + """, + publishTransaction, + new NpgsqlParameter("portfolioGameId", seed.PortfolioGameId), + new NpgsqlParameter("sessionId", rescheduledSessionId)); + var publishCommitTask = CommitAndCaptureSqlStateAsync(publishTransaction); + await WaitUntilBlockedByAsync(observerConnection, publishPid, gatePid); + + await gateTransaction.CommitAsync().WaitAsync(CommandTimeout); + await forceRescheduleTriggerTask.WaitAsync(CommandTimeout); + await rescheduleTransaction.CommitAsync().WaitAsync(CommandTimeout); + + Assert.Equal(PostgresErrorCodes.CheckViolation, await publishCommitTask.WaitAsync(CommandTimeout)); + + await using var verificationConnection = await database.OpenConnectionAsync(); + Assert.False(await ExecuteScalarAsync( + verificationConnection, + "SELECT is_public FROM portfolio_games WHERE id = @portfolioGameId", + parameters: new NpgsqlParameter("portfolioGameId", seed.PortfolioGameId))); + Assert.True(await ExecuteScalarAsync( + verificationConnection, + "SELECT scheduled_at >= now() FROM sessions WHERE id = @sessionId", + parameters: new NpgsqlParameter("sessionId", rescheduledSessionId))); + Assert.Equal(0, await ExecuteScalarAsync( + verificationConnection, + """ + SELECT COUNT(*) + FROM portfolio_game_sessions + WHERE portfolio_game_id = @portfolioGameId + AND session_id = @sessionId + """, + parameters: + [ + new NpgsqlParameter("portfolioGameId", seed.PortfolioGameId), + new NpgsqlParameter("sessionId", rescheduledSessionId) + ])); + } + [Theory] [InlineData(true)] [InlineData(false)] diff --git a/tests/GmRelay.Bot.Tests/Web/PortfolioMigrationTests.cs b/tests/GmRelay.Bot.Tests/Web/PortfolioMigrationTests.cs index 165bf09..d1f9ce5 100644 --- a/tests/GmRelay.Bot.Tests/Web/PortfolioMigrationTests.cs +++ b/tests/GmRelay.Bot.Tests/Web/PortfolioMigrationTests.cs @@ -50,13 +50,11 @@ public sealed class PortfolioMigrationTests Assert.Contains("IF final_scheduled_at >= now() THEN", normalizedMigration, StringComparison.Ordinal); Assert.Contains("PERFORM pg.id FROM portfolio_games pg WHERE EXISTS", normalizedMigration, StringComparison.Ordinal); Assert.Contains("ORDER BY pg.id FOR UPDATE OF pg;", normalizedMigration, StringComparison.Ordinal); + Assert.Contains("ORDER BY pg.id FOR UPDATE OF pg; PERFORM pg_advisory_xact_lock(20260530, 108); UPDATE portfolio_games pg SET is_public = false", normalizedMigration, StringComparison.Ordinal); Assert.Contains("UPDATE portfolio_games pg SET is_public = false, updated_at = now() WHERE pg.is_public = true AND EXISTS (SELECT 1 FROM portfolio_game_sessions pgs JOIN sessions s ON s.id = pgs.session_id WHERE pgs.portfolio_game_id = pg.id AND s.scheduled_at >= now());", normalizedMigration, StringComparison.Ordinal); Assert.Contains("CREATE CONSTRAINT TRIGGER trg_sessions_unpublish_public_portfolio_games_for_future_reschedule AFTER UPDATE OF scheduled_at ON sessions DEFERRABLE INITIALLY DEFERRED FOR EACH ROW EXECUTE FUNCTION unpublish_public_portfolio_games_for_future_session();", normalizedMigration, StringComparison.Ordinal); Assert.DoesNotContain("unpublish_portfolio_game_without_required_links", normalizedMigration, StringComparison.Ordinal); - Assert.DoesNotContain( - "pg_advisory_xact_lock", - normalizedMigration[unpublishFunctionStart..unpublishFunctionEnd], - StringComparison.Ordinal); + Assert.Contains("pg_advisory_xact_lock", normalizedMigration[unpublishFunctionStart..unpublishFunctionEnd], StringComparison.Ordinal); Assert.DoesNotContain("published_at = NULL", normalizedMigration, StringComparison.OrdinalIgnoreCase); Assert.Contains("publication_consent_at TIMESTAMPTZ NOT NULL,", normalizedMigration, StringComparison.Ordinal); }