feat(storage): preserve session continuity at scale - #645
Conversation
Define the verification contract for bounded snapshots, indexed inventory, migration, cleanup, retention, and legacy adoption. Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Map every acceptance criterion to an issue-aligned worker task and dependency wave. Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Record the first worker branch and move the acceptance plan into progress. Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Separate snapshot format, durability, and cross-process temp ownership into dependency-sized subagent tasks. Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Repeated focused subagents exhausted their runtime before producing a test or implementation edit. Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Reset the infrastructure-only failure and dispatch with the inherited unbounded worker budget. Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Record the merged v2 snapshot core and release the first dependent durability task. Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Start the second wave-one subagent on crash and disk-failure guarantees. Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Record the merged crash-durable replacement implementation and release cross-process temp locking. Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Start the final issue-586 worker on cross-process temp ownership and recovery. Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Record the merged cross-process family lock and orphan-temp recovery implementation. Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Separate catalog rebuild, cursor-bounded pagination, and lock decomposition into subagent-sized tasks. Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Start the catalog foundation subagent after the snapshot format wave completed. Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Record the merged rebuildable catalog and release bounded pagination. Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Start the cursor and page-work subagent over the rebuilt catalog. Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Record the merged generation-bound cursor and work-bounded catalog paging implementation. Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Start the final indexed-inventory worker on lock decomposition and reconciliation. Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Coordinate same-family mutations through the stable family flock while isolating catalog rebuild and generation reconciliation from session operations. Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Record the merged lock decomposition and release progressive UI plus maintenance server waves. Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
Co-Authored-By: Juan Antonio Osorio <ozz@stacklok.com>
manual_dream (already shipped on main, #609) and the new storage_health field both landed on field 19, with manual_dream pushed to 23. That's a wire-compatibility break: a mismatched client/server pair would misread one field as the other (message vs bool, different json_name). Restore manual_dream = 19 and give storage_health the next free number (23) instead. Verified with `buf breaking --against origin/main` (now clean) and regenerated code. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…scope/continuation SessionMetadataCursor's new Generation/Scope/Continuation fields were required non-empty on both the client and server side whenever a cursor was present at all, with no capability negotiation. That breaks pagination in both directions during a mixed-version deployment: a version-skewed peer that predates these fields never populates them, so its cursor was rejected outright as invalid instead of degrading. The domain layer already models an empty-generation cursor as legitimate (port.PaginateSessionMetadata's unbound path), and a bound pager already turns a genuine mismatch into ErrSessionMetadataCursorRestart rather than a hard failure. Add legacyOrBoundCursorFields and use it everywhere a cursor triple is checked: accept all-empty (legacy) or all-non-empty (bound), reject only a partial mix (which would indicate real corruption). A version-skewed peer now degrades to a pagination restart instead of a rejected request. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…al cross-check
PlanAutomaticRetention (server.cleanup.go) was a same-package passthrough
to PlanManualRetention with zero production callers — the real automatic
sweep (internal/app/childgc.go's childGC.sweep) calls
sessionretention.Plan directly and never touches it. The parity test
that was supposed to back AC5.5 ("automatic sweeps and manual plans
select the same candidates") called PlanManualRetention under two names
and asserted it equals itself; it could never catch a real divergence
between the two call sites.
Delete the unused shim and its self-referential test. Add a genuine
cross-check in internal/app/childgc_test.go that reads the store's rows,
runs the REAL childGC.sweep, then separately asks
server.PlanManualRetention over the same pre-sweep rows/policy/liveness
and asserts the two selections match.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…oundaries PlanSessionCleanup and 9 sibling migration/health/cleanup methods were classified KindCallerOwned with rationale implying per-principal data scoping. In fact these are store-wide admin-authority operations gated by an allowlist (PlanSessionCleanup plans with owner=nil over the whole store, for example) — only the resulting job/plan/token handle is caller-bound, not the underlying data. The access control itself is sound; the self-documentation an auditor would consult to reason about blast radius was not. Rewrite the 10 rationale strings to say what is actually decided, and add a doc-comment note on KindCallerOwned naming this management-authority sub-case explicitly (a genuinely separate AccessKind would need its own ADR per ADR 0212's closed 4-kind taxonomy; not done here). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…neration-increment invariant Comment-only. Two things a future maintainer needs to know before touching these Lua scripts: - saveMetadataScript/deleteMetadataScript/conditionalDeleteMetadataScript/ adoptMetadataScript all build the owner-index key by string concatenation inside the script rather than declaring it in KEYS[]. Harmless today (only a single-node/Sentinel redis.Client exists), but a Redis Cluster upgrade can't be a client swap — these four scripts need hash-tagged keys or KEYS-array key building first. - The exact-coverage proof in migration.go's verify*Coverage functions depends on every mutator of the metadata sorted sets either incrementing metadataRebuildGenerationKey or being a deliberate, reviewed exemption (adoptMetadataScript, whose writes happen under the fenced migration lock before the verification window opens). Nothing structurally enforces this today; a future fifth mutator that skips it would silently degrade the proof to "probably fine." Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Five reuse findings from the code-duplication review, all in the
storage-core area:
- memstore's encodedBytesLen now calls base64.StdEncoding.EncodedLen
instead of reimplementing the formula.
- port.CompareSessionMetadataOrder replaces three independent hand-rolled
"ModifiedAt desc, ID asc" comparators (port/store.go, jsonlstore's
inventory_catalog.go and metalist.go) with one shared comparator.
Verified algebraically equivalent at all three sites.
- session.PrincipalScopeHash replaces three duplicated
sha256(issuer+subject) computations with one shared raw-hash primitive;
each call site keeps its own nil-check/prefix/truncation logic on top,
so on-disk/wire output is byte-identical to before.
- jsonlstore's writeInventoryFileWithPattern now delegates to the
existing replaceCurrentSnapshot durable-write helper (generalized to
take a temp pattern, an optional zero modifiedAt, and an optional
*os.Root) instead of duplicating the atomic-write dance. Uses
defaultSnapshotOps(), not the store's fault-injectable ops: the
inventory-catalog write must stay invisible to snapshot-write fault
injection, since advanceInventoryGeneration runs before the snapshot
write it's meant to test in both Save and MigrateSessionFamily — using
the injectable ops here made an injected post-rename fault abort
before the snapshot rename it's supposed to exercise, and
misclassified ENOSPC during migration as backend_failure instead of
insufficient_space. Caught by an independent review pass.
- storage_health.go's isSessionStorageFile now uses the existing named
suffix constants instead of hardcoded string literals.
port.CompareSessionMetadataOrder and session.PrincipalScopeHash are new
exported engine API surface (Added/minor per COMPATIBILITY.md) —
engine/api/{port,session}.txt regenerated via task api:update and
engine/CHANGELOG.md updated.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…lidation reuse findings - schedulestore.go's toString gained a []byte case; redisResultString (a near-identical duplicate) is deleted and its ~12 call sites in metadata_index.go/migration.go repointed to toString. - grpcdriver's pageMetadataRequest now calls the existing validPortMetadataCursor instead of duplicating its check inline; the separate timestamp-validity check is untouched. - gofmt -w on metadata_index.go (a trailing-blank-line violation from the redisResultString deletion, plus a pre-existing const-block alignment issue picked up as a side effect). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
planner.go used an unstable sort.Slice comparator (also a latent correctness gap: the equivalent code this package's diff replaced in childgc.go deliberately used SortStableFunc for deterministic cap eviction on tied timestamps) and a hand-rolled linear scan for membership. Replace with slices.SortStableFunc + cmp.Or/cmp.Compare (same ascending ModifiedAt/ID ordering, now genuinely stable) and slices.ContainsFunc. Also switches two plain-string sorts (payload.Live/Leased) to slices.Sort. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… of duplicating them - Sessions list reused the existing scrollWindow helper (already shared by the slash palette, @-mention menu, and /models picker) instead of a hand-rolled persisted-scroll field + keepSessionCursorVisible. Note: this changes the actual scroll behavior from sticky-edge to cursor-centered, matching models.go's existing convention — a real UX change, not purely internal. - Deleted the duplicate binary-unit formatBytes (KiB/MiB) in favor of widening the existing decimal-unit humanizeBytes (int -> int64, added GB/TB steps); its one existing call site and test updated. Storage panels now render decimal (MB) instead of binary (MiB) units — also user-visible, not purely internal, but consistent with humanizeBytes's existing "reconciles with how sizes are reported everywhere" rationale. - Refreshed 2 golden files (verified the diffs are byte-format text only, e.g. "6.0 MiB" -> "6.3 MB", arithmetic checked) and fixed 2 test assertions that hardcoded the old KiB output. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… page PaginateSessionMetadataBound computed its cursor-restart "generation" by JSON-marshalling and SHA-256-hashing the whole filtered row set on every call, making page N cost O(total matching sessions) instead of O(page size) -- a real violation of AC2.1's page-work-bounded contract on memstore, the one adapter that calls this helper (jsonlstore and redisstore each maintain their own real persistent generation counter and don't use it). Change the signature to take the caller's own cheap, monotonic generation signal instead of deriving one from row content. memstore now tracks a real counter, bumped on Save/Delete, matching how jsonlstore's inventory generation and redisstore's metadataRebuildGenerationKey already work (a single global counter, not per-owner -- owner-scope mismatches are still caught separately by the Scope field). Verified against the existing RunMetadataPager conformance test, which already exercises restart-on-mutation. New exported engine API surface change (Added/minor per COMPATIBILITY.md) -- engine/api/port.txt regenerated, engine/CHANGELOG.md updated. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e suite
Neither jsonlstore nor redisstore's port.SessionMigrationStore
implementation was checked against a shared cross-adapter conformance
suite, unlike the other optional capabilities (RunMetadataPager,
RunConditionalPrunable already exist). Each adapter's migration tests
were written independently, with real risk of behavioral drift on
edge cases neither adapter's own author thought to test.
RunSessionMigration exercises the common contract: acquisition-gated
mutation (every mutating/ownership call rejects on an unbound
context), cross-acquisition exclusion (a second Acquire on the same
job id fails while the first is held), tampered-fingerprint handling
(a "changed" skip, never a silent overwrite), and job-checkpoint
round-tripping via Save/Load.
seedMigratable is a caller-supplied hook rather than something the
suite derives generically: neither adapter's ordinary Save produces a
migratable ("not yet current format") candidate, so each adapter's own
test supplies its existing internal seeding mechanism (jsonlstore's
seedMigrationV1; redisstore seeding directly into miniredis before
construction, since its metadata-index-ready state is fixed at open
time -- a real adapter difference the test comment documents).
Along the way: found and worked around a real behavior worth knowing --
jsonlstore's flock-based AcquireSessionMigrationJob retries until the
CALLER's context is done rather than failing fast on contention, so
the suite's contended-acquire check needs a bounded context (matching
what a real caller should already be doing).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two low-risk mechanical findings from the code-duplication review: - newCleanupID (cleanup.go) and newMigrationHandle (migration.go), both in internal/adapter/server, generated a 16-byte crypto/rand hex ID identically. Extracted one shared randomHexID(n int) helper; both callers preserve their exact 16-byte length. - internal/app's parseRetentionDuration and permconfig's validateRetentionDuration implemented identical duration-parsing logic (empty/"0" = disabled, else a non-negative time.ParseDuration value). internal/app already depends on internal/adapter/permconfig, so permconfig now exports ParseRetentionDuration and both callers use it -- validateRetentionDuration discards the parsed value, internal/app's wrapper keeps its existing path-qualified error text. No behavior change; no test pinned the discarded internal error strings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Post-panel-review fixes (12 commits)Ran a full multi-agent panel review (Spec/Standards/Domain axes) over this PR, then addressed everything actionable. Summary of what changed and why: Ship-blockers
Judgement calls
Mechanical/reuse dedup (5 commits)
One regression surfaced and fixed during this dedup pass (caught by an independent review, not by CI): Correctness/coverage gaps found by the panel review
VerificationEvery commit above passed its own targeted tests plus a full |
Stale relative to the engine/CHANGELOG.md edits in the two preceding review-fix commits (task docs:llms wasn't re-run after those). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ting.T CI's golangci-lint (revive unused-parameter) caught what my local go vet/gofmt sweep didn't: the redisstore seed hook's *testing.T param is genuinely unused (the row is seeded before construction, per the comment above it). Rename to _. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ew of the last stretch Three findings from a fresh Opus review pass over the memstore-pager fix and the new SessionMigrationStore conformance suite -- none are correctness bugs in the product, but each is real: - engine/adapter/storeconformance/storeconformance.go: the two post-release assertions in RunSessionMigration used the same boundCtx that release() may itself cancel (redisstore's release cancels the acquisition's operation context), so a passing test proved nothing about the adapter's OWN ownership fence -- it could pass on ctx-cancellation alone. Verified by mutation: with BOTH of redisstore's real fences (the Go-side lost check and the Lua token compare) temporarily removed, the test still passed before this fix, and correctly fails after it. Wrap both assertions in context.WithoutCancel(boundCtx). - engine/port/store.go: PaginateSessionMetadataBound silently degraded to unbound pagination when handed an empty generation, which would let a stale cursor mix rows instead of restarting -- exactly the failure ErrSessionMetadataCursorRestart exists to prevent. Reject an empty generation with an error instead. - Softened the "O(total rows) instead of O(page size)" claim in the doc comment and CHANGELOG: prepareSessionMetadataRows's row copy/sort is still O(rows) per page either way, so this was a large constant-factor win (dropping a JSON encode + SHA-256 of every row), not an asymptotic one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
Verification
task linttask testtask docstask ac-trace-stricttask site:buildgo run ./cmd/mecademoCloses #583
Closes #586
Closes #587
Closes #588
Closes #589
Closes #590
Closes #591
Closes #592
Closes #593
Closes #594
Closes #595
Closes #596