Feature: add cross-process locking around keyring writes

[alexminza]

What

Add cross-process serialization around keyring writes in gog auth import, gog auth add, and any other path that mutates the secrets.Store keyring. Two concurrent invocations can race on the underlying 99designs/keyring file backend, with no in-process guard and no advisory file lock — last writer wins, mid-write read can observe a partial state.

(gog auth credentials set and gog auth service-account set write JSON to $XDG_CONFIG_HOME/gogcli/ via config.WriteFileAtomic, not through the keyring; they have a similar concurrency concern in a different layer — out of scope for this issue, see § "Out of scope".)

Why

Today's storage shape

gogcli persists OAuth secrets via the secrets.Store interface, which wraps 99designs/keyring. On Linux containers without dbus the backend is keyring.FileBackend (one file per key under $XDG_DATA_HOME/gogcli/keyring/, AES-256-GCM-encrypted with the GOG_KEYRING_PASSWORD-derived key).

The library does not provide any concurrency control: 99designs/keyring's fileKeyring.Set is plain os.WriteFile(filename, ..., 0600) — no advisory lock, no tmpfile + rename, no in-process mutex on the Keyring struct (verified by grep sync.Mutex and grep flock returning 0 hits across the library). If gogcli wants concurrent-safe keyring mutations, it has to provide the lock itself. (mcporter took the same approach — its withFileLock lives in mcporter, not in any library dependency.)

Read/write call sites:

• store.SetToken(client, email, tok) — written by auth_import.go, auth_add.go, and (transparently, on rotation) persistingTokenSource in client_auth.go.
• store.DeleteToken(...) — called from removal flows in auth_tokens.go, auth_credentials.go, auth_accounts.go, and (transparently, on identity migration) googleauth/identity_migration.go.

In addition, on-disk config / credentials writes via WriteFileAtomic (internal/config/config.go) use the standard same-directory tempfile + rename pattern. That's atomic at the OS level for a single file, but it doesn't serialize multi-step read-modify-write sequences.

The gap

Searching the entire repo for concurrency primitives around the secret-store / keyring write paths:

• grep -i "sync\.Mutex" internal/secrets/store.go → 0 hits
• grep -i "flock" internal/secrets/ → 0 hits
• grep -i "lockfile\|withFileLock\|advisory" internal/secrets/ → 0 hits

There is no in-process mutex, no advisory file lock, and no application-level read-modify-write guard around the keyring entry namespace. Two concurrent gog auth import invocations (or one auth import racing the SDK-driven persistingTokenSource rotation) can step on each other.

Concrete failure modes

1. Lost refresh-token rotation. Two processes each refresh; the second SetToken clobbers the first's rotated refresh token. The first process is left holding a token that's no longer accepted by Google.
2. Mixed identity state. auth import writes {client, email, refresh_token} while a concurrent persistingTokenSource is updating {subject, scopes, created_at} from a recent refresh response. The keyring entry can end up with the wrong combination.
3. Read-side observation of partial writes. The file backend uses os.WriteFile (O_WRONLY|O_CREATE|O_TRUNC) for each entry, not a tmpfile-rename — a concurrent reader can observe a truncated or partially-written file mid-Set, not just stale content. The risk compounds across keys when one logical operation writes multiple entries (e.g. SetToken's primary + legacy + subject-alias triple).

Precedent in a sibling project

openclaw/mcporter had the same gap, surfaced and resolved by @steipete in openclaw/mcporter#167 "Serialize concurrent file writes" (closed 2026-05-14, fix shipped in v0.11.0 via commit 23565e2 "fix: harden concurrent config writes"). The discussion in #167 inventoried every non-atomic write site (OAuth vault, OAuth persistence, schema cache, daemon metadata) and proposed the same class of fix this issue proposes for gogcli: application-owned serialization around read-modify-write sequences, with atomic writes where the application owns the file write path. (For gogcli's keyring path, the file write happens inside 99designs/keyring's os.WriteFile, so the proposal here is locks only; atomic writes on the parallel config.WriteFileAtomic path already exist.)

The implementation mcporter landed wraps every OAuth vault mutation in withFileLock (src/fs-json.ts), invoked from src/oauth-vault.ts:

export async function saveVaultEntry(definition, patch) {
  await withFileLock(getOAuthVaultPath(), async () => {
    const vault = await readVault();
    // ... mutate ...
    await writeJsonFile(getOAuthVaultPath(), vault);
  });
}

Every read-modify-write goes through the same lock. gogcli's keyring write paths are structurally equivalent — same concurrency concern, no equivalent guard — and the design choices @steipete made in mcporter#167 transfer directly (per-file lock granularity, project-owned @openclaw/fs-safe-style helper, regression tests for concurrent-write / stale-lock / symlink edge cases).

Why the threat model matters for gogcli specifically

The clobber failure mode is documented (and worked around) in third-party deployment patterns: any orchestrator that re-fires credential setup (e.g. on user re-authorization) must avoid re-firing concurrently with an SDK-driven persistingTokenSource rotation. Today that's enforced by convention ("don't fire setup while the agent is making API calls"); a real lock would make it structurally impossible.

Suggested shape

Wrap the keyring read-modify-write sequences in an advisory file lock at the secrets.Store boundary. One lockfile per gogcli config dir (e.g. $XDG_DATA_HOME/gogcli/keyring/.lock) — shared across all keys, since cross-key consistency matters for auth import writing both Token and account-default state.

Implementation sketch

• Add internal/secrets/lock.go exposing a withKeyringLock(fn func() error) error helper backed by golang.org/x/sys/unix.Flock on Unix and a corresponding LockFile API on Windows. Both available in the standard ecosystem; no new external deps.
• Lockfile path: filepath.Join(KeyringDir(), ".lock"). Created with O_CREATE|O_RDWR, mode 0600 (matches the rest of the keyring dir).
• Wrap every public mutation method on KeyringStore (SetToken, DeleteToken, SetDefaultAccount, plus the post-issue counterparts for client_secret storage in Feature: store Google OAuth client_secret in the keyring (parity with the Zoom path) #596 if/when that lands) with an exclusive lock around the full read-modify-write sequence. SetToken in particular reads any existing entry before writing (to detect Subject change and migrate the subject-key alias) — locking just the write half would leave that read racey.
• Wrap every read method (GetToken, ListTokens, Keys, GetDefaultAccount) with a shared lock. Single-key reads need protection too — 99designs/keyring's file backend writes via os.WriteFile (O_TRUNC), not a tmpfile-rename, so a concurrent reader can observe a truncated file mid-write. The shared-lock cost is small (multiple readers don't block each other) and closes the partial-read window without serializing reads against one another.
• Locks are advisory and process-cooperative — matches mcporter's withFileLock design.

Timeout / contention

Block by default; abort with a clear error if the lock can't be acquired in N seconds (proposed: 5s, configurable via GOG_KEYRING_LOCK_TIMEOUT). Same shape as mcporter's withFileLock.

Scope

• internal/secrets/store.go and the surrounding wrapper types (KeyringStore, timeoutKeyring).
• All keyring write callers in internal/cmd/auth_*.go and internal/googleapi/client_auth.go's persistingTokenSource (the SDK-driven rotation path).
• Public API surface unchanged — the lock lives behind secrets.Store.

Out of scope

• Locking for non-keyring config-file writes. gog auth credentials set writes $XDG_CONFIG_HOME/gogcli/credentials*.json; gog auth service-account set writes $XDG_CONFIG_HOME/gogcli/service-accounts/<email>.json; account-clients map / client-domains map mutations write config.json. All go through config.WriteFileAtomic (internal/config/config.go) — atomic per-file, but no cross-file or multi-step read-modify-write guard. Same concurrency concern, different layer. Worth a separate issue once the keyring-side lands; mcporter#167 ended up doing both, but starting narrow keeps this issue tractable. (Note: auth accounts set-default is a keyring write, not a config-file write — KeyringStore.SetDefaultAccount stores defaultAccountKey / defaultAccountKeyForClient entries; the implementation sketch above correctly locks it.)
• macOS Keychain backend semantics. On macOS the OS keychain handles concurrency itself; this proposal targets the file backend where gogcli holds the responsibility.
• Cross-host serialization. Multi-host concurrent writes to a shared keyring dir would be a separate (and weirder) deployment model.

Context

• No existing issue or PR proposes this — searched for keyring lock, secrets race, concurrent auth import, flock keyring; discussions are disabled on this repo. Filing as a clean new ask.
• Related but distinct from #596 (move client_secret to keyring) — that issue increases what's stored in the keyring; this issue makes the storage itself concurrent-safe. The two should land in order: locking first (foundation), client_secret-in-keyring second (consumer of the lock).

References

All references pinned to v0.17.0 (gogcli) and main (mcporter, where pinned tags aren't published):

• internal/secrets/store.go — KeyringStore, Store interface, current write paths (no locking).
• internal/cmd/auth_import.go — one of the concurrent-write call sites.
• internal/cmd/auth_add.go — another concurrent-write call site.
• internal/googleapi/client_auth.go — persistingTokenSource rotation path that races user-initiated writes.
• internal/config/config.go — WriteFileAtomic (single-file atomicity, doesn't cover multi-step sequences).
• openclaw/mcporter#167 — "Serialize concurrent file writes" — the upstream discussion that surfaced and resolved the same gap in the sibling project (closed 2026-05-14).
• openclaw/mcporter — commit 23565e2 "fix: harden concurrent config writes" — the closing commit that landed atomic writes + per-file locks; shipped in v0.11.0.
• openclaw/mcporter — src/oauth-vault.ts — the implementation: every vault mutation wrapped in withFileLock.
• openclaw/mcporter — src/fs-json.ts — withFileLock and atomic-write helpers.

[tokyo-s]

This is especially important for agent and automation use cases. Keyring writes should be protected when multiple processes may touch auth state.