Skip to content

fix(daemon): align ps session selection with inventory - #1186

Merged
mvschwarz merged 3 commits into
mvschwarz:mainfrom
XTLine:fix/session-projection-current
Oct 12, 2026
Merged

mvschwarz merged 3 commits into
mvschwarz:mainfrom
XTLine:fix/session-projection-current

Conversation

@XTLine

@XTLine XTLine commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

What a user gets

rig ps now uses the same current, effectively running registrations for its running count and uptime. After a backward wall-clock adjustment within a running daemon, the summary no longer disagrees with node inventory or subtracts a confirmed-missing replacement from a healthy sibling's count. A stopped rig has no uptime, and a missing replacement cannot make a healthy sibling's uptime older.

The existing inventory and isEffectivelyRunning predicate supply both the count and uptime membership. One archive-filtered batch reads each node's latest-ID registration timestamp; there are no per-node database calls, new public inventory fields, identity-rule changes, migrations or dependencies.

How you verified it

Current head: 1077fa0ad6b9f776cb8b56fb368c271e433b7c06, tree-identical to tested local commit 8f8698379c16ada97246357f045ed37955c39c64. It follows 0809d623 and the original production change 7cad43e2, based on 3bc921233e88a417c788a0ff9ab022630ebbb96d. Node 24.19.0; isolated worktrees with locked dependencies.

  • Original failing-first proof: three rollback failures and four passing forward/equal-clock controls on the base production code. Production SessionRegistry, real migrated SQLite and both projections are exercised; the two clock reads are controlled, with no session IDs or timestamps patched after registration.
  • Uptime follow-up, as requested in review: both new cases failed on 0809d623. A missing-only rig exposed 12s instead of null; a rig with a healthy 6s sibling exposed 12s. On fresh main dbc8abe6, uptime was null and 6s respectively, while the sibling retained the old count/status bug. The fixed tree gets both count/status and uptime right and preserves stored rows.
  • Final focused tests: 260 daemon tests across eight files and 30 CLI HTTP/resource tests passed with one worker. This includes all three summary checks, the real-loopback N=24 burst with its unchanged 250 ms health budget, and child cleanup.
  • npm run build, npm run lint and git diff --check: passed on the final tree, with a 1536 MiB Node heap cap.
  • Ordinary-clock status/identity matrices against both the prior PR head and current main: 48 complete projections stay identical; the one running/confirmed-missing case changes only uptime to null. Every other projected field and persisted session row is preserved. The controls include absent, stale and uncertain identity evidence.
  • Eight bounded performance fixtures (24 rigs, 175 nodes, up to 43,750 historical sessions) preserved complete ordinary projection output. With 43,750 sessions, prior-PR to fixed p50 was 19.07 to 11.81 ms when only the latest row was running, and 23.00 to 12.57 ms when all historical rows were stored-running. These are local measurements, not a timing guarantee.
  • A separate merge-tree check against dbc8abe60721d4e228d488d3fa9cc26039828fbe was clean; the tested candidate is the pinned branch tree, not that newer merge.
  • Three focused commits contain 118, 14 and 53 changed lines. The combined PR is 142 additions and 35 deletions across four files.

The earlier 14-line test-only follow-up repaired D3's obsolete running_count fault trigger after hosted CI exposed it. D3 now proves the injected prepare failure occurred at the rig query, still requires HTTP 500 with no partial payload, and explicitly checks HTTP 200/full membership after recovery. It passes in the final focused run.

The full local aggregate, separate full UI suite and installed-package scenario were not rerun for this follow-up. Hosted CI for the new head must still complete. No native provider, live tmux session or actual system-clock change was exercised.

Anything you were unsure about

The reproduced ordering failure is within one daemon process. The monotonic ULID factory resets on restart, so this does not establish a global insertion-order or clock-rollback guarantee. Other registry, restore and snapshot selectors still use timestamps and remain outside this change. Identity-verdict applicability and stored session history are unchanged.

If this is security-related

Not security-related.

  • One concern per PR; no version bump; no CHANGELOG.md edit
  • Tests added or updated where the change is testable
  • I listed the checks I ran, their results, and any checks I could not run

Summary by CodeRabbit

  • Bug Fixes
    • Running-node counts and uptime now use the same set of effectively running nodes, with uptime based on the earliest available session registration time among them.
    • Session ordering and inventory projections now remain consistent when the system clock moves forward, backward, or stays within the same millisecond.
    • Projections now handle evidence that a registered session is missing, including when another healthy node is present.

@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 97889010-d6a4-4a74-821a-d477756f5f74

📥 Commits

Reviewing files that changed from the base of the PR and between 0809d62 and 1077fa0.


📒 Files selected for processing (2)
  • packages/daemon/src/domain/ps-projection.ts
  • packages/daemon/test/ps-session-ordering.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.



📝 Walkthrough
📝 Walkthrough

Walkthrough

The projection derives running counts and uptime from effectively running inventory entries and their latest session registration timestamps. Tests cover session ordering, projected values across clock changes, missing-session evidence, and query failure recovery.

Changes

Projection and Session Ordering

Layer / File(s) Summary
Projection running count and uptime
packages/daemon/src/domain/ps-projection.ts
The projection queries latest session registration timestamps by node. It derives the running count and earliest timestamp from inventory entries that satisfy isEffectivelyRunning.
Session ordering and projection tests
packages/daemon/test/ps-projection.test.ts, packages/daemon/test/ps-session-ordering.test.ts, packages/daemon/test/ps-summary-stall-red.test.ts
The projection fixture uses a shared monotonic ULID generator. Tests cover clock changes, session status, missing-session evidence, and /api/ps query failure and recovery.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: mvschwarz


Merge Risk: ⚪ Minimal · up to 1077f

No actionable issue remains in the reviewed change; merge after the normal checks complete.

Pre-merge checks | Passed 4 | Failed 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: aligning daemon ps session selection with inventory state.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@mvschwarz

Copy link
Copy Markdown
Owner

Thanks, @XTLine. After a backward wall-clock step, rig ps could say stopped while the inventory showed a running replacement, or show a rig with a healthy node as stopped because another node's replacement was missing. With this PR the count and status follow the inventory's running set and session order. We reviewed it and tested it against main on a test build: the count and status now agree with the inventory, and every case where the two orders agree keeps main's result. The per-rig count query is gone too, which is a nice simplification.

One change requested, to uptime:

  • The uptime query takes each node's latest session by id and uses it for uptime if its stored status is running. So a replacement that's confirmed missing, but still stored as running, is used.
  • On a test build, after a clock rollback with only a confirmed-missing replacement, main shows the rig as stopped with no uptime, and this PR shows stopped with an uptime of 12s. With a healthy sibling, this PR gets the count and status right (1, partial), but the uptime moves from the sibling's 6s to 12s, taken from the missing replacement.
  • Please compute uptime from the same set of current sessions that the count treats as running. A guard only for a zero count would miss the sibling case. Tests for both cases would keep them covered.

After your push, we'll review the change, and then there's a final maintainer check before merge.

@openrig-review openrig-review left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved at 1077fa0. The rig ps summary now counts the same running registrations as node inventory and takes uptime from that same set, so after a backward wall-clock step the summary agrees with inventory instead of reading stopped or counting a missing replacement. Wherever the two session orders agree, the count matches main; a confirmed-missing latest session no longer adds uptime, so uptime agrees with the count and status. Required CI passed at this head, on current main.

— dev60-planner@v-openrig-build

@mvschwarz
mvschwarz merged commit 17707d6 into mvschwarz:main Oct 12, 2026
10 checks passed
@mvschwarz

Copy link
Copy Markdown
Owner

Merged to main, thanks @XTLine. It will ship in the next release. Thanks too for turning the uptime change around within the hour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants