OS service management with cross-platform support and refactor#18
Open
krishna-santosh wants to merge 44 commits into
Open
OS service management with cross-platform support and refactor#18krishna-santosh wants to merge 44 commits into
krishna-santosh wants to merge 44 commits into
Conversation
Add libc (unix) for effective-uid check in the service elevation guard, and windows-service for native Service Control Manager integration.
Accept an injected shutdown future in start_grpc_server and spawn_grpc_server, switching from serve() to serve_with_shutdown(). This enables the server bootstrap to be shared between the foreground serve command and OS service runners that provide their own shutdown trigger (e.g. Windows SCM stop control).
Move the full startup sequence (home dir, secrets, config, migrations, REST + gRPC servers, search indexer, backup scheduler) from main.rs into a new server module with a run() function that accepts an injected shutdown future. This allows both the foreground serve command and OS service runners (systemd, launchd, Windows SCM) to share the exact same bootstrap path. Also loads .env from /.env in addition to cwd, so installed services and mcp stdio (which run from an arbitrary cwd) pick up persisted secrets. The home .env is second-wins so real env vars and the cwd file keep precedence.
Add 'vcms service' subcommand with install, uninstall, status, start, and stop actions for managing the server as a native OS background service: - Linux: systemd unit (install/enable/start via systemctl) - macOS: launchd LaunchDaemon (bootstrap/kickstart via launchctl) - Windows: Service Control Manager via windows-service crate (auto-start, LocalSystem, ACL-hardened ProgramData home) Shared logic (unit file rendering, username validation, home directory resolution, secure .env creation) lives in service/mod.rs so it compiles and tests on every platform. Platform-specific side effects are behind cfg-gated submodules. Security: the service refuses to run as root on unix; Windows uses LocalSystem. The .env file is created with mode 0600, symlink attacks are rejected, and Windows ACLs are locked to SYSTEM + Administrators only.
…le-dir fallback Introduce a dual-layout directory system: split (default) uses the platform's per-type directories via the \directories\ crate, while single mode (VCMS_HOME set or legacy ~/.vcms exists) keeps everything under one root. This aligns with OS conventions (XDG on Linux, Library dirs on macOS, AppData on Windows) while preserving backward compatibility for existing installs.
Replace per-user ~/.vcms with platform-specific system directories for service installs: /var/lib/vcms (Linux), ~/Library/Application Support/vcms (macOS), C:\ProgramData\vcms (Windows). Remove the user_home() helper that resolved tilde expansion, since the daemon no longer nests under a user's home. Update CLI help text to reflect the split layout.
Replace the in-process MCP stdio server (which opened the database, secrets, and search index directly) with a stateless HTTP proxy that forwards JSON-RPC between stdin/stdout and the running server's /mcp endpoint. This means the stdio process needs no disk access and works even when data belongs to a privileged service account. - Remove CmsServer::new_stdio(), verify_stdio_token(), and stdio_token - Rewrite transports/stdio.rs as a reqwest-based HTTP proxy - Simplify run_mcp_stdio() to just read VCMS_MCP_TOKEN and VCMS_MCP_URL - Replace init_stdio_tracing() with config-free init_proxy_tracing() - Delete old mcp_stdio.rs tests, add proxy-focused tests
Update all three documentation files to reflect: - Split/single directory layout with platform-specific paths table - MCP stdio as a thin HTTP proxy (VCMS_MCP_URL env var) - vcms service install/uninstall/status/start/stop commands - Updated environment variable defaults and descriptions
… to match rmcp 2.0
…pure function, add elevation preflight - Add public system_home() as single source of truth for the OS service data directory (C:\ProgramData\vcms, /var/lib/vcms, etc.) - Refactor layout() to gather filesystem/env inputs then delegate to a pure, testable resolve_layout() function with clear precedence: > system service home > legacy ~/.vcms > platform split - Add preflight_system_home() that fails fast with an actionable hint when the active home is the system dir but the process lacks access (e.g. non-elevated CLI against ACL-locked ProgramData) - Call preflight from ensure() so serve/admin/backup/restore surface the permission error early; read-only commands never trip it - Add unit tests for the full resolve_layout() precedence chain
…ed system_home() - Replace hardcoded WINDOWS_SERVICE_HOME const with paths::system_home() so the CLI resolves to the same store as the running service - Rewrite install() with clean-slate approach: stop + delete any lingering service record before creating fresh, avoiding the reuse trap after uninstall where Windows defers deletion and a stale record causes the new service to stay Stopped - Wait for the service to reach Running before reporting success so crash-on-boot is no longer silently swallowed - Add stop_and_wait(), wait_for_state(), wait_until_deleted() helpers with bounded SCM polling (30s timeout, 200ms interval) - Handle ERROR_SERVICE_DOES_NOT_EXIST in uninstall() gracefully - Report honestly when Windows defers deletion (handles still open)
…ocal const Replace SERVICE_HOME const with a service_home() function that delegates to paths::system_home(), ensuring the CLI resolves to the same data directory as the running service. No functional change on Linux.
…ocal const Replace SERVICE_HOME const with a service_home() function that delegates to paths::system_home(), ensuring the CLI resolves to the same data directory as the running service. No functional change on macOS.
The home .env is optional; when it lives in the ACL-locked system service home (e.g. C:\ProgramData\vcms), a non-elevated CLI legitimately cannot read it — most notably vcms mcp stdio, a proxy that needs no home file. Treat permission denied like absent and let the data-touching commands hit paths::ensure()'s preflight, which reports the access problem with an actionable hint.
…rement Document the three-tier single-layout resolution (env > system service home > legacy ~/.vcms) and the new behavior where data-touching commands fail fast when the system home is ACL-locked and the process lacks elevation.
…rement Mirror the AGENTS.md updates: document the three-tier single-layout resolution and the fast-fail behavior when the system home requires elevation.
Clarify that once the system service home exists, any CLI invocation (even with the service stopped or uninstalled) targets the same store rather than creating a per-user copy, and that elevated permissions are required because the directory is owned by SYSTEM/root.
New utils::magic_bytes module compares declared MIME types against file signatures using the infer crate. Types with reliable magic-byte signatures (images, video, audio, PDF, archives, Office formats) are checked; signature-less types (text, CSV, SVG) are skipped. The module is not yet wired into the upload path (follow-up commit).
Extend the StorageProvider trait with start_multipart() that returns an object_store::MultipartUpload. Implementations: - FileSystemStorage: put_multipart on the local ObjectStore - S3Storage: put_multipart on the S3 ObjectStore - MockStorage: in-memory MockMultipartUpload for tests (parts accumulate in a Vec, published on complete, cleared on abort)
…used_at - FileRepository: add set_thumbnail_meta() for attaching thumbnail dimensions and key after background generation - FileRepository::create: InMemoryFileRepository now returns UniqueViolation on duplicate id (matches real DB PK constraint) - AccessTokenLookupRow: add last_used_at field to find_by_prefix queries across sqlite, mysql, and postgres (needed by auth debounce)
Replace the buffered upload path with upload_file_streaming() that streams via object_store multipart in 5 MiB chunks with 2 parts concurrency. Key improvements: - Constant memory regardless of file size (no full-body buffer) - Magic-byte sniffing on first SNIFF_LEN bytes while streaming - Size cap enforced incrementally (abort on overflow) - Pre-generated file ids are single-use via in-flight DashMap claim - Thumbnails generated in a background task after DB record exists - Orphan blob cleanup on DB insert failure - New error variants: AlreadyExists (409), ReadError (400)
New public endpoint PUT /api/v1/files/upload/{token} accepts raw file
bytes using the HMAC-signed token as credential (no session/API key).
The token is verified, single-use enforced, content-type validated
against the token, and size-checked via Content-Length before streaming.
Config changes:
- New upload_token_expiry_secs field (default 900s / 15min)
- SignedUploadToken::generate() refactored to delegate to
generate_with_storage_provider() which now accepts expiry_secs
The multipart upload_file handler is also rewritten to stream the
multipart field body directly into storage via upload_file_streaming.
- Validate content type against the whitelist at mint time (fail fast instead of returning a URL doomed to a 400 PUT) - Resolve the site's actual storage provider instead of hardcoding 'filesystem' - Use configurable upload_token_expiry_secs - Expanded tool description explaining the PUT workflow
Skip the unconditional per-request UPDATE on sessions and access tokens when the 'last seen' timestamp is less than 60s old. This keeps the auth hot path read-only for most requests while still recording activity at a reasonable granularity. - Add parse_db_timestamp() for lenient RFC3339/Postgres/SQLite timestamp parsing - Add needs_touch() helper used by both REST middleware and gRPC interceptor - Touch interval is 60 seconds (TOUCH_INTERVAL_SECS)
The search indexer no longer polls on a 2s interval. Since all content producers run in the server process (vcms mcp stdio is an HTTP proxy), every enqueue rings the in-process Notify. The loop now sleeps until notified, with a one-time startup drain for rows left by a crash. The queue's documentation is updated to clarify it exists purely for durability (crash recovery), not cross-process signaling.
Embedded SPA assets now serve with proper HTTP caching: - assets/ (content-hashed by Vite): immutable, max-age=1y - index.html: no-cache (revalidate every time) - Other files (favicon etc): max-age=1h ETags (embed-time sha256) enable 304 Not Modified responses. The HeaderMap is threaded through to the dashboard handler.
Bind both REST and gRPC listeners before spawning serve loops so a port conflict surfaces as a startup error instead of a background task failure. Add an on_ready callback parameter to server::run() that fires once startup has actually succeeded (DB open, migrations done, ports bound). Startup error messages now include context prefixes (e.g. 'preparing data directories:', 'opening database:') for easier debugging from service hosts where bare IO errors are otherwise opaque. The gRPC server now accepts a pre-bound TcpListener instead of a SocketAddr, using TcpListenerStream for graceful shutdown.
…dening Lifecycle: - Report StartPending during boot, Running only from on_ready callback - start() now waits for Running state before claiming success - stop() waits for Stopped state (with 30s timeout) - Exit code uses ServiceSpecific(1) for vcms failures (visible in Event Log) - Default LOG_OUTPUT to 'file' in service context (no console) - Write failures to logs/service-error.log (plain std::fs, no tracing) - Installer surfaces recent service errors directly in the failure msg ACL hardening: - Two-pass icacls: pass 1 hardens root dir only, pass 2 /reset's children so they re-inherit. Fixes the bug where the old single-pass /T left pre-existing files with an empty DACL (deny-everyone), bricking secrets.toml and vcms.db on reinstall. - Added unit tests exercising both the happy path and repair of bricked files
After sending SIGTERM via launchctl kill, poll launchctl print for up to 10s to confirm the process actually exited. Prints a clear status message on success or a 'check vcms service status' hint on timeout.
Collection migration (rename_map field references): wrap the per-entry UPDATE loop in a single transaction so partial renames are never left behind and per-statement commit overhead is eliminated. Entry file reference inserts: replace per-file-id INSERT with a single multi-row statement (IN/ANY clause). Reduces N round-trips to 1 for entries with many file references.
…g pool tar building, zstd compression, and AES-256-GCM encryption are now run via tokio::task::spawn_blocking so they don't starve the async runtime during large backups. Similarly, decrypt/zstd-decode/untar during restore run on the blocking pool. The wrap/open methods are refactored into static functions (wrap_bytes/open_bytes) that take owned parameters for spawn_blocking compatibility. inspect_sites() is now async.
- Add UPLOAD_TOKEN_EXPIRY_SECS env var documentation - Update search indexer docs: purely event-driven model, no cross-process polling, durability-only queue semantics
…ed URLs REST tests: - Signed URL happy path, expired token, tampered signature, single-use enforcement, magic byte mismatch, content-type header mismatch, oversize rejection, multipart magic byte mismatch gRPC tests: - body_for_mime() helper sends correct magic bytes per declared type MCP tests: - End-to-end create_upload_url then PUT (single-use, file visibility) - Read-only token rejected for upload URL creation - Disallowed content type rejected at mint time
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR restructures runtime directory resolution into split (platform-conventional) vs. single-dir layouts, adds a cross-platform ChangesRuntime directories, service management, and MCP proxy
Streaming multipart uploads and signed upload URLs
Auth touch throttling, backup offload, DB batching, dashboard caching, webhook AES fix
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client as MCP Client
participant Stdio as vcms mcp stdio
participant Server as CMS Server /mcp
Client->>Stdio: JSON-RPC over stdin
Stdio->>Server: POST /mcp with Bearer token
Server-->>Stdio: JSON-RPC response
Stdio-->>Client: JSON-RPC over stdout
sequenceDiagram
participant Client
participant Handler as file_handler
participant FileService
participant Storage as StorageProvider
participant DB as Repository
Client->>Handler: multipart/PUT upload
Handler->>FileService: upload_file_streaming
FileService->>Storage: start_multipart / put_part
FileService->>FileService: sniff magic bytes
FileService->>Storage: complete/abort multipart
FileService->>DB: create file record
FileService-->>Handler: FileWithUrl
sequenceDiagram
participant CLI as vcms CLI
participant ServiceMod as service::run_service
participant Platform as platform dispatch
participant OS as OS Service Manager
CLI->>ServiceMod: run_service(Install)
ServiceMod->>Platform: dispatch(action)
Platform->>Platform: prepare_home/env
Platform->>OS: register unit/plist/SCM entry
OS-->>Platform: start service
Platform-->>CLI: status/success
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/backend/src/services/search/indexer.rs (1)
21-37: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRetry failed drains before waiting again
Adrain()error is only logged here, then the loop goes back tonotify.notified(). Since enqueues are the only wakeup source, a transient DB/index error can leave already-queued rows stuck indefinitely if no new write happens. A small bounded retry/backoff here would let the indexer self-heal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/src/services/search/indexer.rs` around lines 21 - 37, The run loop in search::indexer::run only logs drain() failures and then waits for a new notification, which can leave queued rows stuck after a transient error. Update the loop around drain(&search, &queue, &repository) to retry failed drains with a small bounded backoff before returning to notify.notified().await, so the indexer can self-heal without requiring a fresh enqueue. Keep the startup drain and the normal drain path consistent, and preserve the existing SearchService, SearchQueue, and Repository usage.
🧹 Nitpick comments (5)
apps/backend/src/handlers/backup_handler.rs (1)
702-706: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
bytes.clone()duplicates the entire uploaded artifact in memory.Since
inspect_sitesnow consumesVec<u8>by value, the clone here doubles peak memory for the full backup blob beforestage_upload. For large instance backups this can be a noticeable spike. If you want to avoid it while keeping the "validate before staging" guarantee, consider havinginspect_sitesreturn thebytesback (or acceptArc<[u8]>) so the buffer can be reused rather than copied.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/src/handlers/backup_handler.rs` around lines 702 - 706, The backup inspection flow is cloning the full upload buffer before staging, which doubles peak memory usage in `backup_handler` around `inspect_sites` and `stage_upload`. Refactor the `inspect_sites` path so it can return or retain ownership of the original `bytes` buffer after validation, or switch the shared payload to `Arc<[u8]>`, and then pass that same buffer into `stage_upload` without calling `bytes.clone()`.apps/backend/src/handlers/dashboard_handler.rs (1)
20-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider unit tests for the new caching logic.
cache_control_forandserve_embeddedencode security/perf-relevant behavior (immutable long-cache forassets/*vs.no-cacheforindex.html, and 304 short-circuiting). These are pure/testable functions with no current coverage in the diff.♻️ Example test sketch
#[cfg(test)] mod tests { use super::*; #[test] fn assets_are_immutable() { assert_eq!(cache_control_for("assets/app.js"), "public, max-age=31536000, immutable"); } #[test] fn index_html_is_no_cache() { assert_eq!(cache_control_for("index.html"), "no-cache"); } #[test] fn other_paths_get_short_ttl() { assert_eq!(cache_control_for("favicon.ico"), "public, max-age=3600"); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/src/handlers/dashboard_handler.rs` around lines 20 - 63, Add unit tests for the new caching behavior in dashboard_handler, covering both cache_control_for and serve_embedded. Verify the path-based cache policy for assets/*, index.html, and other files, and add a test that serve_embedded returns a 304 Not Modified when IF_NONE_MATCH matches the computed ETag. Use the existing function names cache_control_for and serve_embedded so the tests stay anchored even if code moves.apps/backend/tests/rest/files_tests.rs (1)
353-363: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffMinting tokens via the internal
cms::signed_uploadcrate breaks the black-box test contract.
mint_upload_url(and the tampered-signature test) construct signed URLs by calling the internal librarySignedUploadToken::generate_with_storage_providerand by hardcodingTEST_HMAC_SECRETto match the server. Per the REST-test conventions these should be black-box: mint the URL over HTTP (e.g., the MCPcreate_upload_urltool, astests/mcp/file_tests.rsalready does) rather than reaching into production internals. This also removes the brittle secret-string duplication that silently breaks if the server config changes.The tampered-signature case can still mint via HTTP and then mutate a byte of the encoded token to force the
InvalidSignaturepath without importing the crate.As per path instructions: "Backend integration tests must not use internal imports; they should communicate only via HTTP/tonic clients against a real server" and "REST API integration tests ... should be black-box tests against a real server using HTTP only."
Also applies to: 414-422
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/tests/rest/files_tests.rs` around lines 353 - 363, The REST tests are bypassing the black-box contract by minting upload tokens with `cms::signed_upload::SignedUploadToken` and `TEST_HMAC_SECRET`. Update `mint_upload_url` and the tampered-signature test to obtain a real signed upload URL through the running server over HTTP, matching the approach used in `tests/mcp/file_tests.rs`. After minting, keep the invalid-signature coverage by mutating the returned encoded token byte instead of reusing internal signing code.Source: Path instructions
apps/backend/src/config.rs (1)
398-398: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared upload expiry constant instead of hardcoding
900here and indefault_config_toml(). This keeps the config default and the template aligned withcrate::signed_upload::DEFAULT_UPLOAD_TOKEN_EXPIRY_SECS.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/src/config.rs` at line 398, The upload token expiry default is hardcoded here instead of using the shared signed upload constant, which can drift from the template. Update the config default in the Config/default handling and the default_config_toml() template to reference crate::signed_upload::DEFAULT_UPLOAD_TOKEN_EXPIRY_SECS so both paths stay aligned.README.md (1)
116-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrecedence between
$VCMS_HOME, the service's system dir, and legacy~/.vcmsisn't spelled out.CLAUDE.md gives an explicit numbered precedence (env var > system service home > legacy
~/.vcms> split dirs). This README section documents each condition individually but doesn't state which wins when more than one applies (e.g.,$VCMS_HOMEset while the service's system dir also exists), which could confuse an operator troubleshooting "wrong data dir" issues.📝 Suggested clarification
Set **`$VCMS_HOME`** to keep everything under a single root instead (and an existing `~/.vcms` is honored automatically, so upgrades don't move your data). + +(Precedence when more than one applies: `$VCMS_HOME` > the `vcms service` system +dir described below > legacy `~/.vcms` > platform split dirs.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 116 - 142, Clarify the directory selection precedence in the Data directory section by explicitly stating which source wins when multiple locations exist: $VCMS_HOME first, then the service’s system directory, then the legacy ~/.vcms fallback, before the split per-type defaults. Update the prose around the vcms service and vcms serve descriptions so operators can tell how the CLI chooses its store when these conditions overlap.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/backend/src/cli.rs`:
- Around line 129-136: The `Install.user` doc comment is inaccurate for Windows:
the `windows.rs` service install path does not silently ignore `--user`, it
rejects non-empty values with an error. Update the `Install` variant’s `user`
field documentation in `cli.rs` to say Windows custom users are unsupported and
the command fails if `--user` is provided, matching the behavior in the Windows
service dispatch.
In `@apps/backend/src/config.rs`:
- Line 134: Add a lower-bound check for upload_token_expiry_secs in the config
loading/validation path so zero or negative values are rejected before use.
Update the Config handling around upload_token_expiry_secs and any related
validation logic in Config::from_env / Config::validate so invalid values return
a clear error instead of allowing SignedUploadToken::verify to treat every token
as expired.
In `@apps/backend/src/repository/mysql/access_token.rs`:
- Line 74: The access token touch/debounce flow is using a MySQL time source
that can drift from chrono::Utc::now(). Update the `AccessTokenRepository` query
path that reads or sets `last_used_at` so it uses UTC consistently, either by
switching the MySQL timestamp source to `UTC_TIMESTAMP()` in the relevant SQL or
by pinning the session time zone to `+00:00` during pool/connection setup. Make
sure the fix is applied in the `needs_touch`/`last_used_at` handling code and
the repository methods that build the access token queries.
In `@apps/backend/src/server.rs`:
- Around line 148-199: `run()` returns too early because the `tokio::select!` in
`spawn_grpc_server`/REST handling only waits for the first server task to
finish, then signals shutdown without awaiting the other handle. Update the
shutdown flow so both `rest_handle` and `grpc_handle` are awaited after the
first one completes (or otherwise joined before returning), ensuring the sibling
server fully drains before `run()` exits; use the existing `shutdown_tx`,
`shutdown_tx2`, `rest_handle`, and `grpc_handle` symbols to keep the graceful
shutdown behavior consistent with the module doc comment.
- Around line 281-291: The admin seeding flow in seed_admin is treating
repository.user.exists(ADMIN_EMAIL) errors as “not found” via unwrap_or(false),
which can lead to an unnecessary create attempt and a panic during startup.
Update the existence check to handle the Result explicitly: if exists() returns
an error, log it and stop or return early instead of proceeding to
repository.user.create(...); only call create when exists() successfully returns
false. Keep the fix localized to seed_admin and preserve the current behavior
when the admin user is genuinely absent.
In `@apps/backend/src/storage/mod.rs`:
- Around line 103-125: The MockMultipartUpload::complete implementation is
constructing a PutResult with an unsupported extensions field for object_store
0.14.0. Update the complete method to return a PutResult using only the fields
available in that version, keeping the existing e_tag and version values and
removing the extra field so the mock compiles cleanly.
---
Outside diff comments:
In `@apps/backend/src/services/search/indexer.rs`:
- Around line 21-37: The run loop in search::indexer::run only logs drain()
failures and then waits for a new notification, which can leave queued rows
stuck after a transient error. Update the loop around drain(&search, &queue,
&repository) to retry failed drains with a small bounded backoff before
returning to notify.notified().await, so the indexer can self-heal without
requiring a fresh enqueue. Keep the startup drain and the normal drain path
consistent, and preserve the existing SearchService, SearchQueue, and Repository
usage.
---
Nitpick comments:
In `@apps/backend/src/config.rs`:
- Line 398: The upload token expiry default is hardcoded here instead of using
the shared signed upload constant, which can drift from the template. Update the
config default in the Config/default handling and the default_config_toml()
template to reference crate::signed_upload::DEFAULT_UPLOAD_TOKEN_EXPIRY_SECS so
both paths stay aligned.
In `@apps/backend/src/handlers/backup_handler.rs`:
- Around line 702-706: The backup inspection flow is cloning the full upload
buffer before staging, which doubles peak memory usage in `backup_handler`
around `inspect_sites` and `stage_upload`. Refactor the `inspect_sites` path so
it can return or retain ownership of the original `bytes` buffer after
validation, or switch the shared payload to `Arc<[u8]>`, and then pass that same
buffer into `stage_upload` without calling `bytes.clone()`.
In `@apps/backend/src/handlers/dashboard_handler.rs`:
- Around line 20-63: Add unit tests for the new caching behavior in
dashboard_handler, covering both cache_control_for and serve_embedded. Verify
the path-based cache policy for assets/*, index.html, and other files, and add a
test that serve_embedded returns a 304 Not Modified when IF_NONE_MATCH matches
the computed ETag. Use the existing function names cache_control_for and
serve_embedded so the tests stay anchored even if code moves.
In `@apps/backend/tests/rest/files_tests.rs`:
- Around line 353-363: The REST tests are bypassing the black-box contract by
minting upload tokens with `cms::signed_upload::SignedUploadToken` and
`TEST_HMAC_SECRET`. Update `mint_upload_url` and the tampered-signature test to
obtain a real signed upload URL through the running server over HTTP, matching
the approach used in `tests/mcp/file_tests.rs`. After minting, keep the
invalid-signature coverage by mutating the returned encoded token byte instead
of reusing internal signing code.
In `@README.md`:
- Around line 116-142: Clarify the directory selection precedence in the Data
directory section by explicitly stating which source wins when multiple
locations exist: $VCMS_HOME first, then the service’s system directory, then the
legacy ~/.vcms fallback, before the split per-type defaults. Update the prose
around the vcms service and vcms serve descriptions so operators can tell how
the CLI chooses its store when these conditions overlap.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7728ac6c-225e-4640-8fe7-3e258f48772d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (64)
AGENTS.mdCLAUDE.mdREADME.mdapps/backend/Cargo.tomlapps/backend/src/cli.rsapps/backend/src/config.rsapps/backend/src/grpc/interceptor.rsapps/backend/src/grpc/server.rsapps/backend/src/handlers/backup_handler.rsapps/backend/src/handlers/dashboard_handler.rsapps/backend/src/handlers/file_handler.rsapps/backend/src/lib.rsapps/backend/src/main.rsapps/backend/src/mcp/auth.rsapps/backend/src/mcp/resources/site_schema.rsapps/backend/src/mcp/server.rsapps/backend/src/mcp/tools/file.rsapps/backend/src/mcp/transports/stdio.rsapps/backend/src/middleware/auth.rsapps/backend/src/paths.rsapps/backend/src/repository/mysql/access_token.rsapps/backend/src/repository/mysql/collection.rsapps/backend/src/repository/mysql/entry.rsapps/backend/src/repository/mysql/file.rsapps/backend/src/repository/postgres/access_token.rsapps/backend/src/repository/postgres/collection.rsapps/backend/src/repository/postgres/entry.rsapps/backend/src/repository/postgres/file.rsapps/backend/src/repository/sqlite/access_token.rsapps/backend/src/repository/sqlite/collection.rsapps/backend/src/repository/sqlite/entry.rsapps/backend/src/repository/sqlite/file.rsapps/backend/src/repository/traits.rsapps/backend/src/router/dashboard.rsapps/backend/src/router/files.rsapps/backend/src/router/mod.rsapps/backend/src/router/openapi.rsapps/backend/src/secrets.rsapps/backend/src/server.rsapps/backend/src/service/linux.rsapps/backend/src/service/macos.rsapps/backend/src/service/mod.rsapps/backend/src/service/windows.rsapps/backend/src/services/backup/mod.rsapps/backend/src/services/error.rsapps/backend/src/services/file.rsapps/backend/src/services/search/indexer.rsapps/backend/src/services/search/queue.rsapps/backend/src/services/webhook.rsapps/backend/src/signed_upload.rsapps/backend/src/storage/filesystem.rsapps/backend/src/storage/mod.rsapps/backend/src/storage/s3.rsapps/backend/src/test_helpers.rsapps/backend/src/tracing.rsapps/backend/src/utils/magic_bytes.rsapps/backend/src/utils/mod.rsapps/backend/tests/common/server.rsapps/backend/tests/grpc/files_tests.rsapps/backend/tests/mcp/file_tests.rsapps/backend/tests/mcp/main.rsapps/backend/tests/mcp/stdio_proxy_tests.rsapps/backend/tests/mcp_stdio.rsapps/backend/tests/rest/files_tests.rs
💤 Files with no reviewable changes (1)
- apps/backend/tests/mcp_stdio.rs
Comment on lines
+103
to
+125
| #[async_trait] | ||
| impl object_store::MultipartUpload for MockMultipartUpload { | ||
| fn put_part(&mut self, data: object_store::PutPayload) -> object_store::UploadPart { | ||
| for chunk in data.iter() { | ||
| self.buf.extend_from_slice(chunk); | ||
| } | ||
| Box::pin(futures_util::future::ready(Ok(()))) | ||
| } | ||
|
|
||
| async fn complete(&mut self) -> object_store::Result<object_store::PutResult> { | ||
| let data = Bytes::from(std::mem::take(&mut self.buf)); | ||
| self.files.lock().unwrap().insert(self.key.clone(), data); | ||
| Ok(object_store::PutResult { | ||
| e_tag: None, | ||
| version: None, | ||
| extensions: Default::default(), | ||
| }) | ||
| } | ||
|
|
||
| async fn abort(&mut self) -> object_store::Result<()> { | ||
| self.buf.clear(); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
object_store 0.14 MultipartUpload trait put_part complete abort PutResult extensions field
💡 Result:
In the object_store crate (version 0.14), the multipart upload interface is primarily handled by the MultipartUpload trait and the lower-level MultipartStore trait [1][2][3]. MultipartUpload Trait The MultipartUpload trait is designed for writing objects in fixed-size chunks [1][2]. Its key methods include: - put_part: Used to upload data parts concurrently [2]. It returns an UploadPart future [1][2]. - complete: Finalizes the multipart upload, making the object visible as an atomic operation [2]. It returns a Result [2]. - abort: Used to abort the upload [2]. If the object is dropped without calling complete, some object stores may automatically clean up uploaded parts [2]. PutResult and Extensions The PutResult struct contains information about a completed put request, specifically e_tag and version [4]. As of version 0.14, PutResult does not contain an extensions field [4]. While an Extensions field exists within the object_store crate for passing context-specific information (such as tracing spans) [5][6], it is currently utilized in option-based structs like PutMultipartOptions and PutOptions, rather than in result types [7][6]. There have been community discussions regarding adding support for Extensions in various *Result objects (including PutResult) to allow stores to pass information back to callers, but this has not been implemented as a field on PutResult itself [8]. For more advanced or manual multipart upload control, users may interact with the MultipartStore trait, which provides create_multipart, put_part, complete_multipart, and abort_multipart methods [3][9].
Citations:
- 1: https://docs.rs/object_store/latest/object_store/trait.MultipartUpload.html
- 2: https://docs.rs/object_store/latest/src/object_store/upload.rs.html
- 3: https://docs.rs/object_store/latest/src/object_store/multipart.rs.html
- 4: https://docs.rs/object_store/latest/object_store/struct.PutResult.html
- 5: https://docs.rs/object_store/latest/object_store/struct.Extensions.html
- 6: https://docs.rs/object_store/latest/object_store/struct.PutMultipartOptions.html
- 7: https://docs.rs/object_store/latest/object_store/struct.PutOptions.html
- 8: Support
Extensionsin*Resultobjects apache/arrow-rs-object-store#740 - 9: https://docs.rs/object_store/latest/object_store/multipart/trait.MultipartStore.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect dependency versions and the relevant implementation.
git ls-files | rg '(^|/)(Cargo.toml|Cargo.lock|apps/backend/src/storage/mod.rs)$'
printf '\n--- Cargo.toml matches ---\n'
rg -n 'object_store|futures_util|async-trait' Cargo.toml apps/backend/Cargo.toml Cargo.lock
printf '\n--- Relevant source slice ---\n'
sed -n '1,220p' apps/backend/src/storage/mod.rs
printf '\n--- object_store entries in Cargo.lock ---\n'
rg -n 'name = "object_store"|version = "0\.' Cargo.lock -A 3 -B 2Repository: velopulent/cms
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrowly inspect the locked object_store version and its dependency edges.
rg -n -A 8 -B 4 '^name = "object_store"|^version = "|^dependencies = \[' Cargo.lock | sed -n '1,120p'
printf '\n--- object_store occurrences only ---\n'
rg -n '^name = "object_store"|^version = "0\.[0-9]+\.[0-9]+"' Cargo.lock | rg 'object_store|version = "0\.14'Repository: velopulent/cms
Length of output: 3999
🌐 Web query:
docs.rs object_store 0.14.0 PutResult extensions MultipartUpload put_part UploadPart
💡 Result:
In the object_store 0.14.0 crate, multipart uploads are primarily handled via two distinct levels of abstraction: the high-level MultipartUpload trait and the low-level MultipartStore trait [1][2]. PutResult The PutResult struct represents the result of a successful put or multipart upload operation [3]. It contains metadata about the newly created object, specifically its e_tag (an optional unique identifier) and version (an optional version indicator) [3]. MultipartUpload (High-level) The MultipartUpload trait is the high-level interface for uploading objects in chunks [1][4]. - Put_part: This method takes a PutPayload and returns an UploadPart, which is a future that must be polled to completion to upload the specific chunk [4]. - Complete: This method finalizes the upload, combining all previously uploaded parts into the final object, and returns a Result [4]. It is considered implementation-defined behavior to call complete before all UploadPart futures have been polled to completion [4]. MultipartStore (Low-level) The MultipartStore trait provides a lower-level interface, intended for scenarios where you need direct control over multipart upload IDs and individual part indexing [2][5]. - put_part: Unlike the high-level variant, this method is an async function that takes a path, a MultipartId, a part_idx (usize), and the data (PutPayload), returning a future that resolves to a PartId [2][5]. - complete_multipart: This method completes the process by taking the path, the MultipartId, and a Vec of all uploaded parts, returning a Result [2][5]. For most use cases, it is recommended to use the higher-level ObjectStore::put_multipart_opts or ObjectStoreExt::put_multipart, which provide simpler interfaces and handle concurrency and chunk management automatically [6][5][7].
Citations:
- 1: https://docs.rs/object_store/latest/object_store/trait.MultipartUpload.html
- 2: https://docs.rs/object_store/latest/object_store/multipart/trait.MultipartStore.html
- 3: https://docs.rs/object_store/latest/object_store/struct.PutResult.html
- 4: https://docs.rs/object_store/latest/src/object_store/upload.rs.html
- 5: https://docs.rs/object_store/latest/src/object_store/multipart.rs.html
- 6: https://docs.rs/object_store/latest/object_store/trait.ObjectStoreExt.html
- 7: https://docs.rs/object_store/latest/object_store/trait.ObjectStore.html
Drop extensions from PutResult here object_store = "0.14" resolves to 0.14.0, and PutResult only has e_tag and version in that release. This field makes the mock fail to compile unless the dependency is bumped.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/src/storage/mod.rs` around lines 103 - 125, The
MockMultipartUpload::complete implementation is constructing a PutResult with an
unsupported extensions field for object_store 0.14.0. Update the complete method
to return a PutResult using only the fields available in that version, keeping
the existing e_tag and version values and removing the extra field so the mock
compiles cleanly.
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
1 participant
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.
Summary by CodeRabbit
New Features
vcms servicesupport for installing and managing native background services on Linux, macOS, and Windows.Bug Fixes