[chinokikiss]

Problem and fix

Fixes #20686.

Two distinct uploaded WALs can have VAD slices with the same absolute speech start.
retrieve_vad_segments() previously exported both to <job>/<timestamp>.wav.
The later write overwrote the earlier recording, segmented_paths collapsed to one
entry, and the source-frame map became unavailable. _run_sync_vad_phase() then
deleted both original WAVs and advanced toward STT with only one surviving file.

Reproduction uses two real PCM16 WAVs with distinct sample values and durations:
one starts at T with speech [1, 3], the other at T+1 with speech [0, 4]. Both exports
have absolute start T+1. This fails in either sequential order and through the real
concurrent VAD phase, without a provider or network call. Production incidence is
not measured; the demonstrated impact is loss of an affected shard before STT.

Namespace the outputs under <job>/.vad/<SHA256(source basename)>/, keeping the
numeric segment basename. Each source retains its bytes and mapping; timestamp
parsing and basename-plus-content durable segment identities remain unchanged.
This changes the existing shared VAD export primitive, without adding a new guard
or independent policy. The pipeline documentation describes cleanup and retry effects.

The legacy v1 endpoint uses the same primitive but retains its per-user directory.
Its file cleanup now removes an empty .vad/<hash> directory after the last segment,
without recursively deleting any files or removing the shared .vad root. If cleanup
removes an empty namespace between mkdir and opening a WAV, the writer recreates it
and retries once; persistent export failures still propagate.

Verification

Python 3.11.15, real Pydub PCM WAV read/slice/export, external providers stubbed:

• Regression suite on unchanged main: 3 failed, 1 passed; with the fix: 4 passed.
• python -m pytest tests/unit/test_sync_opus_decode.py -q: 45 passed.
• python -m pytest tests/unit/test_sync_v2.py -q: 182 passed.
• test_sync_silent_failure.py: 46 passed; test_sync_capture_pipeline.py:
  28 passed; test_wal_audio_coverage.py: 47 passed;
  test_wal_audio_coverage_pipeline.py: 60 passed. 408 tests passed across these six files,
  with each file in a separate process to respect the existing stub harness.
• After merging current main 4bdcc56d0dd4701af04cc423a310eb7445810708, also ran
  test_sync_phase_metrics.py: 15 passed and test_sync_cloud_tasks.py:
  103 passed, covering the upstream instrumentation that now wraps this module.
  526 tests passed total across eight isolated suites.
• The cleanup follow-up's focused cases on the previous PR implementation returned
  6 failed, 4 passed; after the follow-up 10 passed. They cover last-segment
  cleanup, retained sibling audio and unrelated directories, repeated cleanup,
  a deterministic cleanup/write interleaving with exact PCM verification, and
  propagation of a persistent output failure after one retry.
• Tests compare exact PCM samples, frame counts, both source maps, parsed clocks,
  cleanup after the actual concurrent VAD phase, and durable content IDs across job directories.
• One existing path assertion now compares Path objects, preserving its containment
  assertion on Windows as well as POSIX.

No live user audio, production writes, or hosted STT calls were used.

Draft: full repository validation is not green. After merging current main,
all 31 selected local manifest checks were attempted with keep-going. Two checks
initially could not initialize the Windows default uv cache. Re-running those
with the existing workspace cache restored the route-policy check (zero new
missing entries), leaving 30 reported pass and the Firestore index guard failed.
The guard's Windows-generated CRLF test list still sends *.py\r paths to
Bash/pytest. The failure-class history ratchet reported a shallow-history skip;
it is not full-history proof. Metadata validation against current main accepts
Failure-Class: none in the PR body, as required. Diff hygiene, pinned Black,
import purity, async-blocker scanning, module isolation and runtime source closure
passed.

The original submission's full Pyright lane reported 27 optional-package
missing-import errors, identical to its unchanged ffeb8a13 baseline, with none
in the changed pipeline. That comparison is historical; the full type lane was
not rerun against the newly merged main and is not claimed green. The original
separate Firestore guard attempt using an LF list also encountered missing IANA
timezone data and Windows asyncio loopback-block failures; its long-running
query-shapes file was interrupted. No passing guard proof is claimed.

The original standard Git pre-push launcher could not start its Bash child on this host
(NtCreateDirectoryObject access denied). The branch was published through GitHub's
Git data API, verifying the exact tested trees and commit SHAs. No hook script or
upstream source was changed to suppress these failures. Complete canonical checks
on the normal CI host and review the rollout note before marking this ready.
The upstream workflow runs currently report action_required with no jobs started;
this contributor account cannot approve upstream runs. CI approval remains with
the maintainers.

Retry / rollout consideration

New full paths are deterministic within a job. Pre-change Redis full-path checkpoints
will not match them. Durable content checkpoints retain the old identity for identical
audio; legacy jobs without a content ledger can replay STT once at deployment and
retain existing assignment dedupe. This patch prevents the pre-STT file overwrite;
it does not redefine downstream transcript deduplication or promise distinct rows
for simultaneous sources.

Overlap review

Reviewed 91 distinct open PRs returned by scoped VAD, WAL, timestamp/sync,
overwrite, collision and sync-title queries on 2026-10-05. Examined patches for
the 10 touching sync production modules. No existing repair of this collision was
found. #20659's timing instrumentation is now on main and has been merged into
this branch and tested. #20662 adds resource bounds and export error handling but
retains the timestamp-only output path. Upload filename containment is already
covered by #19483 and is excluded. The later 660-open-PR file inventory and the
current VAD/cleanup query found no equivalent per-source directory cleanup patch.
Recheck current main and these PRs before merge because concurrent changes may land.

Product invariants affected

none — the path-based invariant suggestion for this diff reports no locked matches.

Failure class (fixes)

Failure-Class: none

Line-Count-Exception: backend/utils/sync/pipeline.py | 3026 -> 3052 | namespace the existing VAD exporter by source and remove only empty per-source directories while preserving timestamps and retrying a cleanup/write interleaving once

Bounty proposal

Proposed bounty: US$100, subject to explicit maintainer approval and merge.
This is a demonstrated core capture data-loss repair with concurrent byte-level
regression evidence, rather than an export-recipe robustness change. It is not an
existing funded bounty, fixed program rate, or payment entitlement. Eligibility and
amount remain at the maintainers' discretion under the
contribution guide; payment details
would be supplied privately through its documented PayPal process.

Disclosure: investigation, code and tests were prepared with an AI assistant for
@chinokikiss; this does not represent independent human review.

[Git-on-my-level]

Verified the bug and the fix in backend/utils/sync/pipeline.py (retrieve_vad_segments) — two WALs with the same absolute speech start previously both exported to <job>/<timestamp>.wav, so the second write silently replaced the first recording before STT. Namespacing under .vad/<sha256(source-basename)>/ fixes it.

I checked the invariants the PR description claims, and they hold:

• parse_sync_filename_timestamp reads the basename only, so timestamp parsing is unaffected by the new subdirectory.
• compute_sync_segment_id hashes basename + file bytes, so durable segment IDs survive the new path (your retry-identity test asserts this too).
• v2 job cleanup rmtrees the job dir, which removes the .vad directories; os.makedirs(..., exist_ok=True) is idempotent under the concurrent VAD fan-out.

Ran the tests in a worktree at this head: test_sync_opus_decode.py 39 passed, test_sync_v2.py 182 passed, test_wal_audio_coverage_pipeline.py + test_sync_silent_failure.py 94 passed. The one failure in test_sync_capture_pipeline.py (fakeredis unknown command 'eval') reproduces identically on main — environment, not this PR.

One minor non-blocking note: the deprecated v1 lane (routers/sync.py, /v1/sync_local_files) also calls retrieve_vad_segments but works in the shared syncing/{uid}/ directory that is only cleaned per file, so empty .vad/<hash>/ directories will accumulate there. Removing the segment dir when it becomes empty after _cleanup_files (or equivalent) would close that.

The listen_pusher_pipeline.mdx addition is accurate against the code, including the Redis path-checkpoint mismatch note. Before marking ready for review: the repo requires a Failure-Class: declaration on fix: commits (scripts/pr-preflight --suggest) and a make preflight run — that's the remaining checklist item.

Solid fix and reproduction — thank you.

---

This review was produced by AI automation on behalf of the maintainers.

---

by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

[Git-on-my-level]

Thanks for this fix and the exceptionally clear write-up — the root-cause analysis and disclosure of deployment effects are exactly what makes a data-loss bug reviewable.

I verified the core claim and the surrounding pipeline behavior at the current head:

• backend/utils/sync/pipeline.py — the collision is real: both WALs previously exported to {path_dir}/{segment_timestamp}.wav, so equal absolute speech starts clobbered the earlier bytes and collapsed segmented_paths. The .vad/<sha256(basename)>/ namespace fixes it while keeping the numeric basename. I confirmed downstream invariants hold: parse_sync_filename_timestamp only looks at the basename, chronological STT assignment (sorted by get_timestamp_from_path) is unaffected, and compute_sync_segment_id hashes basename+bytes, so durable content IDs stay stable across job directories as claimed. The _cleanup_files addition is appropriately conservative — the rmdir only fires for a literal .vad parent with a 64-char hex name, os.rmdir can never drop a nonempty namespace, and the shared .vad root is retained for concurrent writers. The FileNotFoundError recreate-and-retry-once window is narrow and persistent export failures still propagate.
• backend/tests/unit/test_sync_opus_decode.py — good coverage of the failure mode: both sequential orders, the real concurrent VAD phase, content-ID identity across retry directories, namespace-only-after-last-segment cleanup, the injected cleanup race, and persistent-failure propagation. The cleanup guard test with ('.vad', 'manual') and a 64-char name that is not hex is a nice touch.
• backend/docs/listen_pusher_pipeline.mdx — accurate against the code, including the honest note about legacy Redis path checkpoints no longer matching and the one-time STT replay for jobs without the durable ledger.

One thing for maintainers to weigh at mark-ready time (also flagged in the description): the path-checkpoint invalidation means legacy in-flight jobs lacking the content-ID ledger may replay STT once after deployment. The transcript-assignment dedupe limits the impact, but that deployment window deserves a maintainer look alongside CI results.

No blocking concerns from me. Leaving for human maintainer review before merge.

---

by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

[undivisible]

@chinokikiss this conflicts with main now. Please rebase and push; it still looks valid.

[github-actions]

Hey @chinokikiss 👋

Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request.

After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:

• Project standards — Ensuring consistency across the codebase
• User needs — Making sure changes align with what our users need
• Code best practices — Maintaining code quality and maintainability
• Project direction — Keeping aligned with our product principles and locked invariants

Before your next PR, please skim:

• PRODUCT.md — product north star
• Product invariants — locked rules (shared chat, memory tiers, agent control plane, integrations, brand)

If this was declined for direction or taste, maintainers should cite an invariant ID or open a proposed one — ask if that citation is missing.

Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out.

Thank you for being part of the Omi community!