From 35a2fb24ecd02269aaef2dba8c795c00d5bb9792 Mon Sep 17 00:00:00 2001 From: Jiang Bohan Date: Fri, 17 Apr 2026 14:22:39 +0800 Subject: [PATCH] fix(daemon): handle bidirectional .local drift and case drift in legacy merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review on #1220 flagged two gaps in the legacy-id migration candidate set: 1. Reverse .local: LegacyDaemonIDs only added the stripped variant when the current hostname ended in `.local`. The opposite direction — DB has `foo.local`, current host is `foo` — was missed, so runtimes registered under the `.local` variant stayed orphaned after upgrade. Now both variants (`foo` and `foo.local`) are always emitted, regardless of what `os.Hostname()` currently returns, plus their `-` suffix forms. 2. Case drift: os.Hostname() has been observed returning different casings on the same machine across mDNS/reboot state. A case-sensitive `=` comparison stranded rows like `Jiayuans-MacBook-Pro.local` when the daemon later reported `jiayuans-macbook-pro.local`. FindLegacyRuntimeByDaemonID now uses `LOWER(daemon_id) = LOWER(@daemon_id)` on both sides, so casing differences merge rather than orphan. The (workspace_id, provider) prefix still bounds the scan to a tiny set of rows so the non-indexed LOWER() comparison has negligible cost. Tests: TestLegacyDaemonIDs gets the mixed-case + reverse-direction cases; daemon_test.go adds TestDaemonRegister_MergesLegacyDaemonIDRuntime_ReverseDotLocal and TestDaemonRegister_MergesLegacyDaemonIDRuntime_CaseDrift. --- server/internal/daemon/identity.go | 26 ++--- server/internal/daemon/identity_test.go | 30 +++++- server/internal/handler/daemon.go | 2 +- server/internal/handler/daemon_test.go | 124 ++++++++++++++++++++++++ server/pkg/db/generated/runtime.sql.go | 10 +- server/pkg/db/queries/runtime.sql | 12 ++- 6 files changed, 182 insertions(+), 22 deletions(-) diff --git a/server/internal/daemon/identity.go b/server/internal/daemon/identity.go index 534e46708..8dc5f6310 100644 --- a/server/internal/daemon/identity.go +++ b/server/internal/daemon/identity.go @@ -89,25 +89,29 @@ func EnsureDaemonID(profile string) (string, error) { // - pre-#1070: "" (raw hostname, often ends in .local) // - current: "" with .local drift depending on system state // -// Because os.Hostname() may return either "foo" or "foo.local" depending on -// system state at registration time, we always emit both variants as legacy -// candidates so migration covers any prior registration. +// .local drift is bidirectional — at different times os.Hostname() has +// returned both "foo" and "foo.local" on the same machine (mDNS state, +// system restart, login item order). So regardless of which form is current +// now, we always emit BOTH the bare and .local-suffixed variants so migration +// covers whichever form was persisted previously. Case drift is handled on +// the server side via case-insensitive lookup, so we don't also emit cased +// permutations here. func LegacyDaemonIDs(hostname, profile string) []string { host := strings.TrimSpace(hostname) if host == "" { return nil } stripped := strings.TrimSuffix(host, ".local") + dotLocal := stripped + ".local" - candidates := []string{host} - if stripped != host { - candidates = append(candidates, stripped) - } + hostForms := []string{stripped, dotLocal} + + candidates := make([]string, 0, len(hostForms)*2) + candidates = append(candidates, hostForms...) if profile != "" { - candidates = append(candidates, - host+"-"+profile, - stripped+"-"+profile, - ) + for _, h := range hostForms { + candidates = append(candidates, h+"-"+profile) + } } seen := make(map[string]struct{}, len(candidates)) diff --git a/server/internal/daemon/identity_test.go b/server/internal/daemon/identity_test.go index 8d60a9120..f1d882b69 100644 --- a/server/internal/daemon/identity_test.go +++ b/server/internal/daemon/identity_test.go @@ -96,30 +96,39 @@ func TestLegacyDaemonIDs(t *testing.T) { want []string }{ { + // Bare hostname now — but the DB may still hold the previously + // registered `.local` variant, so we must emit both. name: "plain hostname, no profile", hostname: "MacBook-Pro", - want: []string{"MacBook-Pro"}, + want: []string{"MacBook-Pro", "MacBook-Pro.local"}, }, { + // Dot-local hostname now — the stripped variant may be what the + // DB holds from a prior registration where .local was absent. name: "dot-local hostname, no profile", hostname: "MacBook-Pro.local", - want: []string{"MacBook-Pro.local", "MacBook-Pro"}, + want: []string{"MacBook-Pro", "MacBook-Pro.local"}, }, { name: "plain hostname with profile", hostname: "MacBook-Pro", profile: "staging", - want: []string{"MacBook-Pro", "MacBook-Pro-staging"}, + want: []string{ + "MacBook-Pro", + "MacBook-Pro.local", + "MacBook-Pro-staging", + "MacBook-Pro.local-staging", + }, }, { name: "dot-local hostname with profile", hostname: "MacBook-Pro.local", profile: "staging", want: []string{ - "MacBook-Pro.local", "MacBook-Pro", - "MacBook-Pro.local-staging", + "MacBook-Pro.local", "MacBook-Pro-staging", + "MacBook-Pro.local-staging", }, }, { @@ -127,6 +136,17 @@ func TestLegacyDaemonIDs(t *testing.T) { hostname: "", want: nil, }, + { + // Case drift is handled on the server side (LOWER=LOWER match). + // We still emit the hostname in its current casing here; the SQL + // query normalizes both sides. + name: "mixed case hostname preserved as-is", + hostname: "Jiayuans-MacBook-Pro.local", + want: []string{ + "Jiayuans-MacBook-Pro", + "Jiayuans-MacBook-Pro.local", + }, + }, } for _, tc := range cases { diff --git a/server/internal/handler/daemon.go b/server/internal/handler/daemon.go index b4093bc16..a6d294e5a 100644 --- a/server/internal/handler/daemon.go +++ b/server/internal/handler/daemon.go @@ -334,7 +334,7 @@ func (h *Handler) mergeLegacyRuntimes(r *http.Request, registered db.AgentRuntim old, err := h.Queries.FindLegacyRuntimeByDaemonID(r.Context(), db.FindLegacyRuntimeByDaemonIDParams{ WorkspaceID: registered.WorkspaceID, Provider: provider, - DaemonID: strToText(legacyID), + DaemonID: legacyID, }) if err != nil { // sql.ErrNoRows is the expected common case — nothing to migrate. diff --git a/server/internal/handler/daemon_test.go b/server/internal/handler/daemon_test.go index 5dbf0f3e0..65edd0510 100644 --- a/server/internal/handler/daemon_test.go +++ b/server/internal/handler/daemon_test.go @@ -784,6 +784,130 @@ func TestDaemonRegister_MergesLegacyDaemonIDRuntime(t *testing.T) { } } +// TestDaemonRegister_MergesLegacyDaemonIDRuntime_ReverseDotLocal covers the +// direction missed by the initial implementation: the stored runtime row is +// `host` (no `.local`) but the daemon's current `os.Hostname()` now returns +// `host.local`. The daemon must emit the bare variant as a legacy candidate +// and the server must match it. +func TestDaemonRegister_MergesLegacyDaemonIDRuntime_ReverseDotLocal(t *testing.T) { + if testHandler == nil { + t.Skip("database not available") + } + + ctx := context.Background() + const legacyDaemonID = "ReverseDotLocalHost" // stored without .local + const emittedLegacyID = "ReverseDotLocalHost.local" // daemon now reports with .local + const newDaemonID = "0192a7b0-0011-7ee9-9c21-30a5bcf86aa2" + + var legacyRuntimeID string + if err := testPool.QueryRow(ctx, ` + INSERT INTO agent_runtime (workspace_id, daemon_id, name, runtime_mode, provider, status, device_info, metadata, owner_id, last_seen_at) + VALUES ($1, $2, 'legacy-runtime-reverse', 'local', 'claude', 'offline', '', '{}'::jsonb, $3, now()) + RETURNING id + `, testWorkspaceID, legacyDaemonID, testUserID).Scan(&legacyRuntimeID); err != nil { + t.Fatalf("seed legacy runtime: %v", err) + } + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM agent_runtime WHERE id = $1`, legacyRuntimeID) + }) + + w := httptest.NewRecorder() + req := newRequest("POST", "/api/daemon/register", map[string]any{ + "workspace_id": testWorkspaceID, + "daemon_id": newDaemonID, + "legacy_daemon_ids": []string{"ReverseDotLocalHost", emittedLegacyID}, + "device_name": "ReverseDotLocalHost", + "runtimes": []map[string]any{ + {"name": "reverse-runtime", "type": "claude", "version": "1.0.0", "status": "online"}, + }, + }) + testHandler.DaemonRegister(w, req) + if w.Code != http.StatusOK { + t.Fatalf("DaemonRegister: expected 200, got %d: %s", w.Code, w.Body.String()) + } + + var resp map[string]any + json.NewDecoder(w.Body).Decode(&resp) + newRuntimeID := resp["runtimes"].([]any)[0].(map[string]any)["id"].(string) + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM agent_runtime WHERE id = $1`, newRuntimeID) + }) + + var legacyCount int + if err := testPool.QueryRow(ctx, `SELECT count(*) FROM agent_runtime WHERE id = $1`, legacyRuntimeID).Scan(&legacyCount); err != nil { + t.Fatalf("count legacy runtime: %v", err) + } + if legacyCount != 0 { + t.Fatalf("expected legacy row to be merged and deleted, still present") + } +} + +// TestDaemonRegister_MergesLegacyDaemonIDRuntime_CaseDrift verifies that +// case-only drift in os.Hostname() output (e.g. `Jiayuans-MacBook-Pro.local` +// vs `jiayuans-macbook-pro.local`) still merges the legacy row. The daemon +// emits the id in its current casing; the server-side lookup uses LOWER() on +// both sides so stored and emitted casings can differ without orphaning. +func TestDaemonRegister_MergesLegacyDaemonIDRuntime_CaseDrift(t *testing.T) { + if testHandler == nil { + t.Skip("database not available") + } + + ctx := context.Background() + const storedDaemonID = "Jiayuans-MacBook-Pro.local" // DB has original mixed case + const emittedLegacyID = "jiayuans-macbook-pro.local" // Daemon now reports lowercased + const newDaemonID = "0192a7b0-0022-7ee9-9c21-30a5bcf86aa3" + + var legacyRuntimeID string + if err := testPool.QueryRow(ctx, ` + INSERT INTO agent_runtime (workspace_id, daemon_id, name, runtime_mode, provider, status, device_info, metadata, owner_id, last_seen_at) + VALUES ($1, $2, 'legacy-runtime-case', 'local', 'claude', 'offline', '', '{}'::jsonb, $3, now()) + RETURNING id + `, testWorkspaceID, storedDaemonID, testUserID).Scan(&legacyRuntimeID); err != nil { + t.Fatalf("seed legacy runtime: %v", err) + } + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM agent_runtime WHERE id = $1`, legacyRuntimeID) + }) + + w := httptest.NewRecorder() + req := newRequest("POST", "/api/daemon/register", map[string]any{ + "workspace_id": testWorkspaceID, + "daemon_id": newDaemonID, + "legacy_daemon_ids": []string{emittedLegacyID}, + "device_name": "jiayuans-macbook-pro", + "runtimes": []map[string]any{ + {"name": "case-drift-runtime", "type": "claude", "version": "1.0.0", "status": "online"}, + }, + }) + testHandler.DaemonRegister(w, req) + if w.Code != http.StatusOK { + t.Fatalf("DaemonRegister: expected 200, got %d: %s", w.Code, w.Body.String()) + } + + var resp map[string]any + json.NewDecoder(w.Body).Decode(&resp) + newRuntimeID := resp["runtimes"].([]any)[0].(map[string]any)["id"].(string) + t.Cleanup(func() { + testPool.Exec(context.Background(), `DELETE FROM agent_runtime WHERE id = $1`, newRuntimeID) + }) + + var legacyCount int + if err := testPool.QueryRow(ctx, `SELECT count(*) FROM agent_runtime WHERE id = $1`, legacyRuntimeID).Scan(&legacyCount); err != nil { + t.Fatalf("count legacy runtime: %v", err) + } + if legacyCount != 0 { + t.Fatalf("expected case-drift legacy row to be merged and deleted, still present") + } + + var legacyTrace *string + if err := testPool.QueryRow(ctx, `SELECT legacy_daemon_id FROM agent_runtime WHERE id = $1`, newRuntimeID).Scan(&legacyTrace); err != nil { + t.Fatalf("read legacy_daemon_id: %v", err) + } + if legacyTrace == nil || *legacyTrace != emittedLegacyID { + t.Fatalf("expected legacy_daemon_id trace = %q, got %v", emittedLegacyID, legacyTrace) + } +} + // TestDaemonRegister_LegacyIDNoMatchIsNoop guards the common case where the // daemon sends legacy candidates but no matching row exists (e.g. first // registration on a fresh machine). Registration must still succeed, the new diff --git a/server/pkg/db/generated/runtime.sql.go b/server/pkg/db/generated/runtime.sql.go index a57c836ca..d9ab05f14 100644 --- a/server/pkg/db/generated/runtime.sql.go +++ b/server/pkg/db/generated/runtime.sql.go @@ -118,18 +118,24 @@ const findLegacyRuntimeByDaemonID = `-- name: FindLegacyRuntimeByDaemonID :one SELECT id, workspace_id, daemon_id, name, runtime_mode, provider, status, device_info, metadata, last_seen_at, created_at, updated_at, owner_id, legacy_daemon_id FROM agent_runtime WHERE workspace_id = $1 AND provider = $2 - AND daemon_id = $3 + AND LOWER(daemon_id) = LOWER($3) ` type FindLegacyRuntimeByDaemonIDParams struct { WorkspaceID pgtype.UUID `json:"workspace_id"` Provider string `json:"provider"` - DaemonID pgtype.Text `json:"daemon_id"` + DaemonID string `json:"daemon_id"` } // Looks up a runtime row keyed on a prior (hostname-derived) daemon_id. Used // at register-time to find rows owned by the same machine under its old // identity so agents/tasks can be re-pointed at the new UUID-keyed row. +// +// Comparison is case-insensitive because os.Hostname() has been observed to +// return different casings on the same machine (e.g. `Jiayuans-MacBook-Pro` +// vs `jiayuans-macbook-pro`) across reboots/mDNS state changes. A case- +// sensitive `=` would strand the old row; LOWER() on both sides handles drift +// without forcing the daemon to enumerate cased permutations. func (q *Queries) FindLegacyRuntimeByDaemonID(ctx context.Context, arg FindLegacyRuntimeByDaemonIDParams) (AgentRuntime, error) { row := q.db.QueryRow(ctx, findLegacyRuntimeByDaemonID, arg.WorkspaceID, arg.Provider, arg.DaemonID) var i AgentRuntime diff --git a/server/pkg/db/queries/runtime.sql b/server/pkg/db/queries/runtime.sql index 3943632b2..899530e8b 100644 --- a/server/pkg/db/queries/runtime.sql +++ b/server/pkg/db/queries/runtime.sql @@ -83,10 +83,16 @@ DELETE FROM agent WHERE runtime_id = $1 AND archived_at IS NOT NULL; -- Looks up a runtime row keyed on a prior (hostname-derived) daemon_id. Used -- at register-time to find rows owned by the same machine under its old -- identity so agents/tasks can be re-pointed at the new UUID-keyed row. +-- +-- Comparison is case-insensitive because os.Hostname() has been observed to +-- return different casings on the same machine (e.g. `Jiayuans-MacBook-Pro` +-- vs `jiayuans-macbook-pro`) across reboots/mDNS state changes. A case- +-- sensitive `=` would strand the old row; LOWER() on both sides handles drift +-- without forcing the daemon to enumerate cased permutations. SELECT * FROM agent_runtime -WHERE workspace_id = $1 - AND provider = $2 - AND daemon_id = $3; +WHERE workspace_id = @workspace_id + AND provider = @provider + AND LOWER(daemon_id) = LOWER(@daemon_id); -- name: ReassignAgentsToRuntime :execrows -- Re-points every agent referencing old_runtime_id at new_runtime_id.