fix(hooks): deploy_dir must read an absent preserved key as the daemon default - #50
Merged
Merged
Conversation
…n default
install-network passes '^(timestamp|apn|autoconnect)=' to deploy_dir so a
redeploy won't fight the runtime owner of those keys. The intent was right; the
preservation didn't work, for two compounding reasons.
First, stripping the mutable keys only from the *diff* is not preservation. The
moment any other line differs, deploy_dir installs the source wholesale — and
the source's own values for those keys overwrite the live ones.
Second, and why this went unnoticed for so long: NetworkManager's keyfile writer
omits any property equal to its default, and connection.autoconnect defaults to
true. So `nmcli ... autoconnect yes` writes NO LINE AT ALL, and a "preserve the
line if present" rule has nothing to match. The repo's explicit autoconnect=false
then lands unopposed. The asymmetry is why only Quectel was affected:
Telit autoconnect no -> false, non-default -> written -> survived
Quectel autoconnect yes -> true, IS default -> omitted -> reverted
Fleet impact: every Quectel station silently lost cellular autoconnect after any
OTA or manual post-merge run, staying down until the next boot re-ran
station-boot. A unit reachable only over cellular would have gone dark with no
way back in.
The same mechanism reverted apn= to the repo's `super` — the exact value that
draws 3GPP cause 33 (option-unsubscribed) on a Telenor SIM. So an OTA touching
any unrelated line in station-modem.nmconnection could re-break the cellular
data path the APN provisioner had just fixed.
Fix, per the two complementary options in the investigation:
(a) merge_preserved_keys() builds the payload from the source but takes each
preserved key's value from the DESTINATION, and OMITS the key entirely when
the destination has no such line — reproducing the daemon's default instead
of falling back to the source. Driven off the source's lines, so each key
keeps its original keyfile section. Known limitation, documented at the
function: a preserved key the destination has but the source doesn't ship
can't be placed in the right section, so it isn't carried over.
(b) install-network re-runs modem-datapath.sh after the profile deploy. autoconnect
is a pure function of modem type (VID:PID), so re-deriving it removes the
dependency on preservation altogether. Idempotent, non-fatal, and skipped
under CTT_BUILD_MODE where there is no NetworkManager or modem to probe.
Adds system/scripts/hooks/test/test-deploy-dir.sh — 14 dependency-free checks
over the real deploy_dir (install(1) shimmed so it runs unprivileged): the
Quectel omitted-key case, the Telit non-default case that must not regress,
mutable-keys-only no-op, fresh install, section placement, and idempotency.
Verified to FAIL 2/14 against the pre-fix _lib.sh. Lives outside post-merge.d/
so the orchestrator's *.sh glob never executes it.
Also confirmed against the live keyfile on a v3.3 Quectel station: 0 autoconnect
lines on disk, and the merge emits none while keeping the live apn=internet.cxn
over the repo's super, with autoconnect-retries=0 and route-metric=700 intact.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ir-preserve-defaults
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lands the
deploy_dirpreserve fix called for in the cause-33 investigation (§ follow-ups: "'Preserve the line if present' is unsound for any key whose default-valued form NM omits").Problem
install-network.shpasses'^(timestamp|apn|autoconnect)='todeploy_dirso a redeploy won't fight the runtime owner of those keys. The intent is right; the preservation never worked, for two compounding reasons.1. Stripping the mutable keys only from the diff is not preservation. The moment any other line differs,
deploy_dirinstalls the source wholesale — and the source's values for those keys overwrite the live ones.2. NetworkManager omits properties equal to their default.
connection.autoconnectdefaults to true, sonmcli … autoconnect yeswrites no line at all, and a "preserve the line if present" rule has nothing to match. The repo's explicitautoconnect=falsethen lands unopposed. That asymmetry is why only Quectel was ever hit:no→ false (non-default)autoconnect=falseyes→ true (is the default)falsewonFleet impact: every Quectel station silently lost cellular autoconnect after any OTA or manual
post-mergerun, staying down until the next boot re-ranstation-boot. A unit reachable only over cellular would have gone dark with no way back in.The same mechanism reverted
apn=to the repo'ssuper— the exact value that draws 3GPP cause 33 (option-unsubscribed) on a Telenor SIM. So an OTA touching any unrelated line instation-modem.nmconnectioncould re-break the data path the APN provisioner had just fixed. My test run against the pre-fix code reproduces both reversions.Fix
Both complementary options from the investigation:
(a)
merge_preserved_keys()builds the payload from the source but takes each preserved key's value from the destination, and omits the key entirely when the destination has no such line — reproducing the daemon's default rather than falling back to the source's value. Driven off the source's lines, so each key stays in its original keyfile section.Documented limitation: a preserved key the destination has but the source doesn't ship can't be placed in the right section, so it isn't carried over. Runtime policy re-asserts those — which is exactly what (b) provides.
(b)
install-networkre-runsmodem-datapath.shafter the profile deploy.autoconnectis a pure function of modem type (VID:PID), so re-deriving it drops the dependency on preservation altogether. Idempotent (leaves it unchanged when no known modem is visible), non-fatal (|| log_warn— the profile is already deployed and boot re-runs it), and skipped underCTT_BUILD_MODE, where there's no NetworkManager or modem to probe.Tests
New
system/scripts/hooks/test/test-deploy-dir.sh— 14 dependency-free checks driving the realdeploy_dir(install(1)shimmed so it runs unprivileged):Against the pre-fix
_lib.shit fails 2/14 — preciselyautoconnect line NOT writtenandlive apn preserved. Lives outsidepost-merge.d/so the orchestrator's*.shglob never executes it as a hook.Also validated against a live v3.3 Quectel station, not just fixtures: the on-disk keyfile has 0
autoconnect=lines (the defect's precondition, confirmed in the field), and the merge emits none — while keeping the liveapn=internet.cxnover the repo'ssuper, withautoconnect-retries=0androute-metric=700intact in their correct sections.Notes for review
lts_26_07.iso. This defect is independent of the APN work and affects every Quectel station on the release branch, so it shouldn't wait behindfix/quectel-apn-by-imsi. There's a trivial textual overlap with that branch instation-modem.nmconnection(it addsautoconnect-retries/route-metric); this PR doesn't touch that file.timestamp=sits in the same preserve list andtimestamp=0is likewise NM's default, so it carried the identical latent hole. Harmless in itself, and now handled by the same rule.bash system/scripts/hooks/test/test-deploy-dir.sh). Worth wiring into CI separately — happy to do that if you want it here instead.🤖 Generated with Claude Code