Commit Graph

2 Commits

Author SHA1 Message Date
Bohan Jiang
ba5b7db78e fix(server): persist ModelListStore across replicas via Redis (#2022)
* fix(server): persist ModelListStore across replicas via Redis

The model picker uses a pending-request pattern: the frontend POSTs to
create a request, the daemon pops it on its next heartbeat, runs
agent.ListModels locally, and reports back. Until now the store was a
plain in-memory map per Handler instance.

That works for self-hosted single-instance deploys but fails in any
multi-replica environment (Multica Cloud). Each replica has its own
map, so:

  POST /runtimes/:id/models               → request stored in replica A
  GET  /runtimes/:id/models/<requestId>   → polls land on B/C → 404
  daemon heartbeat                        → only A sees PendingModelList
  POST .../<requestId>/result             → daemon's report has to land on A

Success probability ~1/N². The visible symptom is "No models available"
in the picker for every provider, even those (Claude/Codex) whose
catalog is statically populated end-to-end.

Same shape of bug, same Redis-backed fix as multica-ai/multica#1557 did
for LocalSkillListStore / LocalSkillImportStore. Reuse the operational
playbook (namespaced keys, ZSET-backed pending queue, atomic
ZREM+SET-running via the shared Lua script) so we don't introduce a
second concurrency model for the same primitive.

Changes:
- Convert ModelListStore from struct to interface with context-aware
  methods. Add HasPending for cheap heartbeat-side probing.
- InMemoryModelListStore — single-node fallback, used when REDIS_URL
  is unset (self-hosted dev / tests).
- RedisModelListStore — multi-node implementation using the same key
  layout and Lua atomic claim as RedisLocalSkillListStore.
- Use RunStartedAt (not UpdatedAt) as the running-timeout reference
  point, matching the local-skill stores so subsequent UpdatedAt
  bumps don't reset the running clock.
- Heartbeat now uses the probe-then-pop pattern for the model queue
  (matching local-skills) so a slow Redis can't stall every connected
  daemon. Extends heartbeatMetrics + slow-log with probe_model_ms /
  pop_model_ms / probe_model_timed_out for parity.
- Wire the Redis backend in NewRouterWithOptions when rdb != nil.
- Tests for both backends. Redis tests gate on REDIS_TEST_URL so
  laptop runs without Redis still pass; CI provides it.

Co-authored-by: multica-agent <github@multica.ai>

* fix(server): persist RunStartedAt + retry model report on transient failures

Two follow-ups from PR #2022 review:

1. RedisModelListStore was dropping ModelListRequest.RunStartedAt on
   persistence — the field is tagged json:"-" so it doesn't leak into
   the HTTP response, which made plain json.Marshal(req) silently
   discard it. Across-node readers saw RunStartedAt=nil and
   applyModelListTimeout's running branch became a no-op, so the 60s
   running-timeout escape hatch never fired. CI's
   TestRedisModelListStore_RunningTimeout was failing on this exact
   case. Fix mirrors RedisLocalSkillImportStore's envelope pattern —
   wrap in an internal struct that re-promotes the field. HTTP shape
   stays clean. Adds a no-Redis unit test that pins the round trip.

2. Daemon's handleModelList called d.client.ReportModelListResult
   directly and swallowed any 5xx, leaving the pending request
   stranded in "running" until its 60s server-side timeout — exactly
   the failure mode the multi-node store fix was meant to eliminate.
   Generalize the existing local-skill retry helper into
   reportRuntimeResultWithRetry (kind: model_list / local_skill_list /
   local_skill_import) and wire handleModelList through a new
   reportModelListResult helper. Renames the test-overridable
   var localSkillReportBackoffs → runtimeReportBackoffs to match.

Co-authored-by: multica-agent <github@multica.ai>

---------

Co-authored-by: multica-agent <github@multica.ai>
2026-05-03 11:13:34 +08:00
Bohan Jiang
cbe0cbef56 fix(daemon): retry local-skill reports on transient server errors (#1561)
Review follow-up on PR #1557: the server-side change started returning
500 when the store write failed, but the daemon's handleLocalSkillList /
handleLocalSkillImport were discarding the ReportLocalSkill*Result error
return. Net effect was a silent drop — the daemon moved on, the request
stayed in "running" on the server, and the user saw the same "daemon did
not respond within 30 seconds" timeout the store refactor was supposed
to kill.

Fix: route both report calls through reportLocalSkillResultWithRetry,
which retries on 5xx + network errors with 0 / 0.5s / 2s / 4s backoff
(total ~6.5s, well inside the 60s server-side running timeout), stops
on 4xx (request expired / cross-workspace rejection — retry won't help),
bails on context cancel, and logs Error on exhaustion so ops has a
footprint to grep for.

Tests (server/internal/daemon/local_skill_report_test.go, 6 new cases):
- 500 twice then success -> 3 attempts, second retry lands
- 404 -> exactly 1 attempt (permanent, no retry)
- import 502 then success -> 2 attempts
- All-500 -> burns through all backoff slots then gives up with ERROR log
- Context cancel mid-backoff -> exactly 1 attempt, cancellation logged
- Smoke: report paths hit /api/daemon/runtimes/<rt>/local-skills{,import}/<req>/result

localSkillReportBackoffs is var-assignable so tests can swap in zero-delay
schedules without paying real sleep latency.
2026-04-23 17:39:20 +08:00