datastore sync --push runs pushChanged() twice per invocation (coordinator dedup)

Summary

swamp datastore sync --push invokes S3CacheSyncService.pushChanged() twice on every invocation — once from the command action and again from flushDatastoreSync() at runCli teardown. Both walks scan the full cache and both rounds do S3 PUTs. This is pure waste on the success path, and on the failure path it produces misleading log ordering where "Pushed 1 file(s)" prints before the original error surfaces.

This is a separable follow-up to swamp-club issue #29. Issue #29 covers the primary bug (_catalog.db-wal fails because the catalog lives inside the sync cache) and has its own fix path. This issue is specifically about the coordinator/command-level push duplication and should be fixed independently because the risk profile is different — getting the signalling wrong can cause silent push no-ops, which is worse than the current double-push waste.

Code path

1. src/cli/commands/datastore_sync.ts:68-71 — --push mode uses requireInitializedRepo (not the read-only variant).
2. src/cli/repo_context.ts:325-329 — requireInitializedRepo calls registerDatastoreSync({ service, lock, label }) for S3 datastores, registering the S3CacheSyncService with the global sync coordinator.
3. src/cli/commands/datastore_sync.ts:76-79 — the command action then calls datastoreSync(ctx, deps, { mode: 'push' }).
4. src/libswamp/datastores/sync.ts:172-177 — datastoreSync with mode: 'push' calls deps.pushSync() which calls syncService.pushChanged(). First push.
5. src/cli/mod.ts:939 — after cli.parse returns, runCli calls flushDatastoreSync().
6. src/infrastructure/persistence/datastore_sync_coordinator.ts:196-247 — flushDatastoreSync() iterates all registered entries and calls entry.service.pushChanged(). Second push.
7. src/cli/mod.ts:991 — on error paths, the catch block calls flushDatastoreSync() again, compounding the confusion.

Evidence that the maintainer is already aware

src/cli/commands/datastore_sync.ts:61-62 explicitly comments:

``` // Pull-only mode uses read-only init to avoid acquiring the global // lock and triggering a coordinator push on flush. ```

…and uses `requireInitializedRepoReadOnly` for the pull path specifically to dodge this. The push path hasn't been similarly adjusted.

Impact

• Every S3 `--push` invocation does 2× the S3 PUT cost and 2× the cache walks. Doubled latency, doubled API costs, doubled race surface on any mutable file in the cache (this is what makes the `_catalog.db-wal` failure in issue #29 so confusing — the first push hits the WAL mid-write, the second push runs after SQLite has checkpointed the WAL, so the logs show "Pushed 1 file(s)" success before the original error surfaces as fatal).
• Log ordering is misleading on failures. The second push runs after the first has thrown, so the success message for the second push appears above the fatal error from the first. Anyone debugging a failed push will see conflicting evidence in the log trace.

Suggested fix approaches

Two viable directions, each with a failure mode to watch:

A. Command skips the explicit push, relies on coordinator flush. Remove the `deps.pushSync()` call from `datastoreSync({ mode: 'push' })` and let `flushDatastoreSync()` at `runCli` teardown do the push. This mirrors how normal commands (`model create`, `workflow run`) already work — the coordinator flush is the canonical push point. Risk: the coordinator flush at `datastore_sync_coordinator.ts:235-246` logs failures as `warn` rather than propagating them, so the CLI could exit 0 on a broken push. Any fix in this direction must also make flush errors propagate so the command exits non-zero on push failure.

B. Coordinator flush skips its push when an explicit push has already run. Add a flag on the coordinator entry (e.g. `pushedExplicitly: true`) that `datastoreSync` sets after a successful `pushSync()`, and that `flushDatastoreSyncNamed` checks before calling `pushChanged`. The lock still gets released on flush. Risk: state has to survive the error path — the catch at `src/cli/mod.ts:991` calls flush even when the explicit push threw, and in that case the flag is `false`, so flush would retry the push. That might actually be desirable on transient network errors, but it reintroduces the exact double-push the fix is trying to eliminate.

Approach A is cleaner architecturally — one push point, consistent with how other commands work. Approach B is more surgical but has more moving parts.

Testing requirements

Whatever approach is taken, regression tests must assert:

1. Exactly one `pushChanged` call on the success path of `swamp datastore sync --push`.
2. Exactly one `pushChanged` attempt on the failure path (no duplicate attempts after a throw).
3. The CLI exits non-zero when the push fails (current behavior only exits non-zero because the first push throws; if the first push is removed, the flush-time push must still produce a non-zero exit on failure).
4. `swamp datastore sync --pull` and full `swamp datastore sync` behavior is unchanged (the pull path already uses read-only init and shouldn't regress).

Integration tests should exercise this with a mock `DatastoreSyncService` that counts `pushChanged()` calls — `src/cli/repo_context_test.ts` already has the lock lifecycle harness.

Environment

• swamp CLI (any recent version — this is structural, present since `requireInitializedRepo` started registering sync services with the coordinator)
• Any `@swamp/s3-datastore`-backed repo
• Affects all `swamp datastore sync --push` invocations, not just failing ones

Related

• swamp-club issue #29: "datastore sync --push fails on _catalog.db-wal: catalog SQLite DB lives inside the S3 sync cache" — the primary bug. This issue (double-push) is called out in that report as a "secondary bug" but scoped out of the main fix because the risk profile differs.