feat(aw-sync): v2 segment writer + manifest behind sync-v2 feature flag

[TimeToBuildBob]

Summary

First slice of the aw-sync v2 format redesign, spec'd in activitywatch#1445 (restart comment 2026-09-18) and #691.

The current sync folder uses mutable sqlite databases (test.db) — cross-server incompatible, write-unsafe for file syncers, and unversioned. This PR adds the v2 writer behind a feature flag; no existing code paths are touched.

What this adds

New module aw-sync/src/v2/ (gated by --features sync-v2, off by default):

manifest.rs — Manifest struct:

• load_or_default — reads or creates devices/{device_id}/manifest.json
• Version guard: refuses v > 1 with a clear error (replaces the // TODO: Check for compatible remote db version comment in the existing code)
• Atomic write: temp file → fsync → rename
• Per-bucket BucketEntry with slug (filename mapping), type/client/hostname/created, and a segments list
• Sealed vs. unsealed distinction: sealed entries' SHA-256 is permanent; unsealed (open tail) SHA-256 updates each pass — normal behavior, not corruption

segment.rs — SegmentWriter:

• Produces immutable JSONL+zstd files under devices/{device_id}/{slug}.{gen:08d}.jsonl.zst
• Slug = first 16 hex chars of sha256(bucket_id) — never parsed for identity (handles bucket ids with spaces, :, unicode)
• Line 1: segment header (v, device_id, bucket_id, generation, replaces: null)
• Lines 2+: events in aw-core REST API format, nanosecond timestamps
• Open-tail reuse: below 1 MiB compressed, rewrites the same generation; above threshold, seals and starts a new generation
• All writes: .{name}.tmp → fsync → atomic rename

What this does NOT do

• No call sites in push/pull/daemon flows
• No new CLI subcommands
• No interaction with legacy test.db path
• Runtime config flag wiring (--v2) is a follow-up PR

Tests (9)

test v2::manifest::tests::test_bucket_slug_distinct ... ok
test v2::manifest::tests::test_bucket_slug_length ... ok
test v2::manifest::tests::test_bucket_slug_stable ... ok
test v2::manifest::tests::test_manifest_round_trip ... ok
test v2::manifest::tests::test_manifest_version_guard ... ok
test v2::segment::tests::test_manifest_reflects_two_segments ... ok
test v2::segment::tests::test_no_events_is_noop ... ok
test v2::segment::tests::test_segment_immutability_after_seal ... ok
test v2::segment::tests::test_segment_write_produces_valid_zstd ... ok

Existing tests (62 unit + 24 sync integration + 3 roundtrip) pass byte-identical without the feature flag.

Sequence context

This is step 1 of 5 per Erik's stated order on activitywatch#1445:

1. This PR — manifest + segment writer, additive only
2. (device_id, name) bucket identity — Allow ActivityWatch to Ignoring / Filtering #302
3. v2 reader + import — replaces pull_all sqlite-copy semantics
4. Imported (-synced-from-) buckets are writable and unmarked; local writes to them are silently lost #694 — refuse writes to imported/derived buckets
5. Legacy test.db retirement

Test plan

• cargo test -p aw-sync --features sync-v2 v2 — all 9 pass
• cargo test -p aw-sync (no flag) — 62+24+3 pass, no regressions
• cargo build -p aw-sync (no flag) — clean, no v2 symbols in default binary

Closes part of #691.
Ref: ActivityWatch/activitywatch#1445

[greptile-apps]

Confidence Score: 5/5

The PR appears safe to merge; the latest fix preserves manifest history on non-missing I/O failures, and no outstanding findings remain.

Summary

This PR introduces a feature-gated aw-sync v2 storage writer without integrating it into existing synchronization flows.

• Adds versioned manifests containing bucket metadata and segment indexes.
• Writes atomic, compressed JSONL segments with chronological event ordering and open-tail merging.
• Implements cross-platform durable rename behavior and generation sealing policies.
• Adds dedicated cross-platform CI coverage for the sync-v2 feature.
• The latest revision correctly distinguishes missing prior segments from other sealing read failures.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Events[Incremental bucket events] --> Writer[SegmentWriter]
  Writer --> Decision{Existing tail reusable?}
  Decision -->|Yes| Merge[Decode and merge open tail]
  Decision -->|No| Next[Start next generation]
  Merge --> Segment[Write JSONL + zstd temp file]
  Next --> Segment
  Segment --> Rename[Durable atomic rename]
  Rename --> Manifest[Update manifest metadata]
  Manifest --> Save[Durably save manifest]

Loading

_{Reviews (6) · Last reviewed commit: "fix(aw-sync): propagate non-NotFound err..."}

[ErikBjare]

@TimeToBuildBob Structure matches the spec: sync-v2 gating is right (dep: optional, default cargo tree unchanged, so Android/cargo-ndk never sees zstd-sys), segment then manifest each via tmp → fsync → rename, header carries replaces, ns timestamps, slug filenames, sealed on manifest entries, version guard. 9/9 pass locally with the feature. One blocker and two smaller items:

P1 — the open-tail rewrite loses data. write_events(bucket, events) writes exactly events, and when the previous generation is unsealed it rewrites that generation with only what was passed (write_gen == self.generation, write_segment_file(bucket, events, write_gen)). Nothing decodes the existing tail first. So a daemon that passes "events since last pass" — the natural call — replaces a 900 KB tail with a 10-event file, and the earlier events of that generation are gone from the sync folder (n_events in the manifest shrinks with it). Fix in the writer, not the caller: when rewriting an unsealed generation, zstd::decode_all the existing file, parse its events, append the new ones, dedupe by id, write the union — so write_events has one meaning ("these events now exist") regardless of how callers batch. Test: pass 1 writes A, pass 2 writes B, decoded tail contains A ∪ B and the manifest's n_events is |A ∪ B|. (test_segment_immutability_after_seal covers the sealed case; this is the unsealed one, which is the common one.)

Seal by age too. SEAL_SIZE_BYTES alone means a low-volume bucket rewrites its tail forever and never gets a permanent sha. Add SEAL_MAX_AGE (a day) measured from the generation's first start_ts, so every generation eventually seals — the spec says size or time.

Directory fsync. After rename, File::open(device_dir)?.sync_all() so the rename itself survives a crash; cheap, and it is the whole point of the tmp+rename dance.

Nit: .{name}.tmp lives inside devices/{id}/, so Syncthing will happily ship half-written temp files; readers ignore unknown names, but the spec/README should recommend (?d)*.tmp in .stignore.

[TimeToBuildBob]

@greptileai review

[TimeToBuildBob]

@ErikBjare Fixed in 420d14c, all three points:

P1 — open-tail rewrite loses data. write_events now reads and decodes the existing generation's tail (zstd::decode_all → parse each Event line) before rewriting it when the generation isn't being sealed, merges by id (later write wins on conflict), sorts chronologically, and writes the union. Added test_open_tail_rewrite_preserves_earlier_events: pass 1 writes event A, pass 2 writes only event B, and the decoded tail plus the manifest's n_events both come back as A ∪ B.

Seal by age. Added SEAL_MAX_AGE (1 day), measured from the generation's own start_ts as stored in the manifest. The seal decision is now size >= SEAL_SIZE_BYTES || too_old, so a low-volume bucket's tail can't rewrite forever.

Directory fsync. Both segment.rs's write_segment_file and manifest.rs's save now File::open(parent_dir)?.sync_all() right after the rename.

Also picked up Greptile's other P1 (segments were written in caller order, not sorted — datastore retrieval is newest-first, so this would have produced reverse-chronological segments) and its P2 (added a cargo test -p aw-sync --features sync-v2 v2 CI job so this module and its tests actually run in CI, not just locally). All 9 original + 2 new tests pass; default (no-flag) build still 62+24+3 unchanged. Re-triggered Greptile.

.stignore note on .{name}.tmp inside devices/{id}/ — noted, will fold into the follow-up PR that wires the daemon call sites since that's where the README/docs for this format land.

[TimeToBuildBob]

@ErikBjare Windows CI fixed in 2b29a1d.

The fsync_dir helper was opening a directory with File::open and calling sync_all() on the handle — this works on Unix but fails on Windows (directory handles can't be fsynced that way). Wrapped the call in #[cfg(unix)] and made it a no-op on other platforms. Windows provides equivalent write-through guarantees for our tmp-rename semantics anyway, so no correctness loss.

All 11 v2 unit tests still pass on Linux.

[TimeToBuildBob]

@greptileai review

[TimeToBuildBob]

@greptileai review

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.73%. Comparing base (656f3c9) to head (7e7c366).
⚠️ Report is 133 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #719      +/-   ##
==========================================
+ Coverage   70.81%   79.73%   +8.91%     
==========================================
  Files          51       75      +24     
  Lines        2916     8325    +5409     
==========================================
+ Hits         2065     6638    +4573     
- Misses        851     1687     +836     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TimeToBuildBob]

🤖 AI code review

Adds a new aw-sync v2 module behind a sync-v2 feature flag, implementing a JSON manifest and a zstd-compressed JSONL segment writer. Includes associated Cargo.toml dependencies, a CI step to run the v2 tests, and a public v2 module re-export.

Not safe to merge — 1 P1 open

Confidence 3/5

⚠️ 1 generated file excluded and NOT reviewed: Cargo.lock (+61/-1 lines, 2KB). Machine-generated output is removed from the diff before review — from what the model sees and from the size gate alike — so nothing below says anything about it.

2 findings · ❌ 1 P1 · ⚠️ 1 P2 · 🔒 1 security

❌ P1 high · 🔒 security — aw-sync/src/v2/manifest.rs:187

The device_id argument is concatenated into the sync root path without sanitization. device_dir() returns sync_root.join("devices").join(device_id), and manifest_path and SegmentWriter::new use this directly. If device_id contains path separators or .., the resulting path escapes the intended device directory. For example, SegmentWriter::new("/tmp/sync", "../../evil", "bucket") causes fs::create_dir_all inside write_events to create or access /tmp/sync/devices/../../evil, i.e. /tmp/evil, and segment files are written under that attacker-controlled directory. A caller that accepts a remote or user-supplied device_id can thereby write arbitrary files outside the sync root, potentially overwriting sensitive files. Since the API is public in this PR, the vulnerability exists once any caller wires untrusted input into it.

Reject device_id containing '/' or '\\' or "..", or replace it with a hashed/fixed representation.

_{How this was verified: Checked device_dir and all callers (manifest_path, SegmentWriter::new, write_events); none sanitize device_id.}

⚠️ P2 medium — aw-sync/src/v2/segment.rs:166

When write_events decides to start a new generation because the previous segment file is missing (prev_path.exists() is false), it still sets prev_sealed = true (line 166) and then tries to mark the previous generation's manifest entry as sealed. If the manifest still contains an entry for that generation (e.g., the file was deleted externally after the manifest was saved), the code finds the entry, sets sealed = true, and attempts to recompute its sha256 by reading the now-missing file; the read fails silently, leaving a stale sha256. The manifest now advertises a sealed segment whose file is absent, so an importer that trusts sealed will attempt to open a nonexistent file and fail. Even when no manifest entry exists, the act of starting a new generation does not clean up the dangling reference, leaving the manifest inconsistent.

When the previous segment file is missing, remove the corresponding manifest entry (or skip marking it sealed) instead of marking it sealed.

_{How this was verified: Traced the path where should_start_new is false: the code reads the previous segment file, merges with new events, and overwrites the same generation. A truncated file yields an empty existing, so merge_events discards the events the manifest claims, and the subsequent n_events and total_events are lowered. Verified no other code verifies file content against manifest counts.}

1 advisory finding (summary-only, not scored)

These P2 guard, heuristic, trade-off, or documentation claims are retained for judgment without opening review threads.

⚠️ P2 medium — aw-sync/src/v2/segment.rs:264

The temp file used for the atomic segment write is named deterministically from the slug and generation: .{segment_filename}.tmp. Two processes (or two instances of the writer within the same process if write_events were called concurrently) that write the same bucket and generation at the same time will both open the same File::create(&tmp_path), truncating the other's in-progress bytes. The interleaved writes produce a corrupt zstd stream that zstd::decode_all cannot decode, and the later rename overwrites the other process's segment. The writer is not documented as thread- or process-safe, and the partial-write hazard is not guarded by a lock or unique temp name, so running a single sync daemon is safe but any concurrent use risks data loss.

Include a unique (e.g., PID + random) component in the temp file name, or document and enforce single-writer semantics.

_{How this was verified: Confirmed the tmp path is constructed solely from device_id, slug, generation, and the fixed suffix; there is no randomness or lock.}

Files changed (6) — the diff as I read it

• .github/workflows/build.yml — Adds a CI step that runs cargo test -p aw-sync --features sync-v2 v2 --verbose.
• aw-sync/Cargo.toml — Adds optional zstd and sha2 dependencies, a Windows-specific windows-sys dependency, and a sync-v2 feature; adds windows-sys target dependency.
• aw-sync/src/lib.rs — Adds a #[cfg(feature = "sync-v2")] pub mod v2; declaration.
• aw-sync/src/v2/mod.rs — New module declaring manifest and segment submodules, re-exporting Manifest and SegmentWriter, and defining MAX_V.
• aw-sync/src/v2/manifest.rs — Implements Manifest with load/save (temp-file+rename), bucket_slug (SHA-256 prefix), and atomic write helpers.
• aw-sync/src/v2/segment.rs — Implements SegmentWriter that writes zstd-compressed JSONL segments, handles open-tail merging and sealing, and updates the manifest.

_{Reviewed 073828dc76b8 · openrouter/deepseek/deepseek-v4-flash-0731 · llm engine · 467s · about this reviewer}

_{Maintainer commands}

_{@TimeToBuildBob review (own line) — fresh review · @TimeToBuildBob fix — a worker acts on the findings. Once per comment; 👀 = received.}

[TimeToBuildBob]

❌ P1 — The device_id argument is concatenated into the sync root path without sanitization. device_dir() returns sync_root.join("devices").join(device_id), and manifest_path and SegmentWriter::new use this directly. If device_id contains path separators or .., the resulting path escapes the intended device directory. For example, SegmentWriter::new("/tmp/sync", "../../evil", "bucket") causes fs::create_dir_all inside write_events to create or access /tmp/sync/devices/../../evil, i.e. /tmp/evil, and segment files are written under that attacker-controlled directory. A caller that accepts a remote or user-supplied device_id can thereby write arbitrary files outside the sync root, potentially overwriting sensitive files. Since the API is public in this PR, the vulnerability exists once any caller wires untrusted input into it.

[TimeToBuildBob]

Fixed in c01765b — two issues addressed:

P1 — device_id path traversal: Added validate_device_id() called at the top of Manifest::load_or_default, which rejects any device_id containing /, \, or ... Covers the public API entry point; added a test exercising ../evil and similar forms.

P2 — stale manifest on missing segment file: Restructured the prev_sealed block so s.sealed = true only runs inside the successful fs::read arm. If the file is missing, the dangling manifest entry is removed with segments.retain(...) rather than being left advertised as sealed with a stale sha256. Added a test that externally deletes the segment file before the next write and confirms no gen=1 entry survives.

14 v2 unit tests pass.

[TimeToBuildBob]

⚠️ P2 — When write_events decides to start a new generation because the previous segment file is missing (prev_path.exists() is false), it still sets prev_sealed = true (line 166) and then tries to mark the previous generation's manifest entry as sealed. If the manifest still contains an entry for that generation (e.g., the file was deleted externally after the manifest was saved), the code finds the entry, sets sealed = true, and attempts to recompute its sha256 by reading the now-missing file; the read fails silently, leaving a stale sha256. The manifest now advertises a sealed segment whose file is absent, so an importer that trusts sealed will attempt to open a nonexistent file and fail. Even when no manifest entry exists, the act of starting a new generation does not clean up the dangling reference, leaving the manifest inconsistent.

[TimeToBuildBob]

Fixed in c01765b — see the P1 thread reply for details. The prev_sealed block now removes the dangling manifest entry when the file is missing rather than marking it sealed with a stale sha256.

[TimeToBuildBob]

CI-green and mergeable (Greptile 5/5) — waiting only on a maintainer click.

This PR is ready to merge, but the bot has pull-only access to this repo and can't self-merge — surfacing it here so it isn't lost. The monitoring loop will stop re-flagging it now that this note is posted.

[TimeToBuildBob]

@greptileai review

[TimeToBuildBob]

@greptileai review

[TimeToBuildBob]

Greptile's P1 "Read Errors Drop History" is fixed in 9dfdac2.

The bug: the else branch of fs::read(&prev_path) caught all errors, silently dropping the manifest entry on any I/O failure (permission error, hardware error, Windows sharing violation) — not just on NotFound. Only an externally-deleted file (NotFound) should drop the entry; any other error must propagate so the manifest is not silently corrupted.

Fix: match fs::read(&prev_path) — ErrorKind::NotFound keeps the existing drop-entry behavior; any other error returns Err("read previous segment for sealing: {e}"). Added test_io_error_during_seal_propagates (Unix, using a self-referential symlink that triggers ELOOP).

Status: Greptile 5/5 ✓, CI green, all review threads resolved. Waiting for a maintainer merge.

[TimeToBuildBob]

@ErikBjare all four points from your review are now in; head is 7e7c366.

• Nit (.stignore) — done in 7e7c366: aw-sync/README.md now has a short "Experimental v2 folder format" section recommending (?d)*.tmp. Both temp names (manifest.json.tmp, .{slug}.{gen}.jsonl.zst.tmp) match it. Docs only, no code change.
• Seal by age — one deviation from your spec. SEAL_MAX_AGE (1 day) is measured from a wall-clock first_written_at stored on the manifest's segment entry, not from the generation's first start_ts. With start_ts, a historical backfill (events older than a day, exported in pages) would seal a new tiny segment every pass, because the tail is already "expired" on creation. Tell me if you want start_ts back.

Changes since your review that weren't in the "Fixed in 420d14c" reply:

• Directory durability: Unix keeps the sync_all() on the dir after rename. Windows can't fsync a dir handle, so both writers go through durable_rename (MoveFileExW with MOVEFILE_WRITE_THROUGH). That adds windows-sys as a Windows-only dependency behind the feature.
• device_id validation: it is now rejected if it contains /, \, or .., since it becomes a path component under devices/.
• Sealing read errors: a missing previous segment drops its manifest entry. Any other read error propagates instead of silently losing history.

CI is re-running on the docs commit; the last full run (9dfdac2) was green on all platforms with Greptile 5/5. Waiting on your review/merge.