diff --git a/app/api/GitHub/EvidenceEndpoints.cs b/app/api/GitHub/EvidenceEndpoints.cs index f683ef5..e8c86e1 100644 --- a/app/api/GitHub/EvidenceEndpoints.cs +++ b/app/api/GitHub/EvidenceEndpoints.cs @@ -5,6 +5,7 @@ using Microsoft.EntityFrameworkCore; using SpecThread.Api.Data; using SpecThread.Api.Projects; +using SpecThread.Api.Requirements; namespace SpecThread.Api.GitHub; @@ -72,10 +73,7 @@ private static async Task, ValidationProblem, var here = existing.FindAll(e => e.RepositoryId == repository.RepositoryId); if (reference.Number is { } number && here.Exists(e => e.Number == number)) return AlreadyLinked($"#{number}"); if (reference.Tag is { } wanted && here.Exists(e => e.Kind == "release" && e.Tag == wanted)) return AlreadyLinked($"Release {wanted}"); - if (existing.Count >= MaxEvidence) - { - return TypedResults.Problem($"A requirement can have at most {MaxEvidence} evidence links.", statusCode: StatusCodes.Status409Conflict); - } + if (existing.Count >= MaxEvidence) return LimitReached(); var found = reference switch { @@ -126,16 +124,33 @@ private static async Task, ValidationProblem, : await ReadContentsAsync(github, repository, here.FindAll(e => e.Kind == "release"), [evidence], cancel); if (contents != GitHubFailure.None) return Failed(contents, fullName); + // GitHub was read without holding the team lock. Take it now and check again what the reads relied on. + await using var transaction = await db.Database.BeginTransactionAsync(cancel); + switch (await LockAndRecheckAsync(db, userId, requirementId, repository, cancel)) + { + case Recheck.NotFound: return TypedResults.NotFound(); + case Recheck.Archived: return ProjectEndpoints.Archived(); + case Recheck.Changed: return Changed(); + } + // Counted again under the lock, so links added at the same time cannot pass the limit together. + if (await db.RequirementEvidence.CountAsync(e => e.RequirementId == requirementId, cancel) >= MaxEvidence) return LimitReached(); + db.RequirementEvidence.Add(evidence); try { await db.SaveChangesAsync(cancel); } + catch (DbUpdateConcurrencyException) + { + // A linked release this link updates was removed meanwhile. + return Changed(); + } catch (DbUpdateException) { // Two links of the same item at once: a unique index refused this one. return AlreadyLinked(Label(found.Value)); } + await transaction.CommitAsync(cancel); return TypedResults.Created($"/requirements/{requirementId}/evidence/{evidence.Id}", ToResponse(evidence)); } @@ -145,7 +160,8 @@ private static async Task, ValidationProblem, private static async Task>, NotFound, ProblemHttpResult>> Refresh( Guid requirementId, ClaimsPrincipal user, SpecThreadDbContext db, IGitHubClient github, CancellationToken cancel) { - var scope = await FindAsync(db, user.CallerId(), requirementId, cancel); + var userId = user.CallerId(); + var scope = await FindAsync(db, userId, requirementId, cancel); if (scope is null) return TypedResults.NotFound(); if (scope.Archived) return ProjectEndpoints.Archived(); var repository = await db.ProjectRepositories.AsNoTracking().SingleOrDefaultAsync(r => r.ProjectId == scope.ProjectId, cancel); @@ -185,17 +201,39 @@ private static async Task>, NotFound, ProblemH (release, change) => before[release.Id].Sha == release.Sha && before[change.Id].Change == ChangeSha(change) && before[release.Id].Contains.GetValueOrDefault(change.Id)); if (contents != GitHubFailure.None) return Failed(contents, fullName); - await db.SaveChangesAsync(cancel); + + // GitHub was read without holding the team lock, which can take a while. Take it now and + // check again what the reads relied on. + await using var transaction = await db.Database.BeginTransactionAsync(cancel); + switch (await LockAndRecheckAsync(db, userId, requirementId, repository, cancel)) + { + case Recheck.NotFound: return TypedResults.NotFound(); + case Recheck.Archived: return ProjectEndpoints.Archived(); + case Recheck.Changed: return Changed(); + } + try + { + await db.SaveChangesAsync(cancel); + } + catch (DbUpdateConcurrencyException) + { + // A link was removed while GitHub was being read. Nothing is saved. + return Changed(); + } + await transaction.CommitAsync(cancel); return TypedResults.Ok(await ReadAsync(db, requirementId, cancel)); } private static async Task> Unlink( Guid requirementId, Guid evidenceId, ClaimsPrincipal user, SpecThreadDbContext db, CancellationToken cancel) { - var scope = await FindAsync(db, user.CallerId(), requirementId, cancel); - if (scope is null) return TypedResults.NotFound(); - if (scope.Archived) return ProjectEndpoints.Archived(); + // Serialize with archiving and membership changes in this project's team. + await using var transaction = await db.Database.BeginTransactionAsync(cancel); + var found = await RequirementEndpoints.LockForMember(db, user.CallerId(), requirementId, cancel); + if (found is null) return TypedResults.NotFound(); + if (found.Archived) return ProjectEndpoints.Archived(); var deleted = await db.RequirementEvidence.Where(e => e.Id == evidenceId && e.RequirementId == requirementId).ExecuteDeleteAsync(cancel); + await transaction.CommitAsync(cancel); return deleted == 0 ? TypedResults.NotFound() : TypedResults.NoContent(); } @@ -205,6 +243,22 @@ private static async Task> Unlin .Join(db.ProjectsFor(userId), r => r.ProjectId, p => p.Id, (r, p) => new Scope(p.Id, r.ArchivedAt != null || p.ArchivedAt != null)) .SingleOrDefaultAsync(cancel); + private enum Recheck { Ready, NotFound, Archived, Changed } + + // Call inside a transaction, after reading GitHub and before saving. Locks the project's team, + // then confirms that what the reads relied on still holds: the caller is a member, nothing is + // archived, and the same repository is still connected. + private static async Task LockAndRecheckAsync( + SpecThreadDbContext db, string userId, Guid requirementId, ProjectRepository repository, CancellationToken cancel) + { + var found = await RequirementEndpoints.LockForMember(db, userId, requirementId, cancel); + if (found is null) return Recheck.NotFound; + if (found.Archived) return Recheck.Archived; + var connected = await db.ProjectRepositories.AnyAsync(r => r.ProjectId == found.Requirement.ProjectId && + r.InstallationId == repository.InstallationId && r.RepositoryId == repository.RepositoryId, cancel); + return connected ? Recheck.Ready : Recheck.Changed; + } + // Oldest first by GitHub's own date, so the list reads as a timeline. private static async Task> ReadAsync(SpecThreadDbContext db, Guid requirementId, CancellationToken cancel) => (await db.RequirementEvidence.AsNoTracking().Where(e => e.RequirementId == requirementId) @@ -412,6 +466,12 @@ private static void ApplyChecks(RequirementEvidence evidence, GitHubResult TypedResults.Problem("Connect a GitHub repository to this project first.", statusCode: StatusCodes.Status409Conflict); + private static ProblemHttpResult LimitReached() => + TypedResults.Problem($"A requirement can have at most {MaxEvidence} evidence links.", statusCode: StatusCodes.Status409Conflict); + + private static ProblemHttpResult Changed() => TypedResults.Problem( + "The evidence changed while it was being read from GitHub. Reload and try again.", statusCode: StatusCodes.Status409Conflict); + private static ProblemHttpResult AlreadyLinked(string label) => TypedResults.Problem($"{label} is already linked to this requirement.", statusCode: StatusCodes.Status409Conflict); diff --git a/app/api/GitHub/RepositoryEndpoints.cs b/app/api/GitHub/RepositoryEndpoints.cs index 006b37b..228070b 100644 --- a/app/api/GitHub/RepositoryEndpoints.cs +++ b/app/api/GitHub/RepositoryEndpoints.cs @@ -86,6 +86,14 @@ private static async Task, ValidationProbl } if (installation != GitHubFailure.None) return Failed(installation); + // GitHub was asked without holding the team lock. Take it now and check the caller's role + // and the project's state again, so a demotion or an archive in the meantime wins. + await using var transaction = await db.Database.BeginTransactionAsync(cancel); + var locked = await ProjectEndpoints.LockProjectForCaller(db, projectId, userId, cancel); + if (locked is null) return TypedResults.NotFound(); + if (!await db.CanManageTeam(locked.TeamId, userId, cancel)) return ProjectEndpoints.ManagersOnly(); + if (locked.ArchivedAt is not null) return ProjectEndpoints.Archived(); + var repository = await db.ProjectRepositories.SingleOrDefaultAsync(r => r.ProjectId == projectId, cancel); if (repository is null) { @@ -108,6 +116,7 @@ private static async Task, ValidationProbl // Two connects at once: the other one created the row first. return TypedResults.Problem("The repository connection changed while saving. Reload and try again.", statusCode: StatusCodes.Status409Conflict); } + await transaction.CommitAsync(cancel); return TypedResults.Ok(new ProjectRepositoryEnvelope(ToResponse(repository))); } @@ -116,13 +125,16 @@ private static async Task> Disco Guid projectId, ClaimsPrincipal user, SpecThreadDbContext db, CancellationToken cancel) { var userId = user.CallerId(); - var project = await db.ProjectsFor(userId).AsNoTracking().SingleOrDefaultAsync(p => p.Id == projectId, cancel); + // Serialize with archiving and role changes in this project's team. + await using var transaction = await db.Database.BeginTransactionAsync(cancel); + var project = await ProjectEndpoints.LockProjectForCaller(db, projectId, userId, cancel); if (project is null) return TypedResults.NotFound(); if (!await db.CanManageTeam(project.TeamId, userId, cancel)) return ProjectEndpoints.ManagersOnly(); if (project.ArchivedAt is not null) return ProjectEndpoints.Archived(); // Idempotent: disconnecting a project with no repository succeeds. await db.ProjectRepositories.Where(r => r.ProjectId == projectId).ExecuteDeleteAsync(cancel); + await transaction.CommitAsync(cancel); return TypedResults.NoContent(); } diff --git a/app/api/Requirements/RequirementEndpoints.cs b/app/api/Requirements/RequirementEndpoints.cs index e3206f2..22236c0 100644 --- a/app/api/Requirements/RequirementEndpoints.cs +++ b/app/api/Requirements/RequirementEndpoints.cs @@ -162,7 +162,8 @@ private static async Task, NotFound, ProblemHttp // Like FindForMember, but locks the project's team first and then reads the requirement, so // the archived states and membership it sees cannot change before this transaction ends. - private static async Task LockForMember( + // Call inside a transaction. Every requirement-scoped write (evidence, reviews) uses it. + internal static async Task LockForMember( SpecThreadDbContext db, string userId, Guid requirementId, CancellationToken cancel) { var projectId = await db.Requirements.Where(r => r.Id == requirementId).Select(r => (Guid?)r.ProjectId).SingleOrDefaultAsync(cancel); @@ -209,7 +210,10 @@ private static ProblemHttpResult Stale() => TypedResults.Problem( r.Id, r.ProjectId, r.Title, r.Description, r.CreatedBy, r.Version, r.CreatedAt, r.UpdatedAt, r.ArchivedAt, criteria.Select(c => new CriterionResponse(c.Id, c.Text, c.Position)).ToList()); - private sealed record MemberRequirement(Requirement Requirement, bool ProjectArchived); + internal sealed record MemberRequirement(Requirement Requirement, bool ProjectArchived) + { + public bool Archived => ProjectArchived || Requirement.ArchivedAt is not null; + } private sealed record ValidInput(string Title, string Description, List Criteria); } diff --git a/app/api/Requirements/ReviewEndpoints.cs b/app/api/Requirements/ReviewEndpoints.cs index 328b85f..1a27aab 100644 --- a/app/api/Requirements/ReviewEndpoints.cs +++ b/app/api/Requirements/ReviewEndpoints.cs @@ -28,7 +28,7 @@ public static void MapReviewEndpoints(this IEndpointRouteBuilder app) private static async Task>, NotFound>> List( Guid requirementId, ClaimsPrincipal user, SpecThreadDbContext db, CancellationToken cancel) { - if (await FindAsync(db, user.CallerId(), requirementId, cancel) is null) return TypedResults.NotFound(); + if (!await IsMemberAsync(db, user.CallerId(), requirementId, cancel)) return TypedResults.NotFound(); var reviews = await db.RequirementReviews.AsNoTracking().Where(r => r.RequirementId == requirementId) .OrderByDescending(r => r.DecidedAt).ThenByDescending(r => r.Id).ToListAsync(cancel); return TypedResults.Ok(reviews.ConvertAll(ToResponse)); @@ -46,16 +46,20 @@ private static async Task, ValidationProblem, No if (errors.Any) return TypedResults.ValidationProblem(errors.ToDictionary()); var userId = user.CallerId(); - var scope = await FindAsync(db, userId, requirementId, cancel); - if (scope is null) return TypedResults.NotFound(); - if (scope.Archived) return ProjectEndpoints.Archived(); + // Serialize with edits, archiving, and membership changes in this project's team, so the + // decision is checked against what is there when it is saved. + await using var transaction = await db.Database.BeginTransactionAsync(cancel); + var found = await RequirementEndpoints.LockForMember(db, userId, requirementId, cancel); + if (found is null) return TypedResults.NotFound(); + if (found.Archived) return ProjectEndpoints.Archived(); + var requirement = found.Requirement; // A second person has to look: nobody decides on a requirement they wrote. - if (scope.CreatedBy == userId) + if (requirement.CreatedBy == userId) { return TypedResults.Problem("You created this requirement, so another project member has to review it.", statusCode: StatusCodes.Status403Forbidden); } // Nobody decides on text they have not seen. - if (scope.Version != request.Version) + if (requirement.Version != request.Version) { return TypedResults.Problem("This requirement changed since it was loaded. Reload it and review the current version.", statusCode: StatusCodes.Status409Conflict); } @@ -69,13 +73,14 @@ private static async Task, ValidationProblem, No RequirementId = requirementId, Decision = decision!, Note = note!, - RequirementVersion = scope.Version, + RequirementVersion = requirement.Version, Evidence = JsonSerializer.Serialize(evidence.ConvertAll(e => new ReviewedEvidence(e.Id, e.Kind, Label(e), e.Title, e.State)), SnapshotJson), DecidedBy = userId, DecidedAt = Clock.UtcNow(), }; db.RequirementReviews.Add(review); await db.SaveChangesAsync(cancel); + await transaction.CommitAsync(cancel); return TypedResults.Created($"/requirements/{requirementId}/reviews/{review.Id}", ToResponse(review)); } @@ -94,17 +99,14 @@ private static List ReadSnapshot(RequirementReview review) => _ => evidence.Sha is { Length: >= 7 } sha ? sha[..7] : "", }; - // Null when the requirement does not exist or the caller is not a member of its project. - private static Task FindAsync(SpecThreadDbContext db, string userId, Guid requirementId, CancellationToken cancel) => - db.Requirements.AsNoTracking().Where(r => r.Id == requirementId) - .Join(db.ProjectsFor(userId), r => r.ProjectId, p => p.Id, - (r, p) => new Scope(r.CreatedBy, r.Version, r.ArchivedAt != null || p.ArchivedAt != null)) - .SingleOrDefaultAsync(cancel); + // False when the requirement does not exist or the caller is not a member of its project. + private static Task IsMemberAsync(SpecThreadDbContext db, string userId, Guid requirementId, CancellationToken cancel) => + db.Requirements.Where(r => r.Id == requirementId) + .Join(db.ProjectsFor(userId), r => r.ProjectId, p => p.Id, (r, p) => r.Id) + .AnyAsync(cancel); private static ReviewResponse ToResponse(RequirementReview r) => new(r.Id, r.RequirementId, r.Decision, r.Note, r.RequirementVersion, ReadSnapshot(r), r.DecidedBy, r.DecidedAt); - - private sealed record Scope(string CreatedBy, int Version, bool Archived); } internal sealed record RecordReviewRequest(string? Decision, string? Note, int? Version); diff --git a/app/web/src/app/projects/[projectId]/requirements/[requirementId]/page.tsx b/app/web/src/app/projects/[projectId]/requirements/[requirementId]/page.tsx index c5cebab..6910f77 100644 --- a/app/web/src/app/projects/[projectId]/requirements/[requirementId]/page.tsx +++ b/app/web/src/app/projects/[projectId]/requirements/[requirementId]/page.tsx @@ -10,6 +10,9 @@ import { formatDate } from "@/lib/projects"; type Props = PageProps<"/projects/[projectId]/requirements/[requirementId]">; +/** The evidence actions on this page may read GitHub for up to a minute (ADR-048). */ +export const maxDuration = 60; + export async function generateMetadata({ params }: Props): Promise { const { projectId, requirementId } = await params; return { title: (await requireRequirement(projectId, requirementId)).title }; diff --git a/app/web/src/app/projects/evidence-actions.ts b/app/web/src/app/projects/evidence-actions.ts index 9a8f91c..b4c4bd7 100644 --- a/app/web/src/app/projects/evidence-actions.ts +++ b/app/web/src/app/projects/evidence-actions.ts @@ -10,6 +10,11 @@ export type EvidenceActionState = { error: string | null }; const GITHUB_DOWN = "GitHub could not be reached. Try again in a moment."; const GITHUB_NOT_SET_UP = "GitHub is not set up for this SpecThread deployment yet."; +/** + * Linking and refreshing read GitHub, one call per linked item and per release to compare, so + * they get longer than an ordinary API call. Keep this below the page's maxDuration (ADR-048). + */ +const GITHUB_WRITE_TIMEOUT_MS = 55_000; const refresh = (projectId: string, requirementId: string) => revalidatePath(`/projects/${projectId}/requirements/${requirementId}`); @@ -29,9 +34,10 @@ export async function linkEvidence( if (!isUuid(projectId) || !isUuid(requirementId)) return fail(failed); try { - const response = await apiFetch(`/requirements/${requirementId}/evidence`, { method: "POST", body: JSON.stringify({ reference }) }); + const response = await apiFetch(`/requirements/${requirementId}/evidence`, { method: "POST", body: JSON.stringify({ reference }), timeoutMs: GITHUB_WRITE_TIMEOUT_MS }); if (response.status === 400) return fail(fieldError(await response.json(), "reference") ?? failed); - // 409 explains itself: no repository, already linked, archived, the limit, or the app uninstalled. + // 409 explains itself: no repository, already linked, archived, the limit, the app uninstalled, + // or the evidence changing while GitHub was read. if (response.status === 409) return fail(problemDetail(await response.json()) ?? failed); if (response.status === 502) return fail(GITHUB_DOWN); if (response.status === 503) return fail(GITHUB_NOT_SET_UP); @@ -50,7 +56,7 @@ export async function refreshEvidence(projectId: string, requirementId: string): const failed = "The evidence could not be refreshed. Please try again."; if (!isUuid(projectId) || !isUuid(requirementId)) return { error: failed }; try { - const response = await apiFetch(`/requirements/${requirementId}/evidence/refresh`, { method: "POST" }); + const response = await apiFetch(`/requirements/${requirementId}/evidence/refresh`, { method: "POST", timeoutMs: GITHUB_WRITE_TIMEOUT_MS }); if (response.status === 409) return { error: problemDetail(await response.json()) ?? failed }; if (response.status === 502) return { error: GITHUB_DOWN }; if (response.status === 503) return { error: GITHUB_NOT_SET_UP }; diff --git a/app/web/src/lib/api.ts b/app/web/src/lib/api.ts index e87aa67..087d44f 100644 --- a/app/web/src/lib/api.ts +++ b/app/web/src/lib/api.ts @@ -11,12 +11,15 @@ import { auth } from "./auth"; * see docs/API.md. Throws when SPECTHREAD_API_URL is not configured or there is no session. * Within one server render every call shares a single token exchange. * - * Every call times out after API_TIMEOUT_MS. A GET that fails with a network error, a timeout, - * or 500/502/503/504 is retried once, because those are usually brief (a database connection - * refused for a moment, a restarting API) and a refresh would succeed. Writes are never - * retried, so nothing runs twice (ADR-046). + * Every call times out after API_TIMEOUT_MS, or after `timeoutMs` when the caller knows the API + * needs longer, such as a write that reads many items from GitHub (ADR-048). A GET that fails + * with a network error, a timeout, or 500/502/503/504 is retried once, because those are + * usually brief (a database connection refused for a moment, a restarting API) and a refresh + * would succeed. Writes are never retried, so nothing runs twice (ADR-046). */ -export async function apiFetch(path: `/${string}`, init: RequestInit = {}, incomingHeaders?: Headers): Promise { +export async function apiFetch( + path: `/${string}`, { timeoutMs = API_TIMEOUT_MS, ...init }: RequestInit & { timeoutMs?: number } = {}, incomingHeaders?: Headers, +): Promise { const base = apiBaseUrl(process.env.SPECTHREAD_API_URL); const url = new URL(path, base); if (url.origin !== base.origin) throw new Error("apiFetch path must stay on the API origin."); @@ -27,7 +30,7 @@ export async function apiFetch(path: `/${string}`, init: RequestInit = {}, incom if (typeof init.body === "string" && !requestHeaders.has("content-type")) { requestHeaders.set("content-type", "application/json"); } - const send = () => fetch(url, { ...init, headers: requestHeaders, cache: "no-store", signal: withTimeout(init.signal) }); + const send = () => fetch(url, { ...init, headers: requestHeaders, cache: "no-store", signal: withTimeout(init.signal, timeoutMs) }); if ((init.method ?? "GET").toUpperCase() !== "GET") return send(); try { const response = await send(); @@ -47,8 +50,8 @@ const RETRY_DELAY_MS = 300; const RETRYABLE_STATUSES = new Set([500, 502, 503, 504]); /** The caller's abort signal, if any, combined with this call's timeout. */ -function withTimeout(signal: AbortSignal | null | undefined) { - const timeout = AbortSignal.timeout(API_TIMEOUT_MS); +function withTimeout(signal: AbortSignal | null | undefined, timeoutMs: number) { + const timeout = AbortSignal.timeout(timeoutMs); return signal ? AbortSignal.any([signal, timeout]) : timeout; } diff --git a/docs/API.md b/docs/API.md index 7e4fd38..9acfbd7 100644 --- a/docs/API.md +++ b/docs/API.md @@ -53,8 +53,8 @@ to the project's team; non-members get 404 as if the project did not exist. | POST | `/github/repositories` | any user | `{ githubToken }` | 200 `{ installUrl, repositories: AvailableRepository[], truncated }` | | GET | `/projects/{projectId}/repository` | member | | 200 `{ repository: ProjectRepository \| null }` | -| PUT | `/projects/{projectId}/repository` | owner | `{ installationId, repositoryId, githubToken }` | 200 `{ repository: ProjectRepository }` | -| DELETE | `/projects/{projectId}/repository` | owner | | 204 (idempotent) | +| PUT | `/projects/{projectId}/repository` | team Owner/Admin | `{ installationId, repositoryId, githubToken }` | 200 `{ repository: ProjectRepository }` | +| DELETE | `/projects/{projectId}/repository` | team Owner/Admin | | 204 (idempotent) | | GET | `/requirements/{requirementId}/evidence` | member | | 200 `Evidence[]` (oldest first by GitHub's created time) | | POST | `/requirements/{requirementId}/evidence` | member | `{ reference }` | 201 `Evidence` | @@ -87,8 +87,14 @@ results of pull requests and commits (ADR-035), and what each release contains; commit itself is not re-read. `source` in the request is ignored: links made through this endpoint are always `manual` (ADR-036). 409 carries the reason in `detail`: already linked, 50 links reached, no repository -connected, the app uninstalled, or an archived requirement or project. 502 and 503 -mean the same as for repositories. +connected, the app uninstalled, an archived requirement or project, or the evidence +or connected repository changed while GitHub was being read (reload and retry). 502 +and 503 mean the same as for repositories. + +Link and refresh read GitHub first and only then take the team lock and save, so a +large refresh may take tens of seconds; the web app allows these two calls 55 seconds +(ADR-048). Every evidence, review, and repository write serializes with archiving +and membership changes in the project's team, like requirement writes. `githubToken` is the signed-in user's own GitHub token for the SpecThread GitHub App (ADR-032). In the web app use `gitHubAccess()` from `@/lib/github-data`; never send @@ -98,8 +104,6 @@ that installation is 400 on `repositoryId`; an installation the app can no longe on is 400 on `installationId`. GitHub being unreachable is 502. Missing or rejected app credentials are 503. -Adding a member needs the email of an existing SpecThread account with a verified -address. The match ignores case. Otherwise the response is 400 with an error on `email`. Every team member can read every project and edit its requirements. Only the team Owner/Admin manage projects. `teamId` is required on creation and immutable on rename. Missing/empty IDs return 400 field errors; unknown/inaccessible teams return @@ -181,7 +185,6 @@ author gets 403, and an archived requirement or project 409. Decisions are only added: there is no update or delete. A decision is `outdated` when the requirement's version, or the set of its evidence link IDs, differs from what was reviewed; refreshing evidence does not change that. -``` `ownerUserId` on Project is retained historical creator provenance. Use `teamRole` for management controls; project creation does not grant independent ownership. @@ -267,9 +270,7 @@ from protected pages to /onboarding, and POST /projects returns 409 with `code: "onboarding_required"` for an incomplete existing account. Anonymous requests remain 401, and an account without a user row remains 403. Project access follows the containing team as described above. -Not yet available: activity and audit history. Claude owns global -project pages and creation/deletion forms; this branch implements team tables and -the C# access contract. +Not yet available: activity and audit history. Team management uses authenticated server actions. Role changes require explicit diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 08212c8..5cd4815 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -157,7 +157,7 @@ API direction The ASP.NET API owns domain rules and database access. Next.js should call the documented API rather than duplicate domain logic in route handlers or server actions. Generate or maintain a typed client from OpenAPI only after the contract stabilizes enough to justify it. -Better Auth runs in Next.js with authentication-only database access. Users sign in with verified email and password, Google, or GitHub, and manage provider links at /settings/account (ADR-016, ADR-021). Verification and reset emails use Resend. The JWT plugin signs ES256 tokens that the API validates against the web app's JWKS (ADR-015). Team membership grants project/requirement access and team roles grant project management (ADR-040); team invitations are implemented; GitHub Apps repository integration remains pending. The dashboard shows labeled sample requirements (ADR-020) and, like every page except the landing, sign-in, policy, and token-invitation preview pages, requires a valid session (ADR-023). Auth requires an explicit secret, database URL, canonical origin, and email delivery configuration; remote database connections verify TLS. Optional dashboard integration requires an environment key. Auth validates CA PEM and complete GitHub credential pairs before initialization. Newly written OAuth tokens use Better Auth encryption; historical plaintext is not automatically rewritten. See DEPLOYMENT.md. +Better Auth runs in Next.js with authentication-only database access. Users sign in with verified email and password, Google, or GitHub, and manage provider links at /settings/account (ADR-016, ADR-021). Verification and reset emails use Resend. The JWT plugin signs ES256 tokens that the API validates against the web app's JWKS (ADR-015). Team membership grants project/requirement access and team roles grant project management (ADR-040); team invitations are implemented; a project connects one GitHub repository through the GitHub App (ADR-032). The dashboard shows labeled sample requirements (ADR-020) and, like every page except the landing, sign-in, policy, and token-invitation preview pages, requires a valid session (ADR-023). Auth requires an explicit secret, database URL, canonical origin, and email delivery configuration; remote database connections verify TLS. Optional dashboard integration requires an environment key. Auth validates CA PEM and complete GitHub credential pairs before initialization. Newly written OAuth tokens use Better Auth encryption; historical plaintext is not automatically rewritten. See DEPLOYMENT.md. A deny-by-default Next.js proxy validates the Better Auth session for every non-public page and redirects signed-out visitors to login with the requested page in `next` (ADR-023). Login and signup redirect an existing session to that validated page, defaulting to the dashboard. The route scaffold links placeholder pages with example IDs until product data and API membership authorization are implemented. @@ -173,8 +173,7 @@ user-bound HttpOnly cookie and stops asking on later navigations (ADR-042). Comp survives loss of membership. Server API calls use Better Auth JWTs; browser sidebar reads/preferences stay on same-origin session-checked routes. Startup still performs no database work. Team-owned project authorization and the compact team Projects/archive tables are -implemented using the accepted rules in ADR-040. Claude owns the global -project pages/forms. Member actions, settings, and ownership transfer use the agreed role rules and +implemented using the accepted rules in ADR-040. Member actions, settings, and ownership transfer use the agreed role rules and serialize mutations on the team row. Invitations share that lock, store only hashed secrets, and require acceptance by a verified matching email while the inviter remains authorized. Membership, acceptance, and onboarding completion are diff --git a/docs/DECISIONS.md b/docs/DECISIONS.md index 1953b2a..c2a7ca5 100644 --- a/docs/DECISIONS.md +++ b/docs/DECISIONS.md @@ -1131,6 +1131,38 @@ documents, search, renaming, and unarchiving are future work. Migration ProjectDocuments adds both tables with RLS on and browser grants revoked; CI applies it (ADR-044). +ADR-048: Writes that read GitHub lock the team only to save, and get a longer web timeout + +Status: Accepted + +Context: Requirement, project, and document writes lock the team row so they cannot +commit into a project being archived or for a member being removed (ADR-040, ADR-047). +Evidence link, refresh, and unlink, review decisions, and repository connect and +disconnect did not, and a refresh that met a concurrent unlink failed with an unhandled +500. Two links at once could also pass the 50-link limit. Separately, the web app gave +every API call 10 seconds (ADR-046), while a refresh makes several GitHub calls per +linked item and one per release and change pair, so refreshing a requirement with +many links failed in the browser. + +Decision: Every evidence, review, and repository write runs in a transaction that locks +the project's team and then re-reads membership and archived state, through +`RequirementEndpoints.LockForMember` or `ProjectEndpoints.LockProjectForCaller`. +Writes that read GitHub (link, refresh, connect) read it first, without the lock, so +a slow GitHub never holds up the team's other writes; they then lock, check again +that the caller is a member, nothing is archived, and the same repository is still +connected, and save. Link counts the links again under the lock. A row that changed +or disappeared while GitHub was being read answers 409 ("reload and try again") +instead of saving partial results. `apiFetch` accepts a per-call `timeoutMs`; link and +refresh use 55 seconds, and the requirement page sets `maxDuration = 60` so Vercel lets +its server actions run that long. Every other call keeps the 10-second limit and the +GET retry. + +Consequences: A write can wait briefly for another write in the same team. A refresh +that loses a race is refused, not merged, so the user refreshes again. The first refresh +of a requirement with many releases and many merged pull requests can still approach +the 55-second limit, because each release and change pair is a GitHub compare call; +later refreshes reuse earlier "included" answers. No schema change. + ADR-NNN: Title Status: Proposed, Accepted, Superseded, or Rejected diff --git a/docs/HANDOFF.md b/docs/HANDOFF.md index c7aaca0..de7d524 100644 --- a/docs/HANDOFF.md +++ b/docs/HANDOFF.md @@ -1,6 +1,60 @@ # Shared handoff -## Current task: Project documentation tab (2026-10-18) +## Current task: Evidence locks, refresh timeout, and doc corrections (2026-10-20) + +- Branch: `fix/evidence-locks-and-timeouts`, from `main` at `dac2bcd`. Fixes only; no new + features and no schema change. +- Problems (from a full scan of `main`, where every check passed): + - Refreshing evidence failed in the browser once it took longer than `apiFetch`'s 10-second + limit, which a requirement with about 20 or more linked pull requests reaches. + - A refresh that met a concurrent unlink answered an unhandled 500. + - Evidence link/refresh/unlink, review decisions, and repository connect/disconnect did not + take the team lock, so they could commit into a project being archived or for a member + being removed. Two links at once could pass the 50-link limit. + - `docs/API.md` and `docs/ARCHITECTURE.md` disagreed with the code. +- Completed (ADR-048): + - Those six writes run in a transaction that locks the project's team and re-reads + membership and archived state (`RequirementEndpoints.LockForMember`, now internal, or + `ProjectEndpoints.LockProjectForCaller`). Link, refresh, and connect read GitHub first, + without the lock, then lock, check again (member, not archived, same repository), and + save. Link counts links again under the lock. A row removed while GitHub was read answers + 409 "The evidence changed while it was being read from GitHub. Reload and try again." + - `apiFetch` takes an optional `timeoutMs`; link and refresh use 55 seconds, and the + requirement page sets `maxDuration = 60`. Every other call keeps 10 seconds and the GET retry. + - Docs: repository connect/disconnect is team Owner/Admin; the retired add-member paragraph, + a stray code fence that turned the rest of API.md into code, and two stale + ownership notes are removed; ARCHITECTURE no longer calls the GitHub App pending. +- Changed files: + - API: `GitHub/EvidenceEndpoints.cs`, `GitHub/RepositoryEndpoints.cs`, + `Requirements/RequirementEndpoints.cs`, `Requirements/ReviewEndpoints.cs`. + - Web: `lib/api.ts`, `app/projects/evidence-actions.ts`, + `app/projects/[projectId]/requirements/[requirementId]/page.tsx`. + - Tests: new `tests/support/team-lock.ts` (holds a team's row lock from the test database); + new cases in `tests/schema/{evidence,reviews,github-repository}.spec.ts` and + `tests/e2e/evidence.spec.ts`. + - Docs: `API.md`, `ARCHITECTURE.md`, `DECISIONS.md` (ADR-048), this file. +- Decisions and assumptions: GitHub is never read while the team lock is held, so a slow + GitHub cannot stall the team's other writes. A refresh that loses a race is refused rather + than merged. 55 seconds leaves the server action time to answer before Vercel's 60. +- Checks: + - The six new tests failed against the old code: the lock tests completed without waiting, + and the browser refresh hit the 10-second timeout. All six pass with the fix. + - Passed: `npm run lint`, `npm run typecheck`, `npm run build:api`, + `dotnet format app/api --verify-no-changes --no-restore`, + `dotnet ef migrations has-pending-model-changes` (no changes), and `git diff --check`. + - `CI=1 npm test`: 347 passed (341 before, plus the six new tests). Run with Chromium 141 + instead of the version Playwright 1.63 expects. +- Known issues and risks: + - The first refresh of a requirement with many releases and many merged pull requests can + still approach 55 seconds (one GitHub compare per release and change pair). + - `maxDuration = 60` is not verified on Vercel from here. + - Still out of scope from the scan: the Playwright verification feature, audit history, + evidence policies, suggested links and webhooks, the sample dashboard, and the + placeholder pages. +- Next step: review and merge the pull request, then refresh a requirement with many links on + spec-thread.com and confirm it completes. + +## Previous task: Project documentation tab (2026-10-18) - Branch: `feature/project-docs`, rebuilt on `origin/main` at `35b407c`. The first version was written before the teams, tabs, and black-and-white work reached `main`; it is kept diff --git a/tests/e2e/evidence.spec.ts b/tests/e2e/evidence.spec.ts index 15e793e..8c2513b 100644 --- a/tests/e2e/evidence.spec.ts +++ b/tests/e2e/evidence.spec.ts @@ -406,6 +406,20 @@ test("refreshing reads the current state from GitHub and keeps items GitHub no l await expect(rows(page).nth(2)).toContainText("Open issue"); }); +test("a refresh that takes longer than an ordinary API call still completes", async ({ page }) => { + test.slow(); + const { projectId, requirementId, url } = await createRequirement(page); + const { repositoryId } = await connect(projectId, [issue(4, "Bug")]); + await page.goto(url); + await link(page, "4"); + await setGitHubItems(repositoryId, [issue(4, "Bug (confirmed)")]); + // Past the 10-second limit ordinary API calls have, as a refresh of many links can be. + await failApi(owner.userId, { method: "POST", path: `^/requirements/${requirementId}/evidence/refresh$`, delayMs: 11_000, times: 1 }); + await page.getByRole("button", { name: "Refresh from GitHub" }).click(); + await expect(rows(page).nth(0)).toContainText("Bug (confirmed)", { timeout: 20_000 }); + await expect(page.getByText("The evidence could not be refreshed. Please try again.")).toHaveCount(0); +}); + test("a link is removed without touching the others", async ({ page }) => { const { projectId, url } = await createRequirement(page); await connect(projectId, [issue(1, "Keep"), issue(2, "Remove")]); diff --git a/tests/schema/evidence.spec.ts b/tests/schema/evidence.spec.ts index 8e4a513..cc916c0 100644 --- a/tests/schema/evidence.spec.ts +++ b/tests/schema/evidence.spec.ts @@ -5,6 +5,7 @@ import type { Server } from "node:http"; import { startTestDatabase } from "../../scripts/test-database.mjs"; import { startFakeGitHub } from "../../scripts/test-github.mjs"; import { startApi } from "../support/api-process"; +import { stillWaiting, withTeamLock } from "../support/team-lock"; // The evidence API (ADR-033) called directly, against a disposable PostgreSQL database and a // local stand-in for GitHub. @@ -196,6 +197,64 @@ test("a requirement holds at most 50 links", async ({ request }) => { expect(await (await owner.get(path)).json()).toHaveLength(50); }); +test("linking, refreshing, and unlinking wait for the team lock and refuse once the project is archived", async ({ request }) => { + const { path, projectId } = await setUp(request, [item(1, "One"), item(2, "Two")]); + const member = await as(request, "member"); + const linked = await (await member.post(path, { reference: "1" })).json(); + const archive = "UPDATE public.projects SET archived_at = now() WHERE id = $1"; + + for (const [name, send] of [ + ["link", () => member.post(path, { reference: "2" })], + ["refresh", () => member.post(`${path}/refresh`)], + ["unlink", () => member.delete(`${path}/${linked.id}`)], + ] as const) { + const pending = await withTeamLock(database.pool, projectId, async client => { + const response = send(); + expect(await stillWaiting(response), name).toBe(true); + await client.query(archive, [projectId]); + return { response }; + }); + expect((await pending.response).status(), name).toBe(409); + await database.pool.query("UPDATE public.projects SET archived_at = NULL WHERE id = $1", [projectId]); + } + expect((await (await member.get(path)).json()).map((e: { number: number }) => e.number)).toEqual([1]); +}); + +test("a refresh that races an unlink answers 409 and saves nothing", async ({ request }) => { + const { path, projectId, repositoryId } = await setUp(request, [item(1, "One"), item(2, "Two")]); + const member = await as(request, "member"); + await member.post(path, { reference: "1" }); + const removed = await (await member.post(path, { reference: "2" })).json(); + await request.put(`${gitHub}/__github/repositories/${repositoryId}/items`, { data: { items: [item(1, "One (renamed)"), item(2, "Two")] } }); + + const pending = await withTeamLock(database.pool, projectId, async client => { + const response = member.post(`${path}/refresh`); + expect(await stillWaiting(response)).toBe(true); + await client.query("DELETE FROM public.requirement_evidence WHERE id = $1", [removed.id]); + return { response }; + }); + const refreshed = await pending.response; + expect(refreshed.status()).toBe(409); + expect((await refreshed.json()).detail).toBe("The evidence changed while it was being read from GitHub. Reload and try again."); + expect((await (await member.get(path)).json()).map((e: { title: string }) => e.title)).toEqual(["One"]); +}); + +test("two links added at once never take a requirement past 50", async ({ request }) => { + const { path, projectId, requirementId, repositoryId } = await setUp(request, [item(60, "Sixty"), item(61, "Sixty-one")]); + await database.pool.query(`INSERT INTO public.requirement_evidence + (requirement_id,kind,repository_id,repository_owner,repository_name,number,title,state,url,github_created_at,github_updated_at,linked_by) + SELECT $1,'issue',$2,'acme','web',n,'Seeded','open','https://github.com/acme/web/issues/'||n,now(),now(),'owner' FROM generate_series(1,49) AS n`, + [requirementId, repositoryId]); + const owner = await as(request, "owner"); + const responses = await withTeamLock(database.pool, projectId, async () => { + const both = [owner.post(path, { reference: "60" }), owner.post(path, { reference: "61" })]; + expect(await stillWaiting(Promise.all(both))).toBe(true); + return both; + }); + expect((await Promise.all(responses)).map(response => response.status()).sort()).toEqual([201, 409]); + expect(await (await owner.get(path)).json()).toHaveLength(50); +}); + test("GitHub failures map to 502, 503, and 409 and never save a partial refresh", async ({ request }) => { const { path, repositoryId, installationId } = await setUp(request, [item(1, "One"), item(2, "Two")]); const owner = await as(request, "owner"); diff --git a/tests/schema/github-repository.spec.ts b/tests/schema/github-repository.spec.ts index 8d0922b..6e76d39 100644 --- a/tests/schema/github-repository.spec.ts +++ b/tests/schema/github-repository.spec.ts @@ -4,6 +4,7 @@ import type { Server } from "node:http"; import { startTestDatabase } from "../../scripts/test-database.mjs"; import { startFakeGitHub } from "../../scripts/test-github.mjs"; import { startApi } from "../support/api-process"; +import { stillWaiting, withTeamLock } from "../support/team-lock"; // The repository connection API (ADR-032) called directly, as any signed-in user could, // against a disposable PostgreSQL database and a local stand-in for GitHub. @@ -117,6 +118,34 @@ test("only the owner changes the connection; members read it; others cannot tell expect((await (await member.get(`/projects/${projectId}/repository`)).json()).repository.fullName).toBe("acme/web"); }); +test("connecting and disconnecting wait for the team lock and refuse once the project is archived", async ({ request }) => { + const projectId = await createProject(request); + const owner = await as(request, "owner"); + await gitHubShows(request, "lock-token", [{ id: 71, repositories: [{ id: 701, owner: "acme", name: "web" }] }]); + const archive = "UPDATE public.projects SET archived_at = now() WHERE id = $1"; + const body = { installationId: 71, repositoryId: 701, githubToken: "lock-token" }; + + const connect = await withTeamLock(database.pool, projectId, async client => { + const response = owner.put(`/projects/${projectId}/repository`, body); + expect(await stillWaiting(response)).toBe(true); + await client.query(archive, [projectId]); + return { response }; + }); + expect((await connect.response).status()).toBe(409); + expect(await (await owner.get(`/projects/${projectId}/repository`)).json()).toEqual({ repository: null }); + + await database.pool.query("UPDATE public.projects SET archived_at = NULL WHERE id = $1", [projectId]); + expect((await owner.put(`/projects/${projectId}/repository`, body)).status()).toBe(200); + const disconnect = await withTeamLock(database.pool, projectId, async client => { + const response = owner.delete(`/projects/${projectId}/repository`); + expect(await stillWaiting(response)).toBe(true); + await client.query(archive, [projectId]); + return { response }; + }); + expect((await disconnect.response).status()).toBe(409); + expect((await (await owner.get(`/projects/${projectId}/repository`)).json()).repository).toMatchObject({ repositoryId: 701 }); +}); + test("a repository is connected only when GitHub shows it to that user through that installation", async ({ request }) => { const projectId = await createProject(request); const owner = await as(request, "owner"); diff --git a/tests/schema/reviews.spec.ts b/tests/schema/reviews.spec.ts index ec54196..588c6bf 100644 --- a/tests/schema/reviews.spec.ts +++ b/tests/schema/reviews.spec.ts @@ -2,6 +2,7 @@ import { expect, test, type APIRequestContext } from "@playwright/test"; import { readFile } from "node:fs/promises"; import { startTestDatabase } from "../../scripts/test-database.mjs"; import { startApi } from "../support/api-process"; +import { stillWaiting, withTeamLock } from "../support/team-lock"; // The review API (ADR-038) called directly, against a disposable PostgreSQL database. // Evidence links are inserted as rows, so GitHub is not involved. @@ -182,6 +183,25 @@ test("a decision on a version the reviewer did not see, or on something archived expect(await (await reviewer.get(`/requirements/${other.id}/reviews`)).json()).toEqual([]); }); +test("a decision waits for the team lock, then is refused if the project was archived or the requirement edited meanwhile", async ({ request }) => { + const { projectId, requirementId, path } = await setUp(request); + const reviewer = await as(request, "reviewer"); + for (const [name, change] of [ + ["archived", "UPDATE public.projects SET archived_at = now() WHERE id = $1"], + ["edited", "UPDATE public.requirements SET version = version + 1 WHERE project_id = $1"], + ] as const) { + const pending = await withTeamLock(database.pool, projectId, async client => { + const response = reviewer.post(path, { decision: "accepted", version: 1 }); + expect(await stillWaiting(response), name).toBe(true); + await client.query(change, [projectId]); + return { response }; + }); + expect((await pending.response).status(), name).toBe(409); + await database.pool.query("UPDATE public.projects SET archived_at = NULL WHERE id = $1", [projectId]); + } + expect((await database.pool.query("SELECT count(*)::int AS count FROM public.requirement_reviews WHERE requirement_id = $1", [requirementId])).rows[0].count).toBe(0); +}); + test("requirement lists carry the latest decision and whether it still describes the requirement", async ({ request }) => { const { projectId, requirementId, path } = await setUp(request); const [reviewer, author] = [await as(request, "reviewer"), await as(request, "author")]; diff --git a/tests/support/team-lock.ts b/tests/support/team-lock.ts new file mode 100644 index 0000000..5c9b8af --- /dev/null +++ b/tests/support/team-lock.ts @@ -0,0 +1,34 @@ +import type { Pool, PoolClient } from "pg"; + +/** + * Holds the row lock of a project's team in a test transaction, the way a concurrent team write + * does, and runs `during` while it is held. `during` may run SQL inside the same transaction, for + * example to archive the project, through `client`. The transaction commits after `during`, + * releasing the lock, and rolls back if `during` throws. + */ +export async function withTeamLock(pool: Pool, projectId: string, during: (client: PoolClient) => Promise): Promise { + const client = await pool.connect(); + try { + await client.query("BEGIN"); + await client.query( + "SELECT 1 FROM public.teams WHERE id = (SELECT team_id FROM public.projects WHERE id = $1) FOR UPDATE", [projectId]); + const result = await during(client); + await client.query("COMMIT"); + return result; + } catch (error) { + await client.query("ROLLBACK"); + throw error; + } finally { + client.release(); + } +} + +/** Reports whether `pending` is still unsettled after `ms`, meaning the request is waiting on a lock. */ +export async function stillWaiting(pending: Promise, ms = 500) { + const waiting = Symbol("waiting"); + const settled = await Promise.race([ + pending.then(() => null, () => null), + new Promise(resolve => setTimeout(() => resolve(waiting), ms)), + ]); + return settled === waiting; +}