Session.append cannot set ignorable, so a plugin's own event type makes its sessions unloadable
#5793
Replies: 2 comments
|
This is the 9th independent report of the same write-side gap, and your mechanism analysis matches current master exactly — I verified both halves on
Prior threads from plugin authors who hit the same wall: #1538, #1584, #1619, #2778, #3191, #5463, #5474, and #5769 (a plugin whose custom session event made its log unloadable — the same delayed failure you describe). The consistent root cause across all of them: the read side honours Your proposed shape ( Two notes from the family history:
On your three questions: (1) the retention note already commits the field to staying for out-of-repo plugins, and names event production as the leg that is missing — by the repo's own accounting, a writer is wanted; (3) making |
|
Every citation in this report checks out against Implemented on a fork, since this repository takes no pull requests: a3e00b2, merged into ...opts: T extends SurfaceEventType ? [opts: SurfaceIntent<T>] : [opts?: LogIntent]
Two decisions in it worth disagreeing with if you see it differently: A surface event cannot be marked. The envelope permits the field on any type, so allowing it everywhere would have been one conditional less. But a The option carries no runtime check. The test I care about most is in Checks: On the larger alternative: a registration surface for out-of-tree event types (#3191, #4204) is the better long-term answer, and this does not compete with it — it decides ownership, versioning, and uninstall behaviour, none of which this defect needs, and all of which would sit undecided while sessions keep breaking. If a maintainer takes the registration route, this patch is five lines to drop. 这份报告里的每一处引用我都对照 已在 fork 上实现,因为本仓库不接受 pull request:a3e00b2,已并入 ...opts: T extends SurfaceEventType ? [opts: SurfaceIntent<T>] : [opts?: LogIntent]
其中有两处决定,如果你有不同看法值得提出来: surface 事件不可被标记。 信封允许该字段出现在任何类型上,全面放开可以少一个条件分支。但 该选项不带运行时检查。 我最看重的测试在 检查结果: 关于那个更大的备选方案:为树外事件类型提供注册面(#3191、#4204)是更好的长期答案,本改动并不与之竞争——注册面需要决定归属、版本化和卸载后的行为,这些都不是本缺陷所必需的,而在它们悬而未决期间会话会持续损坏。若维护者选择注册面这条路,这个补丁删掉五行即可。 |
Uh oh!
There was an error while loading. Please reload this page.
The envelope has a marker that lets a reader skip an event whose type it does not know, the restore path honours it, and the repository has committed to keeping it for out-of-repository plugins — but no plugin can write it, because
Session.appendhas nowhere to put it.Where it stops
Session.appendconstructs the event from a fixed literal (packages/core/session/src/index.ts:691-697):There is no
ignorablein it, and no way to get one in: the only optional argument is aSurfaceIntent, carryingsurfaceOpandsourceEventSeqs(:668-672, the type atpackages/core/session/src/types.ts:410-419), and on a non-surface type the compiler rejects an argument there at all.Every other leg of the field works. The envelope declares it and documents what it means (
types.ts:442-452). Seed validation allowlists the key and pins the value totrue(index.ts:213-250, the key at:226, the check at:239). The JSONL log writes the whole envelope verbatim —eventLinesstringifies the record and the only field rewritten on the way to disk issourceEventSeqs(packages/session/session-persistence-jsonl/src/format.ts:254-257,:268-271) — and the wire form declares it (packages/api/session-controller/src/types.ts:431). The restore path reads it:PersistenceCoordinatorrefuses a whole log over any event type outsideKNOWN_SESSION_EVENT_TYPESunless the event is marked (packages/session/session-persistence/src/coordinator.ts:1248-1252, the table atpackages/core/session/src/known-event-types.ts:22-74), and a contract test asserts both halves — the refusal atpackages/session/session-persistence/tests/coordinator-contract.ts:1464, the retention of a marked unknown event at:1478-1485.Only the writer is missing, and the repository says so itself: "First-party writers do not set
ignorablethroughSession.append, while a repository-external plugin is a current consumer" (.agents/notes/implemented/architecture/2026-08-10-session-log-version-mechanism.md:23). A tree sweep agrees — everyignorable: trueliteral is in a test or a fixture, and the one non-test reference is a projector passing an existing value through (packages/session/session-log-deepseek/src/index.ts:78).All references are to
dsh-v0.1.2-rc.1(a66e470).Why this matters to a composed plugin
The event vocabulary is explicitly open.
SessionEventTypeiskeyof SessionEventMapand is documented as "the appendable event-type keys ofSessionEventMap, plugin-merged extensions included" (packages/core/session/src/types.ts:365-366), so a plugin declares its own type by module augmentation andappendaccepts it.KNOWN_SESSION_EVENT_TYPESis generated from this repository's merges only, and says as much: "Downstream (out-of-repo) plugin events are outside this list by construction" (known-event-types.ts:8-22). So the moment a plugin appends its own type, every session it touched is unloadable on the next restart — and the append path will not warn, deliberately: the unknown-type guard is read-side only "because an append-time refusal would stall a live session's durability mid-flight" (coordinator.ts:768-777). A process runs all day and fails on its next load.ignorableis exactly the escape hatch for this, and the repository has already decided to keep it for exactly this consumer. The retention decision was taken after a PR removed the field "after finding no first-party producer" and a third-party plugin turned out to depend on it; the field stays until a replacement covers "event production, persistence, reload, and transport" (.agents/notes/implemented/architecture/2026-08-30-retain-ignorable-external-session-events.md:9,:11,:17). Event production is the leg that is not there.We are one of those plugins. We append one informational record of our own to a session log — a short string, written once when the session is created, read back by folding the log. Nothing in the core has to interpret it; the core only has to not refuse the log because of it.
What we could not make work without a change
2026-08-30-retain-ignorable-external-session-events.md:27).coordinator.ts:1248-1252).SessionStoresubclass. It works —SessionStore.prepareis public and non-final (index.ts:927-953) — but the price is replacing the store every session in the process is constructed by, and seeding happens once, so the record can never be superseded.ctx.sessionPersistence.appendis a public service method taking rawSessionEvent[](coordinator.ts:755;createregisters the header only), but it writes behind the liveSession's back, so the in-memory log and the artifact diverge.The change we would propose
One new options interface, and one more optional field on the call.
and one line in the event literal, spreading
{ ignorable: true }in only when the caller asked for it. A caller that passes nothing gets the literal it gets today, so a composition without such a plugin is unchanged.The diff is two files and about 30 lines, most of it documentation. It touches no schema, no persistence path, no surface validation, and no part of the restore guard — the reader already does the right thing with the marker, and this only lets a writer set it.
The one visible widening is that
optsbecomes optional on a non-surface type rather than absent. Surface metadata stays rejected there:AppendIntentdoes not declaresurfaceOp, soappend('turn/start', { turn: 1 }, { surfaceOp: 'append' })still fails to compile. We checked that specifically, along with the surface types still requiring their metadata.Two things worth flagging.
The signature is mirrored in three generated or gated places, and this diff does not update them:
packages/extensions/tool-cordis/src/api-catalog.ts:4799and:5575are generated (pnpm run gen-cordis-api), and thets type-equivblocks indocs/subsystems/session.md:298-306and:517— withdocs/subsystems/session.zh.md:307and:519— are checked byscripts/verify-type-equiv.ts. They are mechanical, and we would rather agree the shape before regenerating anything.ignorablemeans "informational", and only a writer can honestly say that. The envelope's own wording is that a writer sets it "only on purely informational records whose loss cannot affect reconstruction" (types.ts:442-452). Putting the option onappendkeeps that judgement with the writer, which is where the retention decision already put it — as opposed to any scheme that infers skippability from the type's provenance, which the same decision rejected because "a reader cannot infer that an unknown durable event is informational" (2026-08-30-retain-ignorable-external-session-events.md:25).What we are asking
ignorablesomething this repository wants to offer, or is the field meant to stay first-party-unreachable and reach plugins some other way?AppendIntentthatSurfaceIntentextends the right factoring, or would you rather putignorableonSurfaceIntentdirectly and require the argument everywhere?optsoptional rather than absent on non-surface types is the only signature widening. Is that acceptable, or would you prefer a separate method that never touchesappend's signature?We run this as a local patch over the published
0.1.2-rc.1package today, so we are not blocked on an answer. We would rather not carry a patch over a core package, and this is the smallest of the three we carry — one field, one spread, and a reader that already knows what to do with it. Happy to write it in whatever shape you would accept, or to hear that you would not, which is also a useful answer.All reactions