fix(projects): surface primary-workspace provenance, close the fallback drift hole (BLO-26184) - #1336
Conversation
1 similar comment
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally please review at head This PR was opened during the fleet-wide review outage tracked in BLO-26654 (codex provider quota exhaustion, from 2026-08-12T09:24Z) and has never been reviewed on either surface. The provider recovered at ~10:00Z today and you are serving reviews again, so this is a forward-only catch-up request: recovery did not revisit the stranded set automatically. Review focus: normal full pass at the head above. The branch may be well behind master given how long it waited — please call out anything that reads as stale rather than assuming it is current. |
|
@ally please review at head 348c97f — projects: primary-workspace provenance + fallback drift hole. Focus on whether the fallback removal can leave a project with no resolvable workspace. Context: the original review request on this PR was lost during the codex provider outage (BLO-27123) — codex |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 348c97f
Critical Issues (0)
- None.
Important Issues (1)
- [native-codex]
server/src/services/projects.ts:617—ensureSinglePrimaryWorkspacestill has a correctness hole after the fallback-removal fix: it demotes every workspace, then tries at most five candidate IDs. If the originally selected workspace is concurrently removed and the project has more than five surviving workspaces, the loop exits after the fifth failed promotion whilecandidateIdstill points at an untried row, leaving all rowsisPrimary=falseand the project still without an explicit primary.- Continue until a promotion succeeds or the candidate query is exhausted (or use a transaction/locking strategy that makes the candidate stable); add a regression test with at least six candidates and a removed initial target. The termination comment currently claims all distinct candidates are tried, which is not true with
MAX_ATTEMPTS = 5.
- Continue until a promotion succeeds or the candidate query is exhausted (or use a transaction/locking strategy that makes the candidate stable); add a regression test with at least six candidates and a removed initial target. The termination comment currently claims all distinct candidates are tried, which is not true with
Suggestions (0)
- None.
Strengths
- The response model cleanly distinguishes
explicit,inferred, andnone, and the read-path fallback is observable through both a metric and structured logging. - The diff adds focused embedded-Postgres coverage for zero-workspace, inferred fallback, explicit primary, and write-time healing paths.
Recommended Action
- Fix the Important issue before merge.
- Re-run the focused workspace provenance and regression suites.
… (BLO-26184) Addresses the Important finding in Ally's 2026-08-14 review of #1336. ensureSinglePrimaryWorkspace demotes every workspace unconditionally and then retries promotion against surviving candidates. That retry was capped at MAX_ATTEMPTS = 5, which is a number real candidates can reach: on a project with >5 workspaces whose promotions kept losing the race to a concurrent removal, the loop exited with candidateId still pointing at an untried row. The caller's warning only covered the exhausted branch, so that exit was silent — leaving N workspaces / 0 primaries, which is the exact drift shape this PR exists to close, re-created inside its own fix. The doc comment also claimed all distinct candidates are tried, which was not true under the cap. Extract the walk into promoteFirstSurvivingWorkspace, which returns "promoted" | "candidates_exhausted" | "attempt_cap_reached" so the caller can tell "nothing left to promote" from "gave up with work remaining", and warn on both non-promoted outcomes. The retained maxAttempts (1000) is a safety valve against unbounded concurrent INSERTs only; the real bound is the candidate pool, since each pass marks one distinct id tried and the next candidate is drawn from the untried remainder. Extracting the walk also makes it testable without mocking drizzle: the four new cases are pure and run on every host rather than only where embedded Postgres is available. The regression case uses seven candidates with the initial target plus five more removed, and fails against the old cap with "attempt_cap_reached" instead of "promoted".
|
@ally please re-review at head This addresses the single Important finding from your 2026-08-14 review — the What changed:
Review focus, and where I'd most like a second opinion:
Extraction was also what made the walk testable without mocking drizzle — the four new Full file green locally: 10/10 (6 pre-existing embedded-Postgres + 4 new). |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: f7c2c6d
Prior Findings Dispositioned (1)
- prior:348c97f important 1 — fixed —
server/src/services/projects.ts:606— the promotion walk now tracks tried IDs, keeps selecting an untried candidate after each failed promotion, and only returnsattempt_cap_reachedwhen the deliberate safety ceiling is actually reached.
Critical Issues (0)
- None.
Important Issues (0)
- None.
Suggestions (1)
- [tests]
server/src/__tests__/project-primary-workspace-provenance.test.ts:237— GitHub Actions cancelledGeneral tests (server 4/4)before it emitted a test result. Re-run that shard before merge; the other build, typecheck, e2e, and general-test shards passed.
Strengths
- The extracted promotion walk resolves the previous five-candidate gap and has direct, host-independent regression coverage for both exhaustion and safety-cap outcomes.
- Both non-promoted outcomes now log the project and tried candidates, preserving the intended fail-open behavior without silent drift.
Recommended Action
- Re-run the cancelled general-test shard before merge.
- Consider the suggestion opportunistically.
…lback drift hole (BLO-26184)
The earliest-created fallback stays affirmed behaviour (CTO decision on
BLO-23599): a multi-workspace project with no explicit primary must not
refuse to resolve. What was silent is now observable:
- `pickPrimaryWorkspace` returns a `primaryWorkspaceSource`
("explicit"|"inferred"|"none") alongside the resolved workspace, surfaced
on `GET /api/projects/{id}` and every other project read. The two
plugin-host-services helpers that hardcoded `isPrimary: true` for the
plugin-facing project API now report the same provenance instead of
presenting a guess as a choice.
- A `paperclip_project_primary_workspace_fallback_total` counter plus a
structured warn log (with projectId) fire whenever a multi-workspace
project resolves via the fallback branch, in both projects.ts and the
matching issues.ts execution-workspace materialization path — the
alertable signal that would have caught CDN+ Supply Side Rewards on day
one. Fleet baseline is 0/80, so this counter should read 0 in steady
state.
- ensureSinglePrimaryWorkspace had a TOCTOU hole: it demotes every workspace
unconditionally, then promotes a `keepWorkspaceId` chosen before its own
transaction started. A concurrent write to the same project (e.g. two
overlapping removeWorkspace calls) could delete that exact row in
between, so the promote update silently matched zero rows and the project
was left with N workspaces / 0 primaries — the CDN+ symptom shape. Fixed
by verifying the promote affected a row and retrying against surviving
candidates before giving up with a warning.
- company-portability.ts's workspace import and plugin-host-services.ts
were investigated as root-cause candidates: company-portability's
sequential create loop self-corrects to exactly one primary under normal
execution, and plugin-host-services never writes project_workspaces at
all (read-only), so both are ruled out as the write-path drift cause with
evidence rather than assertion.
New test suite (server/src/__tests__/project-primary-workspace-provenance.test.ts)
covers the acceptance criteria directly: 0-workspace -> null/"none",
multi-workspace/no-primary -> earliest-created + "inferred" + counter
increment, explicit primary -> "explicit", add/remove sequences preserve
exactly one isPrimary=true, and a drifted project self-heals the next time
any workspace write touches it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… (BLO-26184) Addresses the Important finding in Ally's 2026-08-14 review of #1336. ensureSinglePrimaryWorkspace demotes every workspace unconditionally and then retries promotion against surviving candidates. That retry was capped at MAX_ATTEMPTS = 5, which is a number real candidates can reach: on a project with >5 workspaces whose promotions kept losing the race to a concurrent removal, the loop exited with candidateId still pointing at an untried row. The caller's warning only covered the exhausted branch, so that exit was silent — leaving N workspaces / 0 primaries, which is the exact drift shape this PR exists to close, re-created inside its own fix. The doc comment also claimed all distinct candidates are tried, which was not true under the cap. Extract the walk into promoteFirstSurvivingWorkspace, which returns "promoted" | "candidates_exhausted" | "attempt_cap_reached" so the caller can tell "nothing left to promote" from "gave up with work remaining", and warn on both non-promoted outcomes. The retained maxAttempts (1000) is a safety valve against unbounded concurrent INSERTs only; the real bound is the candidate pool, since each pass marks one distinct id tried and the next candidate is drawn from the untried remainder. Extracting the walk also makes it testable without mocking drizzle: the four new cases are pure and run on every host rather than only where embedded Postgres is available. The regression case uses seven candidates with the initial target plus five more removed, and fails against the old cap with "attempt_cap_reached" instead of "promoted".
f7c2c6d to
5e28147
Compare
1 participant
Thinking Path
Linked Issues or Issue Description
Paperclip issue BLO-26184 (internal tracker, no GitHub issue — see https://paperclip.blockcast.net/BLO/issues/BLO-26184), item 3 of BLO-23599.
Problem:
server/src/services/projects.ts'spickPrimaryWorkspacesilently falls back to the earliest-created workspace when no row is flaggedisPrimary, and the API/plugin surfaces present that guess exactly like an explicit choice. CDN+ Supply Side Rewards reached 3 workspaces / 0 primaries with no signal anywhere that resolution was inferred rather than chosen.CTO decision (recorded in the issue): the earliest-created fallback stays — refusing to resolve at read time would turn a mis-route into a fleet-wide outage. The fix is provenance + drift detection, not a refusal path.
What Changed
pickPrimaryWorkspacenow returns{ workspace, source }wheresourceis"explicit" | "inferred" | "none".primaryWorkspaceSourceis added to theProjectread model (optional, to avoid breaking every existing UI fixture — the real services always populate it) and threaded throughattachWorkspaces(the single choke point for get/getById/list/listByIds/update).paperclip_project_primary_workspace_fallback_totalPrometheus counter (server/src/services/metrics.ts) plus a structuredlogger.warn({ projectId }, ...)fire whenever a multi-workspace project resolves via the fallback branch — in bothprojects.tsand the parallel execution-workspace materialization path inissues.ts(same silent-guess shape, same fix). Fleet baseline is 0/80 non-archived projects, so this counter should read 0 in steady state; non-zero is a real drift event.plugin-host-services.tshelpers (getPrimaryWorkspace,getWorkspaceForIssue) hardcodedisPrimary: truefor the plugin-facing project API regardless of provenance — now reportproject.primaryWorkspaceSource === "explicit"instead.company-portability.ts's workspace import loop: ruled out — its sequentialfor...of(noPromise.all) means the first successfully-inserted workspace on any project always becomes primary viacreateWorkspace'sexisting.length === 0check, and laterisPrimary: trueentries correctly demote others. Self-corrects under normal execution.plugin-host-services.ts: ruled out as a write-path cause — grepped forcreateWorkspace/updateWorkspace/removeWorkspaceand found none; it's read-only againstproject_workspaces.ensureSinglePrimaryWorkspacedemotes every workspace on a project unconditionally, then promotes akeepWorkspaceIdthe caller selected before this function's own statements ran. A concurrent write to the same project (e.g. two overlappingremoveWorkspacecalls) can delete that exact row in the window between selection and promotion — the promoteUPDATEthen matches zero rows, and the project is left with N workspaces / 0 primaries. This is the CDN+ symptom shape exactly. Fixed: the promote now verifies it affected a row and retries against surviving candidates (bounded, terminates), warning if every candidate was concurrently removed. I could not confirm this against CDN+'s actual audit trail (not accessible from this environment) — it's presented as a logical TOCTOU proof, fixed regardless of confirmation status, per the issue's "identified and fixed, or documented as unreproducible with the evidence that rules out each candidate" acceptance bar.server/src/__tests__/project-primary-workspace-provenance.test.ts(embedded Postgres) covering the acceptance criteria directly: 0-workspace →null/"none"; multi-workspace/no-primary → earliest-created +"inferred"+ counter increment; explicit primary →"explicit"; fail-open (never throws) for 0 or ≥1 workspaces; add/remove sequences preserve exactly oneisPrimary=true; and a drifted 0-primary project self-heals the next time any workspace write (even one that doesn't touchisPrimary) touches it.Verification
pnpm --filter @paperclipai/shared build,pnpm --filter @paperclipai/plugin-sdk build,pnpm --filter @paperclipai/server exec tsc --noEmit -p .,pnpm --filter @paperclipai/ui exec tsc --noEmit -p .— all clean.pnpm exec vitest run src/__tests__/project-primary-workspace-provenance.test.ts— 6/6 passed, fallback warn log observed firing withprojectId.pnpm exec vitest run src/__tests__/project-icon-persistence.test.ts src/__tests__/metrics-service.test.ts— 69/69 passed (no regression in adjacent project/metrics coverage).workspace-runtime.test.tson unrelated runtime-service port-binding flakiness (EADDRINUSE, service-adoption timing); this PR does not touchworkspace-runtime.ts.General tests (server 1-4/4),Typecheck + Release Registry,Build,e2e,security-review,policy. (Storybook visual regressionisskipped— no UI changes.)repo:Blockcast/paperclipforprimaryWorkspace,pickPrimaryWorkspace,BLO-26184,BLO-23599): no duplicate or overlapping open PR found. #1323 touches issue delegation repo-binding, an unrelated subsystem.Risks
primaryWorkspaceSourceis optional onProject) — no existing consumer ofprimaryWorkspace/workspacesis affected.ensureSinglePrimaryWorkspace's retry loop is bounded (5 attempts) and runs inside the same DB transaction as before; worst case on exhaustion is an unchanged-from-before zero-primary state plus a new warning log, never a new failure mode.project_idgoes on the paired log line, not the metric).Model Used
Claude Sonnet 5 (
claude-sonnet-5[1m], 1M context), via Claude Code — no extended thinking, standard tool use (Bash, Read, Edit, embedded-Postgres test execution).Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template