Skip to content

fix(channels): serialize channel credential writes and roll back partials - #3657

Draft
cpacker wants to merge 1 commit into
fix/secure-token-cachefrom
fix/channel-secret-serialization
Draft

fix(channels): serialize channel credential writes and roll back partials#3657
cpacker wants to merge 1 commit into
fix/secure-token-cachefrom
fix/channel-secret-serialization

Conversation

@cpacker

@cpacker cpacker commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Stacked on #3656 — review #3655 and #3656 first. This PR is 3 of 3 splitting #3423.

This is the part of #3423 least related to its stated purpose: it fixes concurrent-writer clobbering of channel account files, not runtime credential availability. Separating it means a revert of the auth work does not also revert this.

Problem

Channel account secrets were persisted by pushing writes onto a module-level queue and redacting the account file without waiting for them. Two problems followed:

  1. The account file is shared by every account in a channel, so concurrent commands could clobber each other.
  2. A multi-field credential (e.g. Slack's botToken + appToken) could be left half-written with no way back.

Approach

Serialize the full keyring + in-memory + file commit per channel, and await writes before redacting rather than after. Separate processes remain last-writer-wins because OS keyrings offer no conditional writes.

For multi-field credentials, capture the previous values first and restore them if a later field fails, surfacing an AggregateError when the rollback itself also fails.

removeChannelAccountWithSecrets no longer hydrates before deleting. getSecretFieldPaths is schema-driven, so the field list is available without reading the secrets back — which avoids pointless keyring access (and potential approval prompts) on removal.

flushPendingChannelSecretWrites is now a no-op, kept for callers that still reference it.

Rebase note

accounts.ts diverged on main after #3423 was cut — #3650 (WhatsApp messaging controls) added messagePrefix handling to it. Taking #3423's version of the file wholesale drops those four sites and breaks 4 WhatsApp service tests. This PR applies the secret-serialization changes as a 3-way merge onto current main instead, keeping both.

#3423 itself needs a rebase before it merges anywhere for the same reason.

Reviewer note

runSerializedChannelSecretOperation is not reentrant. Today no locked body calls back into hydrateChannelAccountSecrets or getChannelAccountWithSecrets, so it is correct — but a future caller adding one will self-deadlock with no timeout and no error. A dev-mode "already holding the lock for this channel" assertion would turn that into a loud failure; not included here to keep the diff focused.

Validation

  • bun run check — 12/12 pass
  • bun test src/channels/ — 1,053 pass, 0 fail (includes the 4 WhatsApp tests the naive lift broke)
  • Full unit suite — 5,935 pass, 0 fail

🤖 Generated with Claude Code

…ials

Channel account secrets were persisted by pushing writes onto a module-level
queue and redacting the account file without waiting for them. Two problems
followed: the account file is shared by every account in a channel, so
concurrent commands could clobber each other, and a multi-field credential
could be left half-written with no way back.

Serialize the full keyring + in-memory + file commit per channel, and await
writes before redacting rather than after. For multi-field credentials, capture
the previous values first and restore them if a later field fails, surfacing an
AggregateError when the rollback itself fails. Separate processes remain
last-writer-wins because OS keyrings offer no conditional writes.

removeChannelAccountWithSecrets no longer hydrates before deleting.
getSecretFieldPaths is schema-driven, so the field list is available without
reading the secrets back, which avoids pointless keyring access on removal.

flushPendingChannelSecretWrites is now a no-op kept for callers that still
reference it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@cpacker
cpacker marked this pull request as draft August 5, 2026 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants