fix: make file-producing task retries idempotent

[charan-rathore]

Summary

Fixes #32.

File creation currently commits before the document task checkpoint, model tool cache, or attachment-import mapping. Retrying after losing that later checkpoint produces another random file ID and another PDF.

This patch gives those internal operations an opt-in stable identity:

• Document/model fills bind the owner, task, source PDF and canonically ordered field values.
• Attachment imports bind the owner, connection and attachment reference.
• A complete temporary PDF is atomically linked into place without overwriting an existing result. A retry can recover a published file whose metadata insert failed; concurrent metadata inserts use the existing insertIfAbsent primitive.
• Model retries keep the reused file ID only once in artifactIds, including when the artifact checkpoint succeeded but the following tool-cache checkpoint failed.

Ordinary manual imports/fills still create independent files. No schema migration, approval changes, or retries of external provider writes are introduced.

Regression evidence

Before the fix, the document-worker regression failed: after losing the filledId checkpoint, the retry produced a different PDF ID. With the fix, it retains the original artifact and reaches the existing approval flow.

Coverage includes both model checkpoint gaps, attachment mapping failure and reconnection isolation, file publication before failed metadata persistence, concurrent replay, reordered field inputs, owner/operation/input isolation, and independent manual fills. Tests use fictional fixtures and mocked external boundaries.

Validation

On Apple Silicon, Node 24.21.0 / pnpm 11.19.0, with expensive checks serialized:

• pnpm exec tsx --test --test-concurrency=1 tests/*.test.ts apps/mobile/test/*.test.ts — 161 passed before adding the second model checkpoint parameterization.
• pnpm exec tsx --test --test-concurrency=1 --test-name-pattern='model fill reuses' tests/model-worker.test.ts — both checkpoint cases passed on the final test revision.
• pnpm lint — passed.
• pnpm exec tsc --noEmit — passed (root/server/tests; not mobile application typecheck).
• pnpm --dir apps/worker typecheck — passed.
• pnpm build:server — passed.
• git diff --check — passed.

Not run locally: mobile application typecheck or web/iOS/Android exports, Chromium/container integration, live Google/Intelligence accounts. Persistence failures are deterministically injected; these are not actual process-kill or power-loss tests. The change does not add fsync guarantees or cleanup of temporary files left by a killed process.

Scope and attribution

Rechecked the issue and open PRs by issue number and file/artifact replay keywords; no competing #32 fix found. In particular, #37 addresses completed-action approval replay (#36), not file-producing operation identity.

AI assistance (OpenAI Codex, with Instinct research and Jev decision support) was used for implementation, tests and review. Validation above was executed locally; untested paths are explicitly listed.

[kvnloo]

I think this gets to the root of #32 rather than just closing the three observed crash windows.

The important change to me is that file identity now exists before the task/cache receipt:

logical operation → stable file ID → publish effect → persist/recover receipts

instead of deriving operation identity from whichever checkpoint happened to survive.

The tests around lost filledId, lost artifact/cache checkpoints, missing import mappings, concurrent replay, reordered fields, and reconnection isolation make that contract pretty convincing.

One thing I'd preserve as the rule for future file-producing tools is:

retries of one logical operation converge on one artifact; intentionally separate operations still produce separate artifacts.

The manual-fill assertions are especially useful because they prove idempotency hasn't accidentally become content deduplication.

I don't see another change I'd ask for here. The explicitly-unclaimed power-loss/fsync boundary also seems like the right scope line for this fix.

AI-use note: I used an AI assistant to trace the operation-key construction and failure-injection tests against #32; I verified the current PR head before posting.

[charan-rathore]

Rebased onto current main and reran the checks locally: tsc --noEmit is clean and the touched suites all pass (api 9, model-worker 3, workflows 7). Ready for review whenever you have a moment - CI just needs the first-time-contributor approval.

[davidmckayv]

Approved to merge, but it now conflicts with main in tests/model-worker.test.ts after #51 landed. Rebase onto main and it will be merged once CI is green.

[charan-rathore]

Thanks, David. I rebased #45 onto current main after #51, and the PR is mergeable now. The latest CI run on head 0508f59 is still waiting for first-time-contributor approval. Could you approve that run and take another look when you have a chance? Happy to address any failing checks.

[charan-rathore]

Thanks for the close read, kvnloo - really appreciate it. Keeping that rule as the contract for future file-producing tools: retries of one logical operation converge on one artifact, intentionally separate operations produce separate artifacts. Glad the manual-fill assertions made the idempotency-vs-dedup line clear.

[charan-rathore]

Hi @davidmckayv, gentle bump - the PR is rebased and showing as mergeable. Could you approve the Actions run when you get a chance so CI can run? Thanks!

[jerelvelarde]

Reviewed head 0508f59b5dd9.

This is a high-value template reliability fix: a logical fill/import gets a stable artifact identity before the later task/cache/mapping checkpoint, while independent manual operations remain independent. The retry, concurrency and publication-before-metadata tests target the failure windows that matter.

The existing maintainer feedback is positive. The immediate next step is a passing CI run on the current rebased head. Please also coordinate with #47: this PR generates 64-character hex IDs for keyed operations, while that PR accepts only UUID file IDs. They cannot land together unchanged. Keep round-trip coverage for the combined ID contract through file lookup, content reads and attachment use.

No additional blocker found in this static review of this PR by itself; tests were not rerun.

[charan-rathore]

Thanks @jerelvelarde for the review. I will rebase the head so CI gets a clean run, and reconcile the ID contract with #47 (64-char hex keyed IDs vs UUID-only file IDs), including round-trip coverage through file lookup, content reads and attachment use.

[charan-rathore]

Rebased onto main and made the keyed IDs UUID-shaped (same sha256 identity, version and variant bits set) so they work with #47, with a round-trip test through file lookup, content reads and attachments. CI just needs the workflow approval to run.