fix(lark): ingest inbound images and videos as chat attachments (MUL-4934)

[beastpu]

What does this PR do?

Delivers inbound Feishu/Lark images and videos to agents as real Multica chat attachments instead of leaving only [Image] or [Video] placeholders. It covers standalone media messages and media embedded in rich-text post messages.

Thinking path:

1. The adapter already preserves a human-readable placeholder, but the normalized channel envelope has no persisted media reference.
2. Routing, dedup, group-addressing, identity checks, and placeholder persistence remain synchronous; remote media I/O is queued only after the message and dedup mark are durable, keeping it off the connector ACK path.
3. Media jobs are ordered per chat session and capped by a global concurrency slot pool across sessions. Run scheduling is independent and durable: the user message persists its media deadline (channel_media_pending_until, migration 224), the chat task is created deferred with fire_at on that deadline, and media completion promotes it early — so a process crash still yields the placeholder fallback at the deadline instead of losing the run.
4. The task's deferral is re-derived from its sealed input batch inside the same enqueue transaction, closing the READ COMMITTED window where a media message committing mid-enqueue could land inside a queued task and be claimed before its attachment binds.
5. The resolver applies Lark's 100 MiB resource limit and a detached 45-second timeout. Known-length bodies stream to S3 with an explicit content length and unsigned SigV4 payload; unknown-length bodies use the bounded buffered upload path.
6. Download, upload, or attachment-binding failures remain non-fatal and retain the original placeholder text.

Related Issue

Closes #5579

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Refactor / code improvement (no behavior change)
• Documentation update
• Tests (adding or improving test coverage)
• CI / infrastructure

Changes Made

• Add an optional channel-engine MediaResolver seam and detach media processing from the connector ACK path.
• Persist a per-message media deadline (chat_message.channel_media_pending_until, migration 224) and create channel chat tasks deferred with fire_at on it; media completion promotes the task early, and the claim-path deferred promoter covers crashes.
• Seal the trailing channel-message batch to its task at enqueue, then re-derive the deferral from the sealed batch in the same transaction (DeferChatTaskForSealedPendingMedia) so a task is never claimable while its own input still has an unexpired media marker.
• Keep the post-commit media-ready fence failure out of the enqueue error path — the task is already durable, so the router must not treat it as "no task" and clear the typing indicator.
• Preserve per-session message/media ordering; bound cross-session media work with RouterConfig.MediaConcurrency (default 8).
• Split durable message append from the short database-only attachment-binding transaction.
• Download Feishu/Lark message resources through the authenticated HTTP client with a 45-second timeout and 100 MiB size safeguard.
• Support standalone image/video messages plus embedded img and media spans in rich-text posts.
• Stream known-length uploads through local or S3 storage; use fixed-length, non-seekable S3 requests and bounded buffering when the upstream length is unknown.
• Persist immutable channel provenance (chat_message.channel_ingested, migration 225), stamped inside the channel append transaction; cancel finalization gates its restore-delete on TaskHasChannelIngestedMessages over the task's sealed batch, so channel messages survive cancellation even after session archiving or installation rebinds delete the routing binding. Channel tasks settle as "Stopped." on both the synchronous and deferred finalize paths.
• Restore chat_input_task_id to a single meaning (input-batch owner): empty-completion silence and cancel provenance now both derive from channel_ingested looked up by the batch owner id, so sealed channel tasks — and their auto-retry clones, which inherit the owner while messages stay tagged with the parent — keep the Slack/Lark silent-drop contract instead of pushing the no_response fallback body externally.
• Merge upstream main and fix the collision with fix(channels): keep direct chat replies in Multica #5645's outbound gate: both Lark and Slack outbound now classify task origin via engine.TaskInputIsChannelIngested (channel provenance by batch owner) instead of chat_input_task_id presence, so sealed channel replies and failure notices still reach the platform while direct web/mobile replies stay in Multica.
• Reclaim media uploads that never gain an attachment row via a media intent ledger + reconciler (inline deletes were rejected in review as unable to order against in-flight PUTs/COMMITs): channel_media_pending_object (migrations 226-228: table, CONCURRENTLY-built unique index, primary key USING INDEX per repo convention; claim index 229) is upserted before each PUT, cleared inside the attachment-bind transaction (commit landed ⇔ intents gone, atomically), and every failure path deletes nothing inline — an independent reconciler worker claims settled rows via a pending → deleting lease state machine, checks the durable attachment reference only after the claim, deletes unreferenced objects with attempt-based backoff, and reclaims crashed workers via lease expiry. Settle delay is a fixed constant with no correctness weight (invariant-tested at ≥10× every pipeline budget); metrics cover deletes, referenced clears, failures, and backlog.
• Enforce workspace tenancy on every ledger query: the intent upsert's conflict branch only updates within the same workspace, and release/delete take (workspace_id, storage_key, lease_token) — tenancy is never derived from the key string. The reconciler is built and started only when a storage backend exists (a nil deleter would have panicked the worker on pre-existing rows).
• Fence late-materializing PUTs with a tombstone schedule: a deleted object's ledger row survives as tombstoned and is re-deleted at widening intervals (15m/1h/6h/24h) before the row is dropped, so a PUT the client abandoned that lands after the DELETE is still reclaimed — the settle delay no longer carries the PUT/DELETE race.
• Collapse duplicate img/media spans in a rich post before uploading (same derived key), and write local-storage uploads through a temp file renamed into place, so a failed re-upload can no longer destroy an already-bound object.
• Bound every reconciler object delete with its own 30s timeout (the SDK client has none) so a stalled connection cannot wedge the sweep loop; anchor the persisted media deadline to the DB clock (relative budget, now() + make_interval) so writer and readers share one clock; and bound the media queue waits by the message's own deadline — an expired queued job skips the resolver and finalizes the placeholder immediately.
• Remove the dead pre-resolved MediaRefs ingress path (a vestige of the synchronous design): channel.InboundMessage.MediaRefs is now solely ResolveMedia's output channel, always empty on ingress, attachable only through a claimed ledger intent.
• Add MediaResolver.HasMedia (pure in-memory probe on the ACK path) so only messages that actually reference platform media persist a media deadline and enter the media queue — plain text never waits behind the media semaphore and never inherits the 45s crash fallback.
• Add regression coverage for ACK latency, ordered media processing, timeout fallback, HTTP downloads, S3 streaming, message decoding, attachment binding, deferred-task promotion, the mid-enqueue media-append race (deterministic pgx.Tx interleave), the concurrency cap, channel cancel provenance across archive/unbind (queued and started tasks), orphaned-upload reclaim on deadline/bind failure, sealed-channel (and retry-clone) empty-completion silence, bidirectional outbound gating on both platforms, fault-injected lost-ack/rolled-back commits, bind-wins vs reconciler-wins interleavings, lease-expiry reclaim, delete-failure backoff, settle invariants, and the no-media fast path.

How to Test

1. Run make setup-worktree.
2. Run cd server && go test ./internal/integrations/channel/engine ./internal/integrations/lark ./internal/integrations/slack ./internal/storage ./internal/service ./internal/handler -count=1 (service/handler DB-backed tests need DATABASE_URL on a migrated database and skip otherwise).
3. Run cd server && go test -race ./internal/integrations/channel/engine ./internal/integrations/lark ./internal/storage -count=1.
4. Run cd server && go vet ./....
5. Send standalone and rich-text embedded images/videos through a bound Feishu/Lark bot and verify the resulting chat message contains attachments without delaying the connector ACK.

Checklist

• I have included a thinking path that traces from project context to this change
• I have run tests locally and they pass
• I have added or updated tests where applicable
• I have considered and documented risks above
• I will address all reviewer comments before requesting merge

Documentation and UI screenshots are not applicable because this changes the server-side inbound adapter without adding configuration or UI.

AI Disclosure

AI tool used: OpenAI Codex; follow-up review-response commits co-authored with Claude Code.

Prompt / approach: Ported a proven Feishu/Lark media-ingestion fix onto the latest upstream main, traced the adapter-to-channel-to-attachment path, then revised it from maintainer feedback to keep remote media I/O off the ACK path and make non-seekable S3 uploads explicit and testable. A self-review pass then found and fixed the mid-enqueue media-append race with a deterministic regression test, plus the enqueue error-contract and media-concurrency issues.

[vercel]

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

A member of the Team first needs to authorize it.

[beastpu]

Addressed the retry-safety and test-coverage review in 88651c3a2:

• made media object keys deterministic per workspace, installation, message, resource type, and resource key, so an append failure/redelivery overwrites the same object instead of leaking a new upload
• added Router tests proving media resolution runs for valid messages and is skipped for revoked installations, duplicates, unaddressed group messages, unbound users, and non-members
• added graceful-degradation tests for download failure, upload failure, and partial rich-post failure
• added declared-size, unknown-length overflow/exact-boundary, and timeout-cancellation tests
• added database-backed tests for attachment persistence/linking and transaction rollback on link failure

Validation: make test, targeted repeated boundary tests, targeted race tests, and go vet ./... all pass locally.

[Bohan-J]

Thanks for this — the product direction is right (delivering inbound images/videos as real chat attachments, with a graceful placeholder fallback on failure), and the retry-safe object keying, the "resolve media only after routing/dedup/identity/session checks" ordering, the in-tx attachment binding, and the engine/http-layer test coverage are all solid. The whole-message media budget (MediaTimeout capping ResolveMedia) also nicely closes the earlier multi-attachment vs. dedup-claim-window concern.

Two blocking issues remain before merge, both around where/how the download happens.

1. Media resolution runs on the synchronous pre-ACK path and can breach the Lark ~3s ACK deadline

The inbound path is synchronous on the single connection receive goroutine: WSLongConnConnector reads the frame → Enricher.Enrich (bounded by EnrichTimeout, default 2s, explicitly documented to stay "well under Lark's ~3s ACK window") → emit → Router.Handle → dispatch → processClaimed. This PR inserts resolveMedia (up to MediaTimeout = 2.5s) synchronously between EnsureSession and AppendMessage, and the ACK frame (NewAckFrame(frame, true)) is only written after Handle returns.

So worst-case pre-ACK latency becomes enrich (≤2s) + media (≤2.5s) + identity/session/append DB time, which can exceed 3s. Even a standalone image with no enrichment is media 2.5s + DB, right at the edge. When the ACK is late, Lark redelivers the event; the 60s dedup guard keeps that from double-processing, but repeated late ACKs risk long-connection churn/reconnects — the exact thing the codebase avoids elsewhere by detaching the reply/typing calls.

Note the 2.5s default matches ReplyTimeout, but ReplyTimeout runs on a detached goroutine off the ACK path, whereas media runs on it — they can't share the same budget. Suggested direction: move media resolution off the pre-ACK path (e.g. append the message first so it's durable, then download/upload asynchronously and link the attachment to the persisted message before the debounced run flush), or bound it strictly within the ACK headroom left after enrichment + DB and reconcile it with EnrichTimeout.

2. S3Storage.UploadStream streams an unknown-length, non-seekable reader into PutObject — likely rejected by real S3/MinIO, and untested

UploadStream passes the download body straight into s3.PutObject as Body with no ContentLength set (the existing Upload uses a seekable, known-length bytes.NewReader). With aws-sdk-go-v2 v1.97's default RequestChecksumCalculation=WhenSupported, PutObject auto-selects CRC32 and takes the aws-chunked trailing-checksum path; for an unknown-length body it emits Transfer-Encoding: chunked without x-amz-decoded-content-length, which real S3 (and MinIO) require for aws-chunked uploads and will reject at the service.

Because media_ingest.go's uploadResource prefers the streaming path whenever the backend implements it (both local and S3 do), S3-backed deployments would hit this failing path — and since upload failure is non-fatal, media would silently never attach in production. The tests only exercise a fake storage (io.ReadAll), so S3Storage.UploadStream has no real-backend coverage and CI stays green.

The download side usually already knows the size (resp.ContentLength), but it isn't threaded into UploadStream. Fix options: set ContentLength on PutObjectInput (plumb the known size through), buffer into a bytes.Reader like the existing Upload, or use feature/s3/manager.Uploader. Please validate against a real S3/MinIO before merge.

Nits

• maxMessageResourceBytes is 100 MiB, but the ~2.5s synchronous window realistically only fits a few MiB of download+upload. Since the headline case is MP4 video, anything moderately large will hit the timeout and fall back to the [Video] placeholder. The 100 MiB cap only becomes meaningful once media moves off the ACK path (see #1).
• The PR description mentions a "20 MiB limit", but the code now uses 100 MiB — worth updating.
• looksLikeJSON in http_client.go is defined but unused (the actual detection is the inline strings.Contains(contentType, "json") check) — dead code, can be removed.

[beastpu]

Addressed both blocking issues, plus a race a follow-up self-review pass surfaced. Changes since the review, in order:

1. Media resolution is off the pre-ACK path entirely (d90a46db5, 11e1a32f5)

Handle now returns right after append + dedup mark + run scheduling; download/upload/binding run on a detached per-session queue. Run scheduling no longer waits on media in-process — it is durable instead: the user message persists channel_media_pending_until (migration 202), the chat task is created deferred with fire_at on that deadline, and media completion promotes it early via PromoteChannelChatTasksIfMediaReady. A crash between append and binding still yields the placeholder run at the deadline through the existing claim-path deferred promoter. TestRouter_MediaResolutionDoesNotBlockInboundHandle pins the ACK-path contract; TestEnqueueChatTaskDefersForChannelMediaAndPromotesWhenReady pins defer → promote end-to-end. With media off the ACK path, the 100 MiB cap is now real (and the stale 20 MiB mention in the description is fixed).

2. S3Storage.UploadStream sends a fixed-length request (d90a46db5)

ContentLength is now required (sizeBytes <= 0 errors out), the payload is signed as UNSIGNED-PAYLOAD so the non-seekable body never needs a rewind, and RequestChecksumCalculationWhenRequired keeps the SDK off the aws-chunked trailing-checksum path. TestS3StorageUploadStreamUsesFixedLengthRequest asserts against a live httptest S3 endpoint that the request arrives with Content-Length set and no Transfer-Encoding: chunked. Unknown-length bodies fall back to the bounded buffered Upload path. looksLikeJSON dead code removed.

3. Follow-up: mid-enqueue media-append race (a8cb5c136)

Self-review found a READ COMMITTED window in EnqueueChatTask: a media message committing between the session-wide deadline read and the input-batch seal got sealed into a task the deadline read had already decided was queued, so the daemon could claim it before its attachment bound (and the media-ready promotion would no-op against a non-deferred task). Fixed by re-deriving the deferral from the sealed batch inside the same transaction (DeferChatTaskForSealedPendingMedia). TestEnqueueChatTaskDefersWhenMediaMessageCommitsDuringEnqueue reproduces the interleave deterministically via a wrapped pgx.Tx and failed before the fix.

4. Follow-up: enqueue error contract + concurrency bound (b397289ca, 9df4fd1e9)

A post-commit promote-fence failure no longer surfaces as an enqueue error (the task is already durable; the router would otherwise clear the typing indicator and log a spurious failure). Cross-session media work is now capped by RouterConfig.MediaConcurrency (default 8) to bound burst memory (unknown-length uploads can buffer up to 100 MiB each) and Lark download pressure; per-session ordering is unchanged, covered by TestRouter_MediaConcurrencyCapAppliesAcrossSessions.

Validation: engine/lark/slack/storage/service/handler package tests (including the DB-backed ones on a freshly migrated database), -race on engine and service, and go vet all pass locally.

[Bohan-J]

Reviewed at 07731828bbfcacd9669a6edf0ceeeb96c0dc0f63. Both blockers from the previous round are properly closed:

• ACK path — media work is out of Handle entirely now. The inbound path routes, persists the message, and returns; download/upload/binding run on a detached queue, with channel_media_pending_until + a deferred task providing a durable 45s fallback. The enqueue-vs-append and clear-vs-create races are each fenced (in-tx re-derivation via DeferChatTaskForSealedPendingMedia, post-commit PromoteChannelChatTasksIfMediaReady), and the new router/concurrency tests cover the boundaries.
• S3 streaming — ContentLength is now threaded through, the optional trailing-checksum/aws-chunked path is disabled, and unknown-length bodies fall back to a size-capped buffered upload, with TestS3StorageUploadStreamUsesFixedLengthRequest pinning the request shape for a non-seekable reader. The dead looksLikeJSON and the stale 20 MiB figure are gone too.

The new persistence design is a sound direction. Two issues in it still block merge — both are about production data lifecycle rather than the happy path.

1. Cancel-path provenance is derived from a deletable binding, so archived sessions lose inbound channel messages

EnqueueChatTask now sets a self input-owner and seals the inbound user message into the task via LinkUnownedChannelChatMessagesToTask, so channel messages carry a task_id. To stop cancellation from treating those as web drafts, finalizeCancelledChatMessage (and FinalizeDeferredCancelledChat) gate the restore-delete on ChatSessionHasChannelBinding.

The problem is that a binding is not an immutable fact about where a message came from — it only says a binding exists right now. Archiving a session deletes it (handler/chat.go, the req.Archived branch calls DeleteChannelChatSessionBindingBySession), and installation reclaim/rebind deletes bindings while deliberately preserving the chat_session rows for history (DeleteChannelChatSessionBindingsByInstallation in pkg/db/queries/channel.sql). So this sequence loses data:

1. A Feishu/Slack message is ingested and sealed into a task.
2. The session is archived (or the installation is rebound) → binding deleted.
3. The still-queued/running task is cancelled.
4. ChatSessionHasChannelBinding returns false → restorable = true → DeleteUserChatMessageByTask deletes the original inbound message.

This is a regression introduced here: before this PR the channel message carried no task_id, so that delete could never match it. The sender has no composer for the "restored draft" to land in, so the message is simply gone.

Please persist channel provenance as an immutable property of the message or task (not something a routing binding implies) and gate the cancel path on that. Worth a regression test covering ingest → archive/unbind → cancel, for both a queued and a started task.

2. Media timeout / bind failure leaves unreclaimable object-storage orphans

In resolveAndBindMedia, when the deadline fires the already-resolved refs are dropped wholesale (resolved.MediaRefs = nil) — the comment there acknowledges those refs "may already sit in object storage" — and a BindMedia failure is likewise only logged as a warning. In both cases the uploaded objects have no attachment row, so nothing can enumerate them later during workspace/session deletion.

The deterministic-object-key rationale (redelivery overwrites the same key) no longer applies under the new ordering: dedup is marked in-tx alongside the chat message before the detached media work runs, so a redelivered event is dropped as a duplicate and the resolver never runs again for it. That means a multi-resource rich post where the first uploads succeed and a later one times out, or any upload that succeeds before the bind fails, leaves objects behind permanently — up to the per-resource cap each, with no aggregate bound across messages.

Please add explicit compensation for "uploaded but not bound": either delete by StorageKey on timeout/bind failure, or persist the pending objects for a reaper to reclaim. A test asserting that a partial-upload + deadline/bind-failure run leaves nothing outside the attachment table would lock this in.

Nits

• handler/daemon.go (~L2112) still says "Legacy and channel (Slack/Lark) tasks carry a NULL owner and keep the trailing-message selector" — new channel tasks now get a self owner, so this will mislead the next reader about the rolling-deploy branch.
• content_flatten.go (~L25) still says "Attachment ingestion is explicitly out of scope (tracked as a separate attachment-pipeline issue)" — that's what this PR implements.
• feishu_types.go MediaRefs doc says "feishuChannel fills these before handing the normalized message to the channel engine", which predates the detached resolver; media is now bound after the message is appended.

CI is green (6/6) and the branch merges cleanly, so these are the remaining items from my side.

[beastpu]

Addressed both blockers, plus two interactions a follow-up pass surfaced. Reviewed at a707af10a.

1. Cancel provenance is now immutable (8e7cfbc27): migration 204 adds chat_message.channel_ingested, stamped inside the channel append transaction and never mutated. Both cancel finalize paths gate on TaskHasChannelIngestedMessages over the task's sealed batch (keyed by the batch owner id, so retry clones inherit the verdict); the binding-existence check is gone. Regression tests cover ingest → archive/unbind → cancel for queued and started tasks.

2. Uploaded-but-unbound objects are reclaimed (fc07261d0, a707af10a): MediaResolver.DiscardMedia deletes by StorageKey on deadline expiry and bind failure, running on a fresh context after bind/promotion so a bind that failed on the expired finalize budget can't hand the deletes a dead context.

3. chat_input_task_id no longer doubles as origin (8b04669a4, f5e61b122): sealing gave channel tasks an input owner, which broke both empty-completion silence and — after merging main — #5645's outbound gate (every channel reply would have been classified as direct and dropped). Both now derive origin from the channel_ingested provenance via engine.TaskInputIsChannelIngested; tests pin both directions on both platforms, including the retry-clone case.

Also merged main (migrations renumbered to 203/204 after 202 was taken) and fixed the three stale comments.

Two known residuals, both bounded: a rolling-deploy window where messages appended by a pre-upgrade replica carry no stamp (transient, minutes), and a crash between upload and bind still orphans objects (inherent to the delete-on-failure option; a reaper would close it if you'd prefer that instead).

Validation: full go test ./... on a freshly migrated database, -race on engine/lark/slack, go vet, CI green.

[Bohan-J]

Reviewed at a707af10a302f226fb9b58355e26e367bf6e5366. Both blockers from the previous round are correctly closed:

• Cancel-path provenance — migration adds an immutable chat_message.channel_ingested stamp, written inside the message transaction on channel append. Cancel / deferred-cancel / retry-clone now gate the restore-delete on TaskHasChannelIngestedMessages(chatInputOwnerID(task)) — the sealed input batch keyed by chat_input_task_id — instead of the deletable channel_chat_session_binding. Retry clones inherit the owner, so they land on the same discriminator. Regression tests cover ingest → unbind → cancel for both queued and started tasks.
• Known-ref compensation — on deadline or bind failure the already-uploaded refs are collected and deleted by StorageKey (DiscardMedia) under a fresh budget, and the stale comments are updated.
• The post-#5645 merge also stops discriminating direct vs. channel outbound on chat_input_task_id presence, so a sealed channel reply isn't silently dropped.

One issue remains, in the compensation protocol itself: it treats "the call returned an error" as "the side effect definitely did not happen." Both the S3 upload and the DB COMMIT have a result-uncertain window where that assumption is wrong, and the two failure modes corrupt state in opposite directions.

Must-fix — wrong compensation for result-uncertain uploads / commits

1. Upload error → permanent orphan (no crash required). In media_ingest.go, the resolve loop only appends the (deterministic, already-known) StorageKey to MediaRefs when uploadResource returns nil; on error it continues. But PutObject can succeed server-side and still surface a client error — a lost response, a dropped connection, or the media context deadline firing after the object was written. The key never reaches the router, so DiscardMedia is never called for it; and because dedup is now committed in-tx with the chat message before the detached media work runs, the redelivered event is dropped as a duplicate and the resolver never runs again. So an ordinary timeout leaves a permanent, unreclaimable object (up to the per-resource cap), with no aggregate bound across messages.

2. Commit error → deletion of a live, referenced object. ChatSession.BindMediaRefs returns the tx.Commit(...) error verbatim as a bind failure, and resolveAndBindMedia responds to any bind failure by adding every ref to discardRefs and deleting them. But a Commit that returns an error is not a guarantee that the transaction rolled back — a lost commit ack, or the finalize context expiring mid-commit, can return an error while Postgres has durably committed the attachment + link rows. The discard then runs on a fresh context (by design, so it survives a finalize-deadline bind failure — which is exactly the ambiguous-commit case) and deletes the underlying objects. The result is a persisted attachment row pointing at an object that no longer exists — a broken attachment the user will hit.

Please make the compensation distinguish "known-failed" from "unknown-result":

• Upload error path: retain the attempted StorageKey and idempotently delete it under a fresh context (the key is deterministic and already computed, so this is cheap).
• Unknown commit result: before deleting, verify the durable link by message/key (or an equivalent persisted identifier) and only discard when no attachment references the object; otherwise keep it.
• If a reliable decision isn't feasible inline, a persisted pending/outbox row reclaimed by a reaper converges both directions.

Two fault-injection tests would lock this down: (a) storage accepted the object but the client got an upload error → assert no orphan; (b) the DB committed but the client got a commit error → assert the bound object is not deleted.

CI is green (6/6) and the branch merges cleanly, so this is the one item left from my side.

[beastpu]

Rebased on today's main and renumbered the full migration group to 224–229 (ea41afea3); verified the way you did — TestMigrationNumericPrefixesStayUniqueAfterLegacySet now runs against the merged tree itself since main (through 223) is merged into the branch, plus a from-scratch apply of the whole set. CI is green on that basis, not a stale snapshot.

One more hardening found in a self-review pass (c73b75628): the batch claim shared a single 2-minute lease across up to 50 sequentially-processed rows, so a few deletes running at their full 30s timeout could outlive the lease mid-batch and let another replica reclaim the tail — duplicate deletes and inflated attempt/backoff. The lease is now heartbeated before each row's settle (invariant-tested at ≥2× the per-delete timeout), and a lost renewal skips the row to its new owner. Test simulates the mid-batch reclaim.

On the late-PUT residual: a design that fences it exists (post-PUT ownership re-confirmation + the uploader's own ordered self-delete, shrinking the settle assumption to only client-aborted PUTs materializing >15min after abort), but given the object store's bounded request lifetime we've judged the current exposure acceptable and left it unimplemented — happy to add it if you'd rather close that too.

[Bohan-J]

Reviewed at c73b75628de46b7c696e11cdc683ce973e0c3598. The intent-ledger + reconciler is now substantially in place and gets most of the hard parts right: the intent row is written before the PUT; bind only recognizes and deletes a pending row inside the attachment transaction; once the reconciler claims a key (deleting) neither upload nor bind can resurrect or link it; object-store I/O is kept out of DB transactions; failed DELETEs retain the row and back off; and lease tokens, expiry reclaim, tenant predicates, metrics, and the standalone worker are all there. Migrations rebased cleanly to 224–229 with per-statement CREATE INDEX CONCURRENTLY and no FKs/cascades. This is close.

Two blocking issues remain.

1. The late/aborted PUT still isn't fenced — settle is carrying correctness weight the design says it must not

channel_media_reconciler.go states, as the load-bearing property, that the settle delay "carries NO correctness weight" and that correctness comes from the ledger state machine. But the state machine only fences bind vs. DELETE (via the deleting claim that upload/bind check before resurrecting a key). It does not fence a PUT that is still in flight at the object store. The upload-error path deliberately leaves the pending row and returns (its own comment notes "the store may still be processing the PUT"), handing the row to the reconciler. So this sequence still leaks:

1. Resolver writes pending, starts the PUT; the client times out / disconnects and gets an error.
2. After the settle delay the reconciler claims deleting, finds no attachment, DELETEs the key, and clears the ledger row.
3. The original PUT then durably lands at the store — now an object with no ledger row and no attachment, which nothing will ever reclaim.

The only thing preventing step 3 is the assumption that a client-aborted PUT won't materialize after the settle window — i.e. settle is the correctness barrier for the PUT/DELETE race, which contradicts the invariant the file asserts. This is the same residual acknowledged in the design discussion, just relocated into the reconciler.

To close it, the uncertain PUT needs a durable fence that survives the ledger row's deletion: e.g. keep a tombstone / re-delete step keyed by the object so a late materialization is caught, or upload under a staging key reclaimed by a store lifecycle rule and only promote on confirmed bind. Whatever the mechanism, add an interleaving test where the DELETE completes first and the original PUT lands afterward, asserting no surviving object.

2. Duplicate rich-post resources can produce a dangling (or duplicate) attachment on LocalStorage

mediaResourcesFromPost in media_ingest.go appends every img/media span with no dedup, so a post that references the same image_key/file_key twice yields two resources with the same deterministic mediaObjectKey. Combined with LocalStorage.UploadStream, which os.Creates the destination (truncating any existing file) and os.Removes it if the copy errors, this happens:

1. First span uploads successfully → file written, ref added to MediaRefs.
2. Second span (same key) os.Create-truncates that just-written file, then its download stream errors mid-copy → os.Remove deletes the file entirely.
3. The first ref is still in MediaRefs, so bind creates an attachment row pointing at a file that no longer exists — a dangling attachment.

If instead the second upload succeeds, you get two refs with the same key/URL and two attachment rows for one object (duplicate attachment).

Fix: dedup/reuse by deterministic storage key before entering the upload loop, and make LocalStorage.UploadStream write to a temp file in the same directory and atomically rename into place, so a failed write can never destroy an already-successful object. A regression test with a duplicated span (first upload succeeds, second stream fails) would lock this in.

Non-blocking

The reconciler claims a batch of 50 rows under one 2-minute lease and then processes serially. The per-row heartbeat correctly stops a stale owner from continuing, but it doesn't stop a tail row from repeatedly expiring and being reclaimed (bumping attempt) before its turn comes up — the new test even asserts a tail row going from attempt 1 to 2. Consider per-row (on-demand) claiming or bounded concurrency so rows that have never actually had a DELETE attempted don't accrue attempt/backoff. Not a blocker.

CI is green (6/6) and the branch merges cleanly. Fix the two above and it should be in good shape for a final pass.

[beastpu]

Both blockers addressed at 58f8ff492.

1. Late/aborted PUT is now fenced by a tombstone schedule, not by settle (4c42d96f0). After deleting an unreferenced object the ledger row survives as tombstoned and the object is re-deleted at widening intervals (15m/1h/6h/24h, pass index carried in last_error); only then is the row dropped. A PUT that materializes after the DELETE is therefore reclaimed by a later pass — escaping now needs the object to appear more than ~31h after the client abandoned it, rather than resting on a single settle-window bet. Claim/heartbeat/lease/tenancy predicates are unchanged (a tombstone is claimed like any other due row), bind and the intent upsert both still refuse anything that has left pending, tombstones get their own gauge so they can't read as reclaim backlog, and the header comment now states plainly what state fences (bind/commit, no timing assumption) versus what the schedule fences. Your interleaving is the test: DELETE completes, the abandoned PUT materializes right after, and no object survives the schedule.

While re-checking that change I found it had a hole of its own, fixed in 58f8ff492: a tombstone revisit still ran the first-settle reference check, so an attachment carrying the same URL (a re-ingested copy) sent the row down the "referenced → keep object, clear row" branch and abandoned the schedule fencing the original object. Tombstones now go straight to the delete + schedule tail, with a test pinning that a later same-URL attachment doesn't stop the re-deletes.

2. Duplicate post resources and local overwrite (4c42d96f0). Duplicate img/media spans are collapsed by (fetch type, platform key) before the upload loop, and LocalStorage.UploadStream now writes a temp file in the destination directory and renames it into place, so a failed write only discards its own temp file. Tests: a duplicated span downloads/uploads once and yields one ref; a failed re-upload leaves the previous object byte-identical with no temp files behind.

The non-blocking batch/lease point is noted as a follow-up — per-row on-demand claiming so a tail row can't accrue attempt/backoff before its first delete is even tried.

Validation: full affected packages, -race on service/engine, go vet ./..., migrations applied from scratch on the merged tree, CI green.

[Bohan-J]

Reviewed at 58f8ff492bc99c123b129d681ea8b16db649e10c. Both items from the previous round are properly closed: rich-post resources are deduped by (fetch type, platform key) before the upload loop, and LocalStorage.UploadStream now writes to a temp file and atomically renames, so a failed second upload can no longer destroy a prior success (with a regression test that pins it). The late-PUT case is also no longer resting on a single 15-minute settle — the row is kept as a tombstoned entry and re-deleted on a widening 15m/1h/6h/24h schedule, with claim, lease, tenant predicate, and metrics intact. Good progress.

Two new must-fixes, both in the material added this round.

1. The tombstone re-delete deletes the object even when an attachment references it — encoding the one outcome the ledger exists to prevent

On a tombstone revisit, settle in channel_media_reconciler.go skips the reference check and calls settleDeletedObject directly, and TestChannelMediaReconciler_TombstoneIgnoresLaterAttachmentWithSameURL pins this: it creates an attachment carrying the row's URL and asserts the object is deleted anyway. Since the storage key/URL is the same string whether it came from the abandoned PUT or a later write, deleting it removes the only object that attachment can read — a dangling attachment, which is precisely the failure the whole intent-ledger was built to avoid, and it contradicts the principle used everywhere else here ("a reclaimable orphan beats a broken attachment").

Worth being precise about reachability, because it points at the right fix: the ledger PK is storage_key, mediaObjectKey is per-(message, resource), and dedup blocks redelivery of the same message — so a legitimate same-URL rebind during the tombstone window is effectively unreachable. That's the argument for the opposite posture, not this one: a tombstone pass that finds a durable attachment reference is observing a state that "can't happen," i.e. a broken invariant — and the safe response to that is to keep the object and surface an anomaly, never to delete it and manufacture the broken attachment. The comment justifies the delete as "the attachment belongs to a re-ingested copy," but with a deterministic key a re-ingest can't even create a second pending row (PK conflict) or attach to the tombstoned one, so that copy can't get an attachment in the first place.

So: keep the reference check on tombstone passes and treat a positive result as keep-and-alert rather than delete. If re-ingestion at the same key is genuinely intended, the keys must be generation/version-scoped so an old tombstone can only ever delete its own generation — deleting a current, referenced object is not an acceptable branch either way. Please flip the test's contract to assert the referenced object survives.

2. The atomic-write temp file becomes an untracked orphan on a crash

The temp-file + rename change is right for the mid-copy-failure case. But LocalStorage.UploadStream names the temp file with os.CreateTemp(..., "."+base+".tmp-*") — a random suffix. The error paths remove it, but a process crash between CreateTemp and Rename leaves it behind, and nothing can reclaim it: the ledger records only the final storage_key, and DeleteObject removes only the final file and its sidecar. Each such file can approach the 100 MiB cap, and repeated crashes accumulate with no bound. (S3 is unaffected; this is local-storage only.)

Make the temp path deterministic from the storage key so DeleteObject/the reconciler can clean it, or add a safe stale-temp sweep — a defer os.Remove can't cover a crash. A regression test that plants a leftover crash-temp and asserts the reconcile/delete path removes it would lock it in.

Verification

CI is green (6/6), migrations 224–229 keep unique prefixes with per-statement CREATE INDEX CONCURRENTLY and no FKs/cascades, and the branch merges cleanly.

The ledger is close. These two are bounded fixes — a posture change on the tombstone reference check, and bringing the crash-temp into durable cleanup — and after them it should be ready for a final pass.

[beastpu]

Both must-fixes are in, at e7c737566. Two things to flag up front: a self-review pass between your review and this push changed one of the premises your first item reasons from, and it also found that the tombstone I shipped last round had a bug of its own.

1. The tombstone re-delete no longer deletes a referenced object (5d4002c05). You're right, and the posture argument is right: the reference check now runs on every pass, tombstones included. A positive result on a tombstone is treated as the broken invariant it is — the object is kept, the row is cleared, an error-level log carries the key/URL/pass, and a dedicated multica_channel_media_reconciler_tombstone_referenced_total counter makes it alertable rather than silent. The test's contract is flipped: it now asserts the referenced object survives (delete calls stay at 1), the row clears, and the anomaly counter reads 1. I re-applied the old skip-the-check branch to confirm the flipped test fails against it.

2. The keys are now generation-scoped too (911349398, pushed before your review landed, so it isn't a response to it). Your reachability paragraph assumes the key is per (platform message, resource) — that was true at 58f8ff49, and it also made a re-ingest impossible for a different reason than the one you cite: the intent upsert refuses any row that has left pending, so a redelivered message would find the tombstone and skip its upload entirely, producing a placeholder with no media for as long as the schedule runs (~31h). That is reachable — the inbound dedup claim is reclaimable once 60s stale and the dedup row is only vacuumed after 24h, so a crash before processed_at plus a later redelivery hits it. The key is now derived from the chat message the object attaches to, which is exactly the generation scoping you named as the alternative: an old tombstone can only ever delete its own generation. Both fixes are in — the scoping removes the reachability, the reference check removes the branch.

3. The staging file is reclaimable after a crash (ce894b248). The temp path is now derived from the storage key (.<name>.tmp), and DeleteObject removes it alongside the object and its sidecar. That is what makes the ledger reclaim it: the intent row is written before the upload, so the reconciler always reaches this delete for an abandoned key, and the crash leftover goes with it. Regression test plants a leftover in the crash shape (staging file, no object, no sidecar) and asserts the delete path removes it — it fails if the temp path is dropped from DeleteObject. Both read paths also refuse the staging name, since keys come straight from the request URL and a half-written body should not be readable; the predicate is "dot-prefixed basename ending in .tmp", so a user uploading notes.tmp (key <uuid>.tmp) is unaffected, with a test for that too. The name being stable per key means two concurrent writers of the same key would share it; that does not occur here (media keys are per (chat message, resource) and resolved once, every other path mints a fresh UUID key) and such writers already raced on the destination itself.

Correction to my previous comment. I wrote that the tombstone's pass index is carried in last_error. It was, and that was a bug: the failure path writes last_error too, so one failed re-delete erased the position and restarted the walk — with an intermittently failing store the row would never reach the end of the schedule and never be dropped. It is now its own tombstone_pass column, advanced only by a successful delete (374a9fce9), with a test that walks the schedule across a failed re-delete and asserts it resumes at the next pass rather than restarting.

Also in this batch: LocalStorage.Upload (the buffered path taken for unknown-length bodies) went through the same temp-file rename, so the truncate-up-front shape can't come back through the other door, and both paths land 0644 as the direct write did.

Merge mechanics. main took 224 (agent_task_session_rollout_missing), so the ledger group is renumbered to 225–230 with the in-file cross-references updated, and the three CompleteTask call sites this PR adds pass the new sessionRolloutMissing argument (e7c737566).

Worth noting for your next pass: git diff 58f8ff49..HEAD on channel.sql touches only the tombstone write (pass column in, last_error cleared — the delete that got there succeeded, so any earlier failure text is stale). The claim, lease renew, release, bind-claim, reference and count queries are byte-identical to what you verified, so the predicates you checked still hold.

The non-blocking batch/lease point is still open as a follow-up.

Validation: full server suite against a database migrated from scratch on the merged tree (225–230 apply cleanly), -race on service/storage/lark/channel, go vet ./..., CI green.

[Bohan-J]

Heads-up: I pushed one small commit to this branch (958b2fc) to drop gofmt-only churn that was unrelated to the media pipeline.

Six files carried whitespace / comment-reformatting with no functional change, so I reverted them to the base revision:

• server/internal/service/empty_claim_cache.go
• server/internal/integrations/lark/markdown_detect.go
• server/internal/integrations/lark/ws_chunk_assembler.go
• server/internal/integrations/lark/ws_chunk_assembler_test.go
• server/internal/integrations/lark/ws_frame_test.go
• server/internal/integrations/lark/registration_test.go

git diff -w against those files was already empty, so nothing behavioral changed; it just takes the PR from 75 to 69 files and makes the diff easier to review. go vet is clean and the tests covering those files pass. It's a fast-forward, so a git pull should be all you need.

Likely cause is a format-on-save / repo-wide gofmt run picking up files this change doesn't otherwise touch — worth keeping an eye on so it doesn't come back.

One related nit I left alone rather than deciding for you: server/internal/integrations/lark/ws_endpoint.go only reorders the strconv / strings imports (also unrelated to this PR). Happy either way — revert it or keep it, your call.

[Bohan-J]

Merged — thank you for sticking with this one. 🙏

This went through eight review rounds and two architecture reworks, on something that started as "let the agent see an image." I want to be straight that most of that growth came from our review bar, not from you over-scoping: you shipped a working media pipeline early, and then we kept pulling on the thread behind it — first getting the download off the Lark ACK path, then the S3 fixed-length upload, then immutable provenance so cancellation couldn't delete an inbound message, and finally the two-system atomicity problem between object storage and Postgres. That last one is genuinely hard, and it wasn't visible from the original issue.

A few things you did that made this land well:

• You asked before building. Bringing the intent-ledger design up with concrete open questions — scheduler parameters, where the worker runs, which ledger fields ops would need — meant we settled the shape before you wrote 600 lines of it. That saved a round rather than costing one.
• You reworked the architecture twice rather than patching around the problem: media off the ACK path, then the ledger + reconciler when it became clear inline compensation could never answer "did my side effect actually happen?"
• You took the tombstone posture correction properly. Flipping to keep-and-alert on a durable reference — treating a "can't happen" state as a broken invariant instead of something to delete through — is the right instinct for data-lifecycle code.
• The fault-injection tests are the real deliverable. Lost commit ack, late-materializing PUT, crash-orphaned temp file: those interleavings are the kind of thing that normally ships as a production incident. Roughly half the diff is tests, which is the only reason reviewing this was tractable.

One optional follow-up, non-blocking and already noted: the reconciler claims a batch of rows under a single lease and processes them serially, so a tail row can expire and be reclaimed before its turn, inflating its attempt/backoff without a DELETE ever having been tried. Per-row claiming or bounded concurrency would tidy that up if you feel like a small PR.

I also pushed one commit of my own to drop unrelated gofmt churn before merge — nothing behavioral, just to keep the diff reviewable. Thanks again for the patience and the care; Feishu/Lark users get working image and video handling out of this.