delivery: unify background completion inbox contract - #382
Conversation
tt-a1i
left a comment
There was a problem hiding this comment.
需要改:restore 在缺 owner 时会 fail-open 进当前 Session
文档写得很清楚:缺 owner 或 owner 对不上必须进死信,不能改投到另一份 transcript。实现不是这样。
extensions/workflows 恢复 pending 时:
const storedSessionId =
envelope.details.delivery?.ownerSessionId ??
envelope.details.sessionId ??
owner?.sessionId;
if (owner && storedSessionId === owner.sessionId) {
// revive into current Session
}WorkflowDetails.sessionId 是可选的。legacy pending 如果两个存储字段都缺,第三段 fallback 就是当前 owner.sessionId,等式恒成立,旧 run 会复活进现在的 Session。
请改成:只有 ownerSessionId 或 sessionId 确实存在且相等才 rebind;否则 dead-letter。不要用当前 Session 填洞。
这是跨 Session 投递,合入前必须修。
|
已按 review 修复并推送到 |
tt-a1i
left a comment
There was a problem hiding this comment.
Exact-head review: c087cc7f9ac2e3e6f8f19120b60a6d9cc17e66a8
[P1] Conflicting persisted owner fields still restore into one of the two Sessions instead of failing closed.
In extensions/workflows/result-delivery.ts:254-257, restore resolves ownership with nullish precedence:
const storedSessionId =
envelope.details.delivery?.ownerSessionId ??
envelope.details.sessionId ??
undefined;If a persisted record has delivery.ownerSessionId = "session-1" but details.sessionId = "session-2", and the current owner is session-1, lines 262-270 accept and rewrite the record to the current epoch. The contradictory canonical fields are therefore treated as authorization rather than corruption. This can deliver a completion to a transcript even though the persisted run itself names a different Session.
Please require the two fields to agree when both are present. Missing, empty, unowned, mismatched, or conflicting ownership should dead-letter without rewriting either canonical owner field. Keep details.sessionId fallback only as the explicit legacy migration path when delivery.ownerSessionId is absent. Add a restore regression for both conflict orientations.
The previous missing-owner fallback at a873cf38 is fixed at this head; this is the remaining distinct persisted-conflict case.
tt-a1i
left a comment
There was a problem hiding this comment.
Approved at exact head 3de90fd198b7df822523e202b19716d31905b6d4 (tree 0785e2dc695bb774524c201a619eebc801e1b944).
This resolves my earlier REQUEST_CHANGES at c087cc7: restore now rejects both orientations of conflicting persisted delivery.ownerSessionId / details.sessionId, while ordinary producer delivery requires an explicit valid delivery owner and epoch. Missing, empty, unowned, cross-Session, stale-epoch, and conflicting ownership do not enter the inbox or rewrite canonical owner fields. Same-Session legacy restore remains an explicit migration boundary, and valid new-format runs still deliver normally.
I independently exercised the real defer, releaseInline, and restore paths across that ownership matrix and ran the focused completion-inbox, Workflow delivery, and Workflow E2E suites (50/50). The required Node 22, Node 24, and Windows checks are green at this head. I found no remaining P0/P1 issue in this scope.
Problem
Closes #160.
Direct Subagent, Background Terminal, and Workflow each maintained a separate pending/consume/retry gate. Their execution states should remain producer-owned, but delivery identity, Session ownership, generation fencing, and atomic consumption had drifted across three implementations.
Value
A single small contract now proves the shared delivery invariants once: completions cannot cross Session owners or generations, explicit consumption races automatic delivery through one gate, failed batches retain exact identity and order, and dead letters remain inspectable. Producer-specific lifecycle and wake behavior remain unchanged.
Approach
Validation
bun run check— passed.bun run test— 1,253 passed, 0 failed, 1 skipped; Vitest 30 passed.node scripts/run-tests.mjs— 1,253 passed, 0 failed, 1 skipped; Vitest 30 passed.Impact
ownerSessionIdandownerEpoch; legacy records restore compatibly.