credhelper: make config writes non-destructive

Follow-up to 8a556a5. v3 keys accounts by DID and, unlike v2, rewrites the
config during ordinary `docker pull` traffic (migration on first read, DID
backfill after a successful get) rather than only on explicit login. That moved
three latent ways to lose a device.json onto the hot path. A user cannot recover
from any of them: once the file is gone they no longer know which accounts they
had.

save() is now atomic. It writes a temp file in the same directory, fsyncs, and
renames, so the config always holds either the previous contents or the complete
new contents. The truncate-then-write it replaced left a zero-length window that
Ctrl-C, Docker reaping the helper, or a suspend could land in. The fsync matters
on its own: on ext4/XFS a rename can become durable while the new file's data
blocks are not, which resurrects the empty-file case. The rename also replaces
the destination's mode, which incidentally repairs a config restored from a
backup as world-readable (os.WriteFile's perm argument only applies at creation).

An unreadable config no longer degrades into an empty one that the next write
commits. loadConfig returns a usable empty config so read paths can still print
something helpful, but it now marks the unrecoverable cases with
errConfigUnusable, and loadConfigForWrite refuses on that sentinel. This is the
downgrade path: the previous binary hard-gates on Version == 2, so without the
guard a reinstall of an older helper would wipe a v3 file on the first
`docker login`. loadConfig also stops explicitly on a version newer than it
understands instead of falling through the legacy probes to the same empty
config. All six write paths take the guard — get, store, erase, login, logout,
switch. login had the same warn-and-continue-then-save shape as store.

The guard keys on the sentinel rather than on any error, deliberately. The
v2 -> v3 migration returns a fully populated config alongside a "saving migrated
config" error when the directory is not writable, and every pre-v3 user passes
through that path on their next invocation; refusing there would hard-break
`docker pull` for exactly the population that is migrating.

migrateV2toV3 merges DID collisions deterministically. Two v2 entries collapse
onto one v3 key when a handle was renamed and the old entry was never cleaned up,
which is the case v3 exists to fix. The previous loop wrote both into the same
map slot in randomized iteration order, so which account survived varied run to
run, and when the stale one won, get would fail validation and remove it — the
user ended up with no account at all. Iteration is now sorted and collisions
resolve through v2EntryBeats (active entry, then the one holding a secret, then
the smaller key), with the loser donating its secret if the winner has none. The
"No account is dropped" comment was false and is now accurate.

Also deterministic: find() and upsert() resolve their scans through a shared
scanFor helper that prefers DID matches and breaks ties on the smallest key, so
two entries sharing a handle can no longer hand Docker a different secret on each
invocation. upsert additionally refuses to match a handle that already belongs to
a different DID, which previously let it overwrite an unrelated account's DID and
destroy that account's credentials. Nil map entries are tolerated throughout
rather than panicking on Docker's credential path.

resolveHandleDNS is bounded at 2s. It runs synchronously on every store and on
every get for a DID-less account, and net.LookupTXT applies no deadline of its
own, so a blackholing resolver (captive portal, split-horizon VPN) stalled
docker login and docker pull on each invocation.

Tests cover the paths that can lose credentials. Each was checked against the
pre-fix code: the collision, null-entry, DID-theft, find-determinism,
permissions, and out-of-place-write tests all fail or panic without their fix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

pkg/credhelper/cmd_login.go

@@ -29,9 +29,11 @@ func runLogin(cmd *cobra.Command, args []string) error {
 
 	appViewURL := buildAppViewURL(serverURL)
 
-	sc, err := loadConfig()
+	// Login always saves, so a config we could not parse must stop us here:
+	// continuing would write the empty fallback config over the user's accounts.
+	sc, err := loadConfigForWrite()
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "Warning: config load error: %v\n", err)
+		return err
 	}
 
 	// Check if already logged in

pkg/credhelper/cmd_logout.go

@@ -27,9 +27,9 @@ func runLogout(cmd *cobra.Command, args []string) error {
 
 	appViewURL := buildAppViewURL(serverURL)
 
-	sc, err := loadConfig()
+	sc, err := loadConfigForWrite()
 	if err != nil {
-		return fmt.Errorf("loading config: %w", err)
+		return err
 	}
 
 	reg := sc.findRegistry(appViewURL)

pkg/credhelper/cmd_switch.go

@@ -27,9 +27,9 @@ func runSwitch(cmd *cobra.Command, args []string) error {
 
 	appViewURL := buildAppViewURL(serverURL)
 
-	sc, err := loadConfig()
+	sc, err := loadConfigForWrite()
 	if err != nil {
-		return fmt.Errorf("loading config: %w", err)
+		return err
 	}
 
 	reg := sc.findRegistry(appViewURL)

pkg/credhelper/config.go

@@ -2,11 +2,22 @@ package credhelper
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"os"
+	"path/filepath"
+	"sort"
 	"time"
 )
 
+// errConfigUnusable marks a config file that exists on disk but could not be
+// understood — a newer on-disk version, malformed JSON, a truncated file, or an
+// unreadable one. Callers that intend to write MUST refuse when they see it
+// (see [loadConfigForWrite]): loadConfig hands back an empty config so read
+// paths can still print a useful message, and saving that empty config over a
+// real one would destroy every stored account.
+var errConfigUnusable = errors.New("credential config is unusable")
+
 // configVersion is the current on-disk format version. v3 keys the accounts
 // map (and Active) by DID, the stable identity; v2 keyed them by handle, which
 // broke on handle renames. The struct shape is identical between v2 and v3 —
@@ -63,22 +74,31 @@ func loadConfig() (*StoredConfig, error) {
 		if os.IsNotExist(err) {
 			return newStoredConfig(), nil
 		}
-		return newStoredConfig(), err
+		return newStoredConfig(), fmt.Errorf("%w: %v", errConfigUnusable, err)
 	}
 
 	// v3 (current) and v2 share the same struct shape — v2 keyed accounts by
 	// handle, v3 by DID. Unmarshal once, then branch on the version.
 	var sc StoredConfig
 	if err := json.Unmarshal(data, &sc); err == nil && sc.Registries != nil {
-		switch sc.Version {
-		case configVersion:
+		switch {
+		case sc.Version == configVersion:
 			return &sc, nil
-		case 2:
+		case sc.Version == 2:
 			migrated := migrateV2toV3(&sc)
 			if err := migrated.save(); err != nil {
 				return migrated, fmt.Errorf("saving migrated config: %w", err)
 			}
 			return migrated, nil
+		case sc.Version > configVersion:
+			// A newer build wrote this file. Stop here rather than falling
+			// through to the legacy probes: they would all miss, loadConfig
+			// would return an empty config, and the next save would overwrite
+			// every account. This is the downgrade/reinstall path, so it has to
+			// be non-destructive.
+			return newStoredConfig(), fmt.Errorf(
+				"%w: on-disk config is version %d but this build of %s understands version %d; upgrade %s to use it",
+				errConfigUnusable, sc.Version, cfg.BinaryName, configVersion, cfg.BinaryName)
 		}
 	}
 
@@ -139,7 +159,31 @@ func loadConfig() (*StoredConfig, error) {
 		return migrated, nil
 	}
 
-	return newStoredConfig(), fmt.Errorf("unrecognized config format")
+	return newStoredConfig(), fmt.Errorf("%w: unrecognized config format", errConfigUnusable)
+}
+
+// loadConfigForWrite loads the config for a caller that intends to save it back.
+// It refuses only on errConfigUnusable — the paths where loadConfig could not
+// understand the file and therefore handed back an *empty* config, which a save
+// would write over the user's real accounts.
+//
+// Other errors carry a fully populated config and must not block the caller. The
+// v2→v3 migration in particular returns its migrated config alongside a "saving
+// migrated config" error when the config directory is not writable (read-only
+// $HOME in a container, ENOSPC, restrictive mode). Every pre-v3 user passes
+// through that path on their next invocation, so failing it would hard-break
+// `docker pull` and `docker login` for exactly the population that is migrating.
+// The in-memory config is correct; the caller's own save will surface any
+// persistence failure on its own terms.
+func loadConfigForWrite() (*StoredConfig, error) {
+	sc, err := loadConfig()
+	if err != nil && errors.Is(err, errConfigUnusable) {
+		return nil, fmt.Errorf("refusing to modify stored credentials: %w", err)
+	}
+	if err != nil {
+		fmt.Fprintf(os.Stderr, "Warning: %v\n", err)
+	}
+	return sc, nil
 }
 
 func newStoredConfig() *StoredConfig {
@@ -149,20 +193,60 @@ func newStoredConfig() *StoredConfig {
 	}
 }
 
-// save writes the config to disk.
+// save writes the config to disk atomically.
+//
+// This file is the user's only copy of their device secrets, and since v3 it is
+// written during ordinary `docker pull`/`docker login` traffic (migration and
+// DID backfill), not just on explicit login. A truncate-then-write would leave a
+// zero-length or half-written file if the helper is interrupted mid-write —
+// Ctrl-C, Docker reaping the process, a laptop suspending — and the user cannot
+// recover from that because they no longer know which accounts they had.
 func (c *StoredConfig) save() error {
-	path := getConfigPath()
 	data, err := json.MarshalIndent(c, "", "  ")
 	if err != nil {
 		return err
 	}
-	return os.WriteFile(path, data, 0600)
+	return writeFileAtomic(getConfigPath(), data)
+}
+
+// writeFileAtomic writes data to path via a temp file in the same directory
+// followed by a rename, so path always holds either the previous contents or the
+// complete new contents. The rename also replaces the destination's mode with
+// the temp file's 0600, which repairs a config that was left group- or
+// world-readable by a restore from backup.
+func writeFileAtomic(path string, data []byte) error {
+	f, err := os.CreateTemp(filepath.Dir(path), ".device-*.json.tmp")
+	if err != nil {
+		return err
+	}
+	tmp := f.Name()
+	// No-op once the rename below succeeds; on any earlier return it removes
+	// the partial file rather than leaving litter next to the real config.
+	defer os.Remove(tmp) //nolint:errcheck
+
+	if _, err := f.Write(data); err != nil {
+		f.Close() //nolint:errcheck
+		return err
+	}
+	// Flush before renaming. On ext4/XFS a rename can become durable while the
+	// new file's data blocks have not, which would resurrect the very
+	// empty-file case this function exists to prevent.
+	if err := f.Sync(); err != nil {
+		f.Close() //nolint:errcheck
+		return err
+	}
+	if err := f.Close(); err != nil {
+		return err
+	}
+	return os.Rename(tmp, path)
 }
 
 // getOrCreateRegistry returns (or creates) a RegistryConfig for the given URL.
+// A present-but-null entry (possible in a hand-edited or corrupted file) is
+// replaced rather than returned, so callers never dereference nil.
 func (c *StoredConfig) getOrCreateRegistry(registryURL string) *RegistryConfig {
 	reg, ok := c.Registries[registryURL]
-	if !ok {
+	if !ok || reg == nil {
 		reg = &RegistryConfig{
 			Accounts: make(map[string]*Account),
 		}
@@ -176,28 +260,53 @@ func (c *StoredConfig) findRegistry(registryURL string) *RegistryConfig {
 	return c.Registries[registryURL]
 }
 
+// scanFor returns the matching account with the lexically smallest map key,
+// along with that key. Go randomizes map iteration order, so any scan that can
+// match more than one entry has to impose its own order — otherwise the helper
+// hands Docker a different secret from one invocation to the next. Nil entries
+// (hand-edited or corrupted files) are skipped.
+func (r *RegistryConfig) scanFor(match func(*Account) bool) (*Account, string) {
+	var best *Account
+	var bestKey string
+	for k, acct := range r.Accounts {
+		if acct == nil || !match(acct) {
+			continue
+		}
+		if best == nil || k < bestKey {
+			best, bestKey = acct, k
+		}
+	}
+	return best, bestKey
+}
+
 // find returns the account matching identity, which may be a DID or a handle.
-// It checks the map key first (DID or provisional handle key) then scans by
-// DID/Handle, so an image ref by handle resolves even when the map is
+// It checks the map key first (DID or provisional handle key), then scans by
+// DID, then by handle, so an image ref by handle resolves even when the map is
 // DID-keyed. Returns nil if no account matches.
+//
+// DID matches are tried before handle matches, and each scan is deterministic,
+// because two entries can legitimately share a handle: renaming a handle and
+// logging in again leaves the old DID-keyed entry behind with the same label.
 func (r *RegistryConfig) find(identity string) *Account {
 	if identity == "" {
 		return nil
 	}
-	if acct, ok := r.Accounts[identity]; ok {
+	if acct, ok := r.Accounts[identity]; ok && acct != nil {
 		return acct
 	}
-	for _, acct := range r.Accounts {
-		if acct.DID == identity || acct.Handle == identity {
-			return acct
-		}
+	if acct, _ := r.scanFor(func(a *Account) bool { return a.DID == identity }); acct != nil {
+		return acct
 	}
-	return nil
+	acct, _ := r.scanFor(func(a *Account) bool { return a.Handle == identity })
+	return acct
 }
 
 // activeAccount returns the active account, falling back to the sole account
 // when Active is unset or dangling and exactly one account exists.
 func (r *RegistryConfig) activeAccount() *Account {
+	if r == nil {
+		return nil
+	}
 	if acct := r.find(r.Active); acct != nil {
 		return acct
 	}
@@ -228,18 +337,29 @@ func (r *RegistryConfig) upsert(acct *Account) *Account {
 
 	var existing *Account
 	var existingKey string
+
+	// 1. Exact DID key.
 	if acct.DID != "" {
-		if e, ok := r.Accounts[acct.DID]; ok {
+		if e, ok := r.Accounts[acct.DID]; ok && e != nil {
 			existing, existingKey = e, acct.DID
 		}
 	}
+	// 2. Same DID stored under some other key (a provisional handle key that
+	//    has since been backfilled).
+	if existing == nil && acct.DID != "" {
+		existing, existingKey = r.scanFor(func(e *Account) bool { return e.DID == acct.DID })
+	}
+	// 3. Handle match — but never one that already belongs to a *different*
+	//    DID. Two entries can share a handle after a rename, and matching one
+	//    of those would let the DID assignment below overwrite another
+	//    identity's DID, destroying that account's credentials.
 	if existing == nil && acct.Handle != "" {
-		for k, e := range r.Accounts {
-			if e.Handle == acct.Handle || (acct.DID != "" && e.DID == acct.DID) {
-				existing, existingKey = e, k
-				break
+		existing, existingKey = r.scanFor(func(e *Account) bool {
+			if e.Handle != acct.Handle {
+				return false
 			}
-		}
+			return acct.DID == "" || e.DID == "" || e.DID == acct.DID
+		})
 	}
 
 	if existing == nil {
@@ -373,29 +493,100 @@ func (c *StoredConfig) removeAccount(registryURL, identity string) {
 // migrateV2toV3 re-keys a v2 (handle-keyed) config to v3 (DID-keyed) in place.
 // Accounts with a stored DID move under their DID key; DID-less accounts keep
 // the handle as a provisional key and self-heal on the next successful get.
-// No account is dropped.
+//
+// Two v2 entries can collapse onto one v3 key: renaming a handle and logging in
+// again leaves the old entry behind under the same DID, which is precisely the
+// case v3 exists to fix. That is a merge, not a drop — see [v2EntryBeats] for
+// the tie-break, and note the loser still donates its secret if the winner has
+// none. Both the merge and the Active re-point iterate in sorted key order, so
+// the outcome never depends on Go's randomized map iteration.
 func migrateV2toV3(sc *StoredConfig) *StoredConfig {
 	for _, reg := range sc.Registries {
+		if reg == nil {
+			continue
+		}
 		oldActive := reg.Active
 		newAccounts := make(map[string]*Account, len(reg.Accounts))
-		var activeAcct *Account
-		for oldKey, acct := range reg.Accounts {
+		wonWith := make(map[string]string, len(reg.Accounts)) // new key -> winning v2 key
+
+		for _, oldKey := range sortedKeys(reg.Accounts) {
+			acct := reg.Accounts[oldKey]
+			if acct == nil {
+				continue // tolerate a hand-edited or corrupted file
+			}
 			if acct.Handle == "" {
 				acct.Handle = oldKey // v2 map key was the handle
 			}
-			newAccounts[acct.key()] = acct
-			if oldKey == oldActive || acct.Handle == oldActive {
-				activeAcct = acct
+
+			newKey := acct.key()
+			incumbent, collision := newAccounts[newKey]
+			if !collision {
+				newAccounts[newKey], wonWith[newKey] = acct, oldKey
+				continue
+			}
+			if v2EntryBeats(acct, oldKey, incumbent, wonWith[newKey], oldActive) {
+				if acct.DeviceSecret == "" {
+					acct.DeviceSecret = incumbent.DeviceSecret
+				}
+				newAccounts[newKey], wonWith[newKey] = acct, oldKey
+			} else if incumbent.DeviceSecret == "" {
+				incumbent.DeviceSecret = acct.DeviceSecret
 			}
 		}
+
 		reg.Accounts = newAccounts
 		reg.Active = ""
-		reg.setActive(activeAcct)
+		// Prefer the entry whose winning v2 key was the active one; only fall
+		// back to a handle match if none did. Two entries can share a handle
+		// under different DIDs, and a handle match alone would then activate an
+		// arbitrary one of them.
+		for _, newKey := range sortedKeys(newAccounts) {
+			if wonWith[newKey] == oldActive {
+				reg.setActive(newAccounts[newKey])
+				break
+			}
+		}
+		if reg.Active == "" {
+			for _, newKey := range sortedKeys(newAccounts) {
+				if newAccounts[newKey].Handle == oldActive {
+					reg.setActive(newAccounts[newKey])
+					break
+				}
+			}
+		}
 	}
 	sc.Version = configVersion
 	return sc
 }
 
+// v2EntryBeats reports whether candidate should displace incumbent when both v2
+// entries re-key onto the same v3 key. Tie-breaks, in order: the entry v2 had
+// marked active, then the entry that actually carries a device secret, then the
+// lexically smaller v2 key. Every tier is a total order over the inputs, so the
+// winner is the same on every run.
+func v2EntryBeats(cand *Account, candKey string, inc *Account, incKey, oldActive string) bool {
+	candActive := oldActive != "" && (candKey == oldActive || cand.Handle == oldActive)
+	incActive := oldActive != "" && (incKey == oldActive || inc.Handle == oldActive)
+	if candActive != incActive {
+		return candActive
+	}
+	if candHasSecret, incHasSecret := cand.DeviceSecret != "", inc.DeviceSecret != ""; candHasSecret != incHasSecret {
+		return candHasSecret
+	}
+	return candKey < incKey
+}
+
+// sortedKeys returns m's keys in lexical order, for iteration that must be
+// reproducible across runs.
+func sortedKeys(m map[string]*Account) []string {
+	keys := make([]string, 0, len(m))
+	for k := range m {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)
+	return keys
+}
+
 // getUpdateCheckCachePath returns the path to the update check cache file
 func getUpdateCheckCachePath() string {
 	homeDir, err := os.UserHomeDir()

pkg/credhelper/config_safety_test.go

@@ -0,0 +1,407 @@
+package credhelper
+
+import (
+	"encoding/json"
+	"errors"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// Regression tests for the credential-destroying paths in the v3 config format.
+// Each of these could silently empty a user's device.json, which is unrecoverable
+// — they cannot know which accounts they had.
+
+// readConfigFile returns the raw bytes currently on disk at the config path.
+func readConfigFile(t *testing.T, dir string) []byte {
+	t.Helper()
+	data, err := os.ReadFile(filepath.Join(dir, "device.json"))
+	if err != nil {
+		t.Fatalf("reading config: %v", err)
+	}
+	return data
+}
+
+// TestSaveIsAtomic verifies save() actually writes out of place: the destination
+// inode must be REPLACED, not modified. That is the property that makes an
+// interrupted write harmless, and it is what distinguishes this from
+// os.WriteFile's truncate-then-write (which would keep the same inode and expose
+// a zero-length window).
+func TestSaveIsAtomic(t *testing.T) {
+	dir := setupConfigDir(t)
+	path := filepath.Join(dir, "device.json")
+	writeConfigFile(t, dir, `{"version":3,"registries":{}}`)
+
+	before, err := os.Stat(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	sc := newStoredConfig()
+	sc.addAccount("https://atcr.io", &Account{
+		Handle: "evan.jarrett.net", DID: "did:plc:evan", DeviceSecret: "atcr_device_A",
+	})
+	if err := sc.save(); err != nil {
+		t.Fatalf("save: %v", err)
+	}
+
+	after, err := os.Stat(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if os.SameFile(before, after) {
+		t.Error("save() wrote in place (same file); an interrupted write could truncate the config")
+	}
+
+	// The rename must not leave the temp file behind.
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, e := range entries {
+		if strings.HasPrefix(e.Name(), ".device-") {
+			t.Errorf("temp file %q left behind after save", e.Name())
+		}
+	}
+}
+
+// TestWriteFileAtomicPreservesOnFailure verifies a failed write leaves the
+// previous contents completely intact and drops no temp file. Failure is forced
+// by pointing at a path whose parent is not a directory, so os.CreateTemp fails.
+func TestWriteFileAtomicPreservesOnFailure(t *testing.T) {
+	dir := setupConfigDir(t)
+	path := filepath.Join(dir, "device.json")
+	const original = `{"version":3,"registries":{"https://atcr.io":{"active":"did:plc:evan","accounts":{"did:plc:evan":{"handle":"evan.jarrett.net","did":"did:plc:evan","device_secret":"atcr_device_A"}}}}}`
+	writeConfigFile(t, dir, original)
+
+	// A temp file cannot be created inside a regular file.
+	if err := writeFileAtomic(filepath.Join(path, "nested.json"), []byte("x")); err == nil {
+		t.Fatal("writeFileAtomic succeeded against an invalid directory")
+	}
+
+	if got := string(readConfigFile(t, dir)); got != original {
+		t.Errorf("config changed after a failed write:\n got %q\nwant %q", got, original)
+	}
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, e := range entries {
+		if strings.HasPrefix(e.Name(), ".device-") {
+			t.Errorf("temp file %q left behind after a failed write", e.Name())
+		}
+	}
+}
+
+// TestMigrationSaveFailureStillYieldsUsableConfig covers the population that
+// matters most right now: a pre-v3 user whose config directory is not writable.
+// The migration cannot persist, but the in-memory config is complete, so the
+// helper must keep working rather than refusing every operation.
+func TestMigrationSaveFailureStillYieldsUsableConfig(t *testing.T) {
+	if os.Geteuid() == 0 {
+		t.Skip("running as root; mode bits do not restrict writes")
+	}
+	dir := setupConfigDir(t)
+	writeConfigFile(t, dir, `{
+	  "version": 2,
+	  "registries": {
+	    "https://atcr.io": {
+	      "active": "evan.jarrett.net",
+	      "accounts": {"evan.jarrett.net": {"handle": "evan.jarrett.net", "did": "did:plc:evan", "device_secret": "atcr_device_A"}}
+	    }
+	  }
+	}`)
+	// Make the directory read-only so the migration's save fails.
+	if err := os.Chmod(dir, 0500); err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { _ = os.Chmod(dir, 0700) })
+
+	sc, err := loadConfigForWrite()
+	if err != nil {
+		t.Fatalf("loadConfigForWrite refused a config it could read: %v", err)
+	}
+	if sc == nil {
+		t.Fatal("loadConfigForWrite returned nil config")
+	}
+	acct := sc.Registries["https://atcr.io"].Accounts["did:plc:evan"]
+	if acct == nil || acct.DeviceSecret != "atcr_device_A" {
+		t.Fatalf("migrated account missing or wrong: %+v", acct)
+	}
+}
+
+// TestSaveTightensLoosePermissions covers a device.json restored from a tarball
+// or backup with a wide mode. os.WriteFile would have kept 0644 (its perm
+// argument only applies at creation); the atomic rename replaces it.
+func TestSaveTightensLoosePermissions(t *testing.T) {
+	dir := setupConfigDir(t)
+	path := filepath.Join(dir, "device.json")
+	if err := os.WriteFile(path, []byte(`{"version":3,"registries":{}}`), 0644); err != nil {
+		t.Fatal(err)
+	}
+
+	sc := newStoredConfig()
+	if err := sc.save(); err != nil {
+		t.Fatalf("save: %v", err)
+	}
+
+	info, err := os.Stat(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got := info.Mode().Perm(); got != 0600 {
+		t.Errorf("config mode = %o, want 0600 after rewrite", got)
+	}
+}
+
+// TestNewerVersionIsNotWiped is the downgrade path: an older build cannot parse
+// a newer on-disk version. It must refuse to write rather than saving an empty
+// config over the user's accounts.
+func TestNewerVersionIsNotWiped(t *testing.T) {
+	dir := setupConfigDir(t)
+	original := `{
+	  "version": 99,
+	  "registries": {
+	    "https://atcr.io": {
+	      "active": "did:plc:evan",
+	      "accounts": {
+	        "did:plc:evan": {"handle": "evan.jarrett.net", "did": "did:plc:evan", "device_secret": "atcr_device_A"},
+	        "did:plc:bob": {"handle": "bob.example", "did": "did:plc:bob", "device_secret": "atcr_device_B"}
+	      }
+	    }
+	  }
+	}`
+	writeConfigFile(t, dir, original)
+
+	if _, err := loadConfigForWrite(); err == nil {
+		t.Fatal("loadConfigForWrite accepted a newer-version config; it must refuse so callers don't overwrite it")
+	}
+
+	// loadConfig itself still yields a usable empty config for read-only
+	// callers, but reports the error that makes writers back off.
+	sc, err := loadConfig()
+	if err == nil {
+		t.Fatal("loadConfig should report an error for an unsupported version")
+	}
+	if sc == nil {
+		t.Fatal("loadConfig must still return a usable config for read paths")
+	}
+	// The sentinel is what loadConfigForWrite gates on, so it must be present.
+	if !errors.Is(err, errConfigUnusable) {
+		t.Errorf("error does not wrap errConfigUnusable: %v", err)
+	}
+	// The dedicated version branch must produce an actionable message rather
+	// than falling through to the generic "unrecognized config format".
+	if !strings.Contains(err.Error(), "version 99") {
+		t.Errorf("error should name the on-disk version so the user knows to upgrade, got: %v", err)
+	}
+
+	// Nothing may have touched the file.
+	var before, after any
+	if err := json.Unmarshal([]byte(original), &before); err != nil {
+		t.Fatal(err)
+	}
+	if err := json.Unmarshal(readConfigFile(t, dir), &after); err != nil {
+		t.Fatalf("config on disk is no longer valid JSON: %v", err)
+	}
+	if len(readConfigFile(t, dir)) == 0 {
+		t.Fatal("config file was emptied")
+	}
+}
+
+// TestMalformedConfigIsNotWiped covers truncated or corrupt JSON — the state a
+// non-atomic write used to be able to produce.
+func TestMalformedConfigIsNotWiped(t *testing.T) {
+	for name, contents := range map[string]string{
+		"truncated": `{"version":3,"registries":{"https://atcr.io":{"acti`,
+		"empty":     ``,
+		"notJSON":   `this is not json`,
+	} {
+		t.Run(name, func(t *testing.T) {
+			dir := setupConfigDir(t)
+			writeConfigFile(t, dir, contents)
+
+			_, err := loadConfigForWrite()
+			if err == nil {
+				t.Fatal("loadConfigForWrite accepted an unparseable config; writers would overwrite it")
+			}
+			// Gating is by sentinel, not by "any error" — see
+			// TestMigrationSaveFailureStillYieldsUsableConfig for the case that
+			// must NOT be refused.
+			if !errors.Is(err, errConfigUnusable) {
+				t.Errorf("error does not wrap errConfigUnusable, so the guard would let it through: %v", err)
+			}
+			if got := string(readConfigFile(t, dir)); got != contents {
+				t.Errorf("config was modified: got %q, want %q", got, contents)
+			}
+		})
+	}
+}
+
+// TestMigrateV2toV3_DIDCollisionIsDeterministic covers two v2 handle-keyed
+// entries that collapse onto one v3 DID key — the handle-rename case v3 exists
+// to fix. The winner must be the same on every run (Go randomizes map
+// iteration), and the surviving account must keep a usable secret.
+func TestMigrateV2toV3_DIDCollisionIsDeterministic(t *testing.T) {
+	const v2 = `{
+	  "version": 2,
+	  "registries": {
+	    "https://atcr.io": {
+	      "active": "alice.com",
+	      "accounts": {
+	        "alice.bsky.social": {"handle": "alice.bsky.social", "did": "did:plc:alice", "device_secret": "atcr_device_STALE"},
+	        "alice.com": {"handle": "alice.com", "did": "did:plc:alice", "device_secret": "atcr_device_CURRENT"}
+	      }
+	    }
+	  }
+	}`
+
+	// Repeat: a single pass can pass by luck when the bug is map-order dependent.
+	for i := 0; i < 50; i++ {
+		dir := setupConfigDir(t)
+		writeConfigFile(t, dir, v2)
+
+		sc, err := loadConfig()
+		if err != nil {
+			t.Fatalf("loadConfig: %v", err)
+		}
+		reg := sc.Registries["https://atcr.io"]
+		if reg == nil {
+			t.Fatal("registry missing after migration")
+		}
+		if len(reg.Accounts) != 1 {
+			t.Fatalf("accounts = %d, want 1 (both entries share a DID)", len(reg.Accounts))
+		}
+
+		acct := reg.Accounts["did:plc:alice"]
+		if acct == nil {
+			t.Fatal("merged account not keyed by DID")
+		}
+		// The v2 active entry wins, so the current secret survives, not the stale one.
+		if acct.DeviceSecret != "atcr_device_CURRENT" {
+			t.Fatalf("iteration %d: secret = %q, want atcr_device_CURRENT (active entry must win)", i, acct.DeviceSecret)
+		}
+		if acct.Handle != "alice.com" {
+			t.Fatalf("iteration %d: handle = %q, want alice.com", i, acct.Handle)
+		}
+		if reg.Active != "did:plc:alice" {
+			t.Fatalf("iteration %d: active = %q, want did:plc:alice", i, reg.Active)
+		}
+	}
+}
+
+// TestMigrateV2toV3_CollisionDonatesSecret verifies the losing entry still
+// donates its secret when the winner has none, so a merge never produces an
+// account with no credential.
+func TestMigrateV2toV3_CollisionDonatesSecret(t *testing.T) {
+	// Looped: with map-order-dependent code this is a coin flip, so a single
+	// pass can pass by luck.
+	for i := 0; i < 50; i++ {
+		testCollisionDonatesSecret(t, i)
+	}
+}
+
+func testCollisionDonatesSecret(t *testing.T, iteration int) {
+	t.Helper()
+	dir := setupConfigDir(t)
+	// The active entry wins but carries no secret; the loser's must be adopted.
+	writeConfigFile(t, dir, `{
+	  "version": 2,
+	  "registries": {
+	    "https://atcr.io": {
+	      "active": "alice.com",
+	      "accounts": {
+	        "alice.bsky.social": {"handle": "alice.bsky.social", "did": "did:plc:alice", "device_secret": "atcr_device_ONLY"},
+	        "alice.com": {"handle": "alice.com", "did": "did:plc:alice", "device_secret": ""}
+	      }
+	    }
+	  }
+	}`)
+
+	sc, err := loadConfig()
+	if err != nil {
+		t.Fatalf("loadConfig: %v", err)
+	}
+	acct := sc.Registries["https://atcr.io"].Accounts["did:plc:alice"]
+	if acct == nil {
+		t.Fatal("merged account missing")
+	}
+	if acct.DeviceSecret != "atcr_device_ONLY" {
+		t.Fatalf("iteration %d: secret = %q, want atcr_device_ONLY donated from the losing entry", iteration, acct.DeviceSecret)
+	}
+}
+
+// TestMigrateV2toV3_ToleratesNullEntries verifies a hand-edited or corrupted
+// file with null entries doesn't panic the helper on Docker's credential path.
+func TestMigrateV2toV3_ToleratesNullEntries(t *testing.T) {
+	dir := setupConfigDir(t)
+	writeConfigFile(t, dir, `{
+	  "version": 2,
+	  "registries": {
+	    "https://atcr.io": {"active": "a.com", "accounts": {"a.com": null, "b.com": {"handle":"b.com","did":"did:plc:b","device_secret":"atcr_device_B"}}},
+	    "https://other.io": null
+	  }
+	}`)
+
+	sc, err := loadConfig()
+	if err != nil {
+		t.Fatalf("loadConfig: %v", err)
+	}
+	reg := sc.Registries["https://atcr.io"]
+	if reg == nil {
+		t.Fatal("registry missing")
+	}
+	if a := reg.Accounts["did:plc:b"]; a == nil || a.DeviceSecret != "atcr_device_B" {
+		t.Errorf("valid account did not survive alongside a null entry: %+v", a)
+	}
+}
+
+// TestUpsertDoesNotStealAnotherIdentitysDID covers two accounts sharing a handle
+// (a rename leaves the old entry behind). Upserting one must not rewrite the
+// other's DID, which would destroy that account's credentials.
+func TestUpsertDoesNotStealAnotherIdentitysDID(t *testing.T) {
+	reg := &RegistryConfig{Accounts: map[string]*Account{
+		"did:plc:alice": {Handle: "shared.com", DID: "did:plc:alice", DeviceSecret: "atcr_device_ALICE"},
+	}}
+
+	reg.upsert(&Account{Handle: "shared.com", DID: "did:plc:bob", DeviceSecret: "atcr_device_BOB"})
+
+	if len(reg.Accounts) != 2 {
+		t.Fatalf("accounts = %d, want 2 (alice must not be overwritten)", len(reg.Accounts))
+	}
+	alice := reg.Accounts["did:plc:alice"]
+	if alice == nil || alice.DeviceSecret != "atcr_device_ALICE" {
+		t.Errorf("alice was clobbered: %+v", alice)
+	}
+	bob := reg.Accounts["did:plc:bob"]
+	if bob == nil || bob.DeviceSecret != "atcr_device_BOB" {
+		t.Errorf("bob not stored: %+v", bob)
+	}
+}
+
+// TestFindIsDeterministic covers duplicate handles: find must return the same
+// account every call, or Docker gets a different secret run to run.
+func TestFindIsDeterministic(t *testing.T) {
+	newReg := func() *RegistryConfig {
+		return &RegistryConfig{Accounts: map[string]*Account{
+			"did:plc:aaa": {Handle: "shared.com", DID: "did:plc:aaa", DeviceSecret: "atcr_device_A"},
+			"did:plc:zzz": {Handle: "shared.com", DID: "did:plc:zzz", DeviceSecret: "atcr_device_Z"},
+		}}
+	}
+
+	want := newReg().find("shared.com")
+	if want == nil {
+		t.Fatal("find returned nil for a known handle")
+	}
+	for i := 0; i < 50; i++ {
+		if got := newReg().find("shared.com"); got.DID != want.DID {
+			t.Fatalf("iteration %d: find returned %s, want %s consistently", i, got.DID, want.DID)
+		}
+	}
+
+	// An exact DID lookup must still win over the handle scan.
+	if got := newReg().find("did:plc:zzz"); got == nil || got.DID != "did:plc:zzz" {
+		t.Errorf("exact DID lookup returned %+v", got)
+	}
+}

pkg/credhelper/protocol.go

@@ -72,9 +72,13 @@ func runGet(cmd *cobra.Command, args []string) error {
 
 	appViewURL := buildAppViewURL(serverURL)
 
-	sc, err := loadConfig()
+	// `get` can rewrite the config (DID backfill, or removing an account whose
+	// credentials no longer validate), so it must not proceed on a config it
+	// failed to parse — the empty fallback would be saved over the real one.
+	// Returned, not printed: Run's top-level handler already prints it.
+	sc, err := loadConfigForWrite()
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "Warning: config load error: %v\n", err)
+		return err
 	}
 
 	acct, err := sc.resolveAccount(appViewURL, serverURL)
@@ -144,9 +148,14 @@ func runStore(cmd *cobra.Command, args []string) error {
 
 	appViewURL := buildAppViewURL(creds.ServerURL)
 
-	sc, err := loadConfig()
+	// Refuse to write when the existing config couldn't be read. Warning and
+	// continuing would save the empty fallback config, replacing every stored
+	// account with just this one. That is the downgrade path: an older build
+	// cannot parse a v3 file, so the first `docker login` would wipe it.
+	// Returned, not printed: Run's top-level handler already prints it.
+	sc, err := loadConfigForWrite()
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "Warning: config load error: %v\n", err)
+		return err
 	}
 
 	// Docker's store protocol only supplies {username, secret} — no DID. Resolve
@@ -170,9 +179,13 @@ func runErase(cmd *cobra.Command, args []string) error {
 
 	appViewURL := buildAppViewURL(serverURL)
 
-	sc, err := loadConfig()
+	// Erase writes, so an unreadable config means we do nothing rather than
+	// save the empty fallback over it. Reported as success: Docker surfaces a
+	// failing `logout` as a hard error, and there is nothing the user can act on.
+	sc, err := loadConfigForWrite()
 	if err != nil {
-		return nil // No config, nothing to erase
+		fmt.Fprintf(os.Stderr, "Warning: %v\n", err)
+		return nil
 	}
 
 	reg := sc.findRegistry(appViewURL)

pkg/credhelper/resolve.go

@@ -1,6 +1,7 @@
 package credhelper
 
 import (
+	"context"
 	"fmt"
 	"io"
 	"net"
@@ -8,6 +9,15 @@ import (
 	"time"
 )
 
+// dnsResolveTimeout bounds the _atproto TXT lookup. Handle resolution runs on
+// Docker's synchronous credential path (every `store`, and every `get` for an
+// account that has no DID yet), and the default resolver applies no deadline of
+// its own. Without this, a blackholing DNS setup — captive portal, split-horizon
+// VPN — stalls `docker login` and `docker pull` for the resolver's full retry
+// budget on every invocation. Resolution is best-effort, so giving up early just
+// leaves the account on its provisional handle key.
+const dnsResolveTimeout = 2 * time.Second
+
 // resolveHandleDID resolves an AT-proto handle to its DID using the two
 // standard resolution methods, no server or auth required:
 //
@@ -55,7 +65,10 @@ func looksLikeDID(s string) bool {
 // resolveHandleDNS looks up the _atproto.<handle> TXT record and extracts the
 // DID. Returns "" on any failure.
 func resolveHandleDNS(handle string) string {
-	records, err := net.LookupTXT("_atproto." + handle)
+	ctx, cancel := context.WithTimeout(context.Background(), dnsResolveTimeout)
+	defer cancel()
+
+	records, err := net.DefaultResolver.LookupTXT(ctx, "_atproto."+handle)
 	if err != nil {
 		return ""
 	}