molecule-ai/molecule-core
52
0
Fork 2
Code Issues 180 Pull Requests 52 Actions 1 Packages Projects Releases Wiki Activity

fix(channels): check rows.Err after iteration in List and Webhook #2034

Closed
core-be wants to merge 19 commits from fix/channels-rows-err-check into staging
pull from: fix/channels-rows-err-check
merge into: molecule-ai:staging
Conversation 1 Commits 19 Files Changed 23
+326 -67
core-be commented 2026-06-01 03:36:01 +00:00
Member
Copy Link
Copy Source
No description provided.
core-be added 21 commits 2026-06-01 03:36:01 +00:00
fix(workspace-server): CP orphan sweeper closes deprovision split-write race (#2989) 0996255d41 
The deprovision path marks `workspaces.status='removed'` BEFORE calling
the controlplane DELETE. If that CP call fails (transient 5xx, network
hiccup, AWS provider error), the DB row stays at 'removed' with
`instance_id` populated and there's no retry — the EC2 lives forever.
9 prod orphans accumulated over 3 days under this bug.

Adds a SaaS-mode counterpart to the existing Docker `orphan_sweeper`:
- 60s tick (matches the Docker sweeper cadence)
- LIMIT 100 per cycle so a sustained CP outage drains over multiple
  cycles without blowing the request timeout
- Re-issues `cpProv.Stop` for any workspace at status='removed' with a
  non-NULL `instance_id`. Stop is idempotent (AWS terminate on
  already-terminated is a no-op; CP's Deprovision tolerates already-
  deleted DNS) so retries are safe.
- On Stop success, NULLs `instance_id` so the next cycle skips the row.
- On Stop failure, leaves `instance_id` populated for next cycle.

The existing Docker sweeper is gated on `prov != nil`; the new sweeper
is gated on `cpProv != nil`. SaaS tenants get exactly one of the two,
self-hosted tenants get the Docker one — no overlap.

Why this shape over option A (CP-first ordering) or B (durable outbox):
the existing inline path already returns a loud 500 to the user when
CP fails — the only missing piece is automatic retry, which a 60s
sweeper provides without protocol changes, new tables, or new workers.
~30 LOC of production code vs. ~400 for an outbox. RFC discussion in
#2989 comment chain.

Tests:
- 9 unit tests covering happy path, Stop failure, UPDATE failure,
  multiple orphans (one-fails-others-still-process), DB query error,
  nil-DB defense, nil-reaper short-circuit, and the boot-immediate-then-
  tick cadence contract.
- Mutation-tested: status='running' substitution and removed-UPDATE-
  block both fail at least one test.

Out of scope:
- Backfilling the 9 named orphans — they'll heal automatically on the
  first sweep cycle after this lands; no manual cleanup needed.
- Long-term durable-outbox architecture — separate RFC.
review: 5-axis review of PR #3029 and PR #3033 d46ecc058f 
- PR #3029: approve with nit (CP orphan sweeper + registry prefix).
- PR #3033: approve with blocker — README undercounts runtimes by 1
  because #3029 adds codex but #3033 claims 8 runtimes and omits it.
- All tests pass locally.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
fix(http): replace http.DefaultClient in auth-adjacent paths 960baf8884 
Replaces http.DefaultClient in MCP bridge (mcp_tools.go) and CP config
fetch (cp_config.go) with dedicated clients that have transport-level
timeouts. Eliminates a fragility class where global mutable client +
missing context timeout could hang forever on dead workspaces or slow CP.

- mcpHTTPClient: 5s dial, 30s response header, 5s TLS handshake
- cp_config fetch: 10s client timeout matching context deadline

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
fix(delegation): pass context through updateDelegationStatus 543e4e0f57 
updateDelegationStatus was using context.Background() internally,
discarding any caller deadline. This meant status updates could hang
indefinitely if the DB was slow, and cancellation signals from HTTP
request contexts were ignored.

Change signature to accept context.Context and update all call sites.
No behavioral change for happy path; fixes context-timeout hygiene.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
fix(context): propagate ctx to DB queries in provision + discovery 1190e52300 
Two production code paths used context-less sql.DB methods:

1. buildProvisionerConfig (workspace_provision.go:263) called db.DB.QueryRow
   instead of QueryRowContext(ctx, ...) — losing request cancellation.

2. queryPeerMaps (discovery.go:307) called db.DB.Query instead of
   QueryContext(ctx, ...) and did not accept a context parameter. Updated
   signature and all 4 call sites in Peers handler to pass ctx.

No behavioral change for happy path; fixes context-timeout hygiene so
slow queries can be cancelled when the HTTP client disconnects.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
fix(tx): add deferred rollback guard to workspace creation transaction 8fd52156c9 
workspace.go BeginTx had explicit Rollback() on every error path but
no deferred safety net. A panic (or future refactor adding an early
return) would leak the tx hold. Add defer tx.Rollback() immediately
after BeginTx — harmless no-op after Commit, critical safety net on
unexpected exits.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
fix(context): defer rows.Close in restart_context secret key queries 38a9599c7c 
Two QueryContext calls in restart_context used explicit rows.Close()
after the loop. If a panic or early return occurred during iteration,
the underlying connection would leak back to the pool with an active
result set. Switch to defer rows.Close() inside each if-block for
 RAII-style cleanup.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
fix(channels): recover panics in async webhook HandleInbound goroutine 80bbdd22f1 
The async goroutine that processes inbound webhooks had no recover().
A panic inside HandleInbound (e.g., from the A2A proxy or adapter layer)
would crash the entire platform process. Add defer recover() with
workspace-scoped logging so the goroutine exits cleanly and the process
stays alive.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
fix(a2a_proxy): recover panics in fire-and-forget logging goroutines c8d056cedd 
Four fire-and-forget goroutines in a2a_proxy_helpers.go had no recover():
logA2AFailure, logA2ASuccess, logA2AReceiveQueued, and last_outbound_at
update. A panic inside LogActivity or db.DB.ExecContext would crash the
platform process. Add defer recover() with workspace-scoped logging to
each so the goroutine exits cleanly.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
fix(mcp_tools): recover panics in async delegate_task goroutine 092822a3a3
fix(goroutines): recover panics in background and fire-and-forget goroutines de6c5c8778 
Add panic recovery to goroutines that are NOT wrapped by supervised.RunWithRecover:
- session_auth.go: init() cache sweeper (process-level background task)
- ratelimit.go + mcp_ratelimit.go: bucket cleanup tickers
- bundle/importer.go: fire-and-forget provision start during import
- plugins_install.go: delayed restart after plugin uninstall
- terminal.go: WebSocket bridge goroutines (stdout, PTY, stdin)
- a2a_proxy.go: SSE idle watcher

A panic in any of these would previously crash the process or terminate
a subsystem silently. Now they log and swallow the panic, preserving
process stability.
fix(restart_context): use NewTicker instead of time.After in poll loop 980221a98f 
Prevents bounded timer accumulation during the 30s workspace-online wait.
Each time.After created a timer that couldn't be GC'd until it fired;
with ~60 iterations this was minor but unnecessary.
fix(time.After): replace time.After with NewTimer/NewTicker in loops 55f4392b5b 
Prevents timer accumulation in long-running loops:
- restart_context.go: poll loop uses NewTicker instead of time.After
- supervised.go: backoff sleep uses NewTimer with proper Stop
- telegram.go: poll loop uses NewTimer for both retryAfter and poll interval

Each time.After created a timer that couldn't be GC'd until it fired.
In long-running goroutines this caused bounded but unnecessary memory
growth.
fix(bundle): check errors in markFailed and provision URL update d4ae56b51a 
Add error checking + logging to previously ignored db.ExecContext and
broadcaster.RecordAndBroadcast calls in bundle importer. Improves
observability when provision failures or URL updates fail silently.
fix(errcheck): log ignored errors in channels, telegram, approvals 857776f40c 
Apply unchecked-error fixes from errcheck audit (#1062):
- channels/manager.go: log db.ExecContext and broadcaster errors in HandleInbound/SendOutbound; return json.Unmarshal errors in loadChannel
- channels/telegram.go: log bot.Send errors for callback ack and edit message
- handlers/approvals.go: log broadcaster, db.QueryRowContext, and db.ExecContext errors

Improves observability when secondary operations fail silently.
fix(workspace): check error on external workspace status update 6fc5238928
fix(ci): remove || true suppression from lint steps (#1062) 2a1a50a321 
The `|| true` suffix on `go vet` and `golangci-lint` commands caused
the CI job to pass even when lint errors were present, masking real
issues and defeating the purpose of static analysis.

Remove the suppression so lint failures correctly fail the build.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
fix(memory): check rows.Err after iteration to detect partial results 46a865b040 
The List handler previously ignored rows.Err(), which meant a broken
connection or cursor error mid-stream would return a partial 200 OK
instead of a 500. Add the check so interrupted result sets surface as
errors rather than silent data loss.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
fix(events): check rows.Err after iteration in both list handlers 4e4ec3b7cf 
List and ListByWorkspace ignored rows.Err(), so a broken DB cursor
mid-stream would return a partial 200 OK instead of 500. Add the check
to both handlers so interrupted result sets surface as errors.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
fix(channels): remove duplicate EncryptSensitiveFields call in Create 3f09149031 
The Create handler called EncryptSensitiveFields twice in a row,
causing the config to be double-encrypted. The second call was a
regression from a bad merge or copy-paste. Remove the duplicate.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
fix(channels): check rows.Err after iteration in List and Webhook
E2E API Smoke Test / E2E API Smoke Test (pull_request) Blocked by required conditions
Details
Handlers Postgres Integration / Handlers Postgres Integration (pull_request) Blocked by required conditions
Details
Harness Replays / Harness Replays (pull_request) Blocked by required conditions
Details
Runtime PR-Built Compatibility / PR-built wheel + import smoke (pull_request) Blocked by required conditions
Details
branch-protection drift check / Branch protection drift (pull_request) Successful in 7s
Details
cascade-list-drift-gate / check (pull_request) Successful in 6s
Details
Check merge_group trigger on required workflows / Required workflows have merge_group trigger (pull_request) Successful in 5s
Details
Block internal-flavored paths / Block forbidden paths (pull_request) Successful in 21s
Details
Check migration collisions / Migration version collision check (pull_request) Successful in 9s
Details
CI / Detect changes (pull_request) Successful in 8s
Details
CodeQL / Analyze (${{ matrix.language }}) (python) (pull_request) Failing after 4s
Details
CodeQL / Analyze (${{ matrix.language }}) (javascript-typescript) (pull_request) Failing after 6s
Details
CodeQL / Analyze (${{ matrix.language }}) (go) (pull_request) Failing after 7s
Details
Handlers Postgres Integration / detect-changes (pull_request) Successful in 7s
Details
E2E Staging Canvas (Playwright) / detect-changes (pull_request) Successful in 7s
Details
E2E API Smoke Test / detect-changes (pull_request) Successful in 8s
Details
Lint curl status-code capture / Scan workflows for curl status-capture pollution (pull_request) Successful in 5s
Details
Harness Replays / detect-changes (pull_request) Successful in 5s
Details
Runtime PR-Built Compatibility / detect-changes (pull_request) Successful in 10s
Details
qa-review / approved (pull_request) Successful in 6s
Details
Secret scan / Scan diff for credential-shaped strings (pull_request) Failing after 31s
Details
sop-checklist / na-declarations (pull_request) N/A: (none)
Details
security-review / approved (pull_request) Successful in 6s
Details
sop-checklist / review-refire (pull_request) Has been skipped
Details
Ops Scripts Tests / Ops scripts (unittest) (pull_request) Successful in 33s
Details
Runtime Pin Compatibility / PyPI-latest install + import smoke (pull_request) Successful in 1m33s
Details
gate-check-v3 / gate-check (pull_request) Successful in 9s
Details
sop-checklist / all-items-acked (pull_request) Successful in 7s
Details
sop-tier-check / tier-check (pull_request) Successful in 6s
Details
CI / Platform (Go) (pull_request) Failing after 1m11s
Details
CI / Shellcheck (E2E scripts) (pull_request) Successful in 15s
Details
CI / Canvas (Next.js) (pull_request) Failing after 1m55s
Details
CI / Canvas Deploy Reminder (pull_request) Has been skipped
Details
E2E Staging Canvas (Playwright) / Canvas tabs E2E (pull_request) Successful in 2s
Details
CI / Python Lint & Test (pull_request) Failing after 7m9s
Details
1c8343a09d 
List and Webhook handlers ignored rows.Err(), so a broken DB cursor
mid-stream would return a partial 200 OK instead of 500. Add the check
to both handlers so interrupted result sets surface as errors.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
core-be changed target branch from main to staging 2026-06-01 03:39:55 +00:00
core-be force-pushed fix/channels-rows-err-check from 1c8343a09d to e7ecf6203a 2026-06-01 04:23:22 +00:00 Compare
core-be added 1 commit 2026-06-01 12:41:05 +00:00
ci: remove unused canvasUserMessage type to fix lint on staging
Block internal-flavored paths / Block forbidden paths (pull_request) Successful in 4s
Details
CI / Detect changes (pull_request) Successful in 13s
Details
CI / Shellcheck (E2E scripts) (pull_request) Successful in 16s
Details
E2E API Smoke Test / detect-changes (pull_request) Successful in 9s
Details
E2E Chat / detect-changes (pull_request) Successful in 14s
Details
Handlers Postgres Integration / detect-changes (pull_request) Successful in 4s
Details
CI / Platform (Go) (pull_request) Successful in 5m14s
Details
CI / Canvas (Next.js) (pull_request) Successful in 7m5s
Details
CI / Python Lint & Test (pull_request) Successful in 8m31s
Details
Harness Replays / detect-changes (pull_request) Successful in 4s
Details
CI / all-required (pull_request) Successful in 7m41s
Details
Runtime PR-Built Compatibility / detect-changes (pull_request) Successful in 12s
Details
lint-required-no-paths / lint-required-no-paths (pull_request) Successful in 1m12s
Details
Secret scan / Scan diff for credential-shaped strings (pull_request) Successful in 8s
Details
gate-check-v3 / gate-check (pull_request_target) Successful in 5s
Details
qa-review / approved (pull_request_target) Successful in 8s
Details
security-review / approved (pull_request_target) Successful in 5s
Details
sop-checklist / all-items-acked (pull_request) acked: 0/7 — missing: comprehensive-testing, local-postgres-e2e, staging-smoke, +4 — body-unfilled: comprehensive-testing, local-postgres-e2
Details
sop-checklist / na-declarations (pull_request) N/A: (none)
Details
sop-checklist / all-items-acked (pull_request_target) Successful in 6s
Details
sop-tier-check / tier-check (pull_request_target) Successful in 5s
Details
CI / Canvas Deploy Reminder (pull_request) Has been skipped
Details
E2E API Smoke Test / E2E API Smoke Test (pull_request) Successful in 2m25s
Details
Harness Replays / Harness Replays (pull_request) Successful in 30s
Details
Runtime PR-Built Compatibility / PR-built wheel + import smoke (pull_request) Successful in 8s
Details
Handlers Postgres Integration / Handlers Postgres Integration (pull_request) Successful in 2m53s
Details
E2E Chat / E2E Chat (pull_request) Failing after 7m37s
Details
audit-force-merge / audit (pull_request_target) Has been skipped
Details
528a062354 
internal/handlers/a2a_proxy_helpers.go:412 had an unused struct that
causes golangci-lint `unused` failure on every PR targeting staging.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
devops-engineer commented 2026-06-01 18:28:35 +00:00
Member
Copy Link
Copy Source

Closing — based on the dead staging fork; superseded/junk relative to main. this edits .github/workflows/ci.yml, a dead GitHub-Actions workflow that does not exist on main (CI is now .gitea/workflows/), and commits a stray review-pr3029-pr3033.md review artifact. The bundled Go error-check sweep is already present on main independently. Per the staging-deprecation cleanup; target main for future PRs.

Closing — based on the dead `staging` fork; superseded/junk relative to `main`. this edits `.github/workflows/ci.yml`, a dead GitHub-Actions workflow that does not exist on `main` (CI is now `.gitea/workflows/`), and commits a stray `review-pr3029-pr3033.md` review artifact. The bundled Go error-check sweep is already present on `main` independently. Per the staging-deprecation cleanup; target `main` for future PRs.
devops-engineer closed this pull request 2026-06-01 18:28:36 +00:00
Some required checks failed
Block internal-flavored paths / Block forbidden paths (pull_request) Successful in 4s
Details
CI / Detect changes (pull_request) Successful in 13s
Details
CI / Shellcheck (E2E scripts) (pull_request) Successful in 16s
Details
E2E API Smoke Test / detect-changes (pull_request) Successful in 9s
Details
E2E Chat / detect-changes (pull_request) Successful in 14s
Details
Handlers Postgres Integration / detect-changes (pull_request) Successful in 4s
Details
CI / Platform (Go) (pull_request) Successful in 5m14s
Details
CI / Canvas (Next.js) (pull_request) Successful in 7m5s
Details
CI / Python Lint & Test (pull_request) Successful in 8m31s
Details
Harness Replays / detect-changes (pull_request) Successful in 4s
Details
CI / all-required (pull_request) Successful in 7m41s
Required
Details
Runtime PR-Built Compatibility / detect-changes (pull_request) Successful in 12s
Details
lint-required-no-paths / lint-required-no-paths (pull_request) Successful in 1m12s
Details
Secret scan / Scan diff for credential-shaped strings (pull_request) Successful in 8s
Details
gate-check-v3 / gate-check (pull_request_target) Successful in 5s
Details
qa-review / approved (pull_request_target) Successful in 8s
Details
security-review / approved (pull_request_target) Successful in 5s
Details
sop-checklist / all-items-acked (pull_request) acked: 0/7 — missing: comprehensive-testing, local-postgres-e2e, staging-smoke, +4 — body-unfilled: comprehensive-testing, local-postgres-e2
Required
Details
sop-checklist / na-declarations (pull_request) N/A: (none)
Details
sop-checklist / all-items-acked (pull_request_target) Successful in 6s
Details
sop-tier-check / tier-check (pull_request_target) Successful in 5s
Details
CI / Canvas Deploy Reminder (pull_request) Has been skipped
Details
E2E API Smoke Test / E2E API Smoke Test (pull_request) Successful in 2m25s
Details
Harness Replays / Harness Replays (pull_request) Successful in 30s
Details
Runtime PR-Built Compatibility / PR-built wheel + import smoke (pull_request) Successful in 8s
Details
Handlers Postgres Integration / Handlers Postgres Integration (pull_request) Successful in 2m53s
Details
E2E Chat / E2E Chat (pull_request) Failing after 7m37s
Details
audit-force-merge / audit (pull_request_target) Has been skipped
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
area/ci
CI/CD pipeline issues
 do-not-auto-merge
Opt out of autonomous merge-queue merging
 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 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
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#2034
Delete Branch "fix/channels-rows-err-check"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?