Skip to content

fix(config-rom): reject suspended generation exports - #87

Draft
gly11 wants to merge 10 commits into
mrmidi:devfrom
gly11:sbp2/config-rom-generation-safety
Draft

fix(config-rom): reject suspended generation exports#87
gly11 wants to merge 10 commits into
mrmidi:devfrom
gly11:sbp2/config-rom-generation-safety

Conversation

@gly11

@gly11 gly11 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Summary

  • make ConfigROM selector 14 reject suspended cache entries
  • suspend cached ROM visibility on the bus-reset edge while preserving internal lifecycle access
  • add regression coverage for exact-generation exports and controller reset dispatch

Problem

ExportConfigROM documents exact-generation lookup with no stale fallback, but it called the ConfigROMStore overload that returns suspended entries. A bus reset between a client generation read and selector 14 could therefore return a complete ROM from the previous generation and let the client reuse stale node routing information.

The cache also remained export-visible until later rediscovery work updated it, leaving a reset window in which stale bytes could still be returned.

Fix

Use FindByNode(generation, node, false) in the user-client handler. The existing not-cached path then returns empty data for suspended entries, while internal callers can still opt into suspended cache access.

On the controller's bus-reset edge, suspend the ROM cache alongside the existing route and lease invalidation, before protocol producers are notified. The migration preserves dev's current reset ordering.

Verification

  • ConfigROMStoreConcurrencyTests: 8/8 passed
  • ControllerCoreBusResetTests: 1/1 passed
  • full C++ host suite: 1,542/1,542 passed, 6 conditional skips
  • pinned XcodeGen regeneration produced no tracked project drift
  • git diff --check
  • GitHub Build and Test: passed on the stacked dev head
  • no dext install or hardware mutation was required

Dependency and merge order

Depends on #88. This PR targets dev and is stacked directly on the current #88 head.

This PR is a sibling of #84, not a child of it: #84 and #87 have no code dependency and may be reviewed or merged in either order after #88.

Until #88 merges, GitHub's Files changed view will also include #88's CI/project-generation changes. The #87-specific review scope is the top three Config ROM commits and these five files:

  • ASFWDriver/Controller/ControllerCoreInterrupts.cpp
  • ASFWDriver/UserClient/Handlers/ConfigROMHandler.cpp
  • tests/core/CMakeLists.txt
  • tests/core/ControllerCoreBusResetTests.cpp
  • tests/discovery/ConfigROMStoreConcurrencyTests.cpp

Intended merge order:

  1. merge fix(build): pin XcodeGen for CI and local builds, stabilize test failure reporting #88 into dev;
  2. update fix(config-rom): reject suspended generation exports #87 onto the resulting dev tip if GitHub does not collapse the stacked diff automatically, retaining only the three Config ROM commits;
  3. merge fix(async): complete block writes from OUTPUT_LAST status #84 and fix(config-rom): reject suspended generation exports #87 in either order after their checks pass.

@gly11
gly11 force-pushed the sbp2/config-rom-generation-safety branch 2 times, most recently from 808c5a4 to a0cfbcf Compare July 26, 2026 10:43
@gly11
gly11 marked this pull request as ready for review July 26, 2026 13:57

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4b59542c8f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ASFWDriver/UserClient/Handlers/ConfigROMHandler.cpp
gly11 added 9 commits July 28, 2026 18:17
The pin added earlier in this branch only bound CI. build.sh still ran
whatever xcodegen was on PATH, and it regenerates ASFW.xcodeproj on every
non-test-only build. Homebrew currently ships 2.46.0 while the pin was
2.45.4, and their output is not byte-identical -- 2.46.0 reorders the
pbxproj `targets` array. So a single ./build.sh on a brew-current machine
produces a project the drift check rejects, and that check's error message
tells you to regenerate, which reproduces the same diff.

Move the version and its SHA-256 into .xcodegen-version and have both the
workflow and build.sh read it, so the two can no longer disagree.

build.sh now stops with an actionable error when the installed version
differs, rather than warning and skipping regeneration: a build silently
missing a newly added source file is much harder to diagnose than a version
mismatch. --no-xcodegen builds the committed project as-is for machines that
can't install the pin.

Bump the pin to 2.46.0 to match what Homebrew ships today, and regenerate
ASFW.xcodeproj with it. The only change is the `targets` array ordering,
which has no semantic effect.

Document how to install the pinned release in README -- the same download,
pin, and checksum CI uses -- since "install the pinned version" isn't
actionable without it.
@gly11
gly11 force-pushed the sbp2/config-rom-generation-safety branch from 5814bea to 1847198 Compare July 28, 2026 12:14
@gly11
gly11 changed the base branch from main to dev July 28, 2026 12:15
SuspendAll() gets its first production caller in this PR, which makes the rest
of the ROM lifecycle worth checking. ValidateROM() is the only transition out
of Suspended and has no callers anywhere — not in the driver, not in tests —
so ROMState::Validated was unreachable and the `state == Validated` arms of
SuspendAll() were dead.

Rediscovery already does the work ValidateROM() was written for. Insert()
overwrites romsByGuid_[guid] wholesale, state included, and romsByGenNode_ is
keyed by (generation, nodeId), so after a reset the new generation gets fresh
keys and the suspended entries are simply never looked up again. Wiring
ValidateROM() up instead would put a second writer on the same entries and
require deciding which one wins when Insert()'s richer-ROM guard declines an
update — a second source of truth for no behavior gained.

Nothing observes the difference today: FindByGuid() does not filter on state,
and FindByNode(gen, nodeId, allowSuspended=false) looks up the current
generation, whose key never matches a suspended entry.

Remove the function, the enum value, and the dead arms, and say in both the
enum comment and ConfigROM/README.md how a device actually comes back. If the
store later needs to report which devices did not return, that closes the
Suspended → Invalid → PruneInvalid loop and deserves its own change with
hardware validation.
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