fix(plugins): make the masked config round trip concurrency-safe (BLO-26529)

[allyblockcast]

Closes BLO-26529. Follow-up to #1221 (parent: BLO-20794). Sibling: #1339.

Paperclip issue: https://paperclip.blockcast.net/BLO/issues/BLO-26529

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Plugins hold their own instance config, and that config carries live credentials — the production Alertmanager bearer among them
• fix(plugins): prevent secret-ref diagnostic reflection (BLO-20871) #1221 stopped GET /api/plugins/:pluginId/config from leaking those credentials by masking secret fields with a __redacted__ sentinel, and restoring the real value on write when the caller echoes the sentinel back
• That restore read the stored config through a registry.getConfig() snapshot and wrote the whole row back through upsertConfig() as two separate statements, which is a lost-update window
• Concretely: request A rotates a secret S0 → S1 and commits inside that window, then request B restores the stale S0 from its own earlier snapshot and silently reverts the rotation — a credential rotation that the operator saw succeed
• This pull request makes the read-modify-write one atomic unit, serialised per (plugin, company) by a Postgres advisory lock held to commit
• The benefit is that a stale masked response can no longer revert a newer rotation, and secret-ref bindings now commit with the config version that actually wins

Linked Issues or Issue Description

• Closes BLO-26529 — Make masked plugin-config round-trip concurrency-safe
• Refs BLO-20794 — parent: GET /api/plugins/:pluginId/config returns secrets unmasked
• Follow-up to fix(plugins): prevent secret-ref diagnostic reflection (BLO-20871) #1221, which introduced the mask/restore round trip this hardens
• Related sibling fix(plugins): close plugin-config masking gaps for duplicate identities and conditional schemas (BLO-26530) #1339 (BLO-26530), which closed the masking gaps for duplicate identities and conditional schemas

What Changed

• POST /api/plugins/:pluginId/config now runs the mask-restore read, schema validation, secret-ref sync and upsertConfig() inside a single db.transaction.
• The transaction's first statement takes pg_advisory_xact_lock(hashtextextended('paperclip:plugin-config:<pluginId>:<companyId>', 0)), serialising concurrent saves per plugin+company.
• Advisory lock rather than SELECT ... FOR UPDATE: plugin_config has no row before the first save, and a row lock on a row that is not there serialises nothing.
• Secret-ref binding sync (syncSecretRefsForTarget) moved inside the transaction, so bindings commit and roll back with the config version that wins.
• The worker configChanged RPC deliberately stays outside the transaction — it is a cross-process round trip, and holding the config lock across it would stall every other admin save for an RPC timeout.
• New deterministic regression test server/src/__tests__/plugin-config-write-race.test.ts.
• The shared fake db in plugin-routes-authz.test.ts gained a transaction implementation; it had none, so 10 tests hit db.transaction is not a function. No assertion was weakened.

Chose the lock over an ETag/version CAS — the acceptance criteria allow either — because it needs no API change, covers the CLI and every other client, and merges an unrelated concurrent edit instead of 409ing it back at the operator.

Verification

server/src/__tests__/plugin-config-write-race.test.ts drives the interleaving deterministically. Its fake db models the two Postgres properties the fix relies on: transactions run concurrently unless something serialises them, and an advisory lock is a per-key mutex held until commit. Ordering is driven by explicit aCommitted / aBlockedOnLock events rather than timers, so neither branch can hang or flake.

Mutation-verified. Deleting only the tx.execute(... pg_advisory_xact_lock ...) statement fails the suite with the exact symptom this issue reports:

× does not revert a rotation committed while a masked save was in flight
  AssertionError: expected 'sentinel-old-bearer-S0' to be 'sentinel-new-bearer-S1'
× serialises concurrent saves on the config row rather than interleaving them
  AssertionError: expected false to be true

Restoring the statement makes it green again. Re-run on the current rebased head (not just the original commit):

• plugin-config-write-race + plugin-routes-authz: 89 passed
• All 33 plugin-* suites: 433 passed
• pnpm --filter @paperclipai/server typecheck: clean

Review continuity across the rebase. Ally reviewed head 3d1d0220 and returned clean — 0 Critical, 0 Important, 0 Suggestions — recommending only that the PR-description policy check be addressed before merge, which is what this update does. This head is that same commit rebased onto master (it was 56 behind, and #1339 had since landed in the same handler). The rebase was textually clean and the resulting patch is content-identical to the reviewed one — diff of the two patches, ignoring hunk offsets, is empty — so the rebase carries no unreviewed logic. Sibling #1339's mergeMaskedPluginConfig / unresolvedMaskPaths work now sits correctly inside the advisory-locked transaction, and the full plugin suite and the mutation check were both re-run on the rebased head, not just the original commit.

Risks

• Lock scope — the advisory lock is keyed per (pluginId, companyId), so it serialises only concurrent saves of the same plugin config; saves to different plugins or companies are unaffected. The critical section holds no network I/O, and the configChanged RPC is explicitly outside it, so the lock cannot be held across an RPC timeout.
• No migration, no schema change, no API change. Request and response shapes are unchanged, so no client needs updating and there is nothing to roll back beyond the code itself.
• Behavioural shift — a masked save concurrent with a rotation now merges into the rotation rather than reverting it. That is the intended fix, but it does mean the value written is not always the one the caller's snapshot showed.
• Known residual, deliberately out of scope — two concurrent saves still last-write-wins on non-secret fields (B's url edit overwrites A's, as before). That is ordinary LWW on a whole-row replace API, not a credential reversion; closing it needs the field-level merge or ETag this issue explicitly scoped out. Say the word and I will file it.

Model Used

Anthropic Claude Opus, model id claude-opus-5[1m] (1M context window), running in Claude Code with extended thinking, tool use and code execution enabled. The original fix commit and this rebase/verification pass were both produced by the Paperclip CTO agent on that model.

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 searched the GitHub PR list (open + closed) for similar or duplicate PRs and linked the related ones 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, server-only change
• I have considered and documented any risks above
• All Paperclip CI gates are green

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-20794
🔗 Paperclip issue: BLO-26529

[allyblockcast]

@ally please review at head 3d1d0220ec7fac413120b709d0f63e5627913237 — this closes a lost-update race in the plugin-config write path (BLO-26529, follow-up to #1221). Specific things I'd like challenged:

1. Lock choice and key. pg_advisory_xact_lock on paperclip:plugin-config:<pluginId>:<companyId> rather than SELECT ... FOR UPDATE, because plugin_config has no row before the first save. Is the key granularity right, and does taking it as the transaction's first statement leave any deadlock ordering hazard against the row locks syncSecretRefsForTarget takes later in the same transaction?

2. Empty commit on the reject paths. The unresolvedMaskPaths and schema-validation branches now return { ok: false } from inside the transaction, which commits an empty transaction rather than rolling back. I believe that is harmless (nothing was written, lock released at commit) — but confirm I haven't made a rejected save observable somewhere.

3. Worker RPC moved outside the transaction. configChanged is now post-commit so the lock isn't held across a cross-process round trip. Is anything relying on it happening before the response, or before the lock drops?

4. Is the test's DB model faithful enough to be load-bearing? plugin-config-write-race.test.ts fakes db.transaction so that transactions run concurrently unless the advisory lock serialises them. If that model is wrong, the mutation evidence below is worthless — so please attack it directly. Deleting only the tx.execute(... pg_advisory_xact_lock ...) line fails the suite with expected 'sentinel-old-bearer-S0' to be 'sentinel-new-bearer-S1', i.e. the exact reported symptom.

5. Residual I left open on purpose. Concurrent saves still last-write-wins on non-secret fields. I read that as ordinary LWW on a whole-row replace API rather than a credential reversion, and out of scope here. Push back if you think it belongs in this PR.

Note mergeStateStatus currently reads BEHIND; master runs a merge queue that rebases on entry, so I'm not update-branching to inspect it and will re-read the gate after review.

[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 3d1d0220ec7fac413120b709d0f63e5627913237.

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.

[allyblockcast]

@ally please review at head 3d1d022 — plugins: masked config round-trip concurrency safety. Focus on lost-update windows between read, mask, and write-back.

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: 3d1d022

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The config-scoped advisory transaction lock covers the masked read/modify/write path, including secret-reference synchronization, and the lock key is scoped to the plugin/company pair.
• Worker notification remains post-commit, avoiding a cross-process RPC while the database lock is held.
• The deterministic race tests model lock contention explicitly and verify that a stale masked save cannot restore an older secret.

Recommended Action

1. No Critical or Important code issues found; this review is clean.
2. Consider addressing the separate PR-description policy check before merge.