Repository navigation
Conversation
A session file with no session_end record was indistinguishable from a session whose run was still in progress, because a running review has not written session_end yet. A writing process now holds an exclusive OS lock on a <session-id>.lock sidecar for the life of the run, so the kernel - not a heuristic over timestamps - decides whether a session is alive. The lock is released however the process ends, so a crashed or killed run needs no cleanup to read as finished. Close joins Finalize for a run that is abandoned rather than completed: it releases the file handles and the lock without claiming completion.
The viewer reported every session without a session_end as "aborted", which described every review still in progress as well as every one that had died. Status now consults the liveness lock first, so an in-flight run reads as running and a dead one still reads as aborted. Duration is computed in the browser from the start timestamp, because a run that has not written session_end has no final duration to report. The page reloads itself once the run ends, so the terminal figures always come from the server rather than from a guess made client-side. The repository list shows a running badge linking straight to the live session, and both pages poll while a run is in flight.
…ssion # Conflicts: # go.mod
The repository list and the session list now carry a status, so say what the states mean: a session with a live process behind it is running, and one with neither a session_end record nor a live process is aborted. Also record the lock sidecar in the on-disk layout, since a reader who looks for it will find it there.
The list test simulated an abandoned run by closing the session file directly, which left the liveness sidecar locked. An open handle cannot be deleted on Windows, so the test's temp dir cleanup failed there. Closing the session the way an abandoned run does releases the lock too.
os.UserHomeDir() prefers USERPROFILE on Windows, so setting HOME alone left the sessions these tests create in the real profile instead of the temp dir they inspect. The session package already redirects both; the viewer tests now do the same.
|
🔍 OpenCodeReview found 6 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| summary.Running = SessionIsActive(path) | ||
| return summary, items, nil |
There was a problem hiding this comment.
Performance: SessionIsActive is called unconditionally, even for sessions that have a session_end record (where Aborted is already false). For repositories with many completed sessions, this means an unnecessary file-open + lock-attempt syscall per session.
The viewer's peekSession in store.go avoids this by only probing when Running is still true after parsing (i.e., no session_end was found). The same guard should be applied here: only probe when the session lacks a session_end, since a completed session cannot be running.
Suggestion:
| summary.Running = SessionIsActive(path) | |
| return summary, items, nil | |
| if summary.Aborted { | |
| summary.Running = SessionIsActive(path) | |
| } | |
| return summary, items, nil |
| summary.Running = SessionIsActive(path) | ||
| return summary, nil |
There was a problem hiding this comment.
Same performance issue as in LoadDetail: SessionIsActive is called unconditionally. Since this function is called in a loop from ListSessions (line 133), the cost multiplies with the number of sessions. Guard with if summary.Aborted to skip the lock probe for completed sessions.
Suggestion:
| summary.Running = SessionIsActive(path) | |
| return summary, nil | |
| if summary.Aborted { | |
| summary.Running = SessionIsActive(path) | |
| } | |
| return summary, nil |
| const marker = document.querySelector("[data-repo-active]"); | ||
| if (!marker) return; | ||
| if (document.hidden) return; |
There was a problem hiding this comment.
Bug: When no repository has an active session at page load time (marker is null), this early return prevents any polling from being set up. As a result, if a review starts after the page was loaded, the user will never see the new "running" badge appear without a manual refresh.
This contradicts the function's documented purpose: "a run that was not running when the page loaded has started. Reloading then is what puts the badge and the new session on screen."
The fix should remove the if (!marker) return; guard and instead always poll, comparing the current server state against the initial state (which may be "no active repo"). For example, capture the initial state as const initiallyActive = !!document.querySelector("[data-repo-active]"); and then compare hasActiveRepo(html) !== initiallyActive inside the interval.
| if (document.hidden) return; | ||
|
|
||
| setInterval(async () => { |
There was a problem hiding this comment.
Bug: If the repos page is loaded in a background tab (document.hidden === true at load time), this early return prevents the polling interval from ever being created. When the user later switches to the tab, no polling will occur and they won't see newly started reviews.
Compare with watchForCompletion, which correctly only checks document.hidden inside the interval callback (line 60) rather than before creating the timer.
Remove this guard so the interval is always set up; the existing document.hidden check inside the callback (line 103) already handles background-tab throttling.
| .session-status.status-running { color: var(--link); } | ||
| .session-status.status-aborted { color: var(--badge-duration-fg); } | ||
| .session-status.status-legacy { color: var(--text-muted); font-weight: 500; } | ||
| .session-status.status-complete { color: var(--link); } | ||
| .session-status.status-failed { color: var(--danger); } |
There was a problem hiding this comment.
Missing CSS rules: The session-status template in sessions.html can generate classes like status-partial and status-skipped (via the {{else}} fallback status-{{.TerminalState}}), but no corresponding .session-status.status-partial or .session-status.status-skipped rules exist in the stylesheet. These statuses will fall back to the generic .session-status color (--text-secondary) instead of having visually distinct styling consistent with the meta bar (where status-partial uses --badge-duration-fg).
Consider adding:
.session-status.status-partial { color: var(--badge-duration-fg); }
.session-status.status-skipped { color: var(--text-muted); font-weight: 500; }| // first keeps the cost at one open per *candidate* rather than one per | ||
| // archived session, which is what makes this safe for a repository with | ||
| // thousands of them. | ||
| hasSidecar := make(map[string]bool, len(subEntries)) |
There was a problem hiding this comment.
The map is pre-allocated with capacity for all directory entries, but only .lock files are inserted. In a repository with thousands of archived sessions and no active runs (the common case), this over-allocates significantly. Consider counting lock files first or using a smaller initial capacity, since the typical number of concurrent locks per repo is 0 or 1.
The lock handle carried a field nothing read, and two declarations used var where the rest of the file uses const. The live status also needed its own regression coverage: a session whose lock is held reads as running, and one whose lock is gone reads as aborted. The rest of the review round — the agent handoff's deep links and its menu — is the other half of the split and stays with it.
A session that has written its end record cannot be running, and the probe costs an open and a lock attempt. The listing asked for every session, so a repository with thousands of finished ones paid that for each of them. Two smaller things the review turned up: the sidecar map was sized for the whole directory rather than for the locks it holds, and the two statuses the template can produce had no styling of their own.
fix(tool): report an empty code_comment batch instead of success (alibaba#1773)
Description
The viewer reported every review still in progress as aborted.
Aborteddefaulted to
trueand was cleared only by asession_endrecord — but arunning review has not written
session_endyet, so the field really meant"no
session_end", conflating in-progress with dead.This PR separates them using an exclusive OS lock the writing process holds on
a
<session-id>.locksidecar for the life of the run, so the kernel decidesliveness. The viewer then reports running ahead of aborted, and a
session that is still running shows a duration that ticks up in the browser.
Verified in a real browser, not just tests: three session fixtures rendered
through the actual handlers showed
runningfor the locked session,abortedfor the crashed one,legacyfor the finished one.This is the session-liveness half of #1638, which was closed over an unsigned
CLA. The other half — the collapse-all control and the per-finding handoff
into Claude Code / Codex / Cursor — is #1782, and the two do not overlap.
What each commit does
feat(session)Closefor an abandoned runfeat(viewer)runningahead ofaborted; tick duration client-side; reload on completiondocsviewer.mdin all five localesEach builds and passes tests standalone — verified by checking out every
commit individually.
One thing worth reviewing closely
The liveness mechanism. A sidecar file plus an OS lock, rather than an
mtime heuristic. The kernel releases the lock however the process dies, so a
crash needs no cleanup step. The tradeoff: the writer changes, and Windows
needs
LockFileExrather thanflock. Worth a look at whether you wouldrather have the heuristic and keep the writer untouched.
What the review changed
The branch was reviewed with the project's own tool, and the findings were
verified rather than taken on trust. Two were already fixed by the time the
review ran — the reviewer's line numbers pointed at the pre-fix code — and four
were real:
returned early when no repository was active, then compared against a marker
that was truthy on every path reaching it, so it only ever noticed runs
ending. Opening the viewer and only then starting a review — the ordinary
workflow, and the one this feature exists for — showed no badge at all. It
now captures the initial state and compares against the server each poll, in
both directions.
function checked
document.hiddenbefore creating the interval rather thaninside it, so the tab that needed the badge most was the one that would
never look for it. The check moved inside the interval, where the rest of
the file already had it.
session_endrecord cannot be running, and the probe costs an open and alock attempt. The listing now asks only for the sessions that could still be
live, which is what makes it cheap for a repository with thousands of
finished sessions.
partialandskippedfell back to the generic colour instead of being distinguishablefrom the meta bar.
The liveness mechanism also lost a field nothing read, and two declarations
used
varwhere the rest of the file usesconst.The first two of those are the findings from the review round on the combined
branch, which is why the fixes are separate commits: the regression and its
fix are each reviewable on their own.
Type of Change
How Has This Been Tested?
make testpasses locallymake check,make test(0 failures) andmake coverage(91.4%, threshold90%) all pass. Cross-compiles clean for linux, darwin and windows.
Manual testing:
running/aborted/legacyrespectively.New tests cover the lock (held, released, missing sidecar, idempotent close)
and the status rendering.
Notes for reviewers
them shows as
action_requiredwith no jobs at all, which is GitHub holdinga fork pull request's workflows until a maintainer approves them — the
Checks tab offers "Approve and run workflows". Nothing has failed and
nothing is waiting for a runner: the jobs have not started. The same ground
is covered locally by
make check,make testandmake coverage, whichare what the CI
testjob runs, and the repository's ownAnalyze (go)andwindowsjobs pass on the fork.Checklist
go fmt,go vet)AI disclosure
Tool: Claude Code, model
claude-fable-5.Every commit was reviewed and rewritten by hand before being pushed, and the
diff has been read against the request below rather than accepted as the
tool produced it.
I understand every line above and can answer questions on the reasoning
without consulting the tool.
The request this answers
This is the verbatim request. It named three symptoms — a live session shown
as aborted, status and duration disagreeing with the CLI, and duration
computed from the start timestamp in JS. It also contained a misremembering
("version command"); I asked and it meant the viewer, not
ocr version.One design decision I raised rather than assumed, answered before
implementing: how to distinguish a live run from a crashed one. The answer was
a lock file over an mtime heuristic, because the kernel's answer to "is this
process alive" does not need a cleanup step to be right.
Related Issues
🤖 Generated with Claude Code