Skip to content

session/database: no way for a second writer to append without invalidating the runner's session handle (write-lease OCC) #1229

Description

@mastersingh24

Summary

session/database's AppendEvent enforces optimistic concurrency on last_update_time (service.go ~L376 in v2.1.0): an append through a handle older than the row fails with stale session error. That makes every session handle an implicit exclusive write lease — any out-of-band append invalidates every other holder's handle, including a runner mid-turn, whose next streamed event append then fails and kills the turn.

The check itself is reasonable OCC. The problem is there's no affordance for a legitimate second writer, and the semantic is undocumented (discovered empirically — see also #539, which asks for exactly this kind of internals documentation).

Why this matters

Two independent downstream projects hit this in production-shaped code and converged on the same workaround:

  • go-steer/core-agent: a subagent runner writing into the parent's session row invalidated the parent runner's handle mid-stream. Documented in docs/eventlog-decisions.md ("Subagent runs in a derived session row, not the parent's"); fixed with derived session IDs (<parent>:sub:<branch>).
  • go-steer/mast: a daemon appending operator/audit markers (abort, shutdown-interruption) to a session with a live turn killed that turn; worse, the dying turn's cleanup path then erased the marker. Fixed the same way — a companion row (<sid>:mast-ops) — in go-steer/mast#49; constraint written up in docs/durable-execution-design.md.

The derived-row workaround works but has real costs: one logical session fragments across rows, listing surfaces phantom rows unless callers know the suffix scheme, queries need two lookups, and the suffix conventions are project-specific — which erodes the cross-runtime session compatibility story (a Python ADK reader has no idea :sub: or :mast-ops rows belong to their primaries).

Repro

svc, _ := database.NewSessionService(sqlite.Open("s.db")) // AutoMigrate'd
a, _ := svc.Get(ctx, &session.GetRequest{AppName: app, UserID: u, SessionID: sid})
b, _ := svc.Get(ctx, &session.GetRequest{AppName: app, UserID: u, SessionID: sid})
_ = svc.AppendEvent(ctx, b.Session, evB)      // ok — advances last_update_time
err := svc.AppendEvent(ctx, a.Session, evA)   // "stale session error: ..."

Note the in-memory service has no such check, so Service implementations diverge behaviorally — tests that pass in-memory fail on the database service, which is how both projects shipped the bug before catching it.

Request

Any one of these would remove the need for derived-row conventions (in rough preference order):

  1. Opt-in refetch-and-retry on stale — e.g. an AppendEvent option or service-level option that, on the stale check, refetches the row and retries the append. Events are append-only and StateDelta is last-write-wins, so the retry semantics are well-defined.
  2. A cheap handle-refresh APIRefresh(ctx, session) (or Get documented as the sanctioned revalidation) so long-lived holders can revalidate after a known out-of-band write, and runners could retry-once on stale.
  3. At minimum, document the write-lease semantic on Service.AppendEvent — including the in-memory/database divergence — so downstreams design for it up front instead of discovering it as a production incident.

Related: #539 (documenting session internals), #1170 (the stale error's timestamp formatting, which makes the failure harder to diagnose when it does fire).

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions