Ingest Feishu image attachments

[chengweiv5]

Summary

• download inbound Feishu image resources and persist them to Multica object storage
• bind each image as an attachment on the corresponding user chat message so agent tasks can read it
• keep the [Image] text fallback and preserve existing text/post/merge-forward handling
• cover resource download, storage normalization, and chat attachment binding with focused tests

Tests

• go test ./internal/integrations/lark ./internal/integrations/channel/... ./cmd/server
• go vet ./internal/integrations/lark ./internal/integrations/channel/... ./cmd/server
• git diff --check

[vercel]

Someone is attempting to deploy a commit to the IndexLabs Team on Vercel.

A member of the Team first needs to authorize it.

[Bohan-J]

Thanks for growing this into a real ingestion pipeline — the resolver seam, the cleanup contract with the mediaCommitted guard, and the in-transaction attachment linking are all nicely done, and the test coverage is thorough. Product direction is right. There's one blocking issue plus a related timing concern before this can merge.

Blocking: a single media failure takes down the whole Feishu connection (fail-closed)

feishuMediaResolver.Resolve runs synchronously on the connector's ACK path (router.go, step 5), and on any failure — download timeout, Lark 5xx, oversize image, or an object-storage upload error — it returns Result{}, finalizeRelease, fmt.Errorf("resolve media: %w", err).

That error propagates out of Router.Handle, whose contract (router.go:124) is that a non-nil return means an infrastructure failure the adapter should reconnect on. The Feishu connector honors that literally (ws_connector.go:396): a non-nil emit error → ACK 500 (NACK, so Lark redelivers) and return, which tears down the WebSocket and forces a Hub reconnect. Combined with Release on the claim (TestRouter_MediaTimeoutReleasesClaim), a single image that consistently exceeds the 2s budget — a large image, or a self-hosted deployment with cross-region latency to feishu.cn (which the code comments themselves call out) — becomes a redeliver → time out → NACK → reconnect loop whose blast radius is the entire installation's connection, disrupting every other message on it.

It's also strictly worse than today's behavior: on main an image just lands as an empty message; here a failed download means even the [Image] placeholder never gets appended (AppendMessage runs after the media step), so the message is lost entirely.

Media download is a high-frequency, content-dependent, externally-influenced operation — categorically different from the DB failures (ensure session / append) that legitimately warrant a reconnect. It should fail open, not closed.

Suggested fix (fail-open)

• media success: unchanged — persist, append [Image] + attachment, Mark in the same transaction, ACK 200.
• media failure/timeout: log a warning/metric with a classified reason, drop the MediaRef, and still append the original [Image] message; let the in-tx Mark stand and ACK 200 so the agent run still fires. Do not Release, and do not short-circuit to finalizeMark before the append.
• Only the core DB path (dedup / session / append) should Release + NACK + reconnect.

Related: the ACK budget is over-committed

Group enrichment is already bounded at ~2s, and media now adds its own ~2s — both serial and before the 3s Lark ACK deadline, with the DB ensure session + append + the ACK write still to come. Two independently-stackable 2s timeouts can blow the 3s window even on a merely-slow (not failing) request. Consider shortening the media timeout and having enrichment + media draw from a single shared ACK budget that reserves headroom for the DB work and the ACK write, rather than two additive 2s ceilings.

Tests

The current tests pin the behavior that needs to change — TestRouter_MediaTimeoutReleasesClaim asserts Release + no append on timeout, which is exactly the fail-closed path. After the fix I'd expect:

• engine layer: on media timeout, Handle returns nil, the [Image] message is appended, and Mark=1 / Release=0;
• connector layer: the same scenario ACKs 200 and keeps the connection up — while retaining a test that a genuine DB append failure still NACKs + reconnects;
• cleanup should use its own short timeout, with coverage for orphan cleanup when the upload partially succeeded before erroring.

Nits

• defaultMaxResourceBytes = 100<<20 (http_client.go) vs a 2s MediaTimeout (router.go) are inconsistent — 100 MB can't be downloaded and re-uploaded in 2s, so the cap advertises support the timeout can't honor (and hitting it triggers the fail-closed path above). Pick a cap that matches the budget.
• Content-Disposition filename parsing — strings.Trim(strings.TrimPrefix(h, "attachment; filename="), '"') only handles one exact format; filename*=UTF-8''…, different casing, or extra spacing yields a wrong (non-empty) value that slips past the filename == "" fallback. mime.ParseMediaType is more robust.

Longer term (follow-up, not this PR)

The most robust shape is to get media off the ACK hot path entirely: in the same transaction that appends the placeholder + marks dedup, enqueue a durable media job and ACK immediately; a worker then downloads, uploads, binds the attachment, retries by error class, and releases the session's agent run once the attachment is bound (or on terminal failure). A plain goroutine isn't enough — a crash after ACK would permanently lose the attachment, and the agent could start before the binding lands. That's a bigger change and better as its own PR; the synchronous fast-path + fail-open fallback above is the right scope here.

Requesting changes on the fail-open behavior and the ACK budget; the rest are nits. Thanks again for the solid work.

[chengweiv5]

Summary of this update:

• Feishu inbound media handling now fails open. If image download/upload times out or fails, the router logs the failure, cleans up any partial upload, drops the media reference, and still appends the original placeholder message so the connector can ACK successfully.
• Core persistence failures still fail closed. DB/session/append failures continue to return an error so the connector can NACK and reconnect, while orphan media is cleaned up with a separate short timeout.
• The ACK hot path is now bounded more tightly: the default media timeout is 750ms, cleanup uses a separate 500ms timeout, and the default resource download cap is 10 MiB.
• Resource filename parsing now uses mime.ParseMediaType, including filename*, case variations, and spacing/parameter variants.
• Regression coverage was added for media timeout fail-open behavior, partial-upload cleanup, connector ACK behavior, resource size limits, and Content-Disposition parsing.

Validation performed:

• GOCACHE=/tmp/multica-go-cache go test ./internal/integrations/channel/... ./internal/integrations/lark/...
• GOCACHE=/tmp/multica-go-cache go vet ./internal/integrations/channel/... ./internal/integrations/lark/...
• git diff --check

[chengweiv5]

Fixed the inbound image resource download route discovered during self-host validation.

• Moved file_key from the query string into the required path segment: /open-apis/im/v1/messages/{message_id}/resources/{file_key}?type=image.
• Updated the HTTP client regression test to assert the official route shape and path escaping.
• Confirmed the previous implementation produced plain HTTP 404 responses in live logs, rather than permission errors.

Validation:

• GOCACHE=/tmp/multica-go-build-cache GOPATH=/tmp/multica-gopath go test ./internal/integrations/lark/... ./internal/integrations/channel/...
• GOCACHE=/tmp/multica-go-build-cache GOPATH=/tmp/multica-gopath go vet ./internal/integrations/lark/... ./internal/integrations/channel/...
• git diff --check

[chengweiv5]

Self-host validation passed after the resource-path fix.

Tested by building and deploying upstream main at ad7b2389 plus this PR at 92052055. Sending an image to the Feishu bot now downloads and persists the message resource successfully, and the agent receives the image attachment instead of only the [Image] placeholder.

Runtime checks also passed: backend health reported database and migrations OK, the frontend returned HTTP 200, and all three Lark WebSocket connections reconnected successfully after restart.

[chengweiv5]

Addressed the remaining ACK-budget review concern in ac47877:

• Added one 2.5s inbound ACK deadline shared by enrichment and dispatch, with 250ms reserved for the ACK write.
• Reduced the default enrichment cap from 2s to 500ms; media keeps its 750ms cap but now also inherits the shared deadline, so the waits cannot stack beyond the overall budget.
• Bounded ACK/NACK writes by the same deadline.
• Added connector coverage that exhausts enrichment and the remaining dispatch/media budget, then verifies ACK 200 is sent within budget and the connection stays open. The existing infra-error test continues to verify NACK 500 and connection exit.

Validation:

• go test -count=20 ./internal/integrations/lark
• go test ./internal/integrations/channel/engine
• go test -race ./internal/integrations/lark
• go vet ./internal/integrations/lark ./internal/integrations/channel/engine

[Bohan-J]

Thanks for the continued iteration. The resource-URL fix (/open-apis/im/v1/messages/{message_id}/resources/{file_key}?type=image) looks correct, and moving toward a shared ACK budget is the right direction. There's still one blocking issue in how that shared budget interacts with the fail-open path, plus a couple of follow-ups.

🔴 Blocking: the shared processing deadline turns media fail-open back into fail-closed

The connector builds a single per-message context and reuses it for both media resolution and the DB writes:

• In ws_connector.go, processCtx is built with deadline = now + AckTimeout (2.5s) − AckWriteReserve (250ms), and that same context is passed into enrichment (context.WithTimeout(processCtx, …)) and into emit(processCtx, msg) → Router.Handle.
• In engine/router.go, media runs under context.WithTimeout(ctx, MediaTimeout = 750ms) — a child bounded by both 750ms and the parent processCtx deadline.
• On media failure, the fail-open branch logs, runs a synchronous cleanup (CleanupTimeout 500ms), and then falls through to EnsureSession(ctx, …) / AppendMessage(ctx, …) using that same processCtx — there is no fresh context and no reserved DB tail budget.

So in the budget-exhaustion case — e.g. a group message that spends most of the budget on enrichment and is then followed by a slow image — media fails not because of its own 750ms cap but because the shared parent deadline is (nearly) up. The subsequent EnsureSession / AppendMessage then run on an already-cancelled context and return context deadline exceeded. That routes to finalizeRelease + a non-nil error out of Router.Handle, which the connector treats as an infra failure: it NACKs the frame and tears down the WS connection (return fmt.Errorf("dispatch: %w", emitErr) → Hub reconnect + redelivery).

Net effect: the exact "optional media failure → connection-level failure + reconnect + redelivery" chain the fail-open change was meant to cut is reconnected whenever the shared budget is exhausted. It's narrower than before (only under budget pressure), but it's still on the ACK hot path.

Two things make it worse inside that window:

1. The synchronous 500ms cleanup on the fail-open path runs before the DB writes, actively pushing the clock past the deadline.
2. finalizeRelease calls Dedup.Release(ctx, …) with the same expired context and swallows the error (_ = …). If Release itself fails on the dead context, the claim stays held — so Lark's redelivery then hits a still-claimed dedup row, gets dropped as a duplicate, and the message is lost rather than retried.

Suggested fix

• Reserve an explicit DB tail budget before the optional media step: bound media by min(now + MediaTimeout, processDeadline − DBReserve), and if the remaining budget is already below DBReserve, skip media entirely and ingest the [Image] placeholder immediately rather than waiting for the whole processCtx to expire.
• Don't add the fail-open cleanup synchronously to the ACK hot path. Fire it with its own bounded/background context (a missed cleanup just leaves an orphan object for GC) so it can never delay the user message from landing.
• Ensure the DB writes on the fail-open path always run under a live context (the reserved tail of the budget, or a short fresh context), so an optional-media failure can never cascade into a NACK + reconnect. Ideally give Release a context that isn't the one that just expired, so the claim is actually released.

Tests

The current tests pass but happen to miss this exact boundary:

• TestRouter_MediaTimeoutIngestsPlaceholder calls Handle(context.Background(), …) — the parent has no deadline, so only the media child times out and the append always succeeds. (The fake binder also ignores ctx, so it can't observe an expired-ctx DB failure.)
• TestWSConnectorSharesAckBudgetAcrossEnrichmentAndDispatch uses a stub dispatch that waits for processCtx.Done() and then returns success, so it asserts ACK 200 on a path that never runs the real session/append.

Please add a connector-level regression that drives the real Router: let enrichment consume most of the budget, let media block until the shared budget is exhausted, let cleanup run, then assert the placeholder message is still appended, dedup is Marked in-transaction, ACK is 200, and the connection stays up — while keeping a case where a genuine core-DB failure still NACKs + reconnects.

Minor (non-blocking)

• In writeFrameBefore, writeMu.Lock() is taken before SetWriteDeadline, so a ping write holding the mutex can delay an ACK while its deadline isn't yet applied. Because before is an absolute deadline the socket write is still capped once the lock is acquired, so this is bounded — just flagging that the ACK write isn't quite as fully insulated as the surrounding comment implies.

Everything else from the previous round (10MB cap, mime.ParseMediaType for Content-Disposition, the independent cleanup timeout, and orphan cleanup on partial upload) looks good. Once the budget/context boundary above is handled so an optional-media failure can never cascade into a reconnect, this should be ready. Thanks again for sticking with it. 🔄