fix: the configuration must not be used as temporary storage #849

Merged

earl-warren merged 1 commit from earl-warren/runner:wip-valid-volumes into main 2025-08-12 10:09:43 +00:00

Conversation 10 Commits 1 Files changed 4 +16 -26

earl-warren commented 2025-08-11 23:24:16 +00:00

Contributor

Copy link

rc.Config should be treated as read-only and not as a temporary storage for the variable list of valid volumes for containers sharing this configuration.

Refs forgejo/runner#848

• bug fixes
  • PR: fix: the configuration must not be used as temporary storage

rc.Config should be treated as read-only and not as a temporary storage for the variable list of valid volumes for containers sharing this configuration. Refs forgejo/runner#848 <!--start release-notes-assistant--> <!--URL:https://code.forgejo.org/forgejo/runner--> - bug fixes - [PR](https://code.forgejo.org/forgejo/runner/pulls/849): <!--number 849 --><!--line 0 --><!--description Zml4OiB0aGUgY29uZmlndXJhdGlvbiBtdXN0IG5vdCBiZSB1c2VkIGFzIHRlbXBvcmFyeSBzdG9yYWdl-->fix: the configuration must not be used as temporary storage<!--description--> <!--end release-notes-assistant-->

earl-warren added the

Kind/Bug

label 2025-08-11 23:24:16 +00:00

earl-warren self-assigned this 2025-08-11 23:24:16 +00:00

earl-warren force-pushed wip-valid-volumes from f9a509e752 to 28d0a596a9 2025-08-11 23:48:45 +00:00 Compare

earl-warren commented 2025-08-11 23:56:17 +00:00

Author

Contributor

Copy link

It is unclear if this is the root cause of #848, but is certainly is a source of problems.

It is unclear if this is the root cause of https://code.forgejo.org/forgejo/runner/issues/848, but is certainly is a source of problems.

earl-warren changed title from WIP: fix: the configuration must not be used as temporary storage [skip cascade] to fix: the configuration must not be used as temporary storage [skip cascade] 2025-08-12 00:02:07 +00:00

earl-warren requested reviews from viceice, mfenniak 2025-08-12 00:02:22 +00:00

earl-warren commented 2025-08-12 00:03:16 +00:00

Author

Contributor

Copy link

this might fix the rather frequent (once every 5 or 10 runs at least) false negative from #848

this might fix the rather frequent (once every 5 or 10 runs at least) false negative from https://code.forgejo.org/forgejo/runner/issues/848

earl-warren force-pushed wip-valid-volumes from 28d0a596a9 to 82cf344f51 2025-08-12 00:03:39 +00:00 Compare

earl-warren changed title from fix: the configuration must not be used as temporary storage [skip cascade] to fix: the configuration must not be used as temporary storage 2025-08-12 00:04:02 +00:00

earl-warren force-pushed wip-valid-volumes from 82cf344f51 to 3ab438ae41 2025-08-12 00:04:09 +00:00 Compare

cascading-pr referenced this pull request from actions/setup-forgejo 2025-08-12 00:04:30 +00:00

cascading-pr from https://code.forgejo.org/forgejo/runner refs/pull/849/head to forgejo/runner-849 #592

cascading-pr commented 2025-08-12 00:04:31 +00:00

Contributor

Copy link

cascading-pr updated at actions/setup-forgejo#592

cascading-pr updated at https://code.forgejo.org/actions/setup-forgejo/pulls/592

Gusted commented 2025-08-12 00:38:53 +00:00

Owner

Copy link

It seems

act/runner/runner.go

Line 74 in 555b322

ValidVolumes []string // only volumes (and bind mounts) in this slice can be mounted on the job container or service containers

can be removed as well?

It seems https://code.forgejo.org/forgejo/runner/src/commit/555b322ce5922942e846c04a62925e3c09db89b7/act/runner/runner.go#L74 can be removed as well?

mfenniak approved these changes 2025-08-12 04:41:15 +00:00

mfenniak left a comment

Owner

Copy link

Gusted's comment about ValidVolumes no longer being necessary on the Config object seems right at a glance; other than that this change makes sense to me.

I was curious, since I've never used it before, and I did a little test for whether the data race detector would identify this condition. It does... and there are a few other data races identified. After this fix, is there value in pursuing those, eliminating them, and maybe enabling data race detection during tests? I'd volunteer to try, as long as nobody would advise against it (eg. based upon their experience, if the data race detector isn't a valuable tool or the resource usage is too high to justify, etc.?).

eg.

go test -v -race ./act/runner/... --run TestRunner_RunEvent/evalmatrix 2>&1 | tee runner-tests-2.log

Gusted's comment about `ValidVolumes` no longer being necessary on the Config object seems right at a glance; other than that this change makes sense to me. I was curious, since I've never used it before, and I did a little test for whether the data race detector would identify this condition. It does... and there are a few other data races identified. After this fix, is there value in pursuing those, eliminating them, and maybe enabling data race detection during tests? I'd volunteer to try, as long as nobody would advise against it (eg. based upon their experience, if the data race detector isn't a valuable tool or the resource usage is too high to justify, etc.?). eg. ``` go test -v -race ./act/runner/... --run TestRunner_RunEvent/evalmatrix 2>&1 | tee runner-tests-2.log ```

runner-tests-2.log

1.2 MiB

viceice approved these changes 2025-08-12 05:07:33 +00:00

earl-warren commented 2025-08-12 09:59:05 +00:00

Author

Contributor

Copy link

@mfenniak wrote in #849 (comment):

After this fix, is there value in pursuing those, eliminating them, and maybe enabling data race detection during tests? I'd volunteer to try, as long as nobody would advise against it (eg. based upon their experience, if the data race detector isn't a valuable tool or the resource usage is too high to justify, etc.?).

I have no experience using this tool and since it caught this one (which was painful to figure out), I'd say there is value in it. Maybe @Gusted did some work with that and has more experience.

@mfenniak wrote in https://code.forgejo.org/forgejo/runner/pulls/849#issuecomment-53455: > After this fix, is there value in pursuing those, eliminating them, and maybe enabling data race detection during tests? I'd volunteer to try, as long as nobody would advise against it (eg. based upon their experience, if the data race detector isn't a valuable tool or the resource usage is too high to justify, etc.?). I have no experience using this tool and since it caught this one (which was painful to figure out), I'd say there is value in it. Maybe @Gusted did some work with that and has more experience.

earl-warren commented 2025-08-12 09:59:35 +00:00

Author

Contributor

Copy link

@mfenniak wrote in #849 (comment):

eg.

go test -v -race ./act/runner/... --run TestRunner_RunEvent/evalmatrix 2>&1 | tee runner-tests-2.log

I'm afraid to look... very cowardly of me.

@mfenniak wrote in https://code.forgejo.org/forgejo/runner/pulls/849#issuecomment-53455: > eg. > > ```text > go test -v -race ./act/runner/... --run TestRunner_RunEvent/evalmatrix 2>&1 | tee runner-tests-2.log > ``` I'm afraid to look... very cowardly of me.

earl-warren commented 2025-08-12 10:08:17 +00:00

Author

Contributor

Copy link

@Gusted wrote in #849 (comment):

It seems
act/runner/runner.go
Line 74 in 555b322
ValidVolumes []string // only volumes (and bind mounts) in this slice can be mounted on the job container or service containers

can be removed as well?

It is set from the runner configuration value [container].valid_volumes

https://forgejo.org/docs/next/admin/actions/runner-installation/#configuration

internal/app/run/runner.go

Line 331 in 555b322

ValidVolumes: r.cfg.Container.ValidVolumes,

@Gusted wrote in https://code.forgejo.org/forgejo/runner/pulls/849#issuecomment-53427: > It seems > [act/runner/runner.go](https://code.forgejo.org/forgejo/runner/src/commit/555b322ce5922942e846c04a62925e3c09db89b7/act/runner/runner.go#L74) > Line 74 in [555b322](https://code.forgejo.org/forgejo/runner/src/commit/555b322ce5922942e846c04a62925e3c09db89b7) > ` ValidVolumes []string // only volumes (and bind mounts) in this slice can be mounted on the job container or service containers ` > > can be removed as well? It is set from the runner configuration value `[container].valid_volumes` https://forgejo.org/docs/next/admin/actions/runner-installation/#configuration https://code.forgejo.org/forgejo/runner/src/commit/555b322ce5922942e846c04a62925e3c09db89b7/internal/app/run/runner.go#L331

earl-warren merged commit 41f8b03b79 into main 2025-08-12 10:09:43 +00:00

earl-warren deleted branch wip-valid-volumes 2025-08-12 10:09:43 +00:00

earl-warren referenced this pull request from a commit 2025-08-12 10:09:44 +00:00

fix: the configuration must not be used as temporary storage (#849)

earl-warren referenced this pull request 2025-08-12 10:10:12 +00:00

bug: failed to copy content to container (transient test error) #848

Gusted commented 2025-08-14 08:13:43 +00:00

Owner

Copy link

@earl-warren wrote in #849 (comment):

I have no experience using this tool and since it caught this one (which was painful to figure out), I'd say there is value in it. Maybe @Gusted did some work with that and has more experience.

The race detector can be quite useful, but it's proportional to the amount of tests that make sure concurrent work is being done.

@earl-warren wrote in https://code.forgejo.org/forgejo/runner/pulls/849#issuecomment-53512: > I have no experience using this tool and since it caught this one (which was painful to figure out), I'd say there is value in it. Maybe @Gusted did some work with that and has more experience. The race detector can be quite useful, but it's proportional to the amount of tests that make sure concurrent work is being done.

viceice-bot referenced this pull request 2025-08-18 00:10:15 +00:00

Update code.forgejo.org/forgejo/runner Docker tag to v9.1.0 #877

viceice-bot referenced this pull request from actions/setup-forgejo 2025-08-18 00:10:33 +00:00

Update dependency forgejo/runner to v9.1.1 #622

viceice-bot referenced this pull request 2025-08-18 00:31:54 +00:00

Update code.forgejo.org/forgejo/runner Docker tag to v9.1.0 #877

viceice-bot referenced this pull request 2025-08-18 01:02:08 +00:00

Update code.forgejo.org/forgejo/runner Docker tag to v9.1.0 #877

viceice-bot referenced this pull request 2025-08-18 01:31:08 +00:00

Update code.forgejo.org/forgejo/runner Docker tag to v9.1.0 #877

viceice-bot referenced this pull request 2025-08-18 02:01:42 +00:00

Update code.forgejo.org/forgejo/runner Docker tag to v9.1.0 #877

viceice-bot referenced this pull request 2025-08-18 02:31:02 +00:00

Update code.forgejo.org/forgejo/runner Docker tag to v9.1.0 #877

viceice-bot referenced this pull request 2025-08-18 03:01:42 +00:00

Update code.forgejo.org/forgejo/runner Docker tag to v9.1.0 #877

viceice-bot referenced this pull request 2025-08-18 03:31:00 +00:00

Update code.forgejo.org/forgejo/runner Docker tag to v9.1.0 #877

viceice-bot referenced this pull request 2025-08-18 04:01:39 +00:00

Update code.forgejo.org/forgejo/runner Docker tag to v9.1.0 #877

viceice-bot referenced this pull request 2025-08-18 04:31:00 +00:00

Update code.forgejo.org/forgejo/runner Docker tag to v9.1.0 #877

viceice-bot referenced this pull request 2025-08-18 05:02:28 +00:00

Update dependency forgejo/runner to v9.1.0 #879

viceice-bot referenced this pull request 2025-08-18 05:31:15 +00:00

Update dependency forgejo/runner to v9.1.0 #879

earl-warren referenced this pull request from a commit 2025-08-28 17:55:28 +00:00

Update dependency forgejo/runner to v9.1.1 (#622)

Sign in to join this conversation.

Reviewers

No reviewers

mfenniak

viceice

Labels

Clear labels   

FreeBSD

  

Kind/Breaking

Breaking change that won't be backward compatible

  

Kind/Bug

Something is not working

  

Kind/Chore

It is no fun but it needs to be done, this is a chore.

  

Kind/DependencyUpdate

for pull requests that are created by renovate when updating a dependency

  

Kind/Documentation

Documentation changes

  

Kind/Enhancement

Improve existing functionality

  

Kind/Feature

New functionality

  

Kind/Security

This is security issue

  

Kind/Testing

Issue or pull request related to testing

  

Priority

Critical

The priority is critical

  

Priority

High

The priority is high

  

Priority

Low

The priority is low

  

Priority

Medium

The priority is medium

  

Reviewed

Confirmed

Issue has been confirmed

  

Reviewed

Duplicate

This issue or pull request already exists

  

Reviewed

Invalid

Invalid issue

  

Reviewed

Won't Fix

This issue won't be fixed

  

Status

Abandoned

Somebody has started to work on this but abandoned work

  

Status

Blocked

Something is blocking this issue or pull request

  

Status

Need More Info

Feedback is required to reproduce issue or to continue work

  

Windows

Windows ports is an independent project, see https://github.com/Crown0815/Forgejo-runner-windows-builder

  

linux-powerpc64le

powerpc64le

  

linux-riscv64

  

linux-s390x

  

run-end-to-end-tests

Run https://code.forgejo.org/forgejo/end-to-end with a runner compiled from this pull request

  

run-forgejo-tests

Run https://codeberg.org/forgejo/forgejo tests importing the runner from this pull request

No labels

FreeBSD

Kind/Breaking

Kind/Bug

Kind/Chore

Kind/DependencyUpdate

Kind/Documentation

Kind/Enhancement

Kind/Feature

Kind/Security

Kind/Testing

Priority

Critical

Priority

High

Priority

Low

Priority

Medium

Reviewed

Confirmed

Reviewed

Duplicate

Reviewed

Invalid

Reviewed

Won't Fix

Status

Abandoned

Status

Blocked

Status

Need More Info

Windows

linux-powerpc64le

linux-riscv64

linux-s390x

run-end-to-end-tests

run-forgejo-tests

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

earl-warren

5 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

forgejo/runner!849

No description provided.