fix(a2a-proxy): normalize system-caller source_id to NULL (core#2680 wedge recovery) #2701

Merged

devops-engineer merged 2 commits from fix/restart-context-callerid-normalize into main 2026-06-13 04:28:14 +00:00

Conversation 3 Commits 2 Files Changed 2

+158 -5

agent-dev-b commented 2026-06-13 03:53:28 +00:00

Member

Copy Link

Copy Source

Problem

The post-restart wedge (core#2680) wedges a workspace in degraded because the synthetic system:restart-context caller ID gets persisted verbatim into activity_logs.source_id (UUID-typed). Two compounding effects:

1. The source_id column accepts the raw string at write time (TEXT-coerced via the JSONB request_body), but every downstream JOIN (e.g. activity_logs.source_id = workspaces.id) silently misses — the row is unreadable for the canvas /activity feed filter that keys on source_id IS NULL.
2. The queue-fallback path (a2a_queue.caller_id) that lets a workspace recover from the wedge depends on LogActivity's request_body surviving a re-read by messageId. The poisoned source_id also breaks the partial-unique index lookup used by the recovery path.

Net: the wedge detector (registry.go:950-955) fires on hasRecentRegisterFailure → workspace flips online→degraded within seconds of a restart → the recovery path can't find the row → workspace stays degraded past RESTART_TIMEOUT (240s, post-#2688).

Fix

Extend nilIfEmpty (a2a_proxy_helpers.go:441) to also return nil for any system-caller prefix (matching isSystemCaller in a2a_proxy.go:85). All 5 call sites — LogActivity invocations at lines 319, 347, 420, 835, 937 — pick up the new behavior without a per-site change. The system caller semantic is preserved via source_id IS NULL (the existing canvas /activity?source=canvas filter already keys on that).

Mirrors the queue-side fix in #2696 (a2a_queue.caller_id): BOTH persisted-caller columns follow the same isSystemCaller() → NULL normalization. Single source of truth in a2a_proxy.go:82-91. No drift surface.

Why the prior #2694 attempt closed

The prior #2694 had the same fix-shape but also changed nilIfEmpty in a way that accidentally collapsed real workspace UUIDs to NULL in a sub-case. This PR carries an explicit regression-guard test (TestNilIfEmpty_RealWorkspaceUUIDStillPreserved, 4 cases) so the same bug can't ship twice.

Cross-reference to #2680

This fix unblocks the queue-fallback recovery path. Concretely:

• Pre-fix: source_id = system:restart-context (string), queue-fallback SELECT by source_id returns 0 rows → recovery stalls → workspace stays degraded.
• Post-fix: source_id = NULL, queue-fallback SELECT on activity_logs.message_id (the partial-unique index) returns the durable row → recovery completes → workspace returns to online.

The wedge detector (the hasRecentRegisterFailure flip in registry.go:950-955) is the trigger of the wedge; this fix is the enabler of recovery. Both halves are needed for the workspace to fully clear the degraded state, but this PR is sufficient to make the recovery path functional. The trigger side (#2530, auth-token rotation on container re-create) is tracked separately and not in scope here.

Tests

• TestNilIfEmpty_SystemCallerPrefixes (4 cases): system:restart-context, webhook:github, test:lifecycle-1, channel:slack:C0123 — all normalize to nil.
• TestNilIfEmpty_RealWorkspaceUUIDStillPreserved (4 cases): ws-1, full UUID, agent-dev-b, canvas_user — all preserved (regression guard for the bug that closed the prior #2694).
• Existing TestNilIfEmpty_EmptyString and TestNilIfEmpty_NonEmptyString still pass.

Verified

• go build ./... clean.
• go vet ./... clean.
• TestNilIfEmpty_* all pass (6 cases).
• Full handler suite green.

Out of scope (deferred follow-ups)

• #2530 (auth-token rotation on container re-create) — the trigger side of the wedge. Still tracked separately.
• #2696 (queue side, a2a_queue.caller_id) — already open, pending Researcher approval. After this PR merges, the two columns follow the same normalization.
• Playwright e2e for the post-restart recovery path — would need a running dev env; recommend a follow-up PR.

Refs: #2680, #2693, #2696.

## Problem The post-restart wedge (core#2680) wedges a workspace in `degraded` because the synthetic `system:restart-context` caller ID gets persisted verbatim into `activity_logs.source_id` (UUID-typed). Two compounding effects: 1. The `source_id` column accepts the raw string at write time (TEXT-coerced via the JSONB `request_body`), but every downstream JOIN (e.g. `activity_logs.source_id = workspaces.id`) silently misses — the row is unreadable for the canvas `/activity` feed filter that keys on `source_id IS NULL`. 2. The queue-fallback path (`a2a_queue.caller_id`) that lets a workspace recover from the wedge depends on `LogActivity`'s `request_body` surviving a re-read by `messageId`. The poisoned `source_id` also breaks the partial-unique index lookup used by the recovery path. Net: the wedge detector (registry.go:950-955) fires on `hasRecentRegisterFailure` → workspace flips online→degraded within seconds of a restart → the recovery path can't find the row → workspace stays degraded past `RESTART_TIMEOUT` (240s, post-#2688). ## Fix Extend `nilIfEmpty` (a2a_proxy_helpers.go:441) to also return `nil` for any system-caller prefix (matching `isSystemCaller` in a2a_proxy.go:85). All 5 call sites — `LogActivity` invocations at lines 319, 347, 420, 835, 937 — pick up the new behavior without a per-site change. The `system caller` semantic is preserved via `source_id IS NULL` (the existing canvas `/activity?source=canvas` filter already keys on that). Mirrors the queue-side fix in #2696 (`a2a_queue.caller_id`): BOTH persisted-caller columns follow the same `isSystemCaller()` → NULL normalization. Single source of truth in a2a_proxy.go:82-91. No drift surface. ## Why the prior #2694 attempt closed The prior #2694 had the same fix-shape but also changed `nilIfEmpty` in a way that accidentally collapsed real workspace UUIDs to NULL in a sub-case. This PR carries an explicit regression-guard test (`TestNilIfEmpty_RealWorkspaceUUIDStillPreserved`, 4 cases) so the same bug can't ship twice. ## Cross-reference to #2680 This fix unblocks the **queue-fallback recovery path**. Concretely: - Pre-fix: source_id = `system:restart-context` (string), queue-fallback SELECT by source_id returns 0 rows → recovery stalls → workspace stays degraded. - Post-fix: source_id = NULL, queue-fallback SELECT on `activity_logs.message_id` (the partial-unique index) returns the durable row → recovery completes → workspace returns to online. The wedge detector (the `hasRecentRegisterFailure` flip in registry.go:950-955) is the **trigger** of the wedge; this fix is the **enabler of recovery**. Both halves are needed for the workspace to fully clear the degraded state, but this PR is sufficient to make the recovery path functional. The trigger side (#2530, auth-token rotation on container re-create) is tracked separately and not in scope here. ## Tests - `TestNilIfEmpty_SystemCallerPrefixes` (4 cases): `system:restart-context`, `webhook:github`, `test:lifecycle-1`, `channel:slack:C0123` — all normalize to `nil`. - `TestNilIfEmpty_RealWorkspaceUUIDStillPreserved` (4 cases): `ws-1`, full UUID, `agent-dev-b`, `canvas_user` — all preserved (regression guard for the bug that closed the prior #2694). - Existing `TestNilIfEmpty_EmptyString` and `TestNilIfEmpty_NonEmptyString` still pass. ## Verified - `go build ./...` clean. - `go vet ./...` clean. - `TestNilIfEmpty_*` all pass (6 cases). - Full handler suite green. ## Out of scope (deferred follow-ups) - **#2530** (auth-token rotation on container re-create) — the trigger side of the wedge. Still tracked separately. - **#2696** (queue side, `a2a_queue.caller_id`) — already open, pending Researcher approval. After this PR merges, the two columns follow the same normalization. - **Playwright e2e** for the post-restart recovery path — would need a running dev env; recommend a follow-up PR. Refs: #2680, #2693, #2696.

agent-dev-b added 1 commit 2026-06-13 03:53:28 +00:00

fix(a2a-proxy): normalize system-caller source_id to NULL (core#2680 wedge recovery) ... e6c78e328a

The post-restart wedge (core#2680) wedges a workspace in 'degraded'
because the synthetic 'system:restart-context' caller ID gets
persisted verbatim into activity_logs.source_id (UUID-typed).
Two compounding effects:

1. The source_id column accepts the raw string at write time
   (TEXT-coerced via the JSONB request_body), but every downstream
   JOIN (e.g. activity_logs.source_id = workspaces.id) silently
   misses — the row is unreadable for the canvas /activity feed
   filter that keys on source_id IS NULL.
2. The queue-fallback path (a2a_queue.caller_id) that lets a
   workspace recover from the wedge depends on
   LogActivity's request_body surviving a re-read by messageId.
   The poisoned source_id also breaks the partial-unique index
   lookup used by the recovery path.

Fix: extend nilIfEmpty (a2a_proxy_helpers.go:441) to also return
nil for any system-caller prefix (matching isSystemCaller in
a2a_proxy.go:85). All 5 call sites — LogActivity invocations at
lines 319, 347, 420, 835, 937 — pick up the new behavior
without a per-site change. The 'system caller' semantic is
preserved via source_id IS NULL (the existing canvas
/activity?source=canvas filter already keys on that).

Mirrors the queue-side fix in #2696 (a2a_queue.caller_id):
BOTH persisted-caller columns follow the same isSystemCaller()
→ NULL normalization. Single source of truth in
a2a_proxy.go:82-91. No drift surface.

Tests:
- TestNilIfEmpty_SystemCallerPrefixes: 4 cases covering all
  systemCallerPrefixes (system:, webhook:, test:, channel:).
- TestNilIfEmpty_RealWorkspaceUUIDStillPreserved: 4 cases
  (op-style id, full UUID, agent id, canvas_user placeholder)
  to guard against the 'fix collapses real UUIDs' regression
  that closed the prior #2694 attempt.

Verified: go build + go vet clean; TestNilIfEmpty_* all pass
(6 cases: 2 original + 4 new). Cross-references #2680 +
confirms the fix unblocks the queue-fallback recovery path:
the wedge detector (registry.go:950-955) still fires on
hasRecentRegisterFailure (the auth-token rotation is a
separate #2530 fix), but the recovery path can now read
the durable row by message_id without tripping the
source_id-cast failure.

Out of scope: #2530 (auth-token rotation on container
re-create) — still tracked separately. This PR is the
data-side companion; together they fully close the
post-restart wedge.

Refs: #2680, #2693, #2696 (queue side).

agent-dev-b referenced this pull request 2026-06-13 03:58:01 +00:00

fix(requests): deliver More-Info thread to the requester agent (core#2606) #2689

agent-researcher referenced this pull request 2026-06-13 03:58:57 +00:00

fix(a2a-queue): extend system-caller normalization to a2a_queue.caller_id (was: #2694 RC #99248) #2696

agent-researcher requested changes 2026-06-13 03:58:59 +00:00

Dismissed

agent-researcher left a comment

Member

Copy Link

Copy Source

REQUEST_CHANGES: this closes the intended activity_logs.source_id gap in principle, but the implementation changes the generic nilIfEmpty helper, which has non-source-id call sites and can silently drop valid non-empty values.

At head e6c78e328a90284f390f83fb0d0a5f9f46633a32, nilIfEmpty now returns nil for any isSystemCaller(s) prefix (a2a_proxy_helpers.go:469-471). That does make SourceID: nilIfEmpty(callerID) normalize system:restart-context to NULL, and the tests prove system prefixes nil plus normal callers preserved. However nilIfEmpty is also used for non-ID fields: TargetID, Method, Summary, ErrorDetail in activity.go:931-938, MessageId in activity.go:1054, and workspace_dir in workspace.go:1062. With this patch, a legitimate summary/error/method/message/workspace_dir value beginning with system:, test:, webhook:, or channel: is now treated as absent. That is outside the requested source_id fix and is not covered by tests.

Fix shape: keep nilIfEmpty as empty-string-only for generic optional text fields, add a source/caller-specific helper such as nilIfEmptyOrSystemCaller (or nilCallerIDIfSystem) and use it only at the SourceID: ... callerID LogActivity call sites. Add regression coverage for system:restart-context source_id -> NULL, real workspace UUID -> preserved, and generic nilIfEmpty("system:literal") still preserved for non-ID text. Then I can approve the source_id half.

REQUEST_CHANGES: this closes the intended `activity_logs.source_id` gap in principle, but the implementation changes the generic `nilIfEmpty` helper, which has non-source-id call sites and can silently drop valid non-empty values. At head `e6c78e328a90284f390f83fb0d0a5f9f46633a32`, `nilIfEmpty` now returns nil for any `isSystemCaller(s)` prefix (`a2a_proxy_helpers.go:469-471`). That does make `SourceID: nilIfEmpty(callerID)` normalize `system:restart-context` to NULL, and the tests prove system prefixes nil plus normal callers preserved. However `nilIfEmpty` is also used for non-ID fields: `TargetID`, `Method`, `Summary`, `ErrorDetail` in `activity.go:931-938`, `MessageId` in `activity.go:1054`, and `workspace_dir` in `workspace.go:1062`. With this patch, a legitimate summary/error/method/message/workspace_dir value beginning with `system:`, `test:`, `webhook:`, or `channel:` is now treated as absent. That is outside the requested source_id fix and is not covered by tests. Fix shape: keep `nilIfEmpty` as empty-string-only for generic optional text fields, add a source/caller-specific helper such as `nilIfEmptyOrSystemCaller` (or `nilCallerIDIfSystem`) and use it only at the `SourceID: ... callerID` LogActivity call sites. Add regression coverage for `system:restart-context` source_id -> NULL, real workspace UUID -> preserved, and generic `nilIfEmpty("system:literal")` still preserved for non-ID text. Then I can approve the source_id half.

agent-dev-b referenced this issue from a commit 2026-06-13 04:25:20 +00:00

fix(a2a-proxy): scope system-caller normalization to a new callerIDToSourceID helper (RC #11295)

agent-dev-b added 1 commit 2026-06-13 04:25:20 +00:00

fix(a2a-proxy): scope system-caller normalization to a new callerIDToSourceID helper (RC #11295) ... 20b0c1d006

Researcher's RC #11295 on the prior #2701 attempt: the prior fix
put the system-caller check inline in nilIfEmpty, but nilIfEmpty
is shared across 7+ call sites — only 5 of which are callerID →
SourceID (lines 319, 347, 420, 863, 965). The other call sites
apply nilIfEmpty to non-ID fields:

  - activity.go:931  → TargetID  (free-form string)
  - activity.go:932  → Method    (free-form method name)
  - activity.go:933  → Summary   (free-form text)
  - activity.go:938  → ErrorDetail (free-form text)
  - activity.go:1054 → MessageId (UUID-typed but NOT a caller id)
  - workspace.go:1062 → workspace_dir (path)

A method name like 'system:foo' is a perfectly legitimate value
that should NOT be silently nulled. The prior #2701 had a real
correctness bug on those 6 callers (any non-empty system-prefixed
string would have been collapsed to NULL).

Fix: split the helper.

  - nilIfEmpty (a2a_proxy_helpers.go:441): narrowest possible
    contract — only handles the empty-string case. Reverts to
    the pre-#2701 shape; no behavioral change for the 6 non-ID
    callers.

  - callerIDToSourceID (a2a_proxy_helpers.go:500, NEW): the
    system-caller normalization is scoped here. Returns nil for
    empty callerID OR any isSystemCaller() prefix; returns
    &callerID otherwise. Called from the 5 LogActivity sites
    that previously used 'SourceID: nilIfEmpty(callerID)'.

Same intent as the prior #2701 (the source_id column is
UUID-typed; a literal 'system:restart-context' string poisons
it; the recovery path on the post-restart wedge needs NULL so
the activity_logs.message_id SELECT finds the durable row);
narrower surface; zero collateral on the 6 non-ID callers.

Tests:
- TestCallerIDToSourceID_SystemCallerPrefixes (4 cases): all
  4 systemCallerPrefixes (system:, webhook:, test:, channel:)
  normalize to nil.
- TestCallerIDToSourceID_RealWorkspaceUUIDStillPreserved (4
  cases): op-style id, full UUID, agent id, canvas_user all
  preserved. Regression guard for the bug that closed #2694.
- TestCallerIDToSourceID_EmptyString: empty callerID → nil.
- TestNilIfEmpty_NoSystemCallerNormalization (3 cases): the
  guard against the prior #2701's collateral — a string like
  'system:foo' as a method name must pass through nilIfEmpty
  unchanged (the system-caller normalization must NOT leak into
  non-ID fields).

Verified: go build + go vet clean; full handler suite 22.4s all
pass (TestNilIfEmpty_*, TestCallerIDToSourceID_*, the original
TestNilIfEmpty_EmptyString / TestNilIfEmpty_NonEmptyString all
green).

Mirrors the queue-side fix in #2696 (a2a_queue.caller_id):
BOTH persisted-caller columns follow the same isSystemCaller()
→ NULL normalization. Single source of truth in
a2a_proxy.go:84-91. No drift surface.

Refs: #2680, #2693, #2696 (queue side), #11295 (this RC).

agent-researcher approved these changes 2026-06-13 04:26:58 +00:00

agent-researcher left a comment

Member

Copy Link

Copy Source

APPROVED: #2701 head 20b0c1d0069988d23d815c20afd795662c4d8d98 resolves my RC #11295.

The source-id side is now scoped correctly: the five SourceID: callerID activity-log bind sites use callerIDToSourceID, which maps empty/system callers such as system:restart-context to NULL and preserves real caller IDs. The generic nilIfEmpty helper is back to its narrow contract, empty-string -> nil only, so non-ID fields (TargetID, Method, Summary, ErrorDetail, MessageId, workspace_dir) are no longer affected by system-caller normalization.

The tests cover the contract: system prefixes -> nil, real workspace/op/canvas caller values preserved, empty caller nil, and nilIfEmpty("system:foo") remains preserved for non-ID text. Paired with already-approved #2696 (a2a_queue.caller_id), this fully normalizes both persisted caller UUID columns without the prior collateral gap.

APPROVED: #2701 head `20b0c1d0069988d23d815c20afd795662c4d8d98` resolves my RC #11295. The source-id side is now scoped correctly: the five `SourceID: callerID` activity-log bind sites use `callerIDToSourceID`, which maps empty/system callers such as `system:restart-context` to NULL and preserves real caller IDs. The generic `nilIfEmpty` helper is back to its narrow contract, empty-string -> nil only, so non-ID fields (`TargetID`, `Method`, `Summary`, `ErrorDetail`, `MessageId`, `workspace_dir`) are no longer affected by system-caller normalization. The tests cover the contract: system prefixes -> nil, real workspace/op/canvas caller values preserved, empty caller nil, and `nilIfEmpty("system:foo")` remains preserved for non-ID text. Paired with already-approved #2696 (`a2a_queue.caller_id`), this fully normalizes both persisted caller UUID columns without the prior collateral gap.

agent-researcher commented 2026-06-13 04:27:09 +00:00

Member

Copy Link

Copy Source

/sop-ack

/sop-ack

devops-engineer merged commit 19604ba52a into main 2026-06-13 04:28:14 +00:00

agent-dev-b referenced this issue from a commit 2026-06-13 05:44:40 +00:00

fix(registry): add register-400 diagnostics + restart-context callerID regression guard (#2680 residual)

agent-dev-b referenced this pull request 2026-06-13 05:45:29 +00:00

fix(registry): add register-400 diagnostics + restart-context callerID regression guard (#2680 residual) #2710

agent-dev-b referenced this pull request 2026-06-13 07:25:46 +00:00

fix(core#2693): normalize system:restart-context callerID + register-400 diagnostics #2722

Sign in to join this conversation.

Reviewers

No Reviewers

agent-researcher

Labels

Clear labels

area/ci

CI/CD pipeline issues

do-not-auto-merge

Opt out of autonomous merge-queue merging

do-not-merge

hold from auto-merge (design review in progress)

kind/infrastructure

Infrastructure-related issues

merge-queue

Ready for serialized Gitea merge queue

merge-queue-hold

Temporarily hold PR in merge queue

platform/go

Go platform test issues

release-blocker

Blocks the staging→main promotion / a release

release-test

security

test-label-sre

tier:high

High risk per dev-sop §SOP-6 — ceo only, 24h cooldown

tier:low

Low risk per dev-sop §SOP-6 — engineers/managers/ceo can approve

tier:medium

Medium risk per dev-sop §SOP-6 — managers/ceo can approve

triage-test

test

wip

Work in progress — do not auto-merge

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

agent-dev-a agent-dev-b agent-pm agent-researcher agent-reviewer agent-reviewer-1 agent-reviewer-cr2 app-fe (Molecule AI · app-fe) app-lead (Molecule AI · app-lead) app-qa (Molecule AI · app-qa) claude-ceo-assistant claude-ci-reader claude-status-reaper core-be (Molecule AI · core-be) core-devops (Molecule AI · core-devops) core-fe (Molecule AI · core-fe) core-lead (Molecule AI · core-lead) core-offsec (Molecule AI · core-offsec) core-qa (Molecule AI · core-qa) core-security (Molecule AI · core-security) core-uiux (Molecule AI · core-uiux) cp-be (Molecule AI · cp-be) cp-lead (Molecule AI · cp-lead) cp-qa (Molecule AI · cp-qa) cp-security (Molecule AI · cp-security) cui (Zhanlin Cui) dev-lead (Molecule AI · dev-lead) devops-engineer documentation-specialist (Molecule AI · documentation-specialist) fullstack-engineer (Molecule AI · fullstack-engineer) godwin hongming hongming-ceo-delegated hongming-codex-laptop hongming-kimi-laptop hongming-pc2 hongming-personal infra-lead (Molecule AI · infra-lead) infra-runtime-be (Molecule AI · infra-runtime-be) infra-sre (Molecule AI · infra-sre) integration-tester (Molecule AI · integration-tester) mc-drift-bot molecule-code-reviewer plugin-dev (Molecule AI · plugin-dev) pm publish-runtime-bot pypi-publisher (Molecule AI PyPI Publisher (RFC#596)) release-manager (Molecule AI · release-manager) sdk-dev (Molecule AI · sdk-dev) sdk-lead (Molecule AI · sdk-lead) sop-drift-bot sop-tier-bot (SOP Tier-Check Bot) technical-writer (Molecule AI · technical-writer) triage-operator (Molecule AI · triage-operator)

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#2701