Improve app shard service reliability

[dazthecorgi]

One logical node is a master node and and some worker nodes. Worker nodes can be in process (thread workers) or a standalone worker process that the master communicates with via gRPC. Workers always have separate databases from the master.

The master node runs an app shard service that provides access to app shard frames via gRPC. App shard frames are normally stored in the worker databases.

The app shard frames are currently duplicated in the master's database so that the master node can serve them via gRPC. If there is a failure the duplication is not retried, which means there can be gaps in the coverage of the app shard service. And if a node no longer serves a shard, the app frames in the master's store become orphaned.

I think it'd be better to avoid the duplication of data for reliability and consistency. In thread worker mode, the master could directly read the stores of the workers to serve app shard frame requests. In standalone worker mode, requests could be proxied.

[blacks1ne]

I reviewed this against the latest v2.1.0.25 commit (5d1ddca1). I agree with the architectural direction. AppShardService should read from the worker that owns the data rather than maintain a best-effort second copy, but I think the current defect is more severe than the issue describes.

In thread-worker mode, the mirror in crates/quil-node/src/master_node/worker_manager.rs calls only stage_shard_clock_frame. That writes the selector-based staged key. AppShardRpcServer, however, calls get_latest_shard_clock_frame or get_shard_clock_frame, which read the latest-index/canonical filter keys. Only commit_shard_clock_frame copies the staged frame to the canonical key and advances the latest index, and the master mirror never calls it. So this path appears to accumulate duplicate frame bytes without actually making them visible to the RPC, not just create occasional gaps.

There are also several silent failure paths: decode failure, missing header, hash failure, transaction creation failure, and transaction commit failure are all ignored. Separately, AppShardRpcServer converts every store error (including I/O/corruption) into a successful response with frame: None, making operational failures indistinguishable from not-found.

I would replace the direct master ClockStore dependency with an async frame-provider abstraction:

• Thread mode: register filter -> Arc<dyn ClockStore> when ShardActivated arrives and remove it on ShardDeactivated. Routing from activation events is safer than using assignment metadata because set_worker_filter updates metadata before the worker necessarily processes Respawn, and its try_send result is ignored.
• Standalone mode: register AppShardService on the worker's existing gRPC server, or add an internal frame-read RPC, and proxy through the existing master-to-worker channel.

The contract should explicitly say whether only currently assigned shards are served or whether historical locally retained shards remain queryable. If it is active-only, the worker should verify that the requested filter is currently active; otherwise a reassigned worker can still serve old data from its persistent DB. Proxy requests also need deadlines, message-size limits, and meaningful NotFound/Unavailable/Internal status propagation.

Removing the copy improves consistency but makes availability honestly depend on the owning worker. If failover is required, it should select another active shard replica/archive rather than fall back to a stale master-side mirror.

[dazthecorgi]

Thanks, I agree broadly.

In thread-worker mode, the mirror in crates/quil-node/src/master_node/worker_manager.rs calls only stage_shard_clock_frame. That writes the selector-based staged key. AppShardRpcServer, however, calls get_latest_shard_clock_frame or get_shard_clock_frame, which read the latest-index/canonical filter keys. Only commit_shard_clock_frame copies the staged frame to the canonical key and advances the latest index, and the master mirror never calls it. So this path appears to accumulate duplicate frame bytes without actually making them visible to the RPC—not just create occasional gaps.

This has been fixed since in 324ffb3#diff-b75a842ac9503cd3f517b99a4c6c7d7a7917fff72930bc200513fad087341639R553-R565

[blacks1ne]

Confirmed after re-checking the live v2.1.0.25 branch, now at ff5837e6: commit 324ffb3b does fix the specific stage_shard_clock_frame issue I reported.

The updated path now:

1. stages the frame and commits that transaction;
2. opens a second transaction and calls commit_shard_clock_frame(filter, frame_number, selector, ..., false);
3. commits the canonical-frame/latest-index transaction.

That ordering is correct for RocksClockStore: commit_shard_clock_frame reads the staged bytes from the DB, copies them to the canonical filter + frame_number key, and monotonically advances the latest index. A separate second transaction is therefore important: the staged record must be visible before the canonicalization step reads it. The filter, frame number, and selector passed by the caller also match the staged frame.

The subsequent branch-head commit (ff5837e6, CI-only) does not alter this path. So I withdraw the narrow claim that the current branch mirror only stages frames without making them RPC-visible.

This confirms that one correctness issue is fixed; it does not change the broader issue's case for removing duplication. The other failure-handling concerns also remain: for example, both transaction commit() results are still discarded, and no direct regression test appears to cover master-mirror → latest/numbered RPC visibility.

[blacks1ne]

The broader fix is now available for review in #592:

It removes the master-side app-shard frame mirror and serves reads from the acknowledged active worker store instead, with remote-worker proxying and authoritative archive fallback. The regression coverage proves the public RPC succeeds while the master store remains empty, and also covers reassignment/deallocation ordering, stale generations, worker shutdown, error propagation, and responses above 4 MiB.

Colleagues, reviews would be especially helpful around the local/remote worker lifecycle and archive fallback semantics.