Skip to content

Fix 19869: don't export action settings, don't let one broken entry block startup, and don't leave a stale tail when rewriting the export file - #19914

Open
oliveiraallex wants to merge 3 commits into
pharo-project:Pharo14from
oliveiraallex:19869-Startup-preferences-create-broken-StartupAction-calls-on-Export
Open

Fix 19869: don't export action settings, don't let one broken entry block startup, and don't leave a stale tail when rewriting the export file#19914
oliveiraallex wants to merge 3 commits into
pharo-project:Pharo14from
oliveiraallex:19869-Startup-preferences-create-broken-StartupAction-calls-on-Export

Conversation

@oliveiraallex

@oliveiraallex oliveiraallex commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

This PR fixes: #19869 which seems to have started in build 731

Hello guys, I’m starting to familiarize myself with “pair programming with AI,” and I worked on this issue to test out some of my own ideas on how to use it, because Pharo is perfect to it.
All 3 commits I'm proposing are very detailed.
Feel free to let me know if this fix was helpful, if it’s too detailed (issue, commits and pr), if it makes sense or if this is a valid approach or not, thanks
At least the bug has been fixed :)

Summary

Three small, independent commits, one per defect described in #19869:

  1. Write side — Export no longer generates a StartupAction for action/button settings, which have no value to export.
  2. Replay side — a StartupAction that fails is recorded and skipped instead of interrupting the rest of the startup sequence.
  3. File side — the export file is moved aside before being rewritten, so a shorter export cannot leave the tail of a longer previous one behind.

Commit 1 fixes the cause. Commit 2 keeps an already-poisoned image bootable. Commit 3 fixes a distinct corruption that commit 2 cannot catch, because it happens at parse time before any StartupAction exists.

Alternatives considered

  • Redesign StartupAction into a structured, versioned format (class + selector + arguments, instead of generating and compiler evaluate:-ing a perform:with: string), as suggested in the issue. Directionally right — it would also allow detecting a renamed or removed selector before replaying. Out of scope: a much larger, higher-risk change for a bug with a small, uncontroversial cause. Worth its own issue.
  • Fix only the write side and stop there. Rejected: it does nothing for anyone who already exported on 731–735, and nothing at all for defect 3.
  • Use isTransient: true on ActionSettingDeclaration, mirroring what build 731 did for SpIconPacksFetchPresenter. Rejected in favour of hasValue: isTransient is a per-instance opt-out that must be set on every button; hasValue is a per-class contract that covers the whole category, present and future, which is exactly the gap that caused this bug.
  • Keep ex pass and wrap file fileIn in load: instead. That would contain a parse failure to one file, but would not stop a single broken entry from killing every other setting in the same file. Commits 2 and 3 address the two failure modes at their actual causes instead.
  • Delete the old file instead of renaming it in commit 3. Simpler, and leaves no .old.txt siblings — but a write failing midway would then lose the previous export with nothing to fall back on, and anyone hitting this bug would lose the corrupted file that shows what went wrong. Renaming costs one extra file per exported group and matches what SystemSettingsPersistence already does in the same feature area.
  • Fix FileReference>>writeStream to truncate. This is the real, general defect — writeStreamDo: never truncates anywhere in the image, and truncateTo: has no senders. But it has 100+ call sites across 40+ packages and changing it is far beyond fixing Startup preferences create broken StartupAction calls on Export #19869.

Related

@oliveiraallex
Commit 1 — ActionSettingDeclaration must report that it has no export…
4808ae3 
…able value

Problem. exportSettingAction exports any setting whose target is a class and which has no registered default. An action setting never has a default (hasDefault is default ~~ UniqueObject, and nothing calls default: from a button pragma), so the guard is always satisfied and every <systemsettings> button is exported unconditionally. hasValue already exists as the intended guard — PragmaSetting, the grandparent, defines both hasValue -> false and hasDefault -> false — but ActionSettingDeclaration never restored that contract after SettingDeclaration overrode it for genuine value-holding subclasses, and exportSettingAction never checked it.

Fix.

ActionSettingDeclaration >> hasValue [
	"An action setting (a button) holds no persistable value — it triggers a
	 side-effecting action when invoked. It must never be included in an
	 exported startup script, and its 'current value' must never be computed
	 by actually invoking the action (see pharo-project#19869)."

	^ false
]
SettingDeclaration >> exportSettingAction [
	"Return the startup actions that set this setting when the value differs from the default.

	Returns: a StartupAction or nil if no changes

	It is up to the caller to filter nil results"

	target isClass ifFalse: [ ^ nil ].
	self isTransient ifTrue: [ ^ nil ].
	self hasValue ifFalse: [ ^ nil ].

	^ (self hasDefault not or: [ self default ~= self findCurrentSettingValue ])
		  ifTrue: [ self startupAction ]
		  ifFalse: [ "We do nothing" nil ]
]
Only self hasValue ifFalse: [ ^ nil ]. is new. This deliberately generalises past ActionSettingDeclaration: any current or future SettingDeclaration subclass that legitimately has no value is protected for free, exactly as PragmaSetting already was — instead of having to remember isTransient: true on each one individually, which is what build 731's fix required for SpIconPacksFetchPresenter.

Declared side effect on the other pipeline. hasValue is also consulted by StoredSettingsFactory>>fromSettingNodes:, which backs "Store Settings". Today every button produces a realValue: false entry there (harmless, but noise, since ActionSettingDeclaration>>realValue: is a no-op). After this commit those entries are no longer produced. This is intended and is an improvement, but it is a second pipeline being touched and should not be discovered by surprise in review.

Test — added to System-Settings-Tests.

MockSettings gains a button setting mirroring a real one:

MockSettings class >> mockActionNodeOn: aBuilder [
	<mocksystemsettings>
	(aBuilder button: #mockAction)
		label: 'Mock action setting'
]
MockSettings class >> mockAction [
	"Mimics a real action setting: a unary action method with no matching
	 keyword setter — this is the shape that pharo-project#19869 exported incorrectly."

	^ self
]
SystemSettingsPersistenceTest >> testActionSettingIsNotExported [
	"An action/button setting has no persistable value, so #exportSettingAction
	 answers nil for it. Exporting one invokes the action as a side effect through
	 (see pharo-project#19869)."

	| actionNode |
	actionNode := systemSettings nodeNamed: #mockAction.

	"Guard: make sure a nil result really comes from #hasValue and not from an
	 earlier guard in #exportSettingAction, which would make this test vacuous."
	self assert: actionNode item target isClass.
	self deny: actionNode item isTransient.

	self deny: actionNode item hasValue.
	self assert: actionNode item exportSettingAction isNil
]
SystemSettingsPersistenceTest >> testValueHoldingSettingIsStillExported [
	"A setting that does hold a value still exports. This is the other side of the
	 exports unconditionally."

	| node |
	node := systemSettings nodeNamed: #booleanSetting.
	self assert: node item hasValue.
	self assert: node item exportSettingAction isNotNil
]
Worth noting for reviewers: searching the whole Pharo 14 source for exportSettingAction, exportSettings or startupAction inside any *Tests* package returns no matches. The Export pipeline currently has no test coverage at all, which is how pharo-project#19847 shipped this bug through CI. These two tests are its first.

The two guard assertions matter: exportSettingAction has three early exits, and without them a test asserting only isNil would pass even if the fix were reverted. target is set for a root node by SettingTree from the pragma's methodClass instanceSide, so it is MockSettings here, and isClass holds.

Manual verification. On a build-735 image with this commit: opening the Settings Browser and clicking Export, with Iceberg credentials and several banned rules configured, no longer includes IceTipCredentialsSettings, ReRuleManager or StPulse entries. Genuinely value-holding settings (theme colours, fonts, formatter options) are still exported and still replay.

This does not repair files already written by a previous, buggy Export. That is what commit 2 is for.
@oliveiraallex
Commit 2 — a broken StartupAction must not interrupt the rest of startup
f6dd0c6 
Problem. execute catches Halt, Error, records the failure, then does ex pass. In a deployed or headless image an unhandled DoesNotUnderstand here means a crash dialog or a hang on every single boot. This is independent of commit 1: anyone who exported on 731–735 already has broken entries on disk, and commit 1 only stops new ones from being written.

Fix.

StartupAction >> execute [

	| block correctlyExecuted |
	(self hasBeenExecuted and: [ self runOnce ]) ifTrue: [ ^ self ].
	block := self code isBlock
				ifTrue: [ self code ]
				ifFalse: [[ self class compiler evaluate: self code ]].
	correctlyExecuted := true.
	block on: Halt, Error do: [ :ex |
		"A broken StartupAction -- e.g. one referencing a selector that does not
		 exist, see pharo-project#19869 -- must never interrupt the rest of the startup
		 sequence nor open a blocking Debugger on an end user's machine. Record
		 it for diagnosis, mark it as not executed so a later corrected export
		 can still apply, and move on."
		StartupPreferencesLoader default errors add: {ex. self}.
		correctlyExecuted := false.
		ex return ].
	self hasBeenExecuted: correctlyExecuted
]
Two changes, and the second is the one worth arguing about.

ex pass → ex return stops the failure propagating. The error is still recorded in StartupPreferencesLoader default errors exactly as before, so it stays fully observable.

correctlyExecuted := false gives the existing temporary its evident intended meaning. Today it is dead code: it is assigned true and never anything else, because with ex pass the line that reads it was unreachable on failure. Making it meaningful is what keeps this fix from creating a new problem:

StartupPreferencesLoader>>add: keys actions by name in a dictionary that lives in the singleton and is therefore saved inside the image, and it refreshes an existing action's code without ever resetting hasBeenExecuted (verified on a stock image — see Part 4 A). Since every exported action is runOnce: true, marking a failed action as executed would permanently retire it in that image — surviving image saves, and surviving the user fixing the underlying problem and re-exporting. With correctlyExecuted := false, a broken action is retried on the next startup, never blocks, and starts working by itself once a corrected export lands.

Note that this is the one place where the present commit genuinely changes whether an action gets marked: with ex pass the assignment was unreachable on failure, so failures were retried by accident. correctlyExecuted := false preserves that retry behaviour deliberately, rather than losing it as a side effect of no longer re-raising.

The cost is one entry in errors per boot while the action stays broken, and that collection is cleared at the start of every load:, so nothing grows.

Note on Halt. The pre-existing handler catches Halt, Error. With ex return, a self halt deliberately placed inside a startup action for debugging is now recorded and swallowed instead of opening the debugger. If that is considered a regression, ex class == Halt ifTrue: [ ^ ex pass ] before the return restores the old behaviour for that one case. Flagging it explicitly rather than changing it silently.

Tests — new class in StartupPreferences-Tests.

TestCase << #StartupActionTest
	slots: { #loader . #savedActions . #savedErrors };
	package: 'StartupPreferences-Tests'
StartupActionTest >> setUp [
	"#execute writes into the live singleton, whose actions dictionary is part of
	 the running image and holds the user's real startup history. Save and restore
	 it rather than clearing it outright."

	super setUp.
	loader := StartupPreferencesLoader default.
	savedActions := loader actions copy.
	savedErrors := loader errors copy.
	loader cleanSavedActionsAndErrors
]
StartupActionTest >> tearDown [
	loader cleanSavedActionsAndErrors.
	"#actions is a Dictionary, so #addAll: needs a keyed collection — passing an
	 Array of associations would make Dictionary>>addAll: iterate index->element."
	loader actions addAll: savedActions.
	loader errors addAll: savedErrors.
	super tearDown
]
StartupActionTest >> brokenAction [
	^ StartupAction
		name: 'Broken action'
		code: '(Smalltalk globals at: #Object) perform: #thisSelectorDoesNotExist'
		runOnce: true
]
StartupActionTest >> testBrokenActionDoesNotRaise [
	"#execute does not propagate a failure raised by the action's own code. One bad
	 entry in a shared preferences file would otherwise abort the whole startup
	 sequence (see pharo-project#19869)."

	self brokenAction execute
]
StartupActionTest >> testBrokenActionIsRecordedInErrors [
	"The failure is recorded in StartupPreferencesLoader default errors, paired with
	 the action that raised it. Swallowing the exception is only acceptable because
	 it stays observable here."

	| action |
	action := self brokenAction.
	action execute.
	self assert: loader errors size equals: 1.
	self assert: loader errors first second identicalTo: action
]
StartupActionTest >> testBrokenActionIsNotMarkedAsExecuted [
	"A failed action stays unexecuted. #add: keys actions by name in a dictionary that
	 is saved with the image and never resets this flag, so an action marked executed
	 is never attempted again -- across image saves, and after the exported file has
	 been corrected."

	| action |
	action := self brokenAction.
	action execute.
	self deny: action hasBeenExecuted
]
StartupActionTest >> testSuccessfulActionIsMarkedAsExecuted [
	"An action whose code completes is marked executed and records no error. With

	| action executed |
	executed := false.
	action := StartupAction name: 'Working action' code: [ executed := true ] runOnce: true.
	action execute.
	self assert: executed.
	self assert: action hasBeenExecuted.
	self assert: loader errors isEmpty
]
@oliveiraallex
Commit 3 — the export file must be moved aside before being rewritten
ea5ffc3 
Problem. addAtStartup:inDirectory:named: is the single write point for every addAtStartupIn* variant, and it writes through writeStreamDo:, which does not truncate. Rewriting with shorter content leaves the tail of the previous file on disk. The result is not a malformed entry — it is a malformed file: the failure happens in CodeImporter at parse time, so the whole file is lost, every setting in it included. Commit 2 cannot catch this, because no StartupAction object exists yet.

Fix.

The backup is kept in its own method on purpose. It is the one debatable part of this commit — if reviewers prefer a plain delete, or no backup at all, the change is a single line in the caller (self backUpAndRemove: file. → file ensureDelete. or nothing), with no surrounding code to untangle.

StartupPreferencesLoader >> addAtStartup: aCollection inDirectory: aFileReference named: fileName [

	| file |
	aFileReference ensureCreateDirectory.
	file := aFileReference / fileName.
	self backUpAndRemove: file.
	file writeStreamDo: [ :stream | stream nextPutAll: (self buildStreamFor: aCollection) ]
]
StartupPreferencesLoader >> backUpAndRemove: aFileReference [
	"Move an existing startup script aside before it gets rewritten, keeping the
	 previous version as a backup.

	 existing file leaves the previous tail on disk. That tail then fails to parse
	 at startup and takes every setting in the file down with it (see pharo-project#19869).

	 The backup deliberately drops the .st extension. StartupPreferencesHandler
	 collects startup scripts with #filesMatching: '*.st', so a backup still named
	 .st would be loaded on the next startup alongside the current file, and would
	 replay exactly the entries it exists to preserve a copy of.

	 Mirrors SystemSettingsPersistence>>removeFileReference, which solves the same
	 problem for the 'Store Settings' pipeline and picks 'old.txt' for the same
	 reason."

	| backup |
	aFileReference ifAbsent: [ ^ self ].
	backup := aFileReference withExtension: 'old.txt'.
	"#renameTo: fails if the destination already exists."
	backup ensureDelete.
	"#renameTo: mutates its receiver's path, so rename a copy and leave
	 aFileReference pointing at the now-free original path."
	aFileReference copy renameTo: backup basename
]
For system-settings.ston1.st this produces system-settings.ston1.old.txt: withExtension: goes through Path>>withName:extension: on basenameWithoutExtension, which strips only the final extension.

Precedent. The sibling pipeline in the same feature area already does exactly this, and says why:

SystemSettingsPersistence >> removeFileReference [
	"We have to remove the file before writing new settings."

	| newFileReference |
	fileReference ifAbsent: [ ^ self ].
	newFileReference := fileReference withExtension: 'old.txt'.
	newFileReference ensureDelete.
	fileReference copy renameTo: newFileReference basename
]
Keeping a backup rather than deleting outright means a write that fails midway does not lose the previous export, and it gives anyone hitting this bug a copy of the corrupted file to inspect. The cost is one .old.txt sibling per exported group, which is the same cost the "Store Settings" pipeline already accepts.

Tests — new class in StartupPreferences-Tests.

TestCase << #StartupPreferencesLoaderTest
	slots: { #dir . #loader };
	package: 'StartupPreferences-Tests'
StartupPreferencesLoaderTest >> setUp [
	"Only the file-writing path is exercised here, which touches neither the
	 actions dictionary nor the errors collection, so the live singleton needs no
	 saving and restoring."

	super setUp.
	dir := FileSystem memory root / 'prefs'.
	loader := StartupPreferencesLoader default
]
StartupPreferencesLoaderTest >> testRewritingWithShorterContentLeavesNoStaleTail [
	"Rewriting an existing script with shorter content leaves exactly the new
	 content. #writeStreamDo: does not truncate, so the tail of the longer previous
	 file would otherwise remain and stop the whole file from parsing at startup,
	 taking every setting in it down (see pharo-project#19869). Exercised through the private
	 write method because it is the single write point for every #addAtStartupIn*
	 variant, and the public ones all target real folders on disk."

	| long short |
	long := (1 to: 10) collect: [ :i |
		StartupAction name: 'Action ', i printString code: 'Object new' runOnce: true ].
	short := { StartupAction name: 'Action 1' code: 'Object new' runOnce: true }.

	loader addAtStartup: long inDirectory: dir named: 'test.st'.
	loader addAtStartup: short inDirectory: dir named: 'test.st'.

	self
		assert: (dir / 'test.st') contents
		equals: (loader buildStreamFor: short).

	"The previous version is archived outside the '*.st' glob that
	 StartupPreferencesHandler collects, so the folder still holds one startup script."
	self assert: (dir filesMatching: '*.st') size equals: 1
]
FileSystem memory reproduces the defect faithfully: MemoryFileWriteStream>>stream opens a ReadWriteStream on: file bytes from: 1 to: file size, so a short write leaves the earlier bytes past the write position, and flush stores all of them. testRewritingWithShorterContentLeavesNoStaleTail therefore fails before this commit and passes after it.

Manual verification. Export, save, quit, reopen — repeatedly, with the number of exported settings both growing and shrinking. Every generated file now ends at }. + two line breaks with nothing after it, and the previous version sits beside it as .old.txt:

| dir |
dir := StartupPreferencesLoader default preferencesVersionFolder.
dir children
	select: [ :e | e basename endsWith: '.st' ]
	thenCollect: [ :e | e basename -> (e contents endsWith: '}.', String crlf, String crlf) ]
" {'system-settings.ston1.st'->true. 'system-settings.ston2.st'->true}"
@guillep

guillep commented Aug 5, 2026

Copy link
Copy Markdown
Member

Hi @oliveiraallex ! Long time no see! Hope you're well!

I did a fix for the settings issue a couple of weeks ago, that got just integrated, in new tools:

pharo-spec/NewTools#1495

I'll check this one later ;)

@oliveiraallex

oliveiraallex commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Hi Guille! Yes, long time, I'm fine, thanks, I hope you too!

Ok, I will read your PR on Spec, thanks

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@oliveiraallex @guillep