fix(#19): vault all skips broken cache entries instead of aborting#25
Conversation
…rting
exportAllPapersToVault used Effect.forEach in fail-fast mode, so the
first cache entry missing paper.md aborted the whole batch — every
valid entry after it was silently dropped. Building any non-trivial
vault (10+ papers) reliably hit one stale entry and forced the user to
script around the command per-paper.
Each entry now runs through an isolated catch and produces either an
ok or failed attempt record. The command prints a summary
Exported N of M papers to <vault>
Skipped:
<id> — <reason>
and exits 0 as long as at least one paper landed. Only when every
entry failed (and the cache wasn't empty to begin with) does the
command fail with a new VaultExportAllFailed error and exit 1.
Closes p7dotorg#19.
| if (result.exported.length === 0 && result.total > 0) { | ||
| return yield* Effect.fail(new VaultExportAllFailed({ message: "all cache entries failed to export" })) | ||
| } |
There was a problem hiding this comment.
No need to Effect.fail here, since you used Data.TaggedError you can just yield it
EduSantosBrito
left a comment
There was a problem hiding this comment.
Effect-idiomatic review: requesting changes.
The product behavior is right: vault all should not throw away the whole batch because one cache entry is stale. The part I would tighten is the error boundary.
Why this matters in Effect: the E channel is not just an exception bucket; it is the typed control plane of the program. We should only recover from errors that the current operation semantically knows how to recover from. If we catch everything and turn it into a string, the caller loses the ability to distinguish "one bad cache entry" from "the vault cannot be written at all".
Concrete concern:
exportOne(vaultPath, id).pipe(
Effect.map((written): ExportAttempt => ({ kind: "ok", result: written })),
Effect.catch((error): Effect.Effect<ExportAttempt> =>
Effect.succeed({ kind: "failed", id: entry.id, reason: error.message })
)
)This catches all VaultErrors. That includes expected per-entry problems like VaultCacheMissing, but also infrastructure/systemic problems like VaultFsError from permission errors, disk full, failed writes, etc. Those should probably fail the whole command loudly rather than being reported as a skipped paper.
Suggested Effect-style approach:
- Convert only expected per-entry cache problems into skipped entries, e.g. with
Effect.catchTags({ VaultCacheMissing, VaultCacheMalformed }). - Let
VaultFsError,VaultInvalidPath, and config/path failures stay in the error channel. - Keep the failed entry payload typed for as long as possible; only turn it into a display string in the CLI renderer.
Something like:
exportOne(vaultPath, id).pipe(
Effect.map((result): ExportAttempt => ({ _tag: "Exported", result })),
Effect.catchTags({
VaultCacheMissing: (error) => Effect.succeed({ _tag: "Skipped", id: entry.id, error }),
VaultCacheMalformed: (error) => Effect.succeed({ _tag: "Skipped", id: entry.id, error })
})
)That preserves the intended skip-and-warn semantics without flattening the full typed error model.
Verification: I checked this PR locally with tsc --noEmit and the vault tests; they pass.
|
Follow-up: a couple of Effect APIs would fit this PR especially well.
Use this to recover only from the cache-entry failures that are valid to skip: exportOne(vaultPath, id).pipe(
Effect.map((result): ExportAttempt => ({ _tag: "Exported", result })),
Effect.catchTags({
VaultCacheMissing: (error) => Effect.succeed({ _tag: "Skipped", id: entry.id, error }),
VaultCacheMalformed: (error) => Effect.succeed({ _tag: "Skipped", id: entry.id, error })
})
)That leaves
If the intention is truly "make this entry's failure data", const attempt = exportOne(vaultPath, id).pipe(Effect.result)Then render later with Why this matters for someone new to Effect: catching an Effect error does not have to mean "turn it into a string". You can either catch specific tags, or intentionally turn the typed success/failure into data with |
Replace the broad Effect.catch in exportAllPapersToVault with
Effect.catchTags({ VaultCacheMissing, VaultCacheMalformed }). Only
per-entry cache failures (missing paper.md, unsafe target path,
unparseable cached id) are converted to skipped entries; VaultFsError,
VaultInvalidPath, VaultConfigMissing now keep failing the whole
command instead of being reported as a skipped paper.
ExportAttempt and VaultExportFailure hold the typed error object until
the CLI renderer turns it into a display line. The visible 'Skipped:
<id> — <message>' format is unchanged.
Also drop the redundant Effect.fail(new VaultExportAllFailed(...)) in
favour of bare 'yield* new VaultExportAllFailed(...)' (Data.TaggedError
instances are themselves Effects), matching the existing codebase
convention.
|
Addressed both points in 6ca7743:
|
Summary
exportAllPapersToVaultso each cache entry is exported under an isolatedEffect.catch, producing per-id ok/failed records instead of failing the whole batch on the first error.VaultExportAllFailedtagged error and updates thevault allcommand to print a summary and exit 1 only when every entry failed.Exported N of M papers to <path>and aSkipped:block listing each failed<id> — <reason>.Why
paper7 vault allusedEffect.forEachin fail-fast mode. The first cache entry missingpaper.mdaborted the whole batch — every valid entry after it was silently dropped. Building any non-trivial vault (10+ papers from a research session) reliably hits at least one stale entry, so the only workaround today is scriptingpaper7 vault <id>per paper while swallowing failures. Skip-and-warn semantics fix that without giving up on noisy-but-recoverable cache state.Behaviour matrix
Exported N of N papers to <path>Exported K of N papers to <path>+Skipped:blockerror: vault export failed: all cache entries failed to exporton stderrExported 0 of 0 papers to <path>Test plan
npx vitest run→ 171 pass / 0 fail (3 new vault tests + existing tests updated for the new summary format).npx tsc --noEmitclean.Skipped:), all-fail (exit 1 with new error), empty cache (exit 0, zero-count summary).Closes #19.