fix: address bugs and add test coverage for save-sync#3135
Merged
gantoine merged 6 commits intorommapp:save-syncfrom Mar 16, 2026
Merged
fix: address bugs and add test coverage for save-sync#3135gantoine merged 6 commits intorommapp:save-syncfrom
gantoine merged 6 commits intorommapp:save-syncfrom
Conversation
…-sync - Fix broken path construction in FSSyncHandler: build_* methods now return relative paths; sync_watcher uses paths relative to sync base instead of CWD (was completely non-functional in production) - Fix SSH connection leak in push-pull task: conn.close() now in finally - Add log.warning for disabled SSH host key verification - Fix race condition in session operation counter: use atomic SQL increment instead of read-then-write - Extract _increment_session_counter helper, add exc_info to warnings - Replace legacy session.query() with select() in sync_sessions_handler - Fix orphaned session: trigger_push_pull now passes session_id to job - Fix wasteful SSH download when no matched_save exists - Fix BaseModel import collision in sync.py (pydantic -> project base) - Fix ORM mutation in UserSchema.from_orm_with_request: set field on schema instance instead of mutating live ORM object - Mask ssh_password and ssh_key_path in DeviceSchema API response - Fix migration PostgreSQL compatibility: condition ON UPDATE clause on MySQL, drop enum in downgrade - Rename copy-paste artifact rom_user_status_enum
- Restore NoResultFound behavior on update_session, complete_session, fail_session when row is missing (scalar returns None, old .one() raised -- silent None is a semantic regression) - Remove redundant get_session call from _increment_session_counter; the atomic SQL increment is already a no-op on missing rows - Log warning when passed session_id is not found in _sync_device instead of silently creating an orphan session
…king, and auth utils - test_sync_sessions_handler: increment_operations_completed (atomic counter, no-op on missing), NoResultFound on update/complete/fail with nonexistent session - test_sync_watcher: _extract_device_and_platform path parsing (valid, non-incoming, too few parts, nested, outside base), _ensure_conflicts_dir creation and idempotency, process_sync_changes empty/disabled - test_sync (endpoints): negotiate with untracked saves (no_op), server saves not mentioned by client (download), deleted-by-client detection (skip), complete on FAILED/CANCELLED session (400), trigger_push_pull passes session_id in enqueue kwargs - test_device (endpoints): sync_config SSH credential masking (ssh_password/ssh_key_path -> "********"), null config passthrough, config without sensitive fields - test_utils_auth: _get_device_name UA parsing (browser+OS, browser only, OS only, neither), create_or_find_web_device (creates new, returns existing on fingerprint match, updates last_seen)
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
uv sync installs sqlalchemy[mariadb-connector] regardless of which database is being tested, so libmariadb-dev must be present on all runners. The postgres migration job and pytest postgresql matrix were missing this step.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
The syncsessionstatus enum creation used checkfirst=False which fails with DuplicateObject if the type already exists (e.g., test reruns or partial migrations). Matches the pattern used in the downgrade.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
gantoine
approved these changes
Mar 16, 2026
… PostgreSQL The sa.Enum() inline in create_table tried to CREATE TYPE again after the explicit ENUM.create() call. Use the pre-created enum variable with create_type=False for PostgreSQL to avoid DuplicateObject error. Verified locally: upgrade, downgrade, re-upgrade all clean on PostgreSQL.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
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
2 participants
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
Cleanup pass on the save-sync branch addressing correctness issues, a security concern, and test coverage gaps found during review.
Bug fixes
_extract_device_and_platformusedos.path.relpath(path)(relative to CWD, which is/backend), so every incoming file was silently ignored. Fixed to compute paths relative to the sync base directory. Also unifiedbuild_*methods to return relative paths consistently -- callers prependbase_pathas needed.conn.close()in push-pull task was only on the success path. Moved tofinallyblock.saves.pyreadoperations_completed, then wrote+1-- concurrent uploads lose counts. Replaced with atomic SQLSET operations_completed = operations_completed + 1.trigger_push_pullcreated a session but never passed its ID to the background job. The job created its own, leaving the endpoint's session permanently PENDING.from_orm_with_requestsetcurrent_device_idon the live SQLAlchemy object (with# type: ignore). Now sets it on the validated schema instance instead._process_remote_savedownloaded files via SSH even when no matching server save existed, then immediately deleted them.ON UPDATE CURRENT_TIMESTAMPis MySQL-only DDL. Conditioned on dialect. Also added missing enum drop indowngrade().Security
DeviceSchemaserializedsync_configincludingssh_passwordandssh_key_pathto any client withDEVICES_READscope. Added field serializer that masks these with a fixed-length"********".Convention fixes
session.query()calls withselect()API in sync_sessions_handlerfrom pydantic import BaseModelcollision in sync.py (should use project base)rom_user_status_enumvariable in migration (copy-paste from another migration)_increment_session_counterhelperexc_info=Trueto swallowed exception warningslog.warningfor disabled SSH host key verification (was a silent# TODO)Tests (21 new)
increment_operations_completed: atomic counter, no-op on missing sessionNoResultFoundraised on update/complete/fail with nonexistent sessiontrigger_push_pullpassessession_idin enqueue kwargscreate_or_find_web_device: creation, fingerprint matching, last_seen update_get_device_nameUA parsing variantsTest plan