rojo/.cursor/plans/live_syncback_audit_fixes_561d17c7.plan.md

name	overview	todos	isProject
Live Syncback Audit Fixes	Fix 6 issues found during the live syncback audit: resource leaks from mem::forget, crash-on-error instead of recovery, concurrent syncback races, silent malformed input corruption, misleading doc comments, and missing test coverage.	id content status fix1-mem-forget Fix 1: Replace `mem::forget` with `drop()` in serve.rs (lines 61, 135) completed id content status fix2-error-recovery Fix 2: Catch syncback errors and log+recover instead of crashing (serve.rs lines 84-88) completed id content status fix3-concurrent-guard Fix 3: Reject concurrent syncback requests (SyncbackSignal::fire returns bool, api.rs returns 409) completed id content status fix4-validate-counts Fix 4: Validate chunk counts match rbxm blob size (build_dom_from_chunks) completed id content status fix5-doc-comment Fix 5: Correct misleading ServiceChunk doc comment (interface.rs lines 30-42) completed id content status fix6-tests Fix 6: Add 5 new integration tests (attributes/tags, multiple refs, concurrent, malformed, properties-only) completed id content status run-ci Run CI after all fixes to verify everything passes completed	false

Live Syncback Audit Fixes

This plan was generated by /audit from a Plan-mode session. Refer to .cursor/rules/atlas.mdc for full project standards.

Standards

These standards govern every fix in this plan. An implementer MUST read .cursor/rules/atlas.mdc before starting.

Round-Trip Identity (from atlas.mdc Quality Standard)

Syncback (or two-way sync) writes a directory tree. Building an rbxl from that directory tree and forward-syncing it back must produce a bit-identical instance tree -- same names, same classes, same properties, same hierarchy, same ref targets. Any deviation is a bug.

CLI Syncback Parity (from atlas.mdc Two-Way Sync Strategy)

Plugin-based sync must produce exactly the same filesystem output that atlas syncback would give for the same input. Byte-for-byte identical files, identical directory structures, identical naming. Any divergence is a bug until proven otherwise.

Code Quality (from atlas.mdc Code Quality Standard)

DRY code that is easy to reason about and hard to get wrong. The same slugify/dedup/meta-update pattern should not be copy-pasted across files. Small refactors (2-3 call sites) are included as fixes. Major rewrites (10+ call sites) are deferred.

Context

Audited the "Live syncback" feature on the ambiguous-paths branch. This feature adds a one-click full sync from Studio to filesystem via POST /api/syncback. The plugin serializes all services (properties via msgpack, children via a single rbxm blob), sends them to the server, which reconstructs a WeakDom, runs syncback_loop(incremental=false), and writes the result. The server shuts down and restarts after each syncback.

Key files audited:

• src/cli/serve.rs -- serve loop, run_live_syncback(), build_dom_from_chunks()
• src/web/api.rs -- handle_api_syncback() endpoint
• src/web/mod.rs -- SyncbackSignal, LiveServer
• src/web/interface.rs -- ServiceChunk, SyncbackRequest, SyncbackPayload
• plugin/src/App/init.lua -- performSyncback(), SYNCBACK_SERVICES
• plugin/src/ChangeBatcher/encodeService.lua -- service property/ref encoding

No critical round-trip identity violations found. The CLI parity tests (assert_live_matches_cli) provide strong guarantees. The round-trip test (roundtrip_build_syncback_rebuild) covers the full lifecycle. All 6 fixes below are correctness hardening, resilience, and test coverage.

Fixes

Each fix below was approved by the user during the audit quiz. Implement them in order. Each fix includes its test requirements -- a fix is not complete until its tests pass.

---

Fix 1: Replace mem::forget with proper drop

• Status: Approved
• Finding: Step 14 (Challenged Limitations) -- resource leak accumulating per syncback invocation
• Files: [src/cli/serve.rs](src/cli/serve.rs) (lines 61, 135)
• Problem: forget(session_old) at line 135 leaks the entire oneshot ServeSession (tree, VFS, project) after every live syncback. forget(session) at line 61 leaks the initial config-reading session at startup. The oneshot session has change_processor: None, suppressed_paths: None, ref_path_index: None, git_repo_root: None -- all Option types that drop cleanly as None. The Vfs destructor will stop its watcher thread. There is no reason to use forget().
• Solution:
  • Line 135: Replace forget(session_old); with drop(session_old);
  • Line 61: Replace forget(session); with drop(session);
  • Remove use std::mem::forget; from the imports (line 4) if no other uses remain
• Risk: Very low. The oneshot session's fields are all Option::None or standard types. The VFS watcher thread is dormant (no watches registered). Existing live syncback tests exercise this path.
• Verify round-trip: Run parity_scripts and roundtrip_build_syncback_rebuild -- both must pass.
• Verify syncback parity: Run assert_live_matches_cli tests -- filesystem output unchanged.
• Tests required:
  • No new tests needed. Existing tests syncback_twice_different_data and server_comes_back_functional already exercise repeated syncback and verify the server recovers correctly.

---

Fix 2: Recover from syncback errors instead of crashing

• Status: Approved
• Finding: Step 14 (Challenged Limitations) -- server crashes permanently on syncback failure
• Files: [src/cli/serve.rs](src/cli/serve.rs) (lines 84-88)
• Problem: run_live_syncback(&project_path, payload)? propagates errors via ?, crashing the serve process. The plugin shows "Syncback data sent" but the server never comes back.
• Solution: Catch the error and log it, then continue to restart the server:

ServerExitReason::SyncbackRequested(payload) => {
    log::info!("Live syncback requested, running...");
    match run_live_syncback(&project_path, payload) {
        Ok(()) => log::info!("Syncback complete, restarting serve..."),
        Err(err) => log::error!("Live syncback failed: {err:#}. Restarting serve..."),
    }
    continue;
}

• Risk: Very low. The server restarts fresh regardless (new VFS, new session). An error just means the filesystem wasn't updated -- the old state persists.
• Verify round-trip: Existing tests still pass. The error path is tested by Fix 6 test 4.
• Verify syncback parity: N/A -- this fix doesn't change the syncback logic, only error handling.
• Tests required:
  • Rust integration test (Fix 6, test 4): Send malformed chunk counts that trigger an error in build_dom_from_chunks. Verify the server recovers and comes back online.

---

Fix 3: Reject concurrent syncback requests

• Status: Approved
• Finding: Step 14 (Challenged Limitations) -- second syncback silently overwrites first payload
• Files: [src/web/mod.rs](src/web/mod.rs) (SyncbackSignal::fire, lines 43-46), [src/web/api.rs](src/web/api.rs) (handle_api_syncback, line 259)
• Problem: fire() unconditionally overwrites any existing payload. If two requests arrive before the accept loop processes the first, the first payload is silently lost.
• Solution:
  1. Change SyncbackSignal::fire() to return bool:

pub fn fire(&self, payload: SyncbackPayload) -> bool {
    let mut guard = self.payload.lock().unwrap_or_else(|e| e.into_inner());
    if guard.is_some() {
        return false;
    }
    *guard = Some(payload);
    self.notify.notify_one();
    true
}

1. In handle_api_syncback, check the return value:

if !syncback_signal.fire(payload) {
    return msgpack(
        ErrorResponse::bad_request("A syncback is already in progress"),
        StatusCode::CONFLICT,
    );
}

• Risk: Very low. Happy path unchanged. Only adds rejection for the concurrent case.
• Verify round-trip: Existing tests pass (single syncback). New test verifies rejection.
• Verify syncback parity: N/A -- this fix doesn't change syncback output.
• Tests required:
  • Rust integration test (Fix 6, test 3): Fire two syncback requests rapidly, verify the second returns 409 Conflict and the server still processes the first successfully.

---

Fix 4: Validate chunk counts match rbxm blob size

• Status: Approved
• Finding: Step 11g (Audit Step 4, server-side processing) -- silent cursor clamping corrupts subsequent services
• Files: [src/cli/serve.rs](src/cli/serve.rs) (build_dom_from_chunks, lines 184-210)
• Problem: Line 187 let end = (cursor + total).min(cloned_children.len()); silently clamps when counts exceed blob size. This corrupts cursor position for all subsequent services.
• Solution:
  1. Inside the service loop, replace the .min() clamp with an early error:

let end = cursor + total;
anyhow::ensure!(
    end <= cloned_children.len(),
    "Service '{}' claims {} children + {} carriers but only {} instances remain in blob",
    chunk.class_name, child_count, ref_count,
    cloned_children.len().saturating_sub(cursor)
);

1. After the service loop, validate all instances were consumed:

anyhow::ensure!(
    cursor == cloned_children.len(),
    "Service chunks account for {} instances but rbxm blob contains {}",
    cursor, cloned_children.len()
);

• Risk: Very low. Only triggers on malformed input. Normal requests are unaffected. With Fix 2, the error is caught and the server recovers.
• Verify round-trip: Existing tests pass (all have correct counts). New test verifies rejection.
• Verify syncback parity: N/A -- this fix doesn't change output for valid input.
• Tests required:
  • Rust integration test (Fix 6, test 4): Send a request where service chunks claim more children than the blob contains. Verify the server logs an error and recovers.

---

Fix 5: Correct misleading ServiceChunk doc comment

• Status: Approved
• Finding: Step 4 (server-side processing audit) -- doc describes behavior that doesn't exist
• Files: [src/web/interface.rs](src/web/interface.rs) (lines 30-42)
• Problem: Doc says "Indices <= child_count point directly to a child instance" but the implementation only indexes into the carrier subrange. This misleads future maintainers.
• Solution: Replace the doc comment block (lines 30-42) with:

/// Metadata for a single service in the live syncback request.
///
/// Children are NOT stored here -- they live in the top-level `data` blob
/// (a single rbxm containing all children from all services).
///
/// Each service's range in the blob is `child_count + ref_target_count`
/// consecutive entries. The first `child_count` are real children (parented
/// to the service). The next `ref_target_count` are ObjectValue carriers
/// whose `Value` property holds a Ref to the actual target.
///
/// `refs` maps property names to 1-based indices into the carrier subrange
/// (NOT the full service range). Index 1 = first carrier, index 2 = second,
/// etc. Index 0 or out-of-range values are ignored (treated as nil).

• Risk: Zero. Documentation-only change.
• Verify round-trip: N/A
• Verify syncback parity: N/A
• Tests required: None.

---

Fix 6: Add missing test coverage

• Status: Approved
• Finding: Step 13 (audit report) -- several edge cases lack test coverage
• Files: [tests/tests/live_syncback.rs](tests/tests/live_syncback.rs)
• Problem: Missing tests for service attributes/tags, multiple Ref properties, concurrent requests, malformed input, and properties-only services.
• Solution: Add the following tests to tests/tests/live_syncback.rs:

Test 1: parity_service_attributes_and_tags

• Create a Lighting service with custom Attributes (e.g., {"CustomFloat": Float32(1.5)}) and Tags (e.g., ["TestTag"])
• Use make_service_chunk_full with Attributes and Tags encoded as Variant::Attributes and Variant::Tags in the properties map
• Run assert_live_matches_cli

Test 2: parity_multiple_service_refs

• Create a service (Workspace) with two Ref properties: one pointing to a Part child, one pointing to a Light child
• Use make_service_chunk_full with two carrier ObjectValues and two refs entries
• Run assert_live_matches_cli

Test 3: rejects_concurrent_syncback (for Fix 3)

• Use run_serve_test to start a server
• Send a valid syncback request (don't wait for restart)
• Immediately send a second syncback request
• Verify the second returns 409 Conflict
• Wait for the server to come back online (first syncback completes)

Test 4: recovers_from_malformed_chunks (for Fixes 2+4)

• Use run_serve_test to start a server
• Record the initial session info
• Send a syncback request with mismatched counts (e.g., child_count: 999 but empty data blob)
• Verify the server comes back online (new session ID)
• Verify the filesystem is unchanged (the malformed syncback was rejected before writing)

Test 5: parity_properties_only_no_children

• Create 3 services (Lighting, SoundService, TextChatService) all with properties but zero children
• Verify empty rbxm blob case works with properties
• Run assert_live_matches_cli
• Risk: Zero. Test-only additions.
• Verify round-trip: The parity tests verify round-trip by design.
• Verify syncback parity: The parity tests verify CLI parity by design.
• Tests required: Self-contained -- the fix IS the tests.

---

Skipped Fixes

None -- all identified fixes were approved.

Accepted Limitations

rbxm Child Ordering Assumption

• Limitation: Live syncback relies on SerializationService:SerializeInstancesAsync() preserving the array order of top-level instances in the resulting rbxm blob. If Roblox ever changes the serializer to reorder by class, the cursor-based partitioning in build_dom_from_chunks would break.
• Scenario: A future Roblox engine update changes serialization order, causing children to be assigned to wrong services.
• User decision: Accepted for this release. This is the standard approach used by all Rojo-family tools. The rbxm format preserves parent-child relationships and child ordering through the PRNT chunk. Roblox has never changed this behavior.
• Mitigation: Fix 4 adds count validation that would catch the symptom (cursor doesn't consume all instances) even if not the root cause.

Asymmetric ClockTime Exclusion

• Limitation: encodeService.lua excludes Lighting.ClockTime (alias for TimeOfDay) but the server-side build_dom_from_chunks doesn't have this exclusion.
• Scenario: A crafted request with both ClockTime and TimeOfDay could produce conflicting properties. This never happens in normal usage since the plugin prevents it.
• User decision: Accepted. The plugin is the only intended client, and it already filters correctly.
• Mitigation: None needed beyond the existing plugin-side filter.

No Session ID on /api/syncback

• Limitation: Unlike /api/write, the syncback endpoint has no session ID validation. Any client that knows the host/port can trigger a syncback.
• Scenario: On a shared network with serveAddress: "0.0.0.0", someone could overwrite project files.
• User decision: Accepted by design. The syncback is intended to work from the NotConnected page (no active session). The servePlaceIds/blockedPlaceIds whitelist provides protection for team environments.
• Mitigation: Existing place ID validation. The serve address defaults to 127.0.0.1 (localhost only).

Deferred Refactors

Duplicate Service Lists

The service list exists in 3 places that must stay synchronized:

1. SYNCBACK_SERVICES in plugin/src/App/init.lua (lines 7-22)
2. VISIBLE_SERVICES in src/syncback/mod.rs (lines 73-88)
3. Implicitly in test helper code (serve_util.rs)

Currently they match exactly (14 services). If these diverge, Ref properties targeting instances in a missing service would be silently lost. A cross-language shared constant is impossible (Lua vs Rust), but a CI check or a test that compares the lists (e.g., parsing the Lua source) could prevent drift. Deferred because it's a build/CI infrastructure change, not a code change.

build_dom_from_chunks Duplication with Test Helper

build_dom_from_chunks in serve.rs and make_rbxl_from_chunks in serve_util.rs implement nearly identical DOM reconstruction logic. The test helper was written to mirror the production code for CLI parity tests. If the production code changes, the test helper must be updated in lockstep. Consider extracting the shared logic into a librojo function that both can call. Deferred because it requires making build_dom_from_chunks public and adjusting its API.

Test Plan

Tests are not optional. Every approved fix must have test coverage. A fix is not complete until its tests pass.

Rust Integration Tests (tests/tests/live_syncback.rs)

• parity_service_attributes_and_tags -- service with Attributes + Tags survives round-trip (Fix 6, test 1)
• parity_multiple_service_refs -- service with 2+ Ref properties, all carriers resolved correctly (Fix 6, test 2)
• rejects_concurrent_syncback -- second syncback returns 409 Conflict (Fix 6, test 3)
• recovers_from_malformed_chunks -- mismatched counts cause error, server recovers (Fix 6, test 4)
• parity_properties_only_no_children -- 3 services with properties, zero children, empty blob (Fix 6, test 5)

Existing Tests (Must Continue Passing)

All existing tests in tests/tests/live_syncback.rs must pass after all fixes:

• parity_scripts, parity_models_with_properties, parity_special_names, parity_duplicate_names
• parity_deep_hierarchy, parity_mixed_file_types, parity_multi_service
• parity_empty_services_mixed, parity_lighting_properties, parity_soundservice_properties
• parity_starterplayer_properties, parity_textchatservice_properties
• parity_childless_service_with_properties, parity_service_properties_with_children
• parity_multiple_services_with_properties, parity_cross_service_refs
• parity_camera_not_synced, rejects_bad_protocol, rejects_bad_version
• rejects_blocked_place, rejects_unlisted_place, allows_whitelisted_place
• allows_any_place_when_unrestricted, server_comes_back_functional
• syncback_twice_different_data, syncback_replaces_all_old_files
• roundtrip_build_syncback_rebuild

Test Rules

The implementer MUST follow these rules when writing tests for this plan:

For bugs found and fixed

Every bug fix MUST have a test that:

1. Would have FAILED before the fix
2. PASSES after the fix
3. Prevents the bug from regressing

For missing coverage identified

For each missing test coverage gap:

1. Write the test
2. Verify it passes against the current implementation
3. If it fails, investigate -- it may have found another bug

Scope: test every affected layer

For each finding, determine which test layers are affected and write tests in ALL of them:

• Fixes 1-5 affect the Rust server -- integration tests in tests/tests/live_syncback.rs
• No Lua spec tests needed (plugin encoding is unchanged)
• No snapshot tests needed (syncback output format is unchanged)

Final Step: Run CI

After ALL fixes and tests are implemented, run the /ci command (see .cursor/commands/ci.md) to execute the full CI pipeline. Every fix must pass CI before the plan is considered complete. Do not skip this step.