fix(daemon): version+identity-keyed endpoint + version arbitration (#1009) - #1020
Merged
Conversation
…1009) Follow-up to #940/#1006 (which only *warned* on daemon identity mismatch). Two fixes so a wrong-version daemon can no longer silently serve another checkout's builds: 1. **Endpoint keyed by version + cache-identity.** `get_daemon_port()`'s default was a fixed per-user port (8765/8865) shared by every checkout. Now the endpoint is derived from `daemon_endpoint_key()` = FNV-1a of `CARGO_PKG_VERSION | DaemonCacheIdentity.label_value()`: `default_daemon_port()` maps it into the IANA dynamic range (49152–65535), and the port file is named `daemon-<key>.port`. Since fbuild-paths is workspace-versioned, the CLI and daemon derive the same key with no handshake — daemons of different versions land on distinct ports/files and can't serve each other; same-version+cache checkouts still (correctly) share. Dropped the old cross-mode port fallback (an anti-isolation bridge). `FBUILD_DAEMON_PORT` still overrides. 2. **Version arbitration, not mtime.** `ensure_direct_daemon_running` used `cli_binary_mtime > daemon_source_mtime` to decide restarts, so a freshly-built *older* binary (newer mtime) could evict a *newer* daemon. New pure `should_restart_daemon(cli_ver, daemon_ver, cli_mtime, daemon_mtime)`: restart iff the CLI is a newer semver (upgrade) or the same version was rebuilt (mtime); **never** evict a newer daemon. `HealthResponse.version` was already on the wire. Tests: fbuild-paths endpoint-key/port derivation (deterministic, ranged, version -distinct, keyed port file); fbuild-cli arbitration matrix (older-never-evicts, newer-upgrades, same-version-mtime, unparseable-fallback). CLAUDE.md port docs updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe change isolates daemon endpoints by version and cache identity using deterministic dynamic-range ports, and adds semver-aware restart arbitration so direct-daemon acquisition handles upgrades, downgrades, equal versions, and unparsable versions consistently. ChangesDaemon isolation and lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1009. Follow-up to #940/#1006 (which only warned on daemon identity mismatch). Two fixes so a wrong-version daemon can no longer silently serve another checkout's builds:
1. Endpoint keyed by version + cache-identity
get_daemon_port()'s default was a fixed per-user port (8765/8865) shared by every checkout. Now the endpoint is derived fromdaemon_endpoint_key()= FNV-1a ofCARGO_PKG_VERSION | DaemonCacheIdentity.label_value():default_daemon_port()maps it into the IANA dynamic range (49152–65535), and the port file isdaemon-<key>.port. Sincefbuild-pathsis workspace-versioned, the CLI and daemon derive the same key with no handshake — daemons of different versions land on distinct ports/files and can't serve each other; same-version+cache checkouts still (correctly) share. Dropped the old cross-mode port fallback (an anti-isolation bridge).FBUILD_DAEMON_PORTstill overrides.2. Version arbitration, not mtime
ensure_direct_daemon_runningusedcli_binary_mtime > daemon_source_mtimeto decide restarts, so a freshly-built older binary (newer mtime) could evict a newer daemon. New pureshould_restart_daemon(cli_ver, daemon_ver, cli_mtime, daemon_mtime): restart iff the CLI is a newer semver (upgrade) or the same version was rebuilt (mtime); never evict a newer daemon.HealthResponse.versionwas already on the wire.Acceptance criteria
Verified — local + Linux Docker
cargo check --workspacegreen; fbuild-paths 30 + fbuild-cli daemon_client 11 tests pass (Linux Docker, 28.5s). clippy-D warnings+ fmt clean. CLAUDE.md port docs updated.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
FBUILD_DAEMON_PORT.Bug Fixes