Skip to content

fix: close remaining findings from the July 2026 deep architecture review - #90

Merged
ovasylenko merged 2 commits into
mainfrom
fix/deep-review-remaining-2026-07
Jul 6, 2026
Merged

fix: close remaining findings from the July 2026 deep architecture review#90
ovasylenko merged 2 commits into
mainfrom
fix/deep-review-remaining-2026-07

Conversation

@ovasylenko

Copy link
Copy Markdown
Contributor

Summary

Fixes the remaining findings from the July 2026 deep architecture review that hadn't yet been addressed on main (H-3, H-4, H-5, H-7, H-8, M-10 were already fixed earlier in this branch's history):

  • C-1: per-instance heartbeat so recover_stale_instances can't reclaim a step that's still genuinely executing out from under itself.
  • H-9: agent checkpoint KV is deleted on completion so a Loop re-dispatch of the same block id doesn't resume a finished pass's exhausted state.
  • H-15: mobile sync skips the download + signature fetch entirely when the local sequence is already at the manifest's version.
  • H-16: mobile sync commands are marked executed before running their side effects, so a redelivered command (lost ack) can't double-fire.
  • H-17: mobile telemetry auto-flush has a cooldown so a failing endpoint isn't hammered on every recorded event once the buffer crosses 80%.
  • M-2: FieldEncryptor::decrypt_value_strict for always-encrypted fields.
  • M-3: refresh_url is re-validated against the SSRF guard on credential update, not just create.
  • M-6: update_context signal merges into context.data instead of blindly overwriting the whole ExecutionContext (which also wiped runtime/audit).
  • M-7: webhook delivery tasks are tracked via a TaskTracker so shutdown waits for them instead of the runtime killing them mid-request.
  • M-9: StreamBus subscribe/publish/drop use DashMap::remove_if to close a TOCTOU that could orphan a fresh subscriber's channel.
  • M-20: deleted orch8-publisher's unused, broken PushNotifier duplicate (its APNs path was a stub that always errored); orch8-push is the real path.
  • M-21: APNs/FCM push delivery now retries transient failures and invalidates the cached JWT/OAuth token on a 403/401 auth rejection.
  • M-23: mobile HTTP retry backoff uses real random jitter and honors a server's Retry-After header; 429 is now retried like 5xx.
  • M-25: scaffolded orch8.toml used field names that don't exist on SchedulerConfig (silently no-op'd); dev server now binds loopback only instead of 0.0.0.0.
  • L-6: --insecure split into --insecure-auth / --insecure-storage so an operator can accept one risk without being forced into the other.
  • L-9: FieldEncryptor zeroizes decoded key bytes instead of leaving them for a plain Drop.

Deliberately out of scope for this pass (disclosed, not silently dropped): gRPC TLS, full rate limiting, SigV4, mobile atomic upsert, Parallel/Race concurrency hardening, most Low-severity items, and all Structural investments (S-1–S-7).

Test plan

  • cargo fmt --check clean workspace-wide
  • cargo clippy --workspace --all-targets -- -D warnings clean
  • cargo test --workspace (unit + integration, SQLite and Postgres) — all green
  • Each fix has a new regression test; every one was confirmed to fail against the pre-fix code, then the fix was restored and re-verified green (including two genuinely racy concurrency tests — stream_bus's remove_if fix and webhooks's task-tracking fix — which reproduced their bugs under real thread parallelism before the fix landed)
  • Two pre-existing tests that had codified the old buggy behavior as intentional (signal_update_context_replaces_instance_context, signal_update_context_with_config_change) were updated to assert the corrected, review-mandated behavior

…view

Fixes C-1, H-9, H-15, H-16, H-17, M-2, M-3, M-6, M-7, M-9, M-20, M-21,
M-23, M-25, L-6, and L-9 from the deep review (H-3/H-4/H-5/H-7/H-8/M-10
were already fixed in this branch before this commit):

- C-1: per-instance heartbeat so recover_stale_instances can't reclaim a
  step that's still genuinely executing out from under itself.
- H-9: agent checkpoint KV is deleted on completion so a Loop re-dispatch
  of the same block id doesn't resume a finished pass's exhausted state.
- H-15: mobile sync skips download+signature fetch entirely when the
  local sequence is already at the manifest's version.
- H-16: mobile sync commands are marked executed before running their
  side effects, so a redelivered command (lost ack) can't double-fire.
- H-17: mobile telemetry auto-flush has a cooldown so a failing endpoint
  isn't hammered on every recorded event once the buffer crosses 80%.
- M-2: FieldEncryptor::decrypt_value_strict for always-encrypted fields.
- M-3: refresh_url is re-validated against the SSRF guard on credential
  update, not just create.
- M-6: update_context signal merges into context.data instead of blindly
  overwriting the whole ExecutionContext (which also wiped runtime/audit).
- M-7: webhook delivery tasks are tracked via a TaskTracker so shutdown
  waits for them instead of the runtime killing them mid-request.
- M-9: StreamBus subscribe/publish/drop use DashMap::remove_if to close
  a TOCTOU that could orphan a fresh subscriber's channel.
- M-20: deleted orch8-publisher's unused, broken PushNotifier duplicate
  (APNs path was a stub that always errored); orch8-push is the real path.
- M-21: APNs/FCM push delivery now retries transient failures and
  invalidates the cached JWT/OAuth token on a 403/401 auth rejection.
- M-23: mobile HTTP retry backoff uses real random jitter and honors a
  server's Retry-After header; 429 is now retried like 5xx.
- M-25: scaffolded orch8.toml used field names that don't exist on
  SchedulerConfig (silently no-op'd); dev server now binds loopback only.
- L-6: --insecure split into --insecure-auth / --insecure-storage so an
  operator can accept one risk without being forced into the other.
- L-9: FieldEncryptor zeroizes decoded key bytes instead of leaving them
  for a plain Drop.

Verified with cargo fmt --check, clippy -D warnings, and the full test
suite (unit + integration, SQLite and Postgres) all green across the
workspace. Each fix's regression test was confirmed to fail against the
pre-fix code before being restored, per the project's verification
convention.
RUSTDOCFLAGS="-D warnings" cargo doc --document-private-items resolves
the link locally, but rustdoc still flags a public item's docs linking
to a private one under -D rustdoc::private-intra-doc-links (it would
break for a doc build without --document-private-items). Reword as
plain text instead of a link; wait_for_webhook_tasks (public) keeps its
link.
@ovasylenko
ovasylenko merged commit 9305586 into main Jul 6, 2026
14 checks passed
ovasylenko added a commit that referenced this pull request Jul 6, 2026
Durable record of the five-agent review's findings and how they were
closed across PR #88 (15 storage findings) and PR #90 (all remaining
C/H/M/selected-L findings), plus what was deliberately deferred. The
raw findings write-up was a working document and isn't checked in;
this replaces it.
@ovasylenko
ovasylenko deleted the fix/deep-review-remaining-2026-07 branch July 16, 2026 00:10
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.

1 participant