Skip to content

PT-4214 Stage U: backend-authoritative per-project S/R block signal (E2E defects 2+3)#2574

Open
rolfheij-sil wants to merge 10 commits into
pt-4159-edit-block-followupsfrom
pt-4214-per-project-block-signal
Open

PT-4214 Stage U: backend-authoritative per-project S/R block signal (E2E defects 2+3)#2574
rolfheij-sil wants to merge 10 commits into
pt-4159-edit-block-followupsfrom
pt-4214-per-project-block-signal

Conversation

@rolfheij-sil

@rolfheij-sil rolfheij-sil commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

What

PT-4214 Stage U slice: the backend SendReceiveWriteLock becomes the single, per-project
block-signal authority for ALL sync types (manual, scheduled, startup/shutdown), and the
renderer consumes it as a snapshot. Fixes two of Rolf's 2026-07-16 E2E defects at the root:

  • Manual S/R now blocks editing of the synced project (defect 2): the signal originates
    from the gate the Streamlined a number of npm scripts #164 bracket arms on every sync path — no UI path can miss it.
  • Only the synced projects' editors are blocked (defect 3): rejection and the renderer
    driver are both per-project now (previously: global latch, every Scripture editor frozen).

C# (public core, inert upstream)

  • SendReceiveWriteLock.EnterWrite rejection is now per-project (armed ∧ projectId ∈ blocked set); the drain stays global. This deliberately reverses the "global gate"
    rejection semantics
    shipped in PT-4159: auto-sync blocking overlay (edit block for scheduled S/R) #2555/PT-4159: Rebuild S/R write gate without thread affinity #2564: an armed gate rejecting writes to projects it
    isn't syncing is exactly E2E defect 3 at the data layer (once Streamlined a number of npm scripts #164 arms the gate for real),
    and PT9 parity locks only the syncing project. Memory-ordering is unchanged and safe: the
    volatile blocked set is published before the arming CAS, so an armed reader always observes
    the matching set (reasoning documented at the check; reviewed adversarially — verdict sound).
    The EnterWrite_WhileAnotherProjectSyncs_RejectsAllProjects_GlobalGate test was consciously
    replaced by per-project equivalents.
  • New change surface: BlockStateChanged event + GetBlockState() snapshot (raised post-arm /
    post-real-disarm only; stale-token and no-op clears don't raise; out-of-order delivery in
    off-contract crash-recovery races documented as accepted YAGNI).
  • New SendReceiveBlockNotifierService: forwards gate transitions as network event
    paratextBibleSendReceive.onSyncWriteLockChanged ({ isBlocking, projectIds } snapshots) and
    serves paratextBibleSendReceive.getAutoSyncBlocking — the command Auto-sync edit-block follow-ups from #2555 review (PT-4159 / PT-4214 Stage T) #2571's init-seeding
    already calls but nothing served until now. Deviation from PT-4214's open item: the ticket
    suggested registering that command ext-side; serving it from C# is the Stage U end state (the
    gate IS the authority, and it works for all sync types + before extension activation). Round 4: the event is now
    formally registered with main's central event registry at init via a generic
    network:registerEvent request (that method already serves every websocket client; no new
    API — mirrors RegisterRequestHandlerAsync's network:registerMethod call), so the boot-time
    “announced but is not registered” deprecation warning no longer applies to this event. Emission
    itself still follows the SharedStore SendEventAsync precedent; the other C#-origin events
    (<id>-pdp-data:onDidUpdate, shared-store:change) remain on the legacy unregistered-announce
    path — platform-wide migration debt, out of scope here.
  • Sentinel (SR_EDIT_BLOCKED) byte-identical; SetSyncing/Clear/EnterWrite signatures
    unchanged (the in-flight Streamlined a number of npm scripts #164 patch bracket compiles against this unmodified).

Renderer

  • auto-sync-blocking-store: ref-count latch → snapshot of blocked projectIds (200 ms
    show-grace kept). The Stage-T per-blocker safety leashes are deleted, not retained — per
    the ratified PT-4214 §5.2 ("a second, timer-driven opinion about blocking is precisely the
    drift findings 7/8/16 indict"); resilience is re-query of the authority.
  • auto-sync-blocking-service: subscribes onSyncWriteLockChanged only — the ext-emitted
    onAutoSyncBlockingChanged subscription is removed (single source, finding 16). Init
    consult of getAutoSyncBlocking kept (now actually served), live-event-wins guard kept,
    malformed payloads fail safe to unblocked.
  • auto-sync-edit-block-driver: flags an editor iff definition.projectId is in the blocked
    set; handles partial set transitions ({A,B}→{A}, {A}→{B} swap) with the
    unsubscribe-before-unflag ordering preserved (the PT-4159: auto-sync blocking overlay (edit block for scheduled S/R) #2555 live-E2E permanent-block bug class);
    regression tests cover full clear, shrink, swap, grow, rebuilt-mid-block, no-projectId.

Runtime pairing (IMPORTANT)

The signal only fires once the studio patch arms the gate — merge in the same window as
studio #164
(gate arming + stall watchdog). Pairings:

Once merged, the ext engine's onAutoSyncBlockingChanged emit chain
(in the send-receive extension's activation + engine setBlocking path) is vestigial —
removal tracked on PT-4214 (Stage U cleanup), not done here to keep #186's review diff stable.

Out of scope (Stage U remainder, tracked on PT-4214)

Finding 18's editor-mount refactor (driver deletion), the shared latch-store extraction,
throwOnDrainTimeout upgrade path, seam d.ts declarations for the new event/command (deferred
to avoid colliding with #2570's d.ts re-home; the renderer/service use local types).

Verification

C# dotnet test: full suite 1528 passed / 0 failed / 6 skipped (now 1529 with the review-fix
test); SendReceive-filtered 66/0; CSharpier clean. Renderer: targeted 50/0 across
store/service/driver; full core TS suite 1018/0 including app.component.test; typecheck clean
(one pre-existing unrelated buildInfo.json error). Both halves adversarially reviewed
(finder→refuter); all surviving findings fixed in 438d1dd / 506e9fc, including a truthful
armed-empty-set warning and observed (logged) notifier faults. Round 4: central-registry
registration added in 756d33e (SendReceive-filtered 67/0; full C# suite 1530/0).

Rebase note (2026-07-17) — re-seated on #2571's review-fix round

Rebased onto #2571's new tip 56b9934560a (lyonsil's 19-finding review fix round: identity release
tokens in both stores, inert seeding cut, hook types, smalls). Expected supersession: #2571's
auto-sync store/service fixes conflict with this branch's snapshot rewrite and resolve to this
branch's versions — the snapshot model makes the anonymous-pairing problem the #2571 fixes
mitigated unrepresentable. One reconciliation commit added:

Post-rebase verification: renderer suite 548 passed, full typecheck clean (all workspaces), C#
suite 1530 passed / 0 failed, stack linearity verified (#2571#2574#2575).

Follow-up rebase (2026-07-17): rebased onto 37d955d1583 (papi.d.ts regen fix).

🤖 Generated with Claude Code

https://claude.ai/code/session_01VkhEPs8ocAzX4117dtfWLA

Self-review — 2026-07-17 (review-paratext methodology)

No Critical/Important defects; the per-project reversal of #2564's global gate is sound and correctly re-tested. Four analysis passes (api / style / compliance / ux) per .claude/agents/review-analyzer.md, run by a review-lead agent; author interview substituted with the PR body + PRD design records; every finding adversarially verified against code and tests.

# severity finding disposition
1 minor SendReceiveBlockNotifierServiceTests doesn't exercise the registration-failure/exception paths (DummyPapiClient hard-codes success) fixed in 234e46a
2 minor leash deletion means a lost mid-session event leaves a stale read-only banner until the next transition/reload author-context: addressed (ratified design §5.2; PT-4214 tracks editor-mount re-query)
3 minor isProjectBlocked is exported and unit-tested but has no production consumer in this PR author-context: addressed (#2575 consumer)

Verified wins: the per-project EnterWrite reversal is sound (narrowing rejection can only allow more writes, never fewer); the memory-ordering argument (volatile publish-then-flag / acquire-release pairing) is correct.

Gates: C# SendReceive-filtered 67/0, renderer targeted 50/0, typecheck:core clean (one pre-existing unrelated build-artifact error).


This change is Reviewable

rolfheij-sil added a commit that referenced this pull request Jul 17, 2026
… oldest-first pairing

Review findings 1-3 on #2571: pairing anonymous clears with the oldest
outstanding leash both releases a live blocker (a late clear after a leash
fired cancels a newer switch's leash) and starves an abandoned blocker's own
leash (every clear cancels the leash nearest to firing, so blocking persists
indefinitely under ongoing traffic).

Both stores now hand out one release token per block/switch. A token is
released exactly once - by its own clear or its own leash - and a late clear
after the leash fired is a no-op by identity. The dual counter and the
timer-array scan dissolve (findings 14/15): the live-token set is the single
source of truth.

The project-switch events now carry a switchId (declared in NetworkEvents,
emitted by platform-scripture-editor), so the workspace-updating service
pairs every finish with exactly the switch that started it - both review
traces are fixed end-to-end and pinned by integration tests that fail on the
old code. The auto-sync blocking event remains an anonymous boolean, so that
service keeps oldest-first pairing with tombstones for leash-released raises
(fixes the late-clear trace); the abandoned-raise trace is unfixable on an
anonymous wire and is resolved by the backend-authoritative snapshot model in
#2574, as documented in the service.

Also per findings 5/6/13: the inert getAutoSyncBlocking seeding is cut (the
command is registered nowhere at this tip; the doomed consult cost ~9 s of
main-process retry churn per renderer launch because requestNoRetry's flag
does not cross the wire - now documented on requestNoRetry itself). The
"Known limitation" reload note is restored, pointing at PT-4214, whose C#
SendReceiveBlockNotifierService (#2574) serves the command for real. Cutting
the seed also makes initAutoSyncBlockingService synchronous again, so the
index.tsx startup comment is accurate at this tip.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@rolfheij-sil
rolfheij-sil force-pushed the pt-4214-per-project-block-signal branch from 756d33e to 08d0d3d Compare July 17, 2026 10:58
@rolfheij-sil
rolfheij-sil force-pushed the pt-4214-per-project-block-signal branch from 08d0d3d to 22081b9 Compare July 17, 2026 11:12
@rolfheij-sil
rolfheij-sil force-pushed the pt-4159-edit-block-followups branch from 37d955d to b99d516 Compare July 17, 2026 15:35
rolfheij-sil added a commit that referenced this pull request Jul 17, 2026
… oldest-first pairing

Review findings 1-3 on #2571: pairing anonymous clears with the oldest
outstanding leash both releases a live blocker (a late clear after a leash
fired cancels a newer switch's leash) and starves an abandoned blocker's own
leash (every clear cancels the leash nearest to firing, so blocking persists
indefinitely under ongoing traffic).

Both stores now hand out one release token per block/switch. A token is
released exactly once - by its own clear or its own leash - and a late clear
after the leash fired is a no-op by identity. The dual counter and the
timer-array scan dissolve (findings 14/15): the live-token set is the single
source of truth.

The project-switch events now carry a switchId (declared in NetworkEvents,
emitted by platform-scripture-editor), so the workspace-updating service
pairs every finish with exactly the switch that started it - both review
traces are fixed end-to-end and pinned by integration tests that fail on the
old code. The auto-sync blocking event remains an anonymous boolean, so that
service keeps oldest-first pairing with tombstones for leash-released raises
(fixes the late-clear trace); the abandoned-raise trace is unfixable on an
anonymous wire and is resolved by the backend-authoritative snapshot model in
#2574, as documented in the service.

Also per findings 5/6/13: the inert getAutoSyncBlocking seeding is cut (the
command is registered nowhere at this tip; the doomed consult cost ~9 s of
main-process retry churn per renderer launch because requestNoRetry's flag
does not cross the wire - now documented on requestNoRetry itself). The
"Known limitation" reload note is restored, pointing at PT-4214, whose C#
SendReceiveBlockNotifierService (#2574) serves the command for real. Cutting
the seed also makes initAutoSyncBlockingService synchronous again, so the
index.tsx startup comment is accurate at this tip.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
rolfheij-sil and others added 10 commits July 17, 2026 17:41
…lock-state change event (PT-4214 Stage U)

Reverse SendReceiveWriteLock's rejection from a process-wide global gate to a
per-project one: while a set of projects is syncing, EnterWrite now rejects only
writes to THOSE projects — a write to any other project proceeds. This is a
deliberate product requirement (PT9 parity: a sync locks only the project it is
syncing, so a user can keep editing project B while project A syncs). Both
EnterWrite and IsBlocked now consult _blockedProjectIds; the core mutual-exclusion
invariant is unaffected (narrowing rejection can only allow more writes, and the
count++/arm ordering still bars any write to a synced project from racing that
project's file replacement). The read of the blocked set is safe against tearing
because SetSyncing publishes it data-then-flag, so a reader that observes the armed
bit always observes the matching set.

Add a backend-authoritative change-notification surface on the gate:
- public event Action<SendReceiveBlockState>? BlockStateChanged, raised after a
  successful arm (with the armed ids) and after any real disarm (empty); no-op
  Clear()/stale-token Clear(long) do not raise. Subscriber faults are swallowed
  (this pure class has no logger; the notifier owns real error handling).
- public GetBlockState() returning a best-effort snapshot from the armed flag + set.
- readonly record struct SendReceiveBlockState(IsBlocking, ProjectIds) — serializes
  to the exact { isBlocking, projectIds } wire shape via the shared camelCase PAPI
  JSON options.

SetSyncing/Clear/Clear(long)/EnterWrite signatures are unchanged (the in-flight
studio patch PR #164 calls them). Drain semantics stay GLOBAL. The class doc is
rewritten to describe the per-project reversal and the two benign flag/set-skew
windows, both of which resolve to "allow". ResetForTests now also clears
BlockStateChanged subscribers so tests can't leak subscriptions.

Tests: replace the global-gate test with per-project tests (unsynced project
allowed with a working scope that participates in a later drain; synced projects
rejected); fix the nesting-hazard test to use the same project (per-project filter
no longer rejects a different one); add event tests (arm/re-arm/disarm raise, stale
and no-op Clear do not, throwing subscriber can't break arm/clear) and GetBlockState
snapshot tests.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VkhEPs8ocAzX4117dtfWLA
…write gate to the PAPI (PT-4214 Stage U)

Introduce a startup service that forwards SendReceiveWriteLock's block-state
transitions to the renderer so the UI has a backend-authoritative view of whether
an automatic Send/Receive is blocking edits, and for which projects:
- subscribes to SendReceiveWriteLock.BlockStateChanged and fire-and-forgets a
  paratextBibleSendReceive.onSyncWriteLockChanged PAPI event (try/catch + log,
  mirroring SharedStore.Set's event-send handling).
- registers command:paratextBibleSendReceive.getAutoSyncBlocking returning the
  current GetBlockState() snapshot so a renderer can seed on demand.
Both carry the same { isBlocking, projectIds } wire shape.

Wire the service into Program.cs's startup Task.WhenAll alongside the other PAPI
services. Inert in open-source Platform.Bible: nothing arms the gate there, so the
event never fires and the command always returns not-blocking. Because PapiClient
has no network:registerEvent counterpart, the event is an unregistered announcement
following the existing SharedStore STORE_CHANGE_EVENT precedent (TODO PT-4214
follow-up when C# gains registerEvent).

Tests: SendReceiveBlockNotifierServiceTests verifies the command is registered, a
gate arm/clear pushes the event with the right name + snapshot payload, the command
returns the current snapshot, and SendReceiveBlockState serializes to the camelCase
wire shape. Add a DummyPapiClient.InvokeRequestHandler test helper to invoke a
locally-registered handler without a live PAPI connection.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VkhEPs8ocAzX4117dtfWLA
…hot (PT-4214 Stage U)

Replace the ref-counted boolean model with a backend-authoritative SNAPSHOT
model: the store now holds the set of project ids an automatic Send/Receive is
blocking edits on, replaced wholesale by the producer via the new
setBlockedProjects(). Empty set = not blocking.

Add per-project accessors getBlockedProjectIds() and isProjectBlocked()
(undefined project id -> false) alongside the preserved getAutoSyncBlocking()
(any project blocked) and subscribeToAutoSyncBlocking(). The 200 ms show-grace
debounce for the derived visible flag is kept verbatim (PT9-parity UX), and
listeners now notify on any visible-set content change (so a project joining or
leaving an in-flight batch is observable), not just a boolean flip.

Delete the per-blocker safety-leash timer array and all AUTO_SYNC_MAX_DURATION_MS
usage in the store. This is the ratified PT-4214 5.2 decision: the renderer's
SAFETY_TIMEOUT_MS timer is deleted, not retained -- a second, timer-driven
opinion about blocking is precisely the drift findings 7/8/16 indict. Renderer
resilience against a lost event is now re-query of the backend authority (the
service's init consult), not a local timer. These leashes were added by Stage T
on this very base branch (7a103c7); superseding them here is deliberate.

AUTO_SYNC_MAX_DURATION_MS is kept in platform.data.ts (shutdown-tasks.ts still
uses it); its doc comment is updated to record that the renderer-store consumer
is gone per Stage U and the constant's remaining rightful homes are the
shutdown-sync bound and, conceptually, the C# stall watchdog.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VkhEPs8ocAzX4117dtfWLA
…event (PT-4214 Stage U)

Subscribe the auto-sync-blocking service to the C# backend's
paratextBibleSendReceive.onSyncWriteLockChanged event -- a full { isBlocking,
projectIds } snapshot emitted on every gate arm/disarm for ALL sync types
(manual + scheduled + session) -- and forward it to the store's new
setBlockedProjects(isBlocking ? projectIds : []).

Remove the subscription to paratextBibleSendReceive.onAutoSyncBlockingChanged
entirely: the backend gate is now the single signal source (PT-4214 finding 16);
a second renderer-side signal is exactly the drift that finding indicts. A short
comment names the replaced event and why.

Parse the snapshot defensively: a malformed or missing-field payload is treated
as block-none and warns once per service lifetime (fail-safe assume-unblocked,
consistent with the existing init-seeding philosophy -- a broken signal must
never leave editors stuck read-only).

Keep the init consult of paratextBibleSendReceive.getAutoSyncBlocking
(requestNoRetry; it is C#-served now) but parse the NEW snapshot shape, keep the
hasReceivedEvent live-event-wins race guard, and keep the fail-safe
assume-unblocked on rejection (plain Platform.Bible serves the command but
returns not-blocking; older cores lack it and the request rejects).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VkhEPs8ocAzX4117dtfWLA
…4 Stage U)

Make the auto-sync edit-block driver per-project. A Scripture editor web view is
flagged isSyncBlocked iff its definition.projectId is in the store's blocked set;
editors with no projectId, or whose project is not syncing, are never flagged.
This is the fix for E2E defect 3: syncing project A must not block edits in an
open editor of project B.

The driver now reacts to SET CHANGES (backend snapshots) rather than a boolean
raise/clear. On each store notification it diffs the new blocked set against the
last-applied one (no-op when unchanged) and re-applies desired state to every
open editor; the per-editor write-equality guard turns the re-apply into a
minimal diff (flag newly-blocked projects, unflag no-longer-blocked ones).

Preserve the two hard-won behaviors, generalized to per-project:
- mid-block subscriptions: onDidOpenWebView flags a newly-opened editor whose
  project is blocked; onDidUpdateWebView re-flags a rebuilt editor of a blocked
  project (the editor factory forces isSyncBlocked:false on rebuild).
- THE ORDERING FIX: on any transition, tear the re-flag/open handlers down
  BEFORE applying the diff, because updateWebViewDefinitionSync fires
  onDidUpdateWebView synchronously and a live re-flag handler would observe its
  own unflag write and re-flag permanently. Chosen shape: unsubscribe handlers,
  apply the full diff, then resubscribe over the new set if it is non-empty.
  This is correct for PARTIAL transitions too ({A,B} -> {A}): B's editors unflag
  with no live handler to bounce them, A's editors keep their flag, and the
  rebuilt {A} handlers still protect A on a later rebuild.

Adapt the regression tests to per-project semantics (projectId-carrying editors,
getBlockedProjectIds set mock): defect-3 two-project isolation, partial shrink
{A,B}->{A}, own-unflag-must-not-re-flag incl. partial transitions, rebuilt
re-flag only for the blocked project, no-projectId editor never flagged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VkhEPs8ocAzX4117dtfWLA
…er-project docs, observed notifier faults (PT-4214)

Adversarial review of the per-project write-gate found 5 low-severity
doc/behavior-description issues:

- SetSyncing's all-invalid-batch comment/warning claimed an armed empty
  set "rejects writes exactly like any other arm". Under the per-project
  EnterWrite check (armed && set.Contains(id)) that's false: an empty
  set matches nothing, so such an arm blocks NOTHING. Rewrote the
  comment/warning to say so, and pinned it with a new NUnit test
  (SetSyncing_AllInvalidBatch_ArmsButBlocksNoProject).
- SetSyncing's XML doc still described the old global-gate behavior
  ("EnterWrite calls for ANY project fail fast", "arms the global
  gate"); reworded to per-project language consistent with the class
  remarks/EnterWrite/IsBlocked docs.
- Added an honest paragraph to the BlockStateChanged doc: raises happen
  outside the atomic transition and carry no sequence stamp, so the two
  documented off-contract races (any-thread crash-recovery Clear(), or
  a takeover racing a stale Clear(token)) can deliver events out of
  order; self-corrects at the next transition. No sequence stamp added
  (YAGNI under the serialized-scheduler contract).
- SendReceiveBlockNotifierService's try/catch around the discarded
  `_ = PapiClient.SendEventAsync(...)` task was dead: SendEventAsync is
  async, so no exception from it can ever throw synchronously, only
  land unobserved on the discarded Task. Switched to
  ThreadingUtils.RunTask (the existing repo pattern for observing a
  fire-and-forget task's fault) so a failed send is actually logged,
  and documented the deliberate divergence from SharedStore.Set's
  same-shaped (and equally dead) precedent.
- Repaired a garbled sentence in the class doc's "one atomic word"
  paragraph.

Also checked the getNetworkEvent<T> deprecation flagged in
auto-sync-blocking-service.ts:80 — the base branch (7a103c7)
already used the same deprecated explicit-type-parameter overload for
the same reason (the event isn't declared in the public NetworkEvents
map), so left it unchanged to match surrounding code.

dotnet test c-sharp-tests --filter FullyQualifiedName~SendReceive:
66 passed, 0 failed. csharpier --check clean on touched files.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VkhEPs8ocAzX4117dtfWLA
…ition driver test (PT-4214)

getAutoSyncBlocking() had no consumers outside its own test (the papi command
of the same name is served independently by the C# backend); isProjectBlocked
is what the store's real consumers use, plus the upcoming project-settings UI.
Also pins the {A}->{B} pure-swap transition with a driver test, confirming a
stale onDidUpdateWebView from the swapped-out project's editor cannot bounce
it back to blocked.
… registry (PT-4214)

The block-state event was announced unregistered, drawing main's boot-time
deprecation warning like every C#-origin event. No new API is needed, though:
network:registerEvent is an ordinary main-process JSON-RPC method served to
every websocket client, and PapiClient's generic SendRequestAsync can call it
generically — the exact way RegisterRequestHandlerAsync already calls
network:registerMethod. InitializeAsync now registers the event before
subscribing to the gate, making the C# connection the event's single
registered source, so announcements pass the registry check cleanly.
Best-effort: rejection or failure is logged and emitting continues, because
announcing unregistered still works (main warns once) and backend startup
must never break over a registry hiccup.

DummyPapiClient now captures outgoing wire requests (and accepts
network:registerEvent, mirroring ConnectAsync's "pretend we succeeded") so a
test pins the registration. Other C#-origin events (-pdp-data:onDidUpdate,
shared-store:change) stay on the legacy announce path pending the
platform-wide migration.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rebase reconciliation on top of #2571's identity-pairing fix round: drop the
anonymous-boolean integration test whose event seam (onAutoSyncBlockingChanged
+ raise/clear pairing) this branch deletes - the snapshot store makes the
pairing problem it pinned unrepresentable - and update the index.tsx startup
comment, which #2571 made accurate again by cutting the inert seeding: this
branch reintroduces the (now C#-served) init consult, so the services are no
longer both synchronous (review finding 13 on #2571).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SendReceiveBlockNotifierService's InitializeAsync registers the
onSyncWriteLockChanged event with main's central registry best-effort:
on rejection (accepted == false) it warns and continues, and on a thrown
registration it catches, logs, and continues — in both cases still
registering the getAutoSyncBlocking command and still emitting gate
transitions. DummyPapiClient hard-coded network:registerEvent -> true, so
neither branch was exercised; the contract was asserted only by inspection.

Make the dummy's register response configurable via a RegisterEventResponse
Func<bool> (default unchanged: returns true), and add two tests pinning the
best-effort contract: (a) registry rejects (false) -> warning logged, command
still registered, gate transition still emitted; (b) registration throws ->
caught and logged (with the exception detail), command still registered,
transition still emitted.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011xq495P6hN2Us9qtaSzMAe
@rolfheij-sil
rolfheij-sil force-pushed the pt-4214-per-project-block-signal branch from 234e46a to 0bdfd05 Compare July 17, 2026 15:45

@lyonsil lyonsil left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few review comments below, from an AI-assisted review pass with my guidance. These are a curated subset of findings, mostly cleanups plus a couple of correctness notes (the lost-disarm recovery gap and the renderer case-sensitivity). Happy to discuss any of them.

(AI-assisted, with my guidance)

});

return unsubscribe;
(async () => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lost-disarm recovery gap — please add a pointer comment, not a code fix.

The SAFETY_TIMEOUT_MS leash is gone and this init consult is the only backend re-query — there's no re-query on websocket reconnect, C# data-provider restart, or editor mount. So if a disarm is ever lost (the data provider restarts mid-sync and comes back disarmed without re-emitting a disarm for the old arm; a fire-and-forget SendEventAsync faults; an off-contract raise reorders), the store stays non-empty and the affected editors are stuck read-only until the next transition or a full renderer reload — strictly worse than the deleted 10-minute leash, which recovered from exactly this.

I'm not asking for a code fix here — the editor-mount / reconnect re-query is the real fix and it's tracked as out-of-scope Stage-U work. But please add a short comment at this consult noting it's the only re-seed today and that lost-event recovery depends on the PT-4214 editor-mount re-query, so the gap is visible to the next reader rather than implied by the "resilience is re-query of the authority" line in the PR description.

(AI-assisted, with my guidance)

* it here verbatim (an empty array clears blocking entirely).
*/
export function setBlockedProjects(projectIds: ReadonlyArray<string>): void {
const next: ReadonlySet<string> = new Set(projectIds);

@lyonsil lyonsil Jul 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normalize project-id case once, at ingestion — don'''t rely on the front end matching case.

The C# gate matches ids with StringComparer.OrdinalIgnoreCase (its field comment even says "project ID casing varies across call sites"), but everything on the renderer side is case-sensitive: this new Set(projectIds), isProjectBlocked'''s Set.has, and the driver'''s isEditorBlocked. On any casing skew between the synced id and an editor'''s projectId, no banner shows while the backend still rejects each save with the sentinel → silent type-and-revert.

There are too many spots in the TS to make each comparison case-insensitive, and Set/map keys can'''t express a case-insensitive compare cleanly anyway. Let'''s canonicalize to a single case once at ingestion — here where the snapshot enters the store — and canonicalize the editor projectId the same way before the has check in the driver.

Confirmed the canonical case is upper: ProjectMetadata.Id is id.ToUpperInvariant() (c-sharp/Projects/ProjectMetadata.cs:15) and the editor'''s projectId flows from projectMetadata.id, so .toUpperCase() on both sides. Keep the C# OrdinalIgnoreCase as a nice backend fallback, but the front end shouldn'''t assume it can handle mixed case.

(AI-assisted, with my guidance)


applyBlockedSetToAllEditors(next);
// Snapshot a copy so a later store reassignment can never alias what we think we applied.
appliedBlockedProjectIds = new Set(next);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't advance appliedBlockedProjectIds when the apply silently failed.

applyBlockedSetToAllEditors(next) swallows a getAllOpenWebViewDefinitionsSync throw and returns early without flagging anything (the dock-not-registered case), but we still record appliedBlockedProjectIds = new Set(next) here. If the set then stays the same, the areSetsEqual guard at the top of syncState short-circuits every later notification, so any editor that was open-but-unenumerable at that instant never gets flagged for the rest of the sync (only newly opened/rebuilt ones get caught by the handlers).

Have applyBlockedSetToAllEditors report whether it actually applied, and only advance appliedBlockedProjectIds on success.

(AI-assisted, with my guidance)

// Forward every gate transition to the renderer. The subscription lives for the process
// lifetime (this service is a startup singleton, like the other PAPI services), so there is
// no unsubscribe — mirrors SharedStore's process-lifetime change-event handler.
SendReceiveWriteLock.BlockStateChanged += OnBlockStateChanged;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subscribe to the gate before the registration round-trip.

We subscribe to BlockStateChanged here, after awaiting network:registerEvent. A gate transition during that await window isn't forwarded (no subscriber yet). Since an early unregistered announce is harmless (main just logs the deprecation warning), moving the += above the registration await closes the window. Startup-only and inert in core, but cheap to make airtight.

(AI-assisted, with my guidance)

// The block state just changed (armed, with this batch's ids) — announce it. Raised here,
// right after the arm, so the signal fires on every arm including the degraded path; on the
// throwOnDrainTimeout path the subsequent rollback Clear(token) fires its own disarm signal.
RaiseBlockStateChanged(new SendReceiveBlockState(true, armedProjectIds));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arm event fires before the drain → editors flash read-only on a throwOnDrainTimeout rollback.

This raises (true, ids) before the drain. On the throwOnDrainTimeout rollback path the arm still fires this event → the renderer flags the project's editors read-only after the grace → the drain times out (seconds) → the rollback Clear(token) raises (false, []) and unblocks. Net: editors are needlessly read-only for the whole failed-drain window on a sync that aborted and touched no files.

throwOnDrainTimeout is out-of-scope here so not a blocker, but worth a note (or defer the arm-side raise until after a successful drain on that path) for whoever wires it.

(AI-assisted, with my guidance)

*/
export function setBlockedProjects(projectIds: ReadonlyArray<string>): void {
const next: ReadonlySet<string> = new Set(projectIds);
rawBlockedProjectIds = next;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rawBlockedProjectIds writes are dead in 2 of 3 branches.

rawBlockedProjectIds is written on every setBlockedProjects call but read only in the grace-timer callback. In the cleared branch the write is immediately followed by cancelling the only timer that reads it, and in the already-visible branch there's no pending grace to read it — so those two writes are dead stores (and the empty-input case allocates a throwaway Set). Scoping the assignment to the arming branch would make it clear raw is only consumed by the grace.

(AI-assisted, with my guidance)

// Announce the disarm only if this call actually cleared an armed flag — a Clear() when
// nothing was armed is a no-op and must not fire a spurious "changed" signal (consistent
// with the stale-token Clear(long) no-op below).
if ((previous & ArmedFlag) != 0)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not-blocking snapshot is built at three sites.

new SendReceiveBlockState(false, []) is constructed here, in Clear(long), and in GetBlockState's empty branch. A single private static readonly SendReceiveBlockState NotBlocking = new(false, []) (fine for a readonly record struct) would centralize it and keep the empty shape consistent across the disarm and pull paths.

(AI-assisted, with my guidance)

// and continue; emitting unregistered still works, and startup must never break over this).
try
{
bool accepted = await PapiClient.SendRequestAsync<bool>(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider a PapiClient.RegisterEventAsync wrapper instead of a raw SendRequestAsync.

This hand-rolls the network:registerEvent + accepted-bool protocol. PapiClient already centralizes the sibling network:registerMethod in RegisterRequestHandlerAsync (with its timeout-log path and optional documentation arg) but has no symmetric event wrapper. Adding RegisterEventAsync(eventType) on PapiClient and calling it here keeps the event-registration wire contract beside the method one (TS already encapsulates it in createNetworkEventEmitterAsync).

(AI-assisted, with my guidance)

) {
const { isBlocking, projectIds } = payload;
if (typeof isBlocking === 'boolean' && Array.isArray(projectIds)) {
const stringIds = projectIds.filter((id): id is string => typeof id === 'string');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reuse isString.

platform-bible-utils exports an isString type guard, so projectIds.filter(isString) reuses the canonical guard instead of the inline (id): id is string => typeof id === 'string'. (The rest of readBlockedProjectIds's hand-rolled shape check is fine — there's no generic PAPI-payload validator to reuse.)

(AI-assisted, with my guidance)

* No consumer in this diff — the project-settings UI (a stacked follow-up PR) is about to consume
* it to disable per-project actions while that project's automatic sync is blocking edits.
*/
export function isProjectBlocked(projectId: string | undefined): boolean {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isProjectBlocked has no consumer in this PR.

Exported and unit-tested but unused in production here (the driver uses getBlockedProjectIds + its own isEditorBlocked). Documented as ahead of the #2575 project-settings UI, so fine — just flagging it's speculative surface until that lands. (The case-normalization point applies to its Set.has too.)

(AI-assisted, with my guidance)

@lyonsil

lyonsil commented Jul 17, 2026

Copy link
Copy Markdown
Member

Re-review note on a cleanup that can't be anchored inline (its file isn't in this PR's diff):

src/renderer/services/workspace-updating-store.ts:29-30 still reads "(Same pattern as auto-sync-blocking-store; extracting a shared abstraction is deliberately deferred — PT-4214...)". This PR rewrote auto-sync-blocking-store away from that ref-count/leash model, so the reciprocal comment now points at a vanished commonality. Worth updating or removing it in the same change — the blocking store already dropped its half of the note.

(AI-assisted, with my guidance)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants