Repository navigation
fix(usage): keep a running sign-in's credential home taken across a server restart (#2313) - #2325
Conversation
…erver restart (#2313) The one-login-per-home lock lived only in the server's memory, while the login runs in a herdr tab that outlives the server. The service now keeps each holder (home, pane, CLI pid) in <stateDir>/usage-logins.json and, after a restart, holds a home only while herdr still shows that login: its pane running that pid, or that pid as the shell of a moved pane. A pid alone is no proof; herdr errors keep the home taken. Closes #2313
…rvers on one state dir (#2313) A pane ID and a pid could not tell a login apart after a restart: herdr hands pane IDs out again, a move changes the pane ID (racing the pid's capture), and a pid can name any process after a reboot. herdr's terminal ID stays with the tab when it moves and is never reused, so a holder is now that terminal, and one session snapshot says whether its login still runs. Servers on one state dir read each other's holders, and a server writes back the others' entries. The file is written with writePrivateState.
…er the file's guard (#2313) Each server now changes usage-logins.json only through one guarded read-modify-write (withOwnerGuard), adding or removing only entries that are its own (by launch ID or terminal), so a stale view of another server's holder is never written back and concurrent saves cannot drop one. A launch is recorded in the file before anything is awaited, with its server's pid and start time, so another server refuses that home while the tab is still opening; a launch whose server is gone or that is older than a minute no longer holds. The restart test hears the pane's exit on its own herdr subscription, and the send-failure test waits for its retry's CLI, which raced the next test's look at the fixture home.
…keep sign-ins in memory without the file (#2313) reserve() released a holder it found ended and looked again; when the file could not be written, it found the same holder every time, a microtask loop that starved the server. It now replaces that holder with its own launch in the guarded step that finds it unchanged. When the file's guard cannot be taken or the file cannot be written, the server logs it and keeps its sign-ins in memory as before, rather than refusing every sign-in. The two-servers test settles its held request on failure.
…rvers (#2313) Keeping sign-ins in memory when the state file could not be written or its guard not taken or released let a server start a sign-in no other server could see, beside theirs. Taking a home fails the request again, without a tab; freeing a home and recording the opened tab stay best effort. A refused write now ends that request once instead of looking again forever.
Review record (omo-native-code-reviewer + Codex Sol 6.1 fast xhigh)This PR touches a sign-in lock (access control) and is reviewed by both agent reviewers. Findings and how each was resolved, by round: Round 1 (aa39905, pane ID + pid holders)
Round 2 (95a1f61)
Round 3 (b8d5535)
Round 4 (df463b3)
Round 5 (fd0a106)
LOWs not fixed are listed under Known limits in the PR body. Local verification on fd0a106: |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
 |
| Layer / File(s) | Summary |
|---|---|
Holder state and persistence server/usage-login.ts, server/index.ts |
The service tracks launch and login holders in usage-logins.json under the server state directory. |
Holder liveness and reservation server/usage-login.ts |
The service checks local and persisted holders before reserving a credential home. Active holders block a reservation; ended holders can be released or replaced if unchanged. |
Login transitions and contract tests server/usage-login.ts, server/usage-login.contract.test.ts |
Login requests create launch holders and replace them with terminal holders when tabs open. Tests cover server restarts, concurrent servers, stale pane IDs, persistence failures, and retries. |
Changelog changelog.d/usage-login-restart-lock.md |
The changelog describes the open-login behavior across a server restart. |
Priority:
Estimated code review effort: 4 (Complex) | ~40 minutes
Change: Bug fix · Severity of issue fixed: Medium
Possibly related PRs
- devswha/ovis#2309: Its account-keyed Sign in again path uses the reservation behavior changed here.
- devswha/ovis#1931: It changes state-directory resolution, which determines where this change stores reservation state.
Merge Risk: 
b187b
-
This change keeps a running sign-in's credential home reserved across a server restart, using a shared state file. No actionable merge-blocking risk remains. The limits listed in the PR description are accepted tradeoffs.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly and concisely describes the main change: preserving a running sign-in's credential-home reservation across a server restart. Linked Issues check For #2313,UsageLoginServicepersists reservations inusage-logins.jsonandcreateServerassigns the state directory. The reservation records the herdr terminal ID and remains active while that …Out of Scope Changes check The changed implementation, server wiring, contract tests, and changelog support #2313. The tests validate persistence, concurrency, failure handling, and terminal identity. No unrelated product behav…Description check The description follows the template's two sections, "Change" and "Validation", and goes well beyond them. The Change section states the problem (in-memory lock lost on Ovis restart with a sign-in tab… Full details: Linked Issues check
-
Explanation
For
#2313,UsageLoginServicepersists reservations inusage-logins.jsonandcreateServerassigns the state directory. The reservation records the herdr terminal ID and remains active while that terminal appears insessionSnapshot(). Guarded read-modify-write operations protect shared state across server instances. The added contract tests cover restart persistence, concurrent servers, ended logins, failed writes, and reused pane IDs. These changes keep the interactive login running and reject a second login withlogin_in_progress.
Full details: Out of Scope Changes check
-
Explanation
The changed implementation, server wiring, contract tests, and changelog support
#2313. The tests validate persistence, concurrency, failure handling, and terminal identity. No unrelated product behavior or unrelated files are included in the reported changes.
Full details: Description check
-
Explanation
The description follows the template's two sections, "Change" and "Validation", and goes well beyond them. The Change section states the problem (in-memory lock lost on Ovis restart with a sign-in tab still open, allowing a second CLI login into the same credential home), the fix (persistent holders in usage-logins.json with terminal IDs, guarded read-modify-write, fail-closed behavior), and the resulting behavior (409 while the login runs, 200 after it ends). It links the issue (Closes
#2313). The Validation section lists the new contract tests, the owner-guard test, bun run check fast, and manual QA with two real server processes. The template asks for contract-test coverage for HTTP/WS changes, and the PR covers it. The description is long and dense, but it is complete against the template.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 3 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
…ose tab was not recorded (#2313) The merge pass's strict review found two fail-open paths and a LOW: - a usage-logins.json that would not read or parse counted as empty, so after a restart a second login started beside the first; only a missing file is empty now, any other refuses with 409 login_state_unreadable; - when recording the opened tab as the home's holder failed, the login was still sent under a launch entry that expires; the tab is now closed and the request fails unless that record is written and still ours; - the owner guard followed a planted symlink and created its target; it is opened with O_NOFOLLOW. An Add account home made for a login that was never sent is removed on every early return, not only on a thrown error.
Merge pass strict review of b187b88: resolved in 4600ca6Each finding has a test that fails on b187b88 and passes on 4600ca6.
Verification on 4600ca6: |
… written (#2313) writePrivateState answers directorySynced: false when the directory fsync after the rename fails (EIO): the record may be gone after a crash. The merge pass found the tab's holder record accepted that way, after which the login was sent. Outside Windows, which syncs no directory, such a write now fails like a refused one: the claim or tab record is taken back where it landed, the tab is closed and the request fails.
Merge pass: directorySynced:false after the holder rename, resolved in 0600228
Test: closes the tab and sends no login when the record of that tab as the home's holder is not synced ( Verification on 0600228: |
|
Merging 0600228 (closes #2313). A sign-in started from Ovis keeps its credential home taken across a server restart: the holder is recorded in a private state file ( |
Closes #2313
What
Sign in again and Add account allow one login per credential home (#2309), but the lock lived only in the server's memory, while the login itself runs in a herdr tab that outlives the server. After an Ovis restart (update, crash, manual restart) with a sign-in tab still open, the same account could start a second CLI login into the same home, and the two raced each other's tokens.
Now
UsageLoginServicekeeps each holder in<stateDir>/usage-logins.json(viawritePrivateState: 0600, fsync, atomic rename), andcreateServerpoints it at its state dir. A holder is the home key, the tab's pane ID and the tab's herdr terminal ID (tabCreate'sroot_pane.terminal_id), saved as soon as the tab opens, so a restart mid-launch is covered too.A home stays taken while that terminal is in herdr's session snapshot, in any tab:
scripts/herdr-restart-regression.ts), while their pane IDs repeat. Pane IDs and pids are therefore not used as proof: a restarted herdr reuses pane IDs, and a pid can name anything after a reboot.exec), or the shell exits after it (Windows).409 login_in_progressas feat(usage): sign in again to an expired Claude or Codex account #2309, whose message says to finish or close the tab).Servers on one state dir share the file as one record (a source
bun run serverbeside the installed app, say). Each change is one read-modify-write under the file's kernel guard (withOwnerGuard,server/owner-guard.ts), and a server adds or removes only its own entries, matched by launch ID or terminal, and takes over another's only in the step that finds it unchanged and ended; it never writes back a stale view of others. Taking a home fails closed: when the guard cannot be taken or released, or the file cannot be written, the request fails without opening a tab (a sign-in other servers cannot see could run beside another). The login command is sent only after the file names the opened tab as the home's holder; if that write fails (or another server took the entry over), the tab is closed and the request fails, so no login runs under a launch entry that expires. A state file that exists but cannot be read or parsed refuses sign-ins (409 login_state_unreadable, saying to close any open sign-in tab and repair or remove the file); only a missing file counts as empty. Freeing a home is best effort and logged: an entry left behind holds only until a server finds it ended. The guard file is opened without following a symlink (O_NOFOLLOW,server/owner-guard.ts), so a planted link cannot make it create a file elsewhere. A launch is recorded in the file before anything is awaited (its server's pid, its start time and a random ID), so another server refuses that home while the tab is still opening; a launch whose server is gone, or older than 60 s (a launch opens its tab within the request's 30 s), no longer holds. This replaces #2309's in-memory pane + pid check with one snapshot check (no per-panepane.process_infofan-out). No HTTP contract change.Tests
server/usage-login.contract.test.ts, seven new cases, and one inserver/owner-guard.test.ts:409; after the first stops, a restarted server answers409withtabCreatecalled once; once herdr says the login's pane exited,200. On main the restarted server answers200and opens a second tab.409, the first opens its tab,tabCreatecalled once. Fails on a version that keeps launches in memory only (200twice).ENOSPC): one write attempt,500, no tab; once writes work,200. A version that released and looked again re-read the same launch for every refused write (a microtask loop that starved the server); one that swallowed the write started a sign-in no other server could see.{broken; mode 000):409 login_state_unreadable, no tab, no credential home left. Both answered200before.ENOSPCon the second write):500, the tab closed, no command sent to it, the file empty again. Answered200and ran the CLI before.*.guardsymlink; nothing runs and the target is not created.The restart test hears the pane's exit on its own herdr subscription: the shared test server hears no exit before the file's first (
beforeAll), so a filtered run would miss it. The whole file passed in repeated runs, also filtered to the new cases;bun run check fastpassed.Manual QA
The real entrypoint (
bun run server/index.ts) twice on oneOVIS_STATE_DIR, against a throwaway headless herdr session, with a stubclaudeCLI: server A opened the sign-in tab (200,w1:p1, holderterm_65ef2e58586051written); A stopped (port free); server B (new pid, bound the port itself) answered409 login_in_progressfor the same account while the CLI still ran; after the login finished, B answered200and wrote the new tab's terminal. Everything torn down afterwards (no process, port or temp dir left).Known limits (LOW, not fixed here)
server/state-merge.ts) does not carryusage-logins.json: a sign-in tab open during an install's one-time switch betweenherdr-web-uiandovisstate dirs is not seen by the next server.paneSendTextfails after herdr already delivered the command, the sign-in runs while the home is freed. Follow-up.409says so. The file is new in this PR, so no older release writes one.ended(paneId)frees this server's holder by pane ID, so if a moved login's old pane ID is handed to a new pane and that pane exits, the home is freed while the CLI runs.bun:ffi), every sign-in now fails with500instead of starting; managed installs already need that guard at startup, so this affects only a hand-run server on such a host.Reviews
omo-native-code-reviewer and Codex Sol 6.1 fast xhigh, five rounds: both APPROVE on fd0a106; the merge pass's strict review of b187b88 then found two fail-open MEDIUMs (a damaged state file read as empty; the tab's record failing after the login was sent) and a LOW (guard symlink), fixed in the next head. Every HIGH and MEDIUM was fixed; the record of findings and resolutions is in a PR comment. The LOWs left are the known limits above.
Remote bundle
Changes
server/only; noREMOTE_BUNDLE_VERSIONchange (the release PR raises it).