fix(datastore): clarify --timeout scope in SyncTimeoutError message#1223
fix(datastore): clarify --timeout scope in SyncTimeoutError message#1223
Conversation
Addresses review feedback on #1222: 1. The `--timeout` flag only exists on `swamp datastore sync`, but the timeout fires from `flushDatastoreSync()` after every command. A user running e.g. `swamp model run` who hit the timeout would see "Rerun with --timeout" as the first remedy, try `swamp model run --timeout 1800 ...`, get "unknown flag", and then have to scan further. Swap the order so the env var (universal escape hatch) comes first, and rephrase the `--timeout` remedy to explicitly name `swamp datastore sync` as its scope. 2. Re-export `CreateDatastoreSyncDepsOptions` from `src/libswamp/mod.ts` so consumers do not have to reach into the internal path `src/libswamp/datastores/sync.ts`, per CLAUDE.md's barrel-import convention. Adds a scope-clarification test pinning the new contract: the message must name `swamp datastore sync` explicitly and must mention the env var covers implicit syncs. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
There was a problem hiding this comment.
CLI UX Review
Blocking
None.
Suggestions
None.
Verdict
PASS — the error message rewrite is a clean UX improvement. Moving the env var (SWAMP_DATASTORE_SYNC_TIMEOUT_MS) to first position is correct: it's the universal escape hatch and most timeout firings come from implicit sync after write commands, not from an explicit swamp datastore sync. The added scope clarification ("The flag is not available on other commands; use the env var above for those.") is specific and actionable — exactly what a confused user needs. The barrel re-export of CreateDatastoreSyncDepsOptions has no UX impact. No flags changed, no output formats changed, no exit codes affected.
There was a problem hiding this comment.
Code Review
Clean, well-scoped follow-up. Both changes address real issues from the prior review.
Blocking Issues
None.
Suggestions
None — this is a tight, well-tested fix.
Details:
-
Message reordering (
datastore_sync_service.ts:79-93): The env var now leads because it's the universal escape hatch. The--timeoutremedy is explicitly scoped toswamp datastore sync. This correctly addresses the UX trap where users ofswamp model runwould try--timeoutand get "unknown flag". The message wording is clear and actionable. -
Barrel re-export (
libswamp/mod.ts:948):type CreateDatastoreSyncDepsOptionsis now exported from the barrel, fixing the CLAUDE.md import-boundary violation. Confirmed no CLI or presentation code currently imports from the internaldatastores/sync.tspath. -
Tests: The new scope-clarification test (
datastore_sync_service_test.ts:46-58) pins two concrete assertions — that the message namesswamp datastore syncand mentionsimplicitsyncs. This prevents future rewording from reintroducing the original UX trap. The existing four-remedies test is updated to match the new ordering. Test comments are appropriately verbose here since they document why the contract matters. -
DDD:
SyncTimeoutErrorremains a well-placed domain value object extendingUserError. No domain boundaries crossed, no behavioral changes — purely a message improvement with contract tests.
There was a problem hiding this comment.
Adversarial Review
Critical / High
None.
Medium
None.
Low
-
src/domain/datastore/datastore_sync_service.ts:79-80— timeout displayed in raw milliseconds could confuse users. The message renders${timeoutMs}ms(e.g."timed out after 300000ms"). While technically correct and pre-existing (not introduced by this PR), a user-facing remediation message showing300000msinstead of5mor300sis a minor UX speed bump. Not a bug and not introduced here — just noting it as a potential follow-up. -
src/domain/datastore/datastore_sync_service_test.ts:54— scope test asserts substring"swamp datastore sync"which also matches the lock-release remedy line. The assertionassertStringIncludes(err.message, "swamp datastore sync")passes because"swamp datastore sync"appears in the--timeoutscoping sentence, but it would also pass if that sentence were removed and only the lock-release remedy ('swamp datastore lock release --force') remained, since"swamp datastore lock release"starts with"swamp datastore"but does NOT contain"swamp datastore sync"as a substring — so this is actually fine. The test is correct as written; I initially suspected a false-positive but the substring match is precise enough.
Verdict
PASS — The changes are minimal, correct, and well-tested. The message reordering is a clear UX improvement, the new test pins the contract effectively, and the barrel re-export is a one-line fix that aligns with the project's import conventions. No logic, security, concurrency, or resource management concerns.
Summary
Follow-up to #1222 addressing the two actionable findings from review:
1. CLI UX Review —
--timeoutremedy was misleading for implicit syncsThe timeout fires from
flushDatastoreSync()after every command, not justswamp datastore sync. The previous message led with• Rerun with --timeout <seconds>, which would fail for a user whose timeout fired from e.g.swamp model run(the flag only exists onswamp datastore sync).Fix: reorder remedies so the env var (universal escape hatch) is listed first and the
--timeoutflag is explicitly scoped toswamp datastore sync:```
Datastore push to "@swamp/s3-datastore" timed out after 300000ms. Try one of:
• Set SWAMP_DATASTORE_SYNC_TIMEOUT_MS for the duration of your shell session — applies to every command that triggers sync, including the implicit pull/push around write commands.
• If the timeout fired on an explicit 'swamp datastore sync', rerun with --timeout (e.g. --timeout 1800 for large one-off syncs). The flag is not available on other commands; use the env var above for those.
• If you are on @swamp/s3-datastore or @swamp/gcs-datastore at scale, update to the latest extension — the fingerprint fast path short-circuits zero-diff syncs.
• If a prior process crashed without releasing the lock, run 'swamp datastore lock release --force' (add --model / for a specific model lock).
```
New scope-clarification test pins the contract: the message MUST name
swamp datastore syncexplicitly and MUST mention the env var covers implicit syncs. That prevents a future well-meaning reword from reintroducing the same trap.2. Code Review + Adversarial Review —
CreateDatastoreSyncDepsOptionsnot re-exported from barrelThe new interface lived at the internal path
src/libswamp/datastores/sync.ts. Any consumer outside the CLI wanting to type an options variable would have had to violate CLAUDE.md's barrel-import convention. One-line fix: addtype CreateDatastoreSyncDepsOptionsto the export fromsrc/libswamp/mod.ts.What this does NOT change
SyncTimeoutError(label,direction,timeoutMs,cause) are unchanged.Test plan
deno check— cleandeno lint— clean (1094 files)deno fmt— cleandeno run test— 4843 passed / 0 faileddeno run compile— binary builtSyncTimeoutError: --timeout remedy is scoped to 'swamp datastore sync'passesLinks