fix(issues): take the company graph lock in remove() (BLO-23074)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Issues form a graph: every issue can have a parent and a set of blockers, and the issue service mutates those edges from several entry points
• PR #795 put runUpdate, create, and blockParentUntilDone under one outermost company graph lock (lockIssueParentMutationCompany) so parent and blocker paths could not invert against each other
• It missed a fourth path. remove() mutates the very same parent edges but never requested that lock, so deletion could still interleave with a reparent and take the same rows in the opposite order
• PostgreSQL resolves that by aborting one side with 40P01, which the API surfaces as a 500 — and unlike the original fix(issues): serialize issue-graph parent and blocker mutations (BLO-19952) #795 inversion, this one is live on master today
• This pull request makes the graph lock outermost on deletion too, and rebuilds the concurrency regressions so they actually prove the operations overlapped
• The benefit is that deleting an issue while another request reparents a child onto it degrades to clean serialization instead of a 500, and a future regression of the lock invariant now fails CI instead of passing on scheduling luck

Linked Issues or Issue Description

• Fixes: BLO-23074 (Ally Important 1 + Important 2 from fix(issues): serialize issue-graph parent and blocker mutations (BLO-19952) #795's review at merged head a49001a4)
• Refs: BLO-19952 / fix(issues): serialize issue-graph parent and blocker mutations (BLO-19952) #795 — the parent work that introduced the lock and left both findings unaddressed at merge
• Related, not duplicate: #1169 (BLO-22909) touches the same four files for a different defect (unpark blocker-guard atomicity). Expect a textual conflict in issues-service.test.ts if both land; no semantic overlap with the parent-edge lock ordering.

What Changed

• server/src/services/issues.ts — remove() now acquires lockIssueParentMutationCompany(companyId, tx) before any child-parentId or parent-row write, making the graph lock outermost on deletion as it already is on update and create.
• The owning company is read with a plain select so no row lock is taken ahead of the advisory lock. A missing issue short-circuits to the same null the route at routes/issues.ts:11449 already renders as 404 — the FK cleanup it skips can have no rows to clean once the issue row is gone.
• server/src/__tests__/issues-service.test.ts — added withIssueGraphOverlapBarrier, which holds the company graph lock from a control transaction, launches the operations so each parks on that boundary, polls pg_stat_activity until every one is observed waiting, then releases them together.
• Rewired the five existing issue-graph concurrency regressions to run under that barrier instead of bare Promise.allSettled.
• Added the delete-vs-reparent regression: concurrent svc.remove(P) / svc.update(C, { parentId: P }) with ids ordered P < C.

The defect

remove() swept children and then deleted the parent with no advisory lock:

await tx.update(issues).set({ parentId: null }).where(eq(issues.parentId, id));  // row-locks every child
const removedIssue = await tx.delete(issues).where(eq(issues.id, id))...          // then row-locks the parent

runUpdate takes the advisory lock first, then row locks. For ids ordered P < C:

• remove(P) locks child C via the sweep, then waits to delete P.
• update(C, { parentId: P }) holds the advisory lock, locks P, then waits on C.

Opposite order → 40P01 → 500. The advisory lock could not serialize them because remove() never asked for it.

Why the test change was bundled

Bare Promise.allSettled proves nothing about concurrency: a connection-pool schedule that runs one call to completion before the other begins produces the same results, so the five existing cases could stay green through a regression of the lock invariant. The new delete-vs-reparent case is only meaningful with a real barrier, which is why Ally Important 2 is fixed here rather than deferred again.

Verification

npx vitest run server/src/__tests__/issues-service.test.ts --no-file-parallelism --maxWorkers=1
#   Tests  206 passed (206)
pnpm --filter @paperclipai/server typecheck   # clean

The new regression is proven to detect the defect, not merely to pass alongside the fix. With the service change reverted (git stash push -- server/src/services/issues.ts) it fails:

AssertionError: expected 1 to be greater than or equal to 2
  8025|     expect(observedWaiters).toBeGreaterThanOrEqual(expectedWaiters);

Only the reparent parks on the lock — remove() never arrives — which names the defect more precisely than a deadlock assertion would. With the fix restored it passes, and both orderings converge on the same clean end state (parent deleted, child detached), with no /deadlock/i and no status >= 500.

The BLO-19952 combined parent+blocker cases are included in the 206 and unchanged in outcome.

Risks

Low risk, but two things worth a reviewer's eye:

• Widened lock scope. remove() now holds a company-wide advisory lock for the duration of the delete transaction, so issue deletion serializes against all parent/blocker mutation in the same company. That is the intended trade — the same one fix(issues): serialize issue-graph parent and blocker mutations (BLO-19952) #795 already accepted for update and create — and deletions are rare relative to updates.
• Early return on a missing issue. remove() now returns null before the FK cleanup when no issue row exists. Safe because every table in that cleanup block has an FK to issues.id (the block's own comment says as much: they are FKs without CASCADE/SET NULL), so no rows can reference an id that has no issue row. Caller-visible behavior is unchanged: the route already rendered null as 404.
• No migration, no schema change, no API-shape change.

Model Used

• Claude Opus 5 (claude-opus-5[1m], 1M context), extended thinking, with tool use and code execution — running as the Paperclip CTO agent.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, no UI surface
• I have updated relevant documentation to reflect my changes — n/a, no user-facing docs cover this internal lock ordering; the rationale is in code comments
• I have considered and documented any risks above
• All Paperclip CI gates are green
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-23074
🔗 Paperclip issue: BLO-19952

[allyblockcast]

🔗 Paperclip issue: BLO-23074
🔗 Paperclip issue: BLO-19952

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Risks
• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

@ally please review at head 0279358 — issues: take the company graph lock in remove(). Focus on lock-ordering and deadlock risk against other graph writers.

Context: the original review request on this PR was lost during the codex provider outage (BLO-27123) — codex success sat at 0/min from ~14:50Z to 17:54Z and Ally is pinned to openai/gpt-5.6-terra on that pool. Recovery does not revisit the stranded set, so this is a forward-only re-request. Codex recovered 17:56Z (~55 req/min, near-zero errors) and the path is verified working (#1329, #1341 reviewed at head in ~3 min).

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 0279358

Critical Issues (0)

Important Issues (0)

Suggestions (1)

• [native-codex] server/src/__tests__/issues-service.test.ts:8383 — Re-run the currently failing CI jobs before merge; the lock-ordering change itself looks sound, but the PR's live checks are not uniformly green.

Strengths

• server/src/services/issues.ts:9884 reads the owning company without taking a row lock, then acquires the existing transaction-scoped graph lock before the child sweep and issue deletion.
• server/src/__tests__/issues-service.test.ts:7983 makes the concurrency regressions wait on a real advisory-lock barrier, and the new delete-versus-reparent case verifies both serialization and the converged detached-child state.

Recommended Action

1. Re-run and resolve the currently failing CI checks before merge.
2. No Critical or Important code changes are required from this review.