Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 26 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (24)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
esnible
left a comment
There was a problem hiding this comment.
This approval covers only the five commits after a9986693: c1738c03 through 4a7f0c30. That includes the gap fix 4a7f0c30, which the description's list of four doesn't name yet. It does not cover #1263, #1265 or #1266.
This PR is no longer a draft. The description says it was opened as one so it can't merge ahead of #1263/#1265/#1266. All three are still open, so please mark it as a draft again (gh pr ready --undo 1267) until they land.
withArchivedEvents' merge rule holds in every case I traced:
- a contiguous resident tail:
needis 0, so there is no disk read - a resumed session
- paging past the resident tail
- the pinned-intent gap, whether disk starts at or after the intent
- retention having pruned disk below what memory still holds: the whole-session marker stays correct
- a segment pruned or renamed mid-read
Event never answers with a neighbour. Because readSegment tolerates a truncated tail, reading the open segment is safe.
Verified locally at 4a7f0c30:
go test -racepasses oncore/session/...andcore/sessionapi- the
cmd/agentop(tui,apiclient) andcmd/cortexsuites pass go vetandgofmtare cleango list -depsconfirms the agentop binary links neithercore/session/archivenorsessionapi
The "Deferred from review" list matches what I would have raised. One nit inline.
| "github.com/rossoctl/cortex/core/pipeline" | ||
| "github.com/rossoctl/cortex/core/redact" | ||
| "github.com/rossoctl/cortex/core/session" | ||
| "github.com/rossoctl/cortex/core/session/archive" |
There was a problem hiding this comment.
nit: Importing the concrete archive package means every binary that serves the session API now links the archive. That includes cortex-envoy and cortex-cpex, which can never open one (the archive is laptop-only and only cmd/cortex opens it). I measured cortex-envoy built with the envoy profile, stripped: +194 KiB (37.89 MB to 38.09 MB). zstd was already linked there, so this is just the archive package.
A small interface in sessionapi covering Page, Event, Summaries and Stats would keep it out. That is the same reasoning pipeline.ArchiveUsage gives for keeping zstd out of agentop. Fine to defer.
a5fc0c6 to
3dc0c80
Compare
The writer goroutine publishes a read-only index (each session's directory, segment ranges and summary) at every change, and Page, Event and Summaries read it and decode segments on the caller's goroutine, so serving a page never stalls the writer. A page is newest first across segments, includes the open segment up to its last flush, holds at most limit events in memory, and reads events under the session's current id. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
A page takes what the store holds and continues from the session
archive below its oldest event, so a session resumed after a restart,
paged past its resident tail, or only on disk is read the same way;
totalEvents and oldestSeq follow the store's rule over the merged
history. /events/{seq} finds an event that is only on disk.
?archived=true adds sessions the store no longer holds, marked
resident: false, with the archive's usage. Without an archive every
endpoint is unchanged.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
H on the sessions pane toggles history. While it is on, agentop asks for /v1/sessions?archived=true, so sessions the proxy no longer holds are listed beside the resident ones. Their UPDATED cell reads "archived", in the same place cached-only rows read "cached". Enter on one of them opens its timeline through the usual snapshot, which the server now answers from disk. A proxy without an archive, or one that predates it, ignores the parameter and sends no archive object. agentop says that once, so an unchanged list is not mistaken for history being on. When the archive object is present, the first poll reports what it holds and how long it keeps sessions. Only the tests import core/session/archive, so the agentop binary links no zstd. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
The laptop-service doc gets a section on ~/.cortex/sessions: how to opt in and that it runs only on a local install. It says what the files hold (the content itself, unlike the cost ledger), the two bounds and their defaults, what a full disk or a burst costs, and how to stop it and wipe it. The agentop README documents H, the archived marker in UPDATED, and the footer as it actually renders at 100 columns. CLAUDE.md's Session Events API section covers ?archived=true and the archive object, pages that continue from disk, and seq numbering that now runs across restarts. It also covers the archive under Disabling and the reload rules, and adds a paragraph to the chatty-traffic gotcha on what the archive does and does not relieve. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
Fixes review: a pinned intent ahead of a seq gap left the gap unread from disk, and the response was marked as the whole session Files: - core/sessionapi/archive.go - core/sessionapi/archive_test.go Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
rossoctl#1266's review round made a rename move only the renamed store entry's segments, leaving earlier history under the old id. That split returns before rename reaches the unpublish/publish this branch added, so the reader index kept the moved segments and the whole summary under the old id, and had no entry for the new one. Until either id was written again, Page and Event could not find the renamed events, and ?archived=true listed the old id with the moved events still counted. Neither branch had the bug alone: it appeared when this one was rebased onto rossoctl#1266's fix rounds. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
3dc0c80 to
8101526
Compare
Fourth of five PRs for #901 (persist sessions). It reads the archive back: the session API serves history from disk, and agentop lists and opens it. The archive itself stays off by default; PR 5 adds clearing and turns it on for a local install.
What it adds
core/session/archive/read.go)Page(id, before, limit, fn)walks segments newest first. Per segment it keeps only the newestlimitqualifying events, in a ring, so memory is bounded by the page rather than the segment. Events come back under the session's current id, asrekeyLockeddoes for resident ones.Event(id, seq)returns that exact event, never a neighbour.Summaries()lists every archived session with the figuresListSessionsreports.core/sessionapi/archive.go), active only withWithArchive. Without it, every endpoint is unchanged.GET /v1/sessions/{id}follows one merge rule: memory first, then the archive below the oldest seq memory returned.totalEvents/oldestSeqfollow the store's existing rule, applied to the merged history.GET /v1/sessions/{id}/events/{seq}falls back to the archive.GET /v1/sessions?archived=trueadds disk-only sessions markedresident: false. It also returns anarchiveobject with the size and limits, plus four loss indicators, present only when nonzero: dropped events, write errors, dropped renames, and a pause after a full disk. The default list is unchanged.Hon the sessions pane toggles history.archivedin UPDATED, where cached-only rows already readcached.opages back.archiveobject. agentop says so once rather than showing an unchanged list as if history were on.[H] historysits ahead of[u].[$], the new hint pushed both cost keys out of the line at 80 columns. Ahead of[u], it is dropped before them, and a test pins that.docs/laptop-service.mdgets a~/.cortex/sessionssection covering opt-in, what the files hold, the bounds, what a burst or a full disk costs, and how to stop and wipe it.H.cmd/cortex-cpexandscripts/readme-demogainklauspost/compressas an indirect dependency, because the session API now imports the archive.Tests
read_test.gocovers:beforeboundssessionapi/archive_test.gobuilds a restarted proxy: an archive written by a previous process, with a fresh store over it. It checks:events/{seq}from disk?archived=truetui/history_test.goends with an end-to-end test using a real archive and a real session API:H, then Enter on the archived row, and the timeline arrives.Verification
cd core && go vet ./... && go test -count=1 ./..., plus-raceonsession/...,sessionapi,configandreloadercmd/cortex,cmd/cortex-envoy,cmd/agentopandcmd/cortex-praxisbuild and vetcortexand agentop suites pass, and so doesscripts/readme-demo, including its asset-staleness checkgo mod tidy -diffis clean in all 12 modulesgofmtis cleanDeferred from review
Suggestions and nits from the review of d1454ed and its fix round, left for later:
totalEventsand the list'seventCountcome from the archive's all-time count, which retention never lowers, so agentop's "N older" hint does not clear at the retained beginning.oldestSeqstill points below the failed segment;handleGetEventlogs its failures at Debug too.PageandEventdecode a segment from its start and keep its whole string table, and segments have no size cap; the read-cost wording inread.goandcmd/agentop/README.mdoverstates.showHistory,historyNoticedandarchiveUsage, so the "no session archive" notice does not fire on the next pod.archiveUsageis stored but never read, so the archive's loss indicators do not reach the user.publishruns on every archived event on the writer goroutine; publishing on tick, rotation, retire and rename would show readers the same events.limitbelowbefore; the contiguous suffix ending atbefore-1would bound the read to what memory lacks.withArchivedEvents' doc comment states only the memory half of the rule, andcontiguousBelowandmergeBySeqhave no comments.totalEvents20 for a session serving 21 distinct events, and does not exercise memory winning an equal seq or a gap disk cannot fill.Assisted-By: Claude (Anthropic AI) noreply@anthropic.com