fix(sdk): fs adapter flush() tracks in-flight writes; add to T13 contract suite

[vanceingalls]

What

Brief description of the change.

Why

Why is this change needed?

How

How was this implemented? Any notable design decisions?

Test plan

How was this tested?

• Unit tests added/updated
• Manual testing performed
• Documentation updated (if applicable)

[vanceingalls]

• feat(sdk): stage 6 — sub-composition scoped ids (F9) #1434 : 2 dependent PRs (#1429 , #1441 )
• feat(sdk): stage 5 — export adapter factories from package root #1432
• feat(sdk): stage 4 — canUndo/canRedo, removeElement GSAP cascade, override-set cleanup #1431
• feat(sdk): can() returns CanResult; T4 dispatch-boundary tests #1426
• fix(sdk): fs adapter flush() tracks in-flight writes; add to T13 contract suite #1425 👈 (View in Graphite)
• feat(sdk): file-backed fs adapter + setTiming GSAP-script sync; add sdk-playground #1423
• refactor(core): swap studio-api read path from recast to acorn parser (T6e) #1392
• feat(core): parse-parity suite for acorn parser (T6d) #1370 : 1 other dependent PR (#1379 )
• feat(core): acorn GSAP write path — magic-string offset-splice (T6c) #1369
• feat(core): acorn GSAP read path with T6b differential corpus tests #1368
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[james-russo-rames-d-jusso]

Verified at HEAD d22af04.

Tight, correct fix. The inflightWrites: Set<Promise<void>> pattern is the right shape — doWrite is wrapped so the promise is registered BEFORE await, then removed in finally, which means flush() sees every in-flight write whether settling, mid-write, or rejected. Promise.all([...inflightWrites]) snapshots the set at flush-time, so writes that arrive AFTER the flush call don't extend it — that matches typical adapter contract semantics.

The bonus versionCounter monotonic suffix on the appendVersion key is the right call — Date.now() collisions under fast sequential writes would have silently dropped versions when the lexicographic-sort prune ran. Padded to 6 digits → 1M writes/ms headroom, plenty.

T13 contract suite running against mkdtempSync(tmpdir()) per-test is clean: real fs, isolated, no cross-test pollution.

Concerns

• flush() rejection semantics: Promise.all rejects on the FIRST in-flight rejection, which means if write A rejects and B is still pending, B's eventual error never surfaces through flush(). Cross-check with the T13 contract — does the spec require flush to surface ALL errors or just the first? If first-error-only is intentional, a one-line comment would help; otherwise consider Promise.allSettled + aggregate. Not blocking — the errorHandlers callback already fires per-write inside doWrite's catch in the prior shape (this PR's diff doesn't touch that path), so observability is preserved.
• Lens item (c) "error during write reflected in flush rejection" — the T13 diff in this PR (+9 lines) doesn't appear to add an explicit error-during-write-then-flush case. If the existing suite already covers it on the memory adapter and the runPersistAdapterContract runs across both, fine; worth a quick rg "flush.*reject" packages/sdk/src/adapters/persistAdapter.contract.test.ts to confirm.

Determinism: no Date.now() / Math.random() / fetch() in render-time paths. Date.now() is adapter-write (author-time) — fine.

Verdict: approve-with-concerns (none blocking; first-error semantics worth a comment or follow-up).

Review by Rames D Jusso

[miguel-heygen]

CI is blocked by the oxfmt failure in #1423 (root of the stack). The code change itself is clean.

Strengths:

• packages/sdk/src/adapters/fs.ts — tracking in-flight writes as a Set<Promise<void>> that removes itself on settle is the correct pattern; flush() correctly snapshots the current set via Promise.all([...this.inflightWrites]) so concurrent writes started during the await don't defer the caller's flush boundary.
• packages/sdk/src/adapters/fs.ts:versionCounter — ${Date.now()}_${String(counter).padStart(6,"0")} guarantees lexicographic = insertion order within the same millisecond; version stability fix is correct.
• Adding the inflight tracking to the T13 contract suite is good — contracts are the right place to pin this.

Important:

• packages/sdk/src/adapters/fs.ts flush() — there is no timeout or cancellation path. If an in-flight write() never resolves (e.g. underlying fs.promises.writeFile hangs), flush() blocks forever. Low-urgency for a Node-only adapter, but worth a follow-up.

Verdict: APPROVE (pending #1423 oxfmt fix)
Reasoning: Correct fix, well-tested. Unblocked as soon as the root format failure in #1423 is resolved.

— magi

[vanceingalls]

@james-russo-rames-d-jusso — flush() semantics documented

Added a one-line comment to flush() clarifying that Promise.all rejects on first write failure, and that per-write errors are independently surfaced via the persist:error event channel. First-error semantics are intentional — callers who need all-errors should subscribe to persist:error directly.

No change to the Promise.allSettled alternative for now; the event channel provides complete observability.

The base branch was changed.